Add "Increase window height upwards/downwards" actions#150
Add "Increase window height upwards/downwards" actions#150jinliu wants to merge 2 commits intopeterfajdiga:masterfrom
Conversation
The "Stacking offset Y" config is reused as the step size.
peterfajdiga
left a comment
There was a problem hiding this comment.
Thanks for the PR.
I have a problem with this approach, though. When you're changing the height of a window, you're also changing the height of another window (its upper or lower neighbor). But with this approach you have no control over which of the neighbors you're affecting.
I'd prefer to add actions like window-height-increase-up and window-height-increase-down. Both of them increase window height, but the first one grows the window upwards (shrinks the upper neighbor) and the second one grows it downwards (shrinks the lower neighbor).
For decreasing the height of a window, I wouldn't add new actions, to avoid too many actions. Instead, the user can switch focus to a neighbor window and grow that one to achieve the same effect.
|
That also makes sense. But maybe we can provide 4 actions ("move top/bottom border up/down") and let the user decide what suits him better. Yes, it's a bit too many actions. But as long as we don't assign any default keybindings, it seems OK to me. In contrary, the decision to choose 2 from the 4 actions seems a bit random... |
|
The decision to choose the two actions is not completely random. The window that is focused is the one that is currently being used and I think it's more likely that the user will decide they want a bigger view of the window they're using than that they want a smaller one. But maybe having all four actions would still be useful. Either way works for me. |
|
Changed actions to "increate-window-height-{up,down}" |
There was a problem hiding this comment.
Pull request overview
Adds new keyboard actions to resize the focused tiled window vertically by transferring height from the adjacent window, using the existing stackOffsetY setting as the step size.
Changes:
- Add two new keybinding definitions for increasing window height upward/downward.
- Add two corresponding
Actionsmethods that callColumn.adjustWindowHeight(...)withstackOffsetY.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/lib/keyBindings/definition.ts |
Registers two new keybinding entries that invoke the new height-adjust actions. |
src/lib/keyBindings/Actions.ts |
Implements the two new actions by calling adjustWindowHeight with a fixed step and direction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public readonly windowHeightIncreaseUp = (cm: ClientManager, dm: DesktopManager, window: Window, column: Column, grid: Grid) => { | ||
| window.column.adjustWindowHeight( | ||
| window, | ||
| window.column.grid.config.stackOffsetY, | ||
| true, | ||
| ); |
There was a problem hiding this comment.
stackOffsetY is a fixed positive step, but Column.adjustWindowHeight applies it without enforcing any minimum height. With many windows in a column, the adjacent window can be smaller than stackOffsetY, causing otherWindow.height to go negative and resulting in invalid geometries during arrange(). Clamp the delta based on the adjacent window’s kwinClient.minSize.height (and/or a reasonable floor), and no-op when there isn’t enough space to transfer.
| public readonly windowHeightIncreaseDown = (cm: ClientManager, dm: DesktopManager, window: Window, column: Column, grid: Grid) => { | ||
| window.column.adjustWindowHeight( | ||
| window, | ||
| window.column.grid.config.stackOffsetY, | ||
| false, | ||
| ); |
There was a problem hiding this comment.
Same issue as the "increase up" variant: repeated calls can shrink the adjacent (below) window below its minimum height (or even negative) because adjustWindowHeight doesn’t clamp. Please clamp the applied delta using the adjacent window’s kwinClient.minSize.height and skip when the requested step can’t be accommodated.
| name: "window-height-increase-up", | ||
| description: "Increase window height upwards", | ||
| action: () => world.doIfTiledFocused(actions.windowHeightIncreaseUp), | ||
| }, | ||
| { | ||
| name: "window-height-increase-down", | ||
| description: "Increase window height downwards", | ||
| action: () => world.doIfTiledFocused(actions.windowHeightIncreaseDown), |
There was a problem hiding this comment.
PR title mentions "Increase/Decrease window height" actions, but the implementation only adds "window-height-increase-up" and "window-height-increase-down" (both increase the focused window). If decrease actions are also intended, add corresponding bindings/actions (or adjust the PR title/description to match what’s actually implemented).
The "Stacking offset Y" config is reused as the step size.