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.

Sign up for GitHub

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

Jump to bottom

fix(api): various window-related function fixes #27330

Merged
merged 22 commits into from
Mar 9, 2024
Merged

fix(api): various window-related function fixes #27330

merged 22 commits into from
Mar 9, 2024

Conversation

seandewar
Copy link
Member

@seandewar seandewar commented Feb 4, 2024 •
edited

Follows up on some issues in #25550 and (a bunch of) other things

Also includes Vim patches to issues I found while working on things: 8.2.3862, 9.1.{0116,0118,0119,0121,0128,0130} (and 9.1.0117 as a partial patch).

@github-actions github-actions bot added the api libnvim, Nvim RPC API label Feb 4, 2024
@willothy
Copy link
Sponsor Contributor

willothy commented Feb 4, 2024

This looks great! I'm working on fixes for some of the split behavior btw, unless you were planning on doing that.

@seandewar
Copy link
Member Author

This looks great! I'm working on fixes for some of the split behavior btw, unless you were planning on doing that.

Feel free to work on that! I only plan to address some random things I find here.

@willothy
Copy link
Sponsor Contributor

willothy commented Feb 4, 2024

Feel free to work on that! I only plan to address some random things I find here.

Sounds good!

@zeertzjq zeertzjq added this to the 0.10 milestone Feb 7, 2024
@seandewar seandewar changed the title fix(api): some nvim_win_set_config splitting fixes fix(api): some nvim_win_set_config, nvim_open_win fixes Feb 7, 2024
@seandewar seandewar force-pushed the win_set_config-fixes branch 3 times, most recently from 71d3690 to de1f6ad Compare February 12, 2024 01:24
@seandewar

This comment was marked as resolved.

Sign in to view
@seandewar seandewar added the has:vim-patch issue is fixed in vim and patch needs to be ported label Feb 20, 2024
@seandewar seandewar force-pushed the win_set_config-fixes branch 6 times, most recently from c7f250e to 757d1cb Compare February 28, 2024 00:18
@seandewar seandewar changed the title fix(api): some nvim_win_set_config, nvim_open_win fixes fix(api): various window-related function fixes Feb 28, 2024
@seandewar
fix(api): win_set_config autocmds crash when moving win to other tabpage
bcb70ee 
Problem: win_enter autocommands can close new_curwin, crashing if it was the
last window in its tabpage after removing win, or can close parent, crashing
when attempting to split it later.

Solution: remove win first, check that parent is valid after win_enter.

NOTE: This isn't actually quite right, as this means win is not in the window
list or even has a frame when triggering enter autocommands (so it's not
considered valid in the tabpage). This is addressed in later commits.
@seandewar
fix(api): win_set_config fires unnecessary autocmds
233649b 
Problem: win_set_config should have the observable effect of moving an existing
window to another place, but instead fires autocommands as if a new window was
created and entered (and does not fire autocommands reflecting a "return" to the
original window).

Solution: do not fire win_enter-related autocommands when splitting the window,
but continue to fire them when entering the window that fills the new space when
moving a window to a different tabpage, as the new curwin changes.

Also, remove "++once" from the WinEnter autocmd in the other test, as omitting
it also crashed Nvim before this fix.
@seandewar seandewar marked this pull request as ready for review March 8, 2024 23:20
@seandewar
Copy link
Member Author

seandewar commented Mar 8, 2024 •
edited

About time I open this for reviews before the scope increases more 🙈.

There are a few other things I discovered that I haven't addressed here:

  • tabpagewinnr() and nvim_tabpage_get_win() don't work for non-current tabpages with tp_curwins pointing to external windows (the latter calls abort()). This is a consequence of how "tabpage independence" of external windows are implemented (they're moved between tabpages when entering another).

  • Unlike the minimum value of &lines which is based on the minimum frame height, the minimum value of &columns is not based on the minimum frame width. This can cause the assertion (size_t)grid_line_maxcol <= linebuf_size in grid_line_start() to fail (and maybe other issues) if the frames can't fit:

    :execute 'vsplit|'->repeat(99) | set columns=12

  • Some of the textlocky API functions probably ought to check curbuf_locked() too...

Also, I assume nvim_win_set_config is rather unique in its ability to move existing windows between tabpages (unlike stuff like :wincmd T, which actually creates a new window).
Maybe that could cause issues with things using win_valid to check if such a window wasn't closed, or if something saves a reference to the tabpage it was in to check later for whatever reason. Haven't investigated this much, though.

@github-actions github-actions bot requested review from bfredl and famiu March 8, 2024 23:20
@seandewar seandewar added the vim-patch See https://neovim.io/doc/user/dev_vimpatch.html label Mar 8, 2024
@github-actions github-actions bot requested a review from zeertzjq March 8, 2024 23:21
@seandewar
fix(api): open_win fire BufWinEnter for other buffer when !enter && !…
a873f33 
…noautocmd

Problem: BufWinEnter is not fired when not entering a new window, even when a
different buffer is specified and buffer-related autocommands are unblocked
(!noautocmd).

Solution: fire it in the context of the new window and buffer. Do not do it if
the buffer is unchanged, like :{s}buffer.

Be wary of autocommands! For example, it's possible for nvim_win_set_config to
be used in an autocommand to move a window to a different tabpage (in contrast,
things like wincmd T actually create a *new* window, so it may not have been
possible before, meaning other parts of Nvim could assume windows can't do
this... I'd be especially cautious of logic that restores curwin and curtab
without checking if curwin is still valid in curtab, if any such logic exists).

Also, bail early from win_set_buf if setting the temp curwin fails; this
shouldn't be possible, as the callers check that wp is valid, but in case that's
not true, win_set_buf will no longer continue setting a buffer for the wrong
window.

Note that pum_create_float_preview also uses win_set_buf, but from a glance,
doesn't look like it properly checks for autocmds screwing things up (win_enter,
nvim_create_buf...). I haven't addressed that here.

Also adds some test coverage for nvim_open_win autocommands.

Closes neovim#27121.
@seandewar
fix(api): win_set_config update statuslines after removing splits
54022a2 
Problem: nvim_win_set_config does not update statuslines after removing a split.

Solution: call last_status.

Didn't realize this was missing in the original nvim_win_set_config for splits
PR.

As it can only be done for the current tabpage, do it if win_tp == curtab;
enter_tabpage will eventually call last_status anyway when the user enters
another tabpage.
@zeertzjq
test(old): remove Test_floatwin_splitmove()
6416c6b 
Its corresponding test in Vim is in test_popupwin.win, so having it in
the middle of test_window_cmd.vim is strange, and it is now covered by
tests in ui/float_spec.lua anyway.
@zeertzjq
Copy link
Member

zeertzjq commented Mar 9, 2024 •
edited

I also pushed some follow-up commits. That pending test I added is a bit tricky to fix (as it will likely require changing close_last_window_tabpage() and its callsites), so I'll leave it as pending for now.

@zeertzjq zeertzjq force-pushed the win_set_config-fixes branch 3 times, most recently from 88d2c60 to 7dc4cd6 Compare March 9, 2024 05:37
@zeertzjq
fix(window): :close may cause Nvim to quit with autocmd and float
33dfb5a 
Problem:  :close may cause Nvim to quit if an autocommand triggered when
          closing the buffer closes all other non-floating windows and
          there are floating windows.
Solution: Correct the check for the only non-floating window.
@seandewar

This comment was marked as resolved.

Sign in to view
seandewar and others added 2 commits March 9, 2024 18:00
@seandewar
fix(api): win_set_config set tp_curwin of win moved from other tabpage
b52d158 
Problem: nvim_win_set_config does not update the tp_curwin of win's original
tabpage when moving it to another.

Solution: update it if win was the tp_curwin. Add a test.
@seandewar @brammool
vim-patch:8.2.3862: crash on exit with EXITFREE and using win_execute()
c3d22d3 
Problem:    Crash on exit with EXITFREE and using win_execute().
Solution:   Also save and restore tp_topframe. (issue vim/vim#9374)

vim/vim@dab17a0

Couldn't repro the crash in the test, but I only care about this patch so
switch_win sets topframe properly for win_split_ins in nvim_open_win and
nvim_win_set_config.
Add a test using nvim_win_call and :wincmd, as I couldn't repro the issue via
nvim_open_win or nvim_win_set_config (though it's clear they're affected by this
patch).

That said, at that point, could just use {un}use_tabpage inside switch_win
instead, which also updates tp_curwin (though maybe continue to not set it in
restore_win). That would also fix possible inconsistent behaviour such as:

:call win_execute(w, "let curwin_nr1 = tabpagewinnr(1)")
:let curwin_nr2 = tabpagewinnr(1)

Where it's possible for curwin_nr1 != curwin_nr2 if these commands are run from
the 1st tabpage, but window "w" is in the 2nd (as the 1st tabpage's tp_curwin
may still be invalid). I'll probably PR a fix for that later in Vim.

Co-authored-by: Bram Moolenaar <[email protected]>
@zeertzjq

This comment was marked as resolved.

Sign in to view
@seandewar

This comment was marked as resolved.

Sign in to view
@zeertzjq

This comment was marked as resolved.

Sign in to view
@seandewar
Copy link
Member Author

Okie dokie, I've pushed the commits on top of that state (and amended the fix(api): win_set_config set tp_curwin of win moved from other tabpage one to add a bit more to the test).

Here's the diff of that.

@seandewar
Copy link
Member Author

Ok, I'm totally over this PR at this point. 😆

Once checks are green, I'm happy to merge immediately if that sounds good. 👍

@willothy
Copy link
Sponsor Contributor

willothy commented Mar 9, 2024

Awesome! Not sure I could have provided any helpful reviews here as a lot of this is beyond my Vim knowledge (for now...) but this PR has been very informative to read through.

@seandewar seandewar merged commit b596732 into neovim:master Mar 9, 2024
27 checks passed
@seandewar seandewar deleted the win_set_config-fixes branch March 9, 2024 22:32
@github-actions github-actions bot removed request for bfredl and famiu March 9, 2024 22:32
@seandewar
Copy link
Member Author

Now time to hope I didn't break anything 😇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@willothy willothy willothy left review comments

@zeertzjq zeertzjq zeertzjq approved these changes

Assignees
No one assigned
Labels
api libnvim, Nvim RPC API has:vim-patch issue is fixed in vim and patch needs to be ported vim-patch See https://neovim.io/doc/user/dev_vimpatch.html
Projects
None yet
Milestone
0.10
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@seandewar @willothy @zeertzjq