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

Persistent workspaces in hyprland/workspaces #2341

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

zjeffer
Copy link
Contributor

@zjeffer zjeffer commented Jul 24, 2023

Adds persistent_workspaces to the hyprland/workspaces module, based on the implementation from wlr/workspaces.

Apart from what's possible with wlr/workspaces (specifying an array of monitors a workspace should be shown on), you can also specify a fixed number of workspaces each monitor should have, like this:

"persistent_workspaces": {
  "*": 3,    // 3 workspaces by default
  "DP-1": 5 // but DP-1 gets 5 workspaces
}

CC: @MonstrousOgre @Anakael @MightyPlaza

@MonstrousOgre
Copy link
Contributor

This is great! Once this is merged, I can start using this over wlr/workspaces

@MightyPlaza
Copy link
Contributor

I was not using window_, so I just created the var and parsed it on start.
We need to listen to windowCreate and windowDestroy (or more) to keep track of window count

@zjeffer
Copy link
Contributor Author

zjeffer commented Jul 24, 2023

I opted to update all workspace's window counts whenever a window opens or closes, as I can't really see another way to do it. Seems to work pretty well, though.

@zjeffer zjeffer marked this pull request as ready for review July 24, 2023 13:00
@MightyPlaza
Copy link
Contributor

It's possible to listen to (openwindow, movewindow, closewindow) but you'd have to keep track of which window are where for it to work.

@zjeffer
Copy link
Contributor Author

zjeffer commented Jul 24, 2023

Seems to work pretty well, though.

Nevermind, still a small bug when moving a window to another workspace (even when also listening to movewindow): the workspace is not seen as empty until I go back to the empty workspace.

For example, here I move a window from workspace 1 to 4, one by one:

2023-07-24.15-12-24.mp4

A white background means the workspace is not empty, grey means persistent and empty.

@zjeffer
Copy link
Contributor Author

zjeffer commented Jul 24, 2023

Ok I think I fixed it for real this time: now it will update the window counts on openwindow, closewindow, and movewindow, and set it to 0 when the json received from hyprland's IPC does not include the workspace (meaning it is a persistent workspace).

Ready for review.

@MonstrousOgre
Copy link
Contributor

What about listening for destroyworkspace to know if it's empty? Don't empty workspaces get destroyed when not focused?

src/modules/hyprland/workspaces.cpp Outdated Show resolved Hide resolved
src/modules/hyprland/workspaces.cpp Outdated Show resolved Hide resolved
src/modules/hyprland/workspaces.cpp Outdated Show resolved Hide resolved
src/modules/hyprland/workspaces.cpp Outdated Show resolved Hide resolved
@zjeffer zjeffer requested a review from MightyPlaza July 26, 2023 21:07
src/modules/hyprland/workspaces.cpp Outdated Show resolved Hide resolved
active_workspace_name_ = (gIPC->getSocket1JsonReply("activeworkspace"))["name"].asString();

// get monitor ID from name (used by persistent workspaces)
monitor_id_ = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

why use id here instead of desc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I use this ID to calculate the IDs the persistent workspaces should get.

I forked Duckonaut's split-monitor-workspaces Hyprland plugin to make it possible to automatically set up keyboard shortcuts to switch to the xth workspace on the current focused monitor. The plugin calculates the IDs the workspaces should get based on the monitor ID, and here I used the same formula (because the IDs have to match).

Copy link
Contributor

Choose a reason for hiding this comment

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

would make more sense to use workspace name:<monitor-desc-here>_<workspace-number-here>, monitor id can change on restart but monitor desc is persistent.
Also it's more consistent with the rest of hyprland

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But how can I then ensure both waybar and the plugin give the same name to the workspaces?

Copy link
Contributor

Choose a reason for hiding this comment

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

waybar should only use the name you set in the config file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if you want waybar to work with any monitor you connect, without having to specify the name in the config file?

For example, if I connect my laptop to a monitor it has never seen before, waybar and my fork of the plugin will work together to automatically show persistent workspaces and set up the correct keybinds.

If we did it with monitor descriptions, you would have to do all that stuff manually.

Copy link
Contributor

@MightyPlaza MightyPlaza Jul 28, 2023

Choose a reason for hiding this comment

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

use the port name then, monitor desc is for when you can't use that

Copy link
Contributor Author

@zjeffer zjeffer Jul 29, 2023

Choose a reason for hiding this comment

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

I don't understand why it would be any different with the port name instead of the monitor description. I would still have to manually specify the port name in the configs.

In the plugin, I use the formula workspace_name = (monitor_ID * amount_of_workspaces) + index + 1. Here, I use the same formula so that any monitor I connect, both the plugin and the monitor will assign the same index to the same workspace ID (with 5 workspaces per monitor, the 4th workspace on the monitor with ID 2 will have workspace ID = 2*5 + 3 + 1 = 14).

I don't see how I can implement the same functionality with port names or monitor desc. I'm open to suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

simply use named workspaces, containing the monitor name and a number (name:<monitor-desc-here>_<workspace-number-here>)
this way you wouldn't rely on using initial id, which depends on the order you connect the monitors

if you want every monitor you connect on DP-1 (for example) to have the same configuration you would use DP-1 in the config
if you want each monitor you connect on DP-1 (for example) to have a different config you could use the monitor description

using id gives you much less control than either of these options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm starting to understand. But I think changing it to use monitor descriptions might be more appropriate for another PR, as a lot of things would need to be refactored for it to work. Not to mention the changes I would have to make to the plugin.

I think we can merge this if you agree? CC @Alexays

src/modules/hyprland/workspaces.cpp Outdated Show resolved Hide resolved
src/modules/hyprland/workspaces.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@MightyPlaza MightyPlaza left a comment

Choose a reason for hiding this comment

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

LGTM

@zjeffer zjeffer force-pushed the hyprland/workspaces branch from b9cb08b to 6006535 Compare July 30, 2023 10:10
@Alexays Alexays merged commit 86b3e45 into Alexays:master Jul 31, 2023
@zjeffer zjeffer deleted the hyprland/workspaces branch July 31, 2023 17:11
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