Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Granular settings #1516

Merged
merged 57 commits into from
Nov 15, 2017
Merged

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Oct 23, 2017

Required PRs

Description

This PR is very much inspired from @t3chguy's explanations of the matter. This implementation hides a lot of the actual getting and setting code to expose a single common interface where a couple lines of code can be used to update the different levels of a setting.

Granular settings have a human-readable level associated with them to best describe the area of impact. These levels are:

  • device - only affects the current device
  • room-device - only affects the target room on the current device
  • room-account - only affects the target room, for the current user
  • account - only affects the current user
  • room - affects all members of the target room
  • config - affects all users of the application, defined by config.json
  • default - affects everyone, defined by matrix-react-sdk
    The farther you travel down the list of levels, the more and more generic you get. This implementation aims to return early if it finds a value at a given level to effectively 'override' the remaining levels.

Supported settings

● denotes 'supported but no ui/ux will be part of this PR'
✔ denotes 'supported and ui/ux will be part of this PR'
✓ indicates that the setting has been implemented in this PR
default is always supported.

Description per-device per-room per-device per-room per-account per-account per-room config default
✓ Hide redactions
✓ URL previews
✓ Autoplay gifs/videos
✓ Hide read receipts
✓ Don't send typing notifications
✓ Always show timestamps
✓ 12h timestamps
✓ Hide join/leave messages
✓ Hide avatar/display name change messages
✓ Compact timeline layout
✓ Automatic syntax language detection
✓ Automatically replace plaintext emoji
✓ Disable emoji suggestions while typing
✓ Disable big emoji
✓ Disable avatars in pills
✓ Interface language
✓ Theme
✓ Autocomplete delay
✓ Disable P2P for 1:1 calls
✓ Microphone
✓ Webcam
✓ Notifications enabled
✓ Notification bodies enabled
✓ Audio notifications enabled
✓ Never send encrypted messages to unverified devices
✓ Analytics opt-out
✓ Room colour
✓ Labs: Groups
✓ Labs: Pinning
MessageComposerInput.isRichTextEnabled
MessageComposer.showFormatting
✓ Flip local video feed

Note: Color scheming is handled in #1479

This PR addresses:

General TODO items for this PR:

  • Local echo on settings
  • Use SettingsCheckbox all over UserSettings and RoomSettings
  • Migration for blacklistUnverifiedDevices
  • Support hierarchy for blacklistUnverifiedDevices
  • Fix "Never send to unverified devices" option does not persist page reloads element-hq/element-web#5516
  • Support default_theme in config.json (matthew/status branch - just bring in the setting)
  • Fix URL previews in rooms not appearing to save (getLevelAt specificity)
  • Fix automatic language detection
  • Developer documentation on how to use this thing
  • Make ColorSettings use this
  • Enum for levels
  • Make canSetValue check the correct event type (ie: url previews)
  • Migrate notification stuff from UserSettingsStore
  • Validate that setting names exist (instead of returning null)
  • Make 'default' required to indicate type.

@t3chguy
Copy link
Member

t3chguy commented Oct 23, 2017

I think webcam and microphone should just remain local (device) variables, syncing them is pointless as the chance that they'd match between two devices is super slim

@t3chguy
Copy link
Member

t3chguy commented Oct 23, 2017

I believe the design originally also had room-device

@t3chguy
Copy link
Member

t3chguy commented Oct 23, 2017

