Skip to content

fix window sizing issue when running :RegisterEdit from a very small window#11

Merged
tuurep merged 2 commits into
tuurep:masterfrom
nullromo:handle-equalalways
May 13, 2025
Merged

fix window sizing issue when running :RegisterEdit from a very small window#11
tuurep merged 2 commits into
tuurep:masterfrom
nullromo:handle-equalalways

Conversation

@nullromo

@nullromo nullromo commented May 7, 2025

Copy link
Copy Markdown

Closes #1

In order to preserve the layout of existing windows, it makes sense to turn off equalalways while messing with the window splits. Afterwards, we can turn it back on if it was on before.

In order to make sure there is actually enough room to split the new window, we have to slightly enlarge the previous window first when equalalways is off.

TODO list

  • Resize windows properly in most cases
  • Be compatible with global equalalways and noequalalways options.
  • Institute a maximum buffer height.

@tuurep

tuurep commented May 9, 2025

Copy link
Copy Markdown
Owner

Great, for the common case (just opening another register, from a register split) appears like it just works!

Stress testing it with something like: what happens when I have many multiline registers open and starting to run out of space:

image

^ here I repeatedly open a multiline + register, while there is a one-line q register in between

Do you think it's worth doing anything about these edge cases?

@tuurep

tuurep commented May 9, 2025

Copy link
Copy Markdown
Owner

Code looks ok to me

@tuurep

tuurep commented May 9, 2025

Copy link
Copy Markdown
Owner

In the commit messages, could you put the Closes/Related to #<issue> in the body of the message? i.e.

Do foo bar to have more of a baz

Closes #<whichever>

@nullromo nullromo force-pushed the handle-equalalways branch from ddc2f50 to c533d65 Compare May 9, 2025 16:36
@nullromo

nullromo commented May 9, 2025

Copy link
Copy Markdown
Author

I see what you mean about the one buffer growing and the other sometimes being 0 lines tall. I never tested with multiline registers, whoops. It's hard to tell exactly why that happens, but yeah I guess when you run out of space things get tricky. Here's a video just to demonstrate the issue.

Screen.Recording.2025-05-09.093959.mp4

And here's a video of a 1-line register, which actually also has the same problem given enough lack of space.

Screen.Recording.2025-05-09.094527.mp4

Not sure if I'll be able to think of any way to handle it, especially since it's hard to reason what the proper behavior should actually be.

One way around this kind of problem would be to change the plugin behavior such that instead of opening a horizontal split window, it pops up some other kind of temporary buffer, like a floating window. But I don't think that's needed.

I guess the ideal behavior is maybe the register edit window doesn't change ANY window sizes at all, and just consumes the bottom N lines of the current window. But then what if there's not enough room? Again, it's hard to determine the desired behavior.

@tuurep

tuurep commented May 10, 2025

Copy link
Copy Markdown
Owner

Hmm, yeah, the behavior earlier in the multi-arg PR where it would eventually hit a "not enough room" builtin error was nice, is there any way we could force that to happen?

(in this PR it no longer happens neither with too many args or when space runs out one-at-a-time)

There's still one edge case to consider though: imagine you had 100 paragraphs of Lorem Ipsum in a clipboard-style register. Wouldn't want to disallow viewing or editing it so it should have some fairly large maximum size and be a scrollable split. But this further makes the running-out-of-space case more complicated 😄

Yes, needs some thought. Definitely prioritize the simple and realistic usage cases.

@nullromo

Copy link
Copy Markdown
Author

it would eventually hit a "not enough room" builtin error

This still happens. The very end of both the videos above shows it happening.

imagine you had 100 paragraphs of Lorem Ipsum in a clipboard-style register ... it should have some fairly large maximum size

I think we could solve this with just

-     local window_height = #buf_lines
+     local window_height = math.max(#buf_lines, MAX_BUFFER_LINES)

and set MAX_BUFFER_LINES to maybe 20 or so.

@tuurep

tuurep commented May 12, 2025

Copy link
Copy Markdown
Owner

Yeah, max buffer lines sounds like a great idea

@nullromo

Copy link
Copy Markdown
Author

Ok great. Another idea I had was what if the register changes to be much shorter? Should we shrink it?

For instance, the buffer contains 10 lines and then later it updates (after #12 is in place) to just 1 line. Should we resize the window to be 1 line tall?

@tuurep

tuurep commented May 12, 2025

Copy link
Copy Markdown
Owner

Yeah, if it was simple/doable, would like this the most:

  • Shrink when contents become smaller through autocmd
  • Grow when contents become bigger through autocmd
    • This I actually think is the more important one, it can be pretty jarring when it happens that a oneliner becomes a multiliner and you don't realize it will scroll

But I would anticipate a lot of edge cases so let's see how realistic it is 😄

@nullromo

Copy link
Copy Markdown
Author

Ok, good idea. However, I think maybe it's out of the scope of this PR since this was just for opening buffers. Maybe #12 can include the bit about resizing the windows automatically since that's the one about the autocommands.

@tuurep

tuurep commented May 12, 2025

Copy link
Copy Markdown
Owner

Yeah, outside of this PR sounds good

@nullromo

Copy link
Copy Markdown
Author

In that case, do you think this one is ready to merge? Maybe you could summarize any outstanding problem in a new GitHub issue and we can merge this one.

@tuurep

tuurep commented May 13, 2025

Copy link
Copy Markdown
Owner

Yeah most likely, I'll make sure tomorrow

@tuurep

tuurep commented May 13, 2025

Copy link
Copy Markdown
Owner

Yes, this PR is nice and simple, we can merge it

I wanted to understand why this is specifically + 2 and not for example + 1

+    -- make sure the old window is big enough to split
+    vim.api.nvim_win_set_height(old_window_id, old_window_height + 2)

Testing it with + 1 I get a "Not enough room" error using :RegisterEdit from a height 1 buffer

So yes definitely seems right

I'll open 2 related issues now:

@tuurep tuurep merged commit 6c418c6 into tuurep:master May 13, 2025
@tuurep

tuurep commented May 13, 2025

Copy link
Copy Markdown
Owner

Thank you for 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.

Running :Re in an existing registereditor window is messy

2 participants