Reorganise Account Notification Settings #3673
Conversation
…ragment to new 3 new fragments and inherit common function
…ragment to new 3 new fragments and inherit common function
…eature/dla/reorg_account_notification_settings
| import androidx.preference.CheckBoxPreference | ||
| import androidx.preference.PreferenceViewHolder | ||
|
|
||
| open class VectorCheckboxPreference : CheckBoxPreference { |
There was a problem hiding this comment.
This class does not have to be open. It does not have to be sealed as well, but let's keep the default Kotlin behavior (so seal) unless we have to extend the class.
| override val preferenceXmlRes = R.xml.vector_settings_notification_global | ||
|
|
||
| override val prefKeyToPushRuleId: Map<String, String> | ||
| get() = mapOf( |
There was a problem hiding this comment.
Could be simplified with override val prefKeyToPushRuleId = mapOf(
| get() = mapOf( | ||
| "SETTINGS_PUSH_RULE_INVITED_TO_ROOM_PREFERENCE_KEY" to RuleIds.RULE_ID_INVITE_ME, | ||
| "SETTINGS_PUSH_RULE_CALL_INVITATIONS_PREFERENCE_KEY" to RuleIds.RULE_ID_CALL, | ||
| "SETTINGS_PUSH_RULE_MESSAGES_SENT_BY_BOT_PREFERENCE_KEY" to RuleIds.RULE_ID_SUPPRESS_BOTS_NOTIFICATIONS, |
There was a problem hiding this comment.
I'm scared about the suppress wording here. So it mean that the check box has the opposite behavior? I know it's in the spec... EDIT: the UI seems to uncheck the box by default, so I guess it's working correctly.
| /** | ||
| * @return the bing rule status boolean | ||
| */ | ||
| fun ruleStatusIndexFor(ruleAndKind: PushRuleAndKind): Boolean { |
There was a problem hiding this comment.
Please make all the fun private where it's possible, default visibility is public in Kotlin
| <string name="settings_messages_at_room">Messages containing @room</string> | ||
| <string name="settings_messages_in_e2e_one_to_one">Encrypted messages in one-to-one chats</string> | ||
| <string name="settings_messages_in_e2e_group_chat">Encrypted messages in group chats</string> | ||
| <string name="settings_when_rooms_are_upgraded">When rooms are upgraded</string> |
There was a problem hiding this comment.
Lint will complain that we have translation but not original string. So either remove all the existing translations or keep the original string and we do the cleanup in the strings from time to time.
I would go for the first option.
There was a problem hiding this comment.
My bad, actually those strings has just been moved above. Please ignore my comment
| <?xml version="1.0" encoding="utf-8"?> | ||
| <androidx.preference.PreferenceScreen xmlns:android="http://schemas.android.com/apk/res/android"> | ||
| <im.vector.app.core.preference.VectorPreferenceCategory | ||
| android:key="SETTINGS_OTHER" |
There was a problem hiding this comment.
This is never used, but this is not the correct key (copy/paste error).
…eature/dla/reorg_account_notification_settings
| import org.matrix.android.sdk.api.pushrules.toJson | ||
|
|
||
| enum class NotificationIndex(val index: Int) { | ||
| OFF(0), |
There was a problem hiding this comment.
Enum already have indexes by default starting from 0
There was a problem hiding this comment.
Plus I didn't actually end up using the index value 🥴. Removed, thanks.
Implements feature #3646
screenshots
Account Settings > Notifications
Default Notifications
Mentions and Keywords
Other
notes
VectorSettingsBaseFragment,VectorPreferenceVectorPreferenceCategory, etc). Changing the architecture toViewModelsetc I think would be out of the scope of this PR.VectorSwitchPreferenceto replacePushRulePreferenceand moved all business logic to the fragment level.Global NotificationstoDefault Notificationson design's advice