-
Notifications
You must be signed in to change notification settings - Fork 158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial implementation of cosmic-workspace-unstable-v1 #2030
Conversation
1972260
to
e6aec2c
Compare
I don't have any strong preference here since I usually use only one workspace and the integration part is minimal as you said, but what will we do when the |
Yes, depending on the availability of clients that would then support ext-workspaces the idea would be to support both for labwc. |
Fine with me - in terms of scope. Not looked at code. |
e6aec2c
to
8c7e451
Compare
Waybar PR: Alexays/Waybar#3481 |
Yes, at least with the stock |
Yeah I figured I had to use stock first up, so just renamed my user config. I'll play with it some more later. |
There is also https://wayland.app/protocols/kde-plasma-virtual-desktop - not sure of the pros/cons of following Cosmic vs. KDE here. |
The only comment I have about adding different implementations like these is |
The lxqt-panel will use this one if the compositor is kwin_wayland. Desktop pager and "Move to Desktop x" in the windows button right click menu of the taskbar are working fine with it. |
From a quick look:
|
If the cosmic protocol is closer than the upstream MR, then that's reason enough, you've convinced me :) My only other thought: there was a wlr protocol proposed 5(!) years ago, https://gitlab.freedesktop.org/wlroots/wlr-protocols/-/merge_requests/35. It was closed in favor of the upstream wayland-protocols MR, and there's been no discussion on it recently. Would it be worth checking with the wlroots folks if it could be revived, given that the upstream MR seems to have stalled? I do realize wlroots development also happens slowly... if there is no interest there, then I'm fine with adopting the cosmic namespace variant. But it might be worth at least asking. |
That is an even older variant of the current upstream MR (and thus also an older version of the cosmic vendored one). I don't think we want yet another version of the same protocol floating around. My hope is that either the upstream one is merged within the next year or if that is not the case that wlroots will accept an implementation of the cosmic variant instead (for example the protocol implemenation in this PR that could be slightly adapted for wlroots). In the meantime, compositors and panels can add support for the cosmic vendored one which can be very easily adapted for the upstream MR once merged (as of now I think it only replaced the wl_array capabilities with a proper single uint32_t bitset).
As both cosmic and wlroots are members of the wayland-protocols governance body and protocols in the ext- namespace only need 2 ACKS from members I do not think that wlroots would accept a new wlr vendored protocol. At that point it would make way more sense to get the upstream one merged. See also https://gitlab.freedesktop.org/wlroots/wlr-protocols#submitting-new-protocols. |
Okay, this makes sense... a wlroots implementation would continue to use the "cosmic-" protocol namespace then (until the "ext-" version is merged). I am good with this approach. |
94b744d
to
2f71733
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have left a bunch of review comments - but other than those it LGTM. Not tested.
A couple of general points:
- Should we prefix with
wlr_cosmic_
? Couldn't we just docosmic
. For examplewlr_cosmic_workspace_group_output_enter()
- We ought to consider ways to not lose created workspace on Reconfigure. Can do that on separate PR though if we can think of something.
|
||
/* | ||
* .--------------------. | ||
* | TODO | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this TODO
still be here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will work on the remaining items tomorrow and then remove the TODO but none of them should cause any issues.
2f71733
to
e4f63a0
Compare
Thanks.
I've changed everything to a
I've removed the feature for clients to create workspaces completely as its unclear who is in control of the final workspace configuration. We could consider adding that back at some point (it just requires reacting to two signals). |
2bfc868
to
8567dd3
Compare
include/common/array.h
Outdated
* @_val: the item to add to the array | ||
* Note: asserts() when the memory allocation for the new item failed | ||
*/ | ||
#define xwl_array_add(_arr, _val) do { \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is indeed a nice wrapper. I now get it 😄
Suggest that we avoid assert()
and just exit()
with an error message.
Suggest that we call it array_add()
becase:
xwl
makes me think of XWayland- It actually does more than wl_array_add() + die-on-null
Also suggest adding the following to the comment in the header to make it easier to wrap one's head around it:
Let us illustrate the function of this macro by an example:
uint32_t value = 5;
array_add(array, value);
...is the equivalent of the code below which is how you would
otherwise use the wl_array API:
uint32_t *elm = wl_array_add(array, sizeof(uint32_t));
if (!elm) {
perror("failed to allocate memory");
exit(EXIT_FAILURE);
}
*elm = value;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to array_add()
, replaced the assert with exit()
and added your comment.
Have tested with waybar. Works well. LGTM other than the suggestion I just left on the array-macro. |
8567dd3
to
4df9c26
Compare
4df9c26
to
904e0d2
Compare
hello, installed labwc and sfwbar from Consolatis repositorie, work great. However, is there a way where sfwbar taskbar to show only windows from the current workspace? |
No, that requires a further protocol to match windows with workspaces. This one only supports controlling the workspaces themselves. |
We could implement cosmic-toplevel-info-unstable and cosmic-toplevel-management-unstable with a fallback to wlr-foreign-top-level in theory. Although I suspect that's out of scope for now ;) |
Kind of, it also becomes really messy once (and if) the upstream protocols are merged. Then we'd have
I am already quite happy when we can finally switch workspaces via panel (and have an indicator of the active workspace). |
Don't forget the yet-to-be-named ext protocols for toplevel-workspace
interaction (probably two of them as well? Info and management), since
ext-topvel-* and ext-workspace are currently completely unaware of each
other and the plan it to have bridging protocols linking them.
…On Thu, 8 Aug 2024, 20:29 Consolatis, ***@***.***> wrote:
No, that requires a further protocol to match windows with workspaces.
This one only supports controlling the workspaces themselves.
We could implement cosmic-toplevel-info-unstable and
cosmic-toplevel-management-unstable with a fallback to
wlr-foreign-top-level in theory. Although I suspect that's out of scope for
now ;)
Kind of, it also becomes really messy once (and if) the upstream protocols
are merged. Then we'd have
- wlr-foreign-toplevel
- ext-foreign-toplevel-list
- ext-foreign-toplevel-info
- ext-foreign-toplevel-management
- cosmic-foreign-toplevel-info
- cosmic-foreign-toplevel-management
I am already quite happy when we can finally switch workspaces via panel.
—
Reply to this email directly, view it on GitHub
<#2030 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ASHPFFHLNRSTKQ2RUVHV5ODZQO2H7AVCNFSM6AAAAABLPS3Q4KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZWGQZDAOJSG4>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
I don't know this for sure, I think @Zamundaaa would know for sure, but I think the goal is to support |
At some point, yes. |
LGTM |
Wow! Excellent! |
I finally have decided to not wait another 3 years until the official workspace protocol is accepted. This goes very much against my own believes that labwc should only use wlroots or wayland protocols but version 1 of the cosmic-workspaces protocol is basically the same as what is proposed in the upstream protocol. Using the cosmic variant also allows using the protocol without hiding it behind feature flags as the upstream protocol may change in incompatible ways before being accepted.
This PR is an initial implementation of the protocol, mostly written in a compositor agnostic way so there is the potential to also propose this implementation for wlroots with minor changes at some point (e.g. mostly
znew()
vscalloc()
andwl_list_append()
vswl_list_insert(x.prev, y)
).The integration part of the protocol into labwc is purposefully minimal, it only tells the client about the workspaces available, their state + name and allows the client to activate them. The protocol theoretically also allows for clients to configure the workspaces and the protocol implementation allows for that but the labwc integration does not.
Draft PR for sfwbar and Waybar to have something for testing:
Test config for Waybar
~/.config/waybar/config
~/.config/waybar/style.css
(modified the existingbutton:hover
)