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

Initial implementation of named workspaces #314

Conversation

algernon
Copy link
Contributor

@algernon algernon commented Apr 27, 2024

This is an implementation of named, pre-declared workspaces. With this implementation, workspaces can be declared in the configuration file by name:

workspace "name" {
  open-on-output "winit"
}

The open-on-output property is optional, and can be skipped, in which case the workspace will open on the primary output.

All actions that were able to target a workspace by index can now target them by either an index, or a name. In case of the command line, where we do not have types available, this means that workspace names that also pass as u8 cannot be switched to by name, only by index.

Unlike dynamic workspaces, named workspaces do not close when they are empty, they remain static. Like dynamic workspaces, named workspaces are bound to a particular output. Switching to a named workspace, or moving a window or column to one will also switch to, or move the thing in question to the output of the workspace.

When reloading the configuration, newly added named workspaces will be created, and removed ones will lose their name. If any such orphaned workspace was empty, they will be removed. If they weren't, they'll remain as a dynamic workspace, without a name. Re-declaring a workspace with the same name later will create a new one.

Additionally, this also implements a open-on-workspace "<name>" window rule. Matching windows will open on the given workspace (or the current one, if the named workspace does not exist).

@algernon
Copy link
Contributor Author

I have been daily driving this for most of the day, and it is working surprisingly well. For my own needs, this is fine as it is, but I'll try to add a few tests in the coming days.

@algernon algernon force-pushed the you-have-a-name-and-you-have-a-name-and-everyone-has-a-name branch from 8e8043d to dec887a Compare May 2, 2024 16:01
Copy link
Owner

@YaLTeR YaLTeR left a comment

Choose a reason for hiding this comment

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

This looks very reasonable!

Maybe consider making it possible to declare named workspaces on a per-output basis (while also keeping the global declaration, which will create them on the primary display).

Maybe something like:

workspace "name" {
    open-on-output "DP-2"
}

niri-config/src/lib.rs Outdated Show resolved Hide resolved
niri-config/src/lib.rs Outdated Show resolved Hide resolved
src/layout/monitor.rs Outdated Show resolved Hide resolved
@algernon
Copy link
Contributor Author

algernon commented May 4, 2024

This looks very reasonable!

Maybe consider making it possible to declare named workspaces on a per-output basis (while also keeping the global declaration, which will create them on the primary display).

Maybe something like:

workspace "name" {
    open-on-output "DP-2"
}

Yeah, I suppose that'd work better than output "DP-2" { workspace "foo"; workspace "bar"; }, indeed. Feels more natural for the niri config, yup. Will have to think a bit about how to implement that, but... I have an idea.

@algernon
Copy link
Contributor Author

algernon commented May 4, 2024

Pushed a WIP update that changes the option name and syntax, it's now:

workspace "name" {
  open-on-output "eDP-1";
}
workspace "some-other-name"

