Merge pull request #4720 from matrix-org/travis/room-list/sticky

turt2live · web-flow · commit 19a7297a770a · 2020-06-07T19:05:07.000-06:00
Introduce sticky rooms to the new room list
diff --git a/src/stores/room-list/README.md b/src/stores/room-list/README.md
@@ -74,29 +74,29 @@ gets applied to each category in a sub-sub-list fashion. This should result in t
 being sorted alphabetically amongst each other as well as the grey rooms sorted amongst each other, but 
 collectively the tag will be sorted into categories with red being at the top.
 
-<!-- TODO: Implement sticky rooms as described below -->
-
-The algorithm also has a concept of a 'sticky' room which is the room the user is currently viewing.
-The sticky room will remain in position on the room list regardless of other factors going on as typically
-clicking on a room will cause it to change categories into 'idle'. This is done by preserving N rooms
-above the selected room at all times, where N is the number of rooms above the selected rooms when it was
-selected.
-
-For example, if the user has 3 red rooms and selects the middle room, they will always see exactly one
-room above their selection at all times. If they receive another notification, and the tag ordering is 
-specified as Recent, they'll see the new notification go to the top position, and the one that was previously
-there fall behind the sticky room.
-
-The sticky room's category is technically 'idle' while being viewed and is explicitly pulled out of the
-tag sorting algorithm's input as it must maintain its position in the list. When the user moves to another
-room, the previous sticky room gets recalculated to determine which category it needs to be in as the user
-could have been scrolled up while new messages were received.
-
-Further, the sticky room is not aware of category boundaries and thus the user can see a shift in what 
-kinds of rooms move around their selection. An example would be the user having 4 red rooms, the user 
-selecting the third room (leaving 2 above it), and then having the rooms above it read on another device. 
-This would result in 1 red room and 1 other kind of room above the sticky room as it will try to maintain 
-2 rooms above the sticky room.
+### Sticky rooms
+
+When the user visits a room, that room becomes 'sticky' in the list, regardless of ordering algorithm.
+From a code perspective, the underlying algorithm is not aware of a sticky room and instead the base class
+manages which room is sticky. This is to ensure that all algorithms handle it the same.
+
+The sticky flag is simply to say it will not move higher or lower down the list while it is active. For
+example, if using the importance algorithm, the room would naturally become idle once viewed and thus
+would normally fly down the list out of sight. The sticky room concept instead holds it in place, never
+letting it fly down until the user moves to another room.
+
+Only one room can be sticky at a time. Room updates around the sticky room will still hold the sticky
+room in place. The best example of this is the importance algorithm: if the user has 3 red rooms and
+selects the middle room, they will see exactly one room above their selection at all times. If they
+receive another notification which causes the room to move into the topmost position, the room that was
+above the sticky room will move underneath to allow for the new room to take the top slot, maintaining
+the sticky room's position.
+
+Though only applicable to the importance algorithm, the sticky room is not aware of category boundaries 
+and thus the user can see a shift in what kinds of rooms move around their selection. An example would 
+be the user having 4 red rooms, the user selecting the third room (leaving 2 above it), and then having 
+the rooms above it read on another device. This would result in 1 red room and 1 other kind of room 
+above the sticky room as it will try to maintain 2 rooms above the sticky room.
 
 An exception for the sticky room placement is when there's suddenly not enough rooms to maintain the placement
 exactly. This typically happens if the user selects a room and leaves enough rooms where it cannot maintain
diff --git a/src/stores/room-list/RoomListStore2.ts b/src/stores/room-list/RoomListStore2.ts
@@ -29,6 +29,7 @@ import defaultDispatcher from "../../dispatcher/dispatcher";
 import { readReceiptChangeIsFor } from "../../utils/read-receipts";
 import { IFilterCondition } from "./filters/IFilterCondition";
 import { TagWatcher } from "./TagWatcher";
+import RoomViewStore from "../RoomViewStore";
 
 interface IState {
     tagsEnabled?: boolean;
@@ -62,6 +63,7 @@ export class RoomListStore2 extends AsyncStore<ActionPayload> {
 
         this.checkEnabled();
         for (const settingName of this.watchedSettings) SettingsStore.monitorSetting(settingName, null);
+        RoomViewStore.addListener(this.onRVSUpdate);
     }
 
     public get orderedLists(): ITagMap {
@@ -93,6 +95,23 @@ export class RoomListStore2 extends AsyncStore<ActionPayload> {
         this.setAlgorithmClass();
     }
 
+    private onRVSUpdate = () => {
+        if (!this.enabled) return; // TODO: Remove enabled flag when RoomListStore2 takes over
+        if (!this.matrixClient) return; // We assume there won't be RVS updates without a client
+
+        const activeRoomId = RoomViewStore.getRoomId();
+        if (!activeRoomId && this.algorithm.stickyRoom) {
+            this.algorithm.stickyRoom = null;
+        } else if (activeRoomId) {
+            const activeRoom = this.matrixClient.getRoom(activeRoomId);
+            if (!activeRoom) throw new Error(`${activeRoomId} is current in RVS but missing from client`);
+            if (activeRoom !== this.algorithm.stickyRoom) {
+                console.log(`Changing sticky room to ${activeRoomId}`);
+                this.algorithm.stickyRoom = activeRoom;
+            }
+        }
+    };
+
     protected async onDispatch(payload: ActionPayload) {
         if (payload.action === 'MatrixActions.sync') {
             // Filter out anything that isn't the first PREPARED sync.
@@ -110,6 +129,7 @@ export class RoomListStore2 extends AsyncStore<ActionPayload> {
             console.log("Regenerating room lists: Startup");
             await this.readAndCacheSettingsFromStore();
             await this.regenerateAllLists();
+            this.onRVSUpdate(); // fake an RVS update to adjust sticky room, if needed
         }
 
         // TODO: Remove this once the RoomListStore becomes default
@@ -228,9 +248,6 @@ export class RoomListStore2 extends AsyncStore<ActionPayload> {
                 await this.algorithm.handleRoomUpdate(membershipPayload.room, RoomUpdateCause.PossibleTagChange);
                 return;
             }
-        } else if (payload.action === 'view_room') {
-            // TODO: Update sticky room
-            console.log(payload);
         }
     }
 
diff --git a/src/stores/room-list/algorithms/list-ordering/Algorithm.ts b/src/stores/room-list/algorithms/list-ordering/Algorithm.ts
@@ -22,6 +22,7 @@ import { ITagMap, ITagSortingMap } from "../models";
 import DMRoomMap from "../../../../utils/DMRoomMap";
 import { FILTER_CHANGED, IFilterCondition } from "../../filters/IFilterCondition";
 import { EventEmitter } from "events";
+import { UPDATE_EVENT } from "../../../AsyncStore";
 
 // TODO: Add locking support to avoid concurrent writes?
 
@@ -30,14 +31,22 @@ import { EventEmitter } from "events";
  */
 export const LIST_UPDATED_EVENT = "list_updated_event";
 
+interface IStickyRoom {
+    room: Room;
+    position: number;
+    tag: TagID;
+}
+
 /**
  * Represents a list ordering algorithm. This class will take care of tag
  * management (which rooms go in which tags) and ask the implementation to
  * deal with ordering mechanics.
  */
 export abstract class Algorithm extends EventEmitter {
     private _cachedRooms: ITagMap = {};
+    private _cachedStickyRooms: ITagMap = {}; // a clone of the _cachedRooms, with the sticky room
     private filteredRooms: ITagMap = {};
+    private _stickyRoom: IStickyRoom = null;
 
     protected sortAlgorithms: ITagSortingMap;
     protected rooms: Room[] = [];
@@ -51,16 +60,30 @@ export abstract class Algorithm extends EventEmitter {
         super();
     }
 
+    public get stickyRoom(): Room {
+        return this._stickyRoom ? this._stickyRoom.room : null;
+    }
+
+    public set stickyRoom(val: Room) {
+        // setters can't be async, so we call a private function to do the work
+        this.updateStickyRoom(val);
+    }
+
     protected get hasFilters(): boolean {
         return this.allowedByFilter.size > 0;
     }
 
     protected set cachedRooms(val: ITagMap) {
         this._cachedRooms = val;
         this.recalculateFilteredRooms();
+        this.recalculateStickyRoom();
     }
 
     protected get cachedRooms(): ITagMap {
+        // 🐉 Here be dragons.
+        // Note: this is used by the underlying algorithm classes, so don't make it return
+        // the sticky room cache. If it ends up returning the sticky room cache, we end up
+        // corrupting our caches and confusing them.
         return this._cachedRooms;
     }
 
@@ -94,6 +117,67 @@ export abstract class Algorithm extends EventEmitter {
         }
     }
 
+    private async updateStickyRoom(val: Room) {
+        // Note throughout: We need async so we can wait for handleRoomUpdate() to do its thing,
+        // otherwise we risk duplicating rooms.
+
+        // It's possible to have no selected room. In that case, clear the sticky room
+        if (!val) {
+            if (this._stickyRoom) {
+                // Lie to the algorithm and re-add the room to the algorithm
+                await this.handleRoomUpdate(this._stickyRoom.room, RoomUpdateCause.NewRoom);
+            }
+            this._stickyRoom = null;
+            return;
+        }
+
+        // When we do have a room though, we expect to be able to find it
+        const tag = this.roomIdsToTags[val.roomId][0];
+        if (!tag) throw new Error(`${val.roomId} does not belong to a tag and cannot be sticky`);
+        let position = this.cachedRooms[tag].indexOf(val);
+        if (position < 0) throw new Error(`${val.roomId} does not appear to be known and cannot be sticky`);
+
+        // 🐉 Here be dragons.
+        // Before we can go through with lying to the underlying algorithm about a room
+        // we need to ensure that when we do we're ready for the innevitable sticky room
+        // update we'll receive. To prepare for that, we first remove the sticky room and
+        // recalculate the state ourselves so that when the underlying algorithm calls for
+        // the same thing it no-ops. After we're done calling the algorithm, we'll issue
+        // a new update for ourselves.
+        const lastStickyRoom = this._stickyRoom;
+        console.log(`Last sticky room:`, lastStickyRoom);
+        this._stickyRoom = null;
+        this.recalculateStickyRoom();
+
+        // When we do have the room, re-add the old room (if needed) to the algorithm
+        // and remove the sticky room from the algorithm. This is so the underlying
+        // algorithm doesn't try and confuse itself with the sticky room concept.
+        if (lastStickyRoom) {
+            // Lie to the algorithm and re-add the room to the algorithm
+            await this.handleRoomUpdate(lastStickyRoom.room, RoomUpdateCause.NewRoom);
+        }
+        // Lie to the algorithm and remove the room from it's field of view
+        await this.handleRoomUpdate(val, RoomUpdateCause.RoomRemoved);
+
+        // Now that we're done lying to the algorithm, we need to update our position
+        // marker only if the user is moving further down the same list. If they're switching
+        // lists, or moving upwards, the position marker will splice in just fine but if
+        // they went downwards in the same list we'll be off by 1 due to the shifting rooms.
+        if (lastStickyRoom && lastStickyRoom.tag === tag && lastStickyRoom.position <= position) {
+            position++;
+        }
+
+        this._stickyRoom = {
+            room: val,
+            position: position,
+            tag: tag,
+        };
+        this.recalculateStickyRoom();
+
+        // Finally, trigger an update
+        this.emit(LIST_UPDATED_EVENT);
+    }
+
     protected recalculateFilteredRooms() {
         if (!this.hasFilters) {
             return;
@@ -154,6 +238,59 @@ export abstract class Algorithm extends EventEmitter {
         console.log(`[DEBUG] ${filteredRooms.length}/${rooms.length} rooms filtered into ${tagId}`);
     }
 
+    /**
+     * Recalculate the sticky room position. If this is being called in relation to
+     * a specific tag being updated, it should be given to this function to optimize
+     * the call.
+     * @param updatedTag The tag that was updated, if possible.
+     */
+    protected recalculateStickyRoom(updatedTag: TagID = null): void {
+        // 🐉 Here be dragons.
+        // This function does far too much for what it should, and is called by many places.
+        // Not only is this responsible for ensuring the sticky room is held in place at all
+        // times, it is also responsible for ensuring our clone of the cachedRooms is up to
+        // date. If either of these desyncs, we see weird behaviour like duplicated rooms,
+        // outdated lists, and other nonsensical issues that aren't necessarily obvious.
+
+        if (!this._stickyRoom) {
+            // If there's no sticky room, just do nothing useful.
+            if (!!this._cachedStickyRooms) {
+                // Clear the cache if we won't be needing it
+                this._cachedStickyRooms = null;
+                this.emit(LIST_UPDATED_EVENT);
+            }
+            return;
+        }
+
+        if (!this._cachedStickyRooms || !updatedTag) {
+            console.log(`Generating clone of cached rooms for sticky room handling`);
+            const stickiedTagMap: ITagMap = {};
+            for (const tagId of Object.keys(this.cachedRooms)) {
+                stickiedTagMap[tagId] = this.cachedRooms[tagId].map(r => r); // shallow clone
+            }
+            this._cachedStickyRooms = stickiedTagMap;
+        }
+
+        if (updatedTag) {
+            // Update the tag indicated by the caller, if possible. This is mostly to ensure
+            // our cache is up to date.
+            console.log(`Replacing cached sticky rooms for ${updatedTag}`);
+            this._cachedStickyRooms[updatedTag] = this.cachedRooms[updatedTag].map(r => r); // shallow clone
+        }
+
+        // Now try to insert the sticky room, if we need to.
+        // We need to if there's no updated tag (we regenned the whole cache) or if the tag
+        // we might have updated from the cache is also our sticky room.
+        const sticky = this._stickyRoom;
+        if (!updatedTag || updatedTag === sticky.tag) {
+            console.log(`Inserting sticky room ${sticky.room.roomId} at position ${sticky.position} in ${sticky.tag}`);
+            this._cachedStickyRooms[sticky.tag].splice(sticky.position, 0, sticky.room);
+        }
+
+        // Finally, trigger an update
+        this.emit(LIST_UPDATED_EVENT);
+    }
+
     /**
      * Asks the Algorithm to regenerate all lists, using the tags given
      * as reference for which lists to generate and which way to generate
@@ -174,7 +311,7 @@ export abstract class Algorithm extends EventEmitter {
      */
     public getOrderedRooms(): ITagMap {
         if (!this.hasFilters) {
-            return this.cachedRooms;
+            return this._cachedStickyRooms || this.cachedRooms;
         }
         return this.filteredRooms;
     }
diff --git a/src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts b/src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts
@@ -17,7 +17,7 @@ limitations under the License.
 
 import { Algorithm } from "./Algorithm";
 import { Room } from "matrix-js-sdk/src/models/room";
-import { DefaultTagID, RoomUpdateCause, TagID } from "../../models";
+import { RoomUpdateCause, TagID } from "../../models";
 import { ITagMap, SortAlgorithm } from "../models";
 import { sortRoomsWithAlgorithm } from "../tag-sorting";
 import * as Unread from '../../../../Unread';
@@ -82,15 +82,14 @@ export class ImportanceAlgorithm extends Algorithm {
     // HOW THIS WORKS
     // --------------
     //
-    // This block of comments assumes you've read the README one level higher.
+    // This block of comments assumes you've read the README two levels higher.
     // You should do that if you haven't already.
     //
     // Tags are fed into the algorithmic functions from the Algorithm superclass,
     // which cause subsequent updates to the room list itself. Categories within
     // those tags are tracked as index numbers within the array (zero = top), with
     // each sticky room being tracked separately. Internally, the category index
-    // can be found from `this.indices[tag][category]` and the sticky room information
-    // from `this.stickyRoom`.
+    // can be found from `this.indices[tag][category]`.
     //
     // The room list store is always provided with the `this.cachedRooms` results, which are
     // updated as needed and not recalculated often. For example, when a room needs to
@@ -102,17 +101,6 @@ export class ImportanceAlgorithm extends Algorithm {
         [tag: TagID]: ICategoryIndex;
     } = {};
 
-    // TODO: Use this (see docs above)
-    private stickyRoom: {
-        roomId: string;
-        tag: TagID;
-        fromTop: number;
-    } = {
-        roomId: null,
-        tag: null,
-        fromTop: 0,
-    };
-
     constructor() {
         super();
         console.log("Constructed an ImportanceAlgorithm");
@@ -202,6 +190,12 @@ export class ImportanceAlgorithm extends Algorithm {
             return;
         }
 
+        if (cause === RoomUpdateCause.RoomRemoved) {
+            // TODO: Be smarter and splice rather than regen the planet.
+            await this.setKnownRooms(this.rooms.filter(r => r !== room));
+            return;
+        }
+
         let tags = this.roomIdsToTags[room.roomId];
         if (!tags) {
             console.warn(`No tags known for "${room.name}" (${room.roomId})`);
@@ -258,6 +252,8 @@ export class ImportanceAlgorithm extends Algorithm {
             taggedRooms.splice(startIdx, 0, ...sorted);
 
             // Finally, flag that we've done something
+            this.recalculateFilteredRoomsForTag(tag); // update filter to re-sort the list
+            this.recalculateStickyRoom(tag); // update sticky room to make sure it appears if needed
             changed = true;
         }
         return changed;
diff --git a/src/stores/room-list/algorithms/list-ordering/NaturalAlgorithm.ts b/src/stores/room-list/algorithms/list-ordering/NaturalAlgorithm.ts
@@ -46,11 +46,17 @@ export class NaturalAlgorithm extends Algorithm {
             console.warn(`No tags known for "${room.name}" (${room.roomId})`);
             return false;
         }
+        let changed = false;
         for (const tag of tags) {
             // TODO: Optimize this loop to avoid useless operations
             // For example, we can skip updates to alphabetic (sometimes) and manually ordered tags
             this.cachedRooms[tag] = await sortRoomsWithAlgorithm(this.cachedRooms[tag], tag, this.sortAlgorithms[tag]);
+
+            // Flag that we've done something
+            this.recalculateFilteredRoomsForTag(tag); // update filter to re-sort the list
+            this.recalculateStickyRoom(tag); // update sticky room to make sure it appears if needed
+            changed = true;
         }
-        return true; // assume we changed something
+        return changed;
     }
 }
diff --git a/src/stores/room-list/models.ts b/src/stores/room-list/models.ts

Original file line number	Diff line number	Diff line change
`@@ -46,11 +46,17 @@ export class NaturalAlgorithm extends Algorithm {`
`46`	`46`	console.warn(`No tags known for "${room.name}" (${room.roomId})`);
`47`	`47`	`return false;`
`48`	`48`	`}`
	`49`	`+ let changed = false;`
`49`	`50`	`for (const tag of tags) {`
`50`	`51`	`// TODO: Optimize this loop to avoid useless operations`
`51`	`52`	`// For example, we can skip updates to alphabetic (sometimes) and manually ordered tags`
`52`	`53`	`this.cachedRooms[tag] = await sortRoomsWithAlgorithm(this.cachedRooms[tag], tag, this.sortAlgorithms[tag]);`
	`54`	`+`
	`55`	`+ // Flag that we've done something`
	`56`	`+ this.recalculateFilteredRoomsForTag(tag); // update filter to re-sort the list`
	`57`	`+ this.recalculateStickyRoom(tag); // update sticky room to make sure it appears if needed`
	`58`	`+ changed = true;`
`53`	`59`	`}`
`54`		`- return true; // assume we changed something`
	`60`	`+ return changed;`
`55`	`61`	`}`
`56`	`62`	`}`