From 8db67743f74a75667f84546e2cb5195f7960abbc Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 17 Jun 2020 22:42:01 -0600 Subject: [PATCH] Improve room switching in the new room list For https://github.com/vector-im/riot-web/issues/14034 One of the largest issues with room switching was that we'd regenerate the entire list when the sticky room changes, which is obviously detrimental on larger accounts (and even some medium accounts). To work through this, we simply handle the NewRoom and RoomRemoved causes (used by the sticky room handling) as splices rather than in-place updates. Overall this leads to a smoother experience as it means we're doing far less calculations and can even opt out of an update if it isn't required, such as a RoomRemoved cause being fired twice - the second one can result in an update not being required, saving render time. This commit also includes a fix for handling update causes on the sticky room, as the room list loves to print errors when this happens. We don't need to handle any updates because once the sticky room changes it'll get re-added through NewRoom, causing the underlying algorithm to slot it in where needed, effectively handling all the missed updates. --- src/stores/room-list/algorithms/Algorithm.ts | 18 ++-- .../list-ordering/ImportanceAlgorithm.ts | 87 ++++++++++++++----- .../list-ordering/NaturalAlgorithm.ts | 14 ++- .../list-ordering/OrderingAlgorithm.ts | 1 - 4 files changed, 84 insertions(+), 36 deletions(-) diff --git a/src/stores/room-list/algorithms/Algorithm.ts b/src/stores/room-list/algorithms/Algorithm.ts index a89167095d..9cf42a55fc 100644 --- a/src/stores/room-list/algorithms/Algorithm.ts +++ b/src/stores/room-list/algorithms/Algorithm.ts @@ -508,16 +508,14 @@ export class Algorithm extends EventEmitter { return true; } - if (cause === RoomUpdateCause.NewRoom) { - // TODO: Be smarter and insert rather than regen the planet. - await this.setKnownRooms([room, ...this.rooms]); - return true; - } - - if (cause === RoomUpdateCause.RoomRemoved) { - // TODO: Be smarter and splice rather than regen the planet. - await this.setKnownRooms(this.rooms.filter(r => r !== room)); - return true; + // If the update is for a room change which might be the sticky room, prevent it. We + // need to make sure that the causes (NewRoom and RoomRemoved) are still triggered though + // as the sticky room relies on this. + if (cause !== RoomUpdateCause.NewRoom && cause !== RoomUpdateCause.RoomRemoved) { + if (this.stickyRoom === room) { + console.warn(`[RoomListDebug] Received ${cause} update for sticky room ${room.roomId} - ignoring`); + return false; + } } let tags = this.roomIdsToTags[room.roomId]; diff --git a/src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts b/src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts index 325aaf19e6..6e09b0f8d3 100644 --- a/src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts +++ b/src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts @@ -87,7 +87,7 @@ export class ImportanceAlgorithm extends OrderingAlgorithm { public constructor(tagId: TagID, initialSortingAlgorithm: SortAlgorithm) { super(tagId, initialSortingAlgorithm); - console.log("Constructed an ImportanceAlgorithm"); + console.log(`[RoomListDebug] Constructed an ImportanceAlgorithm for ${tagId}`); } // noinspection JSMethodCanBeStatic @@ -151,8 +151,36 @@ export class ImportanceAlgorithm extends OrderingAlgorithm { } } + private async handleSplice(room: Room, cause: RoomUpdateCause): Promise { + if (cause === RoomUpdateCause.NewRoom) { + const category = this.getRoomCategory(room); + this.alterCategoryPositionBy(category, 1, this.indices); + this.cachedOrderedRooms.splice(this.indices[category], 0, room); // splice in the new room (pre-adjusted) + } else if (cause === RoomUpdateCause.RoomRemoved) { + const roomIdx = this.getRoomIndex(room); + if (roomIdx === -1) return false; // no change + const oldCategory = this.getCategoryFromIndices(roomIdx, this.indices); + this.alterCategoryPositionBy(oldCategory, -1, this.indices); + this.cachedOrderedRooms.splice(roomIdx, 1); // remove the room + } else { + throw new Error(`Unhandled splice: ${cause}`); + } + } + + private getRoomIndex(room: Room): number { + let roomIdx = this.cachedOrderedRooms.indexOf(room); + if (roomIdx === -1) { // can only happen if the js-sdk's store goes sideways. + console.warn(`Degrading performance to find missing room in "${this.tagId}": ${room.roomId}`); + roomIdx = this.cachedOrderedRooms.findIndex(r => r.roomId === room.roomId); + } + return roomIdx; + } + public async handleRoomUpdate(room: Room, cause: RoomUpdateCause): Promise { - // TODO: Handle NewRoom and RoomRemoved + if (cause === RoomUpdateCause.NewRoom || cause === RoomUpdateCause.RoomRemoved) { + return this.handleSplice(room, cause); + } + if (cause !== RoomUpdateCause.Timeline && cause !== RoomUpdateCause.ReadReceipt) { throw new Error(`Unsupported update cause: ${cause}`); } @@ -162,11 +190,7 @@ export class ImportanceAlgorithm extends OrderingAlgorithm { return; // Nothing to do here. } - let roomIdx = this.cachedOrderedRooms.indexOf(room); - if (roomIdx === -1) { // can only happen if the js-sdk's store goes sideways. - console.warn(`Degrading performance to find missing room in "${this.tagId}": ${room.roomId}`); - roomIdx = this.cachedOrderedRooms.findIndex(r => r.roomId === room.roomId); - } + const roomIdx = this.getRoomIndex(room); if (roomIdx === -1) { throw new Error(`Room ${room.roomId} has no index in ${this.tagId}`); } @@ -188,12 +212,18 @@ export class ImportanceAlgorithm extends OrderingAlgorithm { // room from the array. } - // The room received an update, so take out the slice and sort it. This should be relatively - // quick because the room is inserted at the top of the category, and most popular sorting - // algorithms will deal with trying to keep the active room at the top/start of the category. - // For the few algorithms that will have to move the thing quite far (alphabetic with a Z room - // for example), the list should already be sorted well enough that it can rip through the - // array and slot the changed room in quickly. + // Sort the category now that we've dumped the room in + await this.sortCategory(category); + + return true; // change made + } + + private async sortCategory(category: Category) { + // This should be relatively quick because the room is usually inserted at the top of the + // category, and most popular sorting algorithms will deal with trying to keep the active + // room at the top/start of the category. For the few algorithms that will have to move the + // thing quite far (alphabetic with a Z room for example), the list should already be sorted + // well enough that it can rip through the array and slot the changed room in quickly. const nextCategoryStartIdx = category === CATEGORY_ORDER[CATEGORY_ORDER.length - 1] ? Number.MAX_SAFE_INTEGER : this.indices[CATEGORY_ORDER[CATEGORY_ORDER.indexOf(category) + 1]]; @@ -202,8 +232,6 @@ export class ImportanceAlgorithm extends OrderingAlgorithm { const unsortedSlice = this.cachedOrderedRooms.splice(startIdx, numSort); const sorted = await sortRoomsWithAlgorithm(unsortedSlice, this.tagId, this.sortingAlgorithm); this.cachedOrderedRooms.splice(startIdx, 0, ...sorted); - - return true; // change made } // noinspection JSMethodCanBeStatic @@ -230,14 +258,29 @@ export class ImportanceAlgorithm extends OrderingAlgorithm { // We also need to update subsequent categories as they'll all shift by nRooms, so we // loop over the order to achieve that. - for (let i = CATEGORY_ORDER.indexOf(fromCategory) + 1; i < CATEGORY_ORDER.length; i++) { - const nextCategory = CATEGORY_ORDER[i]; - indices[nextCategory] -= nRooms; - } + this.alterCategoryPositionBy(fromCategory, -nRooms, indices); + this.alterCategoryPositionBy(toCategory, +nRooms, indices); + } - for (let i = CATEGORY_ORDER.indexOf(toCategory) + 1; i < CATEGORY_ORDER.length; i++) { - const nextCategory = CATEGORY_ORDER[i]; - indices[nextCategory] += nRooms; + private alterCategoryPositionBy(category: Category, n: number, indices: ICategoryIndex) { + // Note: when we alter a category's index, we actually have to modify the ones following + // the target and not the target itself. + + // XXX: If this ever actually gets more than one room passed to it, it'll need more index + // handling. For instance, if 45 rooms are removed from the middle of a 50 room list, the + // index for the categories will be way off. + + const nextOrderIndex = CATEGORY_ORDER.indexOf(category) + 1 + if (n > 0) { + for (let i = nextOrderIndex; i < CATEGORY_ORDER.length; i++) { + const nextCategory = CATEGORY_ORDER[i]; + indices[nextCategory] += Math.abs(n); + } + } else if (n < 0) { + for (let i = nextOrderIndex; i < CATEGORY_ORDER.length; i++) { + const nextCategory = CATEGORY_ORDER[i]; + indices[nextCategory] -= Math.abs(n); + } } // Do a quick check to see if we've completely broken the index diff --git a/src/stores/room-list/algorithms/list-ordering/NaturalAlgorithm.ts b/src/stores/room-list/algorithms/list-ordering/NaturalAlgorithm.ts index cce7372986..96a3f58d2c 100644 --- a/src/stores/room-list/algorithms/list-ordering/NaturalAlgorithm.ts +++ b/src/stores/room-list/algorithms/list-ordering/NaturalAlgorithm.ts @@ -28,7 +28,7 @@ export class NaturalAlgorithm extends OrderingAlgorithm { public constructor(tagId: TagID, initialSortingAlgorithm: SortAlgorithm) { super(tagId, initialSortingAlgorithm); - console.log("Constructed a NaturalAlgorithm"); + console.log(`[RoomListDebug] Constructed a NaturalAlgorithm for ${tagId}`); } public async setRooms(rooms: Room[]): Promise { @@ -36,11 +36,19 @@ export class NaturalAlgorithm extends OrderingAlgorithm { } public async handleRoomUpdate(room, cause): Promise { - // TODO: Handle NewRoom and RoomRemoved - if (cause !== RoomUpdateCause.Timeline && cause !== RoomUpdateCause.ReadReceipt) { + const isSplice = cause === RoomUpdateCause.NewRoom || cause === RoomUpdateCause.RoomRemoved; + const isInPlace = cause === RoomUpdateCause.Timeline || cause === RoomUpdateCause.ReadReceipt; + if (!isSplice && !isInPlace) { throw new Error(`Unsupported update cause: ${cause}`); } + if (cause === RoomUpdateCause.NewRoom) { + this.cachedOrderedRooms.push(room); + } else if (cause === RoomUpdateCause.RoomRemoved) { + const idx = this.cachedOrderedRooms.indexOf(room); + if (idx >= 0) this.cachedOrderedRooms.splice(idx, 1); + } + // TODO: Optimize this to avoid useless operations // For example, we can skip updates to alphabetic (sometimes) and manually ordered tags this.cachedOrderedRooms = await sortRoomsWithAlgorithm(this.cachedOrderedRooms, this.tagId, this.sortingAlgorithm); diff --git a/src/stores/room-list/algorithms/list-ordering/OrderingAlgorithm.ts b/src/stores/room-list/algorithms/list-ordering/OrderingAlgorithm.ts index 263e8a4cd4..f581e30630 100644 --- a/src/stores/room-list/algorithms/list-ordering/OrderingAlgorithm.ts +++ b/src/stores/room-list/algorithms/list-ordering/OrderingAlgorithm.ts @@ -67,6 +67,5 @@ export abstract class OrderingAlgorithm { * @param cause The cause of the update. * @returns True if the update requires the Algorithm to update the presentation layers. */ - // XXX: TODO: We assume this will only ever be a position update and NOT a NewRoom or RemoveRoom change!! public abstract handleRoomUpdate(room: Room, cause: RoomUpdateCause): Promise; }