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

Changed the workspace separation #19

Closed
wants to merge 1 commit into from
Closed

Changed the workspace separation #19

wants to merge 1 commit into from

Conversation

paolobettelini
Copy link

@paolobettelini paolobettelini commented Jun 19, 2023

Hello. So I installed the plugin and it did not just work. Going to a workspace would just go to another monitor and create a mess. I think this is because i have monitors with IDs (from left to right) 0, 2, 1 rather than 0, 1, 2. (I don't know why that is, but this plugin should work nonetheless).

Furthermore, the plugin uses g_vMonitorWorkspaceMap to keep track of workspaces, which could totally be avoided.

My solution is to compute the workspace as such: if you are on monitor ID=N and want to go to workspace W, the actual workspace is given by (W - 1) * monitors + N + 1. This means that 1, 2 and 3 are the first workspace of each monitor, 4, 5and 6 are the second workspace of each monitor and so on.
At initialization, or when a monitor is added/removed, the formula for the workspace is reversed and every workspace is moved to its correct monitor.

This solution is more simple and the workspace computation can be implemented elsewhere.
@zjeffer I think some of your problems may be solved by using this approach.

Please test the code as I only tested it with my weird monitor IDs, but it should work regardless of the ID order.
I also think it will be easier to implement features such as previous and m+1 (those things) since everything is just a simple computation. Tell me what you think :)

@zjeffer
Copy link
Collaborator

zjeffer commented Jun 19, 2023

