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.

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

Update notification settings for intentional mentions #25434

Closed
kerryarchibald opened this issue May 24, 2023 · 8 comments
Closed

Update notification settings for intentional mentions #25434

kerryarchibald opened this issue May 24, 2023 · 8 comments

Comments

@kerryarchibald
Copy link
Contributor

kerryarchibald commented May 24, 2023

Your use case

For #25267

Update notification setting bindings to use new rules from MSC3952 in mentions rules where server support exists.

AC

  • When intentional mentions is not supported (<v1.7) no change to push rules.
  • When intentional mentions is supported (>=v1.7), mentions notifications settings should toggle both new MSC3952 rules and older deprecated .m.rule.contains_display_name, .m.rule.contains_user_name, and .m.rule.roomnotif
    MSC3952 rules should be the master rule, with deprecated rules kept in sync. (this can be achieved by adding syncedRuleIds in push rule definitions)*
  • Synced rules are not forced into sync, if rules are set out-of-sync by another client, EW should not change their values until the user explicitly syncs or changes the setting. (syncedRuleIds behaviour will need to be updated, see monitorSyncedPushRules)
  • When rules are out of sync notify the user in notification settings with a message attached to the relevant rule
Screenshot 2023-05-25 at 15 31 39 - The rule is displayed at the 'loudest' value of any of the synced rules. If `.m.rule.roomnotif` is set to On, and `.m.rule.is_room_mention` is set to Noisy - the room mentions settings should display 'Noisy' with a notice about out of sync rules.
description master rule syncedRules
Notify when someone mentions using @room .m.rule.is_room_mention .m.rule.roomnotif
Notify when someone mentions using @DisplayName or @mxid .m.rule.is_user_mention .m.rule.contains_display_name, .m.rule.contains_user_name

Have you considered any alternatives?

No response

Additional context

Existing UI:
Screenshot 2023-05-24 at 14 12 31

New settings UI uses different language for mention settings:
Screenshot 2023-05-24 at 14 15 35
#24567

@t3chguy
Copy link
Member

t3chguy commented May 24, 2023

EW checks push rules on launch and changes to m.push_rules and updates any out of sync rules.

This sounds like it has the potential to continually undo users' actions from different clients? Sure they'd be wanting a weird combination of rules but inherently this feels wrong

@kerryarchibald
Copy link
Contributor Author

@t3chguy Now that I think about it, I'm not sure when intentional mentions support is coming to mobile so even within Element's ecosystem this would break mentions settings.

@kerryarchibald
Copy link
Contributor Author

Updated the AC with:

  • Synced rules are not forced into sync, if rules are set out-of-sync by another client, EW should not change their values until the user explicitly syncs or changes the setting. (syncedRuleIds behaviour will need to be updated, see monitorSyncedPushRules)
  • When rules are out of sync notify the user in notification settings with a message attached to the relevant rule
    eg:
Screenshot 2023-05-25 at 15 31 39

@daniellekirkwood what do you think?
The situation is:

  1. I turn on @room mentions in EW, this sets .m.rule.is_room_mention and .m.rule.roomnotif to ON
  2. On my phone running old Element ios I turn @room mentions off, this only sets .m.rule.roomnotif to OFF
  3. Open notification settings on EW, how should the @room mention setting toggle display?

@giomfo
Copy link
Member

giomfo commented May 26, 2023

@kerryarchibald @daniellekirkwood From my point of view, EW should display ON the @room mention toggle as soon as one of the 2 push rules is ON. Because the user will observe some @room mention on EW then (only a part of them if one of the rule is OFF)

@daniellekirkwood
Copy link
Contributor

Synced rules are not forced into sync, if rules are set out-of-sync by another client, EW should not change their values until the user explicitly syncs or changes the setting.

Agree

When rules are out of sync notify the user in notification settings with a message attached to the relevant rule

this will need design input - i think we were thinking of having a banner at the top of the page... but also having an 'error' message next to the switch works also. I'm happy to commit to the suggestion here and raise an 'enhancement' to update the design or copy if needed in the future.

"Open notification settings on EW, how should the @room mention setting toggle display?"

If the mobile legacy clients integrate with the MSC then this would not be a problem? If yes, then the other clients that need to catch up with this MSC will also be in this state until they add support for it. While we're in this middle-phase I think I'd rather have too many notifications and err on the side of caution here so if one of the push rules is set to ON then EW shows ON but with the warning message pictured there... ? Have I understood the question correctly, WDYT?

@kerryarchibald
Copy link
Contributor Author

Given the extent of Janne's changes to the Notifications screen, I think it would be cleanest to wait for that to land/stabilise a bit more before implementing this.

@Johennes
Copy link
Contributor

Spoke to @justjanne and it looks like she already included this work in her ongoing branch

@justjanne
Copy link
Contributor

The UI sets the intentional mentions rules correctly and as expected in my tests with our beta server and an element-web build based off the latest develop. I'll close this ticket as completed, if you find issues with this please file them separately (though feel free to link this issue to them)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants