From f93d67fc65c9914286431b384020176c29b31e1d Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 23 Jun 2020 16:49:39 -0600 Subject: [PATCH] Fix sticky room disappearing/jumping in search results Fixes https://github.com/vector-im/riot-web/issues/14124 Fixes https://github.com/vector-im/riot-web/issues/14154 (which was technically supposed to say that the sticky room when filtering was always last) This is all a bit complicated, but the theory is that we end up with a stable list even through filtering. There's some notes within, though I suspect it'll be difficult to understand :( --- src/stores/room-list/algorithms/Algorithm.ts | 47 +++++++++++++++++--- 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/src/stores/room-list/algorithms/Algorithm.ts b/src/stores/room-list/algorithms/Algorithm.ts index 052c58bb83..9eb0d27748 100644 --- a/src/stores/room-list/algorithms/Algorithm.ts +++ b/src/stores/room-list/algorithms/Algorithm.ts @@ -170,12 +170,16 @@ export class Algorithm extends EventEmitter { // 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); + + // We specifically do NOT use the ordered rooms set as it contains the sticky room, which + // means we'll be off by 1 when the user is switching rooms. This leads to visual jumping + // when the user is moving south in the list (not north, because of math). + let position = this.getOrderedRoomsWithoutSticky()[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 + // we need to ensure that when we do we're ready for the inevitable 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 @@ -208,6 +212,12 @@ export class Algorithm extends EventEmitter { position: position, tag: tag, }; + + // We update the filtered rooms just in case, as otherwise users will end up visiting + // a room while filtering and it'll disappear. We don't update the filter earlier in + // this function simply because we don't have to. + this.recalculateFilteredRoomsForTag(tag); + if (lastStickyRoom && lastStickyRoom.tag !== tag) this.recalculateFilteredRoomsForTag(tag); this.recalculateStickyRoom(); // Finally, trigger an update @@ -231,9 +241,7 @@ export class Algorithm extends EventEmitter { // We optimize our lookups by trying to reduce sample size as much as possible // to the rooms we know will be deduped by the Set. const rooms = this.cachedRooms[tagId].map(r => r); // cheap clone - if (this._stickyRoom && this._stickyRoom.tag === tagId && this._stickyRoom.room) { - rooms.push(this._stickyRoom.room); - } + this.tryInsertStickyRoomToFilterSet(rooms, tagId); let remainingRooms = rooms.map(r => r); let allowedRoomsInThisTag = []; let lastFilterPriority = orderedFilters[0].relativePriority; @@ -263,6 +271,7 @@ export class Algorithm extends EventEmitter { this.emit(LIST_UPDATED_EVENT); } + // TODO: Remove or use. protected addPossiblyFilteredRoomsToTag(tagId: TagID, added: Room[]): void { const filters = this.allowedByFilter.keys(); for (const room of added) { @@ -281,7 +290,8 @@ export class Algorithm extends EventEmitter { protected recalculateFilteredRoomsForTag(tagId: TagID): void { console.log(`Recalculating filtered rooms for ${tagId}`); delete this.filteredRooms[tagId]; - const rooms = this.cachedRooms[tagId]; + const rooms = this.cachedRooms[tagId].map(r => r); // cheap clone + this.tryInsertStickyRoomToFilterSet(rooms, tagId); const filteredRooms = rooms.filter(r => this.allowedRoomsByFilters.has(r)); if (filteredRooms.length > 0) { this.filteredRooms[tagId] = filteredRooms; @@ -289,6 +299,17 @@ export class Algorithm extends EventEmitter { console.log(`[DEBUG] ${filteredRooms.length}/${rooms.length} rooms filtered into ${tagId}`); } + protected tryInsertStickyRoomToFilterSet(rooms: Room[], tagId: TagID) { + if (!this._stickyRoom || !this._stickyRoom.room || this._stickyRoom.tag !== tagId) return; + + const position = this._stickyRoom.position; + if (position >= rooms.length) { + rooms.push(this._stickyRoom.room); + } else { + rooms.splice(position, 0, this._stickyRoom.room); + } + } + /** * 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 @@ -377,6 +398,20 @@ export class Algorithm extends EventEmitter { return this.filteredRooms; } + /** + * This returns the same as getOrderedRooms(), but without the sticky room + * map as it causes issues for sticky room handling (see sticky room handling + * for more information). + * @returns {ITagMap} The cached list of rooms, ordered, + * for each tag. May be empty, but never null/undefined. + */ + private getOrderedRoomsWithoutSticky(): ITagMap { + if (!this.hasFilters) { + return this.cachedRooms; + } + return this.filteredRooms; + } + /** * Seeds the Algorithm with a set of rooms. The algorithm will discard all * previously known information and instead use these rooms instead.