Reverse `switch-preset-column-width` by mattkang · Pull Request #1367 · niri-wm/niri

mattkang · 2025-03-31T04:16:42Z

Support switch-preset-column-width in reverse order as well as switching presets without wraparound. Implements #1000

Can specify number of steps and whether to wraparound

mattkang · 2025-03-31T04:19:40Z

@YaLTeR I have added some methods to support this feature, but I'm now unclear as to how to implement this in the IPC interface. Help appreciated thanks.

YaLTeR · 2025-03-31T04:42:31Z

niri-config/src/lib.rs

+    SwitchPresetColumnWidthNext,
+    SwitchPresetColumnWidthPrev,


As we discussed, instead of two new commands it should be just SwitchPresetColumnWidthBack, and both of them should have a #[knuffel(property)] bool cycle argument.

And on the ipc/lib.rs it would be --cycle false. See the recent --focus false on MoveWindowToMonitor

YaLTeR · 2025-03-31T04:43:07Z

src/layout/scrolling.rs

+        self.preset_width_idx = Some(preset_idx);
+    }
+
+    fn cycle_width(&mut self, steps: i32, tile_idx: Option<usize>, wraparound: bool) {


Why did you make this generic by steps? This overcomplicates the logic below quite a bit. Just pass another back: bool or something

I will make an attempt to cleanup logic. Wasn't able to immediately find contributing guidelines. Do you want me to fix via squashing or keep everything as new commits?

I prefer a clean commit history with self-contained changes. In practice most PRs don't do that, so I use squash-and-merge. So do as you prefer

YaLTeR · 2025-03-31T04:43:38Z

src/layout/scrolling.rs

    }

-    fn toggle_width(&mut self, tile_idx: Option<usize>) {
+    fn select_width(&mut self, preset_idx: usize, tile_idx: Option<usize>) {


Why extract this?

While not scoped for this PR, I think it might be useful to specify an exact preset width. For example, the user could bind a shortcut to immediately make the window half size (vs. cycling through all preset widths). This would behave similarly to what is possible right now with full width.

It's already possible. set-column-width 50%

Yes, but is there some advantage that the column retains knowledge that it is at a preset width? And not just an arbitrary width value

Not really. Switch preset will already start with the closest next (or previous) width.

Is it possible there may be rounding issues? For example, the first Mod+R press (imperceptibly) applies a very similar preset width, and the user must then hit Mod+R again to actually increment the window width. I've had this exact experience coming from hyprscroller (or maybe it was PaperWM)

Not really, no. Window sizes are quantized to logical integers on Wayland which makes this sort of stuff less likely

YaLTeR · 2025-03-31T04:43:54Z

src/layout/workspace.rs


+    pub fn increment_width(&mut self) {
+        if self.floating_is_active.get() {
+            self.floating.toggle_window_width(None);


Should be implemented for floating too

YaLTeR · 2025-03-31T04:45:57Z

Don't forget to add the new action to src/layout/tests.rs Op enum

two-horned · 2025-05-25T12:57:57Z

I see this hasn't seen any changes since more than a month. Can I hope to see this (very simple) feature implemented in the near future or no?

mattkang · 2025-05-29T03:09:33Z

@two-horned I definitely still want this. Haven't had enough time to get around to it. Can try this weekend but happy to collaborate on this if you're looking to help.

two-horned · 2025-05-29T04:35:02Z

@two-horned I definitely still want this. Haven't had enough time to get around to it. Can try this weekend but happy to collaborate on this if you're looking to help.

well, you can try out my solution as it was just little changes that needed to be applied to implement this feature.
The configuration file reads it just as the other switch-preset commands and I have been using it ever since.

The only thing that bothered me was while I applied the necessary changes, it was a bit confusing to navigate through to find all the right places, but I guess with the power of modern code editors this still worked OK.

If you wanted to implement a no-wrap-around setting, you can simply do the exact same changes I did: create a new constant parameter for the function and tweak the index calculation a bit. I highly discourage from introducing new functions for this, because you'd need to introduce new functions at many different points to reach them.

YaLTeR · 2025-05-29T04:49:40Z

@two-horned I haven't had a chance to properly review your version yet, but at a glance: not a big fan of the constant generic argument

two-horned · 2025-05-29T06:49:31Z

@two-horned I haven't had a chance to properly review your version yet, but at a glance: not a big fan of the constant generic argument

one can also make this a boolean that's passed to the function (matter of execution speed vs memory consumption).

Either way, I'd say it's better than introducing new function definitions + names for each small distinction. Much of the code I saw regarding this functionality was copy-paste and it's hard to spot differences. For example I had to fix a typo if you see my commit history in the PR #1670.

Another example is, that I find it very strange that that toggle_window_height in src/scrolling.rs:4618 introduces an animate predicate, while every other toggle-functions doesn't. What's even more strange is, that I can't find a way to control this property, because the only existing function call sets animate = true.

There is a huge issue of code duplication and redundancy in general. If a function is only called once by some proxy-function that simply passes the arguments from one place to another, there might be a problem at hand.

YaLTeR · 2025-05-29T07:22:29Z

There is a huge issue of code duplication and redundancy in general.

It's not perfect but there's no "huge issue" (and please don't go refactoring everything). I deal with it slowly whenever it is convenient

two-horned · 2025-05-29T14:50:33Z

There is a huge issue of code duplication and redundancy in general.

It's not perfect but there's no "huge issue" (and please don't go refactoring everything). I deal with it slowly whenever it is convenient

sorry, that came across wrong. i didn't mean "general" as the whole code base (and I would never have the time to touch all the code). what I meant is that in implementing this particular feature, I had to go through stuff that was doing very similar things over and over again.

There is a reason why rust has interfaces in the form of traits. E.g. one can write a trait for the different window modes and only needed to call this method at one place to control all the windows and they'd behave appropriately.

And if you don't belief me traits are useful, you can consider this code example in layout/workspace.rs:

    pub fn center_column(&mut self) {
        if self.floating_is_active.get() {
            self.floating.center_window(None);
        } else {
            self.scrolling.center_column();
        }
    }

    pub fn center_window(&mut self, id: Option<&W::Id>) {
        if id.map_or(self.floating_is_active.get(), |id| {
            self.floating.has_window(id)
        }) {
            self.floating.center_window(id);
        } else {
            self.scrolling.center_window(id);
        }
    }

[...]

    pub fn toggle_width<const FORWARDS: bool>(&mut self) {
        if self.floating_is_active.get() {
            self.floating.toggle_window_width::<FORWARDS>(None);
        } else {
            self.scrolling.toggle_width::<FORWARDS>();
        }
    }

[...]

    pub fn set_column_width(&mut self, change: SizeChange) {
        if self.floating_is_active.get() {
            self.floating.set_window_width(None, change, true);
        } else {
            self.scrolling.set_window_width(None, change);
        }
    }

    pub fn set_window_width(&mut self, window: Option<&W::Id>, change: SizeChange) {
        if window.map_or(self.floating_is_active.get(), |id| {
            self.floating.has_window(id)
        }) {
            self.floating.set_window_width(window, change, true);
        } else {
            self.scrolling.set_window_width(window, change);
        }
    }

    pub fn set_window_height(&mut self, window: Option<&W::Id>, change: SizeChange) {
        if window.map_or(self.floating_is_active.get(), |id| {
            self.floating.has_window(id)
        }) {
            self.floating.set_window_height(window, change, true);
        } else {
            self.scrolling.set_window_height(window, change);
        }
    }

[...]

    pub fn toggle_window_width<const FORWARDS: bool>(&mut self, window: Option<&W::Id>) {
        if window.map_or(self.floating_is_active.get(), |id| {
            self.floating.has_window(id)
        }) {
            self.floating.toggle_window_width::<FORWARDS>(window);
        } else {
            self.scrolling.toggle_window_width::<FORWARDS>(window);
        }
    }

    pub fn toggle_window_height<const FORWARDS: bool>(&mut self, window: Option<&W::Id>) {
        if window.map_or(self.floating_is_active.get(), |id| {
            self.floating.has_window(id)
        }) {
            self.floating.toggle_window_height::<FORWARDS>(window);
        } else {
            self.scrolling.toggle_window_height::<FORWARDS>(window);
        }
    }

Imagine, you'd wanted to add another layout, a nightmare!

Anyways, I hope you don't take this personally, I am just trying to help you out here.

And again, don't worry I am not refactoring.
All I wanted was able to resize my windows and the PR only deals with that :)

YaLTeR · 2025-05-29T18:08:53Z

We could continue this discussion elsewhere, but I'm well aware of traits. Sometimes though it's better to leave things simple and have some duplicate code. You can see in these functions that some of them have different conditions and logic from the others—this is intentional, and writing them out like this makes it trivial to write the intended logic in every case.

Imagine, you'd wanted to add another layout, a nightmare!

I refactor code as I work on it. This code is the result of adding the floating layout (it was only scrolling before, and it was all intertwined with workspace code). When I need something else and if it gets out of hand, I'll refactor as necessary. Premature abstractions without a clear use are just asking for pain down the road. Been there.

A-kly · 2026-03-04T22:15:37Z

Any updates on this?

Sempyos · 2026-03-05T02:33:50Z

Not currently

two-horned · 2026-03-05T03:35:16Z

Any updates on this?

Already merged with #1670

mattkang · 2026-03-05T17:52:50Z

Any updates on this?

Already merged with #1670

@two-horned Is there a way to disable wraparound yet?

two-horned · 2026-03-07T14:34:47Z

Any updates on this?

Already merged with #1670

@two-horned Is there a way to disable wraparound yet?

@mattkang there is now, if you build my branch (see #3571). If you want to give it a review to speed things up, please do :)

mattkang added 4 commits March 30, 2025 22:40

Add select_width

c253a02

Add cycle_width

6857701

Can specify number of steps and whether to wraparound

Add increment_width and decrement_width

4b5519b

Add SwitchPresetColumnWidthNext and SwitchPresetColumnWidthPrev

897d09d

YaLTeR requested changes Mar 31, 2025

View reviewed changes

two-horned mentioned this pull request May 25, 2025

Add cycle back feature for presets of column/window width/height. #1670

Merged

YaLTeR force-pushed the main branch from 744fcd9 to 01be0e6 Compare August 30, 2025 12:19

two-horned mentioned this pull request Mar 7, 2026

[Switch presets] add (no-)wraparound & make cycle-back an OPTION #3571

Open

Uh oh!

Conversation

mattkang commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattkang commented Mar 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattkang Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattkang Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YaLTeR commented Mar 31, 2025

Uh oh!

two-horned commented May 25, 2025

Uh oh!

mattkang commented May 29, 2025

Uh oh!

two-horned commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YaLTeR commented May 29, 2025

Uh oh!

two-horned commented May 29, 2025

Uh oh!

YaLTeR commented May 29, 2025

Uh oh!

two-horned commented May 29, 2025

Uh oh!

YaLTeR commented May 29, 2025

Uh oh!

A-kly commented Mar 4, 2026

Uh oh!

Sempyos commented Mar 5, 2026

Uh oh!

two-horned commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattkang commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

two-horned commented Mar 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mattkang commented Mar 31, 2025 •

edited

Loading

mattkang Mar 31, 2025 •

edited

Loading

mattkang Mar 31, 2025 •

edited

Loading

two-horned commented May 29, 2025 •

edited

Loading

two-horned commented Mar 5, 2026 •

edited

Loading

mattkang commented Mar 5, 2026 •

edited

Loading