Merge pull request #4905 from matrix-org/travis/room-list/room-safety
Improve room safety in the new room listpull/21833/head
Travis Ralston
GitHub
commit
7173ea71a5
|
|
@ -18,7 +18,7 @@ import { Room } from "matrix-js-sdk/src/models/room";
|
|||
import { isNullOrUndefined } from "matrix-js-sdk/src/utils";
|
||||
import DMRoomMap from "../../../utils/DMRoomMap";
|
||||
import { EventEmitter } from "events";
|
||||
import { arrayHasDiff, ArrayUtil } from "../../../utils/arrays";
|
||||
import { arrayDiff, arrayHasDiff, ArrayUtil } from "../../../utils/arrays";
|
||||
import { getEnumValues } from "../../../utils/enums";
|
||||
import { DefaultTagID, RoomUpdateCause, TagID } from "../models";
|
||||
import {
|
||||
|
|
@ -57,6 +57,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; // only not-null when changing the sticky room
|
||||
private sortAlgorithms: ITagSortingMap;
|
||||
private listAlgorithms: IListOrderingMap;
|
||||
private algorithms: IOrderingAlgorithmMap;
|
||||
|
|
@ -162,9 +163,21 @@ 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) {
|
||||
|
|
@ -179,7 +192,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
|
||||
|
|
@ -196,19 +209,41 @@ export class Algorithm extends EventEmitter {
|
|||
// 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;
|
||||
this._stickyRoom = null;
|
||||
this._stickyRoom = null; // clear before we update the algorithm
|
||||
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) {
|
||||
// We don't add the new room if the sticky room isn't changing because that's
|
||||
// an easy way to cause duplication. We have to do room ID checks instead of
|
||||
// referential checks as the references can differ through the lifecycle.
|
||||
if (lastStickyRoom && lastStickyRoom.room && lastStickyRoom.room.roomId !== val.roomId) {
|
||||
// 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);
|
||||
|
||||
// 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");
|
||||
}
|
||||
}
|
||||
|
||||
console.warn(`Sticky room changed tag & position from ${tag} / ${position} `
|
||||
+ `to ${this._stickyRoom.tag} / ${this._stickyRoom.position}`);
|
||||
|
||||
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
|
||||
|
|
@ -560,7 +595,7 @@ export class Algorithm extends EventEmitter {
|
|||
/**
|
||||
* Updates the roomsToTags map
|
||||
*/
|
||||
protected updateTagsFromCache() {
|
||||
private updateTagsFromCache() {
|
||||
const newMap = {};
|
||||
|
||||
const tags = Object.keys(this.cachedRooms);
|
||||
|
|
@ -607,21 +642,94 @@ export class Algorithm extends EventEmitter {
|
|||
* processing.
|
||||
*/
|
||||
public async handleRoomUpdate(room: Room, cause: RoomUpdateCause): Promise<boolean> {
|
||||
// TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
|
||||
console.log(`Handle room update for ${room.roomId} called with cause ${cause}`);
|
||||
if (!this.algorithms) throw new Error("Not ready: no algorithms to determine tags from");
|
||||
|
||||
// Note: check the isSticky against the room ID just in case the reference is wrong
|
||||
const isSticky = this._stickyRoom && this._stickyRoom.room && this._stickyRoom.room.roomId === room.roomId;
|
||||
if (cause === RoomUpdateCause.NewRoom) {
|
||||
const isForLastSticky = this._lastStickyRoom && this._lastStickyRoom.room === room;
|
||||
const roomTags = this.roomIdsToTags[room.roomId];
|
||||
if (roomTags && roomTags.length > 0) {
|
||||
const hasTags = roomTags && roomTags.length > 0;
|
||||
|
||||
// Don't change the cause if the last sticky room is being re-added. If we fail to
|
||||
// pass the cause through as NewRoom, we'll fail to lie to the algorithm and thus
|
||||
// lose the room.
|
||||
if (hasTags && !isForLastSticky) {
|
||||
console.warn(`${room.roomId} is reportedly new but is already known - assuming TagChange instead`);
|
||||
cause = RoomUpdateCause.PossibleTagChange;
|
||||
}
|
||||
|
||||
// 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) && !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);
|
||||
|
||||
// Sanity check
|
||||
if (!this.rooms.includes(room)) {
|
||||
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) {
|
||||
// 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
|
||||
await this.setKnownRooms(this.rooms);
|
||||
return true;
|
||||
let didTagChange = false;
|
||||
const oldTags = this.roomIdsToTags[room.roomId] || [];
|
||||
const newTags = this.getTagsForRoom(room);
|
||||
const diff = arrayDiff(oldTags, newTags);
|
||||
if (diff.removed.length > 0 || diff.added.length > 0) {
|
||||
for (const rmTag of diff.removed) {
|
||||
// TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
|
||||
console.log(`Removing ${room.roomId} from ${rmTag}`);
|
||||
const algorithm: OrderingAlgorithm = this.algorithms[rmTag];
|
||||
if (!algorithm) throw new Error(`No algorithm for ${rmTag}`);
|
||||
await algorithm.handleRoomUpdate(room, RoomUpdateCause.RoomRemoved);
|
||||
}
|
||||
for (const addTag of diff.added) {
|
||||
// TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
|
||||
console.log(`Adding ${room.roomId} to ${addTag}`);
|
||||
const algorithm: OrderingAlgorithm = this.algorithms[addTag];
|
||||
if (!algorithm) throw new Error(`No algorithm for ${addTag}`);
|
||||
await algorithm.handleRoomUpdate(room, RoomUpdateCause.NewRoom);
|
||||
}
|
||||
|
||||
// Update the tag map so we don't regen it in a moment
|
||||
this.roomIdsToTags[room.roomId] = newTags;
|
||||
|
||||
// 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.
|
||||
await this.setStickyRoomAsync(room);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// If the update is for a room change which might be the sticky room, prevent it. We
|
||||
|
|
@ -635,8 +743,9 @@ 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})`);
|
||||
if (!this.roomIdsToTags[room.roomId]) {
|
||||
// TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
|
||||
console.log(`[RoomListDebug] Updating tags for 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]));
|
||||
|
|
@ -646,9 +755,15 @@ export class Algorithm extends EventEmitter {
|
|||
if (!roomTags.length) throw new Error(`Tags cannot be determined for ${room.roomId}`);
|
||||
|
||||
this.roomIdsToTags[room.roomId] = roomTags;
|
||||
|
||||
// TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
|
||||
console.log(`[RoomListDebug] Updated tags for ${room.roomId}:`, roomTags);
|
||||
}
|
||||
|
||||
let tags = this.roomIdsToTags[room.roomId];
|
||||
// TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
|
||||
console.log(`[RoomListDebug] Reached algorithmic handling for ${room.roomId} and cause ${cause}`);
|
||||
|
||||
const tags = this.roomIdsToTags[room.roomId];
|
||||
if (!tags) {
|
||||
console.warn(`No tags known for "${room.name}" (${room.roomId})`);
|
||||
return false;
|
||||
|
|
@ -668,6 +783,8 @@ export class Algorithm extends EventEmitter {
|
|||
changed = true;
|
||||
}
|
||||
|
||||
return true;
|
||||
// TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
|
||||
console.log(`[RoomListDebug] Finished handling ${room.roomId} with cause ${cause} (changed=${changed})`);
|
||||
return changed;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue