From 7180ef690180cf92d1d49811820c46dd60e4d1c6 Mon Sep 17 00:00:00 2001 From: Sean Dewar <6256228+seandewar@users.noreply.github.com> Date: Mon, 15 Apr 2024 00:10:16 +0100 Subject: [PATCH] feat(api)!: nvim_open_win: noautocmd blocks all autocmds #28192 Problem: noautocmd is confusing; despite its name, it doesn't block all autocommands (instead it blocks only those related to setting the buffer), and is commonly used by plugins to open windows while producing minimal side-effects. Solution: be consistent and block all autocommands when noautocmd is set. This includes WinNew (again), plus autocommands from entering the window (if enter is set) like WinEnter, WinLeave, TabEnter, .etc. See the discussion at https://github.com/neovim/neovim/pull/14659#issuecomment-2040029517 for more information. Remove win_set_buf's noautocmd argument, as it's no longer needed. NOTE: pum_create_float_preview sets noautocmd for win_set_buf, but all its callers already use block_autocmds. Despite that, pum_create_float_preview doesn't actually properly handle autocommands (it has no checks for whether those from win_enter or nvim_create_buf free the window). For now, ensure autocommands are blocked within it for correctness (in case it's ever called outside of a block_autocmds context; the function seems to have been refactored in #26739 anyway). --- runtime/doc/api.txt | 5 ++-- runtime/doc/news.txt | 3 +++ runtime/lua/vim/_meta/api.lua | 5 ++-- src/nvim/api/win_config.c | 33 +++++++++++++++--------- src/nvim/api/window.c | 2 +- src/nvim/popupmenu.c | 9 ++++++- src/nvim/window.c | 8 +----- test/functional/api/window_spec.lua | 39 +++++++++++++---------------- 8 files changed, 56 insertions(+), 48 deletions(-) diff --git a/runtime/doc/api.txt b/runtime/doc/api.txt index bf56a09ac7..91b80601da 100644 --- a/runtime/doc/api.txt +++ b/runtime/doc/api.txt @@ -3271,9 +3271,8 @@ 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 autocommands triggered from - setting the `buffer` to display are blocked (e.g: - |BufEnter|, |BufLeave|, |BufWinEnter|). + • noautocmd: If true then all autocommands are blocked for + the duration of the call. • 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/doc/news.txt b/runtime/doc/news.txt index 15bdfb52f6..ae5f808a9c 100644 --- a/runtime/doc/news.txt +++ b/runtime/doc/news.txt @@ -137,6 +137,9 @@ The following changes may require adaptations in user config or plugins. • |:TOhtml| has been rewritten in Lua to support Neovim-specific decorations, and many options have been removed. +• |nvim_open_win()| now blocks all autocommands when `noautocmd` is set, + rather than just those from setting the `buffer` to display in the window. + ============================================================================== BREAKING CHANGES IN HEAD *news-breaking-dev* diff --git a/runtime/lua/vim/_meta/api.lua b/runtime/lua/vim/_meta/api.lua index 678d6d3500..f56c256da6 100644 --- a/runtime/lua/vim/_meta/api.lua +++ b/runtime/lua/vim/_meta/api.lua @@ -1718,9 +1718,8 @@ 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 autocommands triggered from ---- setting the `buffer` to display are blocked (e.g: ---- `BufEnter`, `BufLeave`, `BufWinEnter`). +--- • noautocmd: If true then all autocommands are blocked for +--- the duration of the call. --- • 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 feb4271b5f..11b6b17516 100644 --- a/src/nvim/api/win_config.c +++ b/src/nvim/api/win_config.c @@ -199,9 +199,8 @@ /// - 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 autocommands triggered from setting the -/// `buffer` to display are blocked (e.g: |BufEnter|, |BufLeave|, -/// |BufWinEnter|). +/// - noautocmd: If true then all autocommands are blocked for the duration of +/// the call. /// - 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. @@ -230,6 +229,10 @@ Window nvim_open_win(Buffer buffer, Boolean enter, Dict(win_config) *config, Err } bool is_split = HAS_KEY_X(config, split) || HAS_KEY_X(config, vertical); + Window rv = 0; + if (fconfig.noautocmd) { + block_autocmds(); + } win_T *wp = NULL; tabpage_T *tp = curtab; @@ -239,15 +242,15 @@ Window nvim_open_win(Buffer buffer, Boolean enter, Dict(win_config) *config, Err parent = find_window_by_handle(fconfig.window, err); if (!parent) { // find_window_by_handle has already set the error - return 0; + goto cleanup; } else if (parent->w_floating) { api_set_error(err, kErrorTypeException, "Cannot split a floating window"); - return 0; + goto cleanup; } } if (!check_split_disallowed_err(parent ? parent : curwin, err)) { - return 0; // error already set + goto cleanup; // error already set } if (HAS_KEY_X(config, vertical) && !HAS_KEY_X(config, split)) { @@ -283,7 +286,7 @@ Window nvim_open_win(Buffer buffer, Boolean enter, Dict(win_config) *config, Err if (!ERROR_SET(err)) { api_set_error(err, kErrorTypeException, "Failed to create window"); } - return 0; + goto cleanup; } // Autocommands may close `wp` or move it to another tabpage, so update and check `tp` after each @@ -291,8 +294,8 @@ Window nvim_open_win(Buffer buffer, Boolean enter, Dict(win_config) *config, Err // 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; - { + if (!fconfig.noautocmd) { + switchwin_T switchwin; const int result = switch_win_noblock(&switchwin, wp, tp, true); assert(result == OK); (void)result; @@ -313,7 +316,7 @@ Window nvim_open_win(Buffer buffer, Boolean enter, Dict(win_config) *config, Err autocmd_no_enter++; autocmd_no_leave++; } - win_set_buf(wp, buf, fconfig.noautocmd, err); + win_set_buf(wp, buf, err); if (!fconfig.noautocmd) { tp = win_find_tabpage(wp); } @@ -324,14 +327,20 @@ Window nvim_open_win(Buffer buffer, Boolean enter, Dict(win_config) *config, Err } if (!tp) { api_set_error(err, kErrorTypeException, "Window was closed immediately"); - return 0; + goto cleanup; } if (fconfig.style == kWinStyleMinimal) { win_set_minimal_style(wp); didset_window_options(wp, true); } - return wp->handle; + rv = wp->handle; + +cleanup: + if (fconfig.noautocmd) { + unblock_autocmds(); + } + return rv; #undef HAS_KEY_X } diff --git a/src/nvim/api/window.c b/src/nvim/api/window.c index 30f77c7248..08ecca1380 100644 --- a/src/nvim/api/window.c +++ b/src/nvim/api/window.c @@ -71,7 +71,7 @@ void nvim_win_set_buf(Window window, Buffer buffer, Error *err) api_set_error(err, kErrorTypeException, "%s", e_cmdwin); return; } - win_set_buf(win, buf, false, err); + win_set_buf(win, buf, err); } /// Gets the (1,0)-indexed, buffer-relative cursor position for a given window diff --git a/src/nvim/popupmenu.c b/src/nvim/popupmenu.c index bed0a8df4a..73e019bc50 100644 --- a/src/nvim/popupmenu.c +++ b/src/nvim/popupmenu.c @@ -666,6 +666,7 @@ void pum_redraw(void) } /// create a floating preview window for info +/// Autocommands are blocked for the duration of the call. /// @return NULL when no enough room to show static win_T *pum_create_float_preview(bool enter) { @@ -690,6 +691,9 @@ static win_T *pum_create_float_preview(bool enter) config.height = pum_height; config.style = kWinStyleMinimal; config.hide = true; + + block_autocmds(); + Error err = ERROR_INIT; win_T *wp = win_new_float(NULL, true, config, &err); // TODO(glepnir): remove win_enter usage @@ -701,6 +705,7 @@ static win_T *pum_create_float_preview(bool enter) Buffer b = nvim_create_buf(false, true, &err); if (!b) { win_free(wp, NULL); + unblock_autocmds(); return NULL; } buf_T *buf = find_buffer_by_handle(b, &err); @@ -709,7 +714,9 @@ static win_T *pum_create_float_preview(bool enter) wp->w_float_is_info = true; wp->w_p_diff = false; buf->b_p_bl = false; - win_set_buf(wp, buf, true, &err); + win_set_buf(wp, buf, &err); + + unblock_autocmds(); return wp; } diff --git a/src/nvim/window.c b/src/nvim/window.c index 9f030d2bab..d86584e9ce 100644 --- a/src/nvim/window.c +++ b/src/nvim/window.c @@ -735,16 +735,13 @@ static void cmd_with_count(char *cmd, char *bufp, size_t bufsize, int64_t Prenum } } -void win_set_buf(win_T *win, buf_T *buf, bool noautocmd, Error *err) +void win_set_buf(win_T *win, buf_T *buf, Error *err) FUNC_ATTR_NONNULL_ALL { tabpage_T *tab = win_find_tabpage(win); // no redrawing and don't set the window title RedrawingDisabled++; - if (noautocmd) { - block_autocmds(); - } switchwin_T switchwin; if (switch_win_noblock(&switchwin, win, tab, true) == FAIL) { @@ -770,9 +767,6 @@ void win_set_buf(win_T *win, buf_T *buf, bool noautocmd, Error *err) cleanup: restore_win_noblock(&switchwin, true); - if (noautocmd) { - unblock_autocmds(); - } RedrawingDisabled--; } diff --git a/test/functional/api/window_spec.lua b/test/functional/api/window_spec.lua index 6a99352b3c..d9d3772df2 100644 --- a/test/functional/api/window_spec.lua +++ b/test/functional/api/window_spec.lua @@ -1162,27 +1162,6 @@ describe('API/win', function() end) describe('open_win', function() - it('noautocmd option works', function() - command('autocmd BufEnter,BufLeave,BufWinEnter * let g:fired = 1') - api.nvim_open_win(api.nvim_create_buf(true, true), true, { - relative = 'win', - row = 3, - col = 3, - width = 12, - height = 3, - noautocmd = true, - }) - eq(0, fn.exists('g:fired')) - api.nvim_open_win(api.nvim_create_buf(true, true), true, { - relative = 'win', - row = 3, - col = 3, - width = 12, - height = 3, - }) - eq(1, fn.exists('g:fired')) - end) - it('disallowed in cmdwin if enter=true or buf=cmdwin_buf', function() local new_buf = api.nvim_create_buf(true, true) feed('q:') @@ -1406,6 +1385,24 @@ describe('API/win', function() return info end + it('noautocmd option works', function() + local info = setup_tabbed_autocmd_test() + + api.nvim_open_win( + info.other_buf, + true, + { split = 'left', win = info.tab2_curwin, noautocmd = true } + ) + eq({}, eval('result')) + + api.nvim_open_win( + info.orig_buf, + true, + { relative = 'editor', row = 0, col = 0, width = 10, height = 10, noautocmd = true } + ) + eq({}, eval('result')) + end) + it('fires expected autocmds when creating splits without entering', function() local info = setup_tabbed_autocmd_test()