Skip to content
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 #212

Merged
merged 16 commits into from
Aug 21, 2024

Conversation

Consolatis
Copy link
Contributor

@Consolatis Consolatis commented Jul 26, 2024

This is a partial implementation of the cosmic-workspace-unstable-v1 protocol,

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 allows to list the compositor workspaces including their state (being active or not) and to select the active workspace.
The protocol itself also allows to configure workspaces (create and remove) which is not implemented here (nor in labwc).

Some more caveats:

  • workspace coordinates are not handled
  • there is a hidden and urgent state per workspace that isn't handled in this PR
  • workspace groups are completely ignored in this PR, only the workspaces themselves are shown
  • the output_enter and output_leave group events are not handled in this PR

Most of the above are due to me not being versed enough in the sfwbar internals and thus not sure how to hook them up without complicating the other IPC systems.

This PR is based on top of #211, will rebase once that one is merged.
The codestyle in cosmic-workspaces.c is using tabs, I am completely fine adjusting that if it matters.

There is a labwc draft PR to test against:

Alternatively testing against cosmic itself should hopefully also work but I didn't try that at all.

@LBCrion
Copy link
Owner

LBCrion commented Jul 26, 2024 via email

@01micko
Copy link
Contributor

01micko commented Jul 29, 2024

This seems to work great! When I get time I'll stuff around with the widget to get it looking nice :) Thanks @Consolatis for pursuing this!

@01micko
Copy link
Contributor

01micko commented Jul 29, 2024

Screeny labwc/labwc#2030 (comment)

@LBCrion
Copy link
Owner

LBCrion commented Jul 29, 2024

Apologies for starting with pedantic stuff - ident formatting. I am looking at the substance as well :)

@01micko
Copy link
Contributor

01micko commented Jul 30, 2024

I think this isn't working @LBCrion after your latest commit to @Consolatis PR

  taskbar {
    rows = 1
    css = "* { -GtkWidget-hexpand: false; }" # stretch horizontally
    title_width = 20
    sort = false
    group = false
    icons = true
    labels = true
    action[3] = Menu "winops"
  }
  label {
    css = "* { -GtkWidget-hexpand: true; }"
  }

to restrict the width.

On the plus side this works great in labwc/rc.xml

  <desktops>
    <popupTime>1000</popupTime>
    <names>
      <name>main</name>
      <name>work</name>
      <name>misc</name>
      <name>video</name>
    </names>
  </desktops>

except they are sorted alphabetically.

@01micko
Copy link
Contributor

01micko commented Jul 30, 2024

Tweaked config

recording.mp4

@LBCrion
Copy link
Owner

LBCrion commented Jul 30, 2024

I think this isn't working @LBCrion after your latest commit to @Consolatis PR

Do you mean the title_width property isn't working for the taskbar? The PR should have no impact on this. Does it work with beta15?

except they are sorted alphabetically.

You should be able to add sort = false property to the pager widget.

@LBCrion
Copy link
Owner

LBCrion commented Jul 30, 2024

@Consolatis, are there any plans to implement create_workspace in labwc? The ability to create workspaces from the pager (in the form of pinned workspaces) is quite useful.

Also, is there a way to rename a workspace on the fly? I would like to test the handling of workspace/pin name collision scenario.

@Consolatis
Copy link
Contributor Author

Consolatis commented Jul 30, 2024

@Consolatis, are there any plans to implement create_workspace in labwc? The ability to create workspaces from the pager (in the form of pinned workspaces) is quite useful.

No immediate plans at least. This is something that might be implemented in labwc at some point as the ability to create / rename / delete workspaces is already available within labwc itself (and cosmic workspaces should already mirror these changes).

Also, is there a way to rename a workspace on the fly? I would like to test the handling of workspace/pin name collision scenario.

Version 2 of the protocol added a rename action and corresponding capability.
For testing you can modify the workspace names (and/or count) in rc.xml and then call the Reconfigure action.

@Consolatis
Copy link
Contributor Author

Consolatis commented Jul 30, 2024

