allow to enable/disable notifications per-account #3143
Conversation
|
currently ui.muted config is used, we need a proper core config for this |
|
To test the changes in this pull request, install this apk: |
|
To test the changes in this pull request, install this apk: |
There was a problem hiding this comment.
android part lgtm. but let's wait for core:
i agree, we should use a core settings instead of ui.muted - we then have the option to do $things on the option, eg. not doing FCM (which may or may not be wanted, no need to decide that now).
also, it is in sync with muted-chats, which are also handled in core - that way, we have the option to not fire fresh-events and do the notification-logic more in core (again, this may or may not be wanted, no need to decide that now, it is just about the option)
|
i created a PR offering btw, i think we should migrate the old setting, so that we do not start adding notifications the user has disabled before. we could do that similar to "migrating chat backgrounds" (which can be removed then) |
|
To test the changes in this pull request, install this apk: |
adbenitez
force-pushed
the
adb/issue-3126
branch
from
9fa7ba1
to
93a8e3b
Compare
|
To test the changes in this pull request, install this apk: |
There was a problem hiding this comment.
the migration seems buggy, i could also confirm that in a test.
apart from that and the log, things lgtm.
when these two things are fixed, this PR should maybe better be squashed - i assume this will re-create a commit that not confuses git, sth. is broken in the git history still on some system
| } | ||
| Prefs.setStringPreference(this, "pref_chat_background", ""); | ||
| Prefs.removePreference(this, NOTIFICATION_PREF); |
There was a problem hiding this comment.
as SwitchPreferenceCompat (resp. its base classes) persist changes in "shared preferences" under the name pref_key_enable_notifications as well, the migration is executed whenever the user muted notifications for any account.
on the next app start, all accounts get muted then.
i do not know if we can prevent SwitchPreferenceCompat from persisting things, an easy fix would be to use a different name as pref_enable_notifications (strike the "key")
There was a problem hiding this comment.
good catch! I renamed it as per your suggestion
There was a problem hiding this comment.
the references need to be changed as well (i'd do a grep over .xml and .java and rename everything but the migration)
There was a problem hiding this comment.
I fixed it, I wonder if it is possible to move such keys to some xml file and then don't have this problem, it is easy to overlook and the keys are manually typed in both sides
There was a problem hiding this comment.
for me, there are still dependencies in preferences_notifications.xml as android:dependency="pref_key_enable_notifications" , this leads to crashes on opening the page.
but maybe a commit is missing again due to git hickup ...
There was a problem hiding this comment.
sorry, fixed it and compiled locally this time before pushing it (was thinking it was a quick simple single-line renaming fix and didn't test last times)
Co-authored-by: bjoern <r10s@b44t.com>
|
To test the changes in this pull request, install this apk: |
|
To test the changes in this pull request, install this apk: |
|
To test the changes in this pull request, install this apk: |
see discussion at deltachat/deltachat-android#3143 (review) , there was the idea to show them as 'muted', however, we tried the simple solution first, which make sense as well and does not have the issue with the global counter on the badge icon (where we could not use different colors)
see discussion at deltachat/deltachat-android#3143 (review) , there was the idea to show them as 'muted', however, we tried the simple solution first, which make sense as well and does not have the issue with the global counter on the badge icon (where we could not use different colors)
close #3126