PrivateCoffee/element-web
4
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 3

Ensure the sticky room changes if the tag changes

Browse source 
This fixes a case where a user accepts an invite, which causes a tag change, but the room stays stuck in the invites list. The tag change additionally gets swallowed when the user moves away, causing the room to get lost. 

By moving it when we see it, potentially during a sticky room change itself (though extremely rare), we avoid having the room get lost in the wrong lists. A side effect of this is that accepting an invite puts it at the top of the tag it's going to (usually untagged), however this feels like the best option for the user.

A rare case of a tag change happening during a sticky room change is when a leave event comes in for the sticky room, but because it's come through as a tag change it can get swallowed. If it does get swallowed and the user clicks away, the tag change will happen when the room is re-introduced to the list (fake NewRoom event).
This commit is contained in:
Travis Ralston 2020-07-06 19:32:31 -06:00
parent 4345f972e0
commit dd833f4f2f
1 changed files with 60 additions and 5 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

65
src/stores/room-list/algorithms/Algorithm.ts
View file

@ -58,7 +58,7 @@ export class Algorithm extends EventEmitter {
private _cachedStickyRooms: ITagMap = {}; // a clone of the _cachedRooms, with the sticky room
private filteredRooms: ITagMap = {};
private _stickyRoom: IStickyRoom = null;
private _lastStickyRoom: IStickyRoom = null;
private _lastStickyRoom: IStickyRoom = null; // only not-null when changing the sticky room
private sortAlgorithms: ITagSortingMap;
private listAlgorithms: IListOrderingMap;
private algorithms: IOrderingAlgorithmMap;
@ -165,15 +165,26 @@ export class Algorithm extends EventEmitter {
}
private async updateStickyRoom(val: Room) {
try {
return await this.doUpdateStickyRoom(val);
} finally {
this._lastStickyRoom = null; // clear to indicate we're done changing
}
}
private async doUpdateStickyRoom(val: Room) {
// Note throughout: We need async so we can wait for handleRoomUpdate() to do its thing,
// otherwise we risk duplicating rooms.
// Set the last sticky room to indicate that we're in a change. The code throughout the
// class can safely handle a null room, so this should be safe to do as a backup.
this._lastStickyRoom = this._stickyRoom || <IStickyRoom>{};
// 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
this._lastStickyRoom = stickyRoom;
// Lie to the algorithm and re-add the room to the algorithm
await this.handleRoomUpdate(stickyRoom, RoomUpdateCause.NewRoom);
@ -183,7 +194,7 @@ 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];
let tag = this.roomIdsToTags[val.roomId][0];
if (!tag) throw new Error(`${val.roomId} does not belong to a tag and cannot be sticky`);
// We specifically do NOT use the ordered rooms set as it contains the sticky room, which
@ -201,7 +212,6 @@ export class Algorithm extends EventEmitter {
// a new update for ourselves.
const lastStickyRoom = this._stickyRoom;
this._stickyRoom = null; // clear before we update the algorithm
this._lastStickyRoom = lastStickyRoom;
this.recalculateStickyRoom();
// When we do have the room, re-add the old room (if needed) to the algorithm
@ -214,6 +224,22 @@ export class Algorithm extends EventEmitter {
// Lie to the algorithm and remove the room from it's field of view
await this.handleRoomUpdate(val, RoomUpdateCause.RoomRemoved);
// Check for tag & position changes while we're here. We also check the room to ensure
// it is still the same room.
if (this._stickyRoom) {
if (this._stickyRoom.room !== val) {
// Check the room IDs just in case
if (this._stickyRoom.room.roomId === val.roomId) {
console.warn("Sticky room changed references");
} else {
throw new Error("Sticky room changed while the sticky room was changing");
}
}
tag = this._stickyRoom.tag;
position = this._stickyRoom.position;
}
// 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
@ -619,6 +645,8 @@ export class Algorithm extends EventEmitter {
if (!this.algorithms) throw new Error("Not ready: no algorithms to determine tags from");
const isSticky = this._stickyRoom && this._stickyRoom.room === room;
// TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
console.log(`[RoomListDebug] @@ ${room.roomId} with ${cause} for tags ${this.roomIdsToTags[room.roomId]} and sticky is currently ${this._stickyRoom && this._stickyRoom.room ? this._stickyRoom.room.roomId : '<none>'} and last is ${this._lastStickyRoom && this._lastStickyRoom.room ? this._lastStickyRoom.room.roomId : '<none>'}`);
@ -637,7 +665,7 @@ export class Algorithm extends EventEmitter {
// If we have tags for a room and don't have the room referenced, the room reference
// probably changed. We need to swap out the problematic reference.
if (hasTags && !this.rooms.includes(room)) {
if (hasTags && !this.rooms.includes(room) && !isSticky) {
console.warn(`${room.roomId} is missing from room array but is known - trying to find duplicate`);
this.rooms = this.rooms.map(r => r.roomId === room.roomId ? room : r);
@ -646,9 +674,17 @@ export class Algorithm extends EventEmitter {
throw new Error(`Failed to replace ${room.roomId} with an updated reference`);
}
}
// Like above, update the reference to the sticky room if we need to
if (hasTags && isSticky) {
// Go directly in and set the sticky room's new reference, being careful not
// to trigger a sticky room update ourselves.
this._stickyRoom.room = room;
}
}
if (cause === RoomUpdateCause.PossibleTagChange) {
let didTagChange = false;
const oldTags = this.roomIdsToTags[room.roomId] || [];
const newTags = this.getTagsForRoom(room);
const diff = arrayDiff(oldTags, newTags);
@ -674,11 +710,30 @@ export class Algorithm extends EventEmitter {
// TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
console.log(`Changing update cause for ${room.roomId} to TIMELINE to sort rooms`);
cause = RoomUpdateCause.Timeline;
didTagChange = true;
} else {
// TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
console.warn(`Received no-op update for ${room.roomId} - changing to TIMELINE update`);
cause = RoomUpdateCause.Timeline;
}
if (didTagChange && isSticky) {
// Manually update the tag for the sticky room without triggering a sticky room
// update. The update will be handled implicitly by the sticky room handling and
// requires no changes on our part, if we're in the middle of a sticky room change.
if (this._lastStickyRoom) {
this._stickyRoom = {
room,
tag: this.roomIdsToTags[room.roomId][0],
position: 0, // right at the top as it changed tags
};
} else {
// We have to clear the lock as the sticky room change will trigger updates.
this.lock.release();
await this.setStickyRoomAsync(room);
await this.lock.acquireAsync();
}
}
}
// If the update is for a room change which might be the sticky room, prevent it. We