From 928acbdc116b51627a40d5ae34521d56264133c4 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 21 Jul 2020 19:52:50 -0600 Subject: [PATCH 1/8] Wrap ScrollPanel layout changes in if statements These conditions are rarely true, but when they are it saves ~28ms of forced layout changes. --- src/components/structures/ScrollPanel.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index cb0114b243..7b8fec7aea 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -648,7 +648,9 @@ export default createReactClass({ if (scrollState.stuckAtBottom) { const sn = this._getScrollNode(); - sn.scrollTop = sn.scrollHeight; + 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); } } From 6a29cd33c13c69492f3c703a49d5599c39b33a05 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 21 Jul 2020 19:59:17 -0600 Subject: [PATCH 2/8] Remove tag specificity from notification states We don't need this complexity now that we aren't doing per-tag logic. --- .../RoomNotificationStateStore.ts | 28 +++++-------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/src/stores/notifications/RoomNotificationStateStore.ts b/src/stores/notifications/RoomNotificationStateStore.ts index ed74d3bae6..91147b0cf4 100644 --- a/src/stores/notifications/RoomNotificationStateStore.ts +++ b/src/stores/notifications/RoomNotificationStateStore.ts @@ -22,15 +22,12 @@ import { FetchRoomFn, ListNotificationState } from "./ListNotificationState"; import { Room } from "matrix-js-sdk/src/models/room"; import { RoomNotificationState } from "./RoomNotificationState"; -const INSPECIFIC_TAG = "INSPECIFIC_TAG"; -type INSPECIFIC_TAG = "INSPECIFIC_TAG"; - interface IState {} export class RoomNotificationStateStore extends AsyncStoreWithClient { private static internalInstance = new RoomNotificationStateStore(); - private roomMap = new Map>(); + private roomMap = new Map(); private constructor() { super(defaultDispatcher, {}); @@ -49,7 +46,7 @@ export class RoomNotificationStateStore extends AsyncStoreWithClient { // 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 +56,13 @@ export class RoomNotificationStateStore extends AsyncStoreWithClient { * 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()); + this.roomMap.set(room, new RoomNotificationState(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); + return this.roomMap.get(room); } public static get instance(): RoomNotificationStateStore { @@ -82,10 +70,8 @@ export class RoomNotificationStateStore extends AsyncStoreWithClient { } protected async onNotReady(): Promise { - for (const roomMap of this.roomMap.values()) { - for (const roomState of roomMap.values()) { - roomState.destroy(); - } + for (const roomState of this.roomMap.values()) { + roomState.destroy(); } } From dd16ec070c7446361a0d42d0f91076b13c92723e Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 21 Jul 2020 20:24:16 -0600 Subject: [PATCH 3/8] Replace countRoomsWithNotif with a dedicated NotificationState Fixes https://github.com/vector-im/riot-web/issues/14694 Instead of spending 10-1000ms in a function iterating over a whole lot of room events, we can use our cached state from the Notification State Store. This commit sets up a structure that could be applied to communities in the TagPanel too, as that could probably use a similar optimization. This reduces the updateStatusIndicator() time to just 4ms on average. --- src/RoomNotifs.js | 21 ------- src/components/structures/MatrixChat.tsx | 13 ++-- .../RoomNotificationStateStore.ts | 19 ++++++ .../SummarizedNotificationState.ts | 62 +++++++++++++++++++ 4 files changed, 87 insertions(+), 28 deletions(-) create mode 100644 src/stores/notifications/SummarizedNotificationState.ts diff --git a/src/RoomNotifs.js b/src/RoomNotifs.js index 4614bef378..a86c521ac4 100644 --- a/src/RoomNotifs.js +++ b/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); diff --git a/src/components/structures/MatrixChat.tsx b/src/components/structures/MatrixChat.tsx index 920b7e4dec..e68e1c53ae 100644 --- a/src/components/structures/MatrixChat.tsx +++ b/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 { } updateStatusIndicator(state: string, prevState: string) { - // only count visible rooms to not torment the user with notification counts in rooms they can't see - // it will include highlights from the previous version of the room internally - const notifCount = countRoomsWithNotif(MatrixClientPeg.get().getVisibleRooms()).count; + const notificationState = RoomNotificationStateStore.instance.globalState; + const numUnreadRooms = notificationState.numUnreadStates; // we know that states === rooms here 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) { - this.subTitleStatus += `[${notifCount}]`; + if (numUnreadRooms > 0) { + this.subTitleStatus += `[${numUnreadRooms}]`; } this.setPageSubtitle(); diff --git a/src/stores/notifications/RoomNotificationStateStore.ts b/src/stores/notifications/RoomNotificationStateStore.ts index 91147b0cf4..7c6ecb7f7a 100644 --- a/src/stores/notifications/RoomNotificationStateStore.ts +++ b/src/stores/notifications/RoomNotificationStateStore.ts @@ -21,6 +21,8 @@ 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 { NotificationState } from "./NotificationState"; +import { SummarizedNotificationState } from "./SummarizedNotificationState"; interface IState {} @@ -33,6 +35,23 @@ export class RoomNotificationStateStore extends AsyncStoreWithClient { 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. diff --git a/src/stores/notifications/SummarizedNotificationState.ts b/src/stores/notifications/SummarizedNotificationState.ts new file mode 100644 index 0000000000..9d5773b97e --- /dev/null +++ b/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++; + } + } +} From 507fa01ade0e0a7a5e0ad36cfb73a236f319bebc Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 21 Jul 2020 20:51:40 -0600 Subject: [PATCH 4/8] Remove missed area for notification state fetching --- src/components/views/rooms/RoomList.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/views/rooms/RoomList.tsx b/src/components/views/rooms/RoomList.tsx index 33c844e955..628bc41e60 100644 --- a/src/components/views/rooms/RoomList.tsx +++ b/src/components/views/rooms/RoomList.tsx @@ -210,7 +210,7 @@ export default class RoomList extends React.Component { 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; }); } From 97739c9a739c2be37858b4449c360318573ee148 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 21 Jul 2020 20:56:07 -0600 Subject: [PATCH 5/8] Add more statements to avoid layout changes --- src/components/structures/ScrollPanel.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index 7b8fec7aea..51113f4f56 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -699,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`; - sn.scrollTop = sn.scrollHeight; + 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(); @@ -714,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 From c9da1e187489d06a3e89ed9877ab07213b279ca1 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 21 Jul 2020 20:58:59 -0600 Subject: [PATCH 6/8] Remove even more tags from the notification state fetching for a room --- src/components/views/avatars/DecoratedRoomAvatar.tsx | 2 +- src/components/views/rooms/RoomTile.tsx | 2 +- .../room-list/algorithms/list-ordering/ImportanceAlgorithm.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/components/views/avatars/DecoratedRoomAvatar.tsx b/src/components/views/avatars/DecoratedRoomAvatar.tsx index bb737397dc..daf28400f2 100644 --- a/src/components/views/avatars/DecoratedRoomAvatar.tsx +++ b/src/components/views/avatars/DecoratedRoomAvatar.tsx @@ -44,7 +44,7 @@ export default class DecoratedRoomAvatar extends React.PureComponent { 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, diff --git a/src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts b/src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts index 8427663ad4..9cb7679e89 100644 --- a/src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts +++ b/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; } From cd77434a69e81bc6421ba45847eafb805c0ac768 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 21 Jul 2020 20:59:33 -0600 Subject: [PATCH 7/8] Appease the linter --- src/stores/notifications/RoomNotificationStateStore.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/stores/notifications/RoomNotificationStateStore.ts b/src/stores/notifications/RoomNotificationStateStore.ts index 7c6ecb7f7a..72fdd87ace 100644 --- a/src/stores/notifications/RoomNotificationStateStore.ts +++ b/src/stores/notifications/RoomNotificationStateStore.ts @@ -21,7 +21,6 @@ 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 { NotificationState } from "./NotificationState"; import { SummarizedNotificationState } from "./SummarizedNotificationState"; interface IState {} From 61c5b4f9bfe0b9152518556d0af893b76bb90d03 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 22 Jul 2020 08:23:47 -0600 Subject: [PATCH 8/8] deunderscore --- src/stores/notifications/SummarizedNotificationState.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/stores/notifications/SummarizedNotificationState.ts b/src/stores/notifications/SummarizedNotificationState.ts index 9d5773b97e..372da74f36 100644 --- a/src/stores/notifications/SummarizedNotificationState.ts +++ b/src/stores/notifications/SummarizedNotificationState.ts @@ -24,7 +24,7 @@ import { NotificationState } from "./NotificationState"; * Useful for community notification counts, global notification counts, etc. */ export class SummarizedNotificationState extends NotificationState { - private _totalStatesWithUnread = 0; + private totalStatesWithUnread = 0; constructor() { super(); @@ -34,7 +34,7 @@ export class SummarizedNotificationState extends NotificationState { } public get numUnreadStates(): number { - return this._totalStatesWithUnread; + return this.totalStatesWithUnread; } /** @@ -56,7 +56,7 @@ export class SummarizedNotificationState extends NotificationState { this._color = other.color; } if (other.hasUnreadCount) { - this._totalStatesWithUnread++; + this.totalStatesWithUnread++; } } }