Merge pull request #5034 from matrix-org/travis/fix-perf

Mixed bag of performance improvements: ScrollPanel and notifications

src/RoomNotifs.js

@@ -34,27 +34,6 @@ export function shouldShowMentionBadge(roomNotifState) {
     return MENTION_BADGE_STATES.includes(roomNotifState);
 }
 
-export function countRoomsWithNotif(rooms) {
-    return rooms.reduce((result, room, index) => {
-        const roomNotifState = getRoomNotifsState(room.roomId);
-        const highlight = room.getUnreadNotificationCount('highlight') > 0;
-        const notificationCount = room.getUnreadNotificationCount();
-
-        const notifBadges = notificationCount > 0 && shouldShowNotifBadge(roomNotifState);
-        const mentionBadges = highlight && shouldShowMentionBadge(roomNotifState);
-        const isInvite = room.hasMembershipState(MatrixClientPeg.get().credentials.userId, 'invite');
-        const badges = notifBadges || mentionBadges || isInvite;
-
-        if (badges) {
-            result.count++;
-            if (highlight) {
-                result.highlight = true;
-            }
-        }
-        return result;
-    }, {count: 0, highlight: false});
-}
-
 export function aggregateNotificationCount(rooms) {
     return rooms.reduce((result, room) => {
         const roomNotifState = getRoomNotifsState(room.roomId);

src/components/structures/MatrixChat.tsx

@@ -58,7 +58,6 @@ import { messageForSyncError } from '../../utils/ErrorUtils';
 import ResizeNotifier from "../../utils/ResizeNotifier";
 import AutoDiscoveryUtils, { ValidatedServerConfig } from "../../utils/AutoDiscoveryUtils";
 import DMRoomMap from '../../utils/DMRoomMap';
-import { countRoomsWithNotif } from '../../RoomNotifs';
 import ThemeWatcher from "../../settings/watchers/ThemeWatcher";
 import { FontWatcher } from '../../settings/watchers/FontWatcher';
 import { storeRoomAliasInCache } from '../../RoomAliasCache';
@@ -75,6 +74,7 @@ import {
 import {showToast as showNotificationsToast} from "../../toasts/DesktopNotificationsToast";
 import { OpenToTabPayload } from "../../dispatcher/payloads/OpenToTabPayload";
 import ErrorDialog from "../views/dialogs/ErrorDialog";
+import { RoomNotificationStateStore } from "../../stores/notifications/RoomNotificationStateStore";
 
 /** constants for MatrixChat.state.view */
 export enum Views {
@@ -1844,21 +1844,20 @@ export default class MatrixChat extends React.PureComponent<IProps, IState> {
     }
 
     updateStatusIndicator(state: string, prevState: string) {
-        // only count visible rooms to not torment the user with notification counts in rooms they can't see
+        const notificationState = RoomNotificationStateStore.instance.globalState;
-        // it will include highlights from the previous version of the room internally
+        const numUnreadRooms = notificationState.numUnreadStates; // we know that states === rooms here
-        const notifCount = countRoomsWithNotif(MatrixClientPeg.get().getVisibleRooms()).count;
 
         if (PlatformPeg.get()) {
             PlatformPeg.get().setErrorStatus(state === 'ERROR');
-            PlatformPeg.get().setNotificationCount(notifCount);
+            PlatformPeg.get().setNotificationCount(numUnreadRooms);
         }
 
         this.subTitleStatus = '';
         if (state === "ERROR") {
             this.subTitleStatus += `[${_t("Offline")}] `;
         }
-        if (notifCount > 0) {
+        if (numUnreadRooms > 0) {
-            this.subTitleStatus += `[${notifCount}]`;
+            this.subTitleStatus += `[${numUnreadRooms}]`;
         }
 
         this.setPageSubtitle();

src/components/structures/ScrollPanel.js

@@ -648,7 +648,9 @@ export default createReactClass({
 
         if (scrollState.stuckAtBottom) {
             const sn = this._getScrollNode();
+            if (sn.scrollTop !== sn.scrollHeight) {
                 sn.scrollTop = sn.scrollHeight;
+            }
         } else if (scrollState.trackedScrollToken) {
             const itemlist = this._itemlist.current;
             const trackedNode = this._getTrackedNode();
@@ -657,7 +659,10 @@ export default createReactClass({
                 const bottomDiff = newBottomOffset - scrollState.bottomOffset;
                 this._bottomGrowth += bottomDiff;
                 scrollState.bottomOffset = newBottomOffset;
-                itemlist.style.height = `${this._getListHeight()}px`;
+                const newHeight = `${this._getListHeight()}px`;
+                if (itemlist.style.height !== newHeight) {
+                    itemlist.style.height = newHeight;
+                }
                 debuglog("balancing height because messages below viewport grew by", bottomDiff);
             }
         }
@@ -694,12 +699,16 @@ export default createReactClass({
         const height = Math.max(minHeight, contentHeight);
         this._pages = Math.ceil(height / PAGE_SIZE);
         this._bottomGrowth = 0;
-        const newHeight = this._getListHeight();
+        const newHeight = `${this._getListHeight()}px`;
 
         const scrollState = this.scrollState;
         if (scrollState.stuckAtBottom) {
-            itemlist.style.height = `${newHeight}px`;
+            if (itemlist.style.height !== newHeight) {
+                itemlist.style.height = newHeight;
+            }
+            if (sn.scrollTop !== sn.scrollHeight){
                 sn.scrollTop = sn.scrollHeight;
+            }
             debuglog("updateHeight to", newHeight);
         } else if (scrollState.trackedScrollToken) {
             const trackedNode = this._getTrackedNode();
@@ -709,7 +718,9 @@ export default createReactClass({
             // the currently filled piece of the timeline
             if (trackedNode) {
                 const oldTop = trackedNode.offsetTop;
-                itemlist.style.height = `${newHeight}px`;
+                if (itemlist.style.height !== newHeight) {
+                    itemlist.style.height = newHeight;
+                }
                 const newTop = trackedNode.offsetTop;
                 const topDiff = newTop - oldTop;
                 // important to scroll by a relative amount as

src/components/views/avatars/DecoratedRoomAvatar.tsx

@@ -44,7 +44,7 @@ export default class DecoratedRoomAvatar extends React.PureComponent<IProps, ISt
         super(props);
 
         this.state = {
-            notificationState: RoomNotificationStateStore.instance.getRoomState(this.props.room, this.props.tag),
+            notificationState: RoomNotificationStateStore.instance.getRoomState(this.props.room),
         };
     }
 

src/components/views/rooms/RoomList.tsx

@@ -210,7 +210,7 @@ export default class RoomList extends React.Component<IProps, IState> {
             if (unread) {
                 // filter to only notification rooms (and our current active room so we can index properly)
                 listRooms = listRooms.filter(r => {
-                    const state = RoomNotificationStateStore.instance.getRoomState(r, t);
+                    const state = RoomNotificationStateStore.instance.getRoomState(r);
                     return state.room.roomId === roomId || state.isUnread;
                 });
             }

src/components/views/rooms/RoomTile.tsx

@@ -120,7 +120,7 @@ export default class RoomTile extends React.Component<IProps, IState> {
 
         this.state = {
             hover: false,
-            notificationState: RoomNotificationStateStore.instance.getRoomState(this.props.room, this.props.tag),
+            notificationState: RoomNotificationStateStore.instance.getRoomState(this.props.room),
             selected: ActiveRoomObserver.activeRoomId === this.props.room.roomId,
             notificationsMenuPosition: null,
             generalMenuPosition: null,

src/stores/notifications/RoomNotificationStateStore.ts

@@ -21,21 +21,36 @@ import { DefaultTagID, TagID } from "../room-list/models";
 import { FetchRoomFn, ListNotificationState } from "./ListNotificationState";
 import { Room } from "matrix-js-sdk/src/models/room";
 import { RoomNotificationState } from "./RoomNotificationState";
-
+import { SummarizedNotificationState } from "./SummarizedNotificationState";
-const INSPECIFIC_TAG = "INSPECIFIC_TAG";
-type INSPECIFIC_TAG = "INSPECIFIC_TAG";
 
 interface IState {}
 
 export class RoomNotificationStateStore extends AsyncStoreWithClient<IState> {
     private static internalInstance = new RoomNotificationStateStore();
 
-    private roomMap = new Map<Room, Map<TagID | INSPECIFIC_TAG, RoomNotificationState>>();
+    private roomMap = new Map<Room, RoomNotificationState>();
 
     private constructor() {
         super(defaultDispatcher, {});
     }
 
+    /**
+     * Gets a snapshot of notification state for all visible rooms. The number of states recorded
+     * on the SummarizedNotificationState is equivalent to rooms.
+     */
+    public get globalState(): SummarizedNotificationState {
+        // If we're not ready yet, just return an empty state
+        if (!this.matrixClient) return new SummarizedNotificationState();
+
+        // Only count visible rooms to not torment the user with notification counts in rooms they can't see.
+        // This will include highlights from the previous version of the room internally
+        const globalState = new SummarizedNotificationState();
+        for (const room of this.matrixClient.getVisibleRooms()) {
+            globalState.add(this.getRoomState(room));
+        }
+        return globalState;
+    }
+
     /**
      * Creates a new list notification state. The consumer is expected to set the rooms
      * on the notification state, and destroy the state when it no longer needs it.
@@ -49,7 +64,7 @@ export class RoomNotificationStateStore extends AsyncStoreWithClient<IState> {
         // TODO: Update if/when invites move out of the room list.
         const useTileCount = tagId === DefaultTagID.Invite;
         const getRoomFn: FetchRoomFn = (room: Room) => {
-            return this.getRoomState(room, tagId);
+            return this.getRoomState(room);
         };
         return new ListNotificationState(useTileCount, tagId, getRoomFn);
     }
@@ -59,22 +74,13 @@ export class RoomNotificationStateStore extends AsyncStoreWithClient<IState> {
      * attempt to destroy the returned state as it may be shared with other
      * consumers.
      * @param room The room to get the notification state for.
-     * @param inTagId Optional tag ID to scope the notification state to.
      * @returns The room's notification state.
      */
-    public getRoomState(room: Room, inTagId?: TagID): RoomNotificationState {
+    public getRoomState(room: Room): RoomNotificationState {
         if (!this.roomMap.has(room)) {
-            this.roomMap.set(room, new Map<TagID | INSPECIFIC_TAG, RoomNotificationState>());
+            this.roomMap.set(room, new RoomNotificationState(room));
         }
-
+        return this.roomMap.get(room);
-        const targetTag = inTagId ? inTagId : INSPECIFIC_TAG;
-
-        const forRoomMap = this.roomMap.get(room);
-        if (!forRoomMap.has(targetTag)) {
-            forRoomMap.set(inTagId ? inTagId : INSPECIFIC_TAG, new RoomNotificationState(room));
-        }
-
-        return forRoomMap.get(targetTag);
     }
 
     public static get instance(): RoomNotificationStateStore {
@@ -82,12 +88,10 @@ export class RoomNotificationStateStore extends AsyncStoreWithClient<IState> {
     }
 
     protected async onNotReady(): Promise<any> {
-        for (const roomMap of this.roomMap.values()) {
+        for (const roomState of this.roomMap.values()) {
-            for (const roomState of roomMap.values()) {
             roomState.destroy();
         }
     }
-    }
 
     // We don't need this, but our contract says we do.
     protected async onAction(payload: ActionPayload) {

src/stores/notifications/SummarizedNotificationState.ts

@@ -0,0 +1,62 @@
+/*
+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 { NotificationColor } from "./NotificationColor";
+import { NotificationState } from "./NotificationState";
+
+/**
+ * Summarizes a number of states into a unique snapshot. To populate, call
+ * the add() function with the notification states to be included.
+ *
+ * Useful for community notification counts, global notification counts, etc.
+ */
+export class SummarizedNotificationState extends NotificationState {
+    private totalStatesWithUnread = 0;
+
+    constructor() {
+        super();
+        this._symbol = null;
+        this._count = 0;
+        this._color = NotificationColor.None;
+    }
+
+    public get numUnreadStates(): number {
+        return this.totalStatesWithUnread;
+    }
+
+    /**
+     * Append a notification state to this snapshot, taking the loudest NotificationColor
+     * of the two. By default this will not adopt the symbol of the other notification
+     * state to prevent the count from being lost in typical usage.
+     * @param other The other notification state to append.
+     * @param includeSymbol If true, the notification state's symbol will be taken if one
+     * is present.
+     */
+    public add(other: NotificationState, includeSymbol = false) {
+        if (other.symbol && includeSymbol) {
+            this._symbol = other.symbol;
+        }
+        if (other.count) {
+            this._count += other.count;
+        }
+        if (other.color > this.color) {
+            this._color = other.color;
+        }
+        if (other.hasUnreadCount) {
+            this.totalStatesWithUnread++;
+        }
+    }
+}

src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts

@@ -90,7 +90,7 @@ export class ImportanceAlgorithm extends OrderingAlgorithm {
     private getRoomCategory(room: Room): NotificationColor {
         // It's fine for us to call this a lot because it's cached, and we shouldn't be
         // wasting anything by doing so as the store holds single references
-        const state = RoomNotificationStateStore.instance.getRoomState(room, this.tagId);
+        const state = RoomNotificationStateStore.instance.getRoomState(room);
         return state.color;
     }