Be more positive with setting labels #2504
Conversation
Fixes element-hq/element-web#6435 This is done through an on-the-fly inverter for the settings. All the settings changed are boolean values, so this should be more than safe to just let happen throughout the SettingsStore. Typically a change like this would be done in the individual handlers (similar to how setting names are remapped to different properties or even different storage locations on the fly), however doing that for this many settings would be a huge nightmare and involve changing *all* the layers. By putting a global "invert this" flag on the setting, we can get away with doing the inversion as the last possible step during a read (or write). To speed up calculations of the default values, we cache all the inverted values into a lookup table similar to how we represent the defaults already. Without this, the DefaultHandler would need to iterate the setting list and invert the values, slowing things down over time. We invert the value up front so we can keep the generic inversion logic without checking the level ahead of time. It is fully intended that a default value represents the new setting name, not the legacy name. This commit also includes a debugger for settings because it was hard to visualize what the SettingsStore was doing during development. Some added information is included as it may be helpful for when someone has a problem with their settings and we need to debug it. Typically the debugger would be run in conjunction with `mxSendRageshake`: `mxSettingsStore.debugSetting('showJoinLeaves') && mxSendRageshake('Debugging showJoinLeaves setting')`.
There was a problem hiding this comment.
Looks ok, but I'm wondering whether instead of just proxying the new settings through to the old ones, if migrating the inverted settings all in one go at startup would be beneficial. That way we could get rid of the inverting logic once we can assume everybody moved over to the new settings format... maybe not worth the effort though... wdyt?
src/settings/SettingsStore.js
Outdated
| return SettingsStore._tryControllerOverride(settingName, level, roomId, value, level); | ||
| if (!handler) { | ||
| let value = SettingsStore._tryControllerOverride(setting, level, roomId, null, null); | ||
| if (inverted) value = !value; |
There was a problem hiding this comment.
Hmm, slightly concerned about this important line being duplicated 4x in the code below...
This has the problem of breaking people's ability to downgrade or switch between versions of riot. Most of the settings are cosmetic things, however some are privacy related. I'd be very uncomfortable with risking people not realizing what happened. |
Good point |
|
@bwindels please take another look - I've renamed |
|
This PR gives me great joy :D |
Fixes element-hq/element-web#6435
This is done through an on-the-fly inverter for the settings. All the settings changed are boolean values, so this should be more than safe to just let happen throughout the SettingsStore. Typically a change like this would be done in the individual handlers (similar to how setting names are remapped to different properties or even different storage locations on the fly), however doing that for this many settings would be a huge nightmare and involve changing all the layers. By putting a global "invert this" flag on the setting, we can get away with doing the inversion as the last possible step during a read (or write).
To speed up calculations of the default values, we cache all the inverted values into a lookup table similar to how we represent the defaults already. Without this, the DefaultHandler would need to iterate the setting list and invert the values, slowing things down over time. We invert the value up front so we can keep the generic inversion logic without checking the level ahead of time. It is fully intended that a default value represents the new setting name, not the legacy name.
This commit also includes a debugger for settings because it was hard to visualize what the SettingsStore was doing during development. Some added information is included as it may be helpful for when someone has a problem with their settings and we need to debug it. Typically the debugger would be run in conjunction with
mxSendRageshake:mxSettingsStore.debugSetting('showJoinLeaves') && mxSendRageshake('Debugging showJoinLeaves setting').