Use a global WatchManager for settings

Fixes element-hq/element-web#8936

Watchers are now managed by the SettingsStore itself through a global/default watch manager. As per the included documentation, the watch manager dispatches updates to callbacks which are redirected by the SettingsStore for consumer safety.

docs/settings.md

@@ -143,11 +143,8 @@ class MyComponent extends React.Component {
     settingWatcherRef = null;
 
     componentWillMount() {
-        this.settingWatcherRef = SettingsStore.watchSetting("roomColor", "!example:matrix.org", (settingName, roomId, level, newVal) => {
-            // Always re-read the setting value from the store to avoid reacting to changes which do not have a consequence. For example, the
-            // room color could have been changed at the device level, but an account override prevents that change from making a difference.
-           const actualVal = SettingsStore.getValue(settingName, "!example:matrix.org");
-           if (actualVal !== this.state.color) this.setState({color: actualVal});
+        this.settingWatcherRef = SettingsStore.watchSetting("roomColor", "!example:matrix.org", (settingName, roomId, level, newValAtLevel, newVal) => {
+           this.setState({color: newVal});
         });
     }
 
@@ -188,7 +185,9 @@ If `enableLabs` is true in the configuration, the default for features becomes `
 
 ### Watchers
 
-Watchers can appear complicated under the hood: the request to watch a setting is actually forked off to individual handlers for watching. This means that the handlers need to track their changes and listen for remote changes where possible, but also makes it much easier for the `SettingsStore` to react to changes. The handler is going to know the best things to listen for (specific events, account data, etc) and thus it is left as a responsibility for the handler to track changes. 
+Watchers can appear complicated under the hood: there is a central `WatchManager` which handles the actual invocation of callbacks, and callbacks are managed by the SettingsStore by redirecting the caller's callback to a dedicated callback. This is done so that the caller can reuse the same function as their callback without worrying about whether or not it'll unsubscribe all watchers. 
+
+Setting changes are emitted into the default `WatchManager`, which calculates the new value for the setting. Ideally, we'd also try and suppress updates which don't have a consequence on this value, however there's not an easy way to do this. Instead, we just dispatch an update for all changes and leave it up to the consumer to deduplicate. 
 
 In practice, handlers which rely on remote changes (account data, room events, etc) will always attach a listener to the `MatrixClient`. They then watch for changes to events they care about and send off appropriate updates to the generalized `WatchManager` - a class specifically designed to deduplicate the logic of managing watchers. The handlers which are localized to the local client (device) generally just trigger the `WatchManager` when they manipulate the setting themselves as there's nothing to really 'watch'.
 

src/settings/SettingsStore.js

@@ -27,6 +27,7 @@ import SdkConfig from "../SdkConfig";
 import dis from '../dispatcher';
 import {SETTINGS} from "./Settings";
 import LocalEchoWrapper from "./handlers/LocalEchoWrapper";
+import {WatchManager} from "./WatchManager";
 
 /**
  * Represents the various setting levels supported by the SettingsStore.
@@ -43,6 +44,8 @@ export const SettingLevel = {
     DEFAULT: "default",
 };
 
+const defaultWatchManager = new WatchManager();
+
 // Convert the settings to easier to manage objects for the handlers
 const defaultSettings = {};
 const invertedDefaultSettings = {};
@@ -58,11 +61,11 @@ for (const key of Object.keys(SETTINGS)) {
 }
 
 const LEVEL_HANDLERS = {
-    "device": new DeviceSettingsHandler(featureNames),
-    "room-device": new RoomDeviceSettingsHandler(),
-    "room-account": new RoomAccountSettingsHandler(),
-    "account": new AccountSettingsHandler(),
-    "room": new RoomSettingsHandler(),
+    "device": new DeviceSettingsHandler(featureNames, defaultWatchManager),
+    "room-device": new RoomDeviceSettingsHandler(defaultWatchManager),
+    "room-account": new RoomAccountSettingsHandler(defaultWatchManager),
+    "account": new AccountSettingsHandler(defaultWatchManager),
+    "room": new RoomSettingsHandler(defaultWatchManager),
     "config": new ConfigSettingsHandler(),
     "default": new DefaultSettingsHandler(defaultSettings, invertedDefaultSettings),
 };
@@ -100,32 +103,30 @@ const LEVEL_ORDER = [
  * be enabled).
  */
 export default class SettingsStore {
-    // We support watching settings for changes, and do so only at the levels which are
-    // relevant to the setting. We pass the watcher on to the handlers and aggregate it
-    // before sending it off to the caller. We need to track which callback functions we
-    // provide to the handlers though so we can unwatch it on demand. In practice, we
-    // return a "callback reference" to the caller which correlates to an entry in this
-    // dictionary for each handler's callback function.
+    // We support watching settings for changes, and do this by tracking which callbacks have
+    // been given to us. We end up returning the callbackRef to the caller so they can unsubscribe
+    // at a later point.
     //
     // We also maintain a list of monitors which are special watchers: they cause dispatches
     // when the setting changes. We track which rooms we're monitoring though to ensure we
     // don't duplicate updates on the bus.
-    static _watchers = {}; // { callbackRef => { level => callbackFn } }
+    static _watchers = {}; // { callbackRef => { callbackFn } }
     static _monitors = {}; // { settingName => { roomId => callbackRef } }
 
     /**
      * Watches for changes in a particular setting. This is done without any local echo
-     * wrapping and fires whenever a change is detected in a setting's value. Watching
-     * is intended to be used in scenarios where the app needs to react to changes made
-     * by other devices. It is otherwise expected that callers will be able to use the
-     * Controller system or track their own changes to settings. Callers should retain
+     * wrapping and fires whenever a change is detected in a setting's value, at any level.
+     * Watching is intended to be used in scenarios where the app needs to react to changes
+     * made by other devices. It is otherwise expected that callers will be able to use the
+     * Controller system or track their own changes to settings. Callers should retain the
+     * returned reference to later unsubscribe from updates.
      * @param {string} settingName The setting name to watch
      * @param {String} roomId The room ID to watch for changes in. May be null for 'all'.
      * @param {function} callbackFn A function to be called when a setting change is
-     * detected. Four arguments can be expected: the setting name, the room ID (may be null),
-     * the level the change happened at, and finally the new value for those arguments. The
-     * callback may need to do a call to #getValue() to see if a consequential change has
-     * occurred.
+     * detected. Five arguments can be expected: the setting name, the room ID (may be null),
+     * the level the change happened at, the new value at the given level, and finally the new
+     * value for the setting regardless of level. The callback is responsible for determining
+     * if the change in value is worthwhile enough to react upon.
      * @returns {string} A reference to the watcher that was employed.
      */
     static watchSetting(settingName, roomId, callbackFn) {
@@ -138,21 +139,15 @@ export default class SettingsStore {
         }
 
         const watcherId = `${new Date().getTime()}_${settingName}_${roomId}`;
-        SettingsStore._watchers[watcherId] = {};
-
-        const levels = Object.keys(LEVEL_HANDLERS);
-        for (const level of levels) {
-            const handler = SettingsStore._getHandler(originalSettingName, level);
-            if (!handler) continue;
 
-            const localizedCallback = (changedInRoomId, newVal) => {
-                callbackFn(originalSettingName, changedInRoomId, level, newVal);
-            };
+        const localizedCallback = (changedInRoomId, atLevel, newValAtLevel) => {
+            const newValue = SettingsStore.getValue(originalSettingName);
+            callbackFn(originalSettingName, changedInRoomId, atLevel, newValAtLevel, newValue);
+        };
 
-            console.log(`Starting watcher for ${settingName}@${roomId || '<null room>'} at level ${level}`);
-            SettingsStore._watchers[watcherId][level] = localizedCallback;
-            handler.watchSetting(settingName, roomId, localizedCallback);
-        }
+        console.log(`Starting watcher for ${settingName}@${roomId || '<null room>'}`);
+        SettingsStore._watchers[watcherId] = localizedCallback;
+        defaultWatchManager.watchSetting(settingName, roomId, localizedCallback);
 
         return watcherId;
     }
@@ -166,20 +161,15 @@ export default class SettingsStore {
     static unwatchSetting(watcherReference) {
         if (!SettingsStore._watchers[watcherReference]) return;
 
-        for (const handlerName of Object.keys(SettingsStore._watchers[watcherReference])) {
-            const handler = LEVEL_HANDLERS[handlerName];
-            if (!handler) continue;
-            handler.unwatchSetting(SettingsStore._watchers[watcherReference][handlerName]);
-        }
-
+        defaultWatchManager.unwatchSetting(SettingsStore._watchers[watcherReference]);
         delete SettingsStore._watchers[watcherReference];
     }
 
     /**
      * Sets up a monitor for a setting. This behaves similar to #watchSetting except instead
      * of making a call to a callback, it forwards all changes to the dispatcher. Callers can
      * expect to listen for the 'setting_updated' action with an object containing settingName,
-     * roomId, level, and newValue.
+     * roomId, level, newValueAtLevel, and newValue.
      * @param {string} settingName The setting name to monitor.
      * @param {String} roomId The room ID to monitor for changes in. Use null for all rooms.
      */
@@ -188,12 +178,13 @@ export default class SettingsStore {
 
         const registerWatcher = () => {
             this._monitors[settingName][roomId] = SettingsStore.watchSetting(
-                settingName, roomId, (settingName, inRoomId, level, newValue) => {
+                settingName, roomId, (settingName, inRoomId, level, newValueAtLevel, newValue) => {
                     dis.dispatch({
                         action: 'setting_updated',
                         settingName,
                         roomId: inRoomId,
                         level,
+                        newValueAtLevel,
                         newValue,
                     });
                 },

src/settings/WatchManager.js

@@ -41,7 +41,11 @@ export class WatchManager {
         }
     }
 
-    notifyUpdate(settingName, inRoomId, newValue) {
+    notifyUpdate(settingName, inRoomId, atLevel, newValueAtLevel) {
+        // Dev note: We could avoid raising changes for ultimately inconsequential changes, but
+        // we also don't have a reliable way to get the old value of a setting. Instead, we'll just
+        // let it fall through regardless and let the receiver dedupe if they want to.
+
         if (!this._watchers[settingName]) return;
 
         const roomWatchers = this._watchers[settingName];
@@ -51,7 +55,7 @@ export class WatchManager {
         if (roomWatchers[null]) callbacks.push(...roomWatchers[null]);
 
         for (const callback of callbacks) {
-            callback(inRoomId, newValue);
+            callback(inRoomId, atLevel, newValueAtLevel);
         }
     }
 }

src/settings/handlers/AccountSettingsHandler.js

@@ -16,18 +16,18 @@ limitations under the License.
 */
 
 import MatrixClientPeg from '../../MatrixClientPeg';
-import {WatchManager} from "../WatchManager";
 import MatrixClientBackedSettingsHandler from "./MatrixClientBackedSettingsHandler";
+import {SettingLevel} from "../SettingsStore";
 
 /**
  * Gets and sets settings at the "account" level for the current user.
  * This handler does not make use of the roomId parameter.
  */
 export default class AccountSettingsHandler extends MatrixClientBackedSettingsHandler {
-    constructor() {
+    constructor(watchManager) {
         super();
 
-        this._watchers = new WatchManager();
+        this._watchers = watchManager;
         this._onAccountData = this._onAccountData.bind(this);
     }
 
@@ -48,11 +48,12 @@ export default class AccountSettingsHandler extends MatrixClientBackedSettingsHa
                 val = !val;
             }
 
-            this._watchers.notifyUpdate("urlPreviewsEnabled", null, val);
+            this._watchers.notifyUpdate("urlPreviewsEnabled", null, SettingLevel.ACCOUNT, val);
         } else if (event.getType() === "im.vector.web.settings") {
             // We can't really discern what changed, so trigger updates for everything
             for (const settingName of Object.keys(event.getContent())) {
-                this._watchers.notifyUpdate(settingName, null, event.getContent()[settingName]);
+                const val = event.getContent()[settingName];
+                this._watchers.notifyUpdate(settingName, null, SettingLevel.ACCOUNT, val);
             }
         }
     }
@@ -102,14 +103,6 @@ export default class AccountSettingsHandler extends MatrixClientBackedSettingsHa
         return cli !== undefined && cli !== null;
     }
 
-    watchSetting(settingName, roomId, cb) {
-        this._watchers.watchSetting(settingName, roomId, cb);
-    }
-
-    unwatchSetting(cb) {
-        this._watchers.unwatchSetting(cb);
-    }
-
     _getSettings(eventType = "im.vector.web.settings") {
         const cli = MatrixClientPeg.get();
         if (!cli) return null;

src/settings/handlers/ConfigSettingsHandler.js

@@ -47,12 +47,4 @@ export default class ConfigSettingsHandler extends SettingsHandler {
     isSupported() {
         return true; // SdkConfig is always there
     }
-
-    watchSetting(settingName, roomId, cb) {
-        // no-op: no changes possible
-    }
-
-    unwatchSetting(cb) {
-        // no-op: no changes possible
-    }
 }

src/settings/handlers/DefaultSettingsHandler.js

@@ -52,12 +52,4 @@ export default class DefaultSettingsHandler extends SettingsHandler {
     isSupported() {
         return true;
     }
-
-    watchSetting(settingName, roomId, cb) {
-        // no-op: no changes possible
-    }
-
-    unwatchSetting(cb) {
-        // no-op: no changes possible
-    }
 }

src/settings/handlers/DeviceSettingsHandler.js

@@ -19,6 +19,7 @@ import Promise from 'bluebird';
 import SettingsHandler from "./SettingsHandler";
 import MatrixClientPeg from "../../MatrixClientPeg";
 import {WatchManager} from "../WatchManager";
+import {SettingLevel} from "../SettingsStore";
 
 /**
  * Gets and sets settings at the "device" level for the current device.
@@ -29,11 +30,12 @@ export default class DeviceSettingsHandler extends SettingsHandler {
     /**
      * Creates a new device settings handler
      * @param {string[]} featureNames The names of known features.
+     * @param {WatchManager} watchManager The watch manager to notify updates to
      */
-    constructor(featureNames) {
+    constructor(featureNames, watchManager) {
         super();
         this._featureNames = featureNames;
-        this._watchers = new WatchManager();
+        this._watchers = watchManager;
     }
 
     getValue(settingName, roomId) {
@@ -69,22 +71,22 @@ export default class DeviceSettingsHandler extends SettingsHandler {
         // Special case notifications
         if (settingName === "notificationsEnabled") {
             localStorage.setItem("notifications_enabled", newValue);
-            this._watchers.notifyUpdate(settingName, null, newValue);
+            this._watchers.notifyUpdate(settingName, null, SettingLevel.DEVICE, newValue);
             return Promise.resolve();
         } else if (settingName === "notificationBodyEnabled") {
             localStorage.setItem("notifications_body_enabled", newValue);
-            this._watchers.notifyUpdate(settingName, null, newValue);
+            this._watchers.notifyUpdate(settingName, null, SettingLevel.DEVICE, newValue);
             return Promise.resolve();
         } else if (settingName === "audioNotificationsEnabled") {
             localStorage.setItem("audio_notifications_enabled", newValue);
-            this._watchers.notifyUpdate(settingName, null, newValue);
+            this._watchers.notifyUpdate(settingName, null, SettingLevel.DEVICE, newValue);
             return Promise.resolve();
         }
 
         const settings = this._getSettings() || {};
         settings[settingName] = newValue;
         localStorage.setItem("mx_local_settings", JSON.stringify(settings));
-        this._watchers.notifyUpdate(settingName, null, newValue);
+        this._watchers.notifyUpdate(settingName, null, SettingLevel.DEVICE, newValue);
 
         return Promise.resolve();
     }

src/settings/handlers/LocalEchoWrapper.js

@@ -67,12 +67,4 @@ export default class LocalEchoWrapper extends SettingsHandler {
     isSupported() {
         return this._handler.isSupported();
     }
-
-    watchSetting(settingName, roomId, cb) {
-        this._handler.watchSetting(settingName, roomId, cb);
-    }
-
-    unwatchSetting(cb) {
-        this._handler.unwatchSetting(cb);
-    }
 }

src/settings/handlers/RoomAccountSettingsHandler.js

@@ -17,16 +17,16 @@ limitations under the License.
 
 import MatrixClientPeg from '../../MatrixClientPeg';
 import MatrixClientBackedSettingsHandler from "./MatrixClientBackedSettingsHandler";
-import {WatchManager} from "../WatchManager";
+import {SettingLevel} from "../SettingsStore";
 
 /**
  * Gets and sets settings at the "room-account" level for the current user.
  */
 export default class RoomAccountSettingsHandler extends MatrixClientBackedSettingsHandler {
-    constructor() {
+    constructor(watchManager) {
         super();
 
-        this._watchers = new WatchManager();
+        this._watchers = watchManager;
         this._onAccountData = this._onAccountData.bind(this);
     }
 
@@ -49,13 +49,14 @@ export default class RoomAccountSettingsHandler extends MatrixClientBackedSettin
                 val = !val;
             }
 
-            this._watchers.notifyUpdate("urlPreviewsEnabled", roomId, val);
+            this._watchers.notifyUpdate("urlPreviewsEnabled", roomId, SettingLevel.ROOM_ACCOUNT, val);
         } else if (event.getType() === "org.matrix.room.color_scheme") {
-            this._watchers.notifyUpdate("roomColor", roomId, event.getContent());
+            this._watchers.notifyUpdate("roomColor", roomId, SettingLevel.ROOM_ACCOUNT, event.getContent());
         } else if (event.getType() === "im.vector.web.settings") {
             // We can't really discern what changed, so trigger updates for everything
             for (const settingName of Object.keys(event.getContent())) {
-                this._watchers.notifyUpdate(settingName, roomId, event.getContent()[settingName]);
+                const val = event.getContent()[settingName];
+                this._watchers.notifyUpdate(settingName, roomId, SettingLevel.ROOM_ACCOUNT, val);
             }
         }
     }
@@ -113,14 +114,6 @@ export default class RoomAccountSettingsHandler extends MatrixClientBackedSettin
         return cli !== undefined && cli !== null;
     }
 
-    watchSetting(settingName, roomId, cb) {
-        this._watchers.watchSetting(settingName, roomId, cb);
-    }
-
-    unwatchSetting(cb) {
-        this._watchers.unwatchSetting(cb);
-    }
-
     _getSettings(roomId, eventType = "im.vector.web.settings") {
         const room = MatrixClientPeg.get().getRoom(roomId);
         if (!room) return null;