Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

change realtime preference position #3398

Merged
merged 11 commits into from
Oct 31, 2024
Merged

change realtime preference position #3398

merged 11 commits into from
Oct 31, 2024

Conversation

adbenitez
Copy link
Member

No description provided.

@adbenitez adbenitez added the enhancement actually in development, user visible enhancement label Oct 29, 2024
@adbenitez adbenitez self-assigned this Oct 29, 2024
@github-actions GitHub Actions
Copy link

To test the changes in this pull request, install this apk:
📦 app-preview.apk

@adbenitez adbenitez requested review from r10s and Hocuri October 29, 2024 18:30
r10s
r10s approved these changes Oct 29, 2024
View reviewed changes
Copy link
Member

@r10s r10s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would adapt the string as the advice "enable to ..." is misleading as it will be already enabled with deltachat/deltachat-core-rust#6125

otherwise, lgtm

src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@adbenitez
Copy link
Member Author

feel free to suggest new strings for the explanation, I would like it to transmit the idea "some apps might not work properly if you disable this" since it is more damage than helpful if user disable it just because it seems like an useless feature that just "make apps faster" or "reveals my IP why would I want that?"

@adbenitez adbenitez added the wait-for-core Issue/PR is waiting for core release label Oct 29, 2024
@adbenitez @r10s
Update src/main/res/values/strings.xml
d2d6fff 
Co-authored-by: bjoern <r10s@b44t.com>
@github-actions GitHub Actions
Copy link

To test the changes in this pull request, install this apk:
📦 app-preview.apk

@r10s r10s mentioned this pull request Oct 30, 2024
Merged
@github-actions GitHub Actions
Copy link

To test the changes in this pull request, install this apk:
📦 app-preview.apk

@Hocuri Hocuri mentioned this pull request Oct 30, 2024
Closed
@adbenitez
Copy link
Member Author

adbenitez commented Oct 30, 2024 •
edited
Loading

I added a dialog if real-time is disabled and the app needs it, to give a chance user can quickly fix problems if they disabled the setting without properly understanding scared by the "leaks IP"

@r10s, @Hocuri, @hpk42 improvements to the dialog message welcome

@github-actions GitHub Actions
Copy link

To test the changes in this pull request, install this apk:
📦 app-preview.apk

@github-actions GitHub Actions
Copy link

To test the changes in this pull request, install this apk:
📦 app-preview.apk

Hocuri
Hocuri reviewed Oct 30, 2024
View reviewed changes
Copy link
Collaborator

@Hocuri Hocuri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just did a quick user test on this (we really ought to do those more often! though I was lucky to be around some non-technical users).

Both my users thought that "real-time support" is a chatbot that supports you in real-time😅

src/main/res/values/strings.xml Outdated Show resolved Hide resolved
src/main/res/values/strings.xml Outdated Show resolved Hide resolved
src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@link2xt link2xt mentioned this pull request Oct 30, 2024
Merged
@github-actions GitHub Actions
Copy link

To test the changes in this pull request, install this apk:
📦 app-preview.apk

@github-actions GitHub Actions
Copy link

To test the changes in this pull request, install this apk:
📦 app-preview.apk

@r10s
Copy link
Member

r10s commented Oct 31, 2024 •
edited
Loading

ftr, if we want to come back to a dialog later on, we should remember this commit: d986e83
(it was taken out of this PR as it may interfere with apps dealing nicely with realtime not available. and also may annoy users by nudging into re-enabling, which we maybe do not want. esp. for apps that work with/without realtime as written in the spec. we can reconsider later, if we have more real-life-experience and there are really non-testing-only realtime apps out)

@adbenitez
Copy link
Member Author

adbenitez commented Oct 31, 2024 •
edited
Loading

ftr, there is my app Live Chat that is not for testing only and fully depends on real-time,

besides the release situation I don't agree with the statement that not having the dialog is better than having it, but I plan to add a "don't ask again" checkbox to the dialog to solve the downside for the MINORITY that would find this setting in advance and disable it without wanting to ever enable it again

@adbenitez
Copy link
Member Author

also notice that apps can make the real-time optional in their settings and then control it inside the app without causing the dialog to prompt every time since it is only shown if the app tries to use realtime,

alternatively, the API could be improved so apps realize realtime is disabled and don't try to use it, currently apps are totally blind trying to use realtime without realizing it is not available, it is good enough for apps that don't need it but that is not all apps

@r10s
Copy link
Member

r10s commented Oct 31, 2024

i am not against a dialog, there are a lot of "may" in my "ftr" :)

@r10s r10s removed the wait-for-core Issue/PR is waiting for core release label Oct 31, 2024
@r10s r10s merged commit 72ae90a into main Oct 31, 2024
2 checks passed
@r10s r10s deleted the adb/move-realtime branch October 31, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hocuri Hocuri Hocuri approved these changes

@hpk42 hpk42 hpk42 left review comments

@r10s r10s r10s approved these changes

Assignees

@adbenitez adbenitez

Labels
enhancement actually in development, user visible enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@adbenitez @r10s @hpk42 @Hocuri