-
-
Notifications
You must be signed in to change notification settings - Fork 724
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
Hyprland/workspaces: use Hyprland's workspace rules for persistency #2603
Conversation
f02d755
to
074e0fc
Compare
6aa5622
to
c2cfaab
Compare
8deff8d
to
20213d7
Compare
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 |
Things that still need to be tested:
|
I actually think this is a pretty frequent use case and it's how I currently use it and it works. |
I see: so you move the workspaces using a script, and also create new workspace rules. Can you check if your config and script works with this branch? |
I think @Syndelis wrote the window rewrite stuff, I don't know much about how it works. Can you take a look? |
@zjeffer just tested: it does not work with my script |
It's a bit more complicated than that actually.
So except the issue with the initial setup, it looks fine |
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. |
@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. |
Hey guys! Coming in late but I'll try reproducing and taking a look at the code later today! |
20213d7
to
f6a07c9
Compare
27a2336
to
2abc7cc
Compare
I rebased the branch onto master and fixed the conflicts, but I didn't know what to do with the |
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:
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 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 |
Thanks for the great explanation!
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... |
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 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? |
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 ;) |
@Syndelis I added the extendOrphans. Can you double-check whether I did it correctly? |
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.
Sorry for taking a while, but looks right to me! 🚀
e4b6d6d
to
5255c01
Compare
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? |
5e20f48
to
11310b8
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.
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
Nice work guys! |
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. |
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 You can do the same in the It would simplify the logic and allow you for example to support the removal of a persistent workspace in Hyprland, in It would also remove the need of #2946. |
@aruhier I like your proposal! You're welcome to create a PR if you want to, otherwise I'll do it sometime this week. |
Hyprland now has a workspace rule
persistent
.This PR has the following changes:
hyprctl workspacerules -j
)Example Hyprland config:
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.