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

Allow workspace names to be changed dynamically #904

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

rustn00b
Copy link
Contributor

A trivial PR that adds set-workspace-name and unset-workspace-name actions.

This is really nifty when combined with the Waybar Niri workspace module.

niri-ipc/src/lib.rs Outdated Show resolved Hide resolved
niri-ipc/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 3398 to 3399
// ignore request if the name is an integer
if name.parse::<usize>().is_ok() { return }
Copy link
Owner

Choose a reason for hiding this comment

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

Is this needed? I believe you can name workspaces as numbers in the config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was intended to avoid confusion with other auto-named workspaces. Having two different namespaces keeps things simple. I'm not sure how things are dealt with (possibly very well ;-) when a wkspc is automatically assigned a value that might already be used as an assigned name.

src/layout/mod.rs Outdated Show resolved Hide resolved
src/layout/mod.rs Outdated Show resolved Hide resolved
src/layout/mod.rs Outdated Show resolved Hide resolved
@@ -3394,6 +3394,26 @@ impl<W: LayoutElement> Layout<W> {
monitor.move_workspace_up();
}

pub fn set_workspace_name(&mut self, name: String) {
Copy link
Owner

Choose a reason for hiding this comment

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

Also please add these two operations to the Op enum below so they can participate in the randomized tests.

Copy link
Contributor Author

@rustn00b rustn00b Jan 3, 2025

Choose a reason for hiding this comment

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

The two tests I put in are kind of trivial, they may not clear your bar for completeness/quality.

@rustn00b rustn00b marked this pull request as draft January 3, 2025 14:03
@rustn00b rustn00b marked this pull request as ready for review January 3, 2025 14:34
@rustn00b rustn00b marked this pull request as draft January 3, 2025 17:11
@rustn00b
Copy link
Contributor Author

rustn00b commented Jan 3, 2025

There is an issue when the last workspace has its name set. Based on asserts in test cases, the last item in any monitor's workspace vec must be an unnamed workspace. With the as-is PR, this property is no longer always verified.
I'll fix that and then switch the PR status back to ready-for-review.

@rustn00b rustn00b marked this pull request as ready for review January 4, 2025 19:23
niri-ipc/src/lib.rs Show resolved Hide resolved
niri-ipc/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/layout/mod.rs Outdated Show resolved Hide resolved
Comment on lines 393 to 395
pub fn unname_workspace_with_filter<P>(&mut self, pred: P) -> bool
where
P: Fn(&Workspace<W>) -> bool,
Copy link
Owner

Choose a reason for hiding this comment

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

How about instead of accepting a generic predicate here and in set_workspace_name(), just always accept a WorkspaceId? I think it should make things simpler. And it's not like this can ever be called for multiple workspaces at once (and even then, it would be safer to call it for them one-by-one).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried something, can't say I'm really convinced it's much better. Maybe I'm not looking at it the right way.

@rustn00b rustn00b marked this pull request as draft January 5, 2025 09:57
@rustn00b rustn00b marked this pull request as ready for review January 5, 2025 14:56
@YaLTeR
Copy link
Owner

YaLTeR commented Jan 9, 2025

Pushed a few cleanups, should be good to merge now. Do you mind testing one more time?

@rustn00b
Copy link
Contributor Author

rustn00b commented Jan 9, 2025

Pushed a few cleanups, should be good to merge now. Do you mind testing one more time?

Will do!

@YaLTeR YaLTeR enabled auto-merge (squash) January 10, 2025 05:50
@YaLTeR
Copy link
Owner

YaLTeR commented Jan 10, 2025

One final change, made the workspace reference a non-positional option to make it less confusing what goes where (i wrote that commit message the opposite way, huh). Let's merge this

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.

2 participants