Merge pull request #5070 from matrix-org/travis/room-list/regressions

Fix various small regressions in the room list's behaviour

res/css/structures/_CustomRoomTagPanel.scss

@@ -54,5 +54,5 @@ limitations under the License.
     position: absolute;
     left: -9px;
     border-radius: 0 3px 3px 0;
-    top: 12px; // just feels right (see comment above about designs needing to be updated)
+    top: 5px; // just feels right (see comment above about designs needing to be updated)
 }

src/components/views/rooms/RoomList.tsx

@@ -42,7 +42,7 @@ import { RoomNotificationStateStore } from "../../../stores/notifications/RoomNo
 import SettingsStore from "../../../settings/SettingsStore";
 import CustomRoomTagStore from "../../../stores/CustomRoomTagStore";
 import { arrayFastClone, arrayHasDiff } from "../../../utils/arrays";
-import { objectShallowClone } from "../../../utils/objects";
+import { objectShallowClone, objectWithOnly } from "../../../utils/objects";
 
 interface IProps {
     onKeyDown: (ev: React.KeyboardEvent) => void;
@@ -220,7 +220,12 @@ export default class RoomList extends React.PureComponent<IProps, IState> {
         }
 
         const previousListIds = Object.keys(this.state.sublists);
-        const newListIds = Object.keys(newLists);
+        const newListIds = Object.keys(newLists).filter(t => {
+            if (!isCustomTag(t)) return true; // always include non-custom tags
+
+            // if the tag is custom though, only include it if it is enabled
+            return CustomRoomTagStore.getTags()[t];
+        });
 
         let doUpdate = arrayHasDiff(previousListIds, newListIds);
         if (!doUpdate) {
@@ -240,7 +245,8 @@ export default class RoomList extends React.PureComponent<IProps, IState> {
         if (doUpdate) {
             // We have to break our reference to the room list store if we want to be able to
             // diff the object for changes, so do that.
-            const sublists = objectShallowClone(newLists, (k, v) => arrayFastClone(v));
+            const newSublists = objectWithOnly(newLists, newListIds);
+            const sublists = objectShallowClone(newSublists, (k, v) => arrayFastClone(v));
 
             this.setState({sublists}, () => {
                 this.props.onResize();
@@ -288,8 +294,7 @@ export default class RoomList extends React.PureComponent<IProps, IState> {
         const tagOrder = TAG_ORDER.reduce((p, c) => {
             if (c === CUSTOM_TAGS_BEFORE_TAG) {
                 const customTags = Object.keys(this.state.sublists)
-                    .filter(t => isCustomTag(t))
+                    .filter(t => isCustomTag(t));
-                    .filter(t => CustomRoomTagStore.getTags()[t]); // isSelected
                 p.push(...customTags);
             }
             p.push(c);

src/components/views/rooms/RoomSublist.tsx

@@ -46,8 +46,8 @@ import { Direction } from "re-resizable/lib/resizer";
 import { polyfillTouchEvent } from "../../../@types/polyfill";
 import { RoomNotificationStateStore } from "../../../stores/notifications/RoomNotificationStateStore";
 import RoomListLayoutStore from "../../../stores/room-list/RoomListLayoutStore";
-import { arrayHasOrderChange } from "../../../utils/arrays";
+import { arrayFastClone, arrayHasOrderChange } from "../../../utils/arrays";
-import { objectExcluding, objectHasValueChange } from "../../../utils/objects";
+import { objectExcluding, objectHasDiff } from "../../../utils/objects";
 import TemporaryTile from "./TemporaryTile";
 import { ListNotificationState } from "../../../stores/notifications/ListNotificationState";
 
@@ -115,7 +115,7 @@ export default class RoomSublist extends React.Component<IProps, IState> {
             isResizing: false,
             isExpanded: this.isBeingFiltered ? this.isBeingFiltered : !this.layout.isCollapsed,
             height: 0, // to be fixed in a moment, we need `rooms` to calculate this.
-            rooms: RoomListStore.instance.orderedLists[this.props.tagId] || [],
+            rooms: arrayFastClone(RoomListStore.instance.orderedLists[this.props.tagId] || []),
         };
         // Why Object.assign() and not this.state.height? Because TypeScript says no.
         this.state = Object.assign(this.state, {height: this.calculateInitialHeight()});
@@ -181,7 +181,7 @@ export default class RoomSublist extends React.Component<IProps, IState> {
     }
 
     public shouldComponentUpdate(nextProps: Readonly<IProps>, nextState: Readonly<IState>): boolean {
-        if (objectHasValueChange(this.props, nextProps)) {
+        if (objectHasDiff(this.props, nextProps)) {
             // Something we don't care to optimize has updated, so update.
             return true;
         }
@@ -189,7 +189,7 @@ export default class RoomSublist extends React.Component<IProps, IState> {
         // Do the same check used on props for state, without the rooms we're going to no-op
         const prevStateNoRooms = objectExcluding(this.state, ['rooms']);
         const nextStateNoRooms = objectExcluding(nextState, ['rooms']);
-        if (objectHasValueChange(prevStateNoRooms, nextStateNoRooms)) {
+        if (objectHasDiff(prevStateNoRooms, nextStateNoRooms)) {
             return true;
         }
 
@@ -255,7 +255,7 @@ export default class RoomSublist extends React.Component<IProps, IState> {
         }
 
         const currentRooms = this.state.rooms;
-        const newRooms = RoomListStore.instance.orderedLists[this.props.tagId] || [];
+        const newRooms = arrayFastClone(RoomListStore.instance.orderedLists[this.props.tagId] || []);
         if (arrayHasOrderChange(currentRooms, newRooms)) {
             stateUpdates.rooms = newRooms;
         }

src/stores/notifications/RoomNotificationState.ts

@@ -31,6 +31,7 @@ export class RoomNotificationState extends NotificationState implements IDestroy
         this.room.on("Room.receipt", this.handleReadReceipt);
         this.room.on("Room.timeline", this.handleRoomEventUpdate);
         this.room.on("Room.redaction", this.handleRoomEventUpdate);
+        this.room.on("Room.myMembership", this.handleMembershipUpdate);
         MatrixClientPeg.get().on("Event.decrypted", this.handleRoomEventUpdate);
         MatrixClientPeg.get().on("accountData", this.handleAccountDataUpdate);
         this.updateNotificationState();
@@ -45,6 +46,7 @@ export class RoomNotificationState extends NotificationState implements IDestroy
         this.room.removeListener("Room.receipt", this.handleReadReceipt);
         this.room.removeListener("Room.timeline", this.handleRoomEventUpdate);
         this.room.removeListener("Room.redaction", this.handleRoomEventUpdate);
+        this.room.removeListener("Room.myMembership", this.handleMembershipUpdate);
         if (MatrixClientPeg.get()) {
             MatrixClientPeg.get().removeListener("Event.decrypted", this.handleRoomEventUpdate);
             MatrixClientPeg.get().removeListener("accountData", this.handleAccountDataUpdate);
@@ -57,6 +59,10 @@ export class RoomNotificationState extends NotificationState implements IDestroy
         this.updateNotificationState();
     };
 
+    private handleMembershipUpdate = () => {
+        this.updateNotificationState();
+    };
+
     private handleRoomEventUpdate = (event: MatrixEvent) => {
         const roomId = event.getRoomId();
 

src/stores/room-list/algorithms/Algorithm.ts

@@ -715,7 +715,9 @@ export class Algorithm extends EventEmitter {
                     const algorithm: OrderingAlgorithm = this.algorithms[rmTag];
                     if (!algorithm) throw new Error(`No algorithm for ${rmTag}`);
                     await algorithm.handleRoomUpdate(room, RoomUpdateCause.RoomRemoved);
-                    this.cachedRooms[rmTag] = algorithm.orderedRooms;
+                    this._cachedRooms[rmTag] = algorithm.orderedRooms;
+                    this.recalculateFilteredRoomsForTag(rmTag); // update filter to re-sort the list
+                    this.recalculateStickyRoom(rmTag); // update sticky room to make sure it moves if needed
                 }
                 for (const addTag of diff.added) {
                     if (SettingsStore.getValue("advancedRoomListLogging")) {
@@ -725,7 +727,7 @@ export class Algorithm extends EventEmitter {
                     const algorithm: OrderingAlgorithm = this.algorithms[addTag];
                     if (!algorithm) throw new Error(`No algorithm for ${addTag}`);
                     await algorithm.handleRoomUpdate(room, RoomUpdateCause.NewRoom);
-                    this.cachedRooms[addTag] = algorithm.orderedRooms;
+                    this._cachedRooms[addTag] = algorithm.orderedRooms;
                 }
 
                 // Update the tag map so we don't regen it in a moment
@@ -821,7 +823,7 @@ export class Algorithm extends EventEmitter {
             if (!algorithm) throw new Error(`No algorithm for ${tag}`);
 
             await algorithm.handleRoomUpdate(room, cause);
-            this.cachedRooms[tag] = algorithm.orderedRooms;
+            this._cachedRooms[tag] = algorithm.orderedRooms;
 
             // Flag that we've done something
             this.recalculateFilteredRoomsForTag(tag); // update filter to re-sort the list

src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts

@@ -136,6 +136,9 @@ export class ImportanceAlgorithm extends OrderingAlgorithm {
         } else {
             throw new Error(`Unhandled splice: ${cause}`);
         }
+
+        // changes have been made if we made it here, so say so
+        return true;
     }
 
     public async handleRoomUpdate(room: Room, cause: RoomUpdateCause): Promise<boolean> {

src/utils/objects.ts

@@ -36,6 +36,23 @@ export function objectExcluding(a: any, props: string[]): any {
     }, {});
 }
 
+/**
+ * Gets a new object which represents the provided object, with only some properties
+ * included.
+ * @param a The object to clone properties of. Must be defined.
+ * @param props The property names to keep.
+ * @returns The new object with only the provided properties.
+ */
+export function objectWithOnly(a: any, props: string[]): any {
+    const existingProps = Object.keys(a);
+    const diff = arrayDiff(existingProps, props);
+    if (diff.removed.length === 0) {
+        return objectShallowClone(a);
+    } else {
+        return objectExcluding(a, diff.removed);
+    }
+}
+
 /**
  * Clones an object to a caller-controlled depth. When a propertyCloner is supplied, the
  * object's properties will be passed through it with the return value used as the new
@@ -58,20 +75,6 @@ export function objectShallowClone(a: any, propertyCloner?: (k: string, v: any)
     return newObj;
 }
 
-/**
- * Determines if the two objects, which are assumed to be of the same
- * key shape, have a difference in their values. If a difference is
- * determined, true is returned.
- * @param a The first object. Must be defined.
- * @param b The second object. Must be defined.
- * @returns True if there's a perceptual difference in the object's values.
- */
-export function objectHasValueChange(a: any, b: any): boolean {
-    const aValues = Object.values(a);
-    const bValues = Object.values(b);
-    return arrayHasDiff(aValues, bValues);
-}
-
 /**
  * Determines if any keys were added, removed, or changed between two objects.
  * For changes, simple triple equal comparisons are done, not in-depth