Add ext_workspace_v1 helper - #529
pastthepixels wants to merge 7 commits into
Conversation
note: doesn't support workspace groups fully yet..
| #[non_exhaustive] | ||
| pub struct WorkspaceInfo { | ||
| // ID | ||
| pub id: String, |
There was a problem hiding this comment.
IMO this should be an option, as the compositor may not send this event
| // State | ||
| pub state: Option<WEnum<State>>, | ||
| // Coordinates | ||
| pub coordinates: Vec<u8>, |
There was a problem hiding this comment.
Coordinates are u32. The protocol sends it as an array of bytes, but it should be retransformed.
| pub state: Option<WEnum<State>>, | ||
| // Coordinates | ||
| pub coordinates: Vec<u8>, | ||
| // Capabilities | ||
| pub capabilities: Option<WEnum<WorkspaceCapabilities>>, |
There was a problem hiding this comment.
Both state and capabilities are bitfields, not enums, so IMO wrapping them in WEnum is wrong here.
Instead, it would be better to convert the enum back to the raw bitfield and store that (If more than one flag is set, the value will be stored as WEnum::Unknown(<value>))
Since they are bitfield, wrapping them in Option is kinda superfluous too, as their default value (all flags unset) is a perfectly valid state.
| // Workspaces | ||
| pub workspaces: Vec<ext_workspace_handle_v1::ExtWorkspaceHandleV1>, | ||
| // Capabilities | ||
| pub capabilities: Option<WEnum<GroupCapabilities>>, |
There was a problem hiding this comment.
Same remark as WorkspaceCapabilities, this is also a bitfield, so Option and WEnum doesn't quite match here IMO.
| struct GroupInner { | ||
| current_info: Option<GroupInfo>, | ||
| pending_info: GroupInfo, | ||
| } |
There was a problem hiding this comment.
IMO pending_info should be wrapped in Option here.
There are two advantages to that:
- on
done, you'd just need to doself.current_info = pending_info.take() - the
donehandler could 'know' which workspace/group changed, and call theWorkspaceHandler::update_*method only for items whose state has actually changed.
| /// Returns information about a workspace. | ||
| /// | ||
| /// This may be none if the workspace has been destroyed or the compositor has not sent | ||
| /// information about the workspace yet. | ||
| pub fn info( | ||
| &self, | ||
| workspace: &ext_workspace_handle_v1::ExtWorkspaceHandleV1, | ||
| ) -> Option<WorkspaceInfo> { | ||
| workspace.data::<WorkspaceData>()?.0.lock().unwrap().current_info.clone() | ||
| } | ||
|
|
||
| /// Returns information about a workspace group. | ||
| pub fn info_group( | ||
| &self, | ||
| group: &ext_workspace_group_handle_v1::ExtWorkspaceGroupHandleV1, | ||
| ) -> Option<GroupInfo> { | ||
| group.data::<GroupData>()?.0.lock().unwrap().current_info.clone() | ||
| } |
There was a problem hiding this comment.
While those function works, it would be nice to have a proper handle object for both workspace and groups, on which you could query the state directly.
On that note, the actual handles client code would get could be created after the initial Done event, meaning the committed state would not need to be behind an option, hiding that implementation detail.
| } | ||
| } |
There was a problem hiding this comment.
The WorkspaceManager should either expose a pub fn commit(&self) or the underlying ExtWorkspaceManagerV1
| state.ext_workspace_state().workspaces.push(workspace); | ||
| } | ||
| ext_workspace_manager_v1::Event::Done => { | ||
| // TODO: is cloning really the best for performance? |
There was a problem hiding this comment.
I think here, it would be best not to clone the whole workspace/group Vec and instead build a Vec containing only the changed value (see previous comment about having both pending state and current state being wrapped in option).
That way, you can still have the new/update functions, but you can call them at a point you no-longer need to hold the &mut WorkspaceManager.
For the initial burst of events, it makes no differences, but later on, you'd clone less items overall.
This branch has not been deployed
2 participants
This code might be a bit jank since I'm not familiar with the codebase nor Wayland protocol and mostly modified code from the foreign_toplevel_list helper. Feedback would be appreciated!
This implements a helper for the ext_workspace_v1 protocol aka everything I could read up here. This is good for e.g. graphical shells, like mine which has an implementation here