From bcb70eeac48040fd6d6bfc20cf7fb6f41374a67c Mon Sep 17 00:00:00 2001 From: Sean Dewar Date: Sun, 4 Feb 2024 00:42:36 +0000 Subject: [PATCH 01/22] fix(api): win_set_config autocmds crash when moving win to other tabpage 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. --- src/nvim/api/win_config.c | 7 ++++++- test/functional/api/window_spec.lua | 21 +++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/nvim/api/win_config.c b/src/nvim/api/win_config.c index 3cc520dc78..bb1117b3fe 100644 --- a/src/nvim/api/win_config.c +++ b/src/nvim/api/win_config.c @@ -478,12 +478,17 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err) int dir; new_curwin = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp); } + win_remove(win, win_tp == curtab ? NULL : win_tp); // move to neighboring window if we're moving the current window to a new tabpage if (curwin == win && parent != NULL && new_curwin != NULL && win_tp != win_find_tabpage(parent)) { win_enter(new_curwin, true); + if (!win_valid_any_tab(parent)) { + // win_enter autocommands closed the `parent` to split from. + api_set_error(err, kErrorTypeException, "Window to split was closed"); + return; + } } - win_remove(win, win_tp == curtab ? NULL : win_tp); } else { win_remove(win, win_tp == curtab ? NULL : win_tp); ui_comp_remove_grid(&win->w_grid_alloc); diff --git a/test/functional/api/window_spec.lua b/test/functional/api/window_spec.lua index 097a546ef2..a812d502eb 100644 --- a/test/functional/api/window_spec.lua +++ b/test/functional/api/window_spec.lua @@ -1663,6 +1663,27 @@ describe('API/win', function() }, }, fn.winlayout()) end) + + it('closing new curwin when moving window to other tabpage works', function() + command('split | tabnew') + local w = api.nvim_get_current_win() + local t = api.nvim_get_current_tabpage() + command('tabfirst | autocmd WinEnter * ++once quit') + api.nvim_win_set_config(0, { win = w, split = 'left' }) + -- New tabpage is now the only one, as WinEnter closed the new curwin in the original. + eq(t, api.nvim_get_current_tabpage()) + eq({ t }, api.nvim_list_tabpages()) + end) + + it('closing split parent when moving window to other tabpage aborts', function() + command('split | tabnew') + local w = api.nvim_get_current_win() + command('tabfirst | autocmd WinEnter * call nvim_win_close(' .. w .. ', 1)') + eq( + 'Window to split was closed', + pcall_err(api.nvim_win_set_config, 0, { win = w, split = 'left' }) + ) + end) end) describe('get_config', function() From 233649bc757743f7677b2ae414779192a94aa2ae Mon Sep 17 00:00:00 2001 From: Sean Dewar Date: Sun, 4 Feb 2024 01:50:49 +0000 Subject: [PATCH 02/22] fix(api): win_set_config fires unnecessary autocmds 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. --- src/nvim/api/win_config.c | 28 +++++++----------------- test/functional/api/window_spec.lua | 34 ++++++++++++++++++++++++++++- 2 files changed, 41 insertions(+), 21 deletions(-) diff --git a/src/nvim/api/win_config.c b/src/nvim/api/win_config.c index bb1117b3fe..e53e13e2a3 100644 --- a/src/nvim/api/win_config.c +++ b/src/nvim/api/win_config.c @@ -505,7 +505,7 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err) win->w_pos_changed = true; } - int flags = win_split_flags(fconfig.split, parent == NULL); + int flags = win_split_flags(fconfig.split, parent == NULL) | WSP_NOENTER; if (parent == NULL) { if (!win_split_ins(0, flags, win, 0)) { @@ -514,24 +514,13 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err) return; } } else { - win_execute_T args; - - tabpage_T *tp = win_find_tabpage(parent); - if (!win_execute_before(&args, parent, tp)) { - // TODO(willothy): how should we handle this / what should the message be? - api_set_error(err, kErrorTypeException, "Failed to switch to tabpage %d", tp->handle); - win_execute_after(&args); - return; - } - // This should return the same ptr to `win`, but we check for - // NULL to detect errors. - win_T *res = win_split_ins(0, flags, win, 0); - win_execute_after(&args); - if (!res) { - // TODO(willothy): What should this error message say? - api_set_error(err, kErrorTypeException, "Failed to split window"); - return; - } + switchwin_T switchwin; + // `parent` is valid in its tabpage, so switch_win should not fail. + const int result = switch_win(&switchwin, parent, win_find_tabpage(parent), true); + (void)result; + assert(result == OK); + win_split_ins(0, flags, win, 0); + restore_win(&switchwin, true); } if (HAS_KEY_X(config, width)) { win_setwidth_win(fconfig.width, win); @@ -539,7 +528,6 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err) if (HAS_KEY_X(config, height)) { win_setheight_win(fconfig.height, win); } - redraw_later(win, UPD_NOT_VALID); return; } else { win_config_float(win, fconfig); diff --git a/test/functional/api/window_spec.lua b/test/functional/api/window_spec.lua index a812d502eb..3914090814 100644 --- a/test/functional/api/window_spec.lua +++ b/test/functional/api/window_spec.lua @@ -1668,7 +1668,7 @@ describe('API/win', function() command('split | tabnew') local w = api.nvim_get_current_win() local t = api.nvim_get_current_tabpage() - command('tabfirst | autocmd WinEnter * ++once quit') + command('tabfirst | autocmd WinEnter * quit') api.nvim_win_set_config(0, { win = w, split = 'left' }) -- New tabpage is now the only one, as WinEnter closed the new curwin in the original. eq(t, api.nvim_get_current_tabpage()) @@ -1684,6 +1684,38 @@ describe('API/win', function() pcall_err(api.nvim_win_set_config, 0, { win = w, split = 'left' }) ) end) + + it('expected autocmds when moving window to other tabpage', function() + local new_curwin = api.nvim_get_current_win() + command('split') + local win = api.nvim_get_current_win() + command('tabnew') + local parent = api.nvim_get_current_win() + exec([[ + tabfirst + let result = [] + autocmd WinEnter * let result += ["Enter", win_getid()] + autocmd WinLeave * let result += ["Leave", win_getid()] + autocmd WinNew * let result += ["New", win_getid()] + ]]) + api.nvim_win_set_config(0, { win = parent, split = 'left' }) + -- Shouldn't see WinNew, as we're not creating any new windows, just moving existing ones. + eq({ 'Leave', win, 'Enter', new_curwin }, eval('result')) + end) + + it('no autocmds when moving window within same tabpage', function() + local parent = api.nvim_get_current_win() + exec([[ + split + let result = [] + autocmd WinEnter * let result += ["Enter", win_getid()] + autocmd WinLeave * let result += ["Leave", win_getid()] + autocmd WinNew * let result += ["New", win_getid()] + ]]) + api.nvim_win_set_config(0, { win = parent, split = 'left' }) + -- Shouldn't see any of those events, as we remain in the same window. + eq({}, eval('result')) + end) end) describe('get_config', function() From a873f33993ef84e3f954127038e559e1ac1cac43 Mon Sep 17 00:00:00 2001 From: Sean Dewar Date: Sun, 4 Feb 2024 02:59:26 +0000 Subject: [PATCH 03/22] fix(api): open_win fire BufWinEnter for other buffer when !enter && !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 #27121. --- src/nvim/api/win_config.c | 39 ++++-- src/nvim/window.c | 6 +- test/functional/api/window_spec.lua | 207 ++++++++++++++++++++++++++++ 3 files changed, 242 insertions(+), 10 deletions(-) diff --git a/src/nvim/api/win_config.c b/src/nvim/api/win_config.c index e53e13e2a3..9d63a1997c 100644 --- a/src/nvim/api/win_config.c +++ b/src/nvim/api/win_config.c @@ -261,8 +261,8 @@ Window nvim_open_win(Buffer buffer, Boolean enter, Dict(win_config) *config, Err switchwin_T switchwin; // `parent` is valid in `tp`, so switch_win should not fail. const int result = switch_win(&switchwin, parent, tp, true); - (void)result; assert(result == OK); + (void)result; wp = win_split_ins(0, flags, NULL, 0); restore_win(&switchwin, true); } @@ -276,18 +276,41 @@ Window nvim_open_win(Buffer buffer, Boolean enter, Dict(win_config) *config, Err api_set_error(err, kErrorTypeException, "Failed to create window"); return 0; } + + // Autocommands may close `wp` or move it to another tabpage, so update and check `tp` after each + // event. In each case, `wp` should already be valid in `tp`, so switch_win should not fail. switchwin_T switchwin; - if (switch_win_noblock(&switchwin, wp, tp, true) == OK) { - apply_autocmds(EVENT_WINNEW, NULL, NULL, false, curbuf); + { + const int result = switch_win_noblock(&switchwin, wp, tp, true); + assert(result == OK); + (void)result; + if (apply_autocmds(EVENT_WINNEW, NULL, NULL, false, curbuf)) { + tp = win_find_tabpage(wp); + } + restore_win_noblock(&switchwin, true); } - restore_win_noblock(&switchwin, true); - if (enter) { + if (tp && enter) { goto_tabpage_win(tp, wp); + tp = win_find_tabpage(wp); } - if (win_valid_any_tab(wp) && buf != wp->w_buffer) { - win_set_buf(wp, buf, !enter || fconfig.noautocmd, err); + if (tp && buf != wp->w_buffer) { + const bool noautocmd = !enter || fconfig.noautocmd; + win_set_buf(wp, buf, noautocmd, err); + if (!noautocmd) { + tp = win_find_tabpage(wp); + } + // win_set_buf autocommands were blocked if we didn't enter, but we still want BufWinEnter. + if (noautocmd && !fconfig.noautocmd && wp->w_buffer == buf) { + const int result = switch_win_noblock(&switchwin, wp, tp, true); + assert(result == OK); + (void)result; + if (apply_autocmds(EVENT_BUFWINENTER, NULL, NULL, false, buf)) { + tp = win_find_tabpage(wp); + } + restore_win_noblock(&switchwin, true); + } } - if (!win_valid_any_tab(wp)) { + if (!tp) { api_set_error(err, kErrorTypeException, "Window was closed immediately"); return 0; } diff --git a/src/nvim/window.c b/src/nvim/window.c index e2c4524eaa..d5a6e347e7 100644 --- a/src/nvim/window.c +++ b/src/nvim/window.c @@ -718,6 +718,7 @@ void win_set_buf(win_T *win, buf_T *buf, bool noautocmd, Error *err) kErrorTypeException, "Failed to switch to window %d", win->handle); + goto cleanup; } try_start(); @@ -729,10 +730,11 @@ void win_set_buf(win_T *win, buf_T *buf, bool noautocmd, Error *err) buf->handle); } - // If window is not current, state logic will not validate its cursor. - // So do it now. + // If window is not current, state logic will not validate its cursor. So do it now. + // Still needed if do_buffer returns FAIL (e.g: autocmds abort script after buffer was set). validate_cursor(); +cleanup: restore_win_noblock(&switchwin, true); if (noautocmd) { unblock_autocmds(); diff --git a/test/functional/api/window_spec.lua b/test/functional/api/window_spec.lua index 3914090814..8966c3b086 100644 --- a/test/functional/api/window_spec.lua +++ b/test/functional/api/window_spec.lua @@ -1364,6 +1364,213 @@ describe('API/win', function() }, }, layout) end) + + local function setup_tabbed_autocmd_test() + local info = {} + info.orig_buf = api.nvim_get_current_buf() + info.other_buf = api.nvim_create_buf(true, true) + info.tab1_curwin = api.nvim_get_current_win() + info.tab1 = api.nvim_get_current_tabpage() + command('tab split | split') + info.tab2_curwin = api.nvim_get_current_win() + info.tab2 = api.nvim_get_current_tabpage() + exec([=[ + tabfirst + let result = [] + autocmd TabEnter * let result += [["TabEnter", nvim_get_current_tabpage()]] + autocmd TabLeave * let result += [["TabLeave", nvim_get_current_tabpage()]] + autocmd WinEnter * let result += [["WinEnter", win_getid()]] + autocmd WinLeave * let result += [["WinLeave", win_getid()]] + autocmd WinNew * let result += [["WinNew", win_getid()]] + autocmd WinClosed * let result += [["WinClosed", str2nr(expand(""))]] + autocmd BufEnter * let result += [["BufEnter", win_getid(), bufnr()]] + autocmd BufLeave * let result += [["BufLeave", win_getid(), bufnr()]] + autocmd BufWinEnter * let result += [["BufWinEnter", win_getid(), bufnr()]] + autocmd BufWinLeave * let result += [["BufWinLeave", win_getid(), bufnr()]] + ]=]) + return info + end + + it('fires expected autocmds when creating splits without entering', function() + local info = setup_tabbed_autocmd_test() + + -- For these, don't want BufWinEnter if visiting the same buffer, like :{s}buffer. + -- Same tabpage, same buffer. + local new_win = api.nvim_open_win(0, false, { split = 'left', win = info.tab1_curwin }) + eq({ + { 'WinNew', new_win }, + }, eval('result')) + eq(info.tab1_curwin, api.nvim_get_current_win()) + + -- Other tabpage, same buffer. + command('let result = []') + new_win = api.nvim_open_win(0, false, { split = 'left', win = info.tab2_curwin }) + eq({ + { 'WinNew', new_win }, + }, eval('result')) + eq(info.tab1_curwin, api.nvim_get_current_win()) + + -- Same tabpage, other buffer. + command('let result = []') + new_win = api.nvim_open_win(info.other_buf, false, { split = 'left', win = info.tab1_curwin }) + eq({ + { 'WinNew', new_win }, + { 'BufWinEnter', new_win, info.other_buf }, + }, eval('result')) + eq(info.tab1_curwin, api.nvim_get_current_win()) + + -- Other tabpage, other buffer. + command('let result = []') + new_win = api.nvim_open_win(info.other_buf, false, { split = 'left', win = info.tab2_curwin }) + eq({ + { 'WinNew', new_win }, + { 'BufWinEnter', new_win, info.other_buf }, + }, eval('result')) + eq(info.tab1_curwin, api.nvim_get_current_win()) + end) + + it('fires expected autocmds when creating and entering splits', function() + local info = setup_tabbed_autocmd_test() + + -- Same tabpage, same buffer. + local new_win = api.nvim_open_win(0, true, { split = 'left', win = info.tab1_curwin }) + eq({ + { 'WinNew', new_win }, + { 'WinLeave', info.tab1_curwin }, + { 'WinEnter', new_win }, + }, eval('result')) + + -- Same tabpage, other buffer. + api.nvim_set_current_win(info.tab1_curwin) + command('let result = []') + new_win = api.nvim_open_win(info.other_buf, true, { split = 'left', win = info.tab1_curwin }) + eq({ + { 'WinNew', new_win }, + { 'WinLeave', info.tab1_curwin }, + { 'WinEnter', new_win }, + { 'BufLeave', new_win, info.orig_buf }, + { 'BufEnter', new_win, info.other_buf }, + { 'BufWinEnter', new_win, info.other_buf }, + }, eval('result')) + + -- For these, the other tabpage's prevwin and curwin will change like we switched from its old + -- curwin to the new window, so the extra events near TabEnter reflect that. + -- Other tabpage, same buffer. + api.nvim_set_current_win(info.tab1_curwin) + command('let result = []') + new_win = api.nvim_open_win(0, true, { split = 'left', win = info.tab2_curwin }) + eq({ + { 'WinNew', new_win }, + { 'WinLeave', info.tab1_curwin }, + { 'TabLeave', info.tab1 }, + + { 'WinEnter', info.tab2_curwin }, + { 'TabEnter', info.tab2 }, + { 'WinLeave', info.tab2_curwin }, + { 'WinEnter', new_win }, + }, eval('result')) + + -- Other tabpage, other buffer. + api.nvim_set_current_win(info.tab2_curwin) + api.nvim_set_current_win(info.tab1_curwin) + command('let result = []') + new_win = api.nvim_open_win(info.other_buf, true, { split = 'left', win = info.tab2_curwin }) + eq({ + { 'WinNew', new_win }, + { 'WinLeave', info.tab1_curwin }, + { 'TabLeave', info.tab1 }, + + { 'WinEnter', info.tab2_curwin }, + { 'TabEnter', info.tab2 }, + { 'WinLeave', info.tab2_curwin }, + { 'WinEnter', new_win }, + + { 'BufLeave', new_win, info.orig_buf }, + { 'BufEnter', new_win, info.other_buf }, + { 'BufWinEnter', new_win, info.other_buf }, + }, eval('result')) + + -- Other tabpage, other buffer; but other tabpage's curwin has a new buffer active. + api.nvim_set_current_win(info.tab2_curwin) + local new_buf = api.nvim_create_buf(true, true) + api.nvim_set_current_buf(new_buf) + api.nvim_set_current_win(info.tab1_curwin) + command('let result = []') + new_win = api.nvim_open_win(info.other_buf, true, { split = 'left', win = info.tab2_curwin }) + eq({ + { 'WinNew', new_win }, + { 'BufLeave', info.tab1_curwin, info.orig_buf }, + { 'WinLeave', info.tab1_curwin }, + { 'TabLeave', info.tab1 }, + + { 'WinEnter', info.tab2_curwin }, + { 'TabEnter', info.tab2 }, + { 'BufEnter', info.tab2_curwin, new_buf }, + { 'WinLeave', info.tab2_curwin }, + { 'WinEnter', new_win }, + { 'BufLeave', new_win, new_buf }, + { 'BufEnter', new_win, info.other_buf }, + { 'BufWinEnter', new_win, info.other_buf }, + }, eval('result')) + end) + + it('OK when new window is moved to other tabpage by autocommands', function() + -- Use nvim_win_set_config in the autocommands, as other methods of moving a window to a + -- different tabpage (e.g: wincmd T) actually creates a new window. + local tab0 = api.nvim_get_current_tabpage() + local tab0_win = api.nvim_get_current_win() + command('tabnew') + local new_buf = api.nvim_create_buf(true, true) + local tab1 = api.nvim_get_current_tabpage() + local tab1_parent = api.nvim_get_current_win() + command( + 'tabfirst | autocmd WinNew * ++once call nvim_win_set_config(0, #{split: "left", win: ' + .. tab1_parent + .. '})' + ) + local new_win = api.nvim_open_win(new_buf, true, { split = 'left' }) + eq(tab1, api.nvim_get_current_tabpage()) + eq(new_win, api.nvim_get_current_win()) + eq(new_buf, api.nvim_get_current_buf()) + + -- nvim_win_set_config called after entering. It doesn't follow a curwin that is moved to a + -- different tabpage, but instead moves to the win filling the space, which is tab0_win. + command( + 'tabfirst | autocmd WinEnter * ++once call nvim_win_set_config(0, #{split: "left", win: ' + .. tab1_parent + .. '})' + ) + new_win = api.nvim_open_win(new_buf, true, { split = 'left' }) + eq(tab0, api.nvim_get_current_tabpage()) + eq(tab0_win, api.nvim_get_current_win()) + eq(tab1, api.nvim_win_get_tabpage(new_win)) + eq(new_buf, api.nvim_win_get_buf(new_win)) + + command( + 'tabfirst | autocmd BufEnter * ++once call nvim_win_set_config(0, #{split: "left", win: ' + .. tab1_parent + .. '})' + ) + new_win = api.nvim_open_win(new_buf, true, { split = 'left' }) + eq(tab0, api.nvim_get_current_tabpage()) + eq(tab0_win, api.nvim_get_current_win()) + eq(tab1, api.nvim_win_get_tabpage(new_win)) + eq(new_buf, api.nvim_win_get_buf(new_win)) + end) + + it('does not fire BufWinEnter if win_set_buf fails', function() + exec([[ + set nohidden modified + autocmd WinNew * ++once only! + let fired = v:false + autocmd BufWinEnter * ++once let fired = v:true + ]]) + eq( + 'Failed to set buffer 2', + pcall_err(api.nvim_open_win, api.nvim_create_buf(true, true), false, { split = 'left' }) + ) + eq(false, eval('fired')) + end) end) describe('set_config', function() From e55a502ed413d2bc8954b5227acfb34c8689f979 Mon Sep 17 00:00:00 2001 From: Sean Dewar Date: Sun, 11 Feb 2024 18:45:56 +0000 Subject: [PATCH 04/22] fix(api): open_win fire Buf* events when !enter && !noautocmd if entered early Problem: if switch_win{_noblock} fails to restore the old curwin after WinNew (e.g: it was closed), wp will become the new curwin, but win_set_buf enter events would still be blocked if !enter && !noautocmd. Solution: fire them, as we've actually entered the new window. Note: there's a problem of switch_win{_noblock} failing to restore the old curwin, leaving us in wp without triggering WinEnter/WinLeave, but this affects all callers of switch_win{_noblock} anyways. (It's also not clear how WinLeave can be called if the old curwin was closed already). --- src/nvim/api/win_config.c | 2 +- test/functional/api/window_spec.lua | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/nvim/api/win_config.c b/src/nvim/api/win_config.c index 9d63a1997c..238ec5df1e 100644 --- a/src/nvim/api/win_config.c +++ b/src/nvim/api/win_config.c @@ -294,7 +294,7 @@ Window nvim_open_win(Buffer buffer, Boolean enter, Dict(win_config) *config, Err tp = win_find_tabpage(wp); } if (tp && buf != wp->w_buffer) { - const bool noautocmd = !enter || fconfig.noautocmd; + const bool noautocmd = curwin != wp || fconfig.noautocmd; win_set_buf(wp, buf, noautocmd, err); if (!noautocmd) { tp = win_find_tabpage(wp); diff --git a/test/functional/api/window_spec.lua b/test/functional/api/window_spec.lua index 8966c3b086..e0cb66de41 100644 --- a/test/functional/api/window_spec.lua +++ b/test/functional/api/window_spec.lua @@ -1571,6 +1571,16 @@ describe('API/win', function() ) eq(false, eval('fired')) end) + + it('fires Buf* autocommands when `!enter` if window is entered via autocommands', function() + exec([[ + autocmd WinNew * ++once only! + let fired = v:false + autocmd BufEnter * ++once let fired = v:true + ]]) + api.nvim_open_win(api.nvim_create_buf(true, true), false, { split = 'left' }) + eq(true, eval('fired')) + end) end) describe('set_config', function() From b1e24f240baeea80dcf4a3d8453fed0230fb88fd Mon Sep 17 00:00:00 2001 From: Sean Dewar Date: Sun, 11 Feb 2024 20:15:47 +0000 Subject: [PATCH 05/22] fix(api): avoid open_win UAF if target buf deleted by autocmds Problem: WinNew and win_enter autocommands can delete the target buffer to switch to, causing a heap-use-after-free. Solution: store a bufref to the buffer, check it before attempting to switch. --- src/nvim/api/win_config.c | 6 +++++- test/functional/api/window_spec.lua | 8 ++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/nvim/api/win_config.c b/src/nvim/api/win_config.c index 238ec5df1e..3959e74af9 100644 --- a/src/nvim/api/win_config.c +++ b/src/nvim/api/win_config.c @@ -12,6 +12,7 @@ #include "nvim/ascii_defs.h" #include "nvim/autocmd.h" #include "nvim/autocmd_defs.h" +#include "nvim/buffer.h" #include "nvim/buffer_defs.h" #include "nvim/decoration.h" #include "nvim/decoration_defs.h" @@ -279,6 +280,9 @@ Window nvim_open_win(Buffer buffer, Boolean enter, Dict(win_config) *config, Err // Autocommands may close `wp` or move it to another tabpage, so update and check `tp` after each // event. In each case, `wp` should already be valid in `tp`, so switch_win should not fail. + // Also, autocommands may free the `buf` to switch to, so store a bufref to check. + bufref_T bufref; + set_bufref(&bufref, buf); switchwin_T switchwin; { const int result = switch_win_noblock(&switchwin, wp, tp, true); @@ -293,7 +297,7 @@ Window nvim_open_win(Buffer buffer, Boolean enter, Dict(win_config) *config, Err goto_tabpage_win(tp, wp); tp = win_find_tabpage(wp); } - if (tp && buf != wp->w_buffer) { + if (tp && bufref_valid(&bufref) && buf != wp->w_buffer) { const bool noautocmd = curwin != wp || fconfig.noautocmd; win_set_buf(wp, buf, noautocmd, err); if (!noautocmd) { diff --git a/test/functional/api/window_spec.lua b/test/functional/api/window_spec.lua index e0cb66de41..2f6a02b5d5 100644 --- a/test/functional/api/window_spec.lua +++ b/test/functional/api/window_spec.lua @@ -1581,6 +1581,14 @@ describe('API/win', function() api.nvim_open_win(api.nvim_create_buf(true, true), false, { split = 'left' }) eq(true, eval('fired')) end) + + it('no heap-use-after-free if target buffer deleted by autocommands', function() + local cur_buf = api.nvim_get_current_buf() + local new_buf = api.nvim_create_buf(true, true) + command('autocmd WinNew * ++once call nvim_buf_delete(' .. new_buf .. ', #{force: 1})') + api.nvim_open_win(new_buf, true, { split = 'left' }) + eq(cur_buf, api.nvim_get_current_buf()) + end) end) describe('set_config', function() From 5d58136cccc760f6d95eb45b46f2ad60f06b103b Mon Sep 17 00:00:00 2001 From: Sean Dewar Date: Wed, 7 Feb 2024 17:17:44 +0000 Subject: [PATCH 06/22] fix(api): make open_win/win_set_config check if splitting allowed Problem: splitting is disallowed in some cases to prevent the window layout changes while a window is closing, but it's not checked for. Solution: check for this, and set the API error message directly. (Also sneak in a change to tui.c that got lost from #27352; it's a char* buf, and the memset is assuming one byte each anyway) --- src/nvim/api/win_config.c | 8 +++++ src/nvim/tui/tui.c | 2 +- src/nvim/window.c | 36 ++++++++++++++++------ test/functional/api/window_spec.lua | 47 +++++++++++++++++++++++++++++ 4 files changed, 82 insertions(+), 11 deletions(-) diff --git a/src/nvim/api/win_config.c b/src/nvim/api/win_config.c index 3959e74af9..557c2f37f9 100644 --- a/src/nvim/api/win_config.c +++ b/src/nvim/api/win_config.c @@ -246,6 +246,10 @@ Window nvim_open_win(Buffer buffer, Boolean enter, Dict(win_config) *config, Err } } + if (!check_split_disallowed_err(parent ? parent : curwin, err)) { + return 0; // error already set + } + if (HAS_KEY_X(config, vertical) && !HAS_KEY_X(config, split)) { if (config->vertical) { fconfig.split = p_spr ? kWinSplitRight : kWinSplitLeft; @@ -440,6 +444,10 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err) return; } + if (!check_split_disallowed_err(win, err)) { + return; // error already set + } + if (was_split) { win_T *new_curwin = NULL; diff --git a/src/nvim/tui/tui.c b/src/nvim/tui/tui.c index 7fae34d33f..c332c17e43 100644 --- a/src/nvim/tui/tui.c +++ b/src/nvim/tui/tui.c @@ -1643,7 +1643,7 @@ static void invalidate(TUIData *tui, int top, int bot, int left, int right) static void ensure_space_buf_size(TUIData *tui, size_t len) { if (len > tui->space_buf_len) { - tui->space_buf = xrealloc(tui->space_buf, len * sizeof *tui->space_buf); + tui->space_buf = xrealloc(tui->space_buf, len); memset(tui->space_buf + tui->space_buf_len, ' ', len - tui->space_buf_len); tui->space_buf_len = len; } diff --git a/src/nvim/window.c b/src/nvim/window.c index d5a6e347e7..81f8304b22 100644 --- a/src/nvim/window.c +++ b/src/nvim/window.c @@ -905,19 +905,35 @@ void ui_ext_win_viewport(win_T *wp) } } -/// If "split_disallowed" is set give an error and return FAIL. +/// If "split_disallowed" is set or "wp"s buffer is closing, give an error and return FAIL. /// Otherwise return OK. -static int check_split_disallowed(void) +static int check_split_disallowed(const win_T *wp) + FUNC_ATTR_NONNULL_ALL +{ + Error err = ERROR_INIT; + const bool ok = check_split_disallowed_err(wp, &err); + if (ERROR_SET(&err)) { + emsg(_(err.msg)); + api_clear_error(&err); + } + return ok ? OK : FAIL; +} + +/// Like `check_split_disallowed`, but set `err` to the (untranslated) error message on failure and +/// return false. Otherwise return true. +/// @see check_split_disallowed +bool check_split_disallowed_err(const win_T *wp, Error *err) + FUNC_ATTR_NONNULL_ALL { if (split_disallowed > 0) { - emsg(_("E242: Can't split a window while closing another")); - return FAIL; + api_set_error(err, kErrorTypeException, "E242: Can't split a window while closing another"); + return false; } - if (curwin->w_buffer->b_locked_split) { - emsg(_(e_cannot_split_window_when_closing_buffer)); - return FAIL; + if (wp->w_buffer->b_locked_split) { + api_set_error(err, kErrorTypeException, "%s", e_cannot_split_window_when_closing_buffer); + return false; } - return OK; + return true; } // split the current window, implements CTRL-W s and :split @@ -936,7 +952,7 @@ static int check_split_disallowed(void) // return FAIL for failure, OK otherwise int win_split(int size, int flags) { - if (check_split_disallowed() == FAIL) { + if (check_split_disallowed(curwin) == FAIL) { return FAIL; } @@ -1871,7 +1887,7 @@ static void win_totop(int size, int flags) if (is_aucmd_win(curwin)) { return; } - if (check_split_disallowed() == FAIL) { + if (check_split_disallowed(curwin) == FAIL) { return; } diff --git a/test/functional/api/window_spec.lua b/test/functional/api/window_spec.lua index 2f6a02b5d5..30235d6b84 100644 --- a/test/functional/api/window_spec.lua +++ b/test/functional/api/window_spec.lua @@ -1589,6 +1589,37 @@ describe('API/win', function() api.nvim_open_win(new_buf, true, { split = 'left' }) eq(cur_buf, api.nvim_get_current_buf()) end) + + it('checks if splitting disallowed', function() + command('split | autocmd WinEnter * ++once call nvim_open_win(0, 0, #{split: "right"})') + matches("E242: Can't split a window while closing another$", pcall_err(command, 'quit')) + + command('only | autocmd BufHidden * ++once call nvim_open_win(0, 0, #{split: "left"})') + matches( + 'E1159: Cannot split a window when closing the buffer$', + pcall_err(command, 'new | quit') + ) + + local w = api.nvim_get_current_win() + command( + 'only | new | autocmd BufHidden * ++once call nvim_open_win(0, 0, #{split: "left", win: ' + .. w + .. '})' + ) + matches( + 'E1159: Cannot split a window when closing the buffer$', + pcall_err(api.nvim_win_close, w, true) + ) + + -- OK when using window to different buffer than `win`s. + w = api.nvim_get_current_win() + command( + 'only | autocmd BufHidden * ++once call nvim_open_win(0, 0, #{split: "left", win: ' + .. w + .. '})' + ) + command('new | quit') + end) end) describe('set_config', function() @@ -1941,6 +1972,22 @@ describe('API/win', function() -- Shouldn't see any of those events, as we remain in the same window. eq({}, eval('result')) end) + + it('checks if splitting disallowed', function() + command('split | autocmd WinEnter * ++once call nvim_win_set_config(0, #{split: "right"})') + matches("E242: Can't split a window while closing another$", pcall_err(command, 'quit')) + + command('autocmd BufHidden * ++once call nvim_win_set_config(0, #{split: "left"})') + matches( + 'E1159: Cannot split a window when closing the buffer$', + pcall_err(command, 'new | quit') + ) + + -- OK when using window to different buffer. + local w = api.nvim_get_current_win() + command('autocmd BufHidden * ++once call nvim_win_set_config(' .. w .. ', #{split: "left"})') + command('new | quit') + end) end) describe('get_config', function() From b1577d371a6db43222de9e3a525def82320ebdb1 Mon Sep 17 00:00:00 2001 From: Sean Dewar Date: Wed, 7 Feb 2024 21:44:42 +0000 Subject: [PATCH 07/22] fix(api): make win_set_config with "win" for splits need "split/vertical" Problem: currently, for splits, nvim_win_set_config accepts win without any of split or vertical set, which has little effect and seems error-prone. Solution: require at least one of split or vertical to also be set for splits. Also, update nvim_win_set_config docs, as it's no longer limited to just floating and external windows. --- runtime/doc/api.txt | 8 ++++---- runtime/lua/vim/_meta/api.lua | 8 ++++---- src/nvim/api/win_config.c | 16 ++++++++++------ test/functional/api/window_spec.lua | 9 +++++++++ 4 files changed, 27 insertions(+), 14 deletions(-) diff --git a/runtime/doc/api.txt b/runtime/doc/api.txt index 1b00777532..125976342b 100644 --- a/runtime/doc/api.txt +++ b/runtime/doc/api.txt @@ -3291,11 +3291,11 @@ nvim_win_get_config({window}) *nvim_win_get_config()* Map defining the window configuration, see |nvim_open_win()| nvim_win_set_config({window}, {config}) *nvim_win_set_config()* - Configures window layout. Currently only for floating and external windows - (including changing a split window to those layouts). + Configures window layout. Cannot be used to move the last window in a + tabpage to a different one. - When reconfiguring a floating window, absent option keys will not be - changed. `row`/`col` and `relative` must be reconfigured together. + When reconfiguring a window, absent option keys will not be changed. + `row`/`col` and `relative` must be reconfigured together. Parameters: ~ • {window} Window handle, or 0 for current window diff --git a/runtime/lua/vim/_meta/api.lua b/runtime/lua/vim/_meta/api.lua index 94eab72291..4a179d49f3 100644 --- a/runtime/lua/vim/_meta/api.lua +++ b/runtime/lua/vim/_meta/api.lua @@ -2207,11 +2207,11 @@ function vim.api.nvim_win_remove_ns(window, ns_id) end --- @param buffer integer Buffer handle function vim.api.nvim_win_set_buf(window, buffer) end ---- Configures window layout. Currently only for floating and external windows ---- (including changing a split window to those layouts). +--- Configures window layout. Cannot be used to move the last window in a +--- tabpage to a different one. --- ---- When reconfiguring a floating window, absent option keys will not be ---- changed. `row`/`col` and `relative` must be reconfigured together. +--- When reconfiguring a window, absent option keys will not be changed. +--- `row`/`col` and `relative` must be reconfigured together. --- --- @param window integer Window handle, or 0 for current window --- @param config vim.api.keyset.win_config Map defining the window configuration, see `nvim_open_win()` diff --git a/src/nvim/api/win_config.c b/src/nvim/api/win_config.c index 557c2f37f9..21d6d59b1e 100644 --- a/src/nvim/api/win_config.c +++ b/src/nvim/api/win_config.c @@ -361,11 +361,11 @@ static int win_split_flags(WinSplit split, bool toplevel) return flags; } -/// Configures window layout. Currently only for floating and external windows -/// (including changing a split window to those layouts). +/// Configures window layout. Cannot be used to move the last window in a +/// tabpage to a different one. /// -/// When reconfiguring a floating window, absent option keys will not be -/// changed. `row`/`col` and `relative` must be reconfigured together. +/// When reconfiguring a window, absent option keys will not be changed. +/// `row`/`col` and `relative` must be reconfigured together. /// /// @see |nvim_open_win()| /// @@ -1099,11 +1099,15 @@ static bool parse_float_config(Dict(win_config) *config, WinConfig *fconfig, boo fconfig->window = config->win; } } - } else if (has_relative) { - if (HAS_KEY_X(config, win)) { + } else if (HAS_KEY_X(config, win)) { + if (has_relative) { api_set_error(err, kErrorTypeValidation, "'win' key is only valid with relative='win' and relative=''"); return false; + } else if (!is_split) { + api_set_error(err, kErrorTypeValidation, + "non-float with 'win' requires at least 'split' or 'vertical'"); + return false; } } diff --git a/test/functional/api/window_spec.lua b/test/functional/api/window_spec.lua index 30235d6b84..246ff50ddc 100644 --- a/test/functional/api/window_spec.lua +++ b/test/functional/api/window_spec.lua @@ -1727,6 +1727,15 @@ describe('API/win', function() config = api.nvim_win_get_config(win) eq('', config.relative) eq('below', config.split) + + eq( + "non-float with 'win' requires at least 'split' or 'vertical'", + pcall_err(api.nvim_win_set_config, 0, { win = 0 }) + ) + eq( + "non-float with 'win' requires at least 'split' or 'vertical'", + pcall_err(api.nvim_win_set_config, 0, { win = 0, relative = '' }) + ) end) it('creates top-level splits', function() From a70eae57bd44208a77b5ac29839e8a39ab3c9cd8 Mon Sep 17 00:00:00 2001 From: Sean Dewar Date: Sun, 11 Feb 2024 22:53:37 +0000 Subject: [PATCH 08/22] fix(api): make open_win block only enter/leave events if !enter && !noautocmd Problem: nvim_open_win blocking all win_set_buf autocommands when !enter && !noautocmd is too aggressive. Solution: temporarily block WinEnter/Leave and BufEnter/Leave events when setting the buffer. Delegate the firing of BufWinEnter back to win_set_buf, which also has the advantage of keeping the timing consistent (e.g: before the epilogue in enter_buffer, which also handles restoring the cursor position if autocommands didn't change it, among other things). Reword the documentation for noautocmd a bit. I pondered modifying do_buffer and callees to allow for BufEnter/Leave being conditionally disabled, but it seems too invasive (and potentially error-prone, especially if new code paths to BufEnter/Leave are added in the future). Unfortunately, doing this has the drawback of blocking ALL such events for the duration, which also means blocking unrelated such events; like if window switching occurs in a ++nested autocmd fired by win_set_buf. If this turns out to be a problem in practice, a different solution specialized for nvim_open_win could be considered. :-) --- runtime/doc/api.txt | 6 ++--- runtime/lua/vim/_meta/api.lua | 6 ++--- src/nvim/api/win_config.c | 30 ++++++++++++------------- test/functional/api/window_spec.lua | 34 +++++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 21 deletions(-) diff --git a/runtime/doc/api.txt b/runtime/doc/api.txt index 125976342b..adaf66c2d9 100644 --- a/runtime/doc/api.txt +++ b/runtime/doc/api.txt @@ -3265,9 +3265,9 @@ nvim_open_win({buffer}, {enter}, {config}) *nvim_open_win()* • footer_pos: Footer position. Must be set with `footer` option. Value can be one of "left", "center", or "right". Default is `"left"`. - • noautocmd: If true then no buffer-related autocommand - events such as |BufEnter|, |BufLeave| or |BufWinEnter| may - fire from calling this function. + • noautocmd: If true then autocommands triggered from + setting the `buffer` to display are blocked (e.g: + |BufEnter|, |BufLeave|, |BufWinEnter|). • fixed: If true when anchor is NW or SW, the float window would be kept fixed even if the window would be truncated. • hide: If true the floating window will be hidden. diff --git a/runtime/lua/vim/_meta/api.lua b/runtime/lua/vim/_meta/api.lua index 4a179d49f3..f2f5f43c26 100644 --- a/runtime/lua/vim/_meta/api.lua +++ b/runtime/lua/vim/_meta/api.lua @@ -1719,9 +1719,9 @@ function vim.api.nvim_open_term(buffer, opts) end --- • footer_pos: Footer position. Must be set with `footer` --- option. Value can be one of "left", "center", or "right". --- Default is `"left"`. ---- • noautocmd: If true then no buffer-related autocommand ---- events such as `BufEnter`, `BufLeave` or `BufWinEnter` may ---- fire from calling this function. +--- • noautocmd: If true then autocommands triggered from +--- setting the `buffer` to display are blocked (e.g: +--- `BufEnter`, `BufLeave`, `BufWinEnter`). --- • fixed: If true when anchor is NW or SW, the float window --- would be kept fixed even if the window would be truncated. --- • hide: If true the floating window will be hidden. diff --git a/src/nvim/api/win_config.c b/src/nvim/api/win_config.c index 21d6d59b1e..8608b0dde9 100644 --- a/src/nvim/api/win_config.c +++ b/src/nvim/api/win_config.c @@ -199,9 +199,9 @@ /// - footer_pos: Footer position. Must be set with `footer` option. /// Value can be one of "left", "center", or "right". /// Default is `"left"`. -/// - noautocmd: If true then no buffer-related autocommand events such as -/// |BufEnter|, |BufLeave| or |BufWinEnter| may fire from -/// calling this function. +/// - noautocmd: If true then autocommands triggered from setting the +/// `buffer` to display are blocked (e.g: |BufEnter|, |BufLeave|, +/// |BufWinEnter|). /// - fixed: If true when anchor is NW or SW, the float window /// would be kept fixed even if the window would be truncated. /// - hide: If true the floating window will be hidden. @@ -302,20 +302,20 @@ Window nvim_open_win(Buffer buffer, Boolean enter, Dict(win_config) *config, Err tp = win_find_tabpage(wp); } if (tp && bufref_valid(&bufref) && buf != wp->w_buffer) { - const bool noautocmd = curwin != wp || fconfig.noautocmd; - win_set_buf(wp, buf, noautocmd, err); - if (!noautocmd) { + // win_set_buf temporarily makes `wp` the curwin to set the buffer. + // If not entering `wp`, block Enter and Leave events. (cringe) + const bool au_no_enter_leave = curwin != wp && !fconfig.noautocmd; + if (au_no_enter_leave) { + autocmd_no_enter++; + autocmd_no_leave++; + } + win_set_buf(wp, buf, fconfig.noautocmd, err); + if (!fconfig.noautocmd) { tp = win_find_tabpage(wp); } - // win_set_buf autocommands were blocked if we didn't enter, but we still want BufWinEnter. - if (noautocmd && !fconfig.noautocmd && wp->w_buffer == buf) { - const int result = switch_win_noblock(&switchwin, wp, tp, true); - assert(result == OK); - (void)result; - if (apply_autocmds(EVENT_BUFWINENTER, NULL, NULL, false, buf)) { - tp = win_find_tabpage(wp); - } - restore_win_noblock(&switchwin, true); + if (au_no_enter_leave) { + autocmd_no_enter--; + autocmd_no_leave--; } } if (!tp) { diff --git a/test/functional/api/window_spec.lua b/test/functional/api/window_spec.lua index 246ff50ddc..6e20c81fc2 100644 --- a/test/functional/api/window_spec.lua +++ b/test/functional/api/window_spec.lua @@ -1620,6 +1620,40 @@ describe('API/win', function() ) command('new | quit') end) + + it('restores last known cursor position if BufWinEnter did not move it', function() + -- This test mostly exists to ensure BufWinEnter is executed before enter_buffer's epilogue. + local buf = api.nvim_get_current_buf() + insert([[ + foo + bar baz .etc + i love autocommand bugs! + supercalifragilisticexpialidocious + marvim is actually a human + llanfairpwllgwyngyllgogerychwyrndrobwllllantysiliogogogoch + ]]) + api.nvim_win_set_cursor(0, { 5, 2 }) + command('set nostartofline | enew') + local new_win = api.nvim_open_win(buf, false, { split = 'left' }) + eq({ 5, 2 }, api.nvim_win_get_cursor(new_win)) + + exec([[ + only! + autocmd BufWinEnter * ++once normal! j6l + ]]) + new_win = api.nvim_open_win(buf, false, { split = 'left' }) + eq({ 2, 6 }, api.nvim_win_get_cursor(new_win)) + end) + + it('does not block all win_set_buf autocommands if !enter and !noautocmd', function() + local new_buf = fn.bufadd('foobarbaz') + exec([[ + let triggered = "" + autocmd BufReadCmd * ++once let triggered = bufname() + ]]) + api.nvim_open_win(new_buf, false, { split = 'left' }) + eq('foobarbaz', eval('triggered')) + end) end) describe('set_config', function() From 66f331fef7ad3df480bd02f1705e176d1a07c785 Mon Sep 17 00:00:00 2001 From: Sean Dewar <6256228+seandewar@users.noreply.github.com> Date: Fri, 23 Feb 2024 09:49:28 +0000 Subject: [PATCH 09/22] vim-patch:9.1.0116: win_split_ins may not check available room Problem: win_split_ins has no check for E36 when moving an existing window Solution: check for room and fix the issues in f_win_splitmove() (Sean Dewar) https://github.com/vim/vim/commit/0fd44a5ad81ade342cb54d8984965bdedd2272c8 Omit WSP_FORCE_ROOM, as it's not needed for Nvim's autocmd window, which is floating. Shouldn't be difficult to port later if it's used for anything else. Make win_splitmove continue working for turning floating windows into splits. Move the logic for "unfloating" a float to win_split_ins; unlike splits, no changes to the window layout are needed before calling it, as floats take no room in the window layout and cannot affect the e_noroom check. Add missing tp_curwin-fixing logic for turning external windows into splits, and add a test. NOTE: there are other issues with the way "tabpage independence" is implemented for external windows; namely, some things assume that tp_curwin is indeed a window within that tabpage, and as such, functions like tabpage_winnr and nvim_tabpage_get_win currently don't always work for external windows (with the latter aborting!) Use last_status over frame_add_statusline, as Nvim's last_status already does this for all windows in the current tabpage. Adjust restore_full_snapshot_rec to handle this. This "restore everything" approach is changed in a future commit anyway, so only ensure it's robust enough to just pass tests. Keep check_split_disallowed's current doc comment, as it's actually a bit more accurate here. (I should probably PR Vim to use this one) Allow f_win_splitmove to move a floating "wp" into a split; Nvim supports this. Continue to disallow it from moving the autocommand window into a split (funnily enough, the check wasn't reachable before, as moving a float was disallowed), but now return -1 in that case (win_splitmove also returns FAIL for this, but handling it in f_win_splitmove avoids us needing to switch windows first). Cherry-pick Test_window_split_no_room fix from v9.1.0121. Update nvim_win_set_config to handle win_split_ins failure in later commits. --- src/nvim/api/win_config.c | 12 -- src/nvim/buffer.c | 1 - src/nvim/eval/window.c | 66 +++----- src/nvim/globals.h | 1 + src/nvim/window.c | 217 ++++++++++++++++++++------- test/functional/ui/float_spec.lua | 53 +++++++ test/old/testdir/test_window_cmd.vim | 161 ++++++++++++++++++-- 7 files changed, 391 insertions(+), 120 deletions(-) diff --git a/src/nvim/api/win_config.c b/src/nvim/api/win_config.c index 8608b0dde9..c308cadb7c 100644 --- a/src/nvim/api/win_config.c +++ b/src/nvim/api/win_config.c @@ -526,18 +526,6 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err) } } else { win_remove(win, win_tp == curtab ? NULL : win_tp); - ui_comp_remove_grid(&win->w_grid_alloc); - if (win->w_config.external) { - for (tabpage_T *tp = first_tabpage; tp != NULL; tp = tp->tp_next) { - if (tp == curtab) { - continue; - } - if (tp->tp_curwin == win) { - tp->tp_curwin = tp->tp_firstwin; - } - } - } - win->w_pos_changed = true; } int flags = win_split_flags(fconfig.split, parent == NULL) | WSP_NOENTER; diff --git a/src/nvim/buffer.c b/src/nvim/buffer.c index f6c7229485..b013f43ceb 100644 --- a/src/nvim/buffer.c +++ b/src/nvim/buffer.c @@ -117,7 +117,6 @@ # include "buffer.c.generated.h" #endif -static const char *e_auabort = N_("E855: Autocommands caused command to abort"); static const char e_attempt_to_delete_buffer_that_is_in_use_str[] = N_("E937: Attempt to delete a buffer that is in use: %s"); diff --git a/src/nvim/eval/window.c b/src/nvim/eval/window.c index b8aa0c9641..17b8b01963 100644 --- a/src/nvim/eval/window.c +++ b/src/nvim/eval/window.c @@ -659,55 +659,19 @@ void f_win_screenpos(typval_T *argvars, typval_T *rettv, EvalFuncData fptr) tv_list_append_number(rettv->vval.v_list, wp == NULL ? 0 : wp->w_wincol + 1); } -/// Move the window wp into a new split of targetwin in a given direction -static void win_move_into_split(win_T *wp, win_T *targetwin, int size, int flags) -{ - int height = wp->w_height; - win_T *oldwin = curwin; - - if (wp == targetwin || is_aucmd_win(wp)) { - return; - } - - // Jump to the target window - if (curwin != targetwin) { - win_goto(targetwin); - } - - // Remove the old window and frame from the tree of frames - int dir; - winframe_remove(wp, &dir, NULL); - win_remove(wp, NULL); - last_status(false); // may need to remove last status line - win_comp_pos(); // recompute window positions - - // Split a window on the desired side and put the old window there - win_split_ins(size, flags, wp, dir); - - // If splitting horizontally, try to preserve height - if (size == 0 && !(flags & WSP_VERT)) { - win_setheight_win(height, wp); - if (p_ea) { - win_equal(wp, true, 'v'); - } - } - - if (oldwin != curwin) { - win_goto(oldwin); - } -} - /// "win_splitmove()" function void f_win_splitmove(typval_T *argvars, typval_T *rettv, EvalFuncData fptr) { win_T *wp = find_win_by_nr_or_id(&argvars[0]); win_T *targetwin = find_win_by_nr_or_id(&argvars[1]); + win_T *oldwin = curwin; + + rettv->vval.v_number = -1; if (wp == NULL || targetwin == NULL || wp == targetwin || !win_valid(wp) || !win_valid(targetwin) - || win_float_valid(wp) || win_float_valid(targetwin)) { + || targetwin->w_floating) { emsg(_(e_invalwindow)); - rettv->vval.v_number = -1; return; } @@ -732,7 +696,27 @@ void f_win_splitmove(typval_T *argvars, typval_T *rettv, EvalFuncData fptr) size = (int)tv_dict_get_number(d, "size"); } - win_move_into_split(wp, targetwin, size, flags); + // Check if we can split the target before we bother switching windows. + if (is_aucmd_win(wp) || check_split_disallowed(targetwin) == FAIL) { + return; + } + + if (curwin != targetwin) { + win_goto(targetwin); + } + + // Autocommands may have sent us elsewhere or closed "wp" or "oldwin". + if (curwin == targetwin && win_valid(wp)) { + if (win_splitmove(wp, size, flags) == OK) { + rettv->vval.v_number = 0; + } + } else { + emsg(_(e_auabort)); + } + + if (oldwin != curwin && win_valid(oldwin)) { + win_goto(oldwin); + } } /// "win_gettype(nr)" function diff --git a/src/nvim/globals.h b/src/nvim/globals.h index 22f7daa823..c1c9ae456c 100644 --- a/src/nvim/globals.h +++ b/src/nvim/globals.h @@ -939,6 +939,7 @@ EXTERN const char e_using_float_as_string[] INIT(= N_("E806: Using a Float as a EXTERN const char e_cannot_edit_other_buf[] INIT(= N_("E788: Not allowed to edit another buffer now")); EXTERN const char e_using_number_as_bool_nr[] INIT(= N_("E1023: Using a Number as a Bool: %d")); EXTERN const char e_not_callable_type_str[] INIT(= N_("E1085: Not a callable type: %s")); +EXTERN const char e_auabort[] INIT(= N_("E855: Autocommands caused command to abort")); EXTERN const char e_api_error[] INIT(= N_("E5555: API call: %s")); diff --git a/src/nvim/window.c b/src/nvim/window.c index 81f8304b22..cfa28bbc1f 100644 --- a/src/nvim/window.c +++ b/src/nvim/window.c @@ -455,9 +455,14 @@ newwindow: case 'H': case 'L': CHECK_CMDWIN; - win_totop(Prenum, - ((nchar == 'H' || nchar == 'L') ? WSP_VERT : 0) - | ((nchar == 'H' || nchar == 'K') ? WSP_TOP : WSP_BOT)); + if (firstwin == curwin && lastwin_nofloating() == curwin) { + beep_flush(); + } else { + const int dir = ((nchar == 'H' || nchar == 'L') ? WSP_VERT : 0) + | ((nchar == 'H' || nchar == 'K') ? WSP_TOP : WSP_BOT); + + win_splitmove(curwin, Prenum, dir); + } break; // make all windows the same width and/or height @@ -907,7 +912,7 @@ void ui_ext_win_viewport(win_T *wp) /// If "split_disallowed" is set or "wp"s buffer is closing, give an error and return FAIL. /// Otherwise return OK. -static int check_split_disallowed(const win_T *wp) +int check_split_disallowed(const win_T *wp) FUNC_ATTR_NONNULL_ALL { Error err = ERROR_INIT; @@ -1004,13 +1009,12 @@ win_T *win_split_ins(int size, int flags, win_T *new_wp, int dir) int need_status = 0; int new_size = size; - bool new_in_layout = (new_wp == NULL || new_wp->w_floating); bool vertical = flags & WSP_VERT; bool toplevel = flags & (WSP_TOP | WSP_BOT); // add a status line when p_ls == 1 and splitting the first window if (one_nonfloat() && p_ls == 1 && oldwin->w_status_height == 0) { - if (oldwin->w_height <= p_wmh && new_in_layout) { + if (oldwin->w_height <= p_wmh) { emsg(_(e_noroom)); return NULL; } @@ -1059,7 +1063,7 @@ win_T *win_split_ins(int size, int flags, win_T *new_wp, int dir) available = oldwin->w_frame->fr_width; needed += minwidth; } - if (available < needed && new_in_layout) { + if (available < needed) { emsg(_(e_noroom)); return NULL; } @@ -1139,7 +1143,7 @@ win_T *win_split_ins(int size, int flags, win_T *new_wp, int dir) available = oldwin->w_frame->fr_height; needed += minheight; } - if (available < needed && new_in_layout) { + if (available < needed) { emsg(_(e_noroom)); return NULL; } @@ -1229,8 +1233,27 @@ win_T *win_split_ins(int size, int flags, win_T *new_wp, int dir) // make the contents of the new window the same as the current one win_init(wp, curwin, flags); } else if (wp->w_floating) { - new_frame(wp); + ui_comp_remove_grid(&wp->w_grid_alloc); + if (ui_has(kUIMultigrid)) { + wp->w_pos_changed = true; + } else { + // No longer a float, a non-multigrid UI shouldn't draw it as such + ui_call_win_hide(wp->w_grid_alloc.handle); + win_free_grid(wp, true); + } + + // External windows are independent of tabpages, and may have been the curwin of others. + if (wp->w_config.external) { + FOR_ALL_TABS(tp) { + if (tp != curtab && tp->tp_curwin == wp) { + tp->tp_curwin = tp->tp_firstwin; + } + } + } + wp->w_floating = false; + new_frame(wp); + // non-floating window doesn't store float config or have a border. wp->w_config = WIN_CONFIG_INIT; CLEAR_FIELD(wp->w_border_adj); @@ -1874,48 +1897,67 @@ static void win_rotate(bool upwards, int count) redraw_all_later(UPD_NOT_VALID); } -// Move the current window to the very top/bottom/left/right of the screen. -static void win_totop(int size, int flags) +/// Move "wp" into a new split in a given direction, possibly relative to the +/// current window. +/// "wp" must be valid in the current tabpage. +/// Returns FAIL for failure, OK otherwise. +int win_splitmove(win_T *wp, int size, int flags) { int dir = 0; - int height = curwin->w_height; + int height = wp->w_height; - if (firstwin == curwin && lastwin_nofloating() == curwin) { - beep_flush(); - return; + if (firstwin == wp && lastwin_nofloating() == wp) { + return OK; // nothing to do } - if (is_aucmd_win(curwin)) { - return; - } - if (check_split_disallowed(curwin) == FAIL) { - return; + if (is_aucmd_win(wp) || check_split_disallowed(wp) == FAIL) { + return FAIL; } - if (curwin->w_floating) { - ui_comp_remove_grid(&curwin->w_grid_alloc); - if (ui_has(kUIMultigrid)) { - curwin->w_pos_changed = true; - } else { - // No longer a float, a non-multigrid UI shouldn't draw it as such - ui_call_win_hide(curwin->w_grid_alloc.handle); - win_free_grid(curwin, true); - } + frame_T *frp = NULL; + if (wp->w_floating) { + win_remove(wp, NULL); } else { - // Remove the window and frame from the tree of frames. - winframe_remove(curwin, &dir, NULL); - } - win_remove(curwin, NULL); - last_status(false); // may need to remove last status line - win_comp_pos(); // recompute window positions + // Undoing changes to frames if splitting fails is complicated. + // Save a full snapshot to restore instead. + frp = make_full_snapshot(); - // Split a window on the desired side and put the window there. - win_split_ins(size, flags, curwin, dir); - if (!(flags & WSP_VERT)) { - win_setheight(height); + // Remove the window and frame from the tree of frames. + winframe_remove(wp, &dir, NULL); + win_remove(wp, NULL); + last_status(false); // may need to remove last status line + win_comp_pos(); // recompute window positions + } + + // Split a window on the desired side and put "wp" there. + if (win_split_ins(size, flags, wp, dir) == NULL) { + win_append(wp->w_prev, wp); + if (!wp->w_floating) { + // Restore the previous layout from the snapshot. + xfree(wp->w_frame); + restore_full_snapshot(frp); + + // Vertical separators to the left may have been lost. Restore them. + frp = wp->w_frame; + if (frp->fr_parent->fr_layout == FR_ROW && frp->fr_prev != NULL) { + frame_add_vsep(frp->fr_prev); + } + } + return FAIL; + } + clear_snapshot_rec(frp); + + // If splitting horizontally, try to preserve height. + // Note that win_split_ins autocommands may have immediately made "wp" floating! + if (size == 0 && !(flags & WSP_VERT) && !wp->w_floating) { + win_setheight_win(height, wp); if (p_ea) { - win_equal(curwin, true, 'v'); + // Equalize windows. Note that win_split_ins autocommands may have + // made a window other than "wp" current. + win_equal(curwin, curwin == wp, 'v'); } } + + return OK; } // Move window "win1" to below/right of "win2" and make "win1" the current @@ -2777,13 +2819,10 @@ int win_close(win_T *win, bool free_buf, bool force) ui_comp_remove_grid(&win->w_grid_alloc); assert(first_tabpage != NULL); // suppress clang "Dereference of NULL pointer" if (win->w_config.external) { - for (tabpage_T *tp = first_tabpage; tp != NULL; tp = tp->tp_next) { - if (tp == curtab) { - continue; - } - if (tp->tp_curwin == win) { + FOR_ALL_TABS(tp) { + if (tp != curtab && tp->tp_curwin == win) { // NB: an autocmd can still abort the closing of this window, - // bur carring out this change anyway shouldn't be a catastrophe. + // but carrying out this change anyway shouldn't be a catastrophe. tp->tp_curwin = tp->tp_firstwin; } } @@ -7207,23 +7246,23 @@ void reset_lnums(void) void make_snapshot(int idx) { clear_snapshot(curtab, idx); - make_snapshot_rec(topframe, &curtab->tp_snapshot[idx]); + make_snapshot_rec(topframe, &curtab->tp_snapshot[idx], false); } -static void make_snapshot_rec(frame_T *fr, frame_T **frp) +static void make_snapshot_rec(frame_T *fr, frame_T **frp, bool snap_wins) { *frp = xcalloc(1, sizeof(frame_T)); (*frp)->fr_layout = fr->fr_layout; (*frp)->fr_width = fr->fr_width; (*frp)->fr_height = fr->fr_height; if (fr->fr_next != NULL) { - make_snapshot_rec(fr->fr_next, &((*frp)->fr_next)); + make_snapshot_rec(fr->fr_next, &((*frp)->fr_next), snap_wins); } if (fr->fr_child != NULL) { - make_snapshot_rec(fr->fr_child, &((*frp)->fr_child)); + make_snapshot_rec(fr->fr_child, &((*frp)->fr_child), snap_wins); } - if (fr->fr_layout == FR_LEAF && fr->fr_win == curwin) { - (*frp)->fr_win = curwin; + if (fr->fr_layout == FR_LEAF && (snap_wins || fr->fr_win == curwin)) { + (*frp)->fr_win = fr->fr_win; } } @@ -7340,6 +7379,80 @@ static win_T *restore_snapshot_rec(frame_T *sn, frame_T *fr) return wp; } +/// Return a snapshot of all frames in the current tabpage and which windows are +/// in them. +/// Use clear_snapshot_rec to free the snapshot. +static frame_T *make_full_snapshot(void) +{ + frame_T *frp; + make_snapshot_rec(topframe, &frp, true); + return frp; +} + +/// Restore all frames in the full snapshot "sn" for the current tabpage. +/// Caller must ensure that the screen size didn't change, no windows with frames +/// in the snapshot were freed, and windows with frames not in the snapshot are +/// removed from their frames! +/// Doesn't restore changed window vertical separators. +/// Frees the old frames. Don't call clear_snapshot_rec on "sn" afterwards! +static void restore_full_snapshot(frame_T *sn) +{ + if (sn == NULL) { + return; + } + + clear_snapshot_rec(topframe); + restore_full_snapshot_rec(sn); + curtab->tp_topframe = topframe = sn; + last_status(false); + + // If the amount of space available changed, first try setting the sizes of + // windows with 'winfix{width,height}'. If that doesn't result in the right + // size, forget about that option. + if (topframe->fr_width != Columns) { + frame_new_width(topframe, Columns, false, true); + if (!frame_check_width(topframe, Columns)) { + frame_new_width(topframe, Columns, false, false); + } + } + if (topframe->fr_height != ROWS_AVAIL) { + frame_new_height(topframe, (int)ROWS_AVAIL, false, true); + if (!frame_check_height(topframe, (int)ROWS_AVAIL)) { + frame_new_height(topframe, (int)ROWS_AVAIL, false, false); + } + } + + win_comp_pos(); +} + +static void restore_full_snapshot_rec(frame_T *sn) +{ + if (sn == NULL) { + return; + } + + if (sn->fr_child != NULL) { + sn->fr_child->fr_parent = sn; + } + if (sn->fr_next != NULL) { + sn->fr_next->fr_parent = sn->fr_parent; + sn->fr_next->fr_prev = sn; + } + if (sn->fr_win != NULL) { + sn->fr_win->w_frame = sn; + // Assume for now that all windows have statuslines, so last_status in restore_full_snapshot + // doesn't resize frames to fit any missing statuslines. + sn->fr_win->w_status_height = STATUS_HEIGHT; + sn->fr_win->w_hsep_height = 0; + + // Resize window to fit the frame. + frame_new_height(sn, sn->fr_height, false, false); + frame_new_width(sn, sn->fr_width, false, false); + } + restore_full_snapshot_rec(sn->fr_child); + restore_full_snapshot_rec(sn->fr_next); +} + /// Check that "topfrp" and its children are at the right height. /// /// @param topfrp top frame pointer diff --git a/test/functional/ui/float_spec.lua b/test/functional/ui/float_spec.lua index cdb3b79963..e324d03b30 100644 --- a/test/functional/ui/float_spec.lua +++ b/test/functional/ui/float_spec.lua @@ -550,6 +550,43 @@ describe('float window', function() eq({ w0 }, api.nvim_list_wins()) end) + it('win_splitmove() can move float into a split', function() + command('split') + eq({'col', {{'leaf', 1001}, {'leaf', 1000}}}, fn.winlayout()) + + local win1 = api.nvim_open_win(0, true, {relative = 'editor', row = 1, col = 1, width = 5, height = 5}) + fn.win_splitmove(win1, 1001, {vertical = true}) + eq({'col', {{'row', {{'leaf', win1}, {'leaf', 1001}}}, {'leaf', 1000}}}, fn.winlayout()) + eq('', api.nvim_win_get_config(win1).relative) + + -- Should be unable to create a split relative to a float, though. + local win2 = api.nvim_open_win(0, true, {relative = 'editor', row = 1, col = 1, width = 5, height = 5}) + eq('Vim:E957: Invalid window number', pcall_err(fn.win_splitmove, win1, win2, {vertical = true})) + end) + + it('tp_curwin updated if external window is moved into split', function() + local screen = Screen.new(20, 7) + screen:attach { ext_multigrid = true } + + command('tabnew') + local external_win = api.nvim_open_win(0, true, {external = true, width = 5, height = 5}) + eq(external_win, api.nvim_get_current_win()) + eq(2, fn.tabpagenr()) + command('tabfirst') + api.nvim_set_current_win(external_win) + eq(external_win, api.nvim_get_current_win()) + eq(1, fn.tabpagenr()) + + command('wincmd J') + eq(external_win, api.nvim_get_current_win()) + eq(false, api.nvim_win_get_config(external_win).external) + command('tabnext') + eq(2, fn.tabpagenr()) + neq(external_win, api.nvim_get_current_win()) + + screen:detach() + end) + describe('with only one tabpage,', function() local float_opts = {relative = 'editor', row = 1, col = 1, width = 1, height = 1} local old_buf, old_win @@ -9101,6 +9138,22 @@ describe('float window', function() ]]} end end) + + it('attempt to turn into split with no room', function() + eq('Vim(split):E36: Not enough room', pcall_err(command, 'execute "split |"->repeat(&lines)')) + command('vsplit | wincmd | | wincmd p') + api.nvim_open_win(0, true, {relative = "editor", row = 0, col = 0, width = 5, height = 5}) + local config = api.nvim_win_get_config(0) + eq('editor', config.relative) + + local layout = fn.winlayout() + local restcmd = fn.winrestcmd() + eq('Vim(wincmd):E36: Not enough room', pcall_err(command, 'wincmd K')) + eq('Vim(wincmd):E36: Not enough room', pcall_err(command, 'wincmd J')) + eq(layout, fn.winlayout()) + eq(restcmd, fn.winrestcmd()) + eq(config, api.nvim_win_get_config(0)) + end) end describe('with ext_multigrid', function() diff --git a/test/old/testdir/test_window_cmd.vim b/test/old/testdir/test_window_cmd.vim index da1711a0a1..99a713d2ed 100644 --- a/test/old/testdir/test_window_cmd.vim +++ b/test/old/testdir/test_window_cmd.vim @@ -272,6 +272,16 @@ func Test_window_split_no_room() for s in range(1, hor_split_count) | split | endfor call assert_fails('split', 'E36:') + botright vsplit + wincmd | + let layout = winlayout() + let restcmd = winrestcmd() + call assert_fails('wincmd J', 'E36:') + call assert_fails('wincmd K', 'E36:') + call assert_equal(layout, winlayout()) + call assert_equal(restcmd, winrestcmd()) + only + " N vertical windows need >= 2*(N - 1) + 1 columns: " - 1 column + 1 separator for each window (except last window) " - 1 column for the last window which does not have separator @@ -284,7 +294,39 @@ func Test_window_split_no_room() for s in range(1, ver_split_count) | vsplit | endfor call assert_fails('vsplit', 'E36:') + split + wincmd | + let layout = winlayout() + let restcmd = winrestcmd() + call assert_fails('wincmd H', 'E36:') + call assert_fails('wincmd L', 'E36:') + call assert_equal(layout, winlayout()) + call assert_equal(restcmd, winrestcmd()) + + " Check that the last statusline isn't lost. + " Set its window's width to 2 for the test. + wincmd j + set laststatus=0 winminwidth=0 + vertical resize 2 + set winminwidth& + call setwinvar(winnr('k'), '&statusline', '@#') + let last_stl_row = win_screenpos(0)[0] - 1 + redraw + call assert_equal('@#|', GetScreenStr(last_stl_row)) + call assert_equal('~ |', GetScreenStr(&lines - &cmdheight)) + + let restcmd = winrestcmd() + call assert_fails('wincmd H', 'E36:') + call assert_fails('wincmd L', 'E36:') + call assert_equal(layout, winlayout()) + call assert_equal(restcmd, winrestcmd()) + call setwinvar(winnr('k'), '&statusline', '=-') + redraw + call assert_equal('=-|', GetScreenStr(last_stl_row)) + call assert_equal('~ |', GetScreenStr(&lines - &cmdheight)) + %bw! + set laststatus& endfunc func Test_window_exchange() @@ -1055,6 +1097,44 @@ func Test_win_splitmove() tabnew call assert_fails('call win_splitmove(1, win_getid(1, 1))', 'E957:') tabclose + + split + augroup WinSplitMove + au! + au WinEnter * ++once call win_gotoid(win_getid(winnr('#'))) + augroup END + call assert_fails('call win_splitmove(winnr(), winnr("#"))', 'E855:') + + augroup WinSplitMove + au! + au WinLeave * ++once quit + augroup END + call assert_fails('call win_splitmove(winnr(), winnr("#"))', 'E855:') + + split + split + augroup WinSplitMove + au! + au WinEnter * ++once let s:triggered = v:true + \| call assert_fails('call win_splitmove(winnr("$"), winnr())', 'E242:') + augroup END + quit + call assert_equal(v:true, s:triggered) + unlet! s:triggered + + new + augroup WinSplitMove + au! + au BufHidden * ++once let s:triggered = v:true + \| call assert_fails('call win_splitmove(winnr("#"), winnr())', 'E1159:') + augroup END + hide + call assert_equal(v:true, s:triggered) + unlet! s:triggered + + au! WinSplitMove + augroup! WinSplitMove + %bw! endfunc func Test_floatwin_splitmove() @@ -1062,7 +1142,8 @@ func Test_floatwin_splitmove() let win2 = win_getid() let popup_winid = nvim_open_win(0, 0, {'relative': 'win', \ 'row': 3, 'col': 3, 'width': 12, 'height': 3}) - call assert_fails('call win_splitmove(popup_winid, win2)', 'E957:') + " Nvim: floating windows are supported for the first argument. + " call assert_fails('call win_splitmove(popup_winid, win2)', 'E957:') call assert_fails('call win_splitmove(win2, popup_winid)', 'E957:') call nvim_win_close(popup_winid, 1) @@ -2007,23 +2088,75 @@ func Test_new_help_window_on_error() endfunc func Test_smoothscroll_in_zero_width_window() - let save_lines = &lines - let save_columns = &columns + set cpo+=n number smoothscroll + set winwidth=99999 winminwidth=0 - winsize 0 24 - set cpo+=n - exe "noremap 0 \n\L" - norm 000000 - set number smoothscroll - exe "norm \" + vsplit + call assert_equal(0, winwidth(winnr('#'))) + call win_execute(win_getid(winnr('#')), "norm! \") only! - let &lines = save_lines - let &columns = save_columns - set cpo-=n - unmap 0 - set nonumber nosmoothscroll + set winwidth& winminwidth& + set cpo-=n nonumber nosmoothscroll endfunc +func Test_splitmove_flatten_frame() + split + vsplit + + wincmd L + let layout = winlayout() + wincmd K + wincmd L + call assert_equal(winlayout(), layout) + + only! +endfunc + +func Test_splitmove_autocmd_window_no_room() + " Open as many windows as possible + while v:true + try + split + catch /E36:/ + break + endtry + endwhile + while v:true + try + vsplit + catch /E36:/ + break + endtry + endwhile + + wincmd j + vsplit + call assert_fails('wincmd H', 'E36:') + call assert_fails('wincmd J', 'E36:') + call assert_fails('wincmd K', 'E36:') + call assert_fails('wincmd L', 'E36:') + + edit unload me + enew + bunload! unload\ me + augroup SplitMoveAucmdWin + au! + au BufEnter * ++once let s:triggered = v:true + \| call assert_equal('autocmd', win_gettype()) + augroup END + let layout = winlayout() + let restcmd = winrestcmd() + " bufload opening the autocommand window shouldn't give E36. + call bufload('unload me') + call assert_equal(v:true, s:triggered) + call assert_equal(winlayout(), layout) + call assert_equal(winrestcmd(), restcmd) + + unlet! s:triggered + au! SplitMoveAucmdWin + augroup! SplitMoveAucmdWin + %bw! +endfunc " vim: shiftwidth=2 sts=2 expandtab From 24dfa47e4f4ca41d0c5f8c1c0f851602362c81d3 Mon Sep 17 00:00:00 2001 From: Sean Dewar <6256228+seandewar@users.noreply.github.com> Date: Sat, 24 Feb 2024 23:18:50 +0000 Subject: [PATCH 10/22] vim-patch:partial:9.1.0117: Stop split-moving from firing WinNew and WinNewPre autocommands Problem: win_splitmove fires WinNewPre and possibly WinNew when moving windows, even though no new windows are created. Solution: don't fire WinNew and WinNewPre when inserting an existing window, even if it isn't the current window. Improve the accuracy of related documentation. (Sean Dewar) https://github.com/vim/vim/commit/96cc4aef3d47d0fd70e68908af3d48a0dce8ea70 Partial as WinNewPre has not been ported yet (it currently has problems anyway). --- runtime/doc/builtin.txt | 8 ++++---- runtime/doc/windows.txt | 30 +++++++++++++--------------- runtime/lua/vim/_meta/vimfn.lua | 8 ++++---- src/nvim/eval.lua | 8 ++++---- src/nvim/window.c | 4 +++- test/old/testdir/test_window_cmd.vim | 18 +++++++++++++++++ 6 files changed, 47 insertions(+), 29 deletions(-) diff --git a/runtime/doc/builtin.txt b/runtime/doc/builtin.txt index c5f3946871..4b1ccc0c5c 100644 --- a/runtime/doc/builtin.txt +++ b/runtime/doc/builtin.txt @@ -8901,10 +8901,10 @@ win_screenpos({nr}) *win_screenpos()* tabpage. win_splitmove({nr}, {target} [, {options}]) *win_splitmove()* - Move the window {nr} to a new split of the window {target}. - This is similar to moving to {target}, creating a new window - using |:split| but having the same contents as window {nr}, and - then closing {nr}. + Temporarily switch to window {target}, then move window {nr} + to a new split adjacent to {target}. + Unlike commands such as |:split|, no new windows are created + (the |window-ID| of window {nr} is unchanged after the move). Both {nr} and {target} can be window numbers or |window-ID|s. Both must be in the current tab page. diff --git a/runtime/doc/windows.txt b/runtime/doc/windows.txt index b71e7c80ab..4791e73929 100644 --- a/runtime/doc/windows.txt +++ b/runtime/doc/windows.txt @@ -502,35 +502,33 @@ horizontally split windows. CTRL-W H does it the other way around. *CTRL-W_K* CTRL-W K Move the current window to be at the very top, using the full - width of the screen. This works like closing the current - window and then creating another one with ":topleft split", - except that the current window contents is used for the new - window. + width of the screen. This works like `:topleft split`, except + it is applied to the current window and no new window is + created. *CTRL-W_J* CTRL-W J Move the current window to be at the very bottom, using the - full width of the screen. This works like closing the current - window and then creating another one with ":botright split", - except that the current window contents is used for the new - window. + full width of the screen. This works like `:botright split`, + except it is applied to the current window and no new window + is created. *CTRL-W_H* CTRL-W H Move the current window to be at the far left, using the - full height of the screen. This works like closing the - current window and then creating another one with - `:vert topleft split`, except that the current window contents - is used for the new window. + full height of the screen. This works like + `:vert topleft split`, except it is applied to the current + window and no new window is created. *CTRL-W_L* CTRL-W L Move the current window to be at the far right, using the full - height of the screen. This works like closing the - current window and then creating another one with - `:vert botright split`, except that the current window - contents is used for the new window. + height of the screen. This works like `:vert botright split`, + except it is applied to the current window and no new window + is created. *CTRL-W_T* CTRL-W T Move the current window to a new tab page. This fails if there is only one window in the current tab page. + This works like `:tab split`, except the previous window is + closed. When a count is specified the new tab page will be opened before the tab page with this index. Otherwise it comes after the current tab page. diff --git a/runtime/lua/vim/_meta/vimfn.lua b/runtime/lua/vim/_meta/vimfn.lua index ac25547212..ee68f669f8 100644 --- a/runtime/lua/vim/_meta/vimfn.lua +++ b/runtime/lua/vim/_meta/vimfn.lua @@ -10598,10 +10598,10 @@ function vim.fn.win_move_statusline(nr, offset) end --- @return any function vim.fn.win_screenpos(nr) end ---- Move the window {nr} to a new split of the window {target}. ---- This is similar to moving to {target}, creating a new window ---- using |:split| but having the same contents as window {nr}, and ---- then closing {nr}. +--- Temporarily switch to window {target}, then move window {nr} +--- to a new split adjacent to {target}. +--- Unlike commands such as |:split|, no new windows are created +--- (the |window-ID| of window {nr} is unchanged after the move). --- --- Both {nr} and {target} can be window numbers or |window-ID|s. --- Both must be in the current tab page. diff --git a/src/nvim/eval.lua b/src/nvim/eval.lua index b7120d5dd5..febd022254 100644 --- a/src/nvim/eval.lua +++ b/src/nvim/eval.lua @@ -12699,10 +12699,10 @@ M.funcs = { args = { 2, 3 }, base = 1, desc = [=[ - Move the window {nr} to a new split of the window {target}. - This is similar to moving to {target}, creating a new window - using |:split| but having the same contents as window {nr}, and - then closing {nr}. + Temporarily switch to window {target}, then move window {nr} + to a new split adjacent to {target}. + Unlike commands such as |:split|, no new windows are created + (the |window-ID| of window {nr} is unchanged after the move). Both {nr} and {target} can be window numbers or |window-ID|s. Both must be in the current tab page. diff --git a/src/nvim/window.c b/src/nvim/window.c index cfa28bbc1f..b1135d59fc 100644 --- a/src/nvim/window.c +++ b/src/nvim/window.c @@ -987,6 +987,8 @@ int win_split(int size, int flags) /// When "new_wp" is NULL: split the current window in two. /// When "new_wp" is not NULL: insert this window at the far /// top/left/right/bottom. +/// On failure, if "new_wp" was not NULL, no changes will have been made to the +/// window layout or sizes. /// @return NULL for failure, or pointer to new window win_T *win_split_ins(int size, int flags, win_T *new_wp, int dir) { @@ -1494,7 +1496,7 @@ win_T *win_split_ins(int size, int flags, win_T *new_wp, int dir) if (!(flags & WSP_NOENTER)) { // make the new window the current window - win_enter_ext(wp, WEE_TRIGGER_NEW_AUTOCMDS | WEE_TRIGGER_ENTER_AUTOCMDS + win_enter_ext(wp, (new_wp == NULL ? WEE_TRIGGER_NEW_AUTOCMDS : 0) | WEE_TRIGGER_ENTER_AUTOCMDS | WEE_TRIGGER_LEAVE_AUTOCMDS); } if (vertical) { diff --git a/test/old/testdir/test_window_cmd.vim b/test/old/testdir/test_window_cmd.vim index 99a713d2ed..fc379c198d 100644 --- a/test/old/testdir/test_window_cmd.vim +++ b/test/old/testdir/test_window_cmd.vim @@ -1066,6 +1066,19 @@ func Test_win_splitmove() leftabove split b leftabove vsplit c leftabove split d + + " win_splitmove doesn't actually create or close any windows, so expect an + " unchanged winid and no WinNew/WinClosed events, like :wincmd H/J/K/L. + let s:triggered = [] + augroup WinSplitMove + au! + " Nvim: WinNewPre not ported yet. Also needs full port of v9.1.0117 to pass. + " au WinNewPre * let s:triggered += ['WinNewPre'] + au WinNew * let s:triggered += ['WinNew', win_getid()] + au WinClosed * let s:triggered += ['WinClosed', str2nr(expand(''))] + augroup END + let winid = win_getid() + call assert_equal(0, win_splitmove(winnr(), winnr('l'))) call assert_equal(bufname(winbufnr(1)), 'c') call assert_equal(bufname(winbufnr(2)), 'd') @@ -1088,6 +1101,11 @@ func Test_win_splitmove() call assert_equal(bufname(winbufnr(3)), 'a') call assert_equal(bufname(winbufnr(4)), 'd') call assert_fails('call win_splitmove(winnr(), winnr("k"), v:_null_dict)', 'E1297:') + call assert_equal([], s:triggered) + call assert_equal(winid, win_getid()) + + unlet! s:triggered + au! WinSplitMove only | bd call assert_fails('call win_splitmove(winnr(), 123)', 'E957:') From 1c6b693ec1592f9d193fc9cc1bb03e738fb2bef6 Mon Sep 17 00:00:00 2001 From: Sean Dewar <6256228+seandewar@users.noreply.github.com> Date: Sun, 25 Feb 2024 01:03:26 +0000 Subject: [PATCH 11/22] vim-patch:9.1.0118: Use different restoration strategy in win_splitmove Problem: saving and restoring all frames to split-move is overkill now that WinNewPre is not fired when split-moving. Solution: defer the flattening of frames until win_split_ins begins reorganising them, and attempt to restore the layout by undoing our changes. (Sean Dewar) https://github.com/vim/vim/commit/704966c2545897dfcf426dd9ef946aeb6fa80c38 Adjust winframe_restore to account for Nvim's horizontal separators when the global statusline is in use. Add a test. --- src/nvim/api/win_config.c | 14 +- src/nvim/window.c | 265 ++++++++++++------------- src/nvim/winfloat.c | 2 +- test/functional/ui/statusline_spec.lua | 20 ++ 4 files changed, 151 insertions(+), 150 deletions(-) diff --git a/src/nvim/api/win_config.c b/src/nvim/api/win_config.c index c308cadb7c..978d8515c8 100644 --- a/src/nvim/api/win_config.c +++ b/src/nvim/api/win_config.c @@ -260,7 +260,7 @@ Window nvim_open_win(Buffer buffer, Boolean enter, Dict(win_config) *config, Err int flags = win_split_flags(fconfig.split, parent == NULL) | WSP_NOENTER; if (parent == NULL) { - wp = win_split_ins(0, flags, NULL, 0); + wp = win_split_ins(0, flags, NULL, 0, NULL); } else { tp = win_find_tabpage(parent); switchwin_T switchwin; @@ -268,7 +268,7 @@ Window nvim_open_win(Buffer buffer, Boolean enter, Dict(win_config) *config, Err const int result = switch_win(&switchwin, parent, tp, true); assert(result == OK); (void)result; - wp = win_split_ins(0, flags, NULL, 0); + wp = win_split_ins(0, flags, NULL, 0, NULL); restore_win(&switchwin, true); } if (wp) { @@ -495,11 +495,11 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err) // If the frame doesn't have a parent, the old frame // was the root frame and we need to create a top-level split. int dir; - new_curwin = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp); + new_curwin = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, NULL); } else if (n_frames == 2) { // There are two windows in the frame, we can just rotate it. int dir; - neighbor = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp); + neighbor = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, NULL); new_curwin = neighbor; } else { // There is only one window in the frame, we can't split it. @@ -511,7 +511,7 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err) parent = neighbor; } else { int dir; - new_curwin = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp); + new_curwin = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, NULL); } win_remove(win, win_tp == curtab ? NULL : win_tp); // move to neighboring window if we're moving the current window to a new tabpage @@ -531,7 +531,7 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err) int flags = win_split_flags(fconfig.split, parent == NULL) | WSP_NOENTER; if (parent == NULL) { - if (!win_split_ins(0, flags, win, 0)) { + if (!win_split_ins(0, flags, win, 0, NULL)) { // TODO(willothy): What should this error message say? api_set_error(err, kErrorTypeException, "Failed to split window"); return; @@ -542,7 +542,7 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err) const int result = switch_win(&switchwin, parent, win_find_tabpage(parent), true); (void)result; assert(result == OK); - win_split_ins(0, flags, win, 0); + win_split_ins(0, flags, win, 0, NULL); restore_win(&switchwin, true); } if (HAS_KEY_X(config, width)) { diff --git a/src/nvim/window.c b/src/nvim/window.c index b1135d59fc..2d0010ad6c 100644 --- a/src/nvim/window.c +++ b/src/nvim/window.c @@ -981,16 +981,19 @@ int win_split(int size, int flags) clear_snapshot(curtab, SNAP_HELP_IDX); } - return win_split_ins(size, flags, NULL, 0) == NULL ? FAIL : OK; + return win_split_ins(size, flags, NULL, 0, NULL) == NULL ? FAIL : OK; } /// When "new_wp" is NULL: split the current window in two. /// When "new_wp" is not NULL: insert this window at the far /// top/left/right/bottom. +/// When "to_flatten" is not NULL: flatten this frame before reorganising frames; +/// remains unflattened on failure. +/// /// On failure, if "new_wp" was not NULL, no changes will have been made to the /// window layout or sizes. /// @return NULL for failure, or pointer to new window -win_T *win_split_ins(int size, int flags, win_T *new_wp, int dir) +win_T *win_split_ins(int size, int flags, win_T *new_wp, int dir, frame_T *to_flatten) { win_T *wp = new_wp; @@ -1261,6 +1264,11 @@ win_T *win_split_ins(int size, int flags, win_T *new_wp, int dir) CLEAR_FIELD(wp->w_border_adj); } + // Going to reorganize frames now, make sure they're flat. + if (to_flatten != NULL) { + frame_flatten(to_flatten); + } + bool before; frame_T *curfrp; @@ -1915,38 +1923,29 @@ int win_splitmove(win_T *wp, int size, int flags) return FAIL; } - frame_T *frp = NULL; + frame_T *unflat_altfr = NULL; if (wp->w_floating) { win_remove(wp, NULL); } else { - // Undoing changes to frames if splitting fails is complicated. - // Save a full snapshot to restore instead. - frp = make_full_snapshot(); - - // Remove the window and frame from the tree of frames. - winframe_remove(wp, &dir, NULL); + // Remove the window and frame from the tree of frames. Don't flatten any + // frames yet so we can restore things if win_split_ins fails. + winframe_remove(wp, &dir, NULL, &unflat_altfr); win_remove(wp, NULL); last_status(false); // may need to remove last status line win_comp_pos(); // recompute window positions } // Split a window on the desired side and put "wp" there. - if (win_split_ins(size, flags, wp, dir) == NULL) { + if (win_split_ins(size, flags, wp, dir, unflat_altfr) == NULL) { win_append(wp->w_prev, wp); if (!wp->w_floating) { - // Restore the previous layout from the snapshot. - xfree(wp->w_frame); - restore_full_snapshot(frp); - - // Vertical separators to the left may have been lost. Restore them. - frp = wp->w_frame; - if (frp->fr_parent->fr_layout == FR_ROW && frp->fr_prev != NULL) { - frame_add_vsep(frp->fr_prev); - } + // win_split_ins doesn't change sizes or layout if it fails to insert an + // existing window, so just undo winframe_remove. + winframe_restore(wp, dir, unflat_altfr); + win_comp_pos(); // recompute window positions } return FAIL; } - clear_snapshot_rec(frp); // If splitting horizontally, try to preserve height. // Note that win_split_ins autocommands may have immediately made "wp" floating! @@ -3065,7 +3064,7 @@ static win_T *win_free_mem(win_T *win, int *dirp, tabpage_T *tp) if (!win->w_floating) { // Remove the window and its frame from the tree of frames. frame_T *frp = win->w_frame; - wp = winframe_remove(win, dirp, tp); + wp = winframe_remove(win, dirp, tp, NULL); xfree(frp); } else { *dirp = 'h'; // Dummy value. @@ -3146,9 +3145,11 @@ void win_free_all(void) /// /// @param dirp set to 'v' or 'h' for direction if 'ea' /// @param tp tab page "win" is in, NULL for current +/// @param unflat_altfr if not NULL, set to pointer of frame that got +/// the space, and it is not flattened /// /// @return a pointer to the window that got the freed up space. -win_T *winframe_remove(win_T *win, int *dirp, tabpage_T *tp) +win_T *winframe_remove(win_T *win, int *dirp, tabpage_T *tp, frame_T **unflat_altfr) FUNC_ATTR_NONNULL_ARG(1, 2) { assert(tp == NULL || tp != curtab); @@ -3236,56 +3237,110 @@ win_T *winframe_remove(win_T *win, int *dirp, tabpage_T *tp) frame_comp_pos(frp2, &row, &col); } - if (frp2->fr_next == NULL && frp2->fr_prev == NULL) { - // There is no other frame in this list, move its info to the parent - // and remove it. - frp2->fr_parent->fr_layout = frp2->fr_layout; - frp2->fr_parent->fr_child = frp2->fr_child; - frame_T *frp; - FOR_ALL_FRAMES(frp, frp2->fr_child) { - frp->fr_parent = frp2->fr_parent; + if (unflat_altfr == NULL) { + frame_flatten(frp2); + } else { + *unflat_altfr = frp2; + } + + return wp; +} + +/// Flatten "frp" into its parent frame if it's the only child, also merging its +/// list with the grandparent if they share the same layout. +/// Frees "frp" if flattened; also "frp->fr_parent" if it has the same layout. +/// "frp" must be valid in the current tabpage. +static void frame_flatten(frame_T *frp) + FUNC_ATTR_NONNULL_ALL +{ + if (frp->fr_next != NULL || frp->fr_prev != NULL) { + return; + } + + // There is no other frame in this list, move its info to the parent + // and remove it. + frp->fr_parent->fr_layout = frp->fr_layout; + frp->fr_parent->fr_child = frp->fr_child; + frame_T *frp2; + FOR_ALL_FRAMES(frp2, frp->fr_child) { + frp2->fr_parent = frp->fr_parent; + } + frp->fr_parent->fr_win = frp->fr_win; + if (frp->fr_win != NULL) { + frp->fr_win->w_frame = frp->fr_parent; + } + frp2 = frp->fr_parent; + if (topframe->fr_child == frp) { + topframe->fr_child = frp2; + } + xfree(frp); + + frp = frp2->fr_parent; + if (frp != NULL && frp->fr_layout == frp2->fr_layout) { + // The frame above the parent has the same layout, have to merge + // the frames into this list. + if (frp->fr_child == frp2) { + frp->fr_child = frp2->fr_child; } - frp2->fr_parent->fr_win = frp2->fr_win; - if (frp2->fr_win != NULL) { - frp2->fr_win->w_frame = frp2->fr_parent; + assert(frp2->fr_child); + frp2->fr_child->fr_prev = frp2->fr_prev; + if (frp2->fr_prev != NULL) { + frp2->fr_prev->fr_next = frp2->fr_child; + } + for (frame_T *frp3 = frp2->fr_child;; frp3 = frp3->fr_next) { + frp3->fr_parent = frp; + if (frp3->fr_next == NULL) { + frp3->fr_next = frp2->fr_next; + if (frp2->fr_next != NULL) { + frp2->fr_next->fr_prev = frp3; + } + break; + } } - frp = frp2->fr_parent; if (topframe->fr_child == frp2) { topframe->fr_child = frp; } xfree(frp2); + } +} - frp2 = frp->fr_parent; - if (frp2 != NULL && frp2->fr_layout == frp->fr_layout) { - // The frame above the parent has the same layout, have to merge - // the frames into this list. - if (frp2->fr_child == frp) { - frp2->fr_child = frp->fr_child; - } - assert(frp->fr_child); - frp->fr_child->fr_prev = frp->fr_prev; - if (frp->fr_prev != NULL) { - frp->fr_prev->fr_next = frp->fr_child; - } - frame_T *frp3; - for (frp3 = frp->fr_child;; frp3 = frp3->fr_next) { - frp3->fr_parent = frp2; - if (frp3->fr_next == NULL) { - frp3->fr_next = frp->fr_next; - if (frp->fr_next != NULL) { - frp->fr_next->fr_prev = frp3; - } - break; - } - } - if (topframe->fr_child == frp) { - topframe->fr_child = frp2; - } - xfree(frp); +/// Undo changes from a prior call to winframe_remove, also restoring lost +/// vertical separators and statuslines. +/// Caller must ensure no other changes were made to the layout or window sizes! +static void winframe_restore(win_T *wp, int dir, frame_T *unflat_altfr) + FUNC_ATTR_NONNULL_ALL +{ + frame_T *frp = wp->w_frame; + + // Put "wp"'s frame back where it was. + if (frp->fr_prev != NULL) { + frame_append(frp->fr_prev, frp); + } else { + frame_insert(frp->fr_next, frp); + } + + // Vertical separators to the left may have been lost. Restore them. + if (wp->w_vsep_width == 0 && frp->fr_parent->fr_layout == FR_ROW && frp->fr_prev != NULL) { + frame_add_vsep(frp->fr_prev); + } + + // Statuslines or horizontal separators above may have been lost. Restore them. + if (frp->fr_parent->fr_layout == FR_COL && frp->fr_prev != NULL) { + if (global_stl_height() == 0 && wp->w_status_height == 0) { + frame_add_statusline(frp->fr_prev); + } else if (wp->w_hsep_height == 0) { + frame_add_hsep(frp->fr_prev); } } - return wp; + // Restore the lost room that was redistributed to the altframe. + if (dir == 'v') { + frame_new_height(unflat_altfr, unflat_altfr->fr_height - frp->fr_height, + unflat_altfr == frp->fr_next, false); + } else if (dir == 'h') { + frame_new_width(unflat_altfr, unflat_altfr->fr_width - frp->fr_width, + unflat_altfr == frp->fr_next, false); + } } /// If 'splitbelow' or 'splitright' is set, the space goes above or to the left @@ -7248,23 +7303,23 @@ void reset_lnums(void) void make_snapshot(int idx) { clear_snapshot(curtab, idx); - make_snapshot_rec(topframe, &curtab->tp_snapshot[idx], false); + make_snapshot_rec(topframe, &curtab->tp_snapshot[idx]); } -static void make_snapshot_rec(frame_T *fr, frame_T **frp, bool snap_wins) +static void make_snapshot_rec(frame_T *fr, frame_T **frp) { *frp = xcalloc(1, sizeof(frame_T)); (*frp)->fr_layout = fr->fr_layout; (*frp)->fr_width = fr->fr_width; (*frp)->fr_height = fr->fr_height; if (fr->fr_next != NULL) { - make_snapshot_rec(fr->fr_next, &((*frp)->fr_next), snap_wins); + make_snapshot_rec(fr->fr_next, &((*frp)->fr_next)); } if (fr->fr_child != NULL) { - make_snapshot_rec(fr->fr_child, &((*frp)->fr_child), snap_wins); + make_snapshot_rec(fr->fr_child, &((*frp)->fr_child)); } - if (fr->fr_layout == FR_LEAF && (snap_wins || fr->fr_win == curwin)) { - (*frp)->fr_win = fr->fr_win; + if (fr->fr_layout == FR_LEAF && fr->fr_win == curwin) { + (*frp)->fr_win = curwin; } } @@ -7381,80 +7436,6 @@ static win_T *restore_snapshot_rec(frame_T *sn, frame_T *fr) return wp; } -/// Return a snapshot of all frames in the current tabpage and which windows are -/// in them. -/// Use clear_snapshot_rec to free the snapshot. -static frame_T *make_full_snapshot(void) -{ - frame_T *frp; - make_snapshot_rec(topframe, &frp, true); - return frp; -} - -/// Restore all frames in the full snapshot "sn" for the current tabpage. -/// Caller must ensure that the screen size didn't change, no windows with frames -/// in the snapshot were freed, and windows with frames not in the snapshot are -/// removed from their frames! -/// Doesn't restore changed window vertical separators. -/// Frees the old frames. Don't call clear_snapshot_rec on "sn" afterwards! -static void restore_full_snapshot(frame_T *sn) -{ - if (sn == NULL) { - return; - } - - clear_snapshot_rec(topframe); - restore_full_snapshot_rec(sn); - curtab->tp_topframe = topframe = sn; - last_status(false); - - // If the amount of space available changed, first try setting the sizes of - // windows with 'winfix{width,height}'. If that doesn't result in the right - // size, forget about that option. - if (topframe->fr_width != Columns) { - frame_new_width(topframe, Columns, false, true); - if (!frame_check_width(topframe, Columns)) { - frame_new_width(topframe, Columns, false, false); - } - } - if (topframe->fr_height != ROWS_AVAIL) { - frame_new_height(topframe, (int)ROWS_AVAIL, false, true); - if (!frame_check_height(topframe, (int)ROWS_AVAIL)) { - frame_new_height(topframe, (int)ROWS_AVAIL, false, false); - } - } - - win_comp_pos(); -} - -static void restore_full_snapshot_rec(frame_T *sn) -{ - if (sn == NULL) { - return; - } - - if (sn->fr_child != NULL) { - sn->fr_child->fr_parent = sn; - } - if (sn->fr_next != NULL) { - sn->fr_next->fr_parent = sn->fr_parent; - sn->fr_next->fr_prev = sn; - } - if (sn->fr_win != NULL) { - sn->fr_win->w_frame = sn; - // Assume for now that all windows have statuslines, so last_status in restore_full_snapshot - // doesn't resize frames to fit any missing statuslines. - sn->fr_win->w_status_height = STATUS_HEIGHT; - sn->fr_win->w_hsep_height = 0; - - // Resize window to fit the frame. - frame_new_height(sn, sn->fr_height, false, false); - frame_new_width(sn, sn->fr_width, false, false); - } - restore_full_snapshot_rec(sn->fr_child); - restore_full_snapshot_rec(sn->fr_next); -} - /// Check that "topfrp" and its children are at the right height. /// /// @param topfrp top frame pointer diff --git a/src/nvim/winfloat.c b/src/nvim/winfloat.c index 8fe0315230..cddc4ed9dd 100644 --- a/src/nvim/winfloat.c +++ b/src/nvim/winfloat.c @@ -57,7 +57,7 @@ win_T *win_new_float(win_T *wp, bool last, WinConfig fconfig, Error *err) return NULL; } int dir; - winframe_remove(wp, &dir, NULL); + winframe_remove(wp, &dir, NULL, NULL); XFREE_CLEAR(wp->w_frame); win_comp_pos(); // recompute window positions win_remove(wp, NULL); diff --git a/test/functional/ui/statusline_spec.lua b/test/functional/ui/statusline_spec.lua index fee4b64d44..000e2b1b04 100644 --- a/test/functional/ui/statusline_spec.lua +++ b/test/functional/ui/statusline_spec.lua @@ -11,6 +11,7 @@ local exec = helpers.exec local exec_lua = helpers.exec_lua local eval = helpers.eval local sleep = vim.uv.sleep +local pcall_err = helpers.pcall_err local mousemodels = { 'extend', 'popup', 'popup_setpos' } @@ -474,6 +475,25 @@ describe('global statusline', function() | ]]) end) + + it('horizontal separators unchanged when failing to split-move window', function() + exec([[ + botright split + let &winwidth = &columns + let &winminwidth = &columns + ]]) + eq('Vim(wincmd):E36: Not enough room', pcall_err(command, 'wincmd L')) + command('mode') + screen:expect([[ + | + {1:~ }|*5 + ────────────────────────────────────────────────────────────| + ^ | + {1:~ }|*6 + {2:[No Name] 0,0-1 All}| + | + ]]) + end) end) it('statusline does not crash if it has Arabic characters #19447', function() From 01b27410a347b90820d4255061944c31d20b8f33 Mon Sep 17 00:00:00 2001 From: Sean Dewar <6256228+seandewar@users.noreply.github.com> Date: Sun, 25 Feb 2024 01:11:40 +0000 Subject: [PATCH 12/22] vim-patch:9.1.0119: can move away from cmdwin using win_splitmove() Problem: can switch windows while textlocked via f_win_gotoid and f_win_splitmove (which also allows switching in the cmdwin). Solution: Check text_or_buf_locked in f_win_splitmove() (Sean Dewar) While at it, call text_or_buf_locked() in f_win_gotoid() instead of testing for cmdwin_type() (which text_buf_locked() does and in addition will also verify that the buffer is not locked). https://github.com/vim/vim/commit/f865895c874b0936b0563ebfef7490aac8cb8a1f --- src/nvim/eval/window.c | 6 +++--- test/old/testdir/test_window_cmd.vim | 28 ++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/src/nvim/eval/window.c b/src/nvim/eval/window.c index 17b8b01963..d20fc3f2f2 100644 --- a/src/nvim/eval/window.c +++ b/src/nvim/eval/window.c @@ -14,6 +14,7 @@ #include "nvim/eval/typval.h" #include "nvim/eval/typval_defs.h" #include "nvim/eval/window.h" +#include "nvim/ex_getln.h" #include "nvim/garray.h" #include "nvim/garray_defs.h" #include "nvim/gettext_defs.h" @@ -584,8 +585,7 @@ void f_win_gotoid(typval_T *argvars, typval_T *rettv, EvalFuncData fptr) { int id = (int)tv_get_number(&argvars[0]); - if (cmdwin_type != 0) { - emsg(_(e_cmdwin)); + if (text_or_buf_locked()) { return; } FOR_ALL_TAB_WINDOWS(tp, wp) { @@ -697,7 +697,7 @@ void f_win_splitmove(typval_T *argvars, typval_T *rettv, EvalFuncData fptr) } // Check if we can split the target before we bother switching windows. - if (is_aucmd_win(wp) || check_split_disallowed(targetwin) == FAIL) { + if (is_aucmd_win(wp) || text_or_buf_locked() || check_split_disallowed(targetwin) == FAIL) { return; } diff --git a/test/old/testdir/test_window_cmd.vim b/test/old/testdir/test_window_cmd.vim index fc379c198d..1f2eb2b624 100644 --- a/test/old/testdir/test_window_cmd.vim +++ b/test/old/testdir/test_window_cmd.vim @@ -2177,4 +2177,32 @@ func Test_splitmove_autocmd_window_no_room() %bw! endfunc +func Test_win_gotoid_splitmove_textlock_cmdwin() + call setline(1, 'foo') + new + let curwin = win_getid() + call setline(1, 'bar') + + set debug+=throw indentexpr=win_gotoid(win_getid(winnr('#'))) + call assert_fails('normal! ==', 'E565:') + call assert_equal(curwin, win_getid()) + + set indentexpr=win_splitmove(winnr('#'),winnr()) + call assert_fails('normal! ==', 'E565:') + call assert_equal(curwin, win_getid()) + + %bw! + set debug-=throw indentexpr& + + call feedkeys('q:' + \ .. ":call assert_fails('call win_splitmove(winnr(''#''), winnr())', 'E11:')\" + \ .. ":call assert_equal('command', win_gettype())\" + \ .. ":call assert_equal('', win_gettype(winnr('#')))\", 'ntx') + + call feedkeys('q:' + \ .. ":call assert_fails('call win_gotoid(win_getid(winnr(''#'')))', 'E11:')\" + \ .. ":call assert_equal('command', win_gettype())\" + \ .. ":call assert_equal('', win_gettype(winnr('#')))\", 'ntx') +endfunc + " vim: shiftwidth=2 sts=2 expandtab From b2245307f2acfd7b62cf5d0c5b199c87c2d37b23 Mon Sep 17 00:00:00 2001 From: Sean Dewar <6256228+seandewar@users.noreply.github.com> Date: Sun, 25 Feb 2024 01:15:30 +0000 Subject: [PATCH 13/22] vim-patch:9.1.0121: Infinite loop or signed overflow with 'smoothscroll' Problem: infinite loop in win_update with 'smoothscroll' set when window width is equal to textoff, or signed integer overflow if smaller. Solution: don't revalidate wp->w_skipcol in that case, as no buffer text is being shown. (Sean Dewar) https://github.com/vim/vim/commit/02fcae02a926e4e8379d77fb716da4202029882d Test_window_split_no_room changes were already cherry-picked earlier. --- src/nvim/drawscreen.c | 2 +- test/old/testdir/test_scroll_opt.vim | 38 ++++++++++++++++++++++++++++ test/old/testdir/test_window_cmd.vim | 21 +++------------ 3 files changed, 43 insertions(+), 18 deletions(-) diff --git a/src/nvim/drawscreen.c b/src/nvim/drawscreen.c index 1626e46cf6..402f7fa428 100644 --- a/src/nvim/drawscreen.c +++ b/src/nvim/drawscreen.c @@ -1513,7 +1513,7 @@ static void win_update(win_T *wp) // Make sure skipcol is valid, it depends on various options and the window // width. - if (wp->w_skipcol > 0) { + if (wp->w_skipcol > 0 && wp->w_width_inner > win_col_off(wp)) { int w = 0; int width1 = wp->w_width_inner - win_col_off(wp); int width2 = width1 + win_col_off2(wp); diff --git a/test/old/testdir/test_scroll_opt.vim b/test/old/testdir/test_scroll_opt.vim index a1987ed3c9..8130f7a1ac 100644 --- a/test/old/testdir/test_scroll_opt.vim +++ b/test/old/testdir/test_scroll_opt.vim @@ -963,4 +963,42 @@ func Test_smoothscroll_insert_bottom() call StopVimInTerminal(buf) endfunc +func Test_smoothscroll_in_zero_width_window() + set cpo+=n number smoothscroll + set winwidth=99999 winminwidth=0 + + vsplit + call assert_equal(0, winwidth(winnr('#'))) + call win_execute(win_getid(winnr('#')), "norm! \") + + only! + set winwidth& winminwidth& + set cpo-=n nonumber nosmoothscroll +endfunc + +func Test_smoothscroll_textoff_small_winwidth() + set smoothscroll number + call setline(1, 'llanfairpwllgwyngyllgogerychwyrndrobwllllantysiliogogogoch') + vsplit + + let textoff = getwininfo(win_getid())[0].textoff + execute 'vertical resize' textoff + 1 + redraw + call assert_equal(0, winsaveview().skipcol) + execute "normal! 0\" + redraw + call assert_equal(1, winsaveview().skipcol) + execute 'vertical resize' textoff - 1 + " This caused a signed integer overflow. + redraw + call assert_equal(1, winsaveview().skipcol) + execute 'vertical resize' textoff + " This caused an infinite loop. + redraw + call assert_equal(1, winsaveview().skipcol) + + %bw! + set smoothscroll& number& +endfunc + " vim: shiftwidth=2 sts=2 expandtab diff --git a/test/old/testdir/test_window_cmd.vim b/test/old/testdir/test_window_cmd.vim index 1f2eb2b624..02fa3ac407 100644 --- a/test/old/testdir/test_window_cmd.vim +++ b/test/old/testdir/test_window_cmd.vim @@ -2105,19 +2105,6 @@ func Test_new_help_window_on_error() call assert_equal(expand(""), "'mod'") endfunc -func Test_smoothscroll_in_zero_width_window() - set cpo+=n number smoothscroll - set winwidth=99999 winminwidth=0 - - vsplit - call assert_equal(0, winwidth(winnr('#'))) - call win_execute(win_getid(winnr('#')), "norm! \") - - only! - set winwidth& winminwidth& - set cpo-=n nonumber nosmoothscroll -endfunc - func Test_splitmove_flatten_frame() split vsplit @@ -2131,7 +2118,7 @@ func Test_splitmove_flatten_frame() only! endfunc -func Test_splitmove_autocmd_window_no_room() +func Test_autocmd_window_force_room() " Open as many windows as possible while v:true try @@ -2158,7 +2145,7 @@ func Test_splitmove_autocmd_window_no_room() edit unload me enew bunload! unload\ me - augroup SplitMoveAucmdWin + augroup AucmdWinForceRoom au! au BufEnter * ++once let s:triggered = v:true \| call assert_equal('autocmd', win_gettype()) @@ -2172,8 +2159,8 @@ func Test_splitmove_autocmd_window_no_room() call assert_equal(winrestcmd(), restcmd) unlet! s:triggered - au! SplitMoveAucmdWin - augroup! SplitMoveAucmdWin + au! AucmdWinForceRoom + augroup! AucmdWinForceRoom %bw! endfunc From e3d4dfb6c3fcd22205f6843b96f9a043871113ce Mon Sep 17 00:00:00 2001 From: Sean Dewar <6256228+seandewar@users.noreply.github.com> Date: Sun, 25 Feb 2024 01:22:55 +0000 Subject: [PATCH 14/22] vim-patch:9.1.0128: win_gotoid() may abort even when not switching a window Problem: win_gotoid() checks for textlock and other things when switching to a window that is already current (after v9.1.0119) Solution: return early with success when attempting to switch to curwin (Sean Dewar) https://github.com/vim/vim/commit/2a65e739447949a7aee966ce8a3b75521b2a79ea --- src/nvim/eval/window.c | 5 +++++ test/old/testdir/test_window_cmd.vim | 6 ++++++ 2 files changed, 11 insertions(+) diff --git a/src/nvim/eval/window.c b/src/nvim/eval/window.c index d20fc3f2f2..c2b9574579 100644 --- a/src/nvim/eval/window.c +++ b/src/nvim/eval/window.c @@ -584,6 +584,11 @@ void f_win_getid(typval_T *argvars, typval_T *rettv, EvalFuncData fptr) void f_win_gotoid(typval_T *argvars, typval_T *rettv, EvalFuncData fptr) { int id = (int)tv_get_number(&argvars[0]); + if (curwin->handle == id) { + // Nothing to do. + rettv->vval.v_number = 1; + return; + } if (text_or_buf_locked()) { return; diff --git a/test/old/testdir/test_window_cmd.vim b/test/old/testdir/test_window_cmd.vim index 02fa3ac407..7ea556b11f 100644 --- a/test/old/testdir/test_window_cmd.vim +++ b/test/old/testdir/test_window_cmd.vim @@ -2173,6 +2173,10 @@ func Test_win_gotoid_splitmove_textlock_cmdwin() set debug+=throw indentexpr=win_gotoid(win_getid(winnr('#'))) call assert_fails('normal! ==', 'E565:') call assert_equal(curwin, win_getid()) + " No error if attempting to switch to curwin; nothing happens. + set indentexpr=assert_equal(1,win_gotoid(win_getid())) + normal! == + call assert_equal(curwin, win_getid()) set indentexpr=win_splitmove(winnr('#'),winnr()) call assert_fails('normal! ==', 'E565:') @@ -2188,6 +2192,8 @@ func Test_win_gotoid_splitmove_textlock_cmdwin() call feedkeys('q:' \ .. ":call assert_fails('call win_gotoid(win_getid(winnr(''#'')))', 'E11:')\" + "\ No error if attempting to switch to curwin; nothing happens. + \ .. ":call assert_equal(1, win_gotoid(win_getid()))\" \ .. ":call assert_equal('command', win_gettype())\" \ .. ":call assert_equal('', win_gettype(winnr('#')))\", 'ntx') endfunc From 832bc5c169d8b339ef139ef0bdcefb2e72864e6e Mon Sep 17 00:00:00 2001 From: Sean Dewar <6256228+seandewar@users.noreply.github.com> Date: Sun, 25 Feb 2024 01:30:37 +0000 Subject: [PATCH 15/22] vim-patch:9.1.0130: [security]: UAF if win_split_ins autocommands delete "wp" Problem: heap-use-after-free in win_splitmove if Enter/Leave autocommands from win_split_ins immediately closes "wp". Solution: check that "wp" is valid after win_split_ins. (Sean Dewar) https://github.com/vim/vim/commit/abf7030a5c22257f066fa9c4061ad150d5a82577 --- src/nvim/window.c | 4 ++-- test/old/testdir/test_window_cmd.vim | 9 +++++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/nvim/window.c b/src/nvim/window.c index 2d0010ad6c..80cb9ab6a0 100644 --- a/src/nvim/window.c +++ b/src/nvim/window.c @@ -1948,8 +1948,8 @@ int win_splitmove(win_T *wp, int size, int flags) } // If splitting horizontally, try to preserve height. - // Note that win_split_ins autocommands may have immediately made "wp" floating! - if (size == 0 && !(flags & WSP_VERT) && !wp->w_floating) { + // Note that win_split_ins autocommands may have immediately closed "wp", or made it floating! + if (size == 0 && !(flags & WSP_VERT) && win_valid(wp) && !wp->w_floating) { win_setheight_win(height, wp); if (p_ea) { // Equalize windows. Note that win_split_ins autocommands may have diff --git a/test/old/testdir/test_window_cmd.vim b/test/old/testdir/test_window_cmd.vim index 7ea556b11f..e6d591f8c9 100644 --- a/test/old/testdir/test_window_cmd.vim +++ b/test/old/testdir/test_window_cmd.vim @@ -1150,6 +1150,15 @@ func Test_win_splitmove() call assert_equal(v:true, s:triggered) unlet! s:triggered + split + let close_win = winnr('#') + augroup WinSplitMove + au! + au WinEnter * ++once quit! + augroup END + call win_splitmove(close_win, winnr()) + call assert_equal(0, win_id2win(close_win)) + au! WinSplitMove augroup! WinSplitMove %bw! From d942c2b9432d81e4b509519bd48fa886e37e9ca8 Mon Sep 17 00:00:00 2001 From: Sean Dewar <6256228+seandewar@users.noreply.github.com> Date: Mon, 26 Feb 2024 14:51:31 +0000 Subject: [PATCH 16/22] fix(api): handle win_split_ins failure properly Problem: nvim_win_set_config does not handle failure in win_split_ins properly yet, which can cause all sorts of issues. Also nvim_open_win and nvim_win_set_config do not set the error message to the one from win_split_ins. Solution: handle failure by undoing winframe_remove, like in win_splitmove. Make sure autocommands from switching to the altwin fire within a valid window, and ensure they don't screw things up. Set the error message to that of win_split_ins, if any. Also change a few other small things, including: - adjust win_append to take a tabpage_T * argument, which is more consistent with win_remove (and also allows us to undo a call to win_remove). - allow winframe_restore to restore window positions. Useful if `wp` was in a different tabpage, as a call to win_comp_pos (which only works for the current tabpage) after winframe_restore should no longer be needed. Though enter_tabpage calls win_comp_pos anyway, this has the advantage of ensuring w_winrow/col remains accurate even before entering the tabpage (useful for stuff like win_screenpos, if used on a window in another tabpage). (This change should probably also be PR'd to Vim later, even though it doesn't use winframe_restore for a `wp` in a different tabpage yet). --- src/nvim/api/win_config.c | 149 ++++++++----- src/nvim/autocmd.c | 2 +- src/nvim/window.c | 128 +++++++---- src/nvim/winfloat.c | 2 +- test/functional/api/window_spec.lua | 319 ++++++++++++++++++++++++++-- 5 files changed, 492 insertions(+), 108 deletions(-) diff --git a/src/nvim/api/win_config.c b/src/nvim/api/win_config.c index 978d8515c8..bb28000719 100644 --- a/src/nvim/api/win_config.c +++ b/src/nvim/api/win_config.c @@ -259,18 +259,20 @@ Window nvim_open_win(Buffer buffer, Boolean enter, Dict(win_config) *config, Err } int flags = win_split_flags(fconfig.split, parent == NULL) | WSP_NOENTER; - if (parent == NULL) { - wp = win_split_ins(0, flags, NULL, 0, NULL); - } else { - tp = win_find_tabpage(parent); - switchwin_T switchwin; - // `parent` is valid in `tp`, so switch_win should not fail. - const int result = switch_win(&switchwin, parent, tp, true); - assert(result == OK); - (void)result; - wp = win_split_ins(0, flags, NULL, 0, NULL); - restore_win(&switchwin, true); - } + TRY_WRAP(err, { + if (parent == NULL || parent == curwin) { + wp = win_split_ins(0, flags, NULL, 0, NULL); + } else { + tp = win_find_tabpage(parent); + switchwin_T switchwin; + // `parent` is valid in `tp`, so switch_win should not fail. + const int result = switch_win(&switchwin, parent, tp, true); + assert(result == OK); + (void)result; + wp = win_split_ins(0, flags, NULL, 0, NULL); + restore_win(&switchwin, true); + } + }); if (wp) { wp->w_config = fconfig; } @@ -278,7 +280,9 @@ Window nvim_open_win(Buffer buffer, Boolean enter, Dict(win_config) *config, Err wp = win_new_float(NULL, false, fconfig, err); } if (!wp) { - api_set_error(err, kErrorTypeException, "Failed to create window"); + if (!ERROR_SET(err)) { + api_set_error(err, kErrorTypeException, "Failed to create window"); + } return 0; } @@ -448,17 +452,47 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err) return; // error already set } - if (was_split) { - win_T *new_curwin = NULL; + bool to_split_ok = false; + // If we are moving curwin to another tabpage, switch windows *before* we remove it from the + // window list or remove its frame (if non-floating), so it's valid for autocommands. + const bool curwin_moving_tp + = win == curwin && parent != NULL && win_tp != win_find_tabpage(parent); + if (curwin_moving_tp) { + if (was_split) { + int dir; + win_goto(winframe_find_altwin(win, &dir, NULL, NULL)); + } else { + win_goto(win_valid(prevwin) && prevwin != win ? prevwin : firstwin); + } + // Autocommands may have been a real nuisance and messed things up... + if (curwin == win) { + api_set_error(err, kErrorTypeException, "Failed to switch away from window %d", + win->handle); + return; + } + win_tp = win_find_tabpage(win); + if (!win_tp || !win_valid_any_tab(parent)) { + api_set_error(err, kErrorTypeException, "Windows to split were closed"); + goto restore_curwin; + } + if (was_split == win->w_floating || parent->w_floating) { + api_set_error(err, kErrorTypeException, "Floating state of windows to split changed"); + goto restore_curwin; + } + } + + int dir = 0; + frame_T *unflat_altfr = NULL; + if (was_split) { // If the window is the last in the tabpage or `fconfig.win` is // a handle to itself, we can't split it. if (win->w_frame->fr_parent == NULL) { // FIXME(willothy): if the window is the last in the tabpage but there is another tabpage // and the target window is in that other tabpage, should we move the window to that // tabpage and close the previous one, or just error? - api_set_error(err, kErrorTypeValidation, "Cannot move last window"); - return; + api_set_error(err, kErrorTypeException, "Cannot move last window"); + goto restore_curwin; } else if (parent != NULL && parent->handle == win->handle) { int n_frames = 0; for (frame_T *fr = win->w_frame->fr_parent->fr_child; fr != NULL; fr = fr->fr_next) { @@ -494,64 +528,67 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err) } // If the frame doesn't have a parent, the old frame // was the root frame and we need to create a top-level split. - int dir; - new_curwin = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, NULL); + winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, &unflat_altfr); } else if (n_frames == 2) { // There are two windows in the frame, we can just rotate it. - int dir; - neighbor = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, NULL); - new_curwin = neighbor; + neighbor = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, &unflat_altfr); } else { // There is only one window in the frame, we can't split it. - api_set_error(err, kErrorTypeValidation, "Cannot split window into itself"); - return; + api_set_error(err, kErrorTypeException, "Cannot split window into itself"); + goto restore_curwin; } - // Set the parent to whatever the correct - // neighbor window was determined to be. + // Set the parent to whatever the correct neighbor window was determined to be. parent = neighbor; } else { - int dir; - new_curwin = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, NULL); + winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, &unflat_altfr); } - win_remove(win, win_tp == curtab ? NULL : win_tp); - // move to neighboring window if we're moving the current window to a new tabpage - if (curwin == win && parent != NULL && new_curwin != NULL - && win_tp != win_find_tabpage(parent)) { - win_enter(new_curwin, true); - if (!win_valid_any_tab(parent)) { - // win_enter autocommands closed the `parent` to split from. - api_set_error(err, kErrorTypeException, "Window to split was closed"); - return; - } - } - } else { - win_remove(win, win_tp == curtab ? NULL : win_tp); } + win_remove(win, win_tp == curtab ? NULL : win_tp); int flags = win_split_flags(fconfig.split, parent == NULL) | WSP_NOENTER; - - if (parent == NULL) { - if (!win_split_ins(0, flags, win, 0, NULL)) { - // TODO(willothy): What should this error message say? - api_set_error(err, kErrorTypeException, "Failed to split window"); - return; - } - } else { + TRY_WRAP(err, { + const bool need_switch = parent != NULL && parent != curwin; switchwin_T switchwin; - // `parent` is valid in its tabpage, so switch_win should not fail. - const int result = switch_win(&switchwin, parent, win_find_tabpage(parent), true); - (void)result; - assert(result == OK); - win_split_ins(0, flags, win, 0, NULL); - restore_win(&switchwin, true); + if (need_switch) { + // `parent` is valid in its tabpage, so switch_win should not fail. + const int result = switch_win(&switchwin, parent, win_find_tabpage(parent), true); + (void)result; + assert(result == OK); + } + to_split_ok = win_split_ins(0, flags, win, 0, unflat_altfr) != NULL; + if (!to_split_ok) { + // Restore `win` to the window list now, so it's valid for restore_win (if used). + win_append(win->w_prev, win, win_tp == curtab ? NULL : win_tp); + } + if (need_switch) { + restore_win(&switchwin, true); + } + }); + if (!to_split_ok) { + if (was_split) { + // win_split_ins doesn't change sizes or layout if it fails to insert an existing window, so + // just undo winframe_remove. + winframe_restore(win, dir, unflat_altfr); + } + if (!ERROR_SET(err)) { + api_set_error(err, kErrorTypeException, "Failed to move window %d into split", win->handle); + } + +restore_curwin: + // If `win` was the original curwin, and autocommands didn't move it outside of curtab, be a + // good citizen and try to return to it. + if (curwin_moving_tp && win_valid(win)) { + win_goto(win); + } + return; } + if (HAS_KEY_X(config, width)) { win_setwidth_win(fconfig.width, win); } if (HAS_KEY_X(config, height)) { win_setheight_win(fconfig.height, win); } - return; } else { win_config_float(win, fconfig); win->w_pos_changed = true; diff --git a/src/nvim/autocmd.c b/src/nvim/autocmd.c index 3f93906942..652b6ba74e 100644 --- a/src/nvim/autocmd.c +++ b/src/nvim/autocmd.c @@ -1333,7 +1333,7 @@ void aucmd_prepbuf(aco_save_T *aco, buf_T *buf) block_autocmds(); // We don't want BufEnter/WinEnter autocommands. if (need_append) { - win_append(lastwin, auc_win); + win_append(lastwin, auc_win, NULL); pmap_put(int)(&window_handles, auc_win->handle, auc_win); win_config_float(auc_win, auc_win->w_config); } diff --git a/src/nvim/window.c b/src/nvim/window.c index 80cb9ab6a0..b55dd79260 100644 --- a/src/nvim/window.c +++ b/src/nvim/window.c @@ -1218,13 +1218,13 @@ win_T *win_split_ins(int size, int flags, win_T *new_wp, int dir, frame_T *to_fl if (new_wp == NULL) { wp = win_alloc(oldwin, false); } else { - win_append(oldwin, wp); + win_append(oldwin, wp, NULL); } } else { if (new_wp == NULL) { wp = win_alloc(oldwin->w_prev, false); } else { - win_append(oldwin->w_prev, wp); + win_append(oldwin->w_prev, wp, NULL); } } @@ -1783,13 +1783,13 @@ static void win_exchange(int Prenum) if (wp->w_prev != curwin) { win_remove(curwin, NULL); frame_remove(curwin->w_frame); - win_append(wp->w_prev, curwin); + win_append(wp->w_prev, curwin, NULL); frame_insert(frp, curwin->w_frame); } if (wp != wp2) { win_remove(wp, NULL); frame_remove(wp->w_frame); - win_append(wp2, wp); + win_append(wp2, wp, NULL); if (frp2 == NULL) { frame_insert(wp->w_frame->fr_parent->fr_child, wp->w_frame); } else { @@ -1863,7 +1863,7 @@ static void win_rotate(bool upwards, int count) // find last frame and append removed window/frame after it for (; frp->fr_next != NULL; frp = frp->fr_next) {} - win_append(frp->fr_win, wp1); + win_append(frp->fr_win, wp1, NULL); frame_append(frp, wp1->w_frame); wp2 = frp->fr_win; // previously last window @@ -1878,7 +1878,7 @@ static void win_rotate(bool upwards, int count) assert(frp->fr_parent->fr_child); // append the removed window/frame before the first in the list - win_append(frp->fr_parent->fr_child->fr_win->w_prev, wp1); + win_append(frp->fr_parent->fr_child->fr_win->w_prev, wp1, NULL); frame_insert(frp->fr_parent->fr_child, frp); } @@ -1937,7 +1937,7 @@ int win_splitmove(win_T *wp, int size, int flags) // Split a window on the desired side and put "wp" there. if (win_split_ins(size, flags, wp, dir, unflat_altfr) == NULL) { - win_append(wp->w_prev, wp); + win_append(wp->w_prev, wp, NULL); if (!wp->w_floating) { // win_split_ins doesn't change sizes or layout if it fails to insert an // existing window, so just undo winframe_remove. @@ -2016,7 +2016,7 @@ void win_move_after(win_T *win1, win_T *win2) } win_remove(win1, NULL); frame_remove(win1->w_frame); - win_append(win2, win1); + win_append(win2, win1, NULL); frame_append(win2->w_frame, win1->w_frame); win_comp_pos(); // recompute w_winrow for all windows @@ -3151,6 +3151,55 @@ void win_free_all(void) /// @return a pointer to the window that got the freed up space. win_T *winframe_remove(win_T *win, int *dirp, tabpage_T *tp, frame_T **unflat_altfr) FUNC_ATTR_NONNULL_ARG(1, 2) +{ + frame_T *altfr; + win_T *wp = winframe_find_altwin(win, dirp, tp, &altfr); + if (wp == NULL) { + return NULL; + } + + frame_T *frp_close = win->w_frame; + // Remove this frame from the list of frames. + frame_remove(frp_close); + + if (*dirp == 'v') { + frame_new_height(altfr, altfr->fr_height + frp_close->fr_height, + altfr == frp_close->fr_next, false); + } else { + assert(*dirp == 'h'); + frame_new_width(altfr, altfr->fr_width + frp_close->fr_width, + altfr == frp_close->fr_next, false); + } + + // If rows/columns go to a window below/right its positions need to be + // updated. Can only be done after the sizes have been updated. + if (altfr == frp_close->fr_next) { + int row = win->w_winrow; + int col = win->w_wincol; + + frame_comp_pos(altfr, &row, &col); + } + + if (unflat_altfr == NULL) { + frame_flatten(altfr); + } else { + *unflat_altfr = altfr; + } + + return wp; +} + +/// Find the window that will get the freed space from a call to `winframe_remove`. +/// Makes no changes to the window layout. +/// +/// @param dirp set to 'v' or 'h' for the direction where "altfr" will be resized +/// to fill the space +/// @param tp tab page "win" is in, NULL for current +/// @param altfr if not NULL, set to pointer of frame that will get the space +/// +/// @return a pointer to the window that will get the freed up space. +win_T *winframe_find_altwin(win_T *win, int *dirp, tabpage_T *tp, frame_T **altfr) + FUNC_ATTR_NONNULL_ARG(1, 2) { assert(tp == NULL || tp != curtab); @@ -3161,13 +3210,10 @@ win_T *winframe_remove(win_T *win, int *dirp, tabpage_T *tp, frame_T **unflat_al frame_T *frp_close = win->w_frame; - // Remove the window from its frame. + // Find the window and frame that gets the space. frame_T *frp2 = win_altframe(win, tp); win_T *wp = frame2win(frp2); - // Remove this frame from the list of frames. - frame_remove(frp_close); - if (frp_close->fr_parent->fr_layout == FR_COL) { // When 'winfixheight' is set, try to find another frame in the column // (as close to the closed frame as possible) to distribute the height @@ -3194,8 +3240,6 @@ win_T *winframe_remove(win_T *win, int *dirp, tabpage_T *tp, frame_T **unflat_al } } } - frame_new_height(frp2, frp2->fr_height + frp_close->fr_height, - frp2 == frp_close->fr_next, false); *dirp = 'v'; } else { // When 'winfixwidth' is set, try to find another frame in the column @@ -3223,24 +3267,12 @@ win_T *winframe_remove(win_T *win, int *dirp, tabpage_T *tp, frame_T **unflat_al } } } - frame_new_width(frp2, frp2->fr_width + frp_close->fr_width, - frp2 == frp_close->fr_next, false); *dirp = 'h'; } - // If rows/columns go to a window below/right its positions need to be - // updated. Can only be done after the sizes have been updated. - if (frp2 == frp_close->fr_next) { - int row = win->w_winrow; - int col = win->w_wincol; - - frame_comp_pos(frp2, &row, &col); - } - - if (unflat_altfr == NULL) { - frame_flatten(frp2); - } else { - *unflat_altfr = frp2; + assert(wp != win && frp2 != frp_close); + if (altfr != NULL) { + *altfr = frp2; } return wp; @@ -3305,9 +3337,10 @@ static void frame_flatten(frame_T *frp) } /// Undo changes from a prior call to winframe_remove, also restoring lost -/// vertical separators and statuslines. +/// vertical separators and statuslines, and changed window positions for +/// windows within "unflat_altfr". /// Caller must ensure no other changes were made to the layout or window sizes! -static void winframe_restore(win_T *wp, int dir, frame_T *unflat_altfr) +void winframe_restore(win_T *wp, int dir, frame_T *unflat_altfr) FUNC_ATTR_NONNULL_ALL { frame_T *frp = wp->w_frame; @@ -3333,13 +3366,24 @@ static void winframe_restore(win_T *wp, int dir, frame_T *unflat_altfr) } } + int row = wp->w_winrow; + int col = wp->w_wincol; + // Restore the lost room that was redistributed to the altframe. if (dir == 'v') { frame_new_height(unflat_altfr, unflat_altfr->fr_height - frp->fr_height, unflat_altfr == frp->fr_next, false); + row += frp->fr_height; } else if (dir == 'h') { frame_new_width(unflat_altfr, unflat_altfr->fr_width - frp->fr_width, unflat_altfr == frp->fr_next, false); + col += frp->fr_width; + } + + // If rows/columns went to a window below/right, its positions need to be + // restored. Can only be done after the sizes have been updated. + if (unflat_altfr == frp->fr_next) { + frame_comp_pos(unflat_altfr, &row, &col); } } @@ -4445,7 +4489,7 @@ static void tabpage_check_windows(tabpage_T *old_curtab) if (wp->w_floating) { if (wp->w_config.external) { win_remove(wp, old_curtab); - win_append(lastwin_nofloating(), wp); + win_append(lastwin_nofloating(), wp, NULL); } else { ui_comp_remove_grid(&wp->w_grid_alloc); } @@ -5085,7 +5129,7 @@ win_T *win_alloc(win_T *after, bool hidden) block_autocmds(); // link the window in the window list if (!hidden) { - win_append(after, new_wp); + win_append(after, new_wp, NULL); } new_wp->w_wincol = 0; @@ -5255,21 +5299,29 @@ void win_free_grid(win_T *wp, bool reinit) } } -// Append window "wp" in the window list after window "after". -void win_append(win_T *after, win_T *wp) +/// Append window "wp" in the window list after window "after". +/// +/// @param tp tab page "win" (and "after", if not NULL) is in, NULL for current +void win_append(win_T *after, win_T *wp, tabpage_T *tp) + FUNC_ATTR_NONNULL_ARG(2) { + assert(tp == NULL || tp != curtab); + + win_T **first = tp == NULL ? &firstwin : &tp->tp_firstwin; + win_T **last = tp == NULL ? &lastwin : &tp->tp_lastwin; + // after NULL is in front of the first - win_T *before = after == NULL ? firstwin : after->w_next; + win_T *before = after == NULL ? *first : after->w_next; wp->w_next = before; wp->w_prev = after; if (after == NULL) { - firstwin = wp; + *first = wp; } else { after->w_next = wp; } if (before == NULL) { - lastwin = wp; + *last = wp; } else { before->w_prev = wp; } diff --git a/src/nvim/winfloat.c b/src/nvim/winfloat.c index cddc4ed9dd..10d8c7ac90 100644 --- a/src/nvim/winfloat.c +++ b/src/nvim/winfloat.c @@ -61,7 +61,7 @@ win_T *win_new_float(win_T *wp, bool last, WinConfig fconfig, Error *err) XFREE_CLEAR(wp->w_frame); win_comp_pos(); // recompute window positions win_remove(wp, NULL); - win_append(lastwin_nofloating(), wp); + win_append(lastwin_nofloating(), wp, NULL); } wp->w_floating = true; wp->w_status_height = 0; diff --git a/test/functional/api/window_spec.lua b/test/functional/api/window_spec.lua index 6e20c81fc2..abe1ca9344 100644 --- a/test/functional/api/window_spec.lua +++ b/test/functional/api/window_spec.lua @@ -1654,6 +1654,18 @@ describe('API/win', function() api.nvim_open_win(new_buf, false, { split = 'left' }) eq('foobarbaz', eval('triggered')) end) + + it('sets error when no room', function() + matches('E36: Not enough room$', pcall_err(command, 'execute "split|"->repeat(&lines)')) + matches( + 'E36: Not enough room$', + pcall_err(api.nvim_open_win, 0, true, { split = 'above', win = 0 }) + ) + matches( + 'E36: Not enough room$', + pcall_err(api.nvim_open_win, 0, true, { split = 'below', win = 0 }) + ) + end) end) describe('set_config', function() @@ -1965,23 +1977,89 @@ describe('API/win', function() it('closing new curwin when moving window to other tabpage works', function() command('split | tabnew') - local w = api.nvim_get_current_win() - local t = api.nvim_get_current_tabpage() - command('tabfirst | autocmd WinEnter * quit') - api.nvim_win_set_config(0, { win = w, split = 'left' }) - -- New tabpage is now the only one, as WinEnter closed the new curwin in the original. - eq(t, api.nvim_get_current_tabpage()) - eq({ t }, api.nvim_list_tabpages()) + local t2_win = api.nvim_get_current_win() + command('tabfirst | autocmd WinEnter * ++once quit') + local t1_move_win = api.nvim_get_current_win() + -- win_set_config fails to switch away from "t1_move_win" because the WinEnter autocmd that + -- closed the window we're switched to returns us to "t1_move_win", as it filled the space. + eq( + 'Failed to switch away from window ' .. t1_move_win, + pcall_err(api.nvim_win_set_config, t1_move_win, { win = t2_win, split = 'left' }) + ) + eq(t1_move_win, api.nvim_get_current_win()) + + command('split | split | autocmd WinEnter * ++once quit') + t1_move_win = api.nvim_get_current_win() + -- In this case, we closed the window that we got switched to, but doing so didn't switch us + -- back to "t1_move_win", which is fine. + api.nvim_win_set_config(t1_move_win, { win = t2_win, split = 'left' }) + neq(t1_move_win, api.nvim_get_current_win()) end) - it('closing split parent when moving window to other tabpage aborts', function() + it('messing with "win" or "parent" when moving "win" to other tabpage', function() command('split | tabnew') - local w = api.nvim_get_current_win() - command('tabfirst | autocmd WinEnter * call nvim_win_close(' .. w .. ', 1)') + local t2 = api.nvim_get_current_tabpage() + local t2_win1 = api.nvim_get_current_win() + command('split') + local t2_win2 = api.nvim_get_current_win() + command('split') + local t2_win3 = api.nvim_get_current_win() + + command('tabfirst | autocmd WinEnter * ++once call nvim_win_close(' .. t2_win1 .. ', 1)') + local cur_win = api.nvim_get_current_win() eq( - 'Window to split was closed', - pcall_err(api.nvim_win_set_config, 0, { win = w, split = 'left' }) + 'Windows to split were closed', + pcall_err(api.nvim_win_set_config, 0, { win = t2_win1, split = 'left' }) ) + eq(cur_win, api.nvim_get_current_win()) + + command('split | autocmd WinLeave * ++once quit!') + cur_win = api.nvim_get_current_win() + eq( + 'Windows to split were closed', + pcall_err(api.nvim_win_set_config, 0, { win = t2_win2, split = 'left' }) + ) + neq(cur_win, api.nvim_get_current_win()) + + exec([[ + split + autocmd WinLeave * ++once + \ call nvim_win_set_config(0, #{relative:'editor', row:0, col:0, width:5, height:5}) + ]]) + cur_win = api.nvim_get_current_win() + eq( + 'Floating state of windows to split changed', + pcall_err(api.nvim_win_set_config, 0, { win = t2_win3, split = 'left' }) + ) + eq('editor', api.nvim_win_get_config(0).relative) + eq(cur_win, api.nvim_get_current_win()) + + command('autocmd WinLeave * ++once wincmd J') + cur_win = api.nvim_get_current_win() + eq( + 'Floating state of windows to split changed', + pcall_err(api.nvim_win_set_config, 0, { win = t2_win3, split = 'left' }) + ) + eq('', api.nvim_win_get_config(0).relative) + eq(cur_win, api.nvim_get_current_win()) + + -- Try to make "parent" floating. This should give the same error as before, but because + -- changing a split from another tabpage into a float isn't supported yet, check for that + -- error instead for now. + -- Use ":silent!" to avoid the one second delay from printing the error message. + exec(([[ + autocmd WinLeave * ++once silent! + \ call nvim_win_set_config(%d, #{relative:'editor', row:0, col:0, width:5, height:5}) + ]]):format(t2_win3)) + cur_win = api.nvim_get_current_win() + api.nvim_win_set_config(0, { win = t2_win3, split = 'left' }) + matches( + 'Cannot change window from different tabpage into float$', + api.nvim_get_vvar('errmsg') + ) + -- The error doesn't abort moving the window (or maybe it should, if that's wanted?) + neq(cur_win, api.nvim_get_current_win()) + eq(t2, api.nvim_win_get_tabpage(cur_win)) end) it('expected autocmds when moving window to other tabpage', function() @@ -2031,6 +2109,223 @@ describe('API/win', function() command('autocmd BufHidden * ++once call nvim_win_set_config(' .. w .. ', #{split: "left"})') command('new | quit') end) + + --- Returns a function to get information about the window layout, sizes and positions of a + --- tabpage. + local function define_tp_info_function() + exec_lua([[ + function tp_info(tp) + return { + layout = vim.fn.winlayout(vim.api.nvim_tabpage_get_number(tp)), + pos_sizes = vim.tbl_map( + function(w) + local pos = vim.fn.win_screenpos(w) + return { + row = pos[1], + col = pos[2], + width = vim.fn.winwidth(w), + height = vim.fn.winheight(w) + } + end, + vim.api.nvim_tabpage_list_wins(tp) + ) + } + end + ]]) + + return function(tp) + return exec_lua('return tp_info(...)', tp) + end + end + + it('attempt to move window with no room', function() + -- Fill the 2nd tabpage full of windows until we run out of room. + -- Use &laststatus=0 to ensure restoring missing statuslines doesn't affect things. + command('set laststatus=0 | tabnew') + matches('E36: Not enough room$', pcall_err(command, 'execute "split|"->repeat(&lines)')) + command('vsplit | wincmd | | wincmd p') + local t2 = api.nvim_get_current_tabpage() + local t2_cur_win = api.nvim_get_current_win() + local t2_top_split = fn.win_getid(1) + local t2_bot_split = fn.win_getid(fn.winnr('$')) + local t2_float = api.nvim_open_win( + 0, + false, + { relative = 'editor', row = 0, col = 0, width = 10, height = 10 } + ) + local t2_float_config = api.nvim_win_get_config(t2_float) + local tp_info = define_tp_info_function() + local t2_info = tp_info(t2) + matches( + 'E36: Not enough room$', + pcall_err(api.nvim_win_set_config, 0, { win = t2_top_split, split = 'above' }) + ) + matches( + 'E36: Not enough room$', + pcall_err(api.nvim_win_set_config, 0, { win = t2_top_split, split = 'below' }) + ) + matches( + 'E36: Not enough room$', + pcall_err(api.nvim_win_set_config, 0, { win = t2_bot_split, split = 'above' }) + ) + matches( + 'E36: Not enough room$', + pcall_err(api.nvim_win_set_config, 0, { win = t2_bot_split, split = 'below' }) + ) + matches( + 'E36: Not enough room$', + pcall_err(api.nvim_win_set_config, t2_float, { win = t2_top_split, split = 'above' }) + ) + matches( + 'E36: Not enough room$', + pcall_err(api.nvim_win_set_config, t2_float, { win = t2_top_split, split = 'below' }) + ) + matches( + 'E36: Not enough room$', + pcall_err(api.nvim_win_set_config, t2_float, { win = t2_bot_split, split = 'above' }) + ) + matches( + 'E36: Not enough room$', + pcall_err(api.nvim_win_set_config, t2_float, { win = t2_bot_split, split = 'below' }) + ) + eq(t2_cur_win, api.nvim_get_current_win()) + eq(t2_info, tp_info(t2)) + eq(t2_float_config, api.nvim_win_get_config(t2_float)) + + -- Try to move windows from the 1st tabpage to the 2nd. + command('tabfirst | split | wincmd _') + local t1 = api.nvim_get_current_tabpage() + local t1_cur_win = api.nvim_get_current_win() + local t1_float = api.nvim_open_win( + 0, + false, + { relative = 'editor', row = 5, col = 3, width = 7, height = 6 } + ) + local t1_float_config = api.nvim_win_get_config(t1_float) + local t1_info = tp_info(t1) + matches( + 'E36: Not enough room$', + pcall_err(api.nvim_win_set_config, 0, { win = t2_top_split, split = 'above' }) + ) + matches( + 'E36: Not enough room$', + pcall_err(api.nvim_win_set_config, 0, { win = t2_top_split, split = 'below' }) + ) + matches( + 'E36: Not enough room$', + pcall_err(api.nvim_win_set_config, 0, { win = t2_bot_split, split = 'above' }) + ) + matches( + 'E36: Not enough room$', + pcall_err(api.nvim_win_set_config, 0, { win = t2_bot_split, split = 'below' }) + ) + matches( + 'E36: Not enough room$', + pcall_err(api.nvim_win_set_config, t1_float, { win = t2_top_split, split = 'above' }) + ) + matches( + 'E36: Not enough room$', + pcall_err(api.nvim_win_set_config, t1_float, { win = t2_top_split, split = 'below' }) + ) + matches( + 'E36: Not enough room$', + pcall_err(api.nvim_win_set_config, t1_float, { win = t2_bot_split, split = 'above' }) + ) + matches( + 'E36: Not enough room$', + pcall_err(api.nvim_win_set_config, t1_float, { win = t2_bot_split, split = 'below' }) + ) + eq(t1_cur_win, api.nvim_get_current_win()) + eq(t1_info, tp_info(t1)) + eq(t1_float_config, api.nvim_win_get_config(t1_float)) + end) + + it('attempt to move window from other tabpage with no room', function() + -- Fill up the 1st tabpage with horizontal splits, then create a 2nd with only a few. Go back + -- to the 1st and try to move windows from the 2nd (while it's non-current) to it. Check that + -- window positions and sizes in the 2nd are unchanged. + command('set laststatus=0') + matches('E36: Not enough room$', pcall_err(command, 'execute "split|"->repeat(&lines)')) + + command('tab split') + local t2 = api.nvim_get_current_tabpage() + local t2_top = api.nvim_get_current_win() + command('belowright split') + local t2_mid_left = api.nvim_get_current_win() + command('belowright vsplit') + local t2_mid_right = api.nvim_get_current_win() + command('split | wincmd J') + local t2_bot = api.nvim_get_current_win() + local tp_info = define_tp_info_function() + local t2_info = tp_info(t2) + eq({ + 'col', + { + { 'leaf', t2_top }, + { + 'row', + { + { 'leaf', t2_mid_left }, + { 'leaf', t2_mid_right }, + }, + }, + { 'leaf', t2_bot }, + }, + }, t2_info.layout) + + local function try_move_t2_wins_to_t1() + for _, w in ipairs({ t2_bot, t2_mid_left, t2_mid_right, t2_top }) do + matches( + 'E36: Not enough room$', + pcall_err(api.nvim_win_set_config, w, { win = 0, split = 'below' }) + ) + eq(t2_info, tp_info(t2)) + end + end + command('tabfirst') + try_move_t2_wins_to_t1() + -- Go to the 2nd tabpage to ensure nothing changes after win_comp_pos, last_status, .etc. + -- from enter_tabpage. + command('tabnext') + eq(t2_info, tp_info(t2)) + + -- Check things are fine with the global statusline too, for good measure. + -- Set it while the 2nd tabpage is current, so last_status runs for it. + command('set laststatus=3') + t2_info = tp_info(t2) + command('tabfirst') + try_move_t2_wins_to_t1() + end) + + it('does not switch window when textlocked or in the cmdwin', function() + command('tabnew') + local t2_win = api.nvim_get_current_win() + command('tabfirst') + feed('q:') + local cur_win = api.nvim_get_current_win() + eq( + 'Failed to switch away from window ' .. cur_win, + pcall_err(api.nvim_win_set_config, 0, { split = 'left', win = t2_win }) + ) + eq( + 'E11: Invalid in command-line window; executes, CTRL-C quits', + api.nvim_get_vvar('errmsg') + ) + eq(cur_win, api.nvim_get_current_win()) + command('quit!') + + exec(([[ + new + call setline(1, 'foo') + setlocal debug=throw indentexpr=nvim_win_set_config(0,#{split:'left',win:%d}) + ]]):format(t2_win)) + cur_win = api.nvim_get_current_win() + matches( + 'E565: Not allowed to change text or change window$', + pcall_err(command, 'normal! ==') + ) + eq(cur_win, api.nvim_get_current_win()) + end) end) describe('get_config', function() From e7c262f5553c1c6e1de95bcbdc8cfe7cc9d5e55e Mon Sep 17 00:00:00 2001 From: Sean Dewar <6256228+seandewar@users.noreply.github.com> Date: Tue, 27 Feb 2024 13:25:44 +0000 Subject: [PATCH 17/22] fix(api): patch some cmdwin/textlock holes Problem: there are new ways to escape textlock or break the cmdwin in nvim_win_set_config and nvim_tabpage_set_win. Solution: fix them. Use win_goto to check it in nvim_tabpage_set_win and use the try_start/end pattern like with similar functions such as nvim_set_current_win (which uses the existing msg_list, if set). Careful not to use `wp->handle` when printing the window ID in the error message for nvim_tabpage_set_win, as win_goto autocommands may have freed the window. On a related note, I have a feeling some API functions ought to be checking curbuf_locked... --- src/nvim/api/tabpage.c | 6 ++- src/nvim/api/win_config.c | 6 +++ src/nvim/winfloat.c | 13 ++++++ test/functional/api/tabpage_spec.lua | 26 +++++++++++ test/functional/api/window_spec.lua | 65 +++++++++++++++++++++++++--- 5 files changed, 109 insertions(+), 7 deletions(-) diff --git a/src/nvim/api/tabpage.c b/src/nvim/api/tabpage.c index 040abb1e3f..56a3f1cf23 100644 --- a/src/nvim/api/tabpage.c +++ b/src/nvim/api/tabpage.c @@ -146,7 +146,11 @@ void nvim_tabpage_set_win(Tabpage tabpage, Window win, Error *err) } if (tp == curtab) { - win_enter(wp, true); + try_start(); + win_goto(wp); + if (!try_end(err) && curwin != wp) { + api_set_error(err, kErrorTypeException, "Failed to switch to window %d", win); + } } else if (tp->tp_curwin != wp) { tp->tp_prevwin = tp->tp_curwin; tp->tp_curwin = wp; diff --git a/src/nvim/api/win_config.c b/src/nvim/api/win_config.c index bb28000719..dab1e4e80b 100644 --- a/src/nvim/api/win_config.c +++ b/src/nvim/api/win_config.c @@ -451,6 +451,12 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err) if (!check_split_disallowed_err(win, err)) { return; // error already set } + // Can't move the cmdwin or its old curwin to a different tabpage. + if ((win == cmdwin_win || win == cmdwin_old_curwin) && parent != NULL + && win_find_tabpage(parent) != win_tp) { + api_set_error(err, kErrorTypeException, "%s", e_cmdwin); + return; + } bool to_split_ok = false; // If we are moving curwin to another tabpage, switch windows *before* we remove it from the diff --git a/src/nvim/winfloat.c b/src/nvim/winfloat.c index 10d8c7ac90..3ddff8aa5a 100644 --- a/src/nvim/winfloat.c +++ b/src/nvim/winfloat.c @@ -55,6 +55,19 @@ win_T *win_new_float(win_T *wp, bool last, WinConfig fconfig, Error *err) api_set_error(err, kErrorTypeException, "Cannot change window from different tabpage into float"); return NULL; + } else if (cmdwin_win != NULL && !cmdwin_win->w_floating) { + // cmdwin can't become the only non-float. Check for others. + bool other_nonfloat = false; + for (win_T *wp2 = firstwin; wp2 != NULL && !wp2->w_floating; wp2 = wp2->w_next) { + if (wp2 != wp && wp2 != cmdwin_win) { + other_nonfloat = true; + break; + } + } + if (!other_nonfloat) { + api_set_error(err, kErrorTypeException, "%s", e_cmdwin); + return NULL; + } } int dir; winframe_remove(wp, &dir, NULL, NULL); diff --git a/test/functional/api/tabpage_spec.lua b/test/functional/api/tabpage_spec.lua index 36955c4ace..f7e6eed047 100644 --- a/test/functional/api/tabpage_spec.lua +++ b/test/functional/api/tabpage_spec.lua @@ -1,5 +1,7 @@ local helpers = require('test.functional.helpers')(after_each) local clear, eq, ok = helpers.clear, helpers.eq, helpers.ok +local exec = helpers.exec +local feed = helpers.feed local api = helpers.api local fn = helpers.fn local request = helpers.request @@ -86,6 +88,30 @@ describe('api/tabpage', function() pcall_err(api.nvim_tabpage_set_win, tab1, win3) ) end) + + it('does not switch window when textlocked or in the cmdwin', function() + local target_win = api.nvim_get_current_win() + feed('q:') + local cur_win = api.nvim_get_current_win() + eq( + 'Vim:E11: Invalid in command-line window; executes, CTRL-C quits', + pcall_err(api.nvim_tabpage_set_win, 0, target_win) + ) + eq(cur_win, api.nvim_get_current_win()) + command('quit!') + + exec(([[ + new + call setline(1, 'foo') + setlocal debug=throw indentexpr=nvim_tabpage_set_win(0,%d) + ]]):format(target_win)) + cur_win = api.nvim_get_current_win() + eq( + 'Vim(normal):E5555: API call: Vim:E565: Not allowed to change text or change window', + pcall_err(command, 'normal! ==') + ) + eq(cur_win, api.nvim_get_current_win()) + end) end) describe('{get,set,del}_var', function() diff --git a/test/functional/api/window_spec.lua b/test/functional/api/window_spec.lua index abe1ca9344..ae8a519f11 100644 --- a/test/functional/api/window_spec.lua +++ b/test/functional/api/window_spec.lua @@ -2297,29 +2297,82 @@ describe('API/win', function() try_move_t2_wins_to_t1() end) - it('does not switch window when textlocked or in the cmdwin', function() + it('handles cmdwin and textlock restrictions', function() command('tabnew') + local t2 = api.nvim_get_current_tabpage() local t2_win = api.nvim_get_current_win() command('tabfirst') + local t1_move_win = api.nvim_get_current_win() + command('split') + + -- Can't move the cmdwin, or its old curwin to a different tabpage. + local old_curwin = api.nvim_get_current_win() feed('q:') - local cur_win = api.nvim_get_current_win() eq( - 'Failed to switch away from window ' .. cur_win, + 'E11: Invalid in command-line window; executes, CTRL-C quits', pcall_err(api.nvim_win_set_config, 0, { split = 'left', win = t2_win }) ) eq( 'E11: Invalid in command-line window; executes, CTRL-C quits', - api.nvim_get_vvar('errmsg') + pcall_err(api.nvim_win_set_config, old_curwin, { split = 'left', win = t2_win }) ) - eq(cur_win, api.nvim_get_current_win()) + -- But we can move other windows. + api.nvim_win_set_config(t1_move_win, { split = 'left', win = t2_win }) + eq(t2, api.nvim_win_get_tabpage(t1_move_win)) command('quit!') + -- Can't configure windows such that the cmdwin would become the only non-float. + command('only!') + feed('q:') + eq( + 'E11: Invalid in command-line window; executes, CTRL-C quits', + pcall_err( + api.nvim_win_set_config, + old_curwin, + { relative = 'editor', row = 0, col = 0, width = 5, height = 5 } + ) + ) + -- old_curwin is now no longer the only other non-float, so we can make it floating now. + local t1_new_win = api.nvim_open_win( + api.nvim_create_buf(true, true), + false, + { split = 'left', win = old_curwin } + ) + api.nvim_win_set_config( + old_curwin, + { relative = 'editor', row = 0, col = 0, width = 5, height = 5 } + ) + eq('editor', api.nvim_win_get_config(old_curwin).relative) + -- ...which means we shouldn't be able to also make the new window floating too! + eq( + 'E11: Invalid in command-line window; executes, CTRL-C quits', + pcall_err( + api.nvim_win_set_config, + t1_new_win, + { relative = 'editor', row = 0, col = 0, width = 5, height = 5 } + ) + ) + -- Nothing ought to stop us from making the cmdwin itself floating, though... + api.nvim_win_set_config(0, { relative = 'editor', row = 0, col = 0, width = 5, height = 5 }) + eq('editor', api.nvim_win_get_config(0).relative) + -- We can't make our new window from before floating too, as it's now the only non-float. + eq( + 'Cannot change last window into float', + pcall_err( + api.nvim_win_set_config, + t1_new_win, + { relative = 'editor', row = 0, col = 0, width = 5, height = 5 } + ) + ) + command('quit!') + + -- Can't switch away from window before moving it to a different tabpage during textlock. exec(([[ new call setline(1, 'foo') setlocal debug=throw indentexpr=nvim_win_set_config(0,#{split:'left',win:%d}) ]]):format(t2_win)) - cur_win = api.nvim_get_current_win() + local cur_win = api.nvim_get_current_win() matches( 'E565: Not allowed to change text or change window$', pcall_err(command, 'normal! ==') From 54022a2946aca5de991e7fa1ebc2954340ec20a8 Mon Sep 17 00:00:00 2001 From: Sean Dewar <6256228+seandewar@users.noreply.github.com> Date: Sat, 9 Mar 2024 01:00:33 +0000 Subject: [PATCH 18/22] fix(api): win_set_config update statuslines after removing splits 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. --- src/nvim/api/win_config.c | 4 ++++ test/functional/api/window_spec.lua | 22 ++++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/src/nvim/api/win_config.c b/src/nvim/api/win_config.c index dab1e4e80b..32b2156313 100644 --- a/src/nvim/api/win_config.c +++ b/src/nvim/api/win_config.c @@ -550,6 +550,10 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err) } } win_remove(win, win_tp == curtab ? NULL : win_tp); + if (win_tp == curtab) { + last_status(false); // may need to remove last status line + win_comp_pos(); // recompute window positions + } int flags = win_split_flags(fconfig.split, parent == NULL) | WSP_NOENTER; TRY_WRAP(err, { diff --git a/test/functional/api/window_spec.lua b/test/functional/api/window_spec.lua index ae8a519f11..6b7c550ca8 100644 --- a/test/functional/api/window_spec.lua +++ b/test/functional/api/window_spec.lua @@ -2379,6 +2379,28 @@ describe('API/win', function() ) eq(cur_win, api.nvim_get_current_win()) end) + + it('updates statusline when moving bottom split', function() + local screen = Screen.new(10, 10) + screen:set_default_attr_ids({ + [0] = { bold = true, foreground = Screen.colors.Blue }, -- NonText + [1] = { bold = true, reverse = true }, -- StatusLine + }) + screen:attach() + exec([[ + set laststatus=0 + belowright split + call nvim_win_set_config(0, #{split: 'above', win: win_getid(winnr('#'))}) + ]]) + screen:expect([[ + ^ | + {0:~ }|*3 + {1:[No Name] }| + | + {0:~ }|*3 + | + ]]) + end) end) describe('get_config', function() From 6416c6bc94fa7ae553a6020d0ed2f07dd34ee3f1 Mon Sep 17 00:00:00 2001 From: zeertzjq Date: Sat, 9 Mar 2024 12:27:01 +0800 Subject: [PATCH 19/22] test(old): remove Test_floatwin_splitmove() 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. --- test/old/testdir/test_window_cmd.vim | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/test/old/testdir/test_window_cmd.vim b/test/old/testdir/test_window_cmd.vim index e6d591f8c9..8898d3a02d 100644 --- a/test/old/testdir/test_window_cmd.vim +++ b/test/old/testdir/test_window_cmd.vim @@ -1164,19 +1164,6 @@ func Test_win_splitmove() %bw! endfunc -func Test_floatwin_splitmove() - vsplit - let win2 = win_getid() - let popup_winid = nvim_open_win(0, 0, {'relative': 'win', - \ 'row': 3, 'col': 3, 'width': 12, 'height': 3}) - " Nvim: floating windows are supported for the first argument. - " call assert_fails('call win_splitmove(popup_winid, win2)', 'E957:') - call assert_fails('call win_splitmove(win2, popup_winid)', 'E957:') - - call nvim_win_close(popup_winid, 1) - bwipe -endfunc - " Test for the :only command func Test_window_only() new From 33dfb5a383d7afacda35b8fd392ad18d57db2870 Mon Sep 17 00:00:00 2001 From: zeertzjq Date: Sat, 9 Mar 2024 12:47:40 +0800 Subject: [PATCH 20/22] fix(window): :close may cause Nvim to quit with autocmd and float 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. --- src/nvim/buffer.c | 4 +-- src/nvim/window.c | 46 ++++++++----------------------- test/functional/ui/float_spec.lua | 33 ++++++++++++++++++++++ 3 files changed, 46 insertions(+), 37 deletions(-) diff --git a/src/nvim/buffer.c b/src/nvim/buffer.c index b013f43ceb..7154be36be 100644 --- a/src/nvim/buffer.c +++ b/src/nvim/buffer.c @@ -568,7 +568,7 @@ bool close_buffer(win_T *win, buf_T *buf, int action, bool abort_if_last, bool i } buf->b_locked--; buf->b_locked_split--; - if (abort_if_last && last_nonfloat(win)) { + if (abort_if_last && one_window(win)) { // Autocommands made this the only window. emsg(_(e_auabort)); return false; @@ -587,7 +587,7 @@ bool close_buffer(win_T *win, buf_T *buf, int action, bool abort_if_last, bool i } buf->b_locked--; buf->b_locked_split--; - if (abort_if_last && last_nonfloat(win)) { + if (abort_if_last && one_window(win)) { // Autocommands made this the only window. emsg(_(e_auabort)); return false; diff --git a/src/nvim/window.c b/src/nvim/window.c index b55dd79260..4dc6ed370e 100644 --- a/src/nvim/window.c +++ b/src/nvim/window.c @@ -455,7 +455,7 @@ newwindow: case 'H': case 'L': CHECK_CMDWIN; - if (firstwin == curwin && lastwin_nofloating() == curwin) { + if (one_window(curwin)) { beep_flush(); } else { const int dir = ((nchar == 'H' || nchar == 'L') ? WSP_VERT : 0) @@ -1018,7 +1018,7 @@ win_T *win_split_ins(int size, int flags, win_T *new_wp, int dir, frame_T *to_fl bool toplevel = flags & (WSP_TOP | WSP_BOT); // add a status line when p_ls == 1 and splitting the first window - if (one_nonfloat() && p_ls == 1 && oldwin->w_status_height == 0) { + if (one_window(firstwin) && p_ls == 1 && oldwin->w_status_height == 0) { if (oldwin->w_height <= p_wmh) { emsg(_(e_noroom)); return NULL; @@ -1741,7 +1741,7 @@ static void win_exchange(int Prenum) return; } - if (firstwin == curwin && lastwin_nofloating() == curwin) { + if (one_window(curwin)) { // just one window beep_flush(); return; @@ -1833,7 +1833,7 @@ static void win_rotate(bool upwards, int count) return; } - if (count <= 0 || (firstwin == curwin && lastwin_nofloating() == curwin)) { + if (count <= 0 || one_window(curwin)) { // nothing to do beep_flush(); return; @@ -2495,37 +2495,13 @@ bool last_window(win_T *win) FUNC_ATTR_PURE FUNC_ATTR_WARN_UNUSED_RESULT } /// Check if "win" is the only non-floating window in the current tabpage. +/// +/// This should be used in place of ONE_WINDOW when necessary, +/// with "firstwin" or the affected window as argument depending on the situation. bool one_window(win_T *win) FUNC_ATTR_PURE FUNC_ATTR_WARN_UNUSED_RESULT { - if (win->w_floating) { - return false; - } - - bool seen_one = false; - - FOR_ALL_WINDOWS_IN_TAB(wp, curtab) { - if (!wp->w_floating) { - if (seen_one) { - return false; - } - seen_one = true; - } - } - return true; -} - -/// Like ONE_WINDOW but only considers non-floating windows -bool one_nonfloat(void) FUNC_ATTR_PURE FUNC_ATTR_WARN_UNUSED_RESULT -{ - return firstwin->w_next == NULL || firstwin->w_next->w_floating; -} - -/// if wp is the last non-floating window -/// -/// always false for a floating window -bool last_nonfloat(win_T *wp) FUNC_ATTR_PURE FUNC_ATTR_WARN_UNUSED_RESULT -{ - return wp != NULL && firstwin == wp && !(wp->w_next && !wp->w_floating); + assert(!firstwin->w_floating); + return firstwin == win && (win->w_next == NULL || win->w_next->w_floating); } /// Check if floating windows in the current tab can be closed. @@ -3950,7 +3926,7 @@ void close_others(int message, int forceit) return; } - if (one_nonfloat() && !lastwin->w_floating) { + if (one_window(firstwin) && !lastwin->w_floating) { if (message && !autocmd_busy) { msg(_(m_onlyone), 0); @@ -7224,7 +7200,7 @@ int global_stl_height(void) /// @param morewin pretend there are two or more windows if true. int last_stl_height(bool morewin) { - return (p_ls > 1 || (p_ls == 1 && (morewin || !one_nonfloat()))) ? STATUS_HEIGHT : 0; + return (p_ls > 1 || (p_ls == 1 && (morewin || !one_window(firstwin)))) ? STATUS_HEIGHT : 0; } /// Return the minimal number of rows that is needed on the screen to display diff --git a/test/functional/ui/float_spec.lua b/test/functional/ui/float_spec.lua index e324d03b30..8b7107fdf2 100644 --- a/test/functional/ui/float_spec.lua +++ b/test/functional/ui/float_spec.lua @@ -873,6 +873,39 @@ describe('float window', function() end) end) + describe(':close on non-float with floating windows', function() + it('does not quit Nvim if BufWinLeave makes it the only non-float', function() + exec([[ + let firstbuf = bufnr() + new + let midwin = win_getid() + new + setlocal bufhidden=wipe + call nvim_win_set_config(midwin, + \ #{relative: 'editor', row: 5, col: 5, width: 5, height: 5}) + autocmd BufWinLeave * ++once exe firstbuf .. 'bwipe!' + ]]) + eq('Vim(close):E855: Autocommands caused command to abort', pcall_err(command, 'close')) + assert_alive() + end) + + pending('does not crash if BufWinLeave makes it the only non-float in tabpage', function() + exec([[ + tabnew + let firstbuf = bufnr() + new + let midwin = win_getid() + new + setlocal bufhidden=wipe + call nvim_win_set_config(midwin, + \ #{relative: 'editor', row: 5, col: 5, width: 5, height: 5}) + autocmd BufWinLeave * ++once exe firstbuf .. 'bwipe!' + ]]) + eq('Vim(close):E855: Autocommands caused command to abort', pcall_err(command, 'close')) + assert_alive() + end) + end) + local function with_ext_multigrid(multigrid) local screen, attrs before_each(function() From b52d15853e89149472c1ecd9cce3a84e4af0785a Mon Sep 17 00:00:00 2001 From: Sean Dewar <6256228+seandewar@users.noreply.github.com> Date: Sat, 9 Mar 2024 16:56:32 +0000 Subject: [PATCH 21/22] fix(api): win_set_config set tp_curwin of win moved from other tabpage 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. --- src/nvim/api/win_config.c | 23 ++++++++++++---- src/nvim/window.c | 15 +---------- src/nvim/winfloat.c | 18 +++++++++++++ test/functional/api/window_spec.lua | 41 +++++++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 19 deletions(-) diff --git a/src/nvim/api/win_config.c b/src/nvim/api/win_config.c index 32b2156313..8b8eca62ca 100644 --- a/src/nvim/api/win_config.c +++ b/src/nvim/api/win_config.c @@ -468,7 +468,7 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err) int dir; win_goto(winframe_find_altwin(win, &dir, NULL, NULL)); } else { - win_goto(win_valid(prevwin) && prevwin != win ? prevwin : firstwin); + win_goto(win_float_find_altwin(win, NULL)); } // Autocommands may have been a real nuisance and messed things up... @@ -490,6 +490,8 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err) int dir = 0; frame_T *unflat_altfr = NULL; + win_T *altwin = NULL; + if (was_split) { // If the window is the last in the tabpage or `fconfig.win` is // a handle to itself, we can't split it. @@ -534,10 +536,11 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err) } // If the frame doesn't have a parent, the old frame // was the root frame and we need to create a top-level split. - winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, &unflat_altfr); + altwin = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, &unflat_altfr); } else if (n_frames == 2) { // There are two windows in the frame, we can just rotate it. - neighbor = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, &unflat_altfr); + altwin = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, &unflat_altfr); + neighbor = altwin; } else { // There is only one window in the frame, we can't split it. api_set_error(err, kErrorTypeException, "Cannot split window into itself"); @@ -546,9 +549,12 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err) // Set the parent to whatever the correct neighbor window was determined to be. parent = neighbor; } else { - winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, &unflat_altfr); + altwin = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, &unflat_altfr); } + } else { + altwin = win_float_find_altwin(win, win_tp == curtab ? NULL : win_tp); } + win_remove(win, win_tp == curtab ? NULL : win_tp); if (win_tp == curtab) { last_status(false); // may need to remove last status line @@ -556,12 +562,14 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err) } int flags = win_split_flags(fconfig.split, parent == NULL) | WSP_NOENTER; + tabpage_T *const parent_tp = parent ? win_find_tabpage(parent) : curtab; + TRY_WRAP(err, { const bool need_switch = parent != NULL && parent != curwin; switchwin_T switchwin; if (need_switch) { // `parent` is valid in its tabpage, so switch_win should not fail. - const int result = switch_win(&switchwin, parent, win_find_tabpage(parent), true); + const int result = switch_win(&switchwin, parent, parent_tp, true); (void)result; assert(result == OK); } @@ -593,6 +601,11 @@ restore_curwin: return; } + // If `win` moved tabpages and was the curwin of its old one, select a new curwin for it. + if (win_tp != parent_tp && win_tp->tp_curwin == win) { + win_tp->tp_curwin = altwin; + } + if (HAS_KEY_X(config, width)) { win_setwidth_win(fconfig.width, win); } diff --git a/src/nvim/window.c b/src/nvim/window.c index 4dc6ed370e..ecd2e83500 100644 --- a/src/nvim/window.c +++ b/src/nvim/window.c @@ -3044,20 +3044,7 @@ static win_T *win_free_mem(win_T *win, int *dirp, tabpage_T *tp) xfree(frp); } else { *dirp = 'h'; // Dummy value. - if (tp == NULL) { - if (win_valid(prevwin) && prevwin != win) { - wp = prevwin; - } else { - wp = firstwin; - } - } else { - assert(tp != curtab); - if (tabpage_win_valid(tp, tp->tp_prevwin) && tp->tp_prevwin != win) { - wp = tp->tp_prevwin; - } else { - wp = tp->tp_firstwin; - } - } + wp = win_float_find_altwin(win, tp); } win_free(win, tp); diff --git a/src/nvim/winfloat.c b/src/nvim/winfloat.c index 3ddff8aa5a..65d2c1306b 100644 --- a/src/nvim/winfloat.c +++ b/src/nvim/winfloat.c @@ -319,3 +319,21 @@ win_T *win_float_find_preview(void) } return NULL; } + +/// Select an alternative window to `win` (assumed floating) in tabpage `tp`. +/// +/// Useful for finding a window to switch to if `win` is the current window, but is then closed or +/// moved to a different tabpage. +/// +/// @param tp `win`'s original tabpage, or NULL for current. +win_T *win_float_find_altwin(const win_T *win, const tabpage_T *tp) + FUNC_ATTR_NONNULL_ARG(1) +{ + if (tp == NULL) { + return (win_valid(prevwin) && prevwin != win) ? prevwin : firstwin; + } + + assert(tp != curtab); + return (tabpage_win_valid(tp, tp->tp_prevwin) && tp->tp_prevwin != win) ? tp->tp_prevwin + : tp->tp_firstwin; +} diff --git a/test/functional/api/window_spec.lua b/test/functional/api/window_spec.lua index 6b7c550ca8..a10c8f48ef 100644 --- a/test/functional/api/window_spec.lua +++ b/test/functional/api/window_spec.lua @@ -2401,6 +2401,47 @@ describe('API/win', function() | ]]) end) + + it("updates tp_curwin of moved window's original tabpage", function() + local t1 = api.nvim_get_current_tabpage() + command('tab split | split') + local t2 = api.nvim_get_current_tabpage() + local t2_alt_win = api.nvim_get_current_win() + command('vsplit') + local t2_cur_win = api.nvim_get_current_win() + command('tabprevious') + eq(t2_cur_win, api.nvim_tabpage_get_win(t2)) + + -- tp_curwin is unchanged when moved within the same tabpage. + api.nvim_win_set_config(t2_cur_win, { split = 'left', win = t2_alt_win }) + eq(t2_cur_win, api.nvim_tabpage_get_win(t2)) + + -- Also unchanged if the move failed. + command('let &winwidth = &columns | let &winminwidth = &columns') + matches( + 'E36: Not enough room$', + pcall_err(api.nvim_win_set_config, t2_cur_win, { split = 'left', win = 0 }) + ) + eq(t2_cur_win, api.nvim_tabpage_get_win(t2)) + command('set winminwidth& winwidth&') + + -- But is changed if successfully moved to a different tabpage. + api.nvim_win_set_config(t2_cur_win, { split = 'left', win = 0 }) + eq(t2_alt_win, api.nvim_tabpage_get_win(t2)) + eq(t1, api.nvim_win_get_tabpage(t2_cur_win)) + + -- Now do it for a float, which has different altwin logic. + command('tabnext') + t2_cur_win = + api.nvim_open_win(0, true, { relative = 'editor', row = 5, col = 5, width = 5, height = 5 }) + eq(t2_alt_win, fn.win_getid(fn.winnr('#'))) + command('tabprevious') + eq(t2_cur_win, api.nvim_tabpage_get_win(t2)) + + api.nvim_win_set_config(t2_cur_win, { split = 'left', win = 0 }) + eq(t2_alt_win, api.nvim_tabpage_get_win(t2)) + eq(t1, api.nvim_win_get_tabpage(t2_cur_win)) + end) end) describe('get_config', function() From c3d22d32ee4b4c1911ec15f2a77683d09b09f845 Mon Sep 17 00:00:00 2001 From: Sean Dewar <6256228+seandewar@users.noreply.github.com> Date: Sat, 9 Mar 2024 20:24:08 +0000 Subject: [PATCH 22/22] vim-patch:8.2.3862: crash on exit with EXITFREE and using win_execute() Problem: Crash on exit with EXITFREE and using win_execute(). Solution: Also save and restore tp_topframe. (issue vim/vim#9374) https://github.com/vim/vim/commit/dab17a0689a2f31f69f428975f84b0c3c7ba3030 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 --- src/nvim/eval/window.c | 4 ++++ test/functional/lua/vim_spec.lua | 14 ++++++++++++++ test/old/testdir/test_execute_func.vim | 22 ++++++++++++++++++++++ 3 files changed, 40 insertions(+) diff --git a/src/nvim/eval/window.c b/src/nvim/eval/window.c index c2b9574579..e54f46dcc3 100644 --- a/src/nvim/eval/window.c +++ b/src/nvim/eval/window.c @@ -958,9 +958,11 @@ int switch_win_noblock(switchwin_T *switchwin, win_T *win, tabpage_T *tp, bool n if (no_display) { curtab->tp_firstwin = firstwin; curtab->tp_lastwin = lastwin; + curtab->tp_topframe = topframe; curtab = tp; firstwin = curtab->tp_firstwin; lastwin = curtab->tp_lastwin; + topframe = curtab->tp_topframe; } else { goto_tabpage_tp(tp, false, false); } @@ -989,9 +991,11 @@ void restore_win_noblock(switchwin_T *switchwin, bool no_display) if (no_display) { curtab->tp_firstwin = firstwin; curtab->tp_lastwin = lastwin; + curtab->tp_topframe = topframe; curtab = switchwin->sw_curtab; firstwin = curtab->tp_firstwin; lastwin = curtab->tp_lastwin; + topframe = curtab->tp_topframe; } else { goto_tabpage_tp(switchwin->sw_curtab, false, false); } diff --git a/test/functional/lua/vim_spec.lua b/test/functional/lua/vim_spec.lua index a262d239e8..add3df6d8a 100644 --- a/test/functional/lua/vim_spec.lua +++ b/test/functional/lua/vim_spec.lua @@ -3664,6 +3664,20 @@ describe('lua stdlib', function() ]] ) end) + + it('layout in current tabpage does not affect windows in others', function() + command('tab split') + local t2_move_win = api.nvim_get_current_win() + command('vsplit') + local t2_other_win = api.nvim_get_current_win() + command('tabprevious') + matches('E36: Not enough room$', pcall_err(command, 'execute "split|"->repeat(&lines)')) + command('vsplit') + + -- Without vim-patch:8.2.3862, this gives E36, despite just the 1st tabpage being full. + exec_lua('vim.api.nvim_win_call(..., function() vim.cmd.wincmd "J" end)', t2_move_win) + eq({ 'col', { { 'leaf', t2_other_win }, { 'leaf', t2_move_win } } }, fn.winlayout(2)) + end) end) describe('vim.iconv', function() diff --git a/test/old/testdir/test_execute_func.vim b/test/old/testdir/test_execute_func.vim index 2edae39b8f..d9909e92a6 100644 --- a/test/old/testdir/test_execute_func.vim +++ b/test/old/testdir/test_execute_func.vim @@ -3,6 +3,7 @@ source view_util.vim source check.vim source vim9.vim +source term_util.vim func NestedEval() let nested = execute('echo "nested\nlines"') @@ -177,6 +178,27 @@ func Test_win_execute_visual_redraw() bwipe! endfunc +func Test_win_execute_on_startup() + CheckRunVimInTerminal + + let lines =<< trim END + vim9script + [repeat('x', &columns)]->writefile('Xfile1') + silent tabedit Xfile2 + var id = win_getid() + silent tabedit Xfile3 + autocmd VimEnter * win_execute(id, 'close') + END + call writefile(lines, 'XwinExecute') + let buf = RunVimInTerminal('-p Xfile1 -Nu XwinExecute', {}) + + " this was crashing on exit with EXITFREE defined + call StopVimInTerminal(buf) + + call delete('XwinExecute') + call delete('Xfile1') +endfunc + func Test_execute_cmd_with_null() call assert_equal("", execute(v:_null_string)) call assert_equal("", execute(v:_null_list))