From b6c1b50fd9aeca6e6666886c76d52429716ebd71 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 13 Feb 2019 13:00:35 -0700 Subject: [PATCH 01/16] Early support for improved room algorithm This changes the approach from regenerating every time there's a change to incrementally fixing the room lists. Additionally, this forces the pin options on for people and implements the sticky room behaviour. Known bugs include newly joined rooms, invites, etc not sorting correctly. --- src/stores/RoomListStore.js | 373 ++++++++++++++++++------------------ 1 file changed, 184 insertions(+), 189 deletions(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index d98adc5cae..1eec41a20a 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -19,20 +19,26 @@ import DMRoomMap from '../utils/DMRoomMap'; import Unread from '../Unread'; import SettingsStore from "../settings/SettingsStore"; +const CATEGORY_RED = "red"; +const CATEGORY_GREY = "grey"; +const CATEGORY_BOLD = "bold"; +const CATEGORY_IDLE = "idle"; + +const CATEGORY_ORDER = [CATEGORY_RED, CATEGORY_GREY, CATEGORY_BOLD, CATEGORY_IDLE]; +const LIST_ORDERS = { + "m.favourite": "manual", + "im.vector.fake.invite": "recent", + "im.vector.fake.recent": "recent", + "im.vector.fake.direct": "recent", + "m.lowpriority": "recent", + "im.vector.fake.archived": "recent", +}; + /** * A class for storing application state for categorising rooms in * the RoomList. */ class RoomListStore extends Store { - static _listOrders = { - "m.favourite": "manual", - "im.vector.fake.invite": "recent", - "im.vector.fake.recent": "recent", - "im.vector.fake.direct": "recent", - "m.lowpriority": "recent", - "im.vector.fake.archived": "recent", - }; - constructor() { super(dis); @@ -43,44 +49,39 @@ class RoomListStore extends Store { _init() { // Initialise state + const defaultLists = { + "m.server_notice": [/* { room: js-sdk room, category: string } */], + "im.vector.fake.invite": [], + "m.favourite": [], + "im.vector.fake.recent": [], + "im.vector.fake.direct": [], + "m.lowpriority": [], + "im.vector.fake.archived": [], + }; this._state = { - lists: { - "m.server_notice": [], - "im.vector.fake.invite": [], - "m.favourite": [], - "im.vector.fake.recent": [], - "im.vector.fake.direct": [], - "m.lowpriority": [], - "im.vector.fake.archived": [], - }, + // The rooms in these arrays are ordered according to either the + // 'recents' behaviour or 'manual' behaviour. + lists: defaultLists, + presentationLists: defaultLists, // like `lists`, but with arrays of rooms instead ready: false, - - // The room cache stores a mapping of roomId to cache record. - // Each cache record is a key/value pair for various bits of - // data used to sort the room list. Currently this stores the - // following bits of informations: - // "timestamp": number, The timestamp of the last relevant - // event in the room. - // "notifications": boolean, Whether or not the user has been - // highlighted on any unread events. - // "unread": boolean, Whether or not the user has any - // unread events. - // - // All of the cached values are lazily loaded on read in the - // recents comparator. When an event is received for a particular - // room, all the cached values are invalidated - forcing the - // next read to set new values. The entries do not expire on - // their own. - roomCache: {}, + stickyRoomId: null, }; } _setState(newState) { + if (newState['lists']) { + const presentationLists = {}; + for (const key of Object.keys(newState['lists'])) { + presentationLists[key] = newState['lists'][key].map((e) => e.room); + } + newState['presentationLists'] = presentationLists; + } this._state = Object.assign(this._state, newState); this.__emitChange(); } - __onDispatch(payload) { + __onDispatch = (payload) => { + const logicallyReady = this._matrixClient && this._state.ready; switch (payload.action) { // Initialise state after initial sync case 'MatrixActions.sync': { @@ -89,30 +90,30 @@ class RoomListStore extends Store { } this._matrixClient = payload.matrixClient; - this._generateRoomLists(); + this._generateInitialRoomLists(); } break; case 'MatrixActions.Room.tags': { - if (!this._state.ready) break; - this._generateRoomLists(); + if (!logicallyReady) break; + console.log("!! Tags: ", payload); } break; case 'MatrixActions.Room.timeline': { - if (!this._state.ready || + if (!logicallyReady || !payload.isLiveEvent || !payload.isLiveUnfilteredRoomTimelineEvent || !this._eventTriggersRecentReorder(payload.event) - ) break; + ) { + break; + } - this._clearCachedRoomState(payload.event.getRoomId()); - this._generateRoomLists(); + this._roomUpdateTriggered(payload.event.getRoomId()); } break; // When an event is decrypted, it could mean we need to reorder the room // list because we now know the type of the event. case 'MatrixActions.Event.decrypted': { - // We may not have synced or done an initial generation of the lists - if (!this._matrixClient || !this._state.ready) break; + if (!logicallyReady) break; const roomId = payload.event.getRoomId(); @@ -129,50 +130,57 @@ class RoomListStore extends Store { // Either this event was not added to the live timeline (e.g. pagination) // or it doesn't affect the ordering of the room list. - if (liveTimeline !== eventTimeline || - !this._eventTriggersRecentReorder(payload.event) - ) break; + if (liveTimeline !== eventTimeline || !this._eventTriggersRecentReorder(payload.event)) { + break; + } - this._clearCachedRoomState(payload.event.getRoomId()); - this._generateRoomLists(); + this._roomUpdateTriggered(roomId); } break; case 'MatrixActions.accountData': { + if (!logicallyReady) break; if (payload.event_type !== 'm.direct') break; - this._generateRoomLists(); - } - break; - case 'MatrixActions.Room.accountData': { - if (payload.event_type === 'm.fully_read') { - this._clearCachedRoomState(payload.room.roomId); - this._generateRoomLists(); - } + // TODO: Handle direct chat changes + console.log("!! Direct Chats: ", payload); } break; + // TODO: Remove if not actually needed + // case 'MatrixActions.Room.accountData': { + // if (!logicallyReady) break; + // if (payload.event_type === 'm.fully_read') { + // console.log("!! Fully read: ", payload); + // } + // } + // break; case 'MatrixActions.Room.myMembership': { - this._generateRoomLists(); + if (!logicallyReady) break; + // TODO: Slot room into list + this._roomUpdateTriggered(payload.room.roomId); } break; // This could be a new room that we've been invited to, joined or created // we won't get a RoomMember.membership for these cases if we're not already // a member. case 'MatrixActions.Room': { - if (!this._state.ready || !this._matrixClient.credentials.userId) break; - this._generateRoomLists(); + if (!logicallyReady) break; + // TODO: Slot room into list + this._roomUpdateTriggered(payload.room.roomId); } break; case 'RoomListActions.tagRoom.pending': { + if (!logicallyReady) break; // XXX: we only show one optimistic update at any one time. // Ideally we should be making a list of in-flight requests // that are backed by transaction IDs. Until the js-sdk // supports this, we're stuck with only being able to use // the most recent optimistic update. - this._generateRoomLists(payload.request); + console.log("!! Optimistic tag: ", payload); } break; case 'RoomListActions.tagRoom.failure': { + if (!logicallyReady) break; // Reset state according to js-sdk - this._generateRoomLists(); + console.log("!! Optimistic tag failure: ", payload); } break; case 'on_logged_out': { @@ -182,10 +190,73 @@ class RoomListStore extends Store { this._matrixClient = null; } break; + case 'view_room': { + if (!logicallyReady) break; + + // Note: it is important that we set a new stickyRoomId before setting the old room + // to IDLE. If we don't, the wrong room gets counted as sticky. + const currentSticky = this._state.stickyRoomId; + this._setState({stickyRoomId: payload.room_id}); + if (currentSticky) { + this._setRoomCategory(this._matrixClient.getRoom(currentSticky), CATEGORY_IDLE); + } + } + break; + } + }; + + _roomUpdateTriggered(roomId) { + const room = this._matrixClient.getRoom(roomId); + if (!room) return; + + if (this._state.stickyRoomId !== room.roomId) { + this._setRoomCategory(room, this._calculateCategory(room)); } } - _generateRoomLists(optimisticRequest) { + _setRoomCategory(room, category) { + const listsClone = {}; + const targetCatIndex = CATEGORY_ORDER.indexOf(category); + + // 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. + + for (const key of Object.keys(this._state.lists)) { + listsClone[key] = []; + let pushedEntry = false; + const hasRoom = !!this._state.lists[key].find((e) => e.room.roomId === room.roomId); + 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 (LIST_ORDERS[key] === 'recent' && hasRoom && entry.room.roomId !== this._state.stickyRoomId) { + if (!pushedEntry) { + // If we've hit the top of a boundary (either because there's no rooms in the target or + // we've reached the grouping of rooms), insert our room ahead of the others in the category. + // This ensures that our room is on top (more recent) than the others. + const changedBoundary = CATEGORY_ORDER.indexOf(entry.category) >= targetCatIndex; + if (changedBoundary) { + listsClone[key].push({room: room, category: category}); + pushedEntry = true; + } + } + + // We insert our own record as needed, so don't let the old one through. + if (entry.room.roomId === room.roomId) { + continue; + } + } + + // Fall through and clone the list. + listsClone[key].push(entry); + } + } + + this._setState({lists: listsClone}); + } + + _generateInitialRoomLists() { const lists = { "m.server_notice": [], "im.vector.fake.invite": [], @@ -196,36 +267,20 @@ class RoomListStore extends Store { "im.vector.fake.archived": [], }; - const dmRoomMap = DMRoomMap.shared(); - - // If somehow we dispatched a RoomListActions.tagRoom.failure before a MatrixActions.sync - if (!this._matrixClient) return; - const isCustomTagsEnabled = SettingsStore.isFeatureEnabled("feature_custom_tags"); - this._matrixClient.getRooms().forEach((room, index) => { + this._matrixClient.getRooms().forEach((room) => { const myUserId = this._matrixClient.getUserId(); const membership = room.getMyMembership(); const me = room.getMember(myUserId); - if (membership == "invite") { - lists["im.vector.fake.invite"].push(room); - } else if (membership == "join" || membership === "ban" || (me && me.isKicked())) { + if (membership === "invite") { + lists["im.vector.fake.invite"].push({room, category: CATEGORY_RED}); + } else if (membership === "join" || membership === "ban" || (me && me.isKicked())) { // Used to split rooms via tags let tagNames = Object.keys(room.tags); - if (optimisticRequest && optimisticRequest.room === room) { - // Remove old tag - tagNames = tagNames.filter((tagName) => tagName !== optimisticRequest.oldTag); - // Add new tag - if (optimisticRequest.newTag && - !tagNames.includes(optimisticRequest.newTag) - ) { - tagNames.push(optimisticRequest.newTag); - } - } - // ignore any m. tag names we don't know about tagNames = tagNames.filter((t) => { return (isCustomTagsEnabled && !t.startsWith('m.')) || lists[t] !== undefined; @@ -235,35 +290,31 @@ class RoomListStore extends Store { for (let i = 0; i < tagNames.length; i++) { const tagName = tagNames[i]; lists[tagName] = lists[tagName] || []; - lists[tagName].push(room); + + // We categorize all the tagged rooms the same because we don't actually + // care about the order (it's defined elsewhere) + lists[tagName].push({room, category: CATEGORY_RED}); } } else if (dmRoomMap.getUserIdForRoomId(room.roomId)) { // "Direct Message" rooms (that we're still in and that aren't otherwise tagged) - lists["im.vector.fake.direct"].push(room); + lists["im.vector.fake.direct"].push({room, category: this._calculateCategory(room)}); } else { - lists["im.vector.fake.recent"].push(room); + lists["im.vector.fake.recent"].push({room, category: this._calculateCategory(room)}); } } else if (membership === "leave") { - lists["im.vector.fake.archived"].push(room); + lists["im.vector.fake.archived"].push({room, category: this._calculateCategory(room)}); } }); - // Note: we check the settings up here instead of in the forEach or - // in the _recentsComparator to avoid hitting the SettingsStore a few - // thousand times. - const pinUnread = SettingsStore.getValue("pinUnreadRooms"); - const pinMentioned = SettingsStore.getValue("pinMentionedRooms"); Object.keys(lists).forEach((listKey) => { let comparator; - switch (RoomListStore._listOrders[listKey]) { + switch (LIST_ORDERS[listKey]) { case "recent": - comparator = (roomA, roomB) => { - return this._recentsComparator(roomA, roomB, pinUnread, pinMentioned); - }; + comparator = this._recentsComparator; break; case "manual": default: - comparator = this._getManualComparator(listKey, optimisticRequest); + comparator = this._getManualComparator(listKey); break; } lists[listKey].sort(comparator); @@ -271,52 +322,10 @@ class RoomListStore extends Store { this._setState({ lists, - ready: true, // Ready to receive updates via Room.tags events + ready: true, // Ready to receive updates to ordering }); } - _updateCachedRoomState(roomId, type, value) { - const roomCache = this._state.roomCache; - if (!roomCache[roomId]) roomCache[roomId] = {}; - - if (typeof value !== "undefined") roomCache[roomId][type] = value; - else delete roomCache[roomId][type]; - - this._setState({roomCache}); - } - - _clearCachedRoomState(roomId) { - const roomCache = this._state.roomCache; - delete roomCache[roomId]; - this._setState({roomCache}); - } - - _getRoomState(room, type) { - const roomId = room.roomId; - const roomCache = this._state.roomCache; - if (roomCache[roomId] && typeof roomCache[roomId][type] !== 'undefined') { - return roomCache[roomId][type]; - } - - if (type === "timestamp") { - const ts = this._tsOfNewestEvent(room); - this._updateCachedRoomState(roomId, "timestamp", ts); - return ts; - } else if (type === "unread-muted") { - const unread = Unread.doesRoomHaveUnreadMessages(room); - this._updateCachedRoomState(roomId, "unread-muted", unread); - return unread; - } else if (type === "unread") { - const unread = room.getUnreadNotificationCount() > 0; - this._updateCachedRoomState(roomId, "unread", unread); - return unread; - } else if (type === "notifications") { - const notifs = room.getUnreadNotificationCount("highlight") > 0; - this._updateCachedRoomState(roomId, "notifications", notifs); - return notifs; - } else throw new Error("Unrecognized room cache type: " + type); - } - _eventTriggersRecentReorder(ev) { return ev.getTs() && ( Unread.eventTriggersUnreadCount(ev) || @@ -342,53 +351,36 @@ class RoomListStore extends Store { } } - _recentsComparator(roomA, roomB, pinUnread, pinMentioned) { - // We try and set the ordering to be Mentioned > Unread > Recent - // assuming the user has the right settings, of course. + _calculateCategory(room) { + const mentions = room.getUnreadNotificationCount("highlight") > 0; + if (mentions) return CATEGORY_RED; - const timestampA = this._getRoomState(roomA, "timestamp"); - const timestampB = this._getRoomState(roomB, "timestamp"); - const timestampDiff = timestampB - timestampA; + let unread = room.getUnreadNotificationCount() > 0; + if (unread) return CATEGORY_GREY; - if (pinMentioned) { - const mentionsA = this._getRoomState(roomA, "notifications"); - const mentionsB = this._getRoomState(roomB, "notifications"); - if (mentionsA && !mentionsB) return -1; - if (!mentionsA && mentionsB) return 1; + unread = Unread.doesRoomHaveUnreadMessages(room); + if (unread) return CATEGORY_BOLD; - // If they both have notifications, sort by timestamp. - // If neither have notifications (the fourth check not shown - // here), then try and sort by unread messages and finally by - // timestamp. - if (mentionsA && mentionsB) return timestampDiff; + return CATEGORY_IDLE; + } + + _recentsComparator(entryA, entryB) { + const roomA = entryA.room; + const roomB = entryB.room; + const categoryA = entryA.category; + const categoryB = entryB.category; + + if (categoryA !== categoryB) { + const idxA = CATEGORY_ORDER.indexOf(categoryA); + const idxB = CATEGORY_ORDER.indexOf(categoryB); + if (idxA > idxB) return 1; + if (idxA < idxB) return -1; + return 0; } - if (pinUnread) { - let unreadA = this._getRoomState(roomA, "unread"); - let unreadB = this._getRoomState(roomB, "unread"); - if (unreadA && !unreadB) return -1; - if (!unreadA && unreadB) return 1; - - // If they both have unread messages, sort by timestamp - // If nether have unread message (the fourth check not shown - // here), then just sort by timestamp anyways. - if (unreadA && unreadB) return timestampDiff; - - // Unread can also mean "unread without badge", which is - // different from what the above checks for. We're also - // going to sort those here. - unreadA = this._getRoomState(roomA, "unread-muted"); - unreadB = this._getRoomState(roomB, "unread-muted"); - if (unreadA && !unreadB) return -1; - if (!unreadA && unreadB) return 1; - - // If they both have unread messages, sort by timestamp - // If nether have unread message (the fourth check not shown - // here), then just sort by timestamp anyways. - if (unreadA && unreadB) return timestampDiff; - } - - return timestampDiff; + const timestampA = this._tsOfNewestEvent(roomA); + const timestampB = this._tsOfNewestEvent(roomB); + return timestampB - timestampA; } _lexicographicalComparator(roomA, roomB) { @@ -396,7 +388,10 @@ class RoomListStore extends Store { } _getManualComparator(tagName, optimisticRequest) { - return (roomA, roomB) => { + return (entryA, entryB) => { + const roomA = entryA.room; + const roomB = entryB.room; + let metaA = roomA.tags[tagName]; let metaB = roomB.tags[tagName]; @@ -404,8 +399,8 @@ class RoomListStore extends Store { if (optimisticRequest && roomB === optimisticRequest.room) metaB = optimisticRequest.metaData; // Make sure the room tag has an order element, if not set it to be the bottom - const a = metaA ? metaA.order : undefined; - const b = metaB ? metaB.order : undefined; + const a = metaA ? Number(metaA.order) : undefined; + const b = metaB ? Number(metaB.order) : undefined; // Order undefined room tag orders to the bottom if (a === undefined && b !== undefined) { @@ -414,12 +409,12 @@ 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); }; } getRoomLists() { - return this._state.lists; + return this._state.presentationLists; } } From 9175655c16ecc11b224f9573833f32ee19c8847b Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 13 Feb 2019 13:13:40 -0700 Subject: [PATCH 02/16] Remove old pin unread options They are not forced on, and do nothing. --- .../views/settings/tabs/PreferencesSettingsTab.js | 8 -------- src/i18n/strings/en_EN.json | 3 --- src/settings/Settings.js | 10 ---------- 3 files changed, 21 deletions(-) diff --git a/src/components/views/settings/tabs/PreferencesSettingsTab.js b/src/components/views/settings/tabs/PreferencesSettingsTab.js index b455938563..d76dc8f3dd 100644 --- a/src/components/views/settings/tabs/PreferencesSettingsTab.js +++ b/src/components/views/settings/tabs/PreferencesSettingsTab.js @@ -30,11 +30,6 @@ export default class PreferencesSettingsTab extends React.Component { 'sendTypingNotifications', ]; - static ROOM_LIST_SETTINGS = [ - 'pinUnreadRooms', - 'pinMentionedRooms', - ]; - static TIMELINE_SETTINGS = [ 'autoplayGifsAndVideos', 'urlPreviewsEnabled', @@ -106,9 +101,6 @@ export default class PreferencesSettingsTab extends React.Component { {_t("Composer")} {this._renderGroup(PreferencesSettingsTab.COMPOSER_SETTINGS)} - {_t("Room list")} - {this._renderGroup(PreferencesSettingsTab.ROOM_LIST_SETTINGS)} - {_t("Timeline")} {this._renderGroup(PreferencesSettingsTab.TIMELINE_SETTINGS)} diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index fe41beb7ae..380b9e894f 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -300,8 +300,6 @@ "Enable URL previews for this room (only affects you)": "Enable URL previews for this room (only affects you)", "Enable URL previews by default for participants in this room": "Enable URL previews by default for participants in this room", "Room Colour": "Room Colour", - "Pin rooms I'm mentioned in to the top of the room list": "Pin rooms I'm mentioned in to the top of the room list", - "Pin unread rooms to the top of the room list": "Pin unread rooms to the top of the room list", "Enable widget screenshots on supported widgets": "Enable widget screenshots on supported widgets", "Prompt before sending invites to potentially invalid matrix IDs": "Prompt before sending invites to potentially invalid matrix IDs", "Show developer tools": "Show developer tools", @@ -551,7 +549,6 @@ "Start automatically after system login": "Start automatically after system login", "Preferences": "Preferences", "Composer": "Composer", - "Room list": "Room list", "Timeline": "Timeline", "Autocomplete delay (ms)": "Autocomplete delay (ms)", "To change the room's avatar, you must be a": "To change the room's avatar, you must be a", diff --git a/src/settings/Settings.js b/src/settings/Settings.js index 4108848033..cf68fed8ba 100644 --- a/src/settings/Settings.js +++ b/src/settings/Settings.js @@ -321,16 +321,6 @@ export const SETTINGS = { default: true, controller: new AudioNotificationsEnabledController(), }, - "pinMentionedRooms": { - supportedLevels: LEVELS_ACCOUNT_SETTINGS, - displayName: _td("Pin rooms I'm mentioned in to the top of the room list"), - default: false, - }, - "pinUnreadRooms": { - supportedLevels: LEVELS_ACCOUNT_SETTINGS, - displayName: _td("Pin unread rooms to the top of the room list"), - default: false, - }, "enableWidgetScreenshots": { supportedLevels: LEVELS_ACCOUNT_SETTINGS, displayName: _td('Enable widget screenshots on supported widgets'), From b741b76797a77f2f21785bbfd8652437b5a2c867 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 13 Feb 2019 14:08:19 -0700 Subject: [PATCH 03/16] Handle joins/leaves safely --- src/stores/RoomListStore.js | 66 +++++++++++++++++++++++++++---------- 1 file changed, 48 insertions(+), 18 deletions(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index 1eec41a20a..b2ceb9cfd4 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -154,7 +154,6 @@ class RoomListStore extends Store { // break; case 'MatrixActions.Room.myMembership': { if (!logicallyReady) break; - // TODO: Slot room into list this._roomUpdateTriggered(payload.room.roomId); } break; @@ -163,25 +162,25 @@ class RoomListStore extends Store { // a member. case 'MatrixActions.Room': { if (!logicallyReady) break; - // TODO: Slot room into list this._roomUpdateTriggered(payload.room.roomId); } break; - case 'RoomListActions.tagRoom.pending': { - if (!logicallyReady) break; - // XXX: we only show one optimistic update at any one time. - // Ideally we should be making a list of in-flight requests - // that are backed by transaction IDs. Until the js-sdk - // supports this, we're stuck with only being able to use - // the most recent optimistic update. - console.log("!! Optimistic tag: ", payload); - } - break; - case 'RoomListActions.tagRoom.failure': { - if (!logicallyReady) break; - // Reset state according to js-sdk - console.log("!! Optimistic tag failure: ", payload); - } + // TODO: Re-enable optimistic updates when we support dragging again + // case 'RoomListActions.tagRoom.pending': { + // if (!logicallyReady) break; + // // XXX: we only show one optimistic update at any one time. + // // Ideally we should be making a list of in-flight requests + // // that are backed by transaction IDs. Until the js-sdk + // // supports this, we're stuck with only being able to use + // // the most recent optimistic update. + // console.log("!! Optimistic tag: ", payload); + // } + // break; + // case 'RoomListActions.tagRoom.failure': { + // if (!logicallyReady) break; + // // Reset state according to js-sdk + // console.log("!! Optimistic tag failure: ", payload); + // } break; case 'on_logged_out': { // Reset state without pushing an update to the view, which generally assumes that @@ -218,11 +217,18 @@ class RoomListStore extends Store { const listsClone = {}; const targetCatIndex = CATEGORY_ORDER.indexOf(category); + const myMembership = room.getMyMembership(); + let doInsert = true; + if (myMembership !== "join" && myMembership !== "invite") { + doInsert = false; + } + // 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. + let inserted = false; for (const key of Object.keys(this._state.lists)) { listsClone[key] = []; let pushedEntry = false; @@ -231,7 +237,7 @@ class RoomListStore extends Store { // 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 (LIST_ORDERS[key] === 'recent' && hasRoom && entry.room.roomId !== this._state.stickyRoomId) { - if (!pushedEntry) { + if (!pushedEntry && doInsert) { // If we've hit the top of a boundary (either because there's no rooms in the target or // we've reached the grouping of rooms), insert our room ahead of the others in the category. // This ensures that our room is on top (more recent) than the others. @@ -239,6 +245,7 @@ class RoomListStore extends Store { if (changedBoundary) { listsClone[key].push({room: room, category: category}); pushedEntry = true; + inserted = true; } } @@ -253,6 +260,29 @@ class RoomListStore extends Store { } } + 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.push(myMembership === 'join' ? 'im.vector.fake.recent' : 'im.vector.fake.invite'); + } + } else { + tags = ['im.vector.fake.archived']; + } + for (const tag of tags) { + for (let i = 0; i < listsClone[tag].length; i++) { + const catIdxAtPosition = CATEGORY_ORDER.indexOf(listsClone[tag][i].category); + if (catIdxAtPosition >= targetCatIndex) { + listsClone[tag].splice(i, 0, {room: room, category: category}); + break; + } + } + } + } + this._setState({lists: listsClone}); } From 2eb80f793ceec9bf4d2fa158a9e8cc9110a6ac10 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 13 Feb 2019 14:22:00 -0700 Subject: [PATCH 04/16] Try to handle direct chats and tag changes This is a very blunt approach in that it ignores the sticky room. --- src/stores/RoomListStore.js | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index b2ceb9cfd4..58300e43ea 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -95,7 +95,9 @@ class RoomListStore extends Store { break; case 'MatrixActions.Room.tags': { if (!logicallyReady) break; - console.log("!! Tags: ", payload); + // TODO: Figure out which rooms changed in the tag and only change those. + // This is very blunt and wipes out the sticky room stuff + this._generateInitialRoomLists(); } break; case 'MatrixActions.Room.timeline': { @@ -140,8 +142,9 @@ class RoomListStore extends Store { case 'MatrixActions.accountData': { if (!logicallyReady) break; if (payload.event_type !== 'm.direct') break; - // TODO: Handle direct chat changes - console.log("!! Direct Chats: ", payload); + // TODO: Figure out which rooms changed in the direct chat and only change those. + // This is very blunt and wipes out the sticky room stuff + this._generateInitialRoomLists(); } break; // TODO: Remove if not actually needed @@ -181,7 +184,7 @@ class RoomListStore extends Store { // // Reset state according to js-sdk // console.log("!! Optimistic tag failure: ", payload); // } - break; + // break; case 'on_logged_out': { // Reset state without pushing an update to the view, which generally assumes that // the matrix client isn't `null` and so causing a re-render will cause NPEs. @@ -213,7 +216,7 @@ class RoomListStore extends Store { } } - _setRoomCategory(room, category) { + _setRoomCategory(room, category, targetTags = []) { const listsClone = {}; const targetCatIndex = CATEGORY_ORDER.indexOf(category); @@ -221,6 +224,17 @@ class RoomListStore extends Store { let doInsert = true; if (myMembership !== "join" && myMembership !== "invite") { doInsert = false; + } else { + const dmRoomMap = DMRoomMap.shared(); + if (dmRoomMap.getUserIdForRoomId(room.roomId)) { + if (!targetTags.includes('im.vector.fake.direct')) { + targetTags.push('im.vector.fake.direct'); + } + } else { + if (!targetTags.includes('im.vector.fake.recent')) { + targetTags.push('im.vector.fake.recent'); + } + } } // We need to update all instances of a room to ensure that they are correctly organized @@ -237,7 +251,8 @@ class RoomListStore extends Store { // 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 (LIST_ORDERS[key] === 'recent' && hasRoom && entry.room.roomId !== this._state.stickyRoomId) { - if (!pushedEntry && doInsert) { + const inTag = targetTags.length === 0 || targetTags.includes('im.vector.ake.recent'); + if (!pushedEntry && doInsert && inTag) { // If we've hit the top of a boundary (either because there's no rooms in the target or // we've reached the grouping of rooms), insert our room ahead of the others in the category. // This ensures that our room is on top (more recent) than the others. @@ -266,6 +281,9 @@ class RoomListStore extends Store { let tags = []; if (doInsert) { tags = Object.keys(room.tags); + if (tags.length === 0) { + tags = targetTags; + } if (tags.length === 0) { tags.push(myMembership === 'join' ? 'im.vector.fake.recent' : 'im.vector.fake.invite'); } From 821b34b487169d3813bca78567d3fba61d164064 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 13 Feb 2019 17:03:27 -0700 Subject: [PATCH 05/16] React to read receipt changes from ourselves When a room is read on another device, it should be re-sorted --- src/actions/MatrixActionCreators.js | 19 +++++++++++++++++++ src/stores/RoomListStore.js | 17 ++++++++++++++++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/actions/MatrixActionCreators.js b/src/actions/MatrixActionCreators.js index c1d42ffd0d..c89ec44435 100644 --- a/src/actions/MatrixActionCreators.js +++ b/src/actions/MatrixActionCreators.js @@ -131,6 +131,24 @@ function createRoomTagsAction(matrixClient, roomTagsEvent, room) { return { action: 'MatrixActions.Room.tags', room }; } +/** + * Create a MatrixActions.Room.receipt action that represents a MatrixClient + * `Room.receipt` event, each parameter mapping to a key-value in the action. + * + * @param {MatrixClient} matrixClient the matrix client + * @param {MatrixEvent} event the receipt event. + * @param {Room} room the room the receipt happened in. + * @returns {Object} an action of type MatrixActions.Room.receipt. + */ +function createRoomReceiptAction(matrixClient, event, room) { + return { + action: 'MatrixActions.Room.receipt', + event, + room, + matrixClient, + }; +} + /** * @typedef RoomTimelineAction * @type {Object} @@ -233,6 +251,7 @@ export default { this._addMatrixClientListener(matrixClient, 'Room.accountData', createRoomAccountDataAction); this._addMatrixClientListener(matrixClient, 'Room', createRoomAction); this._addMatrixClientListener(matrixClient, 'Room.tags', createRoomTagsAction); + this._addMatrixClientListener(matrixClient, 'Room.receipt', createRoomReceiptAction); this._addMatrixClientListener(matrixClient, 'Room.timeline', createRoomTimelineAction); this._addMatrixClientListener(matrixClient, 'Room.myMembership', createSelfMembershipAction); this._addMatrixClientListener(matrixClient, 'Event.decrypted', createEventDecryptedAction); diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index 58300e43ea..30d14351e0 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -93,6 +93,20 @@ class RoomListStore extends Store { this._generateInitialRoomLists(); } break; + case 'MatrixActions.Room.receipt': { + if (!logicallyReady) break; + + // First see if the receipt event is for our own user + const myUserId = this._matrixClient.getUserId(); + for (const eventId of Object.keys(payload.event.getContent())) { + const receiptUsers = Object.keys(payload.event.getContent()[eventId]['m.read'] || {}); + if (receiptUsers.includes(myUserId)) { + this._roomUpdateTriggered(payload.room.roomId); + return; + } + } + } + break; case 'MatrixActions.Room.tags': { if (!logicallyReady) break; // TODO: Figure out which rooms changed in the tag and only change those. @@ -212,7 +226,8 @@ class RoomListStore extends Store { if (!room) return; if (this._state.stickyRoomId !== room.roomId) { - this._setRoomCategory(room, this._calculateCategory(room)); + const category = this._calculateCategory(room); + this._setRoomCategory(room, category); } } From 52f48f742246828855c508b6021b0c820c029a68 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 13 Feb 2019 18:19:18 -0700 Subject: [PATCH 06/16] Order by timestamp within the categorized room lists When we load the page, all encrypted events arrive well after we've generated our initial grouping which can cause them to jump to the top of their categories wrongly. For direct chats, this meant that people who don't have a lot of unread messages would see ancient rooms bubbling to the top for no reason after the page has loaded. We still have to track when the last category change was (ie: when we switched from red -> grey) so that when the category doesn't exist in the list we can insert the room at the right place (the start of the last category when we switch beyond the order expected). --- src/stores/RoomListStore.js | 39 ++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index 30d14351e0..84939fa129 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -231,24 +231,22 @@ class RoomListStore extends Store { } } - _setRoomCategory(room, category, targetTags = []) { + _setRoomCategory(room, category) { const listsClone = {}; const targetCatIndex = CATEGORY_ORDER.indexOf(category); + const targetTs = this._tsOfNewestEvent(room); const myMembership = room.getMyMembership(); let doInsert = true; + let targetTags = []; if (myMembership !== "join" && myMembership !== "invite") { doInsert = false; } else { const dmRoomMap = DMRoomMap.shared(); if (dmRoomMap.getUserIdForRoomId(room.roomId)) { - if (!targetTags.includes('im.vector.fake.direct')) { - targetTags.push('im.vector.fake.direct'); - } + targetTags.push('im.vector.fake.direct'); } else { - if (!targetTags.includes('im.vector.fake.recent')) { - targetTags.push('im.vector.fake.recent'); - } + targetTags.push('im.vector.fake.recent'); } } @@ -262,21 +260,40 @@ class RoomListStore extends Store { listsClone[key] = []; let pushedEntry = false; const hasRoom = !!this._state.lists[key].find((e) => e.room.roomId === room.roomId); + let lastCategoryBoundary = 0; + let lastCategoryIndex = 0; 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 (LIST_ORDERS[key] === 'recent' && hasRoom && entry.room.roomId !== this._state.stickyRoomId) { - const inTag = targetTags.length === 0 || targetTags.includes('im.vector.ake.recent'); + const inTag = targetTags.length === 0 || targetTags.includes(key); if (!pushedEntry && doInsert && inTag) { + const entryTs = this._tsOfNewestEvent(entry.room); + const entryCategory = CATEGORY_ORDER.indexOf(entry.category); + // If we've hit the top of a boundary (either because there's no rooms in the target or // we've reached the grouping of rooms), insert our room ahead of the others in the category. // This ensures that our room is on top (more recent) than the others. - const changedBoundary = CATEGORY_ORDER.indexOf(entry.category) >= targetCatIndex; - if (changedBoundary) { - listsClone[key].push({room: room, category: category}); + const changedBoundary = entryCategory > targetCatIndex; + const currentCategory = entryCategory === targetCatIndex; + if (changedBoundary || (currentCategory && targetTs >= entryTs)) { + 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(lastCategoryBoundary, 0, {room, category}); + } else { + // If we're ordering by timestamp, just insert normally + listsClone[key].push({room, category}); + } pushedEntry = true; inserted = true; } + + if (entryCategory !== lastCategoryIndex) { + lastCategoryBoundary = listsClone[key].length - 1; + } + lastCategoryIndex = entryCategory; } // We insert our own record as needed, so don't let the old one through. From c0b63f986f8a730a422e29e421d12c42b375622b Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 13 Feb 2019 19:16:11 -0700 Subject: [PATCH 07/16] Implement a cache on _tsOfNewestEvent: ~75% improvement --- src/stores/RoomListStore.js | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index 84939fa129..f7e9703c73 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -96,7 +96,8 @@ class RoomListStore extends Store { case 'MatrixActions.Room.receipt': { if (!logicallyReady) break; - // First see if the receipt event is for our own user + // First see if the receipt event is for our own user. If it was, trigger + // a room update (we probably read the room on a different device). const myUserId = this._matrixClient.getUserId(); for (const eventId of Object.keys(payload.event.getContent())) { const receiptUsers = Object.keys(payload.event.getContent()[eventId]['m.read'] || {}); @@ -161,14 +162,6 @@ class RoomListStore extends Store { this._generateInitialRoomLists(); } break; - // TODO: Remove if not actually needed - // case 'MatrixActions.Room.accountData': { - // if (!logicallyReady) break; - // if (payload.event_type === 'm.fully_read') { - // console.log("!! Fully read: ", payload); - // } - // } - // break; case 'MatrixActions.Room.myMembership': { if (!logicallyReady) break; this._roomUpdateTriggered(payload.room.roomId); @@ -263,7 +256,6 @@ class RoomListStore extends Store { let lastCategoryBoundary = 0; let lastCategoryIndex = 0; 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 (LIST_ORDERS[key] === 'recent' && hasRoom && entry.room.roomId !== this._state.stickyRoomId) { @@ -386,11 +378,23 @@ class RoomListStore extends Store { } }); + // We use this cache in the recents comparator because _tsOfNewestEvent can take a while + const latestEventTsCache = {}; // roomId => timestamp + Object.keys(lists).forEach((listKey) => { let comparator; switch (LIST_ORDERS[listKey]) { case "recent": - comparator = this._recentsComparator; + comparator = (entryA, entryB) => { + this._recentsComparator(entryA, entryB, (room) => { + if (latestEventTsCache[room.roomId]) { + return latestEventTsCache[room.roomId]; + } + const ts = this._tsOfNewestEvent(room); + latestEventTsCache[room.roomId] = ts; + return ts; + }); + }; break; case "manual": default: @@ -444,7 +448,7 @@ class RoomListStore extends Store { return CATEGORY_IDLE; } - _recentsComparator(entryA, entryB) { + _recentsComparator(entryA, entryB, tsOfNewestEventFn) { const roomA = entryA.room; const roomB = entryB.room; const categoryA = entryA.category; @@ -458,8 +462,8 @@ class RoomListStore extends Store { return 0; } - const timestampA = this._tsOfNewestEvent(roomA); - const timestampB = this._tsOfNewestEvent(roomB); + const timestampA = tsOfNewestEventFn(roomA); + const timestampB = tsOfNewestEventFn(roomB); return timestampB - timestampA; } From b08ab6cd12912fea0ec68a86d6c8f48c45dbd40c Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 13 Feb 2019 19:35:01 -0700 Subject: [PATCH 08/16] Fix boundary math calculations --- src/stores/RoomListStore.js | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index f7e9703c73..caed059905 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -43,6 +43,7 @@ class RoomListStore extends Store { super(dis); this._init(); + this.__onDispatch = this.__onDispatch.bind(this); this._getManualComparator = this._getManualComparator.bind(this); this._recentsComparator = this._recentsComparator.bind(this); } @@ -80,7 +81,7 @@ class RoomListStore extends Store { this.__emitChange(); } - __onDispatch = (payload) => { + __onDispatch(payload) { const logicallyReady = this._matrixClient && this._state.ready; switch (payload.action) { // Initialise state after initial sync @@ -231,7 +232,7 @@ class RoomListStore extends Store { const myMembership = room.getMyMembership(); let doInsert = true; - let targetTags = []; + const targetTags = []; if (myMembership !== "join" && myMembership !== "invite") { doInsert = false; } else { @@ -253,8 +254,8 @@ class RoomListStore extends Store { listsClone[key] = []; let pushedEntry = false; const hasRoom = !!this._state.lists[key].find((e) => e.room.roomId === room.roomId); - let lastCategoryBoundary = 0; - let lastCategoryIndex = 0; + let desiredCategoryBoundaryIndex = 0; + let foundBoundary = 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 @@ -264,16 +265,21 @@ class RoomListStore extends Store { const entryTs = this._tsOfNewestEvent(entry.room); const entryCategory = CATEGORY_ORDER.indexOf(entry.category); + if (entryCategory >= targetCatIndex && !foundBoundary) { + desiredCategoryBoundaryIndex = listsClone[key].length - 1; + foundBoundary = true; + } + // If we've hit the top of a boundary (either because there's no rooms in the target or // we've reached the grouping of rooms), insert our room ahead of the others in the category. // This ensures that our room is on top (more recent) than the others. - const changedBoundary = entryCategory > targetCatIndex; + const changedBoundary = entryCategory >= targetCatIndex; const currentCategory = entryCategory === targetCatIndex; - if (changedBoundary || (currentCategory && targetTs >= entryTs)) { - if (changedBoundary) { + if (changedBoundary || (false && currentCategory && targetTs >= entryTs)) { + if (changedBoundary && false) { // If we changed a boundary, then we've gone too far - go to the top of the last // section instead. - listsClone[key].splice(lastCategoryBoundary, 0, {room, category}); + listsClone[key].splice(desiredCategoryBoundaryIndex, 0, {room, category}); } else { // If we're ordering by timestamp, just insert normally listsClone[key].push({room, category}); @@ -281,11 +287,6 @@ class RoomListStore extends Store { pushedEntry = true; inserted = true; } - - if (entryCategory !== lastCategoryIndex) { - lastCategoryBoundary = listsClone[key].length - 1; - } - lastCategoryIndex = entryCategory; } // We insert our own record as needed, so don't let the old one through. @@ -386,7 +387,7 @@ class RoomListStore extends Store { switch (LIST_ORDERS[listKey]) { case "recent": comparator = (entryA, entryB) => { - this._recentsComparator(entryA, entryB, (room) => { + return this._recentsComparator(entryA, entryB, (room) => { if (latestEventTsCache[room.roomId]) { return latestEventTsCache[room.roomId]; } From 0c7e0a264b921dbf58c6d84063fa573c6e95ba5a Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 13 Feb 2019 19:49:28 -0700 Subject: [PATCH 09/16] Inline documentation --- src/stores/RoomListStore.js | 77 ++++++++++++++++++++++++++----------- 1 file changed, 54 insertions(+), 23 deletions(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index caed059905..d2e94ffd05 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -1,5 +1,5 @@ /* -Copyright 2018 New Vector Ltd +Copyright 2018, 2019 New Vector Ltd Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -19,10 +19,22 @@ import DMRoomMap from '../utils/DMRoomMap'; import Unread from '../Unread'; import SettingsStore from "../settings/SettingsStore"; -const CATEGORY_RED = "red"; -const CATEGORY_GREY = "grey"; -const CATEGORY_BOLD = "bold"; -const CATEGORY_IDLE = "idle"; +/* +Room sorting algorithm: +* Always prefer to have red > grey > bold > idle +* The room being viewed should be sticky (not jump down to the idle list) +* When switching to a new room, sort the last sticky room to the top of the idle list. + +The approach taken by the store is to generate an initial representation of all the +tagged lists (accepting that it'll take a little bit longer to calculate) and make +small changes to that over time. This results in quick changes to the room list while +also having update operations feel more like popping/pushing to a stack. + */ + +const CATEGORY_RED = "red"; // Mentions in the room +const CATEGORY_GREY = "grey"; // Unread notified messages (not mentions) +const CATEGORY_BOLD = "bold"; // Unread messages (not notified, 'Mentions Only' rooms) +const CATEGORY_IDLE = "idle"; // Nothing of interest const CATEGORY_ORDER = [CATEGORY_RED, CATEGORY_GREY, CATEGORY_BOLD, CATEGORY_IDLE]; const LIST_ORDERS = { @@ -70,6 +82,10 @@ class RoomListStore extends Store { } _setState(newState) { + // If we're changing the lists, transparently change the presentation lists (which + // is given to requesting components). This dramatically simplifies our code elsewhere + // while also ensuring we don't need to update all the calling components to support + // categories. if (newState['lists']) { const presentationLists = {}; for (const key of Object.keys(newState['lists'])) { @@ -205,20 +221,24 @@ class RoomListStore extends Store { // Note: it is important that we set a new stickyRoomId before setting the old room // to IDLE. If we don't, the wrong room gets counted as sticky. - const currentSticky = this._state.stickyRoomId; + const currentStickyId = this._state.stickyRoomId; this._setState({stickyRoomId: payload.room_id}); - if (currentSticky) { - this._setRoomCategory(this._matrixClient.getRoom(currentSticky), CATEGORY_IDLE); + if (currentStickyId) { + this._setRoomCategory(this._matrixClient.getRoom(currentStickyId), CATEGORY_IDLE); } } break; } - }; + } _roomUpdateTriggered(roomId) { const room = this._matrixClient.getRoom(roomId); if (!room) return; + // 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 !== room.roomId) { const category = this._calculateCategory(room); this._setRoomCategory(room, category); @@ -227,8 +247,8 @@ class RoomListStore extends Store { _setRoomCategory(room, category) { const listsClone = {}; - const targetCatIndex = CATEGORY_ORDER.indexOf(category); - const targetTs = this._tsOfNewestEvent(room); + const targetCategoryIndex = CATEGORY_ORDER.indexOf(category); + const targetTimestamp = this._tsOfNewestEvent(room); const myMembership = room.getMyMembership(); let doInsert = true; @@ -247,35 +267,43 @@ class RoomListStore extends Store { // 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. + // updated room gets slotted into the right spot. This sacrifices code clarity for not + // iterating on potentially large collections multiple times. let inserted = false; for (const key of Object.keys(this._state.lists)) { listsClone[key] = []; let pushedEntry = false; const hasRoom = !!this._state.lists[key].find((e) => e.room.roomId === room.roomId); + + // 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; + 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 (LIST_ORDERS[key] === 'recent' && hasRoom && entry.room.roomId !== this._state.stickyRoomId) { const inTag = targetTags.length === 0 || targetTags.includes(key); if (!pushedEntry && doInsert && inTag) { - const entryTs = this._tsOfNewestEvent(entry.room); - const entryCategory = CATEGORY_ORDER.indexOf(entry.category); + const entryTimestamp = this._tsOfNewestEvent(entry.room); + const entryCategoryIndex = CATEGORY_ORDER.indexOf(entry.category); - if (entryCategory >= targetCatIndex && !foundBoundary) { + // 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 (either because there's no rooms in the target or - // we've reached the grouping of rooms), insert our room ahead of the others in the category. - // This ensures that our room is on top (more recent) than the others. - const changedBoundary = entryCategory >= targetCatIndex; - const currentCategory = entryCategory === targetCatIndex; - if (changedBoundary || (false && currentCategory && targetTs >= entryTs)) { + // 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 || (false && currentCategory && targetTimestamp >= entryTimestamp)) { if (changedBoundary && false) { // If we changed a boundary, then we've gone too far - go to the top of the last // section instead. @@ -317,8 +345,9 @@ class RoomListStore extends Store { } 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 >= targetCatIndex) { + if (catIdxAtPosition >= targetCategoryIndex) { listsClone[tag].splice(i, 0, {room: room, category: category}); break; } @@ -379,7 +408,9 @@ class RoomListStore extends Store { } }); - // We use this cache in the recents comparator because _tsOfNewestEvent can take a while + // We use this cache in the recents comparator because _tsOfNewestEvent can take a while. This + // cache only needs to survive the sort operation below and should not be implemented outside + // of this function, otherwise the room lists will almost certainly be out of date and wrong. const latestEventTsCache = {}; // roomId => timestamp Object.keys(lists).forEach((listKey) => { From b12362dd379f8a84cdcc4be3727c17106f9d06f8 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 13 Feb 2019 19:51:34 -0700 Subject: [PATCH 10/16] Disable dragging tests for room list We don't support dragging, so don't test it. --- test/components/views/rooms/RoomList-test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/components/views/rooms/RoomList-test.js b/test/components/views/rooms/RoomList-test.js index 0c970edb0b..754367cd23 100644 --- a/test/components/views/rooms/RoomList-test.js +++ b/test/components/views/rooms/RoomList-test.js @@ -180,7 +180,8 @@ describe('RoomList', () => { } function itDoesCorrectOptimisticUpdatesForDraggedRoomTiles() { - describe('does correct optimistic update when dragging from', () => { + // TODO: Re-enable dragging tests when we support dragging again. + xdescribe('does correct optimistic update when dragging from', () => { it('rooms to people', () => { expectCorrectMove(undefined, 'im.vector.fake.direct'); }); From a2a13636ed49e9eb1d97f63e26adc4d6dcd9570a Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 13 Feb 2019 20:02:18 -0700 Subject: [PATCH 11/16] Don't blow up when rooms have no timelines --- src/stores/RoomListStore.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index d2e94ffd05..e38a592f7a 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -450,6 +450,10 @@ class RoomListStore extends Store { } _tsOfNewestEvent(room) { + // Apparently we can have rooms without timelines, at least under testing + // environments. Just return MAX_INT when this happens. + if (!room.timeline) return Number.MAX_SAFE_INTEGER; + for (let i = room.timeline.length - 1; i >= 0; --i) { const ev = room.timeline[i]; if (this._eventTriggersRecentReorder(ev)) { From 45a415f8bf12c9d7806c190b8d07c478d14e16e0 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 13 Feb 2019 20:16:47 -0700 Subject: [PATCH 12/16] Protection around lack of room for tests --- src/stores/RoomListStore.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index e38a592f7a..9a764cd493 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -246,6 +246,8 @@ class RoomListStore extends Store { } _setRoomCategory(room, category) { + if (!room) return; // This should only happen in tests + const listsClone = {}; const targetCategoryIndex = CATEGORY_ORDER.indexOf(category); const targetTimestamp = this._tsOfNewestEvent(room); @@ -419,6 +421,8 @@ class RoomListStore extends Store { case "recent": comparator = (entryA, entryB) => { return this._recentsComparator(entryA, entryB, (room) => { + if (!room) return Number.MAX_SAFE_INTEGER; // Should only happen in tests + if (latestEventTsCache[room.roomId]) { return latestEventTsCache[room.roomId]; } @@ -452,7 +456,7 @@ class RoomListStore extends Store { _tsOfNewestEvent(room) { // Apparently we can have rooms without timelines, at least under testing // environments. Just return MAX_INT when this happens. - if (!room.timeline) return Number.MAX_SAFE_INTEGER; + if (!room || !room.timeline) return Number.MAX_SAFE_INTEGER; for (let i = room.timeline.length - 1; i >= 0; --i) { const ev = room.timeline[i]; From 53fa59f5a4fc068f9a5b79e3e57b8ccac3a8e7de Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 15 Feb 2019 23:40:23 -0700 Subject: [PATCH 13/16] Remove old debugging code The algorithm is correctly applied when these are removed. --- src/stores/RoomListStore.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index 9a764cd493..b998476cc5 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -276,7 +276,7 @@ class RoomListStore extends Store { for (const key of Object.keys(this._state.lists)) { listsClone[key] = []; let pushedEntry = false; - const hasRoom = !!this._state.lists[key].find((e) => e.room.roomId === room.roomId); + const hasRoom = this._state.lists[key].some((e) => e.room.roomId === room.roomId); // 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 @@ -305,8 +305,8 @@ class RoomListStore extends Store { // based on most recent timestamp. const changedBoundary = entryCategoryIndex > targetCategoryIndex; const currentCategory = entryCategoryIndex === targetCategoryIndex; - if (changedBoundary || (false && currentCategory && targetTimestamp >= entryTimestamp)) { - if (changedBoundary && false) { + 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}); From cb15bc968ccffcc3316869824fa9e80f8a17961d Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 15 Feb 2019 23:41:48 -0700 Subject: [PATCH 14/16] Remove excessive dispatch binding --- src/stores/RoomListStore.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index b998476cc5..deac557fbe 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -55,7 +55,6 @@ class RoomListStore extends Store { super(dis); this._init(); - this.__onDispatch = this.__onDispatch.bind(this); this._getManualComparator = this._getManualComparator.bind(this); this._recentsComparator = this._recentsComparator.bind(this); } From 561d1f37ec076b49ed8f1094a506ad2a89c0098c Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 19 Feb 2019 14:56:56 -0700 Subject: [PATCH 15/16] Stick a couple micro optimizations into the setRoomCategory hot path --- src/stores/RoomListStore.js | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index deac557fbe..81b1bb621f 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -231,14 +231,15 @@ class RoomListStore extends Store { } _roomUpdateTriggered(roomId) { - const room = this._matrixClient.getRoom(roomId); - if (!room) return; - // 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 !== room.roomId) { + if (this._state.stickyRoomId !== roomId) { + // Micro optimization: Only look up the room if we're confident we'll need it. + const room = this._matrixClient.getRoom(roomId); + if (!room) return; + const category = this._calculateCategory(room); this._setRoomCategory(room, category); } @@ -249,7 +250,15 @@ class RoomListStore extends Store { const listsClone = {}; const targetCategoryIndex = CATEGORY_ORDER.indexOf(category); - const targetTimestamp = this._tsOfNewestEvent(room); + + // Micro optimization: Support lazily loading the last timestamp in a room + let _targetTimestamp = null; + const targetTimestamp = () => { + if (_targetTimestamp === null) { + _targetTimestamp = this._tsOfNewestEvent(room); + } + return _targetTimestamp; + }; const myMembership = room.getMyMembership(); let doInsert = true; @@ -290,7 +299,15 @@ class RoomListStore extends Store { if (LIST_ORDERS[key] === 'recent' && hasRoom && entry.room.roomId !== this._state.stickyRoomId) { const inTag = targetTags.length === 0 || targetTags.includes(key); if (!pushedEntry && doInsert && inTag) { - const entryTimestamp = this._tsOfNewestEvent(entry.room); + // 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. @@ -304,7 +321,7 @@ class RoomListStore extends Store { // based on most recent timestamp. const changedBoundary = entryCategoryIndex > targetCategoryIndex; const currentCategory = entryCategoryIndex === targetCategoryIndex; - if (changedBoundary || (currentCategory && targetTimestamp >= entryTimestamp)) { + 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. From 64103b7af4986138bf68d0a697b2117422f09173 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 19 Feb 2019 15:15:39 -0700 Subject: [PATCH 16/16] More micro optimizations to make the hot paths faster --- src/stores/RoomListStore.js | 46 ++++++++++++++++++++++++++----------- 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index 81b1bb621f..47480aef85 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -282,23 +282,29 @@ class RoomListStore extends Store { let inserted = false; for (const key of Object.keys(this._state.lists)) { - listsClone[key] = []; - let pushedEntry = false; const hasRoom = this._state.lists[key].some((e) => e.room.roomId === room.roomId); + // 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]; + continue; + } 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]) { // 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 (LIST_ORDERS[key] === 'recent' && hasRoom && entry.room.roomId !== this._state.stickyRoomId) { - const inTag = targetTags.length === 0 || targetTags.includes(key); - if (!pushedEntry && doInsert && inTag) { + 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 = () => { @@ -356,7 +362,7 @@ class RoomListStore extends Store { tags = targetTags; } if (tags.length === 0) { - tags.push(myMembership === 'join' ? 'im.vector.fake.recent' : 'im.vector.fake.invite'); + tags = [myMembership === 'join' ? 'im.vector.fake.recent' : 'im.vector.fake.invite']; } } else { tags = ['im.vector.fake.archived']; @@ -388,7 +394,15 @@ class RoomListStore extends Store { }; const dmRoomMap = DMRoomMap.shared(); - const isCustomTagsEnabled = SettingsStore.isFeatureEnabled("feature_custom_tags"); + + // 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(); @@ -403,7 +417,9 @@ class RoomListStore extends Store { // ignore any m. tag names we don't know about tagNames = tagNames.filter((t) => { - return (isCustomTagsEnabled && !t.startsWith('m.')) || lists[t] !== undefined; + // Speed optimization: Avoid hitting the SettingsStore at all costs by making it the + // last condition possible. + return lists[t] !== undefined || (!t.startsWith('m.') && isCustomTagsEnabled()); }); if (tagNames.length) { @@ -411,9 +427,10 @@ class RoomListStore extends Store { const tagName = tagNames[i]; lists[tagName] = lists[tagName] || []; - // We categorize all the tagged rooms the same because we don't actually - // care about the order (it's defined elsewhere) - lists[tagName].push({room, category: CATEGORY_RED}); + // Default to an arbitrary category for tags which aren't ordered by recents + let category = CATEGORY_IDLE; + if (LIST_ORDERS[tagName] === 'recent') category = this._calculateCategory(room); + lists[tagName].push({room, category: category}); } } else if (dmRoomMap.getUserIdForRoomId(room.roomId)) { // "Direct Message" rooms (that we're still in and that aren't otherwise tagged) @@ -422,7 +439,9 @@ class RoomListStore extends Store { lists["im.vector.fake.recent"].push({room, category: this._calculateCategory(room)}); } } else if (membership === "leave") { - lists["im.vector.fake.archived"].push({room, category: this._calculateCategory(room)}); + // The category of these rooms is not super important, so deprioritize it to the lowest + // possible value. + lists["im.vector.fake.archived"].push({room, category: CATEGORY_IDLE}); } }); @@ -442,6 +461,7 @@ class RoomListStore extends Store { if (latestEventTsCache[room.roomId]) { return latestEventTsCache[room.roomId]; } + const ts = this._tsOfNewestEvent(room); latestEventTsCache[room.roomId] = ts; return ts; @@ -515,7 +535,7 @@ class RoomListStore extends Store { const idxB = CATEGORY_ORDER.indexOf(categoryB); if (idxA > idxB) return 1; if (idxA < idxB) return -1; - return 0; + return 0; // Technically not possible } const timestampA = tsOfNewestEventFn(roomA);