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

Cleanup tasks in the Settings code #12125

Merged
merged 5 commits into from
Jan 11, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 2 additions & 12 deletions src/components/views/elements/SettingsFlag.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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<IProps, IState> {
Expand All @@ -52,7 +50,6 @@ export default class SettingsFlag extends React.Component<IProps, IState> {

this.state = {
value: this.getSettingValue(),
disabled: this.isSettingDisabled(),
};
}

Expand All @@ -72,15 +69,9 @@ export default class SettingsFlag extends React.Component<IProps, IState> {
this.props.isExplicit,
);
}

private isSettingDisabled(): boolean {
return !SettingsStore.isEnabled(this.props.name);
}

private onSettingChange = (): void => {
this.setState({
value: this.getSettingValue(),
disabled: this.isSettingDisabled(),
});
};

Expand All @@ -104,14 +95,13 @@ export default class SettingsFlag extends React.Component<IProps, IState> {
};

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;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note that it was impossible for canChange to be true while state.disabled was true, so this was equivalent to const disabled = !canChange;.


if (this.props.useCheckbox) {
return (
Expand Down
2 changes: 1 addition & 1 deletion src/components/views/settings/CryptographyPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ export default class CryptographyPanel extends React.Component<IProps, IState> {
}

let noSendUnverifiedSetting: JSX.Element | undefined;
if (SettingsStore.isEnabled("blacklistUnverifiedDevices")) {
if (SettingsStore.canSetValue("blacklistUnverifiedDevices", null, SettingLevel.DEVICE)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is at true if we can set the blacklistUnverifiedDevices settings on this device ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Which makes sense, because that's what's going to happen when you toggle the SettingsFlag.

noSendUnverifiedSetting = (
<SettingsFlag
name="blacklistUnverifiedDevices"
Expand Down
2 changes: 1 addition & 1 deletion src/components/views/settings/E2eAdvancedPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,5 +38,5 @@ const E2eAdvancedPanel: React.FC = () => {
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);
}
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,10 @@ export default class SecurityRoomSettingsTab extends React.Component<IProps, ISt
const canEnableEncryption = !isEncrypted && !isEncryptionForceDisabled && hasEncryptionPermission;

let encryptionSettings: JSX.Element | undefined;
if (isEncrypted && SettingsStore.isEnabled("blacklistUnverifiedDevices")) {
if (
isEncrypted &&
SettingsStore.canSetValue("blacklistUnverifiedDevices", this.props.room.roomId, SettingLevel.ROOM_DEVICE)
) {
encryptionSettings = (
<SettingsFlag
name="blacklistUnverifiedDevices"
Expand Down
18 changes: 5 additions & 13 deletions src/settings/SettingsStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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`,
Expand Down Expand Up @@ -516,6 +503,11 @@ 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.
*
* 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});

Expand Down