I've added a commit to the labwc draft PR which allows for creating workspaces. Note that it will be reverted before merging and is not a complete implementation (e.g. once you Reconfigure labwc the new workspaces will be removed - or potentially renamed when the ws count in rc.xml was increased).

…ion of new states, cosmic-workspaces: handle workspace creation correctly.
@LBCrion
Copy link
Owner

LBCrion commented Jul 31, 2024

Thank you, I got the creation working correctly now. Is the idea for the stable implementation for now to only support workspace definition changes via Reconfigure, Since any changes from other sources would need to be reconciled vs the config?

It would probably be cleanest to disable to pins until dynamic workspace creation is supported. I think on sfwbar side, I may introduce a corresponding capability and only display pins if it's enabled.

@Consolatis
Copy link
Contributor Author

Is the idea for the stable implementation for now to only support workspace definition changes via Reconfigure, Since any changes from other sources would need to be reconciled vs the config?

Yes, that is the main stopper for labwc in regards to workspace creation.
We could allow workspace creation and removal (via the REMOVE cap / remove() request) and make them survive a Reconfigure action (by additionally storing them in some external list) but as Labwc never writes to the config the workspaces would be gone on the next labwc restart. That feels somewhat inconsistent so for now Labwc will only support activation.

@LBCrion
Copy link
Owner

LBCrion commented Aug 2, 2024

I think I finally got this PR into state that's ready for merging. @Consolatis, I would appreciate if you could review it to see if I missed / screwed up anything.
I'll be away from the computer for the next week and will probably look to merge it the week unless any issues pop up with it.

@01micko, if you get a chance to test the latest version of this PR, it would be much appreciated.

@Consolatis
Copy link
Contributor Author

I think I finally got this PR into state that's ready for merging. @Consolatis, I would appreciate if you could review it

Will do during the week, thanks for your constant work on sfwbar, really appreciate your dedication.

@01micko
Copy link
Contributor

01micko commented Aug 2, 2024

Thanks guys for the great efforts put into this!

On initial look it works great - both labwc (20 mins ago so latest wip/cosmic_wokspaces) and sfwbar on latest commits.

The 'pins' issue is solved.

Couple of warnings on start up with bone stock config:

~$ sfwbar
i3-ipc*input type:keyboard xkb_switch_layout nexti3-ipc*input type:keyboard xkb_switch_layout prev
(sfwbar:23886): GLib-CRITICAL **: 07:41:53.635: g_hash_table_remove_all: assertion 'hash_table != NULL' failed

I also have a vertical config I've been testing and that works best with this in rc.xml, this is because workspaces doesn't fit in a 12x12 container and gets elipsised.

  <desktops>
    <popupTime>1000</popupTime>
    <names>
      <name>1</name>
      <name>2</name>
      <name>3</name>
      <name>4</name>
    </names>
  </desktops>

The only warnings I get with that is widget conflicts - so my problem.

Which brings me to the UX

  1. I haven't tested with translations yet (will soon) but is it expected that 'workspaces' will be translated when shown in sfwbar? (probably not) translations survive 👍
  2. If you had say 6 workspaces configured that would take up a lot of bar real estate. I usually have at least 2 and often 4. Could we do some string manipulation to just show the desktop number? Or at least make it optional.
  3. CSS - probably more documentation than anything else. I almost have my vertical config as I want.

Many thanks @Consolatis and @LBCrion 😄

@01micko
Copy link
Contributor

01micko commented Aug 2, 2024

Vertical demo

vert.mp4

@Consolatis
Copy link
Contributor Author

Consolatis commented Aug 2, 2024

Using either unicode symbols or emoticons might also work, e.g. something like

  <desktops>
    <names>
      <name>💻</name>
      <name>📺</name>
      <name>📜</name>
    </names>
  </desktops>

@01micko
Copy link
Contributor

01micko commented Aug 2, 2024

I think this isn't working @LBCrion after your latest commit to @Consolatis PR

Do you mean the title_width property isn't working for the taskbar? The PR should have no impact on this. Does it work with beta15?

I didn't realise it wan't part of the stock config! Sorry about that noise. All works as expected with title_width = 25

@01micko
Copy link
Contributor

