Draggable Space Ordering and Space Folders#701
Draggable Space Ordering and Space Folders#701CollinBartlam wants to merge 4 commits intocommetchat:mainfrom
Conversation
Airyzz
left a comment
Airyzz
left a comment
There was a problem hiding this comment.
This is a really cool PR, and will be a great feature to have. I'm mostly just commenting on backend stuff at the moment, haven't had a look over UI yet. Mostly wanting to tweak it to be more inline with how other features in Commet are implemented using Components.
There was a problem hiding this comment.
This should be implemented as a Client Component, with an abstract SidebarLayoutComponent which doesn't implement any matrix-specific functionality, just the bare layout api, and MatrixSidebarLayoutComponent which implements the specifics of how its stored
| import 'package:commet/client/sidebar/sidebar_model.dart'; | ||
|
|
||
| class SidebarData { | ||
| static const String accountDataType = 'im.commet.space_sidebar'; |
There was a problem hiding this comment.
use chat.commet.sidebar_layout instead
| Future<void> _persist() async { | ||
| final data = SidebarData(items: _rawItems); | ||
| _lastAccountDataHash = data.toJson().toString(); | ||
| await SidebarPersistence.writeToAllClients(clientManager.clients, data); |
There was a problem hiding this comment.
I dont think we should be storing the entire layout in all clients, Ideally each client should only store the layout for its own spaces. As is, this would leak user membership info to the homeserver of your other accounts.
I'd suggest each account stores an order string, similarly to how space children are ordered in the spec
You could still allow creating a folder that stores items from multiple accounts by just using the same folder ID
| Map<String, dynamic> toJson() { | ||
| return { | ||
| 'version': version, | ||
| 'items': items.map((item) { |
There was a problem hiding this comment.
Instead of a list, store in a Map, and sort by order string
There was a problem hiding this comment.
I think probably this should be moved out of client and in to a Widget, maybe modify SideNavigationBar to be aware of the component?
There was a problem hiding this comment.
move these into new SidebarLayoutComponent
| required this.id, | ||
| required this.name, | ||
| required this.spaces, | ||
| this.isExpanded = false, |
There was a problem hiding this comment.
UI state probably shouldn't be stored here, at the moment for subspaces we store expanded state just in the widget itself, which I think is preferable
There was a problem hiding this comment.
This seems very similar to ResolvedSidebarItem and similar, why do we need both?
| late CallManager callManager; | ||
|
|
||
| late final DirectMessagesAggregator directMessages; | ||
| late final SidebarManager sidebarManager; |
There was a problem hiding this comment.
I dont think this needs to exist in ClientManager, maybe in the state of SideNavigationBar makes more sense?
2 participants
Implements #644 by adding persistent reordering of spaces, allowing users to drag and drop spaces into the order they desire. Also implements #646 by adding the ability to drag these spaces in and out of folders to help users organize. All of this information is stored via Matrix account data, which allows changes to sync across different Commet clients/instances. Users can edit folders names or remove folders using either a right-click context menu in the desktop layout or a press and hold in the mobile layout.