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

Reimplement resize code #1990

Merged
merged 34 commits into from
Dec 8, 2022

Conversation

har7an
Copy link
Contributor

@har7an har7an commented Dec 5, 2022

This PR reimplements the code for resizing tiled and floating panes and adds new resizing methods along the way.

Scope of this PR

It started out removing more unwrap statements from the code in server::panes. I realized that much of the code for resizing panes is duplicated and changes only in minor aspects (using x or y coordinates, etc.). I decided to rewrite/merge the individual functions to reduce duplicate code and get a better grasp on how resizing works. While doing so I introduced a new data type (the new Resize action) to facilitate this work.

New features

This PR adds the ability to actively decrease pane sizes. Previously, resizing a pane would always increase that panes area, unless the border in the direction being resized hit the viewport boundary, in which case the opposite border was moved (reducing pane area).

Now, a "reducing" resize can be bound to keys, like so:

        bind "h" "Left" { Resize "Increase Left"; }
        bind "j" "Down" { Resize "Increase Down"; }
        bind "k" "Up" { Resize "Increase Up"; }
        bind "l" "Right" { Resize "Increase Right"; }
        bind "H" { Resize "Decrease Left"; }
        bind "J" { Resize "Decrease Down"; }
        bind "K" { Resize "Decrease Up"; }
        bind "L" { Resize "Decrease Right"; }
        bind "+" { Resize "Increase"; }
        bind "-" { Resize "Decrease"; }

and is also accessible from the CLI:

$ zellij action resize increase left

Given the keybinding above and e.g. a floating pane, one can now:

  • press h to move the left border outwards (increase area)
  • press H to move the left border inwards (decrease area)
  • etc.

What has changed

UI changes

From the users perspective almost nothing changed. The hints in the status bar have been renamed to reflect the new behavior, the documentation has been modified where appropriate. Care has been taken to maintain compatibility with existing configs. Hence, a keybinding:

        bind "h" "Left" { Resize "Increase Left"; }

does exactly the same as

        bind "h" "Left" { Resize "Left"; }

because "Increase" is the default which is assumed when nothing else is specified. Previous resize behavior, where an "increasing" resize towards a viewport boundary decreases the panes area from the other side, is maintained, too.

Breaking change: The generic increase/decrease (without a direction specified) will now increase/decrease tiled panes into every direction unlike previously, where a number of preferred directions was tried in order until the first one worked. I'm not sure whether this is a bad thing, personally I prefer the new resize behavior. I'm open to discuss this, however.

From a developers perspective

The Resize type has been extended to allow for greater flexibility and extensibility in the future. The resizing code has been streamlined and was compacted into fewer functions.

Current quirks

Some resize operations shift neighboring panes positions around slightly. I assume this is a side-effect from the code for "solving" pane layouts but haven't extensively tested this yet. This is likely the reason why some of the server tests fail, too.

@har7an
Copy link
Contributor Author

har7an commented Dec 5, 2022

For anyone interested in trying this out, here's a binary with the new resize behavior implemented: zellij.zip

Add this snippet into your config to replace the current "Resize" keybindings:

        bind "h" "Left" { Resize "Increase Left"; }
        bind "j" "Down" { Resize "Increase Down"; }
        bind "k" "Up" { Resize "Increase Up"; }
        bind "l" "Right" { Resize "Increase Right"; }
        bind "H" { Resize "Decrease Left"; }
        bind "J" { Resize "Decrease Down"; }
        bind "K" { Resize "Decrease Up"; }
        bind "L" { Resize "Decrease Right"; }
        bind "+" { Resize "Increase"; }
        bind "-" { Resize "Decrease"; }

@imsnif
Copy link
Member

imsnif commented Dec 5, 2022

Hey @har7an - very happy to see this!

I haven't looked at the code, but a quick note: not sure if you were planning on doing this, but can we please also add this to the default config?

@har7an
Copy link
Contributor Author

har7an commented Dec 5, 2022

can we please also add this to the default config?

Totally, I just wanted to discuss sensible options for the default keybindings before doing so. I like hjkl/HJKL, because I do not use the arrow keys at all. I don't know how other users feel about this. Iirc it's not possible to combine the arrow keys with modifiers, right? Do you (or anyone reading this) have a different default keybinding in mind?

@imsnif
Copy link
Member

imsnif commented Dec 5, 2022

How about <arrow> | Alt+<arrows>? We know they all work (except for the poor mac users whom we need to find a solution for anyway)

@har7an har7an temporarily deployed to cachix December 7, 2022 07:13 — with GitHub Actions Inactive
@har7an har7an temporarily deployed to cachix December 7, 2022 07:14 — with GitHub Actions Inactive
@har7an
Copy link
Contributor Author

har7an commented Dec 7, 2022

Here's the current state: zellij.zip

Best enjoyed with ./zellij setup --clean to make it load a new default config that includes the new behavior. Regarding the keybindings: I can't think of anything to make with the arrow keys. Binding to Alt + arrows is possible, but prevents moving to different panes without leaving resize mode. That's a pretty severe penalty in my personal opinion, but we can still ship it that way, of course.

@har7an har7an temporarily deployed to cachix December 7, 2022 09:12 — with GitHub Actions Inactive
@har7an har7an temporarily deployed to cachix December 7, 2022 09:54 — with GitHub Actions Inactive
@har7an har7an force-pushed the errors/dont-unwrap-in-server-panes branch from c5a9070 to 38b5940 Compare December 7, 2022 10:03
@har7an har7an temporarily deployed to cachix December 7, 2022 10:03 — with GitHub Actions Inactive
@har7an har7an temporarily deployed to cachix December 7, 2022 12:50 — with GitHub Actions Inactive
in floating panes code.
which was previously present in multiple locations.
Also start working on a new Resize Method (type `ResizeStrategy`), to
remove code duplication in the resize code.
with the `ResizeStrategy` type. Add a new action with the ability to
invoke it from the CLI. Take care to maintain backwards-compatibility in
terms of configuring the new resize mode.
but it's currently still broken in a few regards and misses ability to
perform "regular" increase/decrease.
to catch if the total area of all panes (in percent) is different from
100.0 at some point.
caused by the fact that neighboring plugin panes previously weren't
filtered from resize operations, even though they cannot be resized at
all.
that controls whether we invert resize behavior when increasing size
towards a bounadry. This maintains current behavior.
without specifying a direction (towards all possible directions).
Currently broken in some cases.
to preserve resize debug assertions.
caused by checking for the wrong alignments in some cases. Also refactor
the code for looking up aligned panes.
and remove log statements and unused functions.
if the floating pane is hitting a boundary already.
with new functions and result types.
behavior which would previously, upon hitting a boundary, cause the pane
to invert the resize operation, which is wrong. Instead, it now does not
resize floating panes on an undirected resize "increase" in directions
where it hits boundaries.
The values for the resize increments were previously wrong, causing many
of the tests to fail.
to correctly consider fixed-size panes.
with new keybindings for resize mode.
type in `change_pane_size` function.
for undirected resizes, to the way it was before this PR.
for tests working with the default config
@har7an
Copy link
Contributor Author

har7an commented Dec 7, 2022

@imsnif I'm going with the following keybindings now:

  • hjkl/Arrows increase size in the given direction (as before)
  • HJKL decrease size from the given direction

I found no keybinding to bind decreasing resizes to that includes the arrow keys. Is that ok?

@har7an har7an force-pushed the errors/dont-unwrap-in-server-panes branch from 58f58c3 to cb08b2c Compare December 7, 2022 15:48
@har7an har7an temporarily deployed to cachix December 7, 2022 15:49 — with GitHub Actions Inactive
@imsnif
Copy link
Member

imsnif commented Dec 7, 2022

@har7an why not use Alt? We already know it works both with hjkl and arrow keys

@har7an
Copy link
Contributor Author

har7an commented Dec 8, 2022

why not use Alt?

Because it conflicts with the Alt+Arrow keybindings for movement. I don't think this is a good idea for a number of reasons:

  1. It will likely break peoples workflows. Resizing multiple panes requires entering resize mode, resizing, leaving resize mode, moving focus, entering resize mode, ....
  2. It makes the config more complicated in the process

Regarding point 2: This is the current config:

    shared_except "locked" {
        // ...
        bind "Alt h" "Alt Left" { MoveFocusOrTab "Left"; }
        bind "Alt l" "Alt Right" { MoveFocusOrTab "Right"; }
        bind "Alt j" "Alt Down" { MoveFocus "Down"; }
        bind "Alt k" "Alt Up" { MoveFocus "Up"; }
    }

We'll have to change it to:

    shared_except "locked" "resize" {
        // ...
    }

and then add the "basic" keybindings back into resize mode manually. Or am I missing something? In this case I'd prefer to announce this new feature in the changelog, maybe add a comment with the Alt+Arrow bindings into the config and explain the situation there. I suggest we keep the HJKL bindings regardless, but don't do anything with the arrow keys.

@imsnif
Copy link
Member

imsnif commented Dec 8, 2022

Well argued @har7an - I'm convinced. :)

@har7an har7an merged commit 62eaea1 into zellij-org:main Dec 8, 2022
@har7an har7an deleted the errors/dont-unwrap-in-server-panes branch December 13, 2022 07:27
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