01micko commented Aug 3, 2024

I suppose this makes sense 😝

canasta

Still wrestling with the CSS!

@01micko
Copy link
Contributor

01micko commented Aug 4, 2024

Translations do survive. Tested with es_ES locale.

@01micko
Copy link
Contributor

01micko commented Aug 4, 2024

Even works with RTL

ar_locale

@LBCrion
Copy link
Owner

LBCrion commented Aug 4, 2024

Couple of warnings on start up with bone stock config:

~$ sfwbar
i3-ipc*input type:keyboard xkb_switch_layout nexti3-ipc*input type:keyboard xkb_switch_layout prev
(sfwbar:23886): GLib-CRITICAL **: 07:41:53.635: g_hash_table_remove_all: assertion 'hash_table != NULL' failed

These should hopefully be fixed in the main branch now.

@Consolatis
Copy link
Contributor Author

Consolatis commented Aug 12, 2024

Sorry, took a bit longer to come back to this PR. From the protocol perspective all changes look fine to me.

Regarding the path forward, labwc will have a release around Aug. 16th (without cosmic-workspace support) and will then likely merge the server side implementation in its master branch shortly after that. Obviously completely up to you if you want to sync it up with labwc or merge this PR before / after.

There might be some more minor changes on the labwc side of the PR but nothing that would change anything regarding the protocol.

@LBCrion
Copy link
Owner

LBCrion commented Aug 13, 2024

No problem at all. I made a few more small changes in the meantime - namely added an invalid state to avoid updating pager items more often than needed (updating gtk widgets is relatively cpu intensive).
I probably would prefer to merge this before labwc, or at least the part of this PR that deal with changes to workspace API. But I would be curious to see how the ext-workspaces PR progresses in case there is hope of it getting merged soon.

@LBCrion
Copy link
Owner

LBCrion commented Aug 16, 2024

One more thing I had on my mind that would be relevant to this PR is handling of wayland globals. Currently sfwbar registers multiple global handlers in order to enforce the priority of protocols. I think it would be cleaner to get a list of interfaces available and then bind to them if and when necessary (I think we'll need to do this if we implement cosmic workspaces and then need to decide if we want to bind to this interface or to ext-workspaces once that's avaible).

My thinking so far is to do the following:

  1. register a global handler
  2. do a roundtrip to get interfaces
  3. have the global save the information on all interfaces that come through
  4. switch semaphore that will tell the global handler to stop processing new interfaces.
  5. call all of the sfwbar _init functions in order and let each out rummage through the interface list the global handler saved to determine if it wants to bind to an interface.

The question is, can we be confident that all the interfaces will be advertised on the first roundtrip and is there a chance of them going away in the future before we can bind to them?

@Consolatis
Copy link
Contributor Author

Consolatis commented Aug 16, 2024

The question is, can we be confident that all the interfaces will be advertised on the first roundtrip and is there a chance of them going away in the future before we can bind to them?

Yes, I think so. It is already the case with outputs but technically compositors can remove (global_remove event on the wl_registry) and add globals at any point in time. I've never seen compositors do this outside of the wl_output case though.

Edit:
So I think this should mostly work:

  • wl_display.get_registry()
  • wl_display.sync() (thus creating a wl_callback instance)
  • storing the globals in the registry handler
  • in the handler for the wl_callback.done event kick of loading of all the protocols based on the stored list

Not sure how to react to further global and global_remove events after initialization.

@LBCrion
Copy link
Owner

LBCrion commented Aug 18, 2024

I added the interface storage list and updated all protocol implementation to use it in this PR. It seems to work well and should allow to mediate between ext-workspaces and cosmic-workspaces in the future.

@LBCrion LBCrion marked this pull request as ready for review August 21, 2024 20:42
@LBCrion LBCrion merged commit 25b06e2 into LBCrion:main Aug 21, 2024
1 check passed
@Consolatis Consolatis deleted the feature/cosmic_workspaces branch August 21, 2024 21:25
@Consolatis
Copy link
Contributor Author

Thanks for merging and all your improvements over the initial PR!
Will follow with the server side implementation in labwc in a few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants