Skip to content

Add "Increase window height upwards/downwards" actions#150

Open
jinliu wants to merge 2 commits intopeterfajdiga:masterfrom
jinliu:feature/change-window-height
Open

Add "Increase window height upwards/downwards" actions#150
jinliu wants to merge 2 commits intopeterfajdiga:masterfrom
jinliu:feature/change-window-height

Conversation

@jinliu
Copy link
Copy Markdown
Contributor

@jinliu jinliu commented Jan 20, 2026

The "Stacking offset Y" config is reused as the step size.

The "Stacking offset Y" config is reused as the step size.
Copy link
Copy Markdown
Owner

@peterfajdiga peterfajdiga left a comment

Choose a reason for hiding this comment

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

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.

@jinliu
Copy link
Copy Markdown
Contributor Author

jinliu commented Feb 16, 2026

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...

@peterfajdiga
Copy link
Copy Markdown
Owner

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.

Copilot AI review requested due to automatic review settings March 25, 2026 06:33
@jinliu
Copy link
Copy Markdown
Contributor Author

jinliu commented Mar 25, 2026

Changed actions to "increate-window-height-{up,down}"

@jinliu jinliu changed the title Add "Increase/Decrease window height" actions Add "Increase window height upwards/downwards" actions Mar 25, 2026
@jinliu jinliu requested a review from peterfajdiga March 25, 2026 06:35
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 Actions methods that call Column.adjustWindowHeight(...) with stackOffsetY.

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.

Comment on lines +160 to +165
public readonly windowHeightIncreaseUp = (cm: ClientManager, dm: DesktopManager, window: Window, column: Column, grid: Grid) => {
window.column.adjustWindowHeight(
window,
window.column.grid.config.stackOffsetY,
true,
);
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Comment on lines +168 to +173
public readonly windowHeightIncreaseDown = (cm: ClientManager, dm: DesktopManager, window: Window, column: Column, grid: Grid) => {
window.column.adjustWindowHeight(
window,
window.column.grid.config.stackOffsetY,
false,
);
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Comment on lines +107 to +114
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),
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

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).

Copilot uses AI. Check for mistakes.
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.

3 participants