Spaces.includeSubSpaceRoomsInRoomList settings #7682

kerryarchibald · 2022-01-31T16:43:32Z

Adds a setting to allow only showing direct children of a space in the room list:

Here's what your changelog entry will look like:

✨ Features

Spaces.includeSubSpaceRoomsInRoomList settings (#7682). Contributed by @kerryarchibald.

Preview: https://pr7682--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

… setting in SpaceFilterCondition Signed-off-by: Kerry Archibald <kerrya@element.io>

Signed-off-by: Kerry Archibald <kerrya@element.io>

t3chguy

This implementation, whilst functional, pushes into the hard-to-maintain. I think this might be a point where refactoring the SpaceFilterCondition and SpaceNotificationState to be hierarchical (contain child instances of themselves for each sub-space) and then only store Maps from spaceId->direct children would be more sane as then simply culling the hierarchical behaviour in SpaceFilterCondition would achieve this setting

t3chguy · 2022-01-31T16:52:15Z

src/settings/Settings.tsx

@@ -858,6 +858,11 @@ export const SETTINGS: {[setting: string]: ISetting} = {
        default: true,
        controller: new IncompatibleController("showCommunitiesInsteadOfSpaces", null),
    },
+    "Spaces.includeSubSpaceRoomsInRoomList": {
+        displayName: _td("Include all sub-space rooms in Space room list"),


We consider sub-space/subspace jargon and do not use it in the UI

What language should be used? is there a guide for this somewhere?

There is no guide given that Subspaces are still in Beta.

There is no existing language here but all instances of "sub-space" were previously stripped from the app so should not be re-introduced without being done holistically and intentionally.

t3chguy · 2022-01-31T16:52:29Z

test/stores/room-list/filters/SpaceFilterCondition-test.ts

@@ -0,0 +1,237 @@
+import { mocked } from 'ts-jest/utils';


t3chguy · 2022-01-31T16:54:49Z

src/stores/spaces/SpaceStore.ts

@@ -101,6 +101,8 @@ export class SpaceStoreClass extends AsyncStoreWithClient<IState> {
    private notificationStateMap = new Map<SpaceKey, SpaceNotificationState>();
    // Map from space key to Set of room IDs that should be shown as part of that space's filter
    private spaceFilteredRooms = new Map<SpaceKey, Set<string>>(); // won't contain MetaSpace.People
+    // Map from space key to Set of room IDs that should be shown as part of that space's filter
+    private spaceFilteredDirectChildRooms = new Map<SpaceKey, Set<string>>(); // won't contain MetaSpace.People


also presumably does not need to contain the other metaspaces given that'd just be duplicated identical data

Not only does not need to contain, simply would not contain, given you only populate it with spaceIds, so the signature needs updating to reflect reality

t3chguy · 2022-01-31T16:55:33Z

src/stores/spaces/SpaceStore.ts

+        if (space === MetaSpace.Home && this.allRoomsInHome) {
+            return this.getSpaceFilteredRoomIds(space);
+        }


IMO all metaspaces should get handled by the non-direct variant given metaspaces can't have child spaces

t3chguy · 2022-01-31T16:56:18Z

src/stores/spaces/SpaceStore.ts

                const expandedRoomIds = new Set(Array.from(roomIds).flatMap(roomId => {
                    return this.matrixClient.getRoomUpgradeHistory(roomId, true).map(r => r.roomId);
                }));
+                const expandedDirectChildRoomIds = new Set(directChildRoomIds.flatMap(roomId => {
+                    return this.matrixClient.getRoomUpgradeHistory(roomId, true).map(r => r.roomId);
+                }));


this duplicates work between the two loops

kerryarchibald · 2022-02-02T13:46:41Z

@t3chguy Is it the amount of state tracked to check for changes in onStoreUpdate that concerns you?
Adding hierarchical space filters seems like adding quite a lot of complexity for a relatively small win. Maintenance of both direct and indirect child lists in SpaceStore is still needed as isRoomInSpace depends on the indirect child list and is used pretty widely. It also complexifies the application of space settings - should the active spaces settings be applied down the hierarchy?

t3chguy · 2022-02-02T14:14:01Z

@t3chguy Is it the amount of state tracked to check for changes in onStoreUpdate that concerns you? Adding hierarchical space filters seems like adding quite a lot of complexity for a relatively small win. Maintenance of both direct and indirect child lists in SpaceStore is still needed as isRoomInSpace depends on the indirect child list and is used pretty widely. It also complexifies the application of space settings - should the active spaces settings be applied down the hierarchy?

The thing that concerns me is the very minute differences between the various fields, which would go away if things were done hierarchically, like how this new field is indirect children except doesn't concern itself with metaspaces, etc.

t3chguy · 2022-02-04T16:32:54Z

How does this interact with notification badges? If you set it to false then will the notifications badge still aggregate counts of grand*-child rooms?

If it does continue aggregating then this will be confusing, as you click the space with a Red (8) and find no rooms which match that expectation, then you click the (8) itself and it takes you to a room in a different view (subspace).
If it doesn't then you may miss notifications due to the subspace not being visible with the panel collapsed, so this may require some finesse.

Signed-off-by: Kerry Archibald <kerrya@element.io>

kerryarchibald · 2022-02-18T11:01:12Z

@t3chguy this one is back for more review :-)
Hierarchical space filters hit a major snag on cycles being allowed in space hierarchy. Shelving that for now to get this feature over the line.

**Ignore this, need to test notification states

t3chguy · 2022-02-18T11:03:28Z

Hierarchical space filters hit a major snag on cycles being allowed in space hierarchy. Shelving that for now to get this feature over the line.

Sure, the typical approach we take is maintaining and passing around a parentPath Set to prevent hitting upon such a cycle, would be nice if you could stick up what you have into a draft PR so someone else might continue it if you can't :)

kerryarchibald · 2022-02-18T11:31:22Z

Notification states when sub space rooms are hidden are a bit strange. I don't use any other messaging tools with the level of nesting that Element allows so I'm not sure what behaviour is usual. @nadonomy what do you think?

Screen.Recording.2022-02-18.at.12.13.48.mov

Expanding the space panel is ok, but makes it at least 3 clicks to find your unread room.

Compared to probably 2 clicks in the current room list:

t3chguy · 2022-02-18T14:31:58Z

This PR fixes element-hq/element-meta#418

t3chguy

switchToRelatedSpace will also need updating to handle the case where the root space has Spaces.includeSubSpaceRoomsInRoomList set to false and yet it still will choose it but the room will not be visible in the filtered Room List.

t3chguy · 2022-02-24T11:48:59Z

src/stores/spaces/SpaceStore.ts

@@ -136,6 +136,7 @@ export class SpaceStoreClass extends AsyncStoreWithClient<IState> {
        SettingsStore.monitorSetting("Spaces.allRoomsInHome", null);
        SettingsStore.monitorSetting("Spaces.enabledMetaSpaces", null);
        SettingsStore.monitorSetting("Spaces.showPeopleInSpace", null);
+        SettingsStore.monitorSetting("Spaces.includeSubSpaceRoomsInRoomList", null);


Why is this monitoring a setting which it doesn't react to changes of?

Signed-off-by: Kerry Archibald <kerrya@element.io>

QuditWolf · 2023-05-31T14:23:54Z

Seems like a really niche feature add!
What are the odds that this still gets merged?

My active subspace rooms flood the space rooms and I can't find them without searching for them...

Kerry Archibald added 5 commits January 31, 2022 17:26

rough working filter

aa423da

maintain set of direct space children, apply showSubSpaceRoomsInSpace…

90fb06c

… setting in SpaceFilterCondition Signed-off-by: Kerry Archibald <kerrya@element.io>

test isRoomInSpace

ab18347

Signed-off-by: Kerry Archibald <kerrya@element.io>

test SpaceFilterCOndition

5cda60c

Signed-off-by: Kerry Archibald <kerrya@element.io>

lint

e038df5

Signed-off-by: Kerry Archibald <kerrya@element.io>

kerryarchibald added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Jan 31, 2022

kerryarchibald requested a review from a team as a code owner January 31, 2022 16:43

forgotten i18n

307d12c

Signed-off-by: Kerry Archibald <kerrya@element.io>

t3chguy reviewed Jan 31, 2022

View reviewed changes

kerryarchibald marked this pull request as draft February 4, 2022 15:56

Kerry Archibald added 6 commits February 18, 2022 10:33

Merge branch 'develop' into pse-345/exclude-sub-space-rooms

2f6adfd

Signed-off-by: Kerry Archibald <kerrya@element.io>

filter rooms list by showSubSpaceRoomsInSpace

4bd1fd9

Signed-off-by: Kerry Archibald <kerrya@element.io>

Merge branch 'develop' into pse-345/exclude-sub-space-rooms

a54bf83

clunky copy update

b8ec565

Signed-off-by: Kerry Archibald <kerrya@element.io>

missed i18n

3722f83

Signed-off-by: Kerry Archibald <kerrya@element.io>

unused prop

d95e7e5

Signed-off-by: Kerry Archibald <kerrya@element.io>

kerryarchibald marked this pull request as ready for review February 18, 2022 10:58

kerryarchibald marked this pull request as draft February 18, 2022 11:01

kerryarchibald requested review from nadonomy and a team and removed request for nadonomy February 21, 2022 16:50

t3chguy suggested changes Feb 24, 2022

View reviewed changes

Merge branch 'develop' into pse-345/exclude-sub-space-rooms

89c86c2

Signed-off-by: Kerry Archibald <kerrya@element.io>

github-actions bot deployed to Netlify June 1, 2022 09:31 View deployment

Merge branch 'develop' into pse-345/exclude-sub-space-rooms

5fbe211

github-actions bot deployed to Netlify June 27, 2022 14:52 View deployment

Kerry Archibald added 4 commits June 27, 2022 20:37

make includeSubSpaceRoomsInRoomList an account level setting

cb1ab60

add includeSubSpaceRoomsInRoomList setting to user prefs

65d712d

handle includeSubSpaceRoomsInRoomList in switchSpaceIfNeeded

75016ad

add todo

a237932

github-actions bot deployed to Netlify June 27, 2022 19:28 View deployment

t3chguy mentioned this pull request Mar 6, 2024

added setting not to show indirect rooms of spaces in left sidebar #12317

Closed

langleyd closed this Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spaces.includeSubSpaceRoomsInRoomList settings #7682

Spaces.includeSubSpaceRoomsInRoomList settings #7682

kerryarchibald commented Jan 31, 2022 •

edited by github-actions bot

Loading

t3chguy left a comment

t3chguy Jan 31, 2022

kerryarchibald Feb 2, 2022

t3chguy Feb 2, 2022

t3chguy Jan 31, 2022

t3chguy Jan 31, 2022

t3chguy Jan 31, 2022

t3chguy Jan 31, 2022

t3chguy Jan 31, 2022

kerryarchibald commented Feb 2, 2022

t3chguy commented Feb 2, 2022

t3chguy commented Feb 4, 2022 •

edited

Loading

kerryarchibald commented Feb 18, 2022 •

edited

Loading

t3chguy commented Feb 18, 2022 •

edited

Loading

kerryarchibald commented Feb 18, 2022

t3chguy commented Feb 18, 2022

t3chguy left a comment

t3chguy Feb 24, 2022

QuditWolf commented May 31, 2023

Spaces.includeSubSpaceRoomsInRoomList settings #7682

Spaces.includeSubSpaceRoomsInRoomList settings #7682

Conversation

kerryarchibald commented Jan 31, 2022 • edited by github-actions bot Loading

✨ Features

t3chguy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kerryarchibald commented Feb 2, 2022

t3chguy commented Feb 2, 2022

t3chguy commented Feb 4, 2022 • edited Loading

kerryarchibald commented Feb 18, 2022 • edited Loading

t3chguy commented Feb 18, 2022 • edited Loading

kerryarchibald commented Feb 18, 2022

t3chguy commented Feb 18, 2022

t3chguy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuditWolf commented May 31, 2023

kerryarchibald commented Jan 31, 2022 •

edited by github-actions bot

Loading

t3chguy commented Feb 4, 2022 •

edited

Loading

kerryarchibald commented Feb 18, 2022 •

edited

Loading

t3chguy commented Feb 18, 2022 •

edited

Loading