Hello. So I installed the plugin and it did not just work. Going to a workspace would just go to another monitor and create a mess. I think this is because i have monitors with IDs (from left to right) 0, 2, 1 rather than 0, 1, 2. (I don't know why that is, but this plugin should work nonetheless).

Strange. This works fine for me. Do you still have workspace= or wsbind= in your hyprland config? I removed mine, as this plugin should take care of them.

Furthermore, the plugin uses g_vMonitorWorkspaceMap to keep track of workspaces, which could totally be avoided.

I think it makes more sense to use a map to keep track of workspaces, like how it works now.

This solution is more simple and the workspace computation can be implemented elsewhere.
(...)
I also think it will be easier to implement features such as previous and m+1 (those things) since everything is just a simple computation

But the way it works now does not use a computation at all: it's just +1 and -1?

Your code didn't really work for me. My monitors had these mappings which look very strange:

  • Monitor 1 => Workspaces [1, 4, 7, 10, 13]
  • Monitor 2 => Workspaces [3, 6, 9, 12, 15]
  • Monitor 3 => Workspaces [2, 5, 8, 11, 14]

I would expect these to be [1-5], [6-10], [11-15]. Then you don't need to tell other programs to use the same formula: it makes sense by just looking at the numbers.

@paolobettelini
Copy link
Author

paolobettelini commented Jun 19, 2023

Strange. This works fine for me. Do you still have workspace= or wsbind= in your hyprland config? I removed mine, as this plugin should take care of them.

I don't have any of those.

I think it makes more sense to use a map to keep track of workspaces, like how it works now.

Well, we disagree. Obviously I don't want to enforce my fork, you guys do what you want. Personally I don't see how keeping a map is any logical and necessary. Given the monitor you're on and the workspace you want to go to, the actual workspace is given by a simple function f(W, M). There is no need to allocate extra memory.

Your code didn't really work for me. My monitors had these mappings which look very strange:

What do you mean it didn't work? Does it not switch the workspaces correctly? The mapping are correct. Your monitors clearly have IDs (0, 2, 1) (like mine). My method does not work with separating the workspaces into "[1-5], [6-10], [11-15]", it's a completely different approach.

Say you have 3 monitors (well, we both do).
The workspaces 1,2,3 represent the first workspace for each monitor. The workspaces 4,5,6 represent the second workspaces for each monitors. The mapping you have are exactly that. The only weird thing is that, since the IDs are not 0,1,2 but 0,2,1 the workspaces are 1,3,2 rather than 1,2,3, but it doesn't matter.

IMO, this approach is easier. It does not require extra memory allocation and you can go as far as you want. You're not bound to allocate the workspaces 1-10 for the first monitors, you can go as much as you want since the workspaces are distribuited evenly as the number goes up.

Again, I don't want to enforce anything on anybody. I just think this is a better and more flexible approach. Please let me know if the workspaces were all a mess or they did actually work, even though you thought the mappings were strange.

Then you don't need to tell other programs to use the same formula: it makes sense by just looking at the numbers

Well in the end you still have to tell it something. I think it's just easier with the formula. The main difference is that right now you also need to tell other programs the workspace count. With my approach you don't have to tell what the workspace count is (well, unless you need it for something else like displaying an icon for each monitor), it doesn't matter since you have the formula

@zjeffer
Copy link
Collaborator

zjeffer commented Jun 19, 2023

I see where you're coming from, but I feel like it makes more sense to me that each monitor has workspaces that follow each other up: with the current implementation, you know for a fact that the next workspace on your current monitor is current_workspace + 1, while with your method you need to see how many monitors you have, to know what to put in the formula.

Personally I don't see how keeping a map is any logical and necessary. Given the monitor you're on and the workspace you want to go to, the actual workspace is given by a simple function f(W, M). There is no need to allocate extra memory.

  • If your formula ever changes, you would have to change the formula in every other application you use it in (like waybar).
  • We could also pass the map to other applications (like Waybar) with something like FIFO or sockets or whatever.
  • A small map really doesn't allocate much memory, right? I know very little about memory allocation so I don't see the problem.

What do you mean it didn't work? Does it not switch the workspaces correctly? The mapping are correct. Your monitors clearly have IDs (0, 2, 1) (like mine). My method does not work with separating the workspaces into "[1-5], [6-10], [11-15]", it's a completely different approach.

I think I misunderstood your formula, and it looked wrong to me when I tried it out, sorry.

But I still feel it needlessly complicates things. For example: when you add another monitor, and the mappings are refreshed, the numbering has to be updated again, right? For 1-5, 6-10 and 11-15, this is easy to change: you just add 16-20. But for your approach, you now have to update every single monitor in the Waybar config (unless someone improves the persistent_workspace functionality), because the calculation is dependent on the number of monitors.

I'd love to hear your thoughts.

@paolobettelini
Copy link
Author

paolobettelini commented Jun 19, 2023

Ok so I want to start off by saying that both methods are valid.

with the current implementation, you know for a fact that the next workspace on your current monitor is current_workspace + 1

Yes, and with my method the next is current_workspace + monitors.

If your formula ever changes, you would have to change the formula in every other application you use it in (like waybar).

I think this point is nonsense. Both my method and yours have a formula. Yours has the formula workspace_count * current_monitor + workspace. This is the reason why I believe g_vMonitorWorkspaceMap should be completly ditched even in the current version of the code. You don't need to store everything is a matrix.

We could also pass the map to other applications (like Waybar) with something like FIFO or sockets or whatever.

Sorry but this is just obnoxious. As previously stated, other applications can compute your workspaces as workspace_count * current_monitor + workspace. The difference is that with your method the applications necessarily needs to know workspace_count and with mine the numbers of monitors.

A small map really doesn't allocate much memory, right? I know very little about memory allocation so I don't see the problem.

You are absolutely correct, it isn't much, but you don't really need it at all. There really is no good argument that I can find for g_vMonitorWorkspaceMap to exist. You just don't need it, so it's just bad practice to allocate it.

when you add another monitor, and the mappings are refreshed, the numbering has to be updated again

You make a great compelling point with this. Indeed, this is easier to handle with your method.

You may have changed my mind a bit. The main reason why I believe or believed my approach was "cleaner", is because it works with the default 1,2,3 workspaces. Your method does not work with the default 1,2,3 and required to spawn the workspaces 1,11,21 (with count=10), which, to me is a bit "dirty". My approach does not ever need to spawn extra workspaces because of the pigeonhole principle. This is my argument on why I personally prefer my approach, I think it's easier to handle.

As I said, I believe both approaches are completely valid. This was a productive discussion and if you want I can change my code to the 1-10,11-20,21-30 approach. But please consider ditching g_vMonitorWorkspaceMap. There really isn't a good argument for it to exist. The logic and code is much simplier and cleaner if you just compute actual_workspace = workspace_count * current_monitor + workspace and monitor = actual_workspace / workspace_count.

@zjeffer
Copy link
Collaborator

zjeffer commented Jun 20, 2023

As I said, I believe both approaches are completely valid. This was a productive discussion and if you want I can change my code to the 1-10,11-20,21-30 approach. But please consider ditching g_vMonitorWorkspaceMap. There really isn't a good argument for it to exist. The logic and code is much simplier and cleaner if you just compute actual_workspace = workspace_count * current_monitor + workspace and monitor = actual_workspace / workspace_count.

I should mention I have no affiliation with this repo, I'm simply an interested party like you ;) Apologies for not making this clear before.

I think the main reason I preferred having a map saved in memory is to allow other applications to "ask" the plugin what the monitor state is, which would be useful for solving this issue I created in the Waybar repo: Alexays/Waybar#2243. But I realize now that even for that use-case, you don't need to hold it in memory at all and can just calculate it on the spot (in the application itself or in the plugin).

I wonder what @Duckonaut thinks?

@paolobettelini
Copy link
Author

But I realize now that even for that use-case, you don't need to hold it in memory at all and can just calculate it on the spot (in the application itself or in the plugin).

Exactly! I think this is well-established now that we agree on it.

It remains to decide which workspace numbering approach to use.
My main argument is the the 1-11-21 method does not work with the default workspaces or workspaces spawned by hyprland, it needs to spawn extra/specific workspaces, whilst mine will always work without spawning extra workspaces. On the other hand, a naive implementation of my approach messes up everything when a monitor is added, so this should be addressed.

I don't think it matters that much which one we use, but if we settle we can start implementing other features like previous m+2 command and such. Personally this was the first C++ code I ever wrote so I'm not sure I could do the setup for those, but the logic is simple nevertheless.

@zjeffer
Copy link
Collaborator

zjeffer commented Jun 20, 2023

I don't think it matters that much which one we use, but if we settle we can start implementing other features like previous m+2 command and such

There's an open issue for this, but I don't understand why. From my tests, the built-in hyprctl dispatch workspace ... commands work perfectly, why would we need to implement our own implementation?

@paolobettelini
Copy link
Author

It does not work perfectly. The built-in command goes to the next workspaces, yes. Since the plugin uses the 1-10, 11-20, 21-30 approach, the next workspace is usually w+1, but if the workspace is the last one for this monitor, then you will be moved to a different monitor, which is wrong because you're going into a range that isn't yours. And if we settle on my 1-2-3 approach, then the next monitor is not w+1.

Btw, is there a way to override the built-in command or is split-workspace necessary?

@zjeffer
Copy link
Collaborator

zjeffer commented Jun 20, 2023

Btw, is there a way to override the built-in command or is split-workspace necessary?

Not to my knowledge, I think with Hyprland's plugin implementation, you can only add dispatchers, not change them.

@Duckonaut
Copy link
Owner

Sorry for taking this long to respond, but I'm not really a fan of the change. I see how the different indexes could be useful to your workflow, but it just fundamentally changes the idea of the plugin (and breaks my personal scripts for eww 🕴️). I haven't personally tested this with 3 monitors, but if issues like jumping to different monitors than expected happen consistently, I'm sure that can be solved in another way. Memory for a map of 20-30 elements is a non-issue, I don't see a reason to remove the map.

I'll keep this open for discussion but for now no plan to adopt it.

@zjeffer
Copy link
Collaborator

zjeffer commented Sep 13, 2023

I keep my fork updated with my own implementation, if you want to have a look: https://github.com/zjeffer/split-monitor-workspaces

I understand if you don't want the same behaviour, though. But I like the automated aspect of it :)

@Duckonaut
Copy link
Owner

interesting fork, it seems pretty far away from what's currently here, but yeah i can see it. Maybe I'll get around to doing something similar when i decide to refactor the whole thing. But for now indexing and map probably stay where they are

@zjeffer
Copy link
Collaborator

zjeffer commented Sep 13, 2023

The nice thing is I adapted Waybar to use the same formula as the plugin, so the workspaces that are bound automatically by the plugin will match the ones in waybar perfectly, even when disconnecting/reconnecting monitors, or connecting new monitors.

I can just use the following Waybar config for persistent_workspaces:

"persistent_workspaces": {
    "*": 5 // put 5 workspaces on each monitor, with same formula as the plugin
}

@elythh
Copy link

elythh commented Feb 23, 2024

The nice thing is I adapted Waybar to use the same formula as the plugin, so the workspaces that are bound automatically by the plugin will match the ones in waybar perfectly, even when disconnecting/reconnecting monitors, or connecting new monitors.

@zjeffer do you mind sharing how you did that ? I'd love to do something similar. The only solution I found right now is having duplicated waybar config changing the persistent_workspaces per output.
Thing is I can have like 4 different monitors setup in a single week so changing the output in the waybar config each time is really time consuming and annoying.

EDIT: Oh so you forked it, awesome

@zjeffer
Copy link
Collaborator

zjeffer commented Feb 23, 2024

Thing is I can have like 4 different monitors setup in a single week so changing the output in the waybar config each time is really time consuming and annoying.

I have the exact same problem, and this seems to be an issue for no one else while it has been driving me crazy since I started using Hyprland over a year ago. Glad to hear I'm not the only one frustrated with this.

EDIT: Oh so you forked it, awesome

Keep in mind my changes to waybar have already been merged into master, you don't have to use my fork for that. I also recently added support for Hyprland's workspace rules, so if you want to define your persistent workspaces in your Hyprland config instead of the waybar config, that's also possible (make sure you don't mix them, though!).

What makes it work the way I want to is my fork of this repo, which automatically writes those Hyprland persistent workspaces into my Hyprland config. It also has some other improvements, like ensuring the workspaces are always mapped the same way they are on Waybar, it has more logging, it listens to the configReloaded event so that whenever the config changes, the workspaces are kept up to date, etc...

@elythh
Copy link

elythh commented Feb 23, 2024

if you want to define your persistent workspaces in your Hyprland config instead

Thing is I don't want them to be in my hyprland config. Its config is managed with home-manager and since I'm always changing monitors, monday DP-3 might be connected and at 0x0 and the following day may not even by attached or at a different position.

I can't really afford to dynamically update the config, except if I decide to re-run home-manager each time the tmp file created by your fork changes

@elythh
Copy link

elythh commented Feb 23, 2024

When I tried you waybar fork with Duckonaut's split-monitor-workspaces, it seemed to be working perfectly fine. Only issue I have is that the persistent workspaces get deleted if i switch to them and switch back to another one while them still being empty. So I ended up with only the 5 workspaces at boot but only the actives ones (for the current monitors) shortly after

@zjeffer
Copy link
Collaborator

zjeffer commented Feb 23, 2024

When I tried you waybar fork with Duckonaut's split-monitor-workspaces, it seemed to be working perfectly fine. Only issue I have is that the persistent workspaces get deleted if i switch to them and switch back to another one while them still being empty. So I ended up with only the 5 workspaces at boot but only the actives ones (for the current monitors) shortly after

See this PR for a fix for that: Alexays/Waybar#2946

@elythh
Copy link

elythh commented Feb 24, 2024

See this PR for a fix for that: Alexays/Waybar#2946

Thank you !

Indeed it seems I can't read tonight. You're right, persistent workspace is a thing now in waybar,
I don't need your fork.
Also, that's actually odd, the thing I described above doesn't seem to be happening with their ?
Anyway, I'm glad I found someone with the same struggles that has been able to help me

@paolobettelini paolobettelini closed this by deleting the head repository Mar 3, 2024
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.

4 participants