From c908a6cf1e87a109089bee010b59e38bc710c3c3 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 27 Feb 2019 15:55:16 -0700 Subject: [PATCH 1/6] Move complex part of room sorting to a dedicated function Pretty much cut/pasting it in, as there's not really a whole much to help make the code more understandable here. This also includes a comment block longer than the code it describes in hopes it explains away the problem of understanding what it does. Should fix https://github.com/vector-im/riot-web/issues/8861 --- src/stores/RoomListStore.js | 175 ++++++++++++++++++++++-------------- 1 file changed, 109 insertions(+), 66 deletions(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index 31cede6c7d..f1e45cf1d8 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -326,21 +326,119 @@ class RoomListStore extends Store { return tags; } + _slotRoomIntoList(room, category, existingEntries, newList, lastTimestampFn) { + const targetCategoryIndex = CATEGORY_ORDER.indexOf(category); + + // The slotting algorithm works by trying to position the room in the most relevant + // section of the list (red > grey > etc). To accomplish this, we need to consider + // a couple cases: the section existing in the list but having other rooms in it and + // the case of the section simply not existing and needing to be started. In order to + // do this efficiently, we only want to iterate over the list once and solve our sorting + // problem as we go. + // + // Firstly, we'll remove any existing entry that references the room we're trying to + // insert. We don't really want to consider the old entry and want to recreate it. We + // also exclude the sticky (currently active) room from the categorization logic and + // let it pass through wherever it resides in the list: it shouldn't be moving around + // the list too much, so we want to keep it where it is. + // + // The case of the section we want existing is easy to handle: once we hit the section, + // find the room that has a most recent event later than our own and insert just before + // that (making us the more recent room). If we end up hitting the next section before + // we can slot the room in, insert the room at the top of the section as a fallback. We + // do this to ensure that the room doesn't go too far down the list given it was previously + // considered important (in the case of going down in category) or is now more important + // (suddenly becoming red, for instance). The boundary tracking is how we end up achieving + // this, as described in the next paragraphs. + // + // The other case of the section not already existing is a bit more complicated. We track + // the boundaries of each section relative to the list we're currently building so that + // when we miss the section we can insert the room at the right spot. Most importantly, we + // can't assume that the end of the list being built is the right spot because of the last + // paragraph's requirement: the room should be put to the top of a section if the section + // runs out of places to put it. + // + // All told, our tracking looks something like this: + // + // ------ A <- Section boundary (start of red) + // RED + // RED + // RED + // ------ B <- In this example, we have a grey room we want to insert. + // BOLD + // BOLD + // ------ C + // IDLE + // IDLE + // ------ D <- End of list + // + // Given that example, and our desire to insert a GREY room into the list, this iterates + // over the room list until it realizes that BOLD comes after GREY and we're no longer + // in the RED section. Because there's no rooms there, we simply insert there which is + // also a "category boundary". If we change the example to wanting to insert a BOLD room + // which can't be ordered by timestamp with the existing couple rooms, we would still make + // use of the boundary flag to insert at B before changing the boundary indicator to C. + + let desiredCategoryBoundaryIndex = 0; + let foundBoundary = false; + let pushedEntry = false; + + for (const entry of existingEntries) { + // We insert our own record as needed, so don't let the old one through. + if (entry.room.roomId === room.roomId) { + continue; + } + + // if the list is a recent list, and the room appears in this list, and we're + // not looking at a sticky room (sticky rooms have unreliable categories), try + // to slot the new room in + if (entry.room.roomId !== this._state.stickyRoomId && !pushedEntry) { + const entryCategoryIndex = CATEGORY_ORDER.indexOf(entry.category); + + // As per above, check if we're meeting that boundary we wanted to locate. + if (entryCategoryIndex >= targetCategoryIndex && !foundBoundary) { + desiredCategoryBoundaryIndex = newList.length - 1; + foundBoundary = true; + } + + // If we've hit the top of a boundary beyond our target category, insert at the top of + // the grouping to ensure the room isn't slotted incorrectly. Otherwise, try to insert + // based on most recent timestamp. + const changedBoundary = entryCategoryIndex > targetCategoryIndex; + const currentCategory = entryCategoryIndex === targetCategoryIndex; + if (changedBoundary || (currentCategory && lastTimestampFn(room) >= lastTimestampFn(entry.room))) { + if (changedBoundary) { + // If we changed a boundary, then we've gone too far - go to the top of the last + // section instead. + newList.splice(desiredCategoryBoundaryIndex, 0, {room, category}); + } else { + // If we're ordering by timestamp, just insert normally + newList.push({room, category}); + } + pushedEntry = true; + } + } + + // Fall through and clone the list. + newList.push(entry); + } + + return pushedEntry; + } + _setRoomCategory(room, category) { if (!room) return; // This should only happen in tests const listsClone = {}; - const targetCategoryIndex = CATEGORY_ORDER.indexOf(category); // Micro optimization: Support lazily loading the last timestamp in a room - let _targetTimestamp = null; - const targetTimestamp = () => { - if (_targetTimestamp === null) { - _targetTimestamp = this._tsOfNewestEvent(room); + let timestampCache = {}; // {roomId => ts} + const lastTimestamp = (room) => { + if (!timestampCache[room.roomId]) { + timestampCache[room.roomId] = this._tsOfNewestEvent(room); } - return _targetTimestamp; + return timestampCache[room.roomId]; }; - const targetTags = this._getRecommendedTagsForRoom(room); const insertedIntoTags = []; @@ -369,65 +467,8 @@ class RoomListStore extends Store { } else { listsClone[key] = []; - // We track where the boundary within listsClone[key] is just in case our timestamp - // ordering fails. If we can't stick the room in at the correct place in the category - // grouping based on timestamp, we'll stick it at the top of the group which will be - // the index we track here. - let desiredCategoryBoundaryIndex = 0; - let foundBoundary = false; - let pushedEntry = false; - - for (const entry of this._state.lists[key]) { - // We insert our own record as needed, so don't let the old one through. - if (entry.room.roomId === room.roomId) { - continue; - } - - // if the list is a recent list, and the room appears in this list, and we're - // not looking at a sticky room (sticky rooms have unreliable categories), try - // to slot the new room in - if (entry.room.roomId !== this._state.stickyRoomId) { - if (!pushedEntry && shouldHaveRoom) { - // Micro optimization: Support lazily loading the last timestamp in a room - let _entryTimestamp = null; - const entryTimestamp = () => { - if (_entryTimestamp === null) { - _entryTimestamp = this._tsOfNewestEvent(entry.room); - } - return _entryTimestamp; - }; - - const entryCategoryIndex = CATEGORY_ORDER.indexOf(entry.category); - - // As per above, check if we're meeting that boundary we wanted to locate. - if (entryCategoryIndex >= targetCategoryIndex && !foundBoundary) { - desiredCategoryBoundaryIndex = listsClone[key].length - 1; - foundBoundary = true; - } - - // If we've hit the top of a boundary beyond our target category, insert at the top of - // the grouping to ensure the room isn't slotted incorrectly. Otherwise, try to insert - // based on most recent timestamp. - const changedBoundary = entryCategoryIndex > targetCategoryIndex; - const currentCategory = entryCategoryIndex === targetCategoryIndex; - if (changedBoundary || (currentCategory && targetTimestamp() >= entryTimestamp())) { - if (changedBoundary) { - // If we changed a boundary, then we've gone too far - go to the top of the last - // section instead. - listsClone[key].splice(desiredCategoryBoundaryIndex, 0, {room, category}); - } else { - // If we're ordering by timestamp, just insert normally - listsClone[key].push({room, category}); - } - pushedEntry = true; - insertedIntoTags.push(key); - } - } - } - - // Fall through and clone the list. - listsClone[key].push(entry); - } + const pushedEntry = this._slotRoomIntoList( + room, category, this._state.lists[key], listsClone[key], lastTimestamp); if (!pushedEntry) { if (listsClone[key].length === 0) { @@ -437,6 +478,8 @@ class RoomListStore extends Store { // In theory, this should never happen console.warn(`!! Room ${room.roomId} lost: No position available`); } + } else { + insertedIntoTags.push(key); } } } From bafe59fe28a84c7ff70376f475afc289a865a61e Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 27 Feb 2019 18:29:48 -0700 Subject: [PATCH 2/6] Appease the linter --- src/stores/RoomListStore.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index f1e45cf1d8..e8946d5f77 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -432,7 +432,7 @@ class RoomListStore extends Store { const listsClone = {}; // Micro optimization: Support lazily loading the last timestamp in a room - let timestampCache = {}; // {roomId => ts} + const timestampCache = {}; // {roomId => ts} const lastTimestamp = (room) => { if (!timestampCache[room.roomId]) { timestampCache[room.roomId] = this._tsOfNewestEvent(room); From a3342a5790df6a3449849efc44443960b706caf7 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 28 Feb 2019 13:24:05 -0700 Subject: [PATCH 3/6] Standardize on "Category" being the canonical term for room list sections --- src/stores/RoomListStore.js | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index e8946d5f77..ca349f00ba 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -330,9 +330,9 @@ class RoomListStore extends Store { const targetCategoryIndex = CATEGORY_ORDER.indexOf(category); // The slotting algorithm works by trying to position the room in the most relevant - // section of the list (red > grey > etc). To accomplish this, we need to consider - // a couple cases: the section existing in the list but having other rooms in it and - // the case of the section simply not existing and needing to be started. In order to + // category of the list (red > grey > etc). To accomplish this, we need to consider + // a couple cases: the category existing in the list but having other rooms in it and + // the case of the category simply not existing and needing to be started. In order to // do this efficiently, we only want to iterate over the list once and solve our sorting // problem as we go. // @@ -342,25 +342,25 @@ class RoomListStore extends Store { // let it pass through wherever it resides in the list: it shouldn't be moving around // the list too much, so we want to keep it where it is. // - // The case of the section we want existing is easy to handle: once we hit the section, + // The case of the category we want existing is easy to handle: once we hit the category, // find the room that has a most recent event later than our own and insert just before - // that (making us the more recent room). If we end up hitting the next section before - // we can slot the room in, insert the room at the top of the section as a fallback. We + // that (making us the more recent room). If we end up hitting the next category before + // we can slot the room in, insert the room at the top of the category as a fallback. We // do this to ensure that the room doesn't go too far down the list given it was previously // considered important (in the case of going down in category) or is now more important // (suddenly becoming red, for instance). The boundary tracking is how we end up achieving // this, as described in the next paragraphs. // - // The other case of the section not already existing is a bit more complicated. We track - // the boundaries of each section relative to the list we're currently building so that - // when we miss the section we can insert the room at the right spot. Most importantly, we + // The other case of the category not already existing is a bit more complicated. We track + // the boundaries of each category relative to the list we're currently building so that + // when we miss the category we can insert the room at the right spot. Most importantly, we // can't assume that the end of the list being built is the right spot because of the last - // paragraph's requirement: the room should be put to the top of a section if the section + // paragraph's requirement: the room should be put to the top of a category if the category // runs out of places to put it. // // All told, our tracking looks something like this: // - // ------ A <- Section boundary (start of red) + // ------ A <- Category boundary (start of red) // RED // RED // RED From b1e16e9f4967cd83649a69b3345ec6845290ed9d Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 28 Feb 2019 13:55:20 -0700 Subject: [PATCH 4/6] Fix stacktrace when starting riot up with rooms Settings can trigger before we're ready, so don't generate the room list. This also includes a comment to signify to future people that we need to track settings still. --- src/stores/RoomListStore.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index ca349f00ba..292f7beb98 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -133,6 +133,8 @@ class RoomListStore extends Store { const logicallyReady = this._matrixClient && this._state.ready; switch (payload.action) { case 'setting_updated': { + if (!logicallyReady) break; + if (payload.settingName === 'RoomList.orderByImportance') { this.updateSortingAlgorithm(payload.newValue === true ? ALGO_IMPORTANCE : ALGO_RECENT); } else if (payload.settingName === 'feature_custom_tags') { @@ -147,6 +149,10 @@ class RoomListStore extends Store { break; } + // Always ensure that we set any state needed for settings here. It is possible that + // setting updates trigger on startup before we are ready to sync, so we want to make + // sure that the right state is in place before we actually react to those changes. + this._setState({tagsEnabled: SettingsStore.isFeatureEnabled("feature_custom_tags")}); this._matrixClient = payload.matrixClient; From 872cdaa9b35e86fc71a56ecee143015ecc772ca6 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 28 Feb 2019 14:03:03 -0700 Subject: [PATCH 5/6] Use the already available state for checking if custom tags are enabled --- src/stores/RoomListStore.js | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index 292f7beb98..a4138fcf01 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -529,15 +529,6 @@ class RoomListStore extends Store { const dmRoomMap = DMRoomMap.shared(); - // Speed optimization: Hitting the SettingsStore is expensive, so avoid that at all costs. - let _isCustomTagsEnabled = null; - const isCustomTagsEnabled = () => { - if (_isCustomTagsEnabled === null) { - _isCustomTagsEnabled = SettingsStore.isFeatureEnabled("feature_custom_tags"); - } - return _isCustomTagsEnabled; - }; - this._matrixClient.getRooms().forEach((room) => { const myUserId = this._matrixClient.getUserId(); const membership = room.getMyMembership(); @@ -553,7 +544,7 @@ class RoomListStore extends Store { tagNames = tagNames.filter((t) => { // Speed optimization: Avoid hitting the SettingsStore at all costs by making it the // last condition possible. - return lists[t] !== undefined || (!t.startsWith('m.') && isCustomTagsEnabled()); + return lists[t] !== undefined || (!t.startsWith('m.') && this._state.tagsEnabled); }); if (tagNames.length) { From 805676a511704baadc6f20ae886495d95bb2c2ae Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 28 Feb 2019 14:02:39 -0700 Subject: [PATCH 6/6] Don't lose invites when multiple are pending --- src/stores/RoomListStore.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index a4138fcf01..e9ac33b506 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -477,7 +477,9 @@ class RoomListStore extends Store { room, category, this._state.lists[key], listsClone[key], lastTimestamp); if (!pushedEntry) { - if (listsClone[key].length === 0) { + // Special case invites: they don't really have timelines and can easily get lost when + // the user has multiple pending invites. Pushing them is the least worst option. + if (listsClone[key].length === 0 || key === "im.vector.fake.invite") { listsClone[key].push({room, category}); insertedIntoTags.push(key); } else {