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

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

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

zjeffer
Copy link
Contributor

@zjeffer zjeffer commented Oct 23, 2023

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: hyprwm/Hyprland@9fba887

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 zjeffer force-pushed the hyprland/persistent-workspaces branch from f02d755 to 074e0fc Compare November 2, 2023 22:18
@zjeffer zjeffer force-pushed the hyprland/persistent-workspaces branch 4 times, most recently from 6aa5622 to c2cfaab Compare December 30, 2023 15:26
@zjeffer zjeffer marked this pull request as ready for review December 30, 2023 15:26
@zjeffer zjeffer force-pushed the hyprland/persistent-workspaces branch 3 times, most recently from 8deff8d to 20213d7 Compare December 30, 2023 15:46
@zjeffer zjeffer changed the title Hyprland/workspaces: clean up persistent workspaces after Hyprland update Hyprland/workspaces: use Hyprland's workspace rules for persistency Dec 30, 2023
@khaneliman
Copy link
Contributor

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

@zjeffer
Copy link
Contributor Author

zjeffer commented Dec 30, 2023

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)

@Diaoul
Copy link

Diaoul commented Dec 31, 2023

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

@zjeffer
Copy link
Contributor Author

zjeffer commented Dec 31, 2023

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?

@khaneliman
Copy link
Contributor

khaneliman commented Jan 4, 2024

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

@zjeffer
Copy link
Contributor Author

zjeffer commented Jan 5, 2024

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?

@Diaoul
Copy link

Diaoul commented Jan 5, 2024

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

@Diaoul
Copy link

Diaoul commented Jan 5, 2024

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

@khaneliman
Copy link
Contributor

khaneliman commented Jan 5, 2024

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 #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.

@zjeffer
Copy link
Contributor Author

zjeffer commented Jan 5, 2024

@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.

@Syndelis
Copy link
Contributor

Syndelis commented Jan 5, 2024

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

@zjeffer zjeffer force-pushed the hyprland/persistent-workspaces branch from 20213d7 to f6a07c9 Compare January 6, 2024 11:37
@zjeffer zjeffer force-pushed the hyprland/persistent-workspaces branch 2 times, most recently from 27a2336 to 2abc7cc Compare February 4, 2024 19:07
@zjeffer
Copy link
Contributor Author

zjeffer commented Feb 4, 2024

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?

@Syndelis
Copy link
Contributor

Syndelis commented Feb 5, 2024

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

@zjeffer
Copy link
Contributor Author

zjeffer commented Feb 5, 2024

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...

@Syndelis
Copy link
Contributor

Syndelis commented Feb 6, 2024

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

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?

@zjeffer
Copy link
Contributor Author

zjeffer commented Feb 6, 2024

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
Copy link
Contributor Author

zjeffer commented Feb 11, 2024

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

Copy link
Contributor

@Syndelis Syndelis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for taking a while, but looks right to me! 🚀

@zjeffer zjeffer force-pushed the hyprland/persistent-workspaces branch from e4b6d6d to 5255c01 Compare February 17, 2024 22:25
@zjeffer
Copy link
Contributor Author

zjeffer commented Feb 18, 2024

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 zjeffer force-pushed the hyprland/persistent-workspaces branch from 5e20f48 to 11310b8 Compare February 18, 2024 15:05
Copy link
Contributor

@khaneliman khaneliman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regression testing with the existing persistent workspace config works fine and I can see in the logs that it is loading the rules from config. Removing my persistent_workspaces from config and see the log output that it's reading from hyprland workspace rules and showing the extra workspace i have as persistent. LGTM

@Alexays
Copy link
Owner

Alexays commented Feb 19, 2024

Nice work guys!
As always :)

@Alexays Alexays merged commit 9abd0da into Alexays:master Feb 19, 2024
7 of 9 checks passed
@zjeffer zjeffer deleted the hyprland/persistent-workspaces branch February 19, 2024 21:59
@aruhier
Copy link
Contributor

aruhier commented Feb 20, 2024

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
Copy link
Contributor

aruhier commented Feb 21, 2024

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.

@zjeffer
Copy link
Contributor Author

zjeffer commented Feb 21, 2024

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

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.

6 participants