Merge pull request matrix-org#4828 from matrix-org/travis/room-list/proliferation

Fix a number of proliferation issues in the new room list

Author: turt2live

src/stores/room-list/RoomListStore2.ts

@@ -29,6 +29,7 @@ import { IFilterCondition } from "./filters/IFilterCondition";
 import { TagWatcher } from "./TagWatcher";
 import RoomViewStore from "../RoomViewStore";
 import { Algorithm, LIST_UPDATED_EVENT } from "./algorithms/Algorithm";
+import { EffectiveMembership, getEffectiveMembership } from "./membership";
 
 interface IState {
     tagsEnabled?: boolean;
@@ -97,8 +98,10 @@ export class RoomListStore2 extends AsyncStore<ActionPayload> {
             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) {
+            if (!activeRoom) {
+                console.warn(`${activeRoomId} is current in RVS but missing from client - clearing sticky room`);
+                this.algorithm.stickyRoom = null;
+            } else if (activeRoom !== this.algorithm.stickyRoom) {
                 // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
                 console.log(`Changing sticky room to ${activeRoomId}`);
                 this.algorithm.stickyRoom = activeRoom;
@@ -187,10 +190,13 @@ export class RoomListStore2 extends AsyncStore<ActionPayload> {
                 console.log(`[RoomListDebug] Live timeline event ${eventPayload.event.getId()} in ${updatedRoom.roomId}`);
                 if (eventPayload.event.getType() === 'm.room.tombstone' && eventPayload.event.getStateKey() === '') {
                     // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
-                    console.log(`[RoomListDebug] Got tombstone event - regenerating room list`);
-                    // TODO: We could probably be smarter about this: https://github.com/vector-im/riot-web/issues/14035
-                    await this.regenerateAllLists();
-                    return; // don't pass the update down - we will have already handled it in the regen
+                    console.log(`[RoomListDebug] Got tombstone event - trying to remove now-dead room`);
+                    const newRoom = this.matrixClient.getRoom(eventPayload.event.getContent()['replacement_room']);
+                    if (newRoom) {
+                        // If we have the new room, then the new room check will have seen the predecessor
+                        // and did the required updates, so do nothing here.
+                        return;
+                    }
                 }
                 await this.handleRoomUpdate(updatedRoom, RoomUpdateCause.Timeline);
             };
@@ -242,18 +248,49 @@ export class RoomListStore2 extends AsyncStore<ActionPayload> {
             }
         } else if (payload.action === 'MatrixActions.Room.myMembership') {
             const membershipPayload = (<any>payload); // TODO: Type out the dispatcher types
-            if (membershipPayload.oldMembership !== "join" && membershipPayload.membership === "join") {
+            const oldMembership = getEffectiveMembership(membershipPayload.oldMembership);
+            const newMembership = getEffectiveMembership(membershipPayload.membership);
+            if (oldMembership !== EffectiveMembership.Join && newMembership === EffectiveMembership.Join) {
                 // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
                 console.log(`[RoomListDebug] Handling new room ${membershipPayload.room.roomId}`);
-                await this.algorithm.handleRoomUpdate(membershipPayload.room, RoomUpdateCause.NewRoom);
+
+                // If we're joining an upgraded room, we'll want to make sure we don't proliferate
+                // the dead room in the list.
+                const createEvent = membershipPayload.room.currentState.getStateEvents("m.room.create", "");
+                if (createEvent && createEvent.getContent()['predecessor']) {
+                    console.log(`[RoomListDebug] Room has a predecessor`);
+                    const prevRoom = this.matrixClient.getRoom(createEvent.getContent()['predecessor']['room_id']);
+                    if (prevRoom) {
+                        const isSticky = this.algorithm.stickyRoom === prevRoom;
+                        if (isSticky) {
+                            console.log(`[RoomListDebug] Clearing sticky room due to room upgrade`);
+                            await this.algorithm.setStickyRoomAsync(null);
+                        }
+
+                        // Note: we hit the algorithm instead of our handleRoomUpdate() function to
+                        // avoid redundant updates.
+                        console.log(`[RoomListDebug] Removing previous room from room list`);
+                        await this.algorithm.handleRoomUpdate(prevRoom, RoomUpdateCause.RoomRemoved);
+                    }
+                }
+
+                console.log(`[RoomListDebug] Adding new room to room list`);
+                await this.handleRoomUpdate(membershipPayload.room, RoomUpdateCause.NewRoom);
+                return;
+            }
+
+            if (oldMembership !== EffectiveMembership.Invite && newMembership === EffectiveMembership.Invite) {
+                // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
+                console.log(`[RoomListDebug] Handling invite to ${membershipPayload.room.roomId}`);
+                await this.handleRoomUpdate(membershipPayload.room, RoomUpdateCause.NewRoom);
                 return;
             }
 
             // If it's not a join, it's transitioning into a different list (possibly historical)
-            if (membershipPayload.oldMembership !== membershipPayload.membership) {
+            if (oldMembership !== newMembership) {
                 // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
                 console.log(`[RoomListDebug] Handling membership change in ${membershipPayload.room.roomId}`);
-                await this.algorithm.handleRoomUpdate(membershipPayload.room, RoomUpdateCause.PossibleTagChange);
+                await this.handleRoomUpdate(membershipPayload.room, RoomUpdateCause.PossibleTagChange);
                 return;
             }
         }

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

@@ -30,7 +30,7 @@ import {
     SortAlgorithm
 } from "./models";
 import { FILTER_CHANGED, FilterPriority, IFilterCondition } from "../filters/IFilterCondition";
-import { EffectiveMembership, splitRoomsByMembership } from "../membership";
+import { EffectiveMembership, getEffectiveMembership, splitRoomsByMembership } from "../membership";
 import { OrderingAlgorithm } from "./list-ordering/OrderingAlgorithm";
 import { getListAlgorithmInstance } from "./list-ordering";
 
@@ -99,6 +99,14 @@ export class Algorithm extends EventEmitter {
         return this._cachedRooms;
     }
 
+    /**
+     * Awaitable version of the sticky room setter.
+     * @param val The new room to sticky.
+     */
+    public async setStickyRoomAsync(val: Room) {
+        await this.updateStickyRoom(val);
+    }
+
     public getTagSorting(tagId: TagID): SortAlgorithm {
         return this.sortAlgorithms[tagId];
     }
@@ -160,10 +168,13 @@ export class Algorithm extends EventEmitter {
         // It's possible to have no selected room. In that case, clear the sticky room
         if (!val) {
             if (this._stickyRoom) {
+                const stickyRoom = this._stickyRoom.room;
+                this._stickyRoom = null; // clear before we go to update the algorithm
+
                 // Lie to the algorithm and re-add the room to the algorithm
-                await this.handleRoomUpdate(this._stickyRoom.room, RoomUpdateCause.NewRoom);
+                await this.handleRoomUpdate(stickyRoom, RoomUpdateCause.NewRoom);
+                return;
             }
-            this._stickyRoom = null;
             return;
         }
 
@@ -289,6 +300,8 @@ export class Algorithm extends EventEmitter {
     }
 
     protected recalculateFilteredRoomsForTag(tagId: TagID): void {
+        if (!this.hasFilters) return; // don't bother doing work if there's nothing to do
+
         console.log(`Recalculating filtered rooms for ${tagId}`);
         delete this.filteredRooms[tagId];
         const rooms = this.cachedRooms[tagId].map(r => r); // cheap clone
@@ -428,6 +441,13 @@ export class Algorithm extends EventEmitter {
         if (isNullOrUndefined(rooms)) throw new Error(`Array of rooms cannot be null`);
         if (!this.sortAlgorithms) throw new Error(`Cannot set known rooms without a tag sorting map`);
 
+        console.warn("Resetting known rooms, initiating regeneration");
+
+        // Before we go any further we need to clear (but remember) the sticky room to
+        // avoid accidentally duplicating it in the list.
+        const oldStickyRoom = this._stickyRoom;
+        await this.updateStickyRoom(null);
+
         this.rooms = rooms;
 
         const newTags: ITagMap = {};
@@ -458,14 +478,7 @@ export class Algorithm extends EventEmitter {
 
         // Now process all the joined rooms. This is a bit more complicated
         for (const room of memberships[EffectiveMembership.Join]) {
-            let tags = Object.keys(room.tags || {});
-
-            if (tags.length === 0) {
-                // Check to see if it's a DM if it isn't anything else
-                if (DMRoomMap.shared().getUserIdForRoomId(room.roomId)) {
-                    tags = [DefaultTagID.DM];
-                }
-            }
+            const tags = this.getTagsOfJoinedRoom(room);
 
             let inTag = false;
             if (tags.length > 0) {
@@ -494,6 +507,54 @@ export class Algorithm extends EventEmitter {
 
         this.cachedRooms = newTags;
         this.updateTagsFromCache();
+        this.recalculateFilteredRooms();
+
+        // Now that we've finished generation, we need to update the sticky room to what
+        // it was. It's entirely possible that it changed lists though, so if it did then
+        // we also have to update the position of it.
+        if (oldStickyRoom && oldStickyRoom.room) {
+            await this.updateStickyRoom(oldStickyRoom.room);
+            if (this._stickyRoom && this._stickyRoom.room) { // just in case the update doesn't go according to plan
+                if (this._stickyRoom.tag !== oldStickyRoom.tag) {
+                    // We put the sticky room at the top of the list to treat it as an obvious tag change.
+                    this._stickyRoom.position = 0;
+                    this.recalculateStickyRoom(this._stickyRoom.tag);
+                }
+            }
+        }
+    }
+
+    private getTagsForRoom(room: Room): TagID[] {
+        // XXX: This duplicates a lot of logic from setKnownRooms above, but has a slightly
+        // different use case and therefore different performance curve
+
+        const tags: TagID[] = [];
+
+        const membership = getEffectiveMembership(room.getMyMembership());
+        if (membership === EffectiveMembership.Invite) {
+            tags.push(DefaultTagID.Invite);
+        } else if (membership === EffectiveMembership.Leave) {
+            tags.push(DefaultTagID.Archived);
+        } else {
+            tags.push(...this.getTagsOfJoinedRoom(room));
+        }
+
+        if (!tags.length) tags.push(DefaultTagID.Untagged);
+
+        return tags;
+    }
+
+    private getTagsOfJoinedRoom(room: Room): TagID[] {
+        let tags = Object.keys(room.tags || {});
+
+        if (tags.length === 0) {
+            // Check to see if it's a DM if it isn't anything else
+            if (DMRoomMap.shared().getUserIdForRoomId(room.roomId)) {
+                tags = [DefaultTagID.DM];
+            }
+        }
+
+        return tags;
     }
 
     /**
@@ -548,6 +609,14 @@ export class Algorithm extends EventEmitter {
     public async handleRoomUpdate(room: Room, cause: RoomUpdateCause): Promise<boolean> {
         if (!this.algorithms) throw new Error("Not ready: no algorithms to determine tags from");
 
+        if (cause === RoomUpdateCause.NewRoom) {
+            const roomTags = this.roomIdsToTags[room.roomId];
+            if (roomTags && roomTags.length > 0) {
+                console.warn(`${room.roomId} is reportedly new but is already known - assuming TagChange instead`);
+                cause = RoomUpdateCause.PossibleTagChange;
+            }
+        }
+
         if (cause === RoomUpdateCause.PossibleTagChange) {
             // TODO: Be smarter and splice rather than regen the planet. https://github.com/vector-im/riot-web/issues/14035
             // TODO: No-op if no change. https://github.com/vector-im/riot-web/issues/14035
@@ -566,6 +635,19 @@ export class Algorithm extends EventEmitter {
             }
         }
 
+        if (cause === RoomUpdateCause.NewRoom && !this.roomIdsToTags[room.roomId]) {
+            console.log(`[RoomListDebug] Updating tags for new room ${room.roomId} (${room.name})`);
+
+            // Get the tags for the room and populate the cache
+            const roomTags = this.getTagsForRoom(room).filter(t => !isNullOrUndefined(this.cachedRooms[t]));
+
+            // "This should never happen" condition - we specify DefaultTagID.Untagged in getTagsForRoom(),
+            // which means we should *always* have a tag to go off of.
+            if (!roomTags.length) throw new Error(`Tags cannot be determined for ${room.roomId}`);
+
+            this.roomIdsToTags[room.roomId] = roomTags;
+        }
+
         let tags = this.roomIdsToTags[room.roomId];
         if (!tags) {
             console.warn(`No tags known for "${room.name}" (${room.roomId})`);

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

@@ -179,45 +179,51 @@ export class ImportanceAlgorithm extends OrderingAlgorithm {
     }
 
     public async handleRoomUpdate(room: Room, cause: RoomUpdateCause): Promise<boolean> {
-        if (cause === RoomUpdateCause.NewRoom || cause === RoomUpdateCause.RoomRemoved) {
-            return this.handleSplice(room, cause);
-        }
+        try {
+            await this.updateLock.acquireAsync();
 
-        if (cause !== RoomUpdateCause.Timeline && cause !== RoomUpdateCause.ReadReceipt) {
-            throw new Error(`Unsupported update cause: ${cause}`);
-        }
+            if (cause === RoomUpdateCause.NewRoom || cause === RoomUpdateCause.RoomRemoved) {
+                return this.handleSplice(room, cause);
+            }
 
-        const category = this.getRoomCategory(room);
-        if (this.sortingAlgorithm === SortAlgorithm.Manual) {
-            return; // Nothing to do here.
-        }
+            if (cause !== RoomUpdateCause.Timeline && cause !== RoomUpdateCause.ReadReceipt) {
+                throw new Error(`Unsupported update cause: ${cause}`);
+            }
 
-        const roomIdx = this.getRoomIndex(room);
-        if (roomIdx === -1) {
-            throw new Error(`Room ${room.roomId} has no index in ${this.tagId}`);
-        }
+            const category = this.getRoomCategory(room);
+            if (this.sortingAlgorithm === SortAlgorithm.Manual) {
+                return; // Nothing to do here.
+            }
 
-        // Try to avoid doing array operations if we don't have to: only move rooms within
-        // the categories if we're jumping categories
-        const oldCategory = this.getCategoryFromIndices(roomIdx, this.indices);
-        if (oldCategory !== category) {
-            // Move the room and update the indices
-            this.moveRoomIndexes(1, oldCategory, category, this.indices);
-            this.cachedOrderedRooms.splice(roomIdx, 1); // splice out the old index (fixed position)
-            this.cachedOrderedRooms.splice(this.indices[category], 0, room); // splice in the new room (pre-adjusted)
-            // Note: if moveRoomIndexes() is called after the splice then the insert operation
-            // will happen in the wrong place. Because we would have already adjusted the index
-            // for the category, we don't need to determine how the room is moving in the list.
-            // If we instead tried to insert before updating the indices, we'd have to determine
-            // whether the room was moving later (towards IDLE) or earlier (towards RED) from its
-            // current position, as it'll affect the category's start index after we remove the
-            // room from the array.
-        }
+            const roomIdx = this.getRoomIndex(room);
+            if (roomIdx === -1) {
+                throw new Error(`Room ${room.roomId} has no index in ${this.tagId}`);
+            }
 
-        // Sort the category now that we've dumped the room in
-        await this.sortCategory(category);
+            // Try to avoid doing array operations if we don't have to: only move rooms within
+            // the categories if we're jumping categories
+            const oldCategory = this.getCategoryFromIndices(roomIdx, this.indices);
+            if (oldCategory !== category) {
+                // Move the room and update the indices
+                this.moveRoomIndexes(1, oldCategory, category, this.indices);
+                this.cachedOrderedRooms.splice(roomIdx, 1); // splice out the old index (fixed position)
+                this.cachedOrderedRooms.splice(this.indices[category], 0, room); // splice in the new room (pre-adjusted)
+                // Note: if moveRoomIndexes() is called after the splice then the insert operation
+                // will happen in the wrong place. Because we would have already adjusted the index
+                // for the category, we don't need to determine how the room is moving in the list.
+                // If we instead tried to insert before updating the indices, we'd have to determine
+                // whether the room was moving later (towards IDLE) or earlier (towards RED) from its
+                // current position, as it'll affect the category's start index after we remove the
+                // room from the array.
+            }
 
-        return true; // change made
+            // Sort the category now that we've dumped the room in
+            await this.sortCategory(category);
+
+            return true; // change made
+        } finally {
+            await this.updateLock.release();
+        }
     }
 
     private async sortCategory(category: Category) {