From 65d6bfe6e96f77eedbce0af603a25737a7c2ca51 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Thu, 11 Jan 2024 09:49:00 +0000 Subject: [PATCH] Cleanup tasks in the Settings code (#12125) * inline call to `SettingsStore.isEnabled` * Remove confusing `SettingsStore.isEnabled` This was basically the same as `SettingsStore.canSetValue` only more confusing, so let's get rid of it. * Add `configDisablesSetting` value for Settings The current magic where this only works for features (but not beta features!) is, well, magical. And I need more flexibility here. * Remove redundant levels constant `LEVELS_FEATURE` I don't know if this was ever intended to have different semantics to `LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG`, but if it was, those semantics shuold have been written down. They now seem to be used entirely interchangably. --- .../views/elements/SettingsFlag.tsx | 14 +-- .../views/settings/CryptographyPanel.tsx | 2 +- .../views/settings/E2eAdvancedPanel.tsx | 2 +- .../tabs/room/SecurityRoomSettingsTab.tsx | 5 +- src/settings/Settings.tsx | 94 +++++++++++++------ src/settings/SettingsStore.ts | 26 +++-- .../user/PreferencesUserSettingsTab-test.tsx | 1 - 7 files changed, 84 insertions(+), 60 deletions(-) diff --git a/src/components/views/elements/SettingsFlag.tsx b/src/components/views/elements/SettingsFlag.tsx index df4a08c1d5..d44a3323b7 100644 --- a/src/components/views/elements/SettingsFlag.tsx +++ b/src/components/views/elements/SettingsFlag.tsx @@ -40,8 +40,6 @@ interface IProps { interface IState { value: boolean; - /** true if `SettingsStore.isEnabled` returned false. */ - disabled: boolean; } export default class SettingsFlag extends React.Component { @@ -52,7 +50,6 @@ export default class SettingsFlag extends React.Component { this.state = { value: this.getSettingValue(), - disabled: this.isSettingDisabled(), }; } @@ -72,15 +69,9 @@ export default class SettingsFlag extends React.Component { this.props.isExplicit, ); } - - private isSettingDisabled(): boolean { - return !SettingsStore.isEnabled(this.props.name); - } - private onSettingChange = (): void => { this.setState({ value: this.getSettingValue(), - disabled: this.isSettingDisabled(), }); }; @@ -104,14 +95,13 @@ export default class SettingsFlag extends React.Component { }; public render(): React.ReactNode { - const canChange = SettingsStore.canSetValue(this.props.name, this.props.roomId ?? null, this.props.level); + const disabled = !SettingsStore.canSetValue(this.props.name, this.props.roomId ?? null, this.props.level); - if (!canChange && this.props.hideIfCannotSet) return null; + if (disabled && this.props.hideIfCannotSet) return null; const label = this.props.label ?? SettingsStore.getDisplayName(this.props.name, this.props.level); const description = SettingsStore.getDescription(this.props.name); const shouldWarn = SettingsStore.shouldHaveWarning(this.props.name); - const disabled = this.state.disabled || !canChange; if (this.props.useCheckbox) { return ( diff --git a/src/components/views/settings/CryptographyPanel.tsx b/src/components/views/settings/CryptographyPanel.tsx index 0cb04859a7..13151d2f01 100644 --- a/src/components/views/settings/CryptographyPanel.tsx +++ b/src/components/views/settings/CryptographyPanel.tsx @@ -62,7 +62,7 @@ export default class CryptographyPanel extends React.Component { } let noSendUnverifiedSetting: JSX.Element | undefined; - if (SettingsStore.isEnabled("blacklistUnverifiedDevices")) { + if (SettingsStore.canSetValue("blacklistUnverifiedDevices", null, SettingLevel.DEVICE)) { noSendUnverifiedSetting = ( { export default E2eAdvancedPanel; export function isE2eAdvancedPanelPossible(): boolean { - return SettingsStore.isEnabled(SETTING_MANUALLY_VERIFY_ALL_SESSIONS); + return SettingsStore.canSetValue(SETTING_MANUALLY_VERIFY_ALL_SESSIONS, null, SettingLevel.DEVICE); } diff --git a/src/components/views/settings/tabs/room/SecurityRoomSettingsTab.tsx b/src/components/views/settings/tabs/room/SecurityRoomSettingsTab.tsx index bc573ec839..6ac686e063 100644 --- a/src/components/views/settings/tabs/room/SecurityRoomSettingsTab.tsx +++ b/src/components/views/settings/tabs/room/SecurityRoomSettingsTab.tsx @@ -427,7 +427,10 @@ export default class SecurityRoomSettingsTab extends React.Component { isFeature?: false | undefined; + /** + * If true, then the presence of this setting in `config.json` will disable the option in the UI. + * + * In other words, we prevent the user overriding the setting if an explicit value is given in `config.json`; + * though note that users who have already set a non-default value before `config.json` is update will continue + * to use that value (and, indeed, won't be able to change it!) + * + * Obviously, this only really makes sense if `supportedLevels` includes {@link SettingLevel.CONFIG}. + */ + configDisablesSetting?: true; + // Display names are strongly recommended for clarity. // Display name can also be an object for different levels. displayName?: @@ -192,7 +202,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { isFeature: true, labsGroup: LabGroup.VoiceAndVideo, displayName: _td("labs|video_rooms"), - supportedLevels: LEVELS_FEATURE, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, default: false, // Reload to ensure that the left panel etc. get remounted controller: new ReloadOnChangeController(), @@ -230,7 +240,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { [Features.NotificationSettings2]: { isFeature: true, labsGroup: LabGroup.Experimental, - supportedLevels: LEVELS_FEATURE, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|notification_settings"), default: false, betaInfo: { @@ -249,62 +259,70 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_msc3531_hide_messages_pending_moderation": { isFeature: true, labsGroup: LabGroup.Moderation, + configDisablesSetting: true, // Requires a reload since this setting is cached in EventUtils controller: new ReloadOnChangeController(), displayName: _td("labs|msc3531_hide_messages_pending_moderation"), - supportedLevels: LEVELS_FEATURE, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, default: false, }, "feature_report_to_moderators": { isFeature: true, labsGroup: LabGroup.Moderation, + configDisablesSetting: true, displayName: _td("labs|report_to_moderators"), description: _td("labs|report_to_moderators_description"), - supportedLevels: LEVELS_FEATURE, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, default: false, }, "feature_latex_maths": { isFeature: true, labsGroup: LabGroup.Messaging, + configDisablesSetting: true, displayName: _td("labs|latex_maths"), - supportedLevels: LEVELS_FEATURE, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, default: false, }, "feature_pinning": { isFeature: true, labsGroup: LabGroup.Messaging, + configDisablesSetting: true, displayName: _td("labs|pinning"), - supportedLevels: LEVELS_FEATURE, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, default: false, }, "feature_wysiwyg_composer": { isFeature: true, labsGroup: LabGroup.Messaging, + configDisablesSetting: true, displayName: _td("labs|wysiwyg_composer"), description: _td("labs|feature_wysiwyg_composer_description"), - supportedLevels: LEVELS_FEATURE, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, default: false, }, "feature_mjolnir": { isFeature: true, labsGroup: LabGroup.Moderation, + configDisablesSetting: true, displayName: _td("labs|mjolnir"), description: _td("labs|currently_experimental"), - supportedLevels: LEVELS_FEATURE, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, default: false, }, "feature_custom_themes": { isFeature: true, labsGroup: LabGroup.Themes, + configDisablesSetting: true, displayName: _td("labs|custom_themes"), - supportedLevels: LEVELS_FEATURE, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, default: false, }, "feature_dehydration": { isFeature: true, labsGroup: LabGroup.Encryption, + configDisablesSetting: true, displayName: _td("labs|dehydration"), - supportedLevels: LEVELS_FEATURE, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, default: false, }, "useOnlyCurrentProfiles": { @@ -323,22 +341,25 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_html_topic": { isFeature: true, labsGroup: LabGroup.Rooms, - supportedLevels: LEVELS_FEATURE, + configDisablesSetting: true, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|html_topic"), default: false, }, "feature_bridge_state": { isFeature: true, labsGroup: LabGroup.Rooms, - supportedLevels: LEVELS_FEATURE, + configDisablesSetting: true, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|bridge_state"), default: false, }, "feature_jump_to_date": { isFeature: true, labsGroup: LabGroup.Messaging, + configDisablesSetting: true, displayName: _td("labs|jump_to_date"), - supportedLevels: LEVELS_FEATURE, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, default: false, controller: new ServerSupportUnstableFeatureController( "feature_jump_to_date", @@ -368,6 +389,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_sliding_sync": { isFeature: true, labsGroup: LabGroup.Developer, + configDisablesSetting: true, supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|sliding_sync"), description: _td("labs|sliding_sync_description"), @@ -382,32 +404,36 @@ export const SETTINGS: { [setting: string]: ISetting } = { }, "feature_element_call_video_rooms": { isFeature: true, - supportedLevels: LEVELS_FEATURE, labsGroup: LabGroup.VoiceAndVideo, + configDisablesSetting: true, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|element_call_video_rooms"), controller: new ReloadOnChangeController(), default: false, }, "feature_group_calls": { isFeature: true, - supportedLevels: LEVELS_FEATURE, labsGroup: LabGroup.VoiceAndVideo, + configDisablesSetting: true, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|group_calls"), controller: new ReloadOnChangeController(), default: false, }, "feature_disable_call_per_sender_encryption": { isFeature: true, - supportedLevels: LEVELS_FEATURE, labsGroup: LabGroup.VoiceAndVideo, + configDisablesSetting: true, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|feature_disable_call_per_sender_encryption"), default: false, }, "feature_allow_screen_share_only_mode": { isFeature: true, + labsGroup: LabGroup.VoiceAndVideo, + configDisablesSetting: true, supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, description: _td("labs|under_active_development"), - labsGroup: LabGroup.VoiceAndVideo, displayName: _td("labs|allow_screen_share_only_mode"), controller: new ReloadOnChangeController(), default: false, @@ -415,7 +441,8 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_location_share_live": { isFeature: true, labsGroup: LabGroup.Messaging, - supportedLevels: LEVELS_FEATURE, + configDisablesSetting: true, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|location_share_live"), description: _td("labs|location_share_live_description"), shouldWarn: true, @@ -424,7 +451,8 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_dynamic_room_predecessors": { isFeature: true, labsGroup: LabGroup.Rooms, - supportedLevels: LEVELS_FEATURE, + configDisablesSetting: true, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|dynamic_room_predecessors"), description: _td("labs|dynamic_room_predecessors_description"), shouldWarn: true, @@ -433,7 +461,8 @@ export const SETTINGS: { [setting: string]: ISetting } = { [Features.VoiceBroadcast]: { isFeature: true, labsGroup: LabGroup.Messaging, - supportedLevels: LEVELS_FEATURE, + configDisablesSetting: true, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|voice_broadcast"), default: false, }, @@ -445,7 +474,8 @@ export const SETTINGS: { [setting: string]: ISetting } = { [Features.OidcNativeFlow]: { isFeature: true, labsGroup: LabGroup.Developer, - supportedLevels: LEVELS_FEATURE, + configDisablesSetting: true, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|oidc_native_flow"), description: _td("labs|oidc_native_flow_description"), default: false, @@ -454,6 +484,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { // use the rust matrix-sdk-crypto-js for crypto. isFeature: true, labsGroup: LabGroup.Developer, + configDisablesSetting: true, supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|rust_crypto"), description: _td("labs|under_active_development"), @@ -470,9 +501,10 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_render_reaction_images": { isFeature: true, labsGroup: LabGroup.Messaging, + configDisablesSetting: true, displayName: _td("labs|render_reaction_images"), description: _td("labs|render_reaction_images_description"), - supportedLevels: LEVELS_FEATURE, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, default: false, }, /** @@ -521,33 +553,37 @@ export const SETTINGS: { [setting: string]: ISetting } = { }, "feature_hidebold": { isFeature: true, + labsGroup: LabGroup.Rooms, + configDisablesSetting: true, supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|hidebold"), - labsGroup: LabGroup.Rooms, default: false, }, "feature_ask_to_join": { - default: false, - displayName: _td("labs|ask_to_join"), isFeature: true, labsGroup: LabGroup.Rooms, - supportedLevels: LEVELS_FEATURE, + configDisablesSetting: true, + default: false, + displayName: _td("labs|ask_to_join"), + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, }, "feature_new_room_decoration_ui": { isFeature: true, labsGroup: LabGroup.Rooms, + configDisablesSetting: true, displayName: _td("labs|new_room_decoration_ui"), description: _td("labs|under_active_development"), - supportedLevels: LEVELS_FEATURE, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, default: false, controller: new ReloadOnChangeController(), }, "feature_notifications": { isFeature: true, labsGroup: LabGroup.Messaging, + configDisablesSetting: true, displayName: _td("labs|notifications"), description: _td("labs|unrealiable_e2e"), - supportedLevels: LEVELS_FEATURE, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, default: false, }, "useCompactLayout": { diff --git a/src/settings/SettingsStore.ts b/src/settings/SettingsStore.ts index 48ffbd656a..6470289299 100644 --- a/src/settings/SettingsStore.ts +++ b/src/settings/SettingsStore.ts @@ -320,19 +320,6 @@ export default class SettingsStore { } } - /** - * Determines if a setting is enabled. - * If a setting is disabled then it should normally be hidden from the user to de-clutter the user interface. - * This rule is intentionally ignored for labs flags to unveil what features are available with - * the right server support. - * @param {string} settingName The setting to look up. - * @return {boolean} True if the setting is enabled. - */ - public static isEnabled(settingName: string): boolean { - if (!SETTINGS[settingName]) return false; - return !SETTINGS[settingName].controller?.settingDisabled ?? true; - } - /** * Retrieves the reason a setting is disabled if one is assigned. * If a setting is not disabled, or no reason is given by the `SettingController`, @@ -516,6 +503,15 @@ export default class SettingsStore { * Determines if the current user is permitted to set the given setting at the given * level for a particular room. The room ID is optional if the setting is not being * set for a particular room, otherwise it should be supplied. + * + * This takes into account both the value of {@link SettingController#settingDisabled} of the + * `SettingController`, if any; and, for settings where {@link IBaseSetting#configDisablesSetting} is true, + * whether the setting has been given a value in `config.json`. + * + * Typically, if the user cannot set the setting, it should be hidden, to declutter the UI; + * however some settings (typically, the labs flags) are exposed but greyed out, to unveil + * what features are available with the right server support. + * * @param {string} settingName The name of the setting to check. * @param {String} roomId The room ID to check in, may be null. * @param {SettingLevel} level The level to @@ -528,12 +524,12 @@ export default class SettingsStore { throw new Error("Setting '" + settingName + "' does not appear to be a setting."); } - if (!SettingsStore.isEnabled(settingName)) { + if (SETTINGS[settingName].controller?.settingDisabled) { return false; } // When non-beta features are specified in the config.json, we force them as enabled or disabled. - if (SettingsStore.isFeature(settingName) && !SETTINGS[settingName]?.betaInfo) { + if (SETTINGS[settingName]?.configDisablesSetting) { const configVal = SettingsStore.getValueAt(SettingLevel.CONFIG, settingName, roomId, true, true); if (configVal === true || configVal === false) return false; } diff --git a/test/components/views/settings/tabs/user/PreferencesUserSettingsTab-test.tsx b/test/components/views/settings/tabs/user/PreferencesUserSettingsTab-test.tsx index 6eafde504e..292f4e2746 100644 --- a/test/components/views/settings/tabs/user/PreferencesUserSettingsTab-test.tsx +++ b/test/components/views/settings/tabs/user/PreferencesUserSettingsTab-test.tsx @@ -42,7 +42,6 @@ describe("PreferencesUserSettingsTab", () => { beforeEach(() => { stubClient(); jest.spyOn(SettingsStore, "setValue"); - jest.spyOn(SettingsStore, "canSetValue").mockReturnValue(true); jest.spyOn(window, "matchMedia").mockReturnValue({ matches: false } as MediaQueryList); });