From cb6f415a051d38c5555fd143c88a0e8781485ebd Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 24 Jan 2019 20:57:40 -0700 Subject: [PATCH] Be more positive with setting labels Fixes https://github.com/vector-im/riot-web/issues/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')`. --- src/MatrixClientPeg.js | 2 +- src/autocomplete/EmojiProvider.js | 2 +- src/components/structures/BottomLeftMenu.js | 2 +- src/components/structures/LeftPanel.js | 2 +- src/components/structures/RoomView.js | 2 +- src/components/structures/UserSettings.js | 24 +-- src/components/views/messages/TextualBody.js | 4 +- .../views/rooms/MessageComposerInput.js | 4 +- .../settings/tabs/PreferencesSettingsTab.js | 20 +-- .../views/settings/tabs/VoiceSettingsTab.js | 7 +- src/i18n/strings/en_EN.json | 20 +-- src/settings/Settings.js | 84 +++++---- src/settings/SettingsStore.js | 160 ++++++++++++++++-- .../handlers/DefaultSettingsHandler.js | 10 +- src/shouldHideEvent.js | 8 +- 15 files changed, 255 insertions(+), 96 deletions(-) diff --git a/src/MatrixClientPeg.js b/src/MatrixClientPeg.js index 9a77901d2e..16ae22a4d9 100644 --- a/src/MatrixClientPeg.js +++ b/src/MatrixClientPeg.js @@ -183,7 +183,7 @@ class MatrixClientPeg { userId: creds.userId, deviceId: creds.deviceId, timelineSupport: true, - forceTURN: SettingsStore.getValue('webRtcForceTURN', false), + forceTURN: !SettingsStore.getValue('webRtcForcePeerToPeer', false), }; this.matrixClient = createMatrixClient(opts, useIndexedDb); diff --git a/src/autocomplete/EmojiProvider.js b/src/autocomplete/EmojiProvider.js index 8c6495101f..704cdbd55d 100644 --- a/src/autocomplete/EmojiProvider.js +++ b/src/autocomplete/EmojiProvider.js @@ -97,7 +97,7 @@ export default class EmojiProvider extends AutocompleteProvider { } async getCompletions(query: string, selection: SelectionRange, force?: boolean): Array { - if (SettingsStore.getValue("MessageComposerInput.dontSuggestEmoji")) { + if (!SettingsStore.getValue("MessageComposerInput.suggestEmoji")) { return []; // don't give any suggestions if the user doesn't want them } diff --git a/src/components/structures/BottomLeftMenu.js b/src/components/structures/BottomLeftMenu.js index ed8b8a00b7..47b30be450 100644 --- a/src/components/structures/BottomLeftMenu.js +++ b/src/components/structures/BottomLeftMenu.js @@ -170,7 +170,7 @@ module.exports = React.createClass({ const SettingsButton = sdk.getComponent('elements.SettingsButton'); const GroupsButton = sdk.getComponent('elements.GroupsButton'); - const groupsButton = SettingsStore.getValue("TagPanel.disableTagPanel") ? + const groupsButton = !SettingsStore.getValue("TagPanel.enableTagPanel") ? : null; return ( diff --git a/src/components/structures/LeftPanel.js b/src/components/structures/LeftPanel.js index ba0e97366e..4bab919413 100644 --- a/src/components/structures/LeftPanel.js +++ b/src/components/structures/LeftPanel.js @@ -187,7 +187,7 @@ const LeftPanel = React.createClass({ const SearchBox = sdk.getComponent('structures.SearchBox'); const CallPreview = sdk.getComponent('voip.CallPreview'); - const tagPanelEnabled = !SettingsStore.getValue("TagPanel.disableTagPanel"); + const tagPanelEnabled = SettingsStore.getValue("TagPanel.enableTagPanel"); const tagPanel = tagPanelEnabled ? :
; const containerClasses = classNames( diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index ea01c9bcae..8ad81a1343 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -1838,7 +1838,7 @@ module.exports = React.createClass({ const messagePanel = (
); diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index ab4e8b74c7..04328e5151 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -267,25 +267,25 @@ "Increase performance by only loading room members on first view": "Increase performance by only loading room members on first view", "Backup of encryption keys to server": "Backup of encryption keys to server", "Render simple counters in room header": "Render simple counters in room header", - "Disable Emoji suggestions while typing": "Disable Emoji suggestions while typing", + "Enable Emoji suggestions while typing": "Enable Emoji suggestions while typing", "Use compact timeline layout": "Use compact timeline layout", - "Hide removed messages": "Hide removed messages", - "Hide join/leave messages (invites/kicks/bans unaffected)": "Hide join/leave messages (invites/kicks/bans unaffected)", - "Hide avatar changes": "Hide avatar changes", - "Hide display name changes": "Hide display name changes", - "Hide read receipts": "Hide read receipts", + "Show a placeholder for removed messages": "Show a placeholder for removed messages", + "Show join/leave messages (invites/kicks/bans unaffected)": "Show join/leave messages (invites/kicks/bans unaffected)", + "Show avatar changes": "Show avatar changes", + "Show display name changes": "Show display name changes", + "Show read receipts": "Show read receipts", "Show timestamps in 12 hour format (e.g. 2:30pm)": "Show timestamps in 12 hour format (e.g. 2:30pm)", "Always show message timestamps": "Always show message timestamps", "Autoplay GIFs and videos": "Autoplay GIFs and videos", "Always show encryption icons": "Always show encryption icons", "Show a reminder to enable Secure Message Recovery in encrypted rooms": "Show a reminder to enable Secure Message Recovery in encrypted rooms", "Enable automatic language detection for syntax highlighting": "Enable automatic language detection for syntax highlighting", - "Hide avatars in user and room mentions": "Hide avatars in user and room mentions", - "Disable big emoji in chat": "Disable big emoji in chat", - "Don't send typing notifications": "Don't send typing notifications", + "Show avatars in user and room mentions": "Show avatars in user and room mentions", + "Enable big emoji in chat": "Enable big emoji in chat", + "Send typing notifications": "Send typing notifications", "Automatically replace plain text Emoji": "Automatically replace plain text Emoji", "Mirror local video feed": "Mirror local video feed", - "Disable Community Filter Panel": "Disable Community Filter Panel", + "Enable Community Filter Panel": "Enable Community Filter Panel", "Disable Peer-to-Peer for 1:1 calls": "Disable Peer-to-Peer for 1:1 calls", "Send analytics data": "Send analytics data", "Never send encrypted messages to unverified devices from this device": "Never send encrypted messages to unverified devices from this device", diff --git a/src/settings/Settings.js b/src/settings/Settings.js index 4871ee92f9..fc878333e3 100644 --- a/src/settings/Settings.js +++ b/src/settings/Settings.js @@ -77,6 +77,15 @@ export const SETTINGS = { // // settings. The first element is treated as "most preferred". The "default" // // level is always appended to the end. // supportedLevelsAreOrdered: false, + // + // // Optional value to invert a boolean setting's value. The string given will + // // be read as the setting's ID instead of the one provided as the key for the + // // setting definition. By setting this, the returned value will automatically + // // be inverted, except for when the default value is returned. Inversion will + // // occur after the controller is asked for an override. This should be used by + // // historical settings which we don't want existing user's values be wiped. Do + // // not use this for new settings. + // invertedSettingName: "my-negative-setting", // }, "feature_pinning": { isFeature: true, @@ -116,40 +125,46 @@ export const SETTINGS = { supportedLevels: LEVELS_FEATURE, default: false, }, - "MessageComposerInput.dontSuggestEmoji": { + "MessageComposerInput.suggestEmoji": { supportedLevels: LEVELS_ACCOUNT_SETTINGS, - displayName: _td('Disable Emoji suggestions while typing'), - default: false, + displayName: _td('Enable Emoji suggestions while typing'), + default: true, + invertedSettingName: 'MessageComposerInput.dontSuggestEmoji', }, "useCompactLayout": { supportedLevels: LEVELS_ACCOUNT_SETTINGS, displayName: _td('Use compact timeline layout'), default: false, }, - "hideRedactions": { + "showRedactions": { supportedLevels: LEVELS_ROOM_SETTINGS_WITH_ROOM, - displayName: _td('Hide removed messages'), - default: false, + displayName: _td('Show a placeholder for removed messages'), + default: true, + invertedSettingName: 'hideRedactions', }, - "hideJoinLeaves": { + "showJoinLeaves": { supportedLevels: LEVELS_ROOM_SETTINGS_WITH_ROOM, - displayName: _td('Hide join/leave messages (invites/kicks/bans unaffected)'), - default: false, + displayName: _td('Show join/leave messages (invites/kicks/bans unaffected)'), + default: true, + invertedSettingName: 'hideJoinLeaves', }, - "hideAvatarChanges": { + "showAvatarChanges": { supportedLevels: LEVELS_ROOM_SETTINGS_WITH_ROOM, - displayName: _td('Hide avatar changes'), - default: false, + displayName: _td('Show avatar changes'), + default: true, + invertedSettingName: 'hideAvatarChanges', }, - "hideDisplaynameChanges": { + "showDisplaynameChanges": { supportedLevels: LEVELS_ROOM_SETTINGS_WITH_ROOM, - displayName: _td('Hide display name changes'), - default: false, + displayName: _td('Show display name changes'), + default: true, + invertedSettingName: 'hideDisplaynameChanges', }, - "hideReadReceipts": { + "showReadReceipts": { supportedLevels: LEVELS_ROOM_SETTINGS, - displayName: _td('Hide read receipts'), - default: false, + displayName: _td('Show read receipts'), + default: true, + invertedSettingName: 'hideReadReceipts', }, "showTwelveHourTimestamps": { supportedLevels: LEVELS_ACCOUNT_SETTINGS, @@ -181,15 +196,17 @@ export const SETTINGS = { displayName: _td('Enable automatic language detection for syntax highlighting'), default: false, }, - "Pill.shouldHidePillAvatar": { + "Pill.shouldShowPillAvatar": { supportedLevels: LEVELS_ACCOUNT_SETTINGS, - displayName: _td('Hide avatars in user and room mentions'), - default: false, + displayName: _td('Show avatars in user and room mentions'), + default: true, + invertedSettingName: 'Pill.shouldHidePillAvatar', }, - "TextualBody.disableBigEmoji": { + "TextualBody.enableBigEmoji": { supportedLevels: LEVELS_ACCOUNT_SETTINGS, - displayName: _td('Disable big emoji in chat'), - default: false, + displayName: _td('Enable big emoji in chat'), + default: true, + invertedSettingName: 'TextualBody.disableBigEmoji', }, "MessageComposerInput.isRichTextEnabled": { supportedLevels: LEVELS_ACCOUNT_SETTINGS, @@ -199,10 +216,11 @@ export const SETTINGS = { supportedLevels: LEVELS_ACCOUNT_SETTINGS, default: false, }, - "dontSendTypingNotifications": { + "sendTypingNotifications": { supportedLevels: LEVELS_ACCOUNT_SETTINGS, - displayName: _td("Don't send typing notifications"), - default: false, + displayName: _td("Send typing notifications"), + default: true, + invertedSettingName: 'dontSendTypingNotifications', }, "MessageComposerInput.autoReplaceEmoji": { supportedLevels: LEVELS_ACCOUNT_SETTINGS, @@ -214,19 +232,21 @@ export const SETTINGS = { displayName: _td('Mirror local video feed'), default: false, }, - "TagPanel.disableTagPanel": { + "TagPanel.enableTagPanel": { supportedLevels: LEVELS_ACCOUNT_SETTINGS, - displayName: _td('Disable Community Filter Panel'), - default: false, + displayName: _td('Enable Community Filter Panel'), + default: true, + invertedSettingName: 'TagPanel.disableTagPanel', }, "theme": { supportedLevels: ['config'], default: "dharma", }, - "webRtcForceTURN": { + "webRtcForcePeerToPeer": { supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td('Disable Peer-to-Peer for 1:1 calls'), - default: false, + default: true, + invertedSettingName: 'webRtcForceTURN', }, "webrtc_audiooutput": { supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS, diff --git a/src/settings/SettingsStore.js b/src/settings/SettingsStore.js index bb3f412a9b..e7d4a679da 100644 --- a/src/settings/SettingsStore.js +++ b/src/settings/SettingsStore.js @@ -43,10 +43,16 @@ export const SettingLevel = { // Convert the settings to easier to manage objects for the handlers const defaultSettings = {}; +const invertedDefaultSettings = {}; const featureNames = []; for (const key of Object.keys(SETTINGS)) { defaultSettings[key] = SETTINGS[key].default; if (SETTINGS[key].isFeature) featureNames.push(key); + if (SETTINGS[key].invertedSettingName) { + // Invert now so that the rest of the system will invert it back + // to what was intended. + invertedDefaultSettings[key] = !SETTINGS[key].default; + } } const LEVEL_HANDLERS = { @@ -56,7 +62,7 @@ const LEVEL_HANDLERS = { "account": new AccountSettingsHandler(), "room": new RoomSettingsHandler(), "config": new ConfigSettingsHandler(), - "default": new DefaultSettingsHandler(defaultSettings), + "default": new DefaultSettingsHandler(defaultSettings, invertedDefaultSettings), }; // Wrap all the handlers with local echo @@ -200,11 +206,11 @@ export default class SettingsStore { */ static getValueAt(level, settingName, roomId = null, explicit = false, excludeDefault = false) { // Verify that the setting is actually a setting - if (!SETTINGS[settingName]) { + const setting = SETTINGS[settingName]; + if (!setting) { throw new Error("Setting '" + settingName + "' does not appear to be a setting."); } - const setting = SETTINGS[settingName]; const levelOrder = (setting.supportedLevelsAreOrdered ? setting.supportedLevels : LEVEL_ORDER); if (!levelOrder.includes("default")) levelOrder.push("default"); // always include default @@ -220,11 +226,26 @@ export default class SettingsStore { const handlers = SettingsStore._getHandlers(settingName); + // Check if we need to invert the setting at all. Do this after we get the setting + // handlers though, otherwise we'll fail to read the value. + let inverted = false; + if (setting.invertedSettingName) { + //console.warn(`Inverting ${settingName} to be ${setting.invertedSettingName} - legacy setting`); + settingName = setting.invertedSettingName; + inverted = true; + } + if (explicit) { const handler = handlers[level]; - if (!handler) return SettingsStore._tryControllerOverride(settingName, level, roomId, null, null); - const value = handler.getValue(settingName, roomId); - return SettingsStore._tryControllerOverride(settingName, level, roomId, value, level); + if (!handler) { + let value = SettingsStore._tryControllerOverride(setting, level, roomId, null, null); + if (inverted) value = !value; + return value; + } + let value = handler.getValue(settingName, roomId); + value = SettingsStore._tryControllerOverride(setting, level, roomId, value, level); + if (inverted) value = !value; + return value; } for (let i = minIndex; i < levelOrder.length; i++) { @@ -232,22 +253,27 @@ export default class SettingsStore { if (!handler) continue; if (excludeDefault && levelOrder[i] === "default") continue; - const value = handler.getValue(settingName, roomId); + let value = handler.getValue(settingName, roomId); if (value === null || value === undefined) continue; - return SettingsStore._tryControllerOverride(settingName, level, roomId, value, levelOrder[i]); + value = SettingsStore._tryControllerOverride(setting, level, roomId, value, levelOrder[i]); + if (inverted) value = !value; + return value; } - return SettingsStore._tryControllerOverride(settingName, level, roomId, null, null); + let value = SettingsStore._tryControllerOverride(setting, level, roomId, null, null); + if (inverted) value = !value; + return value; } - static _tryControllerOverride(settingName, level, roomId, calculatedValue, calculatedAtLevel) { - const controller = SETTINGS[settingName].controller; + static _tryControllerOverride(setting, level, roomId, calculatedValue, calculatedAtLevel) { + const controller = setting.controller; if (!controller) return calculatedValue; const actualValue = controller.getValueOverride(level, roomId, calculatedValue, calculatedAtLevel); if (actualValue !== undefined && actualValue !== null) return actualValue; return calculatedValue; } + /* eslint-disable valid-jsdoc */ //https://github.com/eslint/eslint/issues/7307 /** * Sets the value for a setting. The room ID is optional if the setting is not being @@ -263,7 +289,8 @@ export default class SettingsStore { /* eslint-enable valid-jsdoc */ static async setValue(settingName, roomId, level, value) { // Verify that the setting is actually a setting - if (!SETTINGS[settingName]) { + const setting = SETTINGS[settingName]; + if (!setting) { throw new Error("Setting '" + settingName + "' does not appear to be a setting."); } @@ -272,13 +299,22 @@ export default class SettingsStore { throw new Error("Setting " + settingName + " does not have a handler for " + level); } + if (setting.invertedSettingName) { + // Note: We can't do this when the `level` is "default", however we also + // know that the user can't possible change the default value through this + // function so we don't bother checking it. + //console.warn(`Inverting ${settingName} to be ${setting.invertedSettingName} - legacy setting`); + settingName = setting.invertedSettingName; + value = !value; + } + if (!handler.canSetValue(settingName, roomId)) { throw new Error("User cannot set " + settingName + " at " + level + " in " + roomId); } await handler.setValue(settingName, roomId, value); - const controller = SETTINGS[settingName].controller; + const controller = setting.controller; if (controller) { controller.onChange(level, roomId, value); } @@ -316,6 +352,101 @@ export default class SettingsStore { return LEVEL_HANDLERS[level].isSupported(); } + /** + * Debugging function for reading explicit setting values without going through the + * complicated/biased functions in the SettingsStore. This will print information to + * the console for analysis. Not intended to be used within the application. + * @param {string} realSettingName The setting name to try and read. + * @param {string} roomId Optional room ID to test the setting in. + */ + static debugSetting(realSettingName, roomId) { + console.log(`--- DEBUG ${realSettingName}`); + + // Note: we intentionally use JSON.stringify here to avoid the console masking the + // problem if there's a type representation issue. Also, this way it is guaranteed + // to show up in a rageshake if required. + + const def = SETTINGS[realSettingName]; + console.log(`--- definition: ${def ? JSON.stringify(def) : ''}`); + console.log(`--- default level order: ${JSON.stringify(LEVEL_ORDER)}`); + console.log(`--- registered handlers: ${JSON.stringify(Object.keys(LEVEL_HANDLERS))}`); + + const doChecks = (settingName) => { + for (const handlerName of Object.keys(LEVEL_HANDLERS)) { + const handler = LEVEL_HANDLERS[handlerName]; + + try { + const value = handler.getValue(settingName, roomId); + console.log(`--- ${handlerName}@${roomId || ''} = ${JSON.stringify(value)}`); + } catch (e) { + console.log(`--- ${handler}@${roomId || ''} THREW ERROR: ${e.message}`); + console.error(e); + } + + if (roomId) { + try { + const value = handler.getValue(settingName, null); + console.log(`--- ${handlerName}@ = ${JSON.stringify(value)}`); + } catch (e) { + console.log(`--- ${handler}@ THREW ERROR: ${e.message}`); + console.error(e); + } + } + } + + console.log(`--- calculating as returned by SettingsStore`); + console.log(`--- these might not match if the setting uses a controller - be warned!`); + + try { + const value = SettingsStore.getValue(settingName, roomId); + console.log(`--- SettingsStore#generic@${roomId || ''} = ${JSON.stringify(value)}`); + } catch (e) { + console.log(`--- SettingsStore#generic@${roomId || ''} THREW ERROR: ${e.message}`); + console.error(e); + } + + if (roomId) { + try { + const value = SettingsStore.getValue(settingName, null); + console.log(`--- SettingsStore#generic@ = ${JSON.stringify(value)}`); + } catch (e) { + console.log(`--- SettingsStore#generic@$ THREW ERROR: ${e.message}`); + console.error(e); + } + } + + for (const level of LEVEL_ORDER) { + try { + const value = SettingsStore.getValueAt(level, settingName, roomId); + console.log(`--- SettingsStore#${level}@${roomId || ''} = ${JSON.stringify(value)}`); + } catch (e) { + console.log(`--- SettingsStore#${level}@${roomId || ''} THREW ERROR: ${e.message}`); + console.error(e); + } + + if (roomId) { + try { + const value = SettingsStore.getValueAt(level, settingName, null); + console.log(`--- SettingsStore#${level}@ = ${JSON.stringify(value)}`); + } catch (e) { + console.log(`--- SettingsStore#${level}@$ THREW ERROR: ${e.message}`); + console.error(e); + } + } + } + }; + + doChecks(realSettingName); + + if (def.invertedSettingName) { + console.log(`--- TESTING INVERTED SETTING NAME`); + console.log(`--- inverted: ${def.invertedSettingName}`); + doChecks(def.invertedSettingName); + } + + console.log(`--- END DEBUG`); + } + static _getHandler(settingName, level) { const handlers = SettingsStore._getHandlers(settingName); if (!handlers[level]) return null; @@ -355,3 +486,6 @@ export default class SettingsStore { return featureState; } } + +// For debugging purposes +global.mxSettingsStore = SettingsStore; diff --git a/src/settings/handlers/DefaultSettingsHandler.js b/src/settings/handlers/DefaultSettingsHandler.js index cf2e660411..11e8b729bc 100644 --- a/src/settings/handlers/DefaultSettingsHandler.js +++ b/src/settings/handlers/DefaultSettingsHandler.js @@ -24,14 +24,20 @@ export default class DefaultSettingsHandler extends SettingsHandler { /** * Creates a new default settings handler with the given defaults * @param {object} defaults The default setting values, keyed by setting name. + * @param {object} invertedDefaults The default inverted setting values, keyed by setting name. */ - constructor(defaults) { + constructor(defaults, invertedDefaults) { super(); this._defaults = defaults; + this._invertedDefaults = invertedDefaults; } getValue(settingName, roomId) { - return this._defaults[settingName]; + let value = this._defaults[settingName]; + if (value === undefined) { + value = this._invertedDefaults[settingName]; + } + return value; } setValue(settingName, roomId, newValue) { diff --git a/src/shouldHideEvent.js b/src/shouldHideEvent.js index adc89a126a..47c901cd9f 100644 --- a/src/shouldHideEvent.js +++ b/src/shouldHideEvent.js @@ -44,14 +44,14 @@ export default function shouldHideEvent(ev) { const isEnabled = (name) => SettingsStore.getValue(name, ev.getRoomId()); // Hide redacted events - if (ev.isRedacted() && isEnabled('hideRedactions')) return true; + if (ev.isRedacted() && !isEnabled('showRedactions')) return true; const eventDiff = memberEventDiff(ev); if (eventDiff.isMemberEvent) { - if ((eventDiff.isJoin || eventDiff.isPart) && isEnabled('hideJoinLeaves')) return true; - if (eventDiff.isAvatarChange && isEnabled('hideAvatarChanges')) return true; - if (eventDiff.isDisplaynameChange && isEnabled('hideDisplaynameChanges')) return true; + if ((eventDiff.isJoin || eventDiff.isPart) && !isEnabled('showJoinLeaves')) return true; + if (eventDiff.isAvatarChange && !isEnabled('showAvatarChanges')) return true; + if (eventDiff.isDisplaynameChange && !isEnabled('showDisplaynameChanges')) return true; } return false;