Restabilize room list ordering with prefiltering on spaces/communities

Fixes https://github.com/vector-im/element-web/issues/16799

This change replaces the "relative priority" system for filters with a kind model. The kind is used to differentiate and optimize when/where a filter condition is applied, resulting in a more stable ordering of the room list. The included documentation describes what this means in detail.

This also introduces a way to inhibit updates being emitted from the Algorithm class given what we're doing to the poor thing will cause it to do a bunch of recalculation. Inhibiting the update and implicitly applying it (as part of our updateFn.mark()/trigger steps) results in much better performance.

This has been tested on my own account with both communities and spaces of varying complexity: it feels faster, though the measurements appear to be within an error tolerance of each other (read: there's no performance impact of this).

docs/room-list-store.md

@@ -124,12 +124,19 @@ all kinds of filtering.
 
 ## Filtering
 
-Filters are provided to the store as condition classes, which are then passed along to the algorithm
+Filters are provided to the store as condition classes and have two major kinds: Prefilters and Runtime.
-implementations. The implementations then get to decide how to actually filter the rooms, however in
-practice the base `Algorithm` class deals with the filtering in a more optimized/generic way.
 
-The results of filters get cached to avoid needlessly iterating over potentially thousands of rooms,
+Prefilters flush out rooms which shouldn't appear to the algorithm implementations. Typically this is
-as the old room list store does. When a filter condition changes, it emits an update which (in this
+due to some higher order room list filtering (such as spaces or tags) deliberately exposing a subset of
+rooms to the user. The algorithm implementations will not see a room being prefiltered out.
+
+Runtime filters are used for more dynamic filtering, such as the user filtering by room name. These
+filters are passed along to the algorithm implementations where those implementations decide how and
+when to apply the filter. In practice, the base `Algorithm` class ends up doing the heavy lifting for
+optimization reasons.
+
+The results of runtime filters get cached to avoid needlessly iterating over potentially thousands of
+rooms, as the old room list store does. When a filter condition changes, it emits an update which (in this
 case) the `Algorithm` class will pick up and act accordingly. Typically, this also means filtering a
 minor subset where possible to avoid over-iterating rooms.
 
@@ -137,6 +144,13 @@ All filter conditions are considered "stable" by the consumers, meaning that the
 expect a change in the condition unless the condition says it has changed. This is intentional to
 maintain the caching behaviour described above.
 
+One might ask why we don't just use prefilter conditions for everything, and the answer is one of slight
+subtly: in the cases of prefilters we are knowingly exposing the user to a workspace-style UX where
+room notifications are self-contained within that workspace. Runtime filters tend to not want to affect
+visible notification counts (as it doesn't want the room header to suddenly be confusing to the user as
+they type), and occasionally UX like "found 2/12 rooms" is desirable. If prefiltering were used instead,
+the notification counts would vary while the user was typing and "found 2/12" UX would not be possible.
+
 ## Class breakdowns
 
 The `RoomListStore` is the major coordinator of various algorithm implementations, which take care

src/stores/room-list/RoomListStore.ts

@@ -1,6 +1,5 @@
 /*
-Copyright 2018, 2019 New Vector Ltd
+Copyright 2018-2021 The Matrix.org Foundation C.I.C.
-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.
@@ -23,7 +22,7 @@ import { IListOrderingMap, ITagMap, ITagSortingMap, ListAlgorithm, SortAlgorithm
 import {ActionPayload} from "../../dispatcher/payloads";
 import defaultDispatcher from "../../dispatcher/dispatcher";
 import {readReceiptChangeIsFor} from "../../utils/read-receipts";
-import { FILTER_CHANGED, IFilterCondition } from "./filters/IFilterCondition";
+import {FILTER_CHANGED, FilterKind, IFilterCondition} from "./filters/IFilterCondition";
 import {TagWatcher} from "./TagWatcher";
 import RoomViewStore from "../RoomViewStore";
 import {Algorithm, LIST_UPDATED_EVENT} from "./algorithms/Algorithm";
@@ -57,6 +56,7 @@ export class RoomListStoreClass extends AsyncStoreWithClient<IState> {
     private initialListsGenerated = false;
     private algorithm = new Algorithm();
     private filterConditions: IFilterCondition[] = [];
+    private prefilterConditions: IFilterCondition[] = [];
     private tagWatcher: TagWatcher;
     private spaceWatcher: SpaceWatcher;
     private updateFn = new MarkedExecution(() => {
@@ -104,6 +104,7 @@ export class RoomListStoreClass extends AsyncStoreWithClient<IState> {
     public async resetStore() {
         await this.reset();
         this.filterConditions = [];
+        this.prefilterConditions = [];
         this.initialListsGenerated = false;
         this.setupWatchers();
 
@@ -435,6 +436,39 @@ export class RoomListStoreClass extends AsyncStoreWithClient<IState> {
         }
     }
 
+    private async recalculatePrefiltering() {
+        if (!this.algorithm) return;
+        if (!this.algorithm.hasTagSortingMap) return; // we're still loading
+
+        if (SettingsStore.getValue("advancedRoomListLogging")) {
+            // TODO: Remove debug: https://github.com/vector-im/element-web/issues/14602
+            console.log("Calculating new prefiltered room list");
+        }
+
+        // Inhibit updates because we're about to lie heavily to the algorithm
+        this.algorithm.updatesInhibited = true;
+
+        // Figure out which rooms are about to be valid, and the state of affairs
+        const rooms = this.getPlausibleRooms();
+        const currentSticky = this.algorithm.stickyRoom;
+        const stickyIsStillPresent = currentSticky && rooms.includes(currentSticky);
+
+        // Reset the sticky room before resetting the known rooms so the algorithm
+        // doesn't freak out.
+        await this.algorithm.setStickyRoom(null);
+        await this.algorithm.setKnownRooms(rooms);
+
+        // Set the sticky room back, if needed, now that we have updated the store.
+        // This will use relative stickyness to the new room set.
+        if (stickyIsStillPresent) {
+            await this.algorithm.setStickyRoom(currentSticky);
+        }
+
+        // Finally, mark an update and resume updates from the algorithm
+        this.updateFn.mark();
+        this.algorithm.updatesInhibited = false;
+    }
+
     public async setTagSorting(tagId: TagID, sort: SortAlgorithm) {
         await this.setAndPersistTagSorting(tagId, sort);
         this.updateFn.trigger();
@@ -557,6 +591,34 @@ export class RoomListStoreClass extends AsyncStoreWithClient<IState> {
         this.updateFn.trigger();
     };
 
+    private onPrefilterUpdated = async () => {
+        await this.recalculatePrefiltering();
+        this.updateFn.trigger();
+    };
+
+    private getPlausibleRooms(): Room[] {
+        if (!this.matrixClient) return [];
+
+        let rooms = [
+            ...this.matrixClient.getVisibleRooms(),
+            // also show space invites in the room list
+            ...this.matrixClient.getRooms().filter(r => r.isSpaceRoom() && r.getMyMembership() === "invite"),
+        ].filter(r => VisibilityProvider.instance.isRoomVisible(r));
+
+        if (this.prefilterConditions.length > 0) {
+            rooms = rooms.filter(r => {
+                for (const filter of this.prefilterConditions) {
+                    if (!filter.isVisible(r)) {
+                        return false;
+                    }
+                }
+                return true;
+            });
+        }
+
+        return rooms;
+    }
+
     /**
      * Regenerates the room whole room list, discarding any previous results.
      *
@@ -568,11 +630,7 @@ export class RoomListStoreClass extends AsyncStoreWithClient<IState> {
     public async regenerateAllLists({trigger = true}) {
         console.warn("Regenerating all room lists");
 
-        const rooms = [
+        const rooms = this.getPlausibleRooms();
-            ...this.matrixClient.getVisibleRooms(),
-            // also show space invites in the room list
-            ...this.matrixClient.getRooms().filter(r => r.isSpaceRoom() && r.getMyMembership() === "invite"),
-        ].filter(r => VisibilityProvider.instance.isRoomVisible(r));
 
         const customTags = new Set<TagID>();
         if (this.state.tagsEnabled) {
@@ -606,11 +664,18 @@ export class RoomListStoreClass extends AsyncStoreWithClient<IState> {
             // TODO: Remove debug: https://github.com/vector-im/element-web/issues/14602
             console.log("Adding filter condition:", filter);
         }
+        let promise = Promise.resolve(); // use a promise to maintain sync API contract
+        if (filter.kind === FilterKind.Prefilter) {
+            filter.on(FILTER_CHANGED, this.onPrefilterUpdated);
+            this.prefilterConditions.push(filter);
+            promise = this.recalculatePrefiltering();
+        } else {
             this.filterConditions.push(filter);
             if (this.algorithm) {
                 this.algorithm.addFilterCondition(filter);
             }
-        this.updateFn.trigger();
+        }
+        promise.then(() => this.updateFn.trigger());
     }
 
     public removeFilter(filter: IFilterCondition): void {
@@ -618,7 +683,8 @@ export class RoomListStoreClass extends AsyncStoreWithClient<IState> {
             // TODO: Remove debug: https://github.com/vector-im/element-web/issues/14602
             console.log("Removing filter condition:", filter);
         }
-        const idx = this.filterConditions.indexOf(filter);
+        let promise = Promise.resolve(); // use a promise to maintain sync API contract
+        let idx = this.filterConditions.indexOf(filter);
         if (idx >= 0) {
             this.filterConditions.splice(idx, 1);
 
@@ -626,7 +692,13 @@ export class RoomListStoreClass extends AsyncStoreWithClient<IState> {
                 this.algorithm.removeFilterCondition(filter);
             }
         }
-        this.updateFn.trigger();
+        idx = this.prefilterConditions.indexOf(filter);
+        if (idx >= 0) {
+            filter.off(FILTER_CHANGED, this.onPrefilterUpdated);
+            this.prefilterConditions.splice(idx, 1);
+            promise = this.recalculatePrefiltering();
+        }
+        promise.then(() => this.updateFn.trigger());
     }
 
     /**

src/stores/room-list/algorithms/Algorithm.ts

@@ -1,5 +1,5 @@
 /*
-Copyright 2020 The Matrix.org Foundation C.I.C.
+Copyright 2020, 2021 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.
@@ -18,8 +18,7 @@ import { Room } from "matrix-js-sdk/src/models/room";
 import { isNullOrUndefined } from "matrix-js-sdk/src/utils";
 import DMRoomMap from "../../../utils/DMRoomMap";
 import { EventEmitter } from "events";
-import { arrayDiff, arrayHasDiff, ArrayUtil } from "../../../utils/arrays";
+import { arrayDiff, arrayHasDiff } from "../../../utils/arrays";
-import { getEnumValues } from "../../../utils/enums";
 import { DefaultTagID, RoomUpdateCause, TagID } from "../models";
 import {
     IListOrderingMap,
@@ -29,7 +28,7 @@ import {
     ListAlgorithm,
     SortAlgorithm,
 } from "./models";
-import { FILTER_CHANGED, FilterPriority, IFilterCondition } from "../filters/IFilterCondition";
+import { FILTER_CHANGED, FilterKind, IFilterCondition } from "../filters/IFilterCondition";
 import { EffectiveMembership, getEffectiveMembership, splitRoomsByMembership } from "../../../utils/membership";
 import { OrderingAlgorithm } from "./list-ordering/OrderingAlgorithm";
 import { getListAlgorithmInstance } from "./list-ordering";
@@ -79,6 +78,11 @@ export class Algorithm extends EventEmitter {
     private allowedByFilter: Map<IFilterCondition, Room[]> = new Map<IFilterCondition, Room[]>();
     private allowedRoomsByFilters: Set<Room> = new Set<Room>();
 
+    /**
+     * Set to true to suspend emissions of algorithm updates.
+     */
+    public updatesInhibited = false;
+
     public constructor() {
         super();
     }
@@ -87,6 +91,14 @@ export class Algorithm extends EventEmitter {
         return this._stickyRoom ? this._stickyRoom.room : null;
     }
 
+    public get knownRooms(): Room[] {
+        return this.rooms;
+    }
+
+    public get hasTagSortingMap(): boolean {
+        return !!this.sortAlgorithms;
+    }
+
     protected get hasFilters(): boolean {
         return this.allowedByFilter.size > 0;
     }
@@ -164,7 +176,7 @@ export class Algorithm extends EventEmitter {
 
             // If we removed the last filter, tell consumers that we've "updated" our filtered
             // view. This will trick them into getting the complete room list.
-            if (!this.hasFilters) {
+            if (!this.hasFilters && !this.updatesInhibited) {
                 this.emit(LIST_UPDATED_EVENT);
             }
         }
@@ -174,6 +186,7 @@ export class Algorithm extends EventEmitter {
         await this.recalculateFilteredRooms();
 
         // re-emit the update so the list store can fire an off-cycle update if needed
+        if (this.updatesInhibited) return;
         this.emit(FILTER_CHANGED);
     }
 
@@ -299,6 +312,7 @@ export class Algorithm extends EventEmitter {
         this.recalculateStickyRoom();
 
         // Finally, trigger an update
+        if (this.updatesInhibited) return;
         this.emit(LIST_UPDATED_EVENT);
     }
 
@@ -309,10 +323,6 @@ export class Algorithm extends EventEmitter {
 
         console.warn("Recalculating filtered room list");
         const filters = Array.from(this.allowedByFilter.keys());
-        const orderedFilters = new ArrayUtil(filters)
-            .groupBy(f => f.relativePriority)
-            .orderBy(getEnumValues(FilterPriority))
-            .value;
         const newMap: ITagMap = {};
         for (const tagId of Object.keys(this.cachedRooms)) {
             // Cheaply clone the rooms so we can more easily do operations on the list.
@@ -322,16 +332,7 @@ export class Algorithm extends EventEmitter {
             this.tryInsertStickyRoomToFilterSet(rooms, tagId);
             let remainingRooms = rooms.map(r => r);
             let allowedRoomsInThisTag = [];
-            let lastFilterPriority = orderedFilters[0].relativePriority;
+            for (const filter of filters) {
-            for (const filter of orderedFilters) {
-                if (filter.relativePriority !== lastFilterPriority) {
-                    // Every time the filter changes priority, we want more specific filtering.
-                    // To accomplish that, reset the variables to make it look like the process
-                    // has started over, but using the filtered rooms as the seed.
-                    remainingRooms = allowedRoomsInThisTag;
-                    allowedRoomsInThisTag = [];
-                    lastFilterPriority = filter.relativePriority;
-                }
                 const filteredRooms = remainingRooms.filter(r => filter.isVisible(r));
                 for (const room of filteredRooms) {
                     const idx = remainingRooms.indexOf(room);
@@ -350,6 +351,7 @@ export class Algorithm extends EventEmitter {
         const allowedRooms = Object.values(newMap).reduce((rv, v) => { rv.push(...v); return rv; }, <Room[]>[]);
         this.allowedRoomsByFilters = new Set(allowedRooms);
         this.filteredRooms = newMap;
+        if (this.updatesInhibited) return;
         this.emit(LIST_UPDATED_EVENT);
     }
 
@@ -404,6 +406,7 @@ export class Algorithm extends EventEmitter {
             if (!!this._cachedStickyRooms) {
                 // Clear the cache if we won't be needing it
                 this._cachedStickyRooms = null;
+                if (this.updatesInhibited) return;
                 this.emit(LIST_UPDATED_EVENT);
             }
             return;
@@ -446,6 +449,7 @@ export class Algorithm extends EventEmitter {
         }
 
         // Finally, trigger an update
+        if (this.updatesInhibited) return;
         this.emit(LIST_UPDATED_EVENT);
     }
 

src/stores/room-list/filters/CommunityFilterCondition.ts

@@ -1,5 +1,5 @@
 /*
-Copyright 2020 The Matrix.org Foundation C.I.C.
+Copyright 2020, 2021 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.
@@ -15,7 +15,7 @@ limitations under the License.
 */
 
 import { Room } from "matrix-js-sdk/src/models/room";
-import { FILTER_CHANGED, FilterPriority, IFilterCondition } from "./IFilterCondition";
+import { FILTER_CHANGED, FilterKind, IFilterCondition } from "./IFilterCondition";
 import { Group } from "matrix-js-sdk/src/models/group";
 import { EventEmitter } from "events";
 import GroupStore from "../../GroupStore";
@@ -39,9 +39,8 @@ export class CommunityFilterCondition extends EventEmitter implements IFilterCon
         this.onStoreUpdate(); // trigger a false update to seed the store
     }
 
-    public get relativePriority(): FilterPriority {
+    public get kind(): FilterKind {
-        // Lowest priority so we can coarsely find rooms.
+        return FilterKind.Prefilter;
-        return FilterPriority.Lowest;
     }
 
     public isVisible(room: Room): boolean {

src/stores/room-list/filters/IFilterCondition.ts

@@ -1,5 +1,5 @@
 /*
-Copyright 2020 The Matrix.org Foundation C.I.C.
+Copyright 2020, 2021 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.
@@ -19,10 +19,19 @@ import { EventEmitter } from "events";
 
 export const FILTER_CHANGED = "filter_changed";
 
-export enum FilterPriority {
+export enum FilterKind {
-    Lowest,
+    /**
-    // in the middle would be Low, Normal, and High if we had a need
+     * A prefilter is one which coarsely determines which rooms are
-    Highest,
+     * available for runtime filtering/rendering. Typically this will
+     * be things like Space selection.
+     */
+    Prefilter,
+
+    /**
+     * Runtime filters operate on the data set exposed by prefilters.
+     * Typically these are dynamic values like room name searching.
+     */
+    Runtime,
 }
 
 /**
@@ -39,10 +48,9 @@ export enum FilterPriority {
  */
 export interface IFilterCondition extends EventEmitter {
     /**
-     * The relative priority that this filter should be applied with.
+     * The kind of filter this presents.
-     * Lower priorities get applied first.
      */
-    relativePriority: FilterPriority;
+    kind: FilterKind;
 
     /**
      * Determines if a given room should be visible under this

src/stores/room-list/filters/NameFilterCondition.ts

@@ -1,5 +1,5 @@
 /*
-Copyright 2020 The Matrix.org Foundation C.I.C.
+Copyright 2020, 2021 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.
@@ -15,7 +15,7 @@ limitations under the License.
 */
 
 import { Room } from "matrix-js-sdk/src/models/room";
-import { FILTER_CHANGED, FilterPriority, IFilterCondition } from "./IFilterCondition";
+import { FILTER_CHANGED, FilterKind, IFilterCondition } from "./IFilterCondition";
 import { EventEmitter } from "events";
 import { removeHiddenChars } from "matrix-js-sdk/src/utils";
 import { throttle } from "lodash";
@@ -31,9 +31,8 @@ export class NameFilterCondition extends EventEmitter implements IFilterConditio
         super();
     }
 
-    public get relativePriority(): FilterPriority {
+    public get kind(): FilterKind {
-        // We want this one to be at the highest priority so it can search within other filters.
+        return FilterKind.Runtime;
-        return FilterPriority.Highest;
     }
 
     public get search(): string {

src/stores/room-list/filters/SpaceFilterCondition.ts

@@ -17,7 +17,7 @@ limitations under the License.
 import { EventEmitter } from "events";
 import { Room } from "matrix-js-sdk/src/models/room";
 
-import { FILTER_CHANGED, FilterPriority, IFilterCondition } from "./IFilterCondition";
+import { FILTER_CHANGED, FilterKind, IFilterCondition } from "./IFilterCondition";
 import { IDestroyable } from "../../../utils/IDestroyable";
 import SpaceStore, {HOME_SPACE} from "../../SpaceStore";
 import { setHasDiff } from "../../../utils/sets";
@@ -32,9 +32,8 @@ export class SpaceFilterCondition extends EventEmitter implements IFilterConditi
     private roomIds = new Set<Room>();
     private space: Room = null;
 
-    public get relativePriority(): FilterPriority {
+    public get kind(): FilterKind {
-        // Lowest priority so we can coarsely find rooms.
+        return FilterKind.Prefilter;
-        return FilterPriority.Lowest;
     }
 
     public isVisible(room: Room): boolean {
@@ -46,12 +45,7 @@ export class SpaceFilterCondition extends EventEmitter implements IFilterConditi
         this.roomIds = SpaceStore.instance.getSpaceFilteredRoomIds(this.space);
 
         if (setHasDiff(beforeRoomIds, this.roomIds)) {
-            // XXX: Room List Store has a bug where rooms which are synced after the filter is set
-            // are excluded from the filter, this is a workaround for it.
             this.emit(FILTER_CHANGED);
-            setTimeout(() => {
-                this.emit(FILTER_CHANGED);
-            }, 500);
         }
     };