From 099661c2aacd2eb360d8dcb6ac9e24972db16bc6 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 22 Jun 2020 10:18:38 -0600 Subject: [PATCH 1/4] Only fire setting changes for changed settings Previously we were firing updates for everything, which is bad. This has an effect of causing the room list to update itself every time the user goes to toggle some account settings. --- .../handlers/AccountSettingsHandler.js | 9 ++-- .../MatrixClientBackedSettingsHandler.js | 4 ++ .../handlers/RoomAccountSettingsHandler.js | 9 ++-- src/settings/handlers/RoomSettingsHandler.js | 13 +++-- src/utils/arrays.ts | 22 +++++++++ src/utils/objects.ts | 49 +++++++++++++++++++ 6 files changed, 97 insertions(+), 9 deletions(-) create mode 100644 src/utils/objects.ts diff --git a/src/settings/handlers/AccountSettingsHandler.js b/src/settings/handlers/AccountSettingsHandler.js index fea2e92c62..7b78c39c3a 100644 --- a/src/settings/handlers/AccountSettingsHandler.js +++ b/src/settings/handlers/AccountSettingsHandler.js @@ -18,6 +18,7 @@ limitations under the License. import {MatrixClientPeg} from '../../MatrixClientPeg'; import MatrixClientBackedSettingsHandler from "./MatrixClientBackedSettingsHandler"; import {SettingLevel} from "../SettingsStore"; +import {objectKeyChanges} from "../../utils/objects"; const BREADCRUMBS_LEGACY_EVENT_TYPE = "im.vector.riot.breadcrumb_rooms"; const BREADCRUMBS_EVENT_TYPE = "im.vector.setting.breadcrumbs"; @@ -45,7 +46,7 @@ export default class AccountSettingsHandler extends MatrixClientBackedSettingsHa newClient.on("accountData", this._onAccountData); } - _onAccountData(event) { + _onAccountData(event, prevEvent) { if (event.getType() === "org.matrix.preview_urls") { let val = event.getContent()['disable']; if (typeof(val) !== "boolean") { @@ -56,8 +57,10 @@ export default class AccountSettingsHandler extends MatrixClientBackedSettingsHa 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())) { + // Figure out what changed and fire those updates + const prevContent = prevEvent ? prevEvent.getContent() : {}; + const changedSettings = objectKeyChanges(prevContent, event.getContent()); + for (const settingName of changedSettings) { const val = event.getContent()[settingName]; this._watchers.notifyUpdate(settingName, null, SettingLevel.ACCOUNT, val); } diff --git a/src/settings/handlers/MatrixClientBackedSettingsHandler.js b/src/settings/handlers/MatrixClientBackedSettingsHandler.js index effe7ae9a7..63725b4dff 100644 --- a/src/settings/handlers/MatrixClientBackedSettingsHandler.js +++ b/src/settings/handlers/MatrixClientBackedSettingsHandler.js @@ -42,6 +42,10 @@ export default class MatrixClientBackedSettingsHandler extends SettingsHandler { MatrixClientBackedSettingsHandler._instances.push(this); } + get client() { + return MatrixClientBackedSettingsHandler._matrixClient; + } + initMatrixClient() { console.warn("initMatrixClient not overridden"); } diff --git a/src/settings/handlers/RoomAccountSettingsHandler.js b/src/settings/handlers/RoomAccountSettingsHandler.js index 1e9d3f7bed..1c818cef71 100644 --- a/src/settings/handlers/RoomAccountSettingsHandler.js +++ b/src/settings/handlers/RoomAccountSettingsHandler.js @@ -18,6 +18,7 @@ limitations under the License. import {MatrixClientPeg} from '../../MatrixClientPeg'; import MatrixClientBackedSettingsHandler from "./MatrixClientBackedSettingsHandler"; import {SettingLevel} from "../SettingsStore"; +import {objectKeyChanges} from "../../utils/objects"; const ALLOWED_WIDGETS_EVENT_TYPE = "im.vector.setting.allowed_widgets"; @@ -40,7 +41,7 @@ export default class RoomAccountSettingsHandler extends MatrixClientBackedSettin newClient.on("Room.accountData", this._onAccountData); } - _onAccountData(event, room) { + _onAccountData(event, room, prevEvent) { const roomId = room.roomId; if (event.getType() === "org.matrix.room.preview_urls") { @@ -55,8 +56,10 @@ export default class RoomAccountSettingsHandler extends MatrixClientBackedSettin } else if (event.getType() === "org.matrix.room.color_scheme") { 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())) { + // Figure out what changed and fire those updates + const prevContent = prevEvent ? prevEvent.getContent() : {}; + const changedSettings = objectKeyChanges(prevContent, event.getContent()); + for (const settingName of changedSettings) { const val = event.getContent()[settingName]; this._watchers.notifyUpdate(settingName, roomId, SettingLevel.ROOM_ACCOUNT, val); } diff --git a/src/settings/handlers/RoomSettingsHandler.js b/src/settings/handlers/RoomSettingsHandler.js index 6407818450..4116f26220 100644 --- a/src/settings/handlers/RoomSettingsHandler.js +++ b/src/settings/handlers/RoomSettingsHandler.js @@ -18,6 +18,7 @@ limitations under the License. import {MatrixClientPeg} from '../../MatrixClientPeg'; import MatrixClientBackedSettingsHandler from "./MatrixClientBackedSettingsHandler"; import {SettingLevel} from "../SettingsStore"; +import {objectKeyChanges} from "../../utils/objects"; /** * Gets and sets settings at the "room" level. @@ -38,8 +39,12 @@ export default class RoomSettingsHandler extends MatrixClientBackedSettingsHandl newClient.on("RoomState.events", this._onEvent); } - _onEvent(event) { + _onEvent(event, state, prevEvent) { const roomId = event.getRoomId(); + const room = this.client.getRoom(roomId); + if (!room) throw new Error(`Unknown room caused state update: ${roomId}`); + + if (state !== room.currentState) return; // ignore state updates which are not current if (event.getType() === "org.matrix.room.preview_urls") { let val = event.getContent()['disable']; @@ -51,8 +56,10 @@ export default class RoomSettingsHandler extends MatrixClientBackedSettingsHandl 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())) { + // Figure out what changed and fire those updates + const prevContent = prevEvent ? prevEvent.getContent() : {}; + const changedSettings = objectKeyChanges(prevContent, event.getContent()); + for (const settingName of changedSettings) { this._watchers.notifyUpdate(settingName, roomId, SettingLevel.ROOM, event.getContent()[settingName]); } } diff --git a/src/utils/arrays.ts b/src/utils/arrays.ts index fea376afcd..8175d89464 100644 --- a/src/utils/arrays.ts +++ b/src/utils/arrays.ts @@ -46,6 +46,28 @@ export function arrayDiff(a: T[], b: T[]): { added: T[], removed: T[] } { }; } +/** + * Returns the union of two arrays. + * @param a The first array. Must be defined. + * @param b The second array. Must be defined. + * @returns The union of the arrays. + */ +export function arrayUnion(a: T[], b: T[]): T[] { + return a.filter(i => b.includes(i)); +} + +/** + * Merges arrays, deduping contents using a Set. + * @param a The arrays to merge. + * @returns The merged array. + */ +export function arrayMerge(...a: T[][]): T[] { + return Array.from(a.reduce((c, v) => { + v.forEach(i => c.add(i)); + return c; + }, new Set())); +} + /** * Helper functions to perform LINQ-like queries on arrays. */ diff --git a/src/utils/objects.ts b/src/utils/objects.ts new file mode 100644 index 0000000000..cbb311cc48 --- /dev/null +++ b/src/utils/objects.ts @@ -0,0 +1,49 @@ +/* +Copyright 2020 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { arrayDiff, arrayMerge, arrayUnion } from "./arrays"; + +/** + * Determines the keys added, changed, and removed between two objects. + * For changes, simple triple equal comparisons are done, not in-depth + * tree checking. + * @param a The first object. Must be defined. + * @param b The second object. Must be defined. + * @returns The difference between the keys of each object. + */ +export function objectDiff(a: any, b: any): { changed: string[], added: string[], removed: string[] } { + const aKeys = Object.keys(a); + const bKeys = Object.keys(b); + const keyDiff = arrayDiff(aKeys, bKeys); + const possibleChanges = arrayUnion(aKeys, bKeys); + const changes = possibleChanges.filter(k => a[k] !== b[k]); + + return {changed: changes, added: keyDiff.added, removed: keyDiff.removed}; +} + +/** + * Gets all the key changes (added, removed, or value difference) between + * two objects. Triple equals is used to compare values, not in-depth tree + * checking. + * @param a The first object. Must be defined. + * @param b The second object. Must be defined. + * @returns The keys which have been added, removed, or changed between the + * two objects. + */ +export function objectKeyChanges(a: any, b: any): string[] { + const diff = objectDiff(a, b); + return arrayMerge(diff.removed, diff.added, diff.changed); +} From fe65b7631dfd9b985e1e1f852a4e795dca617fd9 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 22 Jun 2020 10:57:08 -0600 Subject: [PATCH 2/4] Soften warning about lack of rooms in setting updates --- src/settings/handlers/RoomSettingsHandler.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/settings/handlers/RoomSettingsHandler.js b/src/settings/handlers/RoomSettingsHandler.js index 4116f26220..2ed82b577d 100644 --- a/src/settings/handlers/RoomSettingsHandler.js +++ b/src/settings/handlers/RoomSettingsHandler.js @@ -42,9 +42,12 @@ export default class RoomSettingsHandler extends MatrixClientBackedSettingsHandl _onEvent(event, state, prevEvent) { const roomId = event.getRoomId(); const room = this.client.getRoom(roomId); - if (!room) throw new Error(`Unknown room caused state update: ${roomId}`); - if (state !== room.currentState) return; // ignore state updates which are not current + // Note: the tests often fire setting updates that don't have rooms in the store, so + // we fail softly here. We shouldn't assume that the state being fired is current + // state, but we also don't need to explode just because we didn't find a room. + if (!room) console.warn(`Unknown room caused setting update: ${roomId}`); + if (room && state !== room.currentState) return; // ignore state updates which are not current if (event.getType() === "org.matrix.room.preview_urls") { let val = event.getContent()['disable']; From 9e3c101172c1f14c827de53d0d548ba79dbff6c6 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 22 Jun 2020 11:24:04 -0600 Subject: [PATCH 3/4] Clone reads of account data to prevent mutation --- src/settings/handlers/AccountSettingsHandler.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/settings/handlers/AccountSettingsHandler.js b/src/settings/handlers/AccountSettingsHandler.js index 7b78c39c3a..c396a9d4de 100644 --- a/src/settings/handlers/AccountSettingsHandler.js +++ b/src/settings/handlers/AccountSettingsHandler.js @@ -162,7 +162,7 @@ export default class AccountSettingsHandler extends MatrixClientBackedSettingsHa const event = cli.getAccountData(eventType); if (!event || !event.getContent()) return null; - return event.getContent(); + return JSON.parse(JSON.stringify(event.getContent())); // clone to prevent mutation } _notifyBreadcrumbsUpdate(event) { From 1fe3e33dbf944cc0f078c18c253d019523af540d Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 22 Jun 2020 14:14:43 -0600 Subject: [PATCH 4/4] Factor out cloning to a util and use it everywhere --- src/FromWidgetPostMessageApi.js | 5 +++-- src/ScalarMessaging.js | 5 +++-- src/components/views/terms/InlineTermsAgreement.js | 3 ++- src/settings/handlers/AccountSettingsHandler.js | 4 ++-- src/settings/handlers/RoomAccountSettingsHandler.js | 4 ++-- src/settings/handlers/RoomSettingsHandler.js | 4 ++-- src/utils/WidgetUtils.js | 3 ++- src/utils/objects.ts | 11 +++++++++++ src/widgets/WidgetApi.ts | 3 ++- 9 files changed, 29 insertions(+), 13 deletions(-) diff --git a/src/FromWidgetPostMessageApi.js b/src/FromWidgetPostMessageApi.js index 102afa6bf1..1b4aa19ebf 100644 --- a/src/FromWidgetPostMessageApi.js +++ b/src/FromWidgetPostMessageApi.js @@ -25,6 +25,7 @@ import RoomViewStore from "./stores/RoomViewStore"; import {IntegrationManagers} from "./integrations/IntegrationManagers"; import SettingsStore from "./settings/SettingsStore"; import {Capability} from "./widgets/WidgetApi"; +import {objectClone} from "./utils/objects"; const WIDGET_API_VERSION = '0.0.2'; // Current API version const SUPPORTED_WIDGET_API_VERSIONS = [ @@ -247,7 +248,7 @@ export default class FromWidgetPostMessageApi { * @param {Object} res Response data */ sendResponse(event, res) { - const data = JSON.parse(JSON.stringify(event.data)); + const data = objectClone(event.data); data.response = res; event.source.postMessage(data, event.origin); } @@ -260,7 +261,7 @@ export default class FromWidgetPostMessageApi { */ sendError(event, msg, nestedError) { console.error('Action:' + event.data.action + ' failed with message: ' + msg); - const data = JSON.parse(JSON.stringify(event.data)); + const data = objectClone(event.data); data.response = { error: { message: msg, diff --git a/src/ScalarMessaging.js b/src/ScalarMessaging.js index 315c2d86f4..b33aa57359 100644 --- a/src/ScalarMessaging.js +++ b/src/ScalarMessaging.js @@ -244,16 +244,17 @@ import RoomViewStore from './stores/RoomViewStore'; import { _t } from './languageHandler'; import {IntegrationManagers} from "./integrations/IntegrationManagers"; import {WidgetType} from "./widgets/WidgetType"; +import {objectClone} from "./utils/objects"; function sendResponse(event, res) { - const data = JSON.parse(JSON.stringify(event.data)); + const data = objectClone(event.data); data.response = res; event.source.postMessage(data, event.origin); } function sendError(event, msg, nestedError) { console.error("Action:" + event.data.action + " failed with message: " + msg); - const data = JSON.parse(JSON.stringify(event.data)); + const data = objectClone(event.data); data.response = { error: { message: msg, diff --git a/src/components/views/terms/InlineTermsAgreement.js b/src/components/views/terms/InlineTermsAgreement.js index bccd686cd3..55719fe57f 100644 --- a/src/components/views/terms/InlineTermsAgreement.js +++ b/src/components/views/terms/InlineTermsAgreement.js @@ -18,6 +18,7 @@ import React from "react"; import PropTypes from "prop-types"; import {_t, pickBestLanguage} from "../../../languageHandler"; import * as sdk from "../../.."; +import {objectClone} from "../../../utils/objects"; export default class InlineTermsAgreement extends React.Component { static propTypes = { @@ -56,7 +57,7 @@ export default class InlineTermsAgreement extends React.Component { } _togglePolicy = (index) => { - const policies = JSON.parse(JSON.stringify(this.state.policies)); // deep & cheap clone + const policies = objectClone(this.state.policies); policies[index].checked = !policies[index].checked; this.setState({policies}); }; diff --git a/src/settings/handlers/AccountSettingsHandler.js b/src/settings/handlers/AccountSettingsHandler.js index c396a9d4de..732ce6c550 100644 --- a/src/settings/handlers/AccountSettingsHandler.js +++ b/src/settings/handlers/AccountSettingsHandler.js @@ -18,7 +18,7 @@ limitations under the License. import {MatrixClientPeg} from '../../MatrixClientPeg'; import MatrixClientBackedSettingsHandler from "./MatrixClientBackedSettingsHandler"; import {SettingLevel} from "../SettingsStore"; -import {objectKeyChanges} from "../../utils/objects"; +import {objectClone, objectKeyChanges} from "../../utils/objects"; const BREADCRUMBS_LEGACY_EVENT_TYPE = "im.vector.riot.breadcrumb_rooms"; const BREADCRUMBS_EVENT_TYPE = "im.vector.setting.breadcrumbs"; @@ -162,7 +162,7 @@ export default class AccountSettingsHandler extends MatrixClientBackedSettingsHa const event = cli.getAccountData(eventType); if (!event || !event.getContent()) return null; - return JSON.parse(JSON.stringify(event.getContent())); // clone to prevent mutation + return objectClone(event.getContent()); // clone to prevent mutation } _notifyBreadcrumbsUpdate(event) { diff --git a/src/settings/handlers/RoomAccountSettingsHandler.js b/src/settings/handlers/RoomAccountSettingsHandler.js index 1c818cef71..b2af81779b 100644 --- a/src/settings/handlers/RoomAccountSettingsHandler.js +++ b/src/settings/handlers/RoomAccountSettingsHandler.js @@ -18,7 +18,7 @@ limitations under the License. import {MatrixClientPeg} from '../../MatrixClientPeg'; import MatrixClientBackedSettingsHandler from "./MatrixClientBackedSettingsHandler"; import {SettingLevel} from "../SettingsStore"; -import {objectKeyChanges} from "../../utils/objects"; +import {objectClone, objectKeyChanges} from "../../utils/objects"; const ALLOWED_WIDGETS_EVENT_TYPE = "im.vector.setting.allowed_widgets"; @@ -137,6 +137,6 @@ export default class RoomAccountSettingsHandler extends MatrixClientBackedSettin const event = room.getAccountData(eventType); if (!event || !event.getContent()) return null; - return event.getContent(); + return objectClone(event.getContent()); // clone to prevent mutation } } diff --git a/src/settings/handlers/RoomSettingsHandler.js b/src/settings/handlers/RoomSettingsHandler.js index 2ed82b577d..d8e775742c 100644 --- a/src/settings/handlers/RoomSettingsHandler.js +++ b/src/settings/handlers/RoomSettingsHandler.js @@ -18,7 +18,7 @@ limitations under the License. import {MatrixClientPeg} from '../../MatrixClientPeg'; import MatrixClientBackedSettingsHandler from "./MatrixClientBackedSettingsHandler"; import {SettingLevel} from "../SettingsStore"; -import {objectKeyChanges} from "../../utils/objects"; +import {objectClone, objectKeyChanges} from "../../utils/objects"; /** * Gets and sets settings at the "room" level. @@ -117,6 +117,6 @@ export default class RoomSettingsHandler extends MatrixClientBackedSettingsHandl const event = room.currentState.getStateEvents(eventType, ""); if (!event || !event.getContent()) return null; - return event.getContent(); + return objectClone(event.getContent()); // clone to prevent mutation } } diff --git a/src/utils/WidgetUtils.js b/src/utils/WidgetUtils.js index b48ec481ba..f7f4be202b 100644 --- a/src/utils/WidgetUtils.js +++ b/src/utils/WidgetUtils.js @@ -31,6 +31,7 @@ import {IntegrationManagers} from "../integrations/IntegrationManagers"; import {Capability} from "../widgets/WidgetApi"; import {Room} from "matrix-js-sdk/src/models/room"; import {WidgetType} from "../widgets/WidgetType"; +import {objectClone} from "./objects"; export default class WidgetUtils { /* Returns true if user is able to send state events to modify widgets in this room @@ -222,7 +223,7 @@ export default class WidgetUtils { const client = MatrixClientPeg.get(); // Get the current widgets and clone them before we modify them, otherwise // we'll modify the content of the old event. - const userWidgets = JSON.parse(JSON.stringify(WidgetUtils.getUserWidgets())); + const userWidgets = objectClone(WidgetUtils.getUserWidgets()); // Delete existing widget with ID try { diff --git a/src/utils/objects.ts b/src/utils/objects.ts index cbb311cc48..14fa928ce2 100644 --- a/src/utils/objects.ts +++ b/src/utils/objects.ts @@ -47,3 +47,14 @@ export function objectKeyChanges(a: any, b: any): string[] { const diff = objectDiff(a, b); return arrayMerge(diff.removed, diff.added, diff.changed); } + +/** + * Clones an object by running it through JSON parsing. Note that this + * will destroy any complicated object types which do not translate to + * JSON. + * @param obj The object to clone. + * @returns The cloned object + */ +export function objectClone(obj: any): any { + return JSON.parse(JSON.stringify(obj)); +} diff --git a/src/widgets/WidgetApi.ts b/src/widgets/WidgetApi.ts index 795c6648ef..d5f2f2258e 100644 --- a/src/widgets/WidgetApi.ts +++ b/src/widgets/WidgetApi.ts @@ -19,6 +19,7 @@ limitations under the License. import { randomString } from "matrix-js-sdk/src/randomstring"; import { EventEmitter } from "events"; +import { objectClone } from "../utils/objects"; export enum Capability { Screenshot = "m.capability.screenshot", @@ -140,7 +141,7 @@ export class WidgetApi extends EventEmitter { private replyToRequest(payload: ToWidgetRequest, reply: any) { if (!window.parent) return; - const request = JSON.parse(JSON.stringify(payload)); + const request = objectClone(payload); request.response = reply; window.parent.postMessage(request, this.origin);