Hiding redactions (element-hq/element-web#3589) - room setting (without room)

I'd disagree on without room because for a todo-list style room it'd be ideal for redactions to be hidden

@turt2live
Copy link
Member Author

@t3chguy webcam and microphone should indeed be device only, I just mislabeled them (fixed).

What settings would room-device be used for? As much as this is currently optimistic code, I'm hesitant to add too much excess.

The without room on hiding redactions is only to prevent room administrators from setting the room default for it. Users would still be able to hide redactions for themselves in particular rooms.

@ara4n
Copy link
Member

ara4n commented Oct 23, 2017

This is looking super-promising. I haven't had a chance to look at the code, but I assume the end result is an API which just provides a get/setSetting which magically abstracts away the various different levels of granularity?

As you've pointed out, the vast majority of settings only make sense at a given level of granularity (typically per-device or per-account). However, I'm failing to grok the table you've written here - I wonder if it's worth spelling it out as an actual table so we can check we're all on the same page? Something like:

feature per-device per-room-per-account per-account per-room global-default
hide URL previews
room colour

perhaps?

@t3chguy
Copy link
Member

t3chguy commented Oct 23, 2017

@turt2live url previews for instance

@freelock
Copy link

How do groups factor into this? I would hope that certain groups can contain defaults that apply to all of its rooms, which might be overrideable per room (perhaps with a setting to control what level may override group defaults...)

Perhaps between room and default?

@turt2live
Copy link
Member Author

For the moment, groups are completely ignored for this PR intentionally. Groups don't have a concept of a permissions system, so can't be implemented. When groups have a permissions system, they'll be easily implemented into this system.

@turt2live
Copy link
Member Author

Thinking a bit more about groups: Their factorability is directly affected by how the permissions system gets implemented. If they have an arbitrary value store for users (matrix-org/synapse#2569) and another for the group itself (matrix-org/synapse#2570), then it would be fairly easy to add support for the tiers. I'd imagine group overrides would squeeze in between per-room and account (effectively making the chain room > group > account).

@ara4n
Copy link
Member

ara4n commented Oct 23, 2017

the grid is excellent - thank you! I only spot one weirdness when doing a sanitycheck:

  • Shouldn't room colour be per-room-per-account as well as 'default'? (not that default even works atm, although I'm about to make it work in a theming branch)

@turt2live
Copy link
Member Author

Oops! Missed that one. All settings are able to have their default be undefined, which is why all settings support it. default is the only special case where a falsey value is considered valid for a setting, so in the case of room colour, the default would be null (or whatever your theming branch ends up saying it is).

@ara4n
Copy link
Member

ara4n commented Oct 26, 2017

(random thought: there is a lot of overlap between granular settings and the nightmare that is push notification settings - i.e. "adjust volume per room, per device-per-room, globally, etc". Given that notif settings have a whole 'nother level of fractaline complexity (per origin user, per content, etc) i think that having a proper push notif API is certainly the right approach to have taken, but just observing the parallels).

GranularSettingStore is a class to manage settings of varying granularity, such as URL previews at the device, room, and account levels.

Signed-off-by: Travis Ralston <[email protected]>
Signed-off-by: Travis Ralston <[email protected]>
This does away with the room- and account-style settings, and just replaces them with `supportedLevels`. The handlers have also been moved out to be in better support of the other options, like SdkConfig and per-room-per-device.

Signed-off-by: Travis Ralston <[email protected]>
@turt2live turt2live force-pushed the travis/granular-settings branch from 83d8733 to 989bdcf Compare October 29, 2017 01:13
Signed-off-by: Travis Ralston <[email protected]>
This breaks language selection.

Signed-off-by: Travis Ralston <[email protected]>
This currently causes a split-brain scenario for the application due to the priority of each level. Granular settings assumes a simple override, however the crypto setting wants per room to be overriden with the global setting, regardless of the room setting. Some additional comments are needed on the intended behaviour.

Signed-off-by: Travis Ralston <[email protected]>
Signed-off-by: Travis Ralston <[email protected]>
Signed-off-by: Travis Ralston <[email protected]>
Signed-off-by: Travis Ralston <[email protected]>
Signed-off-by: Travis Ralston <[email protected]>
Signed-off-by: Travis Ralston <[email protected]>
@turt2live turt2live changed the title [WIP] Granular settings Granular settings Nov 5, 2017
@ara4n
Copy link
Member

ara4n commented Nov 8, 2017

So i've just worked through the whole enchilada again, courtesy of UA900's lovely in-flight wifi. The only stuff which jumps out at me is:

  • Are we sure that the per-device setting for blacklistUnverifiedDevices should trump the per-device-per-room setting? Even if I've said "this phone should never send to untrusted users", it feels really weird i can't chose to override that on some given rooms. (Sidenote is that the blacklist unverified device feature feels broken in general; it just creates UISIs everywhere, and the UnknownDeviceDialog stuff stops us sending messages to untrusted devices anyway. Perhaps we should just ditch the feature, although it's still an interesting thought experiment from the perspective of granular settings, and to check that we have the necessary flexibility in the API)
  • I'm slightly surprised at notificationToolbar becoming a setting.
    • If a given option is something is clearly never ever ever going to be granular, or exposed in UserSettings etc, do we really want it in SettingsStore?

Otherwise it really looks great to me, and I am very tempted to 'merge early, merge often' so that we can test it in the upcoming groups RC...

@turt2live
Copy link
Member Author

@ara4n I've gone ahead and made blacklistUnverifiedDevices have a different level order than the other settings to achieve the behaviour desired. This means this PR now also requires matrix-org/matrix-js-sdk#568 (linked in OP too).

I've also dropped the notification toolbar setting. That isn't a setting, and shouldn't be in here.

@ara4n
Copy link
Member

ara4n commented Nov 15, 2017

tests pass locally so merging \o/ :)

@ara4n ara4n merged commit 060a890 into matrix-org:develop Nov 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants