From 93673eff1249b8688e7861e6dc9e2be1f0c35eef Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 26 Feb 2019 12:43:10 -0700 Subject: [PATCH 1/2] Use a global WatchManager for settings Fixes https://github.com/vector-im/riot-web/issues/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 | 11 ++- src/settings/SettingsStore.js | 73 ++++++++----------- src/settings/WatchManager.js | 8 +- .../handlers/AccountSettingsHandler.js | 19 ++--- .../handlers/ConfigSettingsHandler.js | 8 -- .../handlers/DefaultSettingsHandler.js | 8 -- .../handlers/DeviceSettingsHandler.js | 15 ++-- src/settings/handlers/LocalEchoWrapper.js | 8 -- .../handlers/RoomAccountSettingsHandler.js | 21 ++---- .../handlers/RoomDeviceSettingsHandler.js | 18 ++--- src/settings/handlers/RoomSettingsHandler.js | 18 ++--- src/settings/handlers/SettingsHandler.js | 23 ------ src/stores/RoomListStore.js | 55 +++++++++----- 13 files changed, 111 insertions(+), 174 deletions(-) diff --git a/docs/settings.md b/docs/settings.md index 9762e7a73e..8121a09141 100644 --- a/docs/settings.md +++ b/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'. \ No newline at end of file diff --git a/src/settings/SettingsStore.js b/src/settings/SettingsStore.js index 1704ad9db2..9501bac205 100644 --- a/src/settings/SettingsStore.js +++ b/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, atLevel, newValAtLevel) => { + const newValue = SettingsStore.getValue(originalSettingName); + callbackFn(originalSettingName, changedInRoomId, atLevel, newValAtLevel, newValue); + }; - const localizedCallback = (changedInRoomId, newVal) => { - callbackFn(originalSettingName, changedInRoomId, level, newVal); - }; - - console.log(`Starting watcher for ${settingName}@${roomId || ''} at level ${level}`); - SettingsStore._watchers[watcherId][level] = localizedCallback; - handler.watchSetting(settingName, roomId, localizedCallback); - } + console.log(`Starting watcher for ${settingName}@${roomId || ''}`); + SettingsStore._watchers[watcherId] = localizedCallback; + defaultWatchManager.watchSetting(settingName, roomId, localizedCallback); return watcherId; } @@ -166,12 +161,7 @@ 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]; } @@ -179,7 +169,7 @@ export default class SettingsStore { * 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, }); }, diff --git a/src/settings/WatchManager.js b/src/settings/WatchManager.js index 0561529392..472b13966f 100644 --- a/src/settings/WatchManager.js +++ b/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); } } } diff --git a/src/settings/handlers/AccountSettingsHandler.js b/src/settings/handlers/AccountSettingsHandler.js index 3bffc9151e..675b2f8ead 100644 --- a/src/settings/handlers/AccountSettingsHandler.js +++ b/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; diff --git a/src/settings/handlers/ConfigSettingsHandler.js b/src/settings/handlers/ConfigSettingsHandler.js index 095347a542..a54ad1cef6 100644 --- a/src/settings/handlers/ConfigSettingsHandler.js +++ b/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 - } } diff --git a/src/settings/handlers/DefaultSettingsHandler.js b/src/settings/handlers/DefaultSettingsHandler.js index 83824850b3..37bc5b2348 100644 --- a/src/settings/handlers/DefaultSettingsHandler.js +++ b/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 - } } diff --git a/src/settings/handlers/DeviceSettingsHandler.js b/src/settings/handlers/DeviceSettingsHandler.js index 457fb888e9..bca7679a47 100644 --- a/src/settings/handlers/DeviceSettingsHandler.js +++ b/src/settings/handlers/DeviceSettingsHandler.js @@ -18,7 +18,7 @@ limitations under the License. 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 +29,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 +70,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(); } diff --git a/src/settings/handlers/LocalEchoWrapper.js b/src/settings/handlers/LocalEchoWrapper.js index 3b1200f0b7..e6964f9bf7 100644 --- a/src/settings/handlers/LocalEchoWrapper.js +++ b/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); - } } diff --git a/src/settings/handlers/RoomAccountSettingsHandler.js b/src/settings/handlers/RoomAccountSettingsHandler.js index 448b42f61e..3c8a1f9941 100644 --- a/src/settings/handlers/RoomAccountSettingsHandler.js +++ b/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; diff --git a/src/settings/handlers/RoomDeviceSettingsHandler.js b/src/settings/handlers/RoomDeviceSettingsHandler.js index 710c5e6255..a0981ffbab 100644 --- a/src/settings/handlers/RoomDeviceSettingsHandler.js +++ b/src/settings/handlers/RoomDeviceSettingsHandler.js @@ -17,17 +17,17 @@ limitations under the License. import Promise from 'bluebird'; import SettingsHandler from "./SettingsHandler"; -import {WatchManager} from "../WatchManager"; +import {SettingLevel} from "../SettingsStore"; /** * Gets and sets settings at the "room-device" level for the current device in a particular * room. */ export default class RoomDeviceSettingsHandler extends SettingsHandler { - constructor() { + constructor(watchManager) { super(); - this._watchers = new WatchManager(); + this._watchers = watchManager; } getValue(settingName, roomId) { @@ -52,7 +52,7 @@ export default class RoomDeviceSettingsHandler extends SettingsHandler { if (!value["blacklistUnverifiedDevicesPerRoom"]) value["blacklistUnverifiedDevicesPerRoom"] = {}; value["blacklistUnverifiedDevicesPerRoom"][roomId] = newValue; localStorage.setItem("mx_local_settings", JSON.stringify(value)); - this._watchers.notifyUpdate(settingName, roomId, newValue); + this._watchers.notifyUpdate(settingName, roomId, SettingLevel.ROOM_DEVICE, newValue); return Promise.resolve(); } @@ -63,7 +63,7 @@ export default class RoomDeviceSettingsHandler extends SettingsHandler { localStorage.setItem(this._getKey(settingName, roomId), newValue); } - this._watchers.notifyUpdate(settingName, roomId, newValue); + this._watchers.notifyUpdate(settingName, roomId, SettingLevel.ROOM_DEVICE, newValue); return Promise.resolve(); } @@ -75,14 +75,6 @@ export default class RoomDeviceSettingsHandler extends SettingsHandler { return localStorage !== undefined && localStorage !== null; } - watchSetting(settingName, roomId, cb) { - this._watchers.watchSetting(settingName, roomId, cb); - } - - unwatchSetting(cb) { - this._watchers.unwatchSetting(cb); - } - _read(key) { const rawValue = localStorage.getItem(key); if (!rawValue) return null; diff --git a/src/settings/handlers/RoomSettingsHandler.js b/src/settings/handlers/RoomSettingsHandler.js index 1622b44dd0..79626e2186 100644 --- a/src/settings/handlers/RoomSettingsHandler.js +++ b/src/settings/handlers/RoomSettingsHandler.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" level. */ export default class RoomSettingsHandler extends MatrixClientBackedSettingsHandler { - constructor() { + constructor(watchManager) { super(); - this._watchers = new WatchManager(); + this._watchers = watchManager; this._onEvent = this._onEvent.bind(this); } @@ -49,11 +49,11 @@ export default class RoomSettingsHandler extends MatrixClientBackedSettingsHandl val = !val; } - this._watchers.notifyUpdate("urlPreviewsEnabled", roomId, val); + this._watchers.notifyUpdate("urlPreviewsEnabled", roomId, SettingLevel.ROOM, 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, roomId, event.getContent()[settingName]); + this._watchers.notifyUpdate(settingName, roomId, SettingLevel.ROOM, event.getContent()[settingName]); } } } @@ -101,14 +101,6 @@ export default class RoomSettingsHandler extends MatrixClientBackedSettingsHandl 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; diff --git a/src/settings/handlers/SettingsHandler.js b/src/settings/handlers/SettingsHandler.js index 0a704d5be7..d1566d6bfa 100644 --- a/src/settings/handlers/SettingsHandler.js +++ b/src/settings/handlers/SettingsHandler.js @@ -69,27 +69,4 @@ export default class SettingsHandler { isSupported() { return false; } - - /** - * Watches for a setting change within this handler. The caller should preserve - * a reference to the callback so that it may be unwatched. The caller should - * additionally provide a unique callback for multiple watchers on the same setting. - * @param {string} settingName The setting name to watch for changes in. - * @param {String} roomId The room ID to watch for changes in. - * @param {function} cb A function taking two arguments: the room ID the setting changed - * in and the new value for the setting at this level in the given room. - */ - watchSetting(settingName, roomId, cb) { - throw new Error("Invalid operation: watchSetting was not overridden"); - } - - /** - * Unwatches a previously watched setting. If the callback is not associated with - * a watcher, this is a no-op. - * @param {function} cb A callback function previously supplied to watchSetting - * which should no longer be used. - */ - unwatchSetting(cb) { - throw new Error("Invalid operation: unwatchSetting was not overridden"); - } } diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index 082535543c..63d18eca23 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -46,6 +46,20 @@ const LIST_ORDERS = { "im.vector.fake.archived": "recent", }; +/** + * Identifier for the "breadcrumb" (or "sort by most important room first") algorithm. + * Includes a provision for keeping the currently open room from flying down the room + * list. + * @type {string} + */ +const ALGO_IMPORTANCE = "importance"; + +/** + * Identifier for classic sorting behaviour: sort by the most recent message first. + * @type {string} + */ +const ALGO_RECENT = "recent"; + /** * A class for storing application state for categorising rooms in * the RoomList. @@ -60,19 +74,18 @@ class RoomListStore extends Store { } /** - * Alerts the RoomListStore to a potential change in how room list sorting should - * behave. - * @param {boolean} forceRegeneration True to force a change in the algorithm + * Changes the sorting algorithm used by the RoomListStore. + * @param {string} algorithm The new algorithm to use. Should be one of the ALGO_* constants. */ - updateSortingAlgorithm(forceRegeneration = false) { - const byImportance = SettingsStore.getValue("RoomList.orderByImportance"); - if (byImportance !== this._state.orderRoomsByImportance || forceRegeneration) { - console.log("Updating room sorting algorithm: sortByImportance=" + byImportance); - this._setState({orderRoomsByImportance: byImportance}); + updateSortingAlgorithm(algorithm) { + // Dev note: We only have two algorithms at the moment, but it isn't impossible that we want + // multiple in the future. Also constants make things slightly clearer. + const byImportance = algorithm === ALGO_IMPORTANCE; + console.log("Updating room sorting algorithm: sortByImportance=" + byImportance); + this._setState({orderRoomsByImportance: byImportance}); - // Trigger a resort of the entire list to reflect the change in algorithm - this._generateInitialRoomLists(); - } + // Trigger a resort of the entire list to reflect the change in algorithm + this._generateInitialRoomLists(); } _init() { @@ -97,6 +110,7 @@ class RoomListStore extends Store { }; SettingsStore.monitorSetting('RoomList.orderByImportance', null); + SettingsStore.monitorSetting('feature_custom_tags', null); } _setState(newState) { @@ -120,13 +134,10 @@ class RoomListStore extends Store { switch (payload.action) { case 'setting_updated': { if (payload.settingName === 'RoomList.orderByImportance') { - this.updateSortingAlgorithm(); + this.updateSortingAlgorithm(payload.newValue === true ? ALGO_IMPORTANCE : ALGO_RECENT); } else if (payload.settingName === 'feature_custom_tags') { - const isActive = SettingsStore.isFeatureEnabled("feature_custom_tags"); - if (isActive !== this._state.tagsEnabled) { - this._setState({tagsEnabled: isActive}); - this.updateSortingAlgorithm(/*force=*/true); - } + this._setState({tagsEnabled: payload.newValue}); + this._generateInitialRoomLists(); // Tags means we have to start from scratch } } break; @@ -139,7 +150,10 @@ class RoomListStore extends Store { this._setState({tagsEnabled: SettingsStore.isFeatureEnabled("feature_custom_tags")}); this._matrixClient = payload.matrixClient; - this.updateSortingAlgorithm(/*force=*/true); + + const algorithm = SettingsStore.getValue("RoomList.orderByImportance") + ? ALGO_IMPORTANCE : ALGO_RECENT; + this.updateSortingAlgorithm(algorithm); } break; case 'MatrixActions.Room.receipt': { @@ -445,6 +459,11 @@ class RoomListStore extends Store { } _generateInitialRoomLists() { + // Log something to show that we're throwing away the old results. This is for the inevitable + // question of "why is 100% of my CPU going towards Riot?" - a quick look at the logs would reveal + // that something is wrong with the RoomListStore. + console.log("Generating initial room lists"); + const lists = { "m.server_notice": [], "im.vector.fake.invite": [], From fbffd3e97e2689175942b39afee75513ab7a737f Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 26 Feb 2019 12:47:20 -0700 Subject: [PATCH 2/2] Make the settings documentation fit within 120 characters per line --- docs/settings.md | 132 ++++++++++++++++++++++++++++++++++++----------- 1 file changed, 101 insertions(+), 31 deletions(-) diff --git a/docs/settings.md b/docs/settings.md index 8121a09141..c88888663b 100644 --- a/docs/settings.md +++ b/docs/settings.md @@ -1,11 +1,15 @@ # Settings Reference -This document serves as developer documentation for using "Granular Settings". Granular Settings allow users to specify different values for a setting at particular levels of interest. For example, a user may say that in a particular room they want URL previews off, but in all other rooms they want them enabled. The `SettingsStore` helps mask the complexity of dealing with the different levels and exposes easy to use getters and setters. +This document serves as developer documentation for using "Granular Settings". Granular Settings allow users to specify +different values for a setting at particular levels of interest. For example, a user may say that in a particular room +they want URL previews off, but in all other rooms they want them enabled. The `SettingsStore` helps mask the complexity +of dealing with the different levels and exposes easy to use getters and setters. ## Levels -Granular Settings rely on a series of known levels in order to use the correct value for the scenario. These levels, in order of prioirty, are: +Granular Settings rely on a series of known levels in order to use the correct value for the scenario. These levels, in +order of prioirty, are: * `device` - The current user's device * `room-device` - The current user's device, but only when in a specific room * `room-account` - The current user's account, but only when in a specific room @@ -14,12 +18,14 @@ Granular Settings rely on a series of known levels in order to use the correct v * `config` - Values are defined by `config.json` * `default` - The hardcoded default for the settings -Individual settings may control which levels are appropriate for them as part of the defaults. This is often to ensure that room administrators cannot force account-only settings upon participants. +Individual settings may control which levels are appropriate for them as part of the defaults. This is often to ensure +that room administrators cannot force account-only settings upon participants. ## Settings -Settings are the different options a user may set or experience in the application. These are pre-defined in `src/settings/Settings.js` under the `SETTINGS` constant and have the following minimum requirements: +Settings are the different options a user may set or experience in the application. These are pre-defined in +`src/settings/Settings.js` under the `SETTINGS` constant and have the following minimum requirements: ``` // The ID is used to reference the setting throughout the application. This must be unique. "theSettingId": { @@ -47,13 +53,21 @@ Settings are the different options a user may set or experience in the applicati ### Getting values for a setting -After importing `SettingsStore`, simply make a call to `SettingsStore.getValue`. The `roomId` parameter should always be supplied where possible, even if the setting does not have a per-room level value. This is to ensure that the value returned is best represented in the room, particularly if the setting ever gets a per-room level in the future. +After importing `SettingsStore`, simply make a call to `SettingsStore.getValue`. The `roomId` parameter should always +be supplied where possible, even if the setting does not have a per-room level value. This is to ensure that the value +returned is best represented in the room, particularly if the setting ever gets a per-room level in the future. -In settings pages it is often desired to have the value at a particular level instead of getting the calculated value. Call `SettingsStore.getValueAt` to get the value of a setting at a particular level, and optionally make it explicitly at that level. By default `getValueAt` will traverse the tree starting at the provided level; making it explicit means it will not go beyond the provided level. When using `getValueAt`, please be sure to use `SettingLevel` to represent the target level. +In settings pages it is often desired to have the value at a particular level instead of getting the calculated value. +Call `SettingsStore.getValueAt` to get the value of a setting at a particular level, and optionally make it explicitly +at that level. By default `getValueAt` will traverse the tree starting at the provided level; making it explicit means +it will not go beyond the provided level. When using `getValueAt`, please be sure to use `SettingLevel` to represent the +target level. ### Setting values for a setting -Values are defined at particular levels and should be done in a safe manner. There are two checks to perform to ensure a clean save: is the level supported and can the user actually set the value. In most cases, neither should be an issue although there are circumstances where this changes. An example of a safe call is: +Values are defined at particular levels and should be done in a safe manner. There are two checks to perform to ensure a +clean save: is the level supported and can the user actually set the value. In most cases, neither should be an issue +although there are circumstances where this changes. An example of a safe call is: ```javascript const isSupported = SettingsStore.isLevelSupported(SettingLevel.ROOM); if (isSupported) { @@ -64,11 +78,13 @@ if (isSupported) { } ``` -These checks may also be performed in different areas of the application to avoid the verbose example above. For instance, the component which allows changing the setting may be hidden conditionally on the above conditions. +These checks may also be performed in different areas of the application to avoid the verbose example above. For +instance, the component which allows changing the setting may be hidden conditionally on the above conditions. ##### `SettingsFlag` component -Where possible, the `SettingsFlag` component should be used to set simple "flip-a-bit" (true/false) settings. The `SettingsFlag` also supports simple radio button options, such as the theme the user would like to use. +Where possible, the `SettingsFlag` component should be used to set simple "flip-a-bit" (true/false) settings. The +`SettingsFlag` also supports simple radio button options, such as the theme the user would like to use. ```html { @@ -133,7 +172,12 @@ SettingsStore.getValue(...); // this will return the value set in `setValue` abo ## Watching for changes -Most use cases do not need to set up a watcher because they are able to react to changes as they are made, or the changes which are made are not significant enough for it to matter. Watchers are intended to be used in scenarios where it is important to react to changes made by other logged in devices. Typically, this would be done within the component itself, however the component should not be aware of the intricacies of setting inversion or remapping to particular data structures. Instead, a generic watcher interface is provided on `SettingsStore` to watch (and subsequently unwatch) for changes in a setting. +Most use cases do not need to set up a watcher because they are able to react to changes as they are made, or the +changes which are made are not significant enough for it to matter. Watchers are intended to be used in scenarios where +it is important to react to changes made by other logged in devices. Typically, this would be done within the component +itself, however the component should not be aware of the intricacies of setting inversion or remapping to particular +data structures. Instead, a generic watcher interface is provided on `SettingsStore` to watch (and subsequently unwatch) +for changes in a setting. An example of a watcher in action would be: @@ -143,9 +187,10 @@ class MyComponent extends React.Component { settingWatcherRef = null; componentWillMount() { - this.settingWatcherRef = SettingsStore.watchSetting("roomColor", "!example:matrix.org", (settingName, roomId, level, newValAtLevel, newVal) => { - this.setState({color: newVal}); - }); + const callback = (settingName, roomId, level, newValAtLevel, newVal) => { + this.setState({color: newVal}); + }; + this.settingWatcherRef = SettingsStore.watchSetting("roomColor", "!example:matrix.org", callback); } componentWillUnmount() { @@ -157,21 +202,37 @@ class MyComponent extends React.Component { # Maintainers Reference -The granular settings system has a few complex parts to power it. This section is to document how the `SettingsStore` is supposed to work. +The granular settings system has a few complex parts to power it. This section is to document how the `SettingsStore` is +supposed to work. ### General information -The `SettingsStore` uses the hardcoded `LEVEL_ORDER` constant to ensure that it is using the correct override procedure. The array is checked from left to right, simulating the behaviour of overriding values from the higher levels. Each level should be defined in this array, including `default`. +The `SettingsStore` uses the hardcoded `LEVEL_ORDER` constant to ensure that it is using the correct override procedure. +The array is checked from left to right, simulating the behaviour of overriding values from the higher levels. Each +level should be defined in this array, including `default`. -Handlers (`src/settings/handlers/SettingsHandler.js`) represent a single level and are responsible for getting and setting values at that level. Handlers also provide additional information to the `SettingsStore` such as if the level is supported or if the current user may set values at the level. The `SettingsStore` will use the handler to enforce checks and manipulate settings. Handlers are also responsible for dealing with migration patterns or legacy settings for their level (for example, a setting being renamed or using a different key from other settings in the underlying store). Handlers are provided to the `SettingsStore` via the `LEVEL_HANDLERS` constant. `SettingsStore` will optimize lookups by only considering handlers that are supported on the platform. +Handlers (`src/settings/handlers/SettingsHandler.js`) represent a single level and are responsible for getting and +setting values at that level. Handlers also provide additional information to the `SettingsStore` such as if the level +is supported or if the current user may set values at the level. The `SettingsStore` will use the handler to enforce +checks and manipulate settings. Handlers are also responsible for dealing with migration patterns or legacy settings for +their level (for example, a setting being renamed or using a different key from other settings in the underlying store). +Handlers are provided to the `SettingsStore` via the `LEVEL_HANDLERS` constant. `SettingsStore` will optimize lookups by +only considering handlers that are supported on the platform. -Local echo is achieved through `src/settings/handlers/LocalEchoWrapper.js` which acts as a wrapper around a given handler. This is automatically applied to all defined `LEVEL_HANDLERS` and proxies the calls to the wrapped handler where possible. The echo is achieved by a simple object cache stored within the class itself. The cache is invalidated immediately upon the proxied save call succeeding or failing. +Local echo is achieved through `src/settings/handlers/LocalEchoWrapper.js` which acts as a wrapper around a given +handler. This is automatically applied to all defined `LEVEL_HANDLERS` and proxies the calls to the wrapped handler +where possible. The echo is achieved by a simple object cache stored within the class itself. The cache is invalidated +immediately upon the proxied save call succeeding or failing. -Controllers are notified of changes by the `SettingsStore`, and are given the opportunity to override values after the `SettingsStore` has deemed the value calculated. Controllers are invoked as the last possible step in the code. +Controllers are notified of changes by the `SettingsStore`, and are given the opportunity to override values after the +`SettingsStore` has deemed the value calculated. Controllers are invoked as the last possible step in the code. ### Features -Features automatically get considered as `disabled` if they are not listed in the `SdkConfig` or `enable_labs` is false/not set. Features are always checked against the configuration before going through the level order as they have the option of being forced-on or forced-off for the application. This is done by the `features` section and looks something like this: +Features automatically get considered as `disabled` if they are not listed in the `SdkConfig` or `enable_labs` is +false/not set. Features are always checked against the configuration before going through the level order as they have +the option of being forced-on or forced-off for the application. This is done by the `features` section and looks +something like this: ``` "features": { @@ -185,9 +246,18 @@ If `enableLabs` is true in the configuration, the default for features becomes ` ### Watchers -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. +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. +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'. +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'. \ No newline at end of file