Fix home vs all rooms requiring app reload and change default to home

Consolidate ALL_ROOMS and HOME_SPACE storage
Fix behaviour when recalled room is no longer part of the target space
Improve tests

src/components/views/spaces/SpacePanel.tsx

@@ -137,15 +137,22 @@ const InnerSpacePanel = React.memo<IInnerSpacePanelProps>(({ children, isPanelCo
     const [invites, spaces, activeSpace] = useSpaces();
     const activeSpaces = activeSpace ? [activeSpace] : [];
 
-    const homeNotificationState = SpaceStore.spacesTweakAllRoomsEnabled
+    let homeTooltip: string;
-        ? RoomNotificationStateStore.instance.globalState : SpaceStore.instance.getNotificationState(HOME_SPACE);
+    let homeNotificationState: NotificationState;
+    if (SpaceStore.instance.allRoomsInHome) {
+        homeTooltip = _t("All rooms");
+        homeNotificationState = RoomNotificationStateStore.instance.globalState;
+    } else {
+        homeTooltip = _t("Home");
+        homeNotificationState = SpaceStore.instance.getNotificationState(HOME_SPACE);
+    }
 
     return <div className="mx_SpaceTreeLevel">
         <SpaceButton
             className="mx_SpaceButton_home"
             onClick={() => SpaceStore.instance.setActiveSpace(null)}
             selected={!activeSpace}
-            tooltip={SpaceStore.spacesTweakAllRoomsEnabled ? _t("All rooms") : _t("Home")}
+            tooltip={homeTooltip}
             notificationState={homeNotificationState}
             isNarrow={isPanelCollapsed}
         />

src/settings/Settings.tsx

@@ -187,8 +187,7 @@ export const SETTINGS: {[setting: string]: ISetting} = {
     "feature_spaces.all_rooms": {
         displayName: _td("Show all rooms in Home"),
         supportedLevels: LEVELS_FEATURE,
-        default: true,
+        default: false,
-        controller: new ReloadOnChangeController(),
     },
     "feature_dnd": {
         isFeature: true,

src/stores/SpaceStore.tsx

@@ -37,9 +37,8 @@ import { EnhancedMap, mapDiff } from "../utils/maps";
 import { setHasDiff } from "../utils/sets";
 import RoomViewStore from "./RoomViewStore";
 import { Action } from "../dispatcher/actions";
-import { arrayHasDiff } from "../utils/arrays";
+import { arrayHasDiff, arrayHasOrderChange } from "../utils/arrays";
 import { objectDiff } from "../utils/objects";
-import { arrayHasOrderChange } from "../utils/arrays";
 import { reorderLexicographically } from "../utils/stringOrderField";
 import { TAG_ORDER } from "../components/views/rooms/RoomList";
 import { shouldShowSpaceSettings } from "../utils/space";
@@ -48,6 +47,7 @@ import { _t } from "../languageHandler";
 import GenericToast from "../components/views/toasts/GenericToast";
 import Modal from "../Modal";
 import InfoDialog from "../components/views/dialogs/InfoDialog";
+import { SettingUpdatedPayload } from "../dispatcher/payloads/SettingUpdatedPayload";
 
 type SpaceKey = string | symbol;
 
@@ -61,6 +61,7 @@ export const SUGGESTED_ROOMS = Symbol("suggested-rooms");
 export const UPDATE_TOP_LEVEL_SPACES = Symbol("top-level-spaces");
 export const UPDATE_INVITED_SPACES = Symbol("invited-spaces");
 export const UPDATE_SELECTED_SPACE = Symbol("selected-space");
+export const UPDATE_HOME_BEHAVIOUR = Symbol("home-behaviour");
 // Space Room ID/HOME_SPACE will be emitted when a Space's children change
 
 export interface ISuggestedRoom extends ISpaceSummaryRoom {
@@ -69,12 +70,10 @@ export interface ISuggestedRoom extends ISpaceSummaryRoom {
 
 const MAX_SUGGESTED_ROOMS = 20;
 
-// All of these settings cause the page to reload and can be costly if read frequently, so read them here only
+// This setting causes the page to reload and can be costly if read frequently, so read it here only
 const spacesEnabled = SettingsStore.getValue("feature_spaces");
-const spacesTweakAllRoomsEnabled = SettingsStore.getValue("feature_spaces.all_rooms");
 
-const homeSpaceKey = spacesTweakAllRoomsEnabled ? "ALL_ROOMS" : "HOME_SPACE";
+const getSpaceContextKey = (space?: Room) => `mx_space_context_${space?.roomId || "HOME_SPACE"}`;
-const getSpaceContextKey = (space?: Room) => `mx_space_context_${space?.roomId || homeSpaceKey}`;
 
 const partitionSpacesAndRooms = (arr: Room[]): [Room[], Room[]] => { // [spaces, rooms]
     return arr.reduce((result, room: Room) => {
@@ -102,10 +101,6 @@ const getRoomFn: FetchRoomFn = (room: Room) => {
 };
 
 export class SpaceStoreClass extends AsyncStoreWithClient<IState> {
-    constructor() {
-        super(defaultDispatcher, {});
-    }
-
     // The spaces representing the roots of the various tree-like hierarchies
     private rootSpaces: Room[] = [];
     // The list of rooms not present in any currently joined spaces
@@ -122,6 +117,13 @@ export class SpaceStoreClass extends AsyncStoreWithClient<IState> {
     private _invitedSpaces = new Set<Room>();
     private spaceOrderLocalEchoMap = new Map<string, string>();
     private _restrictedJoinRuleSupport?: IRoomCapability;
+    private _allRoomsInHome: boolean = SettingsStore.getValue("feature_spaces.all_rooms");
+
+    constructor() {
+        super(defaultDispatcher, {});
+
+        SettingsStore.monitorSetting("feature_spaces.all_rooms", null);
+    }
 
     public get invitedSpaces(): Room[] {
         return Array.from(this._invitedSpaces);
@@ -139,13 +141,16 @@ export class SpaceStoreClass extends AsyncStoreWithClient<IState> {
         return this._suggestedRooms;
     }
 
+    public get allRoomsInHome(): boolean {
+        return this._allRoomsInHome;
+    }
+
     public async setActiveRoomInSpace(space: Room | null): Promise<void> {
         if (space && !space.isSpaceRoom()) return;
         if (space !== this.activeSpace) await this.setActiveSpace(space);
 
         if (space) {
-            const notificationState = this.getNotificationState(space.roomId);
+            const roomId = this.getNotificationState(space.roomId).getFirstRoomWithNotifications();
-            const roomId = notificationState.getFirstRoomWithNotifications();
             defaultDispatcher.dispatch({
                 action: "view_room",
                 room_id: roomId,
@@ -200,7 +205,8 @@ export class SpaceStoreClass extends AsyncStoreWithClient<IState> {
             // else if the last viewed room in this space is joined then view that
             // else view space home or home depending on what is being clicked on
             if (space?.getMyMembership() !== "invite" &&
-                this.matrixClient?.getRoom(roomId)?.getMyMembership() === "join"
+                this.matrixClient?.getRoom(roomId)?.getMyMembership() === "join" &&
+                this.getSpaceFilteredRoomIds(space).has(roomId)
             ) {
                 defaultDispatcher.dispatch({
                     action: "view_room",
@@ -377,7 +383,7 @@ export class SpaceStoreClass extends AsyncStoreWithClient<IState> {
     }
 
     public getSpaceFilteredRoomIds = (space: Room | null): Set<string> => {
-        if (!space && spacesTweakAllRoomsEnabled) {
+        if (!space && this.allRoomsInHome) {
             return new Set(this.matrixClient.getVisibleRooms().map(r => r.roomId));
         }
         return this.spaceFilteredRooms.get(space?.roomId || HOME_SPACE) || new Set();
@@ -474,7 +480,7 @@ export class SpaceStoreClass extends AsyncStoreWithClient<IState> {
     };
 
     private showInHomeSpace = (room: Room) => {
-        if (spacesTweakAllRoomsEnabled) return true;
+        if (this.allRoomsInHome) return true;
         if (room.isSpaceRoom()) return false;
         return !this.parentMap.get(room.roomId)?.size // put all orphaned rooms in the Home Space
             || DMRoomMap.shared().getUserIdForRoomId(room.roomId) // put all DMs in the Home Space
@@ -506,7 +512,7 @@ export class SpaceStoreClass extends AsyncStoreWithClient<IState> {
         const oldFilteredRooms = this.spaceFilteredRooms;
         this.spaceFilteredRooms = new Map();
 
-        if (!spacesTweakAllRoomsEnabled) {
+        if (!this.allRoomsInHome) {
             // put all room invites in the Home Space
             const invites = visibleRooms.filter(r => !r.isSpaceRoom() && r.getMyMembership() === "invite");
             this.spaceFilteredRooms.set(HOME_SPACE, new Set<string>(invites.map(room => room.roomId)));
@@ -562,8 +568,10 @@ export class SpaceStoreClass extends AsyncStoreWithClient<IState> {
         });
 
         this.spaceFilteredRooms.forEach((roomIds, s) => {
+            if (this.allRoomsInHome && s === HOME_SPACE) return; // we'll be using the global notification state, skip
+
             // Update NotificationStates
-            this.getNotificationState(s)?.setRooms(visibleRooms.filter(room => {
+            this.getNotificationState(s).setRooms(visibleRooms.filter(room => {
                 if (!roomIds.has(room.roomId)) return false;
 
                 if (DMRoomMap.shared().getUserIdForRoomId(room.roomId)) {
@@ -663,7 +671,7 @@ export class SpaceStoreClass extends AsyncStoreWithClient<IState> {
                 // TODO confirm this after implementing parenting behaviour
                 if (room.isSpaceRoom()) {
                     this.onSpaceUpdate();
-                } else if (!spacesTweakAllRoomsEnabled) {
+                } else if (!this.allRoomsInHome) {
                     this.onRoomUpdate(room);
                 }
                 this.emit(room.roomId);
@@ -687,7 +695,7 @@ export class SpaceStoreClass extends AsyncStoreWithClient<IState> {
             if (order !== lastOrder) {
                 this.notifyIfOrderChanged();
             }
-        } else if (ev.getType() === EventType.Tag && !spacesTweakAllRoomsEnabled) {
+        } else if (ev.getType() === EventType.Tag && !this.allRoomsInHome) {
             // If the room was in favourites and now isn't or the opposite then update its position in the trees
             const oldTags = lastEv?.getContent()?.tags || {};
             const newTags = ev.getContent()?.tags || {};
@@ -698,7 +706,7 @@ export class SpaceStoreClass extends AsyncStoreWithClient<IState> {
     };
 
     private onAccountData = (ev: MatrixEvent, lastEvent: MatrixEvent) => {
-        if (ev.getType() === EventType.Direct) {
+        if (!this.allRoomsInHome && ev.getType() === EventType.Direct) {
             const lastContent = lastEvent.getContent();
             const content = ev.getContent();
 
@@ -733,10 +741,8 @@ export class SpaceStoreClass extends AsyncStoreWithClient<IState> {
             this.matrixClient.removeListener("Room.myMembership", this.onRoom);
             this.matrixClient.removeListener("Room.accountData", this.onRoomAccountData);
             this.matrixClient.removeListener("RoomState.events", this.onRoomState);
-            if (!spacesTweakAllRoomsEnabled) {
             this.matrixClient.removeListener("accountData", this.onAccountData);
         }
-        }
         await this.reset();
     }
 
@@ -746,9 +752,7 @@ export class SpaceStoreClass extends AsyncStoreWithClient<IState> {
         this.matrixClient.on("Room.myMembership", this.onRoom);
         this.matrixClient.on("Room.accountData", this.onRoomAccountData);
         this.matrixClient.on("RoomState.events", this.onRoomState);
-        if (!spacesTweakAllRoomsEnabled) {
         this.matrixClient.on("accountData", this.onAccountData);
-        }
 
         this.matrixClient.getCapabilities().then(capabilities => {
             this._restrictedJoinRuleSupport = capabilities
@@ -779,7 +783,7 @@ export class SpaceStoreClass extends AsyncStoreWithClient<IState> {
                     // as it will cause you to end up in the wrong room
                     this.setActiveSpace(room, false);
                 } else if (
-                    (!spacesTweakAllRoomsEnabled || this.activeSpace) &&
+                    (!this.allRoomsInHome || this.activeSpace) &&
                     !this.getSpaceFilteredRoomIds(this.activeSpace).has(roomId)
                 ) {
                     this.switchToRelatedSpace(roomId);
@@ -791,17 +795,33 @@ export class SpaceStoreClass extends AsyncStoreWithClient<IState> {
                 window.localStorage.setItem(getSpaceContextKey(this.activeSpace), payload.room_id);
                 break;
             }
+
             case "after_leave_room":
                 if (this._activeSpace && payload.room_id === this._activeSpace.roomId) {
                     this.setActiveSpace(null, false);
                 }
                 break;
+
             case Action.SwitchSpace:
                 if (payload.num === 0) {
                     this.setActiveSpace(null);
                 } else if (this.spacePanelSpaces.length >= payload.num) {
                     this.setActiveSpace(this.spacePanelSpaces[payload.num - 1]);
                 }
+                break;
+
+            case Action.SettingUpdated: {
+                const settingUpdatedPayload = payload as SettingUpdatedPayload;
+                if (settingUpdatedPayload.settingName === "feature_spaces.all_rooms") {
+                    const newValue = SettingsStore.getValue("feature_spaces.all_rooms");
+                    if (this.allRoomsInHome !== newValue) {
+                        this._allRoomsInHome = newValue;
+                        this.emit(UPDATE_HOME_BEHAVIOUR, this.allRoomsInHome);
+                        this.rebuild(); // rebuild everything
+                    }
+                }
+                break;
+            }
         }
     }
 
@@ -872,7 +892,6 @@ export class SpaceStoreClass extends AsyncStoreWithClient<IState> {
 
 export default class SpaceStore {
     public static spacesEnabled = spacesEnabled;
-    public static spacesTweakAllRoomsEnabled = spacesTweakAllRoomsEnabled;
 
     private static internalInstance = new SpaceStoreClass();
 

test/stores/SpaceStore-test.ts

@@ -21,6 +21,7 @@ import { RoomMember } from "matrix-js-sdk/src/models/room-member";
 import "./SpaceStore-setup"; // enable space lab
 import "../skinned-sdk"; // Must be first for skinning to work
 import SpaceStore, {
+    UPDATE_HOME_BEHAVIOUR,
     UPDATE_INVITED_SPACES,
     UPDATE_SELECTED_SPACE,
     UPDATE_TOP_LEVEL_SPACES,
@@ -30,6 +31,8 @@ import { mkEvent, stubClient } from "../test-utils";
 import DMRoomMap from "../../src/utils/DMRoomMap";
 import { MatrixClientPeg } from "../../src/MatrixClientPeg";
 import defaultDispatcher from "../../src/dispatcher/dispatcher";
+import SettingsStore from "../../src/settings/SettingsStore";
+import { SettingLevel } from "../../src/settings/SettingLevel";
 
 jest.useFakeTimers();
 
@@ -79,8 +82,16 @@ describe("SpaceStore", () => {
         jest.runAllTimers();
     };
 
+    const setShowAllRooms = async (value: boolean) => {
+        if (store.allRoomsInHome === value) return;
+        const emitProm = testUtils.emitPromise(store, UPDATE_HOME_BEHAVIOUR);
+        await SettingsStore.setValue("feature_spaces.all_rooms", null, SettingLevel.DEVICE, value);
+        jest.runAllTimers(); // run async dispatch
+        await emitProm;
+    };
+
     beforeEach(() => {
-        jest.runAllTimers();
+        jest.runAllTimers(); // run async dispatch
         client.getVisibleRooms.mockReturnValue(rooms = []);
     });
     afterEach(async () => {
@@ -346,10 +357,16 @@ describe("SpaceStore", () => {
                 expect(store.getSpaceFilteredRoomIds(null).has(invite2)).toBeTruthy();
             });
 
-            it("home space does contain rooms/low priority even if they are also shown in a space", () => {
+            it("all rooms space does contain rooms/low priority even if they are also shown in a space", async () => {
+                await setShowAllRooms(true);
                 expect(store.getSpaceFilteredRoomIds(null).has(room1)).toBeTruthy();
             });
 
+            it("home space doesn't contain rooms/low priority if they are also shown in a space", async () => {
+                await setShowAllRooms(false);
+                expect(store.getSpaceFilteredRoomIds(null).has(room1)).toBeFalsy();
+            });
+
             it("space contains child rooms", () => {
                 const space = client.getRoom(space1);
                 expect(store.getSpaceFilteredRoomIds(space).has(fav1)).toBeTruthy();
@@ -592,20 +609,30 @@ describe("SpaceStore", () => {
     });
 
     describe("context switching tests", () => {
-        const fn = jest.spyOn(defaultDispatcher, "dispatch");
+        let dispatcherRef;
+        let currentRoom = null;
 
         beforeEach(async () => {
             [room1, room2, orphan1].forEach(mkRoom);
             mkSpace(space1, [room1, room2]);
             mkSpace(space2, [room2]);
             await run();
+
+            dispatcherRef = defaultDispatcher.register(payload => {
+                if (payload.action === "view_room" || payload.action === "view_home_page") {
+                    currentRoom = payload.room_id || null;
+                }
+            });
         });
         afterEach(() => {
-            fn.mockClear();
             localStorage.clear();
+            defaultDispatcher.unregister(dispatcherRef);
         });
 
-        const getCurrentRoom = () => fn.mock.calls.reverse().find(([p]) => p.action === "view_room")?.[0].room_id;
+        const getCurrentRoom = () => {
+            jest.runAllTimers();
+            return currentRoom;
+        };
 
         it("last viewed room in target space is the current viewed and in both spaces", async () => {
             await store.setActiveSpace(client.getRoom(space1));
@@ -642,6 +669,14 @@ describe("SpaceStore", () => {
             expect(getCurrentRoom()).toBe(space2);
         });
 
+        it("last viewed room is target space is no longer in that space", async () => {
+            await store.setActiveSpace(client.getRoom(space1));
+            viewRoom(room1);
+            localStorage.setItem(`mx_space_context_${space2}`, room1);
+            await store.setActiveSpace(client.getRoom(space2));
+            expect(getCurrentRoom()).toBe(space2); // Space home instead of room1
+        });
+
         it("no last viewed room in target space", async () => {
             await store.setActiveSpace(client.getRoom(space1));
             viewRoom(room1);
@@ -653,7 +688,7 @@ describe("SpaceStore", () => {
             await store.setActiveSpace(client.getRoom(space1));
             viewRoom(room1);
             await store.setActiveSpace(null);
-            expect(fn.mock.calls[fn.mock.calls.length - 1][0]).toStrictEqual({ action: "view_home_page" });
+            expect(getCurrentRoom()).toBeNull(); // Home
         });
     });
 
@@ -707,6 +742,7 @@ describe("SpaceStore", () => {
         });
 
         it("when switching rooms in the all rooms home space don't switch to related space", async () => {
+            await setShowAllRooms(true);
             viewRoom(room2);
             await store.setActiveSpace(null, false);
             viewRoom(room1);