Waybar icon indicating copy to clipboard operation
Waybar copied to clipboard

Hyprland/workspaces: use Hyprland's workspace rules for persistency

Open zjeffer opened this issue 1 year ago • 18 comments

Hyprland now has a workspace rule persistent.

This PR has the following changes:

  • Persistent workspaces can now be specified in Hyprland's configuration
    • show the persistent workspaces for every rule in the workspacerules (hyprctl workspacerules -j)
  • Added a "configreloaded" event listener, which will reinitialize all workspaces
    • This makes it possible to keep changing persistent workspaces configuration while the bar is running, without having to restart it.
    • This requires this Hyprland commit: https://github.com/hyprwm/Hyprland/commit/9fba887cc9c3a8616b43f5778634541099c89c43

Example Hyprland config:

workspace = 1, monitor:eDP-1, persistent:true
workspace = 2, monitor:eDP-1, persistent:true
workspace = 3, monitor:eDP-1, persistent:true
workspace = 4, monitor:eDP-1, persistent:true
workspace = 5, monitor:eDP-1, persistent:true

workspace = 6, monitor:DP-1, persistent:true
workspace = 7, monitor:DP-1, persistent:true
workspace = 8, monitor:DP-1, persistent:true
workspace = 9, monitor:DP-1, persistent:true
workspace = 10, monitor:DP-1, persistent:true

workspace = 11, monitor:HDMI-A-1, persistent:true
workspace = 12, monitor:HDMI-A-1, persistent:true
workspace = 13, monitor:HDMI-A-1, persistent:true
workspace = 14, monitor:HDMI-A-1, persistent:true
workspace = 15, monitor:HDMI-A-1, persistent:true

With my fork of Duckonaut's split-monitor-workspaces plugin, you can specify how many persistent workspaces you want to automatically create on every monitor. It will create a file /tmp/hyprland-workspace-rules which contains these rules. Simply source the created file in your Hyprland config and it'll create the workspaces in Waybar too.

zjeffer avatar Oct 23 '23 17:10 zjeffer

Cool, I’ll definitely test this when I get home after this weekend. Curious to see how it affects some of the workspaces functionality I’ve been trying to debug

khaneliman avatar Dec 30 '23 16:12 khaneliman

Things that still need to be tested:

  • Moving workspaces to another monitor (not sure what should/would happen. I guess persistent workspaces should never be moved, but what if the user does without changing the config?)
  • The window functionality (I've never used this)

zjeffer avatar Dec 30 '23 16:12 zjeffer

Moving workspaces to another monitor (not sure what should/would happen. I guess persistent workspaces should never be moved, but what if the user does without changing the config?)

I actually think this is a pretty frequent use case and it's how I currently use it and it works. When I dock my laptop, new monitors are added, I move some of my workspaces to the new monitors. See this gist (latest version of it in my dotfiles) and my initial feature request to Hyprland

Diaoul avatar Dec 31 '23 16:12 Diaoul

I see: so you move the workspaces using a script, and also create new workspace rules. I don't know if this PR currently handles that properly. It successfully removes old persistent workspaces when you reload the Hyprland config, but I don't think a script that calls hyprctl will properly move them.

Can you check if your config and script works with this branch?

zjeffer avatar Dec 31 '23 16:12 zjeffer

Things that still need to be tested:

* Moving workspaces to another monitor (not sure what should/would happen. I guess persistent workspaces should never be moved, but what if the user does without changing the config?)

* The window functionality (I've never used this)

If you're talking about the window-rewrite functionality, it seems to still work the same as it did before (for me) while using this PR as the input for nix flake. I hoped it would somehow solve my issue with the window-rewrite not working on boot and needing to restart waybar, but it didn't. I was able to remove the persistent_workspaces part of my waybar config and the persistent workspaces appeared in the bar still.

* Added a "configreloaded" event listener, which will reinitialize all workspaces
  
  * This makes it possible to keep changing persistent workspaces configuration while the bar is running, without having to restart it.

This didn't work for me though, I did the hyprctl reload and as soon as it reloads waybar the window-rewrite stuff got wiped out. image image

khaneliman avatar Jan 04 '24 23:01 khaneliman

This didn't work for me though, I did the hyprctl reload and as soon as it reloads waybar the window-rewrite stuff got wiped out.

I think @Syndelis wrote the window rewrite stuff, I don't know much about how it works. Can you take a look?

zjeffer avatar Jan 05 '24 16:01 zjeffer

@zjeffer just tested: it does not work with my script

Diaoul avatar Jan 05 '24 18:01 Diaoul

It's a bit more complicated than that actually.

  • When starting waybar: it seems to work fine for the first 2 monitors but the workspaces from the 3rd one are displayed on the bar of the 1st. Running my script does nothing since it does not try to move workspaces that are already attached to the right monitor
  • When unplugging / plugging monitors, the issue is solved
  • When I manually move workspaces with hyprctl dispatch moveworkspacetomonitor "3 DP-4" the bar is updated accordingly
  • Running my script after messing up with moveworkspacetomonitor restores the desired layout

So except the issue with the initial setup, it looks fine

Debug logs

Diaoul avatar Jan 05 '24 18:01 Diaoul

Doing some debugging... I have figured out the one issue with the window-rewrite not reloading/applying correctly.... I also see it works when I drag a window across monitors now. (Haven't tried seeing if it fixes windows on initial load during boot). Looks like the css isn't being applied though after reload, so i'm gonna to try and fix that too.

@zjeffer @Syndelis Created https://github.com/Alexays/Waybar/pull/2809 to address the bulk of the issue. Will look into it more tonight or tomorrow for the styling on reload. IT seemed like the empty class was being applied on reload but then would go away when changing workspaces.

khaneliman avatar Jan 05 '24 19:01 khaneliman

@Syndelis @khaneliman @Diaoul I'll give you all write access to my fork, so if you want to make changes to this PR you can do that :)

I'll try to find some time this weekend to look into the bugs.

zjeffer avatar Jan 05 '24 22:01 zjeffer

Hey guys! Coming in late but I'll try reproducing and taking a look at the code later today!

Syndelis avatar Jan 05 '24 22:01 Syndelis

I rebased the branch onto master and fixed the conflicts, but I didn't know what to do with the extendOrphans method. @Syndelis can you help out with this? What does the method do? Does it need to be readded to the initializeWorkspaces function? When should it be called?

zjeffer avatar Feb 04 '24 19:02 zjeffer

I rebased the branch onto master and fixed the conflicts, but I didn't know what to do with the extendOrphans method. @Syndelis can you help out with this? What does the method do?

The method extends the map of orphan windows with the data from the clients JSON and a workspace ID to function as a filter of which windows will be "orphaned".

This map of orphan windows exists to hold all windows that couldn't be assigned to a workspace. For example, in a multi-monitor setup, if "Firefox" is in workspace "1", but workspace "1" only exists in the left monitor's bar, then it will be a orphaned in the right monitor's bar.

The reason we store the orphan windows somewhere in the first place is to avoid fetching the clients JSON everytime a window is moved. Rather than that, we simply "cut" the window from its old workspace and "paste" into the new workspace. If it wasn't in a workspace before, then we can take it from the orphan's map.


Now for the TL;DR:

Does it need to be readded to the initializeWorkspaces function? When should it be called?

Yes, it should be called whenever a workspace is conditionally initialized (i.e. the workspace has been created/already exists on Hyprland, but we're being selective about adding it to this current bar). This includes an else to this if just like it is in master, but also as an else to Workspaces::onWorkspaceCreated's if. The latter isn't currently in master, but I suspect that's precisely why a specific bug exists, so you could either add it now or I could bring it on a PR after this one is merged.


With all this being said, I could add it for you since you granted me permisssions on your repo previously. Otherwise, if you'd prefer to add it yourself, I'll be glad to review the changes

Syndelis avatar Feb 05 '24 15:02 Syndelis

Thanks for the great explanation!

With all this being said, I could add it for you since you granted me permisssions on your repo previously. Otherwise, if you'd prefer to add it yourself, I'll be glad to review the changes

You're welcome to do it if you want to, otherwise I'll probably have a look at adding it sometime next weekend.


As for something else, this PR currently removes the old persistent_workspaces config option. I'm thinking we should maybe still keep the old functionality as well? Otherwise lots of people's bars will break and they might not want to adapt to the workspace_rules way of working...

zjeffer avatar Feb 05 '24 20:02 zjeffer

As for something else, this PR currently removes the old persistent_workspaces config option. I'm thinking we should maybe still keep the old functionality as well? Otherwise lots of people's bars will break and they might not want to adapt to the workspace_rules way of working...

As a way to prevent holding this functionality forever, we could maybe add a red exclamation mark button whenever that config is present, with a hover text saying something in the lines of:

[!CAUTION] The persistent_workspaces configuration has been deprecated in favor of Hyprland's official setting for persistent workspaces. More info here <link to a docpage here on Waybar detailing the issue>

Saying this because most people will run Waybar via another script, so it's likely that logging in the terminal won't be read by many people. What do you think?

Syndelis avatar Feb 06 '24 14:02 Syndelis

I agree it would at least need a visible warning to inform the user the old configuration is incompatible (maybe we should write some kind of global warning system that can be accessed by any module to inform the user of things like this?).

But I think a lot of people are very happy that they can simply do something like this:

"hyprland/workspaces": {
   "*": 5
}

to easily spawn 5 workspaces on every monitor, which even works when you connect new monitors or disconnect them. This PR removes this option.

To do the same thing with this PR, you'd need a plugin like my fork of Duckonaut's split-monitor-workspaces, which is in my opinion kind of a hacky way to set up workspace rules dynamically (like when monitors get (dis)connected).

I'm worried that people might be very annoyed if they have to suddenly install a plugin to accomplish what was so easy before.


As an aside, I really think Hyprland should have a built-in way to dynamically create persistent workspaces, but I feel like very few people seem to need this feature. I think it's stupid how I even need Duckonaut's plugin to set up keybinds so when I press super + 2 it would go to the focused monitor's second workspace. It's the biggest problem I have with Hyprland, so if you or anyone else has a better way to handle all this stuff, please let me know ;)

zjeffer avatar Feb 06 '24 18:02 zjeffer

@Syndelis I added the extendOrphans. Can you double-check whether I did it correctly?

zjeffer avatar Feb 11 '24 16:02 zjeffer

I think I fixed the last bugs. Persistent workspaces configuration now works both in the old way (waybar config) and the new way (Hyprland workspacerules).

@khaneliman @Diaoul can you guys retest too?

zjeffer avatar Feb 18 '24 14:02 zjeffer

Nice work guys! As always :)

Alexays avatar Feb 19 '24 21:02 Alexays

I found a bug related to this PR in cases where the workspaces are not set as persistent in Hyprland but only in Waybar: #2945.

aruhier avatar Feb 20 '24 21:02 aruhier

After reading this PR to fix #2945 (solution proposed in my review of #2946), I have a few problems with the logic here mixing the Waybar config and the Hyprland state.

What about creating 2 attributes in Workspace like bool m_isPersistentConfig and bool m_isPersistentRule , and update Workspace::isPersistent() to return m_isPersistentConfig || m_isPersistentRule so the Waybar config has precedency over the hyprland state?

You can do the same in the workspace_data by splitting the 2.

It would simplify the logic and allow you for example to support the removal of a persistent workspace in Hyprland, in Workspaces::removeWorkspace(). Right now, if I create a workspace in Hyprland, set it as persistent in a Hyprland rule, then remove it in Hyprland (by removing its persistency and emptying all its windows), the workspace will always be considered as persistent in Waybar and not removed. Which makes sense right now, as you have no way to differentiate if the workspace is persistent because of the Waybar config or dynamically through an Hyprland rule.

It would also remove the need of #2946.

aruhier avatar Feb 21 '24 00:02 aruhier

@aruhier I like your proposal! You're welcome to create a PR if you want to, otherwise I'll do it sometime this week.

zjeffer avatar Feb 21 '24 17:02 zjeffer