From 37ec7e6f7edea88feaa5f94f1d1868363f7f5c51 Mon Sep 17 00:00:00 2001
From: Travis Ralston <travpc@gmail.com>
Date: Thu, 28 Nov 2019 13:40:28 -0700
Subject: [PATCH] Ensure the settings page accurately represents theme choices

Fixes https://github.com/vector-im/riot-web/issues/11518

This also fixes a bug where the the theme logic can incorrectly get stuck in the light theme - the ThemeController was overriding *all* values, not just supposed themes. Null values aren't overridden so that the various theme logic bits don't assume the user has chosen the light theme explicitly.
---
 .../tabs/user/GeneralUserSettingsTab.js       | 49 ++++++++++++++++---
 src/settings/controllers/ThemeController.js   |  2 +
 src/theme.js                                  |  2 +
 3 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/src/components/views/settings/tabs/user/GeneralUserSettingsTab.js b/src/components/views/settings/tabs/user/GeneralUserSettingsTab.js
index b518f7c81b..cae4b19891 100644
--- a/src/components/views/settings/tabs/user/GeneralUserSettingsTab.js
+++ b/src/components/views/settings/tabs/user/GeneralUserSettingsTab.js
@@ -49,8 +49,6 @@ export default class GeneralUserSettingsTab extends React.Component {
 
         this.state = {
             language: languageHandler.getCurrentLanguage(),
-            theme: SettingsStore.getValueAt(SettingLevel.ACCOUNT, "theme"),
-            useSystemTheme: SettingsStore.getValueAt(SettingLevel.DEVICE, "use_system_theme"),
             haveIdServer: Boolean(MatrixClientPeg.get().getIdentityServerUrl()),
             serverSupportsSeparateAddAndBind: null,
             idServerHasUnsignedTerms: false,
@@ -62,6 +60,7 @@ export default class GeneralUserSettingsTab extends React.Component {
             },
             emails: [],
             msisdns: [],
+            ...this._calculateThemeState(),
         };
 
         this.dispatcherRef = dis.register(this._onAction);
@@ -80,6 +79,39 @@ export default class GeneralUserSettingsTab extends React.Component {
         dis.unregister(this.dispatcherRef);
     }
 
+    _calculateThemeState() {
+        // We have to mirror the logic from ThemeWatcher.getEffectiveTheme so we
+        // show the right values for things.
+
+        const themeChoice = SettingsStore.getValueAt(SettingLevel.ACCOUNT, "theme");
+        const systemThemeExplicit = SettingsStore.getValueAt(
+            SettingLevel.DEVICE, "use_system_theme", null, false, true);
+        const themeExplicit = SettingsStore.getValueAt(
+            SettingLevel.DEVICE, "theme", null, false, true);
+
+        // If the user has enabled system theme matching, use that.
+        if (systemThemeExplicit) {
+            return {
+                theme: themeChoice,
+                useSystemTheme: true,
+            };
+        }
+
+        // If the user has set a theme explicitly, use that (no system theme matching)
+        if (themeExplicit) {
+            return {
+                theme: themeChoice,
+                useSystemTheme: false,
+            };
+        }
+
+        // Otherwise assume the defaults for the settings
+        return {
+            theme: themeChoice,
+            useSystemTheme: SettingsStore.getValueAt(SettingLevel.DEVICE, "use_system_theme"),
+        };
+    }
+
     _onAction = (payload) => {
         if (payload.action === 'id_server_changed') {
             this.setState({haveIdServer: Boolean(MatrixClientPeg.get().getIdentityServerUrl())});
@@ -89,11 +121,11 @@ export default class GeneralUserSettingsTab extends React.Component {
 
     _onEmailsChange = (emails) => {
         this.setState({ emails });
-    }
+    };
 
     _onMsisdnsChange = (msisdns) => {
         this.setState({ msisdns });
-    }
+    };
 
     async _getThreepidState() {
         const cli = MatrixClientPeg.get();
@@ -193,9 +225,9 @@ export default class GeneralUserSettingsTab extends React.Component {
 
     _onUseSystemThemeChanged = (checked) => {
         this.setState({useSystemTheme: checked});
+        SettingsStore.setValue("use_system_theme", null, SettingLevel.DEVICE, checked);
         dis.dispatch({action: 'recheck_theme'});
-    }
-
+    };
 
     _onPasswordChangeError = (err) => {
         // TODO: Figure out a design that doesn't involve replacing the current dialog
@@ -307,12 +339,15 @@ export default class GeneralUserSettingsTab extends React.Component {
 
     _renderThemeSection() {
         const SettingsFlag = sdk.getComponent("views.elements.SettingsFlag");
+        const LabelledToggleSwitch = sdk.getComponent("views.elements.LabelledToggleSwitch");
 
         const themeWatcher = new ThemeWatcher();
         let systemThemeSection;
         if (themeWatcher.isSystemThemeSupported()) {
             systemThemeSection = <div>
-                <SettingsFlag name="use_system_theme" level={SettingLevel.DEVICE}
+                <LabelledToggleSwitch
+                    value={this.state.useSystemTheme}
+                    label={SettingsStore.getDisplayName("use_system_theme")}
                     onChange={this._onUseSystemThemeChanged}
                 />
             </div>;
diff --git a/src/settings/controllers/ThemeController.js b/src/settings/controllers/ThemeController.js
index a15b4e78cd..aae43673bf 100644
--- a/src/settings/controllers/ThemeController.js
+++ b/src/settings/controllers/ThemeController.js
@@ -20,6 +20,8 @@ import {DEFAULT_THEME, enumerateThemes} from "../../theme";
 
 export default class ThemeController extends SettingController {
     getValueOverride(level, roomId, calculatedValue, calculatedAtLevel) {
+        if (!calculatedValue) return null; // Don't override null themes
+
         const themes = enumerateThemes();
         // Override in case some no longer supported theme is stored here
         if (!themes[calculatedValue]) {
diff --git a/src/theme.js b/src/theme.js
index 3f50f8ba88..1ba87a2048 100644
--- a/src/theme.js
+++ b/src/theme.js
@@ -80,6 +80,8 @@ export class ThemeWatcher {
     }
 
     getEffectiveTheme() {
+        // Dev note: Much of this logic is replicated in the GeneralUserSettingsTab
+
         // If the user has specifically enabled the system matching option (excluding default),
         // then use that over anything else. We pick the lowest possible level for the setting
         // to ensure the ordering otherwise works.