Merge pull request #2634 from matrix-org/travis/better-room-sorting

"Breadcrumb" room sorting algorithm

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);

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 {
                     <span className="mx_SettingsTab_subheading">{_t("Composer")}</span>
                     {this._renderGroup(PreferencesSettingsTab.COMPOSER_SETTINGS)}
 
-                    <span className="mx_SettingsTab_subheading">{_t("Room list")}</span>
-                    {this._renderGroup(PreferencesSettingsTab.ROOM_LIST_SETTINGS)}
-
                     <span className="mx_SettingsTab_subheading">{_t("Timeline")}</span>
                     {this._renderGroup(PreferencesSettingsTab.TIMELINE_SETTINGS)}
 

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",
@@ -552,7 +550,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",

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: true,
-    },
-    "pinUnreadRooms": {
-        supportedLevels: LEVELS_ACCOUNT_SETTINGS,
-        displayName: _td("Pin unread rooms to the top of the room list"),
-        default: true,
-    },
     "enableWidgetScreenshots": {
         supportedLevels: LEVELS_ACCOUNT_SETTINGS,
         displayName: _td('Enable widget screenshots on supported widgets'),

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,20 +19,38 @@ import DMRoomMap from '../utils/DMRoomMap';
 import Unread from '../Unread';
 import SettingsStore from "../settings/SettingsStore";
 
+/*
+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 = {
+    "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 +61,43 @@ 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: {
+            // The rooms in these arrays are ordered according to either the
-                "m.server_notice": [],
+            // 'recents' behaviour or 'manual' behaviour.
-                "im.vector.fake.invite": [],
+            lists: defaultLists,
-                "m.favourite": [],
+            presentationLists: defaultLists, // like `lists`, but with arrays of rooms instead
-                "im.vector.fake.recent": [],
-                "im.vector.fake.direct": [],
-                "m.lowpriority": [],
-                "im.vector.fake.archived": [],
-            },
             ready: false,
-
+            stickyRoomId: null,
-            // 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: {},
         };
     }
 
     _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'])) {
+                presentationLists[key] = newState['lists'][key].map((e) => e.room);
+            }
+            newState['presentationLists'] = presentationLists;
+        }
         this._state = Object.assign(this._state, newState);
         this.__emitChange();
     }
 
     __onDispatch(payload) {
+        const logicallyReady = this._matrixClient && this._state.ready;
         switch (payload.action) {
             // Initialise state after initial sync
             case 'MatrixActions.sync': {
@@ -89,30 +106,47 @@ class RoomListStore extends Store {
                 }
 
                 this._matrixClient = payload.matrixClient;
-                this._generateRoomLists();
+                this._generateInitialRoomLists();
+            }
+            break;
+            case 'MatrixActions.Room.receipt': {
+                if (!logicallyReady) break;
+
+                // 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'] || {});
+                    if (receiptUsers.includes(myUserId)) {
+                        this._roomUpdateTriggered(payload.room.roomId);
+                        return;
+                    }
+                }
             }
             break;
             case 'MatrixActions.Room.tags': {
-                if (!this._state.ready) break;
+                if (!logicallyReady) break;
-                this._generateRoomLists();
+                // 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': {
-                if (!this._state.ready ||
+                if (!logicallyReady ||
                     !payload.isLiveEvent ||
                     !payload.isLiveUnfilteredRoomTimelineEvent ||
                     !this._eventTriggersRecentReorder(payload.event)
-                ) break;
+                ) {
+                    break;
+                }
 
-                this._clearCachedRoomState(payload.event.getRoomId());
+                this._roomUpdateTriggered(payload.event.getRoomId());
-                this._generateRoomLists();
             }
             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 (!logicallyReady) break;
-                if (!this._matrixClient || !this._state.ready) break;
 
                 const roomId = payload.event.getRoomId();
 
@@ -129,52 +163,51 @@ 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 ||
+                if (liveTimeline !== eventTimeline || !this._eventTriggersRecentReorder(payload.event)) {
-                    !this._eventTriggersRecentReorder(payload.event)
+                    break;
-                ) break;
+                }
 
-                this._clearCachedRoomState(payload.event.getRoomId());
+                this._roomUpdateTriggered(roomId);
-                this._generateRoomLists();
             }
             break;
             case 'MatrixActions.accountData': {
+                if (!logicallyReady) break;
                 if (payload.event_type !== 'm.direct') break;
-                this._generateRoomLists();
+                // 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
-            break;
+                this._generateInitialRoomLists();
-            case 'MatrixActions.Room.accountData': {
-                if (payload.event_type === 'm.fully_read') {
-                    this._clearCachedRoomState(payload.room.roomId);
-                    this._generateRoomLists();
-                }
             }
             break;
             case 'MatrixActions.Room.myMembership': {
-                this._generateRoomLists();
+                if (!logicallyReady) break;
+                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;
+                if (!logicallyReady) break;
-                this._generateRoomLists();
+                this._roomUpdateTriggered(payload.room.roomId);
-            }
-            break;
-            case 'RoomListActions.tagRoom.pending': {
-                // 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);
-            }
-            break;
-            case 'RoomListActions.tagRoom.failure': {
-                // Reset state according to js-sdk
-                this._generateRoomLists();
             }
             break;
+            // 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
                 // the matrix client isn't `null` and so causing a re-render will cause NPEs.
@@ -182,10 +215,174 @@ 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 currentStickyId = this._state.stickyRoomId;
+                this._setState({stickyRoomId: payload.room_id});
+                if (currentStickyId) {
+                    this._setRoomCategory(this._matrixClient.getRoom(currentStickyId), CATEGORY_IDLE);
+                }
+            }
+            break;
         }
     }
 
-    _generateRoomLists(optimisticRequest) {
+    _roomUpdateTriggered(roomId) {
+        // 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) {
+            // 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);
+        }
+    }
+
+    _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);
+            }
+            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');
+            }
+        }
+
+        // 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.
+
+        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);
+
+            // 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 (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;
+                        }
+
+                        // 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;
+                            inserted = 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);
+            }
+        }
+
+        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'];
+            }
+            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;
+                    }
+                }
+            }
+        }
+
+        this._setState({lists: listsClone});
+    }
+
+    _generateInitialRoomLists() {
         const lists = {
             "m.server_notice": [],
             "im.vector.fake.invite": [],
@@ -196,74 +393,84 @@ class RoomListStore extends Store {
             "im.vector.fake.archived": [],
         };
 
-
         const dmRoomMap = DMRoomMap.shared();
 
-        // If somehow we dispatched a RoomListActions.tagRoom.failure before a MatrixActions.sync
+        // Speed optimization: Hitting the SettingsStore is expensive, so avoid that at all costs.
-        if (!this._matrixClient) return;
+        let _isCustomTagsEnabled = null;
+        const isCustomTagsEnabled = () => {
+            if (_isCustomTagsEnabled === null) {
+                _isCustomTagsEnabled = SettingsStore.isFeatureEnabled("feature_custom_tags");
+            }
+            return _isCustomTagsEnabled;
+        };
 
-        const isCustomTagsEnabled = SettingsStore.isFeatureEnabled("feature_custom_tags");
+        this._matrixClient.getRooms().forEach((room) => {
-
-        this._matrixClient.getRooms().forEach((room, index) => {
             const myUserId = this._matrixClient.getUserId();
             const membership = room.getMyMembership();
             const me = room.getMember(myUserId);
 
-            if (membership == "invite") {
+            if (membership === "invite") {
-                lists["im.vector.fake.invite"].push(room);
+                lists["im.vector.fake.invite"].push({room, category: CATEGORY_RED});
-            } else if (membership == "join" || membership === "ban" || (me && me.isKicked())) {
+            } 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;
+                    // 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) {
                     for (let i = 0; i < tagNames.length; i++) {
                         const tagName = tagNames[i];
                         lists[tagName] = lists[tagName] || [];
-                        lists[tagName].push(room);
+
+                        // 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)
-                    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);
+                // 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});
             }
         });
 
-        // Note: we check the settings up here instead of in the forEach or
+        // We use this cache in the recents comparator because _tsOfNewestEvent can take a while. This
-        // in the _recentsComparator to avoid hitting the SettingsStore a few
+        // cache only needs to survive the sort operation below and should not be implemented outside
-        // thousand times.
+        // of this function, otherwise the room lists will almost certainly be out of date and wrong.
-        const pinUnread = SettingsStore.getValue("pinUnreadRooms");
+        const latestEventTsCache = {}; // roomId => timestamp
-        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) => {
+                    comparator = (entryA, entryB) => {
-                        return this._recentsComparator(roomA, roomB, pinUnread, pinMentioned);
+                        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];
+                            }
+
+                            const ts = this._tsOfNewestEvent(room);
+                            latestEventTsCache[room.roomId] = ts;
+                            return ts;
+                        });
                     };
                     break;
                 case "manual":
                 default:
-                    comparator = this._getManualComparator(listKey, optimisticRequest);
+                    comparator = this._getManualComparator(listKey);
                     break;
             }
             lists[listKey].sort(comparator);
@@ -271,52 +478,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) ||
@@ -325,6 +490,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 || !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)) {
@@ -342,53 +511,36 @@ class RoomListStore extends Store {
         }
     }
 
-    _recentsComparator(roomA, roomB, pinUnread, pinMentioned) {
+    _calculateCategory(room) {
-        // We try and set the ordering to be Mentioned > Unread > Recent
+        const mentions = room.getUnreadNotificationCount("highlight") > 0;
-        // assuming the user has the right settings, of course.
+        if (mentions) return CATEGORY_RED;
 
-        const timestampA = this._getRoomState(roomA, "timestamp");
+        let unread = room.getUnreadNotificationCount() > 0;
-        const timestampB = this._getRoomState(roomB, "timestamp");
+        if (unread) return CATEGORY_GREY;
-        const timestampDiff = timestampB - timestampA;
 
-        if (pinMentioned) {
+        unread = Unread.doesRoomHaveUnreadMessages(room);
-            const mentionsA = this._getRoomState(roomA, "notifications");
+        if (unread) return CATEGORY_BOLD;
-            const mentionsB = this._getRoomState(roomB, "notifications");
-            if (mentionsA && !mentionsB) return -1;
-            if (!mentionsA && mentionsB) return 1;
 
-            // If they both have notifications, sort by timestamp.
+        return CATEGORY_IDLE;
-            // If neither have notifications (the fourth check not shown
+    }
-            // here), then try and sort by unread messages and finally by
+
-            // timestamp.
+    _recentsComparator(entryA, entryB, tsOfNewestEventFn) {
-            if (mentionsA && mentionsB) return timestampDiff;
+        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; // Technically not possible
         }
 
-        if (pinUnread) {
+        const timestampA = tsOfNewestEventFn(roomA);
-            let unreadA = this._getRoomState(roomA, "unread");
+        const timestampB = tsOfNewestEventFn(roomB);
-            let unreadB = this._getRoomState(roomB, "unread");
+        return timestampB - timestampA;
-            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;
     }
 
     _lexicographicalComparator(roomA, roomB) {
@@ -396,7 +548,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 +559,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 a = metaA ? Number(metaA.order) : undefined;
-            const b = metaB ? metaB.order : undefined;
+            const b = metaB ? Number(metaB.order) : undefined;
 
             // Order undefined room tag orders to the bottom
             if (a === undefined && b !== undefined) {
@@ -414,12 +569,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;
     }
 }
 

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');
             });