Merge pull request #4803 from matrix-org/travis/room-list/setting-diff

Only fire setting changes for changed settings

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,

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,

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});
     };

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 {objectClone, 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);
             }
@@ -159,7 +162,7 @@ export default class AccountSettingsHandler extends MatrixClientBackedSettingsHa
 
         const event = cli.getAccountData(eventType);
         if (!event || !event.getContent()) return null;
-        return event.getContent();
+        return objectClone(event.getContent()); // clone to prevent mutation
     }
 
     _notifyBreadcrumbsUpdate(event) {

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");
     }

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 {objectClone, 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);
             }
@@ -134,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
     }
 }

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 {objectClone, objectKeyChanges} from "../../utils/objects";
 
 /**
  * Gets and sets settings at the "room" level.
@@ -38,8 +39,15 @@ 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);
+
+        // 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'];
@@ -51,8 +59,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]);
             }
         }
@@ -107,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
     }
 }

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 {

src/utils/arrays.ts

@@ -46,6 +46,28 @@ export function arrayDiff<T>(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<T>(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<T>(...a: T[][]): T[] {
+    return Array.from(a.reduce((c, v) => {
+        v.forEach(i => c.add(i));
+        return c;
+    }, new Set<T>()));
+}
+
 /**
  * Helper functions to perform LINQ-like queries on arrays.
  */

src/utils/objects.ts

@@ -0,0 +1,60 @@
+/*
+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);
+}
+
+/**
+ * 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));
+}

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);