...and it actually does as advertised! It does open the "name" workspace on the primary output if eDP-1 is not present, but if/when that appears, it will move it there. (I'm (ab)using Workspace::original_output)

However, this whole thing falls apart in a multi-monitor setup, because workspaces are searched per-monitor. So focus-named-workspace name will do nothing if it is initiated from an output other than eDP-1 (assuming eDP-1 is present, and the workspace is there). It really needs to be made into a global search, spanning monitors.

I had an attempt at that last week, but it got a wee bit messy. I don't remember anymore where, or how, though. I'll try again tomorrow, and see where it leads.

@algernon algernon force-pushed the you-have-a-name-and-you-have-a-name-and-everyone-has-a-name branch 2 times, most recently from 9243d63 to e094db5 Compare May 5, 2024 11:47
@algernon
Copy link
Contributor Author

algernon commented May 5, 2024

Updated the PR with plenty of changes - also reflected in the updated initial commit.

I am not sure how much further I can push this, the remaining todo list items feel like they require a deeper knowledge of Rust and Niri than I have. Conceptually, none of them feel hard, but I've been fighting the borrow checker and the compiler for quite a bit to even get here. I'm not sure I have it in me to go much further.

Time permitting, I will have another go at the remaining stuff next weekend. For now, I'll go and test drive the changes I made in the past 24 hours =)

@algernon algernon force-pushed the you-have-a-name-and-you-have-a-name-and-everyone-has-a-name branch 2 times, most recently from 83ccd28 to 256013e Compare May 7, 2024 21:19
@algernon
Copy link
Contributor Author

algernon commented May 7, 2024

Just pushed an update that changes the syntax from workspace "name" { ... } to workspaces { "name" { .... } }, because with this syntax, it was much easier to enforce name uniqueness at parse time. I also think it feels more natural with sway, too: plenty of other configuration parts are grouped similarly (inputs, outputs, binds, etc).

@algernon
Copy link
Contributor Author

algernon commented May 8, 2024

Just pushed an update that changes the syntax from workspace "name" { ... } to workspaces { "name" { .... } }, because with this syntax, it was much easier to enforce name uniqueness at parse time. I also think it feels more natural with sway, too: plenty of other configuration parts are grouped similarly (inputs, outputs, binds, etc).

Following the discussion on matrix, I changed the syntax back to workspace "name", and moved the uniqueness validation to a post-parse validation step. This results in less accurate reporting (since niri can't report the offending line anymore), but lets us use the originally envisioned syntax. It's also less code.

Technically it would still be possible to report it at parse time, using ctx.get() & ctx.set() when decoding workspace names, but that felt a bit clunky. I'll give it another try later, but the post-parse validation is not bad.

@algernon
Copy link
Contributor Author

algernon commented May 8, 2024

Technically it would still be possible to report it at parse time, using ctx.get() & ctx.set() when decoding workspace names, but that felt a bit clunky. I'll give it another try later, but the post-parse validation is not bad.

I had another go at it. It's still a bit clunky and verbose, but the parse-time error reporting is nice. And we can stick to the current syntax, too!

@algernon algernon force-pushed the you-have-a-name-and-you-have-a-name-and-everyone-has-a-name branch 2 times, most recently from ba9c2e2 to 37711d1 Compare May 11, 2024 14:26
@algernon
Copy link
Contributor Author

I think that all functionality is in place now. The code is a tad on the ugly and inefficient side here and there, but things generally work. I'm going to test drive it today, and hopefully write some tests & clean up the code and commit history tomorrow.

@algernon algernon force-pushed the you-have-a-name-and-you-have-a-name-and-everyone-has-a-name branch from 9da6f81 to d8ad6f2 Compare May 11, 2024 20:51
@algernon
Copy link
Contributor Author

I've been daily driving this for the past three weeks, the latest iteration for the past ~6 hours or so, and am quite happy with where it is at. It's not perfect, some of the code I'm not really proud of, especially around the open-on-workspace window rule parts, but this is the best I can do with my limited Rust knowledge at this time.

I have added a few invariants to layout.verify_invariants() that should hold at all times. I think there's not a lot more that can be added there.

There's also the question of being able to add/remove named workspaces at runtime, without adding them to the configuration file. I think this would be great for temporary named workspaces, and for experimentation. I did not implement those yet, because this PR is already big and complex, and run-time named workspaces would, I fear, open another can of worms. I do plan to implement those, but in a followup pull request instead.

I wanted to write tests too, but... those parts of the code are way beyond my Rust skills. I'm afraid I will not be able to write tests. Best I can do is describe what I would test, but someone else will need to turn that into code.

With all that said, I think this is as far as I can take it, so I'll be marking it as ready for review.

@algernon algernon marked this pull request as ready for review May 11, 2024 21:03
@algernon algernon requested a review from YaLTeR May 11, 2024 21:03
Copy link
Owner

@YaLTeR YaLTeR left a comment

Choose a reason for hiding this comment

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

This is looking great! I don't have any big architectural changes; only smaller fixes and refactors.

niri-config/src/lib.rs Show resolved Hide resolved
niri-ipc/src/lib.rs Show resolved Hide resolved
niri-ipc/src/lib.rs Outdated Show resolved Hide resolved
niri-config/src/lib.rs Outdated Show resolved Hide resolved
niri-ipc/src/lib.rs Outdated Show resolved Hide resolved
src/layout/mod.rs Outdated Show resolved Hide resolved
src/niri.rs Outdated Show resolved Hide resolved
src/niri.rs Outdated Show resolved Hide resolved
src/niri.rs Outdated Show resolved Hide resolved
src/window/unmapped.rs Show resolved Hide resolved
@YaLTeR YaLTeR force-pushed the you-have-a-name-and-you-have-a-name-and-everyone-has-a-name branch from 4bfa58c to 9fdbdb1 Compare May 16, 2024 07:23
@YaLTeR
Copy link
Owner

YaLTeR commented May 16, 2024

Barring dogfooding, this should now be ready. I put all my changes into separate commits for now.

algernon and others added 3 commits May 16, 2024 12:13
This is an implementation of named, pre-declared workspaces. With this
implementation, workspaces can be declared in the configuration file by
name:

```
workspace "name" {
  open-on-output "winit"
}
```

The `open-on-output` property is optional, and can be skipped, in which
case the workspace will open on the primary output.

All actions that were able to target a workspace by index can now target
them by either an index, or a name. In case of the command line, where
we do not have types available, this means that workspace names that
also pass as `u8` cannot be switched to by name, only by index.

Unlike dynamic workspaces, named workspaces do not close when they are
empty, they remain static. Like dynamic workspaces, named workspaces are
bound to a particular output. Switching to a named workspace, or moving
a window or column to one will also switch to, or move the thing in
question to the output of the workspace.

When reloading the configuration, newly added named workspaces will be
created, and removed ones will lose their name. If any such orphaned
workspace was empty, they will be removed. If they weren't, they'll
remain as a dynamic workspace, without a name. Re-declaring a workspace
with the same name later will create a new one.

Additionally, this also implements a `open-on-workspace "<name>"` window
rule. Matching windows will open on the given workspace (or the current
one, if the named workspace does not exist).

Signed-off-by: Gergely Nagy <niri@gergo.csillger.hu>
@YaLTeR YaLTeR force-pushed the you-have-a-name-and-you-have-a-name-and-everyone-has-a-name branch from 9fdbdb1 to 92c68bd Compare May 16, 2024 08:16
@YaLTeR
Copy link
Owner

YaLTeR commented May 16, 2024

Added docs.

@YaLTeR YaLTeR enabled auto-merge (rebase) May 16, 2024 08:16
@YaLTeR YaLTeR merged commit 4e31f7e into YaLTeR:main May 16, 2024
9 checks passed
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.

None yet

2 participants