From 86e19147760e4d1c7a3fd765f9d5d48664ae5c8d Mon Sep 17 00:00:00 2001 From: David Langley Date: Mon, 10 Jun 2024 11:07:31 +0100 Subject: [PATCH 01/10] Make config override other settings levels and add tests --- src/settings/Settings.tsx | 56 ++++++++++++++--------------- src/settings/SettingsStore.ts | 42 +++++++++++++++++----- test/settings/SettingsStore-test.ts | 25 +++++++++++++ 3 files changed, 85 insertions(+), 38 deletions(-) diff --git a/src/settings/Settings.tsx b/src/settings/Settings.tsx index 3650e518148..f1c57c20dba 100644 --- a/src/settings/Settings.tsx +++ b/src/settings/Settings.tsx @@ -134,15 +134,13 @@ export interface IBaseSetting { isFeature?: false | undefined; /** - * If true, then the presence of this setting in `config.json` will disable the option in the UI. + * If true, then the presence of this setting in `config.json` take precence over settings at other levels and 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`. - * XXX: 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!): https://github.com/element-hq/element-web/issues/26877 * * Obviously, this only really makes sense if `supportedLevels` includes {@link SettingLevel.CONFIG}. */ - configDisablesSetting?: true; + configOverridesSetting?: true; // Display names are strongly recommended for clarity. // Display name can also be an object for different levels. @@ -268,7 +266,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_msc3531_hide_messages_pending_moderation": { isFeature: true, labsGroup: LabGroup.Moderation, - configDisablesSetting: true, + configOverridesSetting: true, // Requires a reload since this setting is cached in EventUtils controller: new ReloadOnChangeController(), displayName: _td("labs|msc3531_hide_messages_pending_moderation"), @@ -278,7 +276,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_report_to_moderators": { isFeature: true, labsGroup: LabGroup.Moderation, - configDisablesSetting: true, + configOverridesSetting: true, displayName: _td("labs|report_to_moderators"), description: _td("labs|report_to_moderators_description"), supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, @@ -287,7 +285,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_latex_maths": { isFeature: true, labsGroup: LabGroup.Messaging, - configDisablesSetting: true, + configOverridesSetting: true, displayName: _td("labs|latex_maths"), supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, default: false, @@ -295,7 +293,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_pinning": { isFeature: true, labsGroup: LabGroup.Messaging, - configDisablesSetting: true, + configOverridesSetting: true, displayName: _td("labs|pinning"), supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, default: false, @@ -303,7 +301,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_wysiwyg_composer": { isFeature: true, labsGroup: LabGroup.Messaging, - configDisablesSetting: true, + configOverridesSetting: true, displayName: _td("labs|wysiwyg_composer"), description: _td("labs|feature_wysiwyg_composer_description"), supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, @@ -312,7 +310,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_mjolnir": { isFeature: true, labsGroup: LabGroup.Moderation, - configDisablesSetting: true, + configOverridesSetting: true, displayName: _td("labs|mjolnir"), description: _td("labs|currently_experimental"), supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, @@ -321,7 +319,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_custom_themes": { isFeature: true, labsGroup: LabGroup.Themes, - configDisablesSetting: true, + configOverridesSetting: true, displayName: _td("labs|custom_themes"), supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, default: false, @@ -329,7 +327,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_dehydration": { isFeature: true, labsGroup: LabGroup.Encryption, - configDisablesSetting: true, + configOverridesSetting: true, displayName: _td("labs|dehydration"), supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, default: false, @@ -350,7 +348,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_html_topic": { isFeature: true, labsGroup: LabGroup.Rooms, - configDisablesSetting: true, + configOverridesSetting: true, supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|html_topic"), default: false, @@ -358,7 +356,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_bridge_state": { isFeature: true, labsGroup: LabGroup.Rooms, - configDisablesSetting: true, + configOverridesSetting: true, supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|bridge_state"), default: false, @@ -366,7 +364,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_jump_to_date": { isFeature: true, labsGroup: LabGroup.Messaging, - configDisablesSetting: true, + configOverridesSetting: true, displayName: _td("labs|jump_to_date"), supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, default: false, @@ -398,7 +396,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_sliding_sync": { isFeature: true, labsGroup: LabGroup.Developer, - configDisablesSetting: true, + configOverridesSetting: true, supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|sliding_sync"), description: _td("labs|sliding_sync_description"), @@ -414,7 +412,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_element_call_video_rooms": { isFeature: true, labsGroup: LabGroup.VoiceAndVideo, - configDisablesSetting: true, + configOverridesSetting: true, supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|element_call_video_rooms"), controller: new ReloadOnChangeController(), @@ -423,7 +421,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_group_calls": { isFeature: true, labsGroup: LabGroup.VoiceAndVideo, - configDisablesSetting: true, + configOverridesSetting: true, supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|group_calls"), controller: new ReloadOnChangeController(), @@ -432,7 +430,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_disable_call_per_sender_encryption": { isFeature: true, labsGroup: LabGroup.VoiceAndVideo, - configDisablesSetting: true, + configOverridesSetting: true, supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|feature_disable_call_per_sender_encryption"), default: false, @@ -440,7 +438,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_allow_screen_share_only_mode": { isFeature: true, labsGroup: LabGroup.VoiceAndVideo, - configDisablesSetting: true, + configOverridesSetting: true, supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, description: _td("labs|under_active_development"), displayName: _td("labs|allow_screen_share_only_mode"), @@ -450,7 +448,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_location_share_live": { isFeature: true, labsGroup: LabGroup.Messaging, - configDisablesSetting: true, + configOverridesSetting: true, supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|location_share_live"), description: _td("labs|location_share_live_description"), @@ -460,7 +458,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_dynamic_room_predecessors": { isFeature: true, labsGroup: LabGroup.Rooms, - configDisablesSetting: true, + configOverridesSetting: true, supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|dynamic_room_predecessors"), description: _td("labs|dynamic_room_predecessors_description"), @@ -470,7 +468,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { [Features.VoiceBroadcast]: { isFeature: true, labsGroup: LabGroup.Messaging, - configDisablesSetting: true, + configOverridesSetting: true, supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|voice_broadcast"), default: false, @@ -483,7 +481,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { [Features.OidcNativeFlow]: { isFeature: true, labsGroup: LabGroup.Developer, - configDisablesSetting: true, + configOverridesSetting: true, supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|oidc_native_flow"), description: _td("labs|oidc_native_flow_description"), @@ -493,7 +491,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { // use the rust matrix-sdk-crypto-wasm for crypto. isFeature: true, labsGroup: LabGroup.Developer, - // unlike most features, `configDisablesSetting` is false here. + // unlike most features, `configOverridesSetting` is false here. supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|rust_crypto"), description: () => { @@ -527,7 +525,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_render_reaction_images": { isFeature: true, labsGroup: LabGroup.Messaging, - configDisablesSetting: true, + configOverridesSetting: true, displayName: _td("labs|render_reaction_images"), description: _td("labs|render_reaction_images_description"), supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, @@ -609,7 +607,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_ask_to_join": { isFeature: true, labsGroup: LabGroup.Rooms, - configDisablesSetting: true, + configOverridesSetting: true, default: false, displayName: _td("labs|ask_to_join"), supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, @@ -617,7 +615,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_new_room_decoration_ui": { isFeature: true, labsGroup: LabGroup.Rooms, - configDisablesSetting: true, + configOverridesSetting: true, displayName: _td("labs|new_room_decoration_ui"), description: _td("labs|under_active_development"), supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, @@ -627,7 +625,7 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_notifications": { isFeature: true, labsGroup: LabGroup.Messaging, - configDisablesSetting: true, + configOverridesSetting: true, displayName: _td("labs|notifications"), description: _td("labs|unrealiable_e2e"), supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, diff --git a/src/settings/SettingsStore.ts b/src/settings/SettingsStore.ts index 7b1b6493d0e..51a680ba036 100644 --- a/src/settings/SettingsStore.ts +++ b/src/settings/SettingsStore.ts @@ -370,6 +370,13 @@ export default class SettingsStore { explicit = false, excludeDefault = false, ): any { + let finalLevel: SettingLevel = level; + // For some config settings (mostly: non-beta features), a value in config.json overrides the local setting + // (ie: we force them as enabled or disabled). In this case we such read the value from the + if (level !== SettingLevel.CONFIG && this.doesConfigOverrideSetting(settingName, roomId)) { + finalLevel = SettingLevel.CONFIG; + } + // Verify that the setting is actually a setting const setting = SETTINGS[settingName]; if (!setting) { @@ -379,8 +386,8 @@ export default class SettingsStore { const levelOrder = getLevelOrder(setting); if (!levelOrder.includes(SettingLevel.DEFAULT)) levelOrder.push(SettingLevel.DEFAULT); // always include default - const minIndex = levelOrder.indexOf(level); - if (minIndex === -1) throw new Error(`Level "${level}" for setting "${settingName}" is not prioritized`); + const minIndex = levelOrder.indexOf(finalLevel); + if (minIndex === -1) throw new Error(`Level "${finalLevel}" for setting "${settingName}" is not prioritized`); const handlers = SettingsStore.getHandlers(settingName); @@ -392,12 +399,12 @@ export default class SettingsStore { } if (explicit) { - const handler = handlers[level]; + const handler = handlers[finalLevel]; if (!handler) { - return SettingsStore.getFinalValue(setting, level, roomId, null, null); + return SettingsStore.getFinalValue(setting, finalLevel, roomId, null, null); } const value = handler.getValue(settingName, roomId); - return SettingsStore.getFinalValue(setting, level, roomId, value, level); + return SettingsStore.getFinalValue(setting, finalLevel, roomId, value, finalLevel); } for (let i = minIndex; i < levelOrder.length; i++) { @@ -407,10 +414,10 @@ export default class SettingsStore { const value = handler.getValue(settingName, roomId); if (value === null || value === undefined) continue; - return SettingsStore.getFinalValue(setting, level, roomId, value, levelOrder[i]); + return SettingsStore.getFinalValue(setting, finalLevel, roomId, value, levelOrder[i]); } - return SettingsStore.getFinalValue(setting, level, roomId, null, null); + return SettingsStore.getFinalValue(setting, finalLevel, roomId, null, null); } /** @@ -529,8 +536,11 @@ export default class SettingsStore { } // For some config settings (mostly: non-beta features), a value in config.json overrides the local setting - // (ie: we force them as enabled or disabled). - if (SETTINGS[settingName]?.configDisablesSetting) { + // (ie: we force them as enabled or disabled). In this case we such not let the user change the setting. + if (this.doesConfigOverrideSetting(settingName, roomId)) { + return false; + } + if (SETTINGS[settingName]?.configOverridesSetting) { const configVal = SettingsStore.getValueAt(SettingLevel.CONFIG, settingName, roomId, true, true); if (configVal === true || configVal === false) return false; } @@ -540,6 +550,20 @@ export default class SettingsStore { return handler.canSetValue(settingName, roomId); } + /** + * Determines if the given setting's config should override other levels + * @param settingName + * @param roomId + * @returns + */ + private static doesConfigOverrideSetting(settingName: string, roomId: string | null) { + if (SETTINGS[settingName]?.configOverridesSetting) { + const configVal = SettingsStore.getValueAt(SettingLevel.CONFIG, settingName, roomId, true, true); + if (configVal === true || configVal === false) return true; + } + return false; + } + /** * Determines if the given level is supported on this device. * @param {SettingLevel} level The level diff --git a/test/settings/SettingsStore-test.ts b/test/settings/SettingsStore-test.ts index 25f055d9b5e..c73f4f8244d 100644 --- a/test/settings/SettingsStore-test.ts +++ b/test/settings/SettingsStore-test.ts @@ -15,6 +15,7 @@ limitations under the License. */ import BasePlatform from "../../src/BasePlatform"; +import SdkConfig from "../../src/SdkConfig"; import { SettingLevel } from "../../src/settings/SettingLevel"; import SettingsStore from "../../src/settings/SettingsStore"; import { mockPlatformPeg } from "../test-utils"; @@ -27,6 +28,9 @@ const TEST_DATA = [ }, ]; +// An existing setting that has configOverridesSetting set to true. +const SETTING_NAME_WITH_CONFIG_OVERRIDE = "feature_new_room_decoration_ui"; + describe("SettingsStore", () => { let platformSettings: Record; @@ -42,6 +46,7 @@ describe("SettingsStore", () => { getSettingValue: jest.fn().mockImplementation((settingName: string) => { return platformSettings[settingName]; }), + reload: jest.fn(), } as unknown as BasePlatform); TEST_DATA.forEach((d) => { @@ -49,6 +54,10 @@ describe("SettingsStore", () => { }); }); + beforeEach(() => { + SdkConfig.reset(); + }); + describe("getValueAt", () => { TEST_DATA.forEach((d) => { it(`should return the value "${d.level}"."${d.name}"`, () => { @@ -57,5 +66,21 @@ describe("SettingsStore", () => { expect(SettingsStore.getValueAt(d.level, d.name)).toBe(d.value); }); }); + + it(`configOverridesSetting correctly overrides setting`, async () => { + SdkConfig.put({ + features: { + [SETTING_NAME_WITH_CONFIG_OVERRIDE]: false, + }, + }); + + await SettingsStore.setValue(SETTING_NAME_WITH_CONFIG_OVERRIDE, null, SettingLevel.DEVICE, true); + expect(SettingsStore.getValueAt(SettingLevel.DEVICE, SETTING_NAME_WITH_CONFIG_OVERRIDE)).toBe(false); + }); + + it(`configOverridesSetting doesn't incorrectly override setting`, async () => { + await SettingsStore.setValue(SETTING_NAME_WITH_CONFIG_OVERRIDE, null, SettingLevel.DEVICE, true); + expect(SettingsStore.getValueAt(SettingLevel.DEVICE, SETTING_NAME_WITH_CONFIG_OVERRIDE)).toBe(true); + }); }); }); From c65c6efdbd33696e656db6f5e4e71ec4a535acb1 Mon Sep 17 00:00:00 2001 From: David Langley Date: Mon, 10 Jun 2024 11:11:03 +0100 Subject: [PATCH 02/10] fix documentation --- src/settings/Settings.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/settings/Settings.tsx b/src/settings/Settings.tsx index f1c57c20dba..43b2ae4b0f6 100644 --- a/src/settings/Settings.tsx +++ b/src/settings/Settings.tsx @@ -134,7 +134,7 @@ export interface IBaseSetting { isFeature?: false | undefined; /** - * If true, then the presence of this setting in `config.json` take precence over settings at other levels and will disable the option in the UI. + * If true, then the presence of this setting in `config.json` will cause the config level to take precedence over settings at other levels and 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`. * From 845a2c3598c14d8a2eaa94391424eaca776cd0a9 Mon Sep 17 00:00:00 2001 From: David Langley Date: Mon, 10 Jun 2024 11:17:26 +0100 Subject: [PATCH 03/10] lint --- src/settings/SettingsStore.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/settings/SettingsStore.ts b/src/settings/SettingsStore.ts index 51a680ba036..7d981d63685 100644 --- a/src/settings/SettingsStore.ts +++ b/src/settings/SettingsStore.ts @@ -512,7 +512,7 @@ export default class SettingsStore { * 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, + * `SettingController`, if any; and, for settings where {@link IBaseSetting#configOverridesSetting} 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; @@ -556,7 +556,7 @@ export default class SettingsStore { * @param roomId * @returns */ - private static doesConfigOverrideSetting(settingName: string, roomId: string | null) { + private static doesConfigOverrideSetting(settingName: string, roomId: string | null): boolean { if (SETTINGS[settingName]?.configOverridesSetting) { const configVal = SettingsStore.getValueAt(SettingLevel.CONFIG, settingName, roomId, true, true); if (configVal === true || configVal === false) return true; From c7bda77d2a726db0449a86a242976a1880241c83 Mon Sep 17 00:00:00 2001 From: David Langley Date: Mon, 10 Jun 2024 11:26:37 +0100 Subject: [PATCH 04/10] Use a const for finalLevel. --- src/settings/SettingsStore.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/settings/SettingsStore.ts b/src/settings/SettingsStore.ts index 7d981d63685..0c036de0ac6 100644 --- a/src/settings/SettingsStore.ts +++ b/src/settings/SettingsStore.ts @@ -370,12 +370,12 @@ export default class SettingsStore { explicit = false, excludeDefault = false, ): any { - let finalLevel: SettingLevel = level; // For some config settings (mostly: non-beta features), a value in config.json overrides the local setting - // (ie: we force them as enabled or disabled). In this case we such read the value from the - if (level !== SettingLevel.CONFIG && this.doesConfigOverrideSetting(settingName, roomId)) { - finalLevel = SettingLevel.CONFIG; - } + // (ie: we force them as enabled or disabled). In this case we should read the value from the config. + const finalLevel: SettingLevel = + level !== SettingLevel.CONFIG && this.doesConfigOverrideSetting(settingName, roomId) + ? SettingLevel.CONFIG + : level; // Verify that the setting is actually a setting const setting = SETTINGS[settingName]; From c63522a21786c939db463dd3eb58a70327f22bfc Mon Sep 17 00:00:00 2001 From: David Langley Date: Mon, 10 Jun 2024 11:55:20 +0100 Subject: [PATCH 05/10] respect the explicit parameter --- src/settings/SettingsStore.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/settings/SettingsStore.ts b/src/settings/SettingsStore.ts index 0c036de0ac6..83006a02eaa 100644 --- a/src/settings/SettingsStore.ts +++ b/src/settings/SettingsStore.ts @@ -356,6 +356,13 @@ export default class SettingsStore { * Gets a setting's value at a particular level, ignoring all levels that are more specific. * @param {SettingLevel|"config"|"default"} level The * level to look at. + * + * This takes into account the value of {@link IBaseSetting#configOverridesSetting} of the `SettingController`. + * If {@link IBaseSetting#configOverridesSetting} is true, and the config level contains a valid value, + * this value will be read from config level rather than level passed. + * However the `explicit` paramter is respected, in that if true the value will only be read from the level specified. + * In this case it is therefore the responsibilty of the calling code to ensure the config level setting is respected. + * * @param {string} settingName The name of the setting to read. * @param {String} roomId The room ID to read the setting value in, may be null. * @param {boolean} explicit If true, this method will not consider other levels, just the one @@ -373,7 +380,7 @@ export default class SettingsStore { // For some config settings (mostly: non-beta features), a value in config.json overrides the local setting // (ie: we force them as enabled or disabled). In this case we should read the value from the config. const finalLevel: SettingLevel = - level !== SettingLevel.CONFIG && this.doesConfigOverrideSetting(settingName, roomId) + !explicit && level !== SettingLevel.CONFIG && this.doesConfigOverrideSetting(settingName, roomId) ? SettingLevel.CONFIG : level; From 1147afb926f5d061bac565f4fefcabecb0397a7a Mon Sep 17 00:00:00 2001 From: David Langley Date: Mon, 10 Jun 2024 19:08:18 +0100 Subject: [PATCH 06/10] Use supportedLevelsAreOrdered for config overrides rather than a separate setting. --- .../views/elements/SettingsFlag.tsx | 12 +- src/settings/Settings.tsx | 107 ++++++++---------- src/settings/SettingsStore.ts | 77 ++++++------- test/settings/SettingsStore-test.ts | 11 +- 4 files changed, 103 insertions(+), 104 deletions(-) diff --git a/src/components/views/elements/SettingsFlag.tsx b/src/components/views/elements/SettingsFlag.tsx index d44a3323b71..8fc2f283d56 100644 --- a/src/components/views/elements/SettingsFlag.tsx +++ b/src/components/views/elements/SettingsFlag.tsx @@ -62,6 +62,17 @@ export default class SettingsFlag extends React.Component { } private getSettingValue(): boolean { + // If a level defined in props is overridden by level at a high presenence, it gets disabled + // and we should show the overridding value. + if ( + !!SettingsStore.settingIsOveriddenAtAHigherLevel( + this.props.name, + this.props.roomId ?? null, + this.props.level, + ) + ) { + return !!SettingsStore.getValue(this.props.name); + } return !!SettingsStore.getValueAt( this.props.level, this.props.name, @@ -96,7 +107,6 @@ export default class SettingsFlag extends React.Component { public render(): React.ReactNode { const disabled = !SettingsStore.canSetValue(this.props.name, this.props.roomId ?? null, this.props.level); - if (disabled && this.props.hideIfCannotSet) return null; const label = this.props.label ?? SettingsStore.getDisplayName(this.props.name, this.props.level); diff --git a/src/settings/Settings.tsx b/src/settings/Settings.tsx index 43b2ae4b0f6..fe8cd3fcccb 100644 --- a/src/settings/Settings.tsx +++ b/src/settings/Settings.tsx @@ -71,6 +71,7 @@ const LEVELS_ROOM_SETTINGS_WITH_ROOM = [ const LEVELS_ACCOUNT_SETTINGS = [SettingLevel.DEVICE, SettingLevel.ACCOUNT, SettingLevel.CONFIG]; const LEVELS_DEVICE_ONLY_SETTINGS = [SettingLevel.DEVICE]; const LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG = [SettingLevel.DEVICE, SettingLevel.CONFIG]; +const LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED = [SettingLevel.CONFIG, SettingLevel.DEVICE]; const LEVELS_UI_FEATURE = [ SettingLevel.CONFIG, // in future we might have a .well-known level or something @@ -133,15 +134,6 @@ export type SettingValueType = export interface IBaseSetting { isFeature?: false | undefined; - /** - * If true, then the presence of this setting in `config.json` will cause the config level to take precedence over settings at other levels and 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`. - * - * Obviously, this only really makes sense if `supportedLevels` includes {@link SettingLevel.CONFIG}. - */ - configOverridesSetting?: true; - // Display names are strongly recommended for clarity. // Display name can also be an object for different levels. displayName?: @@ -266,70 +258,70 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_msc3531_hide_messages_pending_moderation": { isFeature: true, labsGroup: LabGroup.Moderation, - configOverridesSetting: true, // Requires a reload since this setting is cached in EventUtils controller: new ReloadOnChangeController(), displayName: _td("labs|msc3531_hide_messages_pending_moderation"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, }, "feature_report_to_moderators": { isFeature: true, labsGroup: LabGroup.Moderation, - configOverridesSetting: true, displayName: _td("labs|report_to_moderators"), description: _td("labs|report_to_moderators_description"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, }, "feature_latex_maths": { isFeature: true, labsGroup: LabGroup.Messaging, - configOverridesSetting: true, displayName: _td("labs|latex_maths"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, }, "feature_pinning": { isFeature: true, labsGroup: LabGroup.Messaging, - configOverridesSetting: true, displayName: _td("labs|pinning"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, }, "feature_wysiwyg_composer": { isFeature: true, labsGroup: LabGroup.Messaging, - configOverridesSetting: true, displayName: _td("labs|wysiwyg_composer"), description: _td("labs|feature_wysiwyg_composer_description"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, }, "feature_mjolnir": { isFeature: true, labsGroup: LabGroup.Moderation, - configOverridesSetting: true, displayName: _td("labs|mjolnir"), description: _td("labs|currently_experimental"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, }, "feature_custom_themes": { isFeature: true, labsGroup: LabGroup.Themes, - configOverridesSetting: true, displayName: _td("labs|custom_themes"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, }, "feature_dehydration": { isFeature: true, labsGroup: LabGroup.Encryption, - configOverridesSetting: true, displayName: _td("labs|dehydration"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, }, "useOnlyCurrentProfiles": { @@ -348,25 +340,25 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_html_topic": { isFeature: true, labsGroup: LabGroup.Rooms, - configOverridesSetting: true, - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, displayName: _td("labs|html_topic"), default: false, }, "feature_bridge_state": { isFeature: true, labsGroup: LabGroup.Rooms, - configOverridesSetting: true, - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, displayName: _td("labs|bridge_state"), default: false, }, "feature_jump_to_date": { isFeature: true, labsGroup: LabGroup.Messaging, - configOverridesSetting: true, displayName: _td("labs|jump_to_date"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, controller: new ServerSupportUnstableFeatureController( "feature_jump_to_date", @@ -396,8 +388,8 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_sliding_sync": { isFeature: true, labsGroup: LabGroup.Developer, - configOverridesSetting: true, - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, displayName: _td("labs|sliding_sync"), description: _td("labs|sliding_sync_description"), shouldWarn: true, @@ -412,8 +404,8 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_element_call_video_rooms": { isFeature: true, labsGroup: LabGroup.VoiceAndVideo, - configOverridesSetting: true, - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, displayName: _td("labs|element_call_video_rooms"), controller: new ReloadOnChangeController(), default: false, @@ -421,8 +413,8 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_group_calls": { isFeature: true, labsGroup: LabGroup.VoiceAndVideo, - configOverridesSetting: true, - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, displayName: _td("labs|group_calls"), controller: new ReloadOnChangeController(), default: false, @@ -430,16 +422,16 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_disable_call_per_sender_encryption": { isFeature: true, labsGroup: LabGroup.VoiceAndVideo, - configOverridesSetting: true, - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, displayName: _td("labs|feature_disable_call_per_sender_encryption"), default: false, }, "feature_allow_screen_share_only_mode": { isFeature: true, labsGroup: LabGroup.VoiceAndVideo, - configOverridesSetting: true, - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, description: _td("labs|under_active_development"), displayName: _td("labs|allow_screen_share_only_mode"), controller: new ReloadOnChangeController(), @@ -448,8 +440,8 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_location_share_live": { isFeature: true, labsGroup: LabGroup.Messaging, - configOverridesSetting: true, - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, displayName: _td("labs|location_share_live"), description: _td("labs|location_share_live_description"), shouldWarn: true, @@ -458,8 +450,8 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_dynamic_room_predecessors": { isFeature: true, labsGroup: LabGroup.Rooms, - configOverridesSetting: true, - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, displayName: _td("labs|dynamic_room_predecessors"), description: _td("labs|dynamic_room_predecessors_description"), shouldWarn: true, @@ -468,8 +460,8 @@ export const SETTINGS: { [setting: string]: ISetting } = { [Features.VoiceBroadcast]: { isFeature: true, labsGroup: LabGroup.Messaging, - configOverridesSetting: true, - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, displayName: _td("labs|voice_broadcast"), default: false, }, @@ -481,8 +473,8 @@ export const SETTINGS: { [setting: string]: ISetting } = { [Features.OidcNativeFlow]: { isFeature: true, labsGroup: LabGroup.Developer, - configOverridesSetting: true, - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, displayName: _td("labs|oidc_native_flow"), description: _td("labs|oidc_native_flow_description"), default: false, @@ -491,7 +483,6 @@ export const SETTINGS: { [setting: string]: ISetting } = { // use the rust matrix-sdk-crypto-wasm for crypto. isFeature: true, labsGroup: LabGroup.Developer, - // unlike most features, `configOverridesSetting` is false here. supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|rust_crypto"), description: () => { @@ -525,10 +516,10 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_render_reaction_images": { isFeature: true, labsGroup: LabGroup.Messaging, - configOverridesSetting: true, displayName: _td("labs|render_reaction_images"), description: _td("labs|render_reaction_images_description"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, }, /** @@ -607,28 +598,28 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_ask_to_join": { isFeature: true, labsGroup: LabGroup.Rooms, - configOverridesSetting: true, default: false, displayName: _td("labs|ask_to_join"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, }, "feature_new_room_decoration_ui": { isFeature: true, labsGroup: LabGroup.Rooms, - configOverridesSetting: true, displayName: _td("labs|new_room_decoration_ui"), description: _td("labs|under_active_development"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, controller: new ReloadOnChangeController(), }, "feature_notifications": { isFeature: true, labsGroup: LabGroup.Messaging, - configOverridesSetting: true, displayName: _td("labs|notifications"), description: _td("labs|unrealiable_e2e"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, }, "useCompactLayout": { diff --git a/src/settings/SettingsStore.ts b/src/settings/SettingsStore.ts index 83006a02eaa..c5b708aff9f 100644 --- a/src/settings/SettingsStore.ts +++ b/src/settings/SettingsStore.ts @@ -356,13 +356,6 @@ export default class SettingsStore { * Gets a setting's value at a particular level, ignoring all levels that are more specific. * @param {SettingLevel|"config"|"default"} level The * level to look at. - * - * This takes into account the value of {@link IBaseSetting#configOverridesSetting} of the `SettingController`. - * If {@link IBaseSetting#configOverridesSetting} is true, and the config level contains a valid value, - * this value will be read from config level rather than level passed. - * However the `explicit` paramter is respected, in that if true the value will only be read from the level specified. - * In this case it is therefore the responsibilty of the calling code to ensure the config level setting is respected. - * * @param {string} settingName The name of the setting to read. * @param {String} roomId The room ID to read the setting value in, may be null. * @param {boolean} explicit If true, this method will not consider other levels, just the one @@ -377,13 +370,6 @@ export default class SettingsStore { explicit = false, excludeDefault = false, ): any { - // For some config settings (mostly: non-beta features), a value in config.json overrides the local setting - // (ie: we force them as enabled or disabled). In this case we should read the value from the config. - const finalLevel: SettingLevel = - !explicit && level !== SettingLevel.CONFIG && this.doesConfigOverrideSetting(settingName, roomId) - ? SettingLevel.CONFIG - : level; - // Verify that the setting is actually a setting const setting = SETTINGS[settingName]; if (!setting) { @@ -393,8 +379,8 @@ export default class SettingsStore { const levelOrder = getLevelOrder(setting); if (!levelOrder.includes(SettingLevel.DEFAULT)) levelOrder.push(SettingLevel.DEFAULT); // always include default - const minIndex = levelOrder.indexOf(finalLevel); - if (minIndex === -1) throw new Error(`Level "${finalLevel}" for setting "${settingName}" is not prioritized`); + const minIndex = levelOrder.indexOf(level); + if (minIndex === -1) throw new Error(`Level "${level}" for setting "${settingName}" is not prioritized`); const handlers = SettingsStore.getHandlers(settingName); @@ -406,12 +392,12 @@ export default class SettingsStore { } if (explicit) { - const handler = handlers[finalLevel]; + const handler = handlers[level]; if (!handler) { - return SettingsStore.getFinalValue(setting, finalLevel, roomId, null, null); + return SettingsStore.getFinalValue(setting, level, roomId, null, null); } const value = handler.getValue(settingName, roomId); - return SettingsStore.getFinalValue(setting, finalLevel, roomId, value, finalLevel); + return SettingsStore.getFinalValue(setting, level, roomId, value, level); } for (let i = minIndex; i < levelOrder.length; i++) { @@ -421,10 +407,10 @@ export default class SettingsStore { const value = handler.getValue(settingName, roomId); if (value === null || value === undefined) continue; - return SettingsStore.getFinalValue(setting, finalLevel, roomId, value, levelOrder[i]); + return SettingsStore.getFinalValue(setting, level, roomId, value, levelOrder[i]); } - return SettingsStore.getFinalValue(setting, finalLevel, roomId, null, null); + return SettingsStore.getFinalValue(setting, level, roomId, null, null); } /** @@ -519,8 +505,8 @@ export default class SettingsStore { * 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#configOverridesSetting} is true, - * whether the setting has been given a value in `config.json`. + * `SettingController`, if any; and, for settings where {@link IBaseSetting#supportedLevelsAreOrdered} is true, + * checks whether a level of higher precedence is set. * * 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 @@ -528,26 +514,23 @@ export default class SettingsStore { * * @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 - * check at. + * @param {SettingLevel} level The level to check at. * @return {boolean} True if the user may set the setting, false otherwise. */ public static canSetValue(settingName: string, roomId: string | null, level: SettingLevel): boolean { + const setting = SETTINGS[settingName]; // Verify that the setting is actually a setting - if (!SETTINGS[settingName]) { + if (!setting) { throw new Error("Setting '" + settingName + "' does not appear to be a setting."); } - if (SETTINGS[settingName].controller?.settingDisabled) { + if (setting.controller?.settingDisabled) { return false; } - // For some config settings (mostly: non-beta features), a value in config.json overrides the local setting - // (ie: we force them as enabled or disabled). In this case we such not let the user change the setting. - if (this.doesConfigOverrideSetting(settingName, roomId)) { - return false; - } - if (SETTINGS[settingName]?.configOverridesSetting) { + // // For some config settings (mostly: non-beta features), a value in config.json overrides the local setting + // // (ie: we force them as enabled or disabled). In this case we should not let the user change the setting. + if (setting?.supportedLevelsAreOrdered && this.settingIsOveriddenAtAHigherLevel(settingName, roomId, level)) { const configVal = SettingsStore.getValueAt(SettingLevel.CONFIG, settingName, roomId, true, true); if (configVal === true || configVal === false) return false; } @@ -558,15 +541,29 @@ export default class SettingsStore { } /** - * Determines if the given setting's config should override other levels - * @param settingName - * @param roomId + * Determines if the setting at the specified level is overidden by one at a higher level in the level order. + * @param settingName The name of the setting to check. + * @param roomId The room ID to check in, may be null. + * @param level The level to check at. * @returns */ - private static doesConfigOverrideSetting(settingName: string, roomId: string | null): boolean { - if (SETTINGS[settingName]?.configOverridesSetting) { - const configVal = SettingsStore.getValueAt(SettingLevel.CONFIG, settingName, roomId, true, true); - if (configVal === true || configVal === false) return true; + public static settingIsOveriddenAtAHigherLevel( + settingName: string, + roomId: string | null, + level: SettingLevel, + ): boolean { + const setting = SETTINGS[settingName]; + const levelOrders = getLevelOrder(setting); + const maxLevel = levelOrders.indexOf(level); + if (maxLevel === -1) throw new Error(`Level "${level}" for setting "${settingName}" is not prioritized`); + + const handlers = SettingsStore.getHandlers(settingName); + for (let i = 0; i < maxLevel; i++) { + const handler = handlers[levelOrders[i]]; + if (!handler) continue; + const value = handler.getValue(settingName, roomId); + if (value === null || value === undefined) continue; + return true; } return false; } diff --git a/test/settings/SettingsStore-test.ts b/test/settings/SettingsStore-test.ts index c73f4f8244d..e1c27ab309b 100644 --- a/test/settings/SettingsStore-test.ts +++ b/test/settings/SettingsStore-test.ts @@ -28,7 +28,9 @@ const TEST_DATA = [ }, ]; -// An existing setting that has configOverridesSetting set to true. +/** + * An existing setting that has {@link IBaseSetting#supportedLevelsAreOrdered} set to true. + */ const SETTING_NAME_WITH_CONFIG_OVERRIDE = "feature_new_room_decoration_ui"; describe("SettingsStore", () => { @@ -67,18 +69,17 @@ describe("SettingsStore", () => { }); }); - it(`configOverridesSetting correctly overrides setting`, async () => { + it(`supportedLevelsAreOrdered correctly overrides setting`, async () => { SdkConfig.put({ features: { [SETTING_NAME_WITH_CONFIG_OVERRIDE]: false, }, }); - await SettingsStore.setValue(SETTING_NAME_WITH_CONFIG_OVERRIDE, null, SettingLevel.DEVICE, true); - expect(SettingsStore.getValueAt(SettingLevel.DEVICE, SETTING_NAME_WITH_CONFIG_OVERRIDE)).toBe(false); + expect(SettingsStore.getValue(SETTING_NAME_WITH_CONFIG_OVERRIDE)).toBe(false); }); - it(`configOverridesSetting doesn't incorrectly override setting`, async () => { + it(`supportedLevelsAreOrdered doesn't incorrectly override setting`, async () => { await SettingsStore.setValue(SETTING_NAME_WITH_CONFIG_OVERRIDE, null, SettingLevel.DEVICE, true); expect(SettingsStore.getValueAt(SettingLevel.DEVICE, SETTING_NAME_WITH_CONFIG_OVERRIDE)).toBe(true); }); From eeb2795c30ea6588069290a965ba62b97545cca8 Mon Sep 17 00:00:00 2001 From: David Langley Date: Tue, 11 Jun 2024 09:31:55 +0100 Subject: [PATCH 07/10] Fix typos --- src/components/views/elements/SettingsFlag.tsx | 3 ++- src/settings/SettingsStore.ts | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/components/views/elements/SettingsFlag.tsx b/src/components/views/elements/SettingsFlag.tsx index 8fc2f283d56..f2d176ec403 100644 --- a/src/components/views/elements/SettingsFlag.tsx +++ b/src/components/views/elements/SettingsFlag.tsx @@ -62,7 +62,7 @@ export default class SettingsFlag extends React.Component { } private getSettingValue(): boolean { - // If a level defined in props is overridden by level at a high presenence, it gets disabled + // If a level defined in props is overridden by a level at a high presedence, it gets disabled // and we should show the overridding value. if ( !!SettingsStore.settingIsOveriddenAtAHigherLevel( @@ -107,6 +107,7 @@ export default class SettingsFlag extends React.Component { public render(): React.ReactNode { const disabled = !SettingsStore.canSetValue(this.props.name, this.props.roomId ?? null, this.props.level); + if (disabled && this.props.hideIfCannotSet) return null; const label = this.props.label ?? SettingsStore.getDisplayName(this.props.name, this.props.level); diff --git a/src/settings/SettingsStore.ts b/src/settings/SettingsStore.ts index c5b708aff9f..bce77aabcdb 100644 --- a/src/settings/SettingsStore.ts +++ b/src/settings/SettingsStore.ts @@ -528,8 +528,8 @@ export default class SettingsStore { return false; } - // // For some config settings (mostly: non-beta features), a value in config.json overrides the local setting - // // (ie: we force them as enabled or disabled). In this case we should not let the user change the setting. + // For some config settings (mostly: non-beta features), a value in config.json overrides the local setting + // (ie: we force them as enabled or disabled). In this case we should not let the user change the setting. if (setting?.supportedLevelsAreOrdered && this.settingIsOveriddenAtAHigherLevel(settingName, roomId, level)) { const configVal = SettingsStore.getValueAt(SettingLevel.CONFIG, settingName, roomId, true, true); if (configVal === true || configVal === false) return false; From c4e89319d49e5ec61de1e7e185506675884e2320 Mon Sep 17 00:00:00 2001 From: David Langley Date: Tue, 11 Jun 2024 11:58:13 +0100 Subject: [PATCH 08/10] Fix mock in UserSetttingsDialog-test --- src/settings/SettingsStore.ts | 5 ++++- test/components/views/dialogs/UserSettingsDialog-test.tsx | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/settings/SettingsStore.ts b/src/settings/SettingsStore.ts index bce77aabcdb..e08fa64967d 100644 --- a/src/settings/SettingsStore.ts +++ b/src/settings/SettingsStore.ts @@ -530,7 +530,10 @@ export default class SettingsStore { // For some config settings (mostly: non-beta features), a value in config.json overrides the local setting // (ie: we force them as enabled or disabled). In this case we should not let the user change the setting. - if (setting?.supportedLevelsAreOrdered && this.settingIsOveriddenAtAHigherLevel(settingName, roomId, level)) { + if ( + setting?.supportedLevelsAreOrdered && + SettingsStore.settingIsOveriddenAtAHigherLevel(settingName, roomId, level) + ) { const configVal = SettingsStore.getValueAt(SettingLevel.CONFIG, settingName, roomId, true, true); if (configVal === true || configVal === false) return false; } diff --git a/test/components/views/dialogs/UserSettingsDialog-test.tsx b/test/components/views/dialogs/UserSettingsDialog-test.tsx index 72232d5e1b5..e3586ddc07c 100644 --- a/test/components/views/dialogs/UserSettingsDialog-test.tsx +++ b/test/components/views/dialogs/UserSettingsDialog-test.tsx @@ -54,6 +54,7 @@ jest.mock("../../../../src/settings/SettingsStore", () => ({ getDescription: jest.fn(), shouldHaveWarning: jest.fn(), disabledMessage: jest.fn(), + settingIsOveriddenAtAHigherLevel: jest.fn(), })); jest.mock("../../../../src/SdkConfig", () => ({ From bedf253e0ce50d5858d13731f70696829d50396a Mon Sep 17 00:00:00 2001 From: David Langley Date: Thu, 13 Jun 2024 16:39:18 +0100 Subject: [PATCH 09/10] Special case disabling of setting tos use config overrides. --- .../views/elements/SettingsFlag.tsx | 2 +- src/settings/SettingsStore.ts | 30 +++++++++---------- .../views/dialogs/UserSettingsDialog-test.tsx | 2 +- 3 files changed, 16 insertions(+), 18 deletions(-) diff --git a/src/components/views/elements/SettingsFlag.tsx b/src/components/views/elements/SettingsFlag.tsx index f2d176ec403..293a112f122 100644 --- a/src/components/views/elements/SettingsFlag.tsx +++ b/src/components/views/elements/SettingsFlag.tsx @@ -65,7 +65,7 @@ export default class SettingsFlag extends React.Component { // If a level defined in props is overridden by a level at a high presedence, it gets disabled // and we should show the overridding value. if ( - !!SettingsStore.settingIsOveriddenAtAHigherLevel( + !!SettingsStore.settingIsOveriddenAtConfigLevel( this.props.name, this.props.roomId ?? null, this.props.level, diff --git a/src/settings/SettingsStore.ts b/src/settings/SettingsStore.ts index e08fa64967d..99fe2fe6348 100644 --- a/src/settings/SettingsStore.ts +++ b/src/settings/SettingsStore.ts @@ -532,10 +532,9 @@ export default class SettingsStore { // (ie: we force them as enabled or disabled). In this case we should not let the user change the setting. if ( setting?.supportedLevelsAreOrdered && - SettingsStore.settingIsOveriddenAtAHigherLevel(settingName, roomId, level) + SettingsStore.settingIsOveriddenAtConfigLevel(settingName, roomId, level) ) { - const configVal = SettingsStore.getValueAt(SettingLevel.CONFIG, settingName, roomId, true, true); - if (configVal === true || configVal === false) return false; + return false; } const handler = SettingsStore.getHandler(settingName, level); @@ -544,31 +543,30 @@ export default class SettingsStore { } /** - * Determines if the setting at the specified level is overidden by one at a higher level in the level order. + * Determines if the setting at the specified level is overidden by one at a config level. * @param settingName The name of the setting to check. * @param roomId The room ID to check in, may be null. * @param level The level to check at. * @returns */ - public static settingIsOveriddenAtAHigherLevel( + public static settingIsOveriddenAtConfigLevel( settingName: string, roomId: string | null, level: SettingLevel, ): boolean { const setting = SETTINGS[settingName]; const levelOrders = getLevelOrder(setting); - const maxLevel = levelOrders.indexOf(level); - if (maxLevel === -1) throw new Error(`Level "${level}" for setting "${settingName}" is not prioritized`); - - const handlers = SettingsStore.getHandlers(settingName); - for (let i = 0; i < maxLevel; i++) { - const handler = handlers[levelOrders[i]]; - if (!handler) continue; - const value = handler.getValue(settingName, roomId); - if (value === null || value === undefined) continue; - return true; + const configIndex = levelOrders.indexOf(SettingLevel.CONFIG); + const levelIndex = levelOrders.indexOf(level); + console.log("settingIsOveriddenAtConfigLevel"); + console.log(settingName); + console.log(configIndex); + console.log(levelIndex); + if (configIndex === -1 || levelIndex === -1 || configIndex >= levelIndex) { + return false; } - return false; + const configVal = SettingsStore.getValueAt(SettingLevel.CONFIG, settingName, roomId, true, true); + return configVal === true || configVal === false; } /** diff --git a/test/components/views/dialogs/UserSettingsDialog-test.tsx b/test/components/views/dialogs/UserSettingsDialog-test.tsx index e3586ddc07c..f404b7f208f 100644 --- a/test/components/views/dialogs/UserSettingsDialog-test.tsx +++ b/test/components/views/dialogs/UserSettingsDialog-test.tsx @@ -54,7 +54,7 @@ jest.mock("../../../../src/settings/SettingsStore", () => ({ getDescription: jest.fn(), shouldHaveWarning: jest.fn(), disabledMessage: jest.fn(), - settingIsOveriddenAtAHigherLevel: jest.fn(), + settingIsOveriddenAtConfigLevel: jest.fn(), })); jest.mock("../../../../src/SdkConfig", () => ({ From d8955dec01a55b2eaaa7f4c0b42bb674e55849e3 Mon Sep 17 00:00:00 2001 From: David Langley Date: Fri, 14 Jun 2024 09:46:56 +0100 Subject: [PATCH 10/10] remove logs --- src/components/views/elements/SettingsFlag.tsx | 6 +----- src/settings/SettingsStore.ts | 4 ---- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/src/components/views/elements/SettingsFlag.tsx b/src/components/views/elements/SettingsFlag.tsx index 293a112f122..86b82c64e44 100644 --- a/src/components/views/elements/SettingsFlag.tsx +++ b/src/components/views/elements/SettingsFlag.tsx @@ -65,11 +65,7 @@ export default class SettingsFlag extends React.Component { // If a level defined in props is overridden by a level at a high presedence, it gets disabled // and we should show the overridding value. if ( - !!SettingsStore.settingIsOveriddenAtConfigLevel( - this.props.name, - this.props.roomId ?? null, - this.props.level, - ) + SettingsStore.settingIsOveriddenAtConfigLevel(this.props.name, this.props.roomId ?? null, this.props.level) ) { return !!SettingsStore.getValue(this.props.name); } diff --git a/src/settings/SettingsStore.ts b/src/settings/SettingsStore.ts index 99fe2fe6348..2afe03424c3 100644 --- a/src/settings/SettingsStore.ts +++ b/src/settings/SettingsStore.ts @@ -558,10 +558,6 @@ export default class SettingsStore { const levelOrders = getLevelOrder(setting); const configIndex = levelOrders.indexOf(SettingLevel.CONFIG); const levelIndex = levelOrders.indexOf(level); - console.log("settingIsOveriddenAtConfigLevel"); - console.log(settingName); - console.log(configIndex); - console.log(levelIndex); if (configIndex === -1 || levelIndex === -1 || configIndex >= levelIndex) { return false; }