From 5f760fbf4a438f7e1b5615ef3825c0c04b15feeb Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Sun, 24 Feb 2019 19:44:47 -0700 Subject: [PATCH 1/2] Change the room list algo to eagerly delete and carefully insert Previously it made some complicated assumptions about the contexts it was called in (it generally assumed it just had to shuffle rooms between tags and didn't really handle new rooms very well). The algorithm now eagerly tries to drop rooms from tags and carefully inserts them. The actual insertion logic is mostly untouched: the only part changed is how it handles failure to insert into tag. It shouldn't be possible for this algorithm to completely skip a room unless the tag is empty, so we special case that. There are several TODO comments to be addressed here. Namely, it doesn't handle manually ordered tags (favourites, custom, etc) and doesn't check if tags are even enabled. Changes in this area are waiting for https://github.com/matrix-org/matrix-react-sdk/pull/2686 to land to take advantage of monitoring the settings flag for tags. --- src/stores/RoomListStore.js | 226 +++++++++++++++++++----------------- 1 file changed, 120 insertions(+), 106 deletions(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index 0a11c2774a..50a7dd22a5 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -180,7 +180,7 @@ class RoomListStore extends Store { break; case 'MatrixActions.Room.myMembership': { if (!logicallyReady) break; - this._roomUpdateTriggered(payload.room.roomId); + this._roomUpdateTriggered(payload.room.roomId, true); } break; // This could be a new room that we've been invited to, joined or created @@ -188,7 +188,7 @@ class RoomListStore extends Store { // a member. case 'MatrixActions.Room': { if (!logicallyReady) break; - this._roomUpdateTriggered(payload.room.roomId); + this._roomUpdateTriggered(payload.room.roomId, true); } break; // TODO: Re-enable optimistic updates when we support dragging again @@ -230,12 +230,12 @@ class RoomListStore extends Store { } } - _roomUpdateTriggered(roomId) { + _roomUpdateTriggered(roomId, ignoreSticky) { // We don't calculate categories for sticky rooms because we have a moderate // interest in trying to maintain the category that they were last in before // being artificially flagged as IDLE. Also, this reduces the amount of time // we spend in _setRoomCategory ever so slightly. - if (this._state.stickyRoomId !== roomId) { + if (this._state.stickyRoomId !== roomId || ignoreSticky) { // Micro optimization: Only look up the room if we're confident we'll need it. const room = this._matrixClient.getRoom(roomId); if (!room) return; @@ -245,6 +245,31 @@ class RoomListStore extends Store { } } + _getRecommendedTagsForRoom(room) { + const tags = []; + + const myMembership = room.getMyMembership(); + if (myMembership === 'join' || myMembership === 'invite') { + // Stack the user's tags on top + // TODO: Consider whether tags are enabled at all + tags.push(...(room.tags || [])); + + const dmRoomMap = DMRoomMap.shared(); + if (dmRoomMap.getUserIdForRoomId(room.roomId)) { + tags.push("im.vector.fake.direct"); + } else if (myMembership === 'invite') { + tags.push("im.vector.fake.invite"); + } else if (tags.length === 0) { + tags.push("im.vector.fake.recent"); + } + } else { + tags.push("im.vector.fake.archived"); + } + + + return tags; + } + _setRoomCategory(room, category) { if (!room) return; // This should only happen in tests @@ -260,127 +285,116 @@ class RoomListStore extends Store { return _targetTimestamp; }; - const myMembership = room.getMyMembership(); - let doInsert = true; - const targetTags = []; - if (myMembership !== "join" && myMembership !== "invite") { - doInsert = false; - } else { - const dmRoomMap = DMRoomMap.shared(); - if (dmRoomMap.getUserIdForRoomId(room.roomId)) { - targetTags.push('im.vector.fake.direct'); - } else { - targetTags.push('im.vector.fake.recent'); - } - } + const targetTags = this._getRecommendedTagsForRoom(room); + const insertedIntoTags = []; - // We need to update all instances of a room to ensure that they are correctly organized - // in the list. We do this by shallow-cloning the entire `lists` object using a single - // iterator. Within the loop, we also rebuild the list of rooms per tag (key) so that the - // updated room gets slotted into the right spot. This sacrifices code clarity for not - // iterating on potentially large collections multiple times. + // We need to make sure all the tags (lists) are updated with the room's new position. We + // generally only get called here when there's a new room to insert or a room has potentially + // changed positions within the list. + // + // We do all our checks by iterating over the rooms in the existing lists, trying to insert + // our room where we can. As a guiding principle, we should be removing the room from all + // tags, and insert the room into targetTags. We should perform the deletion before the addition + // where possible to keep a consistent state. By the end of this, targetTags should be the + // same as insertedIntoTags. - let inserted = false; for (const key of Object.keys(this._state.lists)) { - const hasRoom = this._state.lists[key].some((e) => e.room.roomId === room.roomId); + const shouldHaveRoom = targetTags.includes(key); - // Speed optimization: Skip the loop below if we're not going to do anything productive - if (!hasRoom || LIST_ORDERS[key] !== 'recent') { - listsClone[key] = this._state.lists[key]; - if (LIST_ORDERS[key] !== 'recent' && (hasRoom || targetTags.includes(key))) { - // Ensure that we don't try and sort the room into the tag - inserted = true; - doInsert = false; - } - continue; + // Speed optimization: Don't do complicated math if we don't have to. + if (!shouldHaveRoom) { + listsClone[key] = this._state.lists[key].filter((e) => e.room.roomId !== room.roomId); } 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; + // Tags sorted by recents are more complicated than manually ordered tags, so hope + // for the best. + // TODO: Use the SettingsStore watcher to determine if tags are enabled or not + if (LIST_ORDERS[key] !== 'recent') { + // TODO: Actually insert the room + } else { + // 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]) { - // 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 && doInsert && (targetTags.length === 0 || targetTags.includes(key))) { - // 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; + 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 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}); + // 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); + } } - pushedEntry = true; - inserted = true; } + + // Fall through and clone the list. + listsClone[key].push(entry); } - // We insert our own record as needed, so don't let the old one through. - if (entry.room.roomId === room.roomId) { - continue; + if (!pushedEntry) { + if (listsClone[key].length === 0) { + listsClone[key].push({room, category}); + insertedIntoTags.push(key); + } else { + // In theory, this should never happen + console.warn(`!! Room ${room.roomId} lost: No position available`); + } } } - - // Fall through and clone the list. - listsClone[key].push(entry); } } - if (!inserted) { - // There's a good chance that we just joined the room, so we need to organize it - // We also could have left it... - let tags = []; - if (doInsert) { - tags = Object.keys(room.tags); - if (tags.length === 0) { - tags = targetTags; - } - if (tags.length === 0) { - tags = [myMembership === 'join' ? 'im.vector.fake.recent' : 'im.vector.fake.invite']; - } - } else { - tags = ['im.vector.fake.archived']; + // Double check that we inserted the room in the right places + for (const targetTag of targetTags) { + let count = 0; + for (const insertedTag of insertedIntoTags) { + if (insertedTag === targetTag) count++; } - for (const tag of tags) { - for (let i = 0; i < listsClone[tag].length; i++) { - // Just find the top of our category grouping and insert it there. - const catIdxAtPosition = CATEGORY_ORDER.indexOf(listsClone[tag][i].category); - if (catIdxAtPosition >= targetCategoryIndex) { - listsClone[tag].splice(i, 0, {room: room, category: category}); - break; - } - } + + if (count !== 1) { + console.warn(`!! Room ${room.roomId} inserted ${count} times`); } } @@ -574,7 +588,7 @@ class RoomListStore extends Store { return -1; } - return a === b ? this._lexicographicalComparator(roomA, roomB) : ( a > b ? 1 : -1); + return a === b ? this._lexicographicalComparator(roomA, roomB) : (a > b ? 1 : -1); }; } From 3d152da8224d87a063dd23ad31d67cef952545b1 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 25 Feb 2019 13:39:08 -0700 Subject: [PATCH 2/2] Support custom tags in the new algorithm --- .../handlers/AccountSettingsHandler.js | 1 - src/stores/RoomListStore.js | 169 ++++++++++-------- 2 files changed, 94 insertions(+), 76 deletions(-) diff --git a/src/settings/handlers/AccountSettingsHandler.js b/src/settings/handlers/AccountSettingsHandler.js index 4bef585e6b..3bffc9151e 100644 --- a/src/settings/handlers/AccountSettingsHandler.js +++ b/src/settings/handlers/AccountSettingsHandler.js @@ -52,7 +52,6 @@ export default class AccountSettingsHandler extends MatrixClientBackedSettingsHa } else if (event.getType() === "im.vector.web.settings") { // We can't really discern what changed, so trigger updates for everything for (const settingName of Object.keys(event.getContent())) { - console.log(settingName); this._watchers.notifyUpdate(settingName, null, event.getContent()[settingName]); } } diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index d74500717e..082535543c 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -64,7 +64,7 @@ class RoomListStore extends Store { * behave. * @param {boolean} forceRegeneration True to force a change in the algorithm */ - updateSortingAlgorithm(forceRegeneration=false) { + updateSortingAlgorithm(forceRegeneration = false) { const byImportance = SettingsStore.getValue("RoomList.orderByImportance"); if (byImportance !== this._state.orderRoomsByImportance || forceRegeneration) { console.log("Updating room sorting algorithm: sortByImportance=" + byImportance); @@ -119,8 +119,15 @@ class RoomListStore extends Store { const logicallyReady = this._matrixClient && this._state.ready; switch (payload.action) { case 'setting_updated': { - if (payload.settingName !== 'RoomList.orderByImportance') break; - this.updateSortingAlgorithm(); + if (payload.settingName === 'RoomList.orderByImportance') { + this.updateSortingAlgorithm(); + } else if (payload.settingName === 'feature_custom_tags') { + const isActive = SettingsStore.isFeatureEnabled("feature_custom_tags"); + if (isActive !== this._state.tagsEnabled) { + this._setState({tagsEnabled: isActive}); + this.updateSortingAlgorithm(/*force=*/true); + } + } } break; // Initialise state after initial sync @@ -129,6 +136,8 @@ class RoomListStore extends Store { break; } + this._setState({tagsEnabled: SettingsStore.isFeatureEnabled("feature_custom_tags")}); + this._matrixClient = payload.matrixClient; this.updateSortingAlgorithm(/*force=*/true); } @@ -269,14 +278,19 @@ class RoomListStore extends Store { } } + _filterTags(tags) { + tags = tags ? Object.keys(tags) : []; + if (this._state.tagsEnabled) return tags; + return tags.filter((t) => !!LIST_ORDERS[t]); + } + _getRecommendedTagsForRoom(room) { const tags = []; const myMembership = room.getMyMembership(); if (myMembership === 'join' || myMembership === 'invite') { // Stack the user's tags on top - // TODO: Consider whether tags are enabled at all - tags.push(...(room.tags || [])); + tags.push(...this._filterTags(room.tags)); const dmRoomMap = DMRoomMap.shared(); if (dmRoomMap.getUserIdForRoomId(room.roomId)) { @@ -328,84 +342,83 @@ class RoomListStore extends Store { // Speed optimization: Don't do complicated math if we don't have to. if (!shouldHaveRoom) { listsClone[key] = this._state.lists[key].filter((e) => e.room.roomId !== room.roomId); + } else if (LIST_ORDERS[key] !== 'recent') { + // Manually ordered tags are sorted later, so for now we'll just clone the tag + // and add our room if needed + listsClone[key] = this._state.lists[key].filter((e) => e.room.roomId !== room.roomId); + listsClone[key].push({room, category}); + insertedIntoTags.push(key); } else { listsClone[key] = []; - // Tags sorted by recents are more complicated than manually ordered tags, so hope - // for the best. - // TODO: Use the SettingsStore watcher to determine if tags are enabled or not - if (LIST_ORDERS[key] !== 'recent') { - // TODO: Actually insert the room - } else { - // 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; + // 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); + 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 (!pushedEntry) { - if (listsClone[key].length === 0) { - listsClone[key].push({room, category}); - insertedIntoTags.push(key); - } else { - // In theory, this should never happen - console.warn(`!! Room ${room.roomId} lost: No position available`); + // 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); + } + + if (!pushedEntry) { + if (listsClone[key].length === 0) { + listsClone[key].push({room, category}); + insertedIntoTags.push(key); + } else { + // In theory, this should never happen + console.warn(`!! Room ${room.roomId} lost: No position available`); + } } } } @@ -422,6 +435,12 @@ class RoomListStore extends Store { } } + // Sort the favourites before we set the clone + for (const tag of Object.keys(listsClone)) { + if (LIST_ORDERS[tag] === 'recent') continue; // skip recents (pre-sorted) + listsClone[tag].sort(this._getManualComparator(tag)); + } + this._setState({lists: listsClone}); }