[vim/vim] Check for room when split-moving windows, fix some f_win_splitmove bugs (PR #14042)
Sean Dewar
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/14042
Commit Summary
- 23f0c01 Check for room when split-moving windows and fix f_win_splitmove bugs
- aa738b8 Use different layout restoration strategy in win_split_ins
File Changes
(6 files)
- M src/autocmd.c (2)
- M src/evalwindow.c (70)
- M src/proto/window.pro (4)
- M src/testdir/test_window_cmd.vim (134)
- M src/vim.h (1)
- M src/window.c (192)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
yeah, we may need to amend the documentation, that because of E36 WinNew may not be triggered.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Sean Dewar
Seems good 👍
Glancing at the code, I can see a few other potential issues with how WinNewPre is currently handled (e.g: it's fired while curwin might be invalid and has no entry in the window list or frame, plus switching windows and tabpages in the autocmd could be problematic). I'll probably try to fix those things and the documentation in a different PR when I have the time.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
thanks. Let me know when it's ready to review and I'll have a closer look.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Sean Dewar
@seandewar pushed 3 commits.
- b6037e0 Check for room when split-moving windows, fix some f_win_splitmove bugs
- 93ad2e1 Stop split-moving from firing WinNew and WinNewPre autocommands
- 2b6834d Use different restoration strategy in win_splitmove
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Sean Dewar
@seandewar commented on this pull request.
In src/window.c:
>
if (ONE_WINDOW)
- {
- beep_flush();
- return;
- }
- if (check_split_disallowed() == FAIL)
- return;
+ return OK; // nothing to do
+ if (check_split_disallowed(wp) == FAIL)
This is consistent with how win_totop worked, but maybe it's not necessary to check the wp->w_buffer->b_locked_split part before split-moving, as it won't produce many views into the same buffer (which I think was the motivation for the flag in v8.2.2476). I haven't checked this, though.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Sean Dewar
@seandewar pushed 4 commits.
- eccf7cc Check for room when split-moving windows, fix some f_win_splitmove bugs
- 0838daf Stop split-moving from firing WinNew and WinNewPre autocommands
- 85a0a76 Use different restoration strategy in win_splitmove
- 11e842c Check text_or_buf_locked in f_win_gotoid and f_win_splitmove
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
Thanks , looks good
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
zeertzjq
zeertzjq left a comment (vim/vim#14042)There are also other issues I found with the current implementation of WinNewPre
that need addressing:
it allows switching windows and tabpages, which can cause incorrect windows to
be split/moved, and big problems when switching tabpages.it fires before win_split_ins checks for room, before it makes any changes to
window sizes or before it considers allocating a new window. This should be
changed or documented.
Vim 9.2 release is getting near. Do you still plan to fix these problems?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Sean Dewar
seandewar left a comment (vim/vim#14042)Ah, sorry! Likely not any time soon, as I'm lacking the motivation.
Feel free to take that over, if you're interested. 🤗
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
zeertzjq
zeertzjq left a comment (vim/vim#14042)I'm not sure about this. While these indeed are potential problems, they do not cause a crash, as win_split_ins() just splits the current window. Even if it switches to a window with a closing buffer, it won't lead to a crash either because of #19088.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Sean Dewar
seandewar left a comment (vim/vim#14042)Hmm, my memory is hazy, but I recall trigger_winnewpre also being called in win_new_tabpage (or somewhere close) at a pretty unsafe spot, which may be what 1. is referring to, but I can't find it now.
That said, the trigger_winnewpre in win_split_ins looks pretty innocent to me now. 🤔
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.