From 900c234434d6590c8ab25be46699cb5eb263081c Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 27 Jul 2020 17:33:27 -0600 Subject: [PATCH] Internalize notification state handling for lists This reduces the update cost of rooms changing, and fixes a bug where when a sublist became filtered it would change the notification count of the sublist. This does change the expected usage of the state store to ensuring that only one place updates the rooms on the list states, which is currently the room list store. Ideally the state store could listen to the room list store to update itself, however due to a complicated require() loop it is not possible. --- src/components/views/rooms/RoomSublist.tsx | 15 ++++++--------- .../notifications/RoomNotificationStateStore.ts | 15 +++++++++------ src/stores/room-list/RoomListStore.ts | 13 ++++++++++++- src/stores/room-list/algorithms/Algorithm.ts | 4 ++++ 4 files changed, 31 insertions(+), 16 deletions(-) diff --git a/src/components/views/rooms/RoomSublist.tsx b/src/components/views/rooms/RoomSublist.tsx index 5d76cf7b05..28b23eaa6d 100644 --- a/src/components/views/rooms/RoomSublist.tsx +++ b/src/components/views/rooms/RoomSublist.tsx @@ -38,7 +38,6 @@ import { DefaultTagID, TagID } from "../../../stores/room-list/models"; import dis from "../../../dispatcher/dispatcher"; import defaultDispatcher from "../../../dispatcher/dispatcher"; import NotificationBadge from "./NotificationBadge"; -import { ListNotificationState } from "../../../stores/notifications/ListNotificationState"; import AccessibleTooltipButton from "../elements/AccessibleTooltipButton"; import { Key } from "../../../Keyboard"; import { ActionPayload } from "../../../dispatcher/payloads"; @@ -50,6 +49,7 @@ import RoomListLayoutStore from "../../../stores/room-list/RoomListLayoutStore"; import { arrayHasOrderChange } from "../../../utils/arrays"; import { objectExcluding, objectHasValueChange } from "../../../utils/objects"; import TemporaryTile from "./TemporaryTile"; +import { NotificationState } from "../../../stores/notifications/NotificationState"; const SHOW_N_BUTTON_HEIGHT = 28; // As defined by CSS const RESIZE_HANDLE_HEIGHT = 4; // As defined by CSS @@ -86,7 +86,6 @@ interface ResizeDelta { type PartialDOMRect = Pick; interface IState { - notificationState: ListNotificationState; contextMenuPosition: PartialDOMRect; isResizing: boolean; isExpanded: boolean; // used for the for expand of the sublist when the room list is being filtered @@ -102,6 +101,7 @@ export default class RoomSublist extends React.Component { private layout: ListLayout; private heightAtStart: number; private isBeingFiltered: boolean; + private notificationState: NotificationState; constructor(props: IProps) { super(props); @@ -109,8 +109,8 @@ export default class RoomSublist extends React.Component { this.layout = RoomListLayoutStore.instance.getLayoutFor(this.props.tagId); this.heightAtStart = 0; this.isBeingFiltered = !!RoomListStore.instance.getFirstNameFilterCondition(); + this.notificationState = RoomNotificationStateStore.instance.getListState(this.props.tagId); this.state = { - notificationState: RoomNotificationStateStore.instance.getListState(this.props.tagId), contextMenuPosition: null, isResizing: false, isExpanded: this.isBeingFiltered ? this.isBeingFiltered : !this.layout.isCollapsed, @@ -119,7 +119,6 @@ export default class RoomSublist extends React.Component { }; // Why Object.assign() and not this.state.height? Because TypeScript says no. this.state = Object.assign(this.state, {height: this.calculateInitialHeight()}); - this.state.notificationState.setRooms(this.state.rooms); this.dispatcherRef = defaultDispatcher.register(this.onAction); RoomListStore.instance.on(LISTS_UPDATE_EVENT, this.onListsUpdated); } @@ -173,7 +172,6 @@ export default class RoomSublist extends React.Component { } public componentDidUpdate(prevProps: Readonly, prevState: Readonly) { - this.state.notificationState.setRooms(this.state.rooms); const prevExtraTiles = prevState.filteredExtraTiles || prevProps.extraBadTilesThatShouldntExist; // as the rooms can come in one by one we need to reevaluate // the amount of available rooms to cap the amount of requested visible rooms by the layout @@ -239,7 +237,6 @@ export default class RoomSublist extends React.Component { } public componentWillUnmount() { - this.state.notificationState.destroy(); defaultDispatcher.unregister(this.dispatcherRef); RoomListStore.instance.off(LISTS_UPDATE_EVENT, this.onListsUpdated); } @@ -409,8 +406,8 @@ export default class RoomSublist extends React.Component { } else { // find the first room with a count of the same colour as the badge count room = this.state.rooms.find((r: Room) => { - const notifState = this.state.notificationState.getForRoom(r); - return notifState.count > 0 && notifState.color === this.state.notificationState.color; + const notifState = this.notificationState.getForRoom(r); + return notifState.count > 0 && notifState.color === this.notificationState.color; }); } @@ -626,7 +623,7 @@ export default class RoomSublist extends React.Component { const badge = ( { private static internalInstance = new RoomNotificationStateStore(); private roomMap = new Map(); + private listMap = new Map(); private constructor() { super(defaultDispatcher, {}); @@ -52,21 +53,23 @@ export class RoomNotificationStateStore extends AsyncStoreWithClient { } /** - * 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. - * @param tagId The tag to create the notification state for. + * Gets an instance of the list state class for the given tag. + * @param tagId The tag to get the notification state for. * @returns The notification state for the tag. */ public getListState(tagId: TagID): ListNotificationState { - // Note: we don't cache these notification states as the consumer is expected to call - // .setRooms() on the returned object, which could confuse other consumers. + if (this.listMap.has(tagId)) { + return this.listMap.get(tagId); + } // 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); }; - return new ListNotificationState(useTileCount, tagId, getRoomFn); + const state = new ListNotificationState(useTileCount, tagId, getRoomFn); + this.listMap.set(tagId, state); + return state; } /** diff --git a/src/stores/room-list/RoomListStore.ts b/src/stores/room-list/RoomListStore.ts index 7049381c75..c3e7fd5c51 100644 --- a/src/stores/room-list/RoomListStore.ts +++ b/src/stores/room-list/RoomListStore.ts @@ -33,6 +33,7 @@ import RoomListLayoutStore from "./RoomListLayoutStore"; import { MarkedExecution } from "../../utils/MarkedExecution"; import { AsyncStoreWithClient } from "../AsyncStoreWithClient"; import { NameFilterCondition } from "./filters/NameFilterCondition"; +import { RoomNotificationStateStore } from "../notifications/RoomNotificationStateStore"; interface IState { tagsEnabled?: boolean; @@ -55,7 +56,12 @@ export class RoomListStoreClass extends AsyncStoreWithClient { private algorithm = new Algorithm(); private filterConditions: IFilterCondition[] = []; private tagWatcher = new TagWatcher(this); - private updateFn = new MarkedExecution(() => this.emit(LISTS_UPDATE_EVENT)); + private updateFn = new MarkedExecution(() => { + for (const tagId of Object.keys(this.unfilteredLists)) { + RoomNotificationStateStore.instance.getListState(tagId).setRooms(this.unfilteredLists[tagId]); + } + this.emit(LISTS_UPDATE_EVENT); + }); private readonly watchedSettings = [ 'feature_custom_tags', @@ -72,6 +78,11 @@ export class RoomListStoreClass extends AsyncStoreWithClient { this.algorithm.on(FILTER_CHANGED, this.onAlgorithmFilterUpdated); } + public get unfilteredLists(): ITagMap { + if (!this.algorithm) return {}; // No tags yet. + return this.algorithm.getUnfilteredRooms(); + } + public get orderedLists(): ITagMap { if (!this.algorithm) return {}; // No tags yet. return this.algorithm.getOrderedRooms(); diff --git a/src/stores/room-list/algorithms/Algorithm.ts b/src/stores/room-list/algorithms/Algorithm.ts index 667084d653..9b2779d900 100644 --- a/src/stores/room-list/algorithms/Algorithm.ts +++ b/src/stores/room-list/algorithms/Algorithm.ts @@ -465,6 +465,10 @@ export class Algorithm extends EventEmitter { return this.filteredRooms; } + public getUnfilteredRooms(): ITagMap { + return this._cachedStickyRooms || this.cachedRooms; + } + /** * This returns the same as getOrderedRooms(), but without the sticky room * map as it causes issues for sticky room handling (see sticky room handling