refactor(change): do API changes to buffer without curbuf switch

[bfredl]

warning: early working-on-the-train-stream-of-conciousness draft.

Most of the messy things when changing a non-current buffer is not about the buffer, it is about windows. In particular, it is about curwin.

When editing a non-current buffer which is displayed in some other window in the current tabpage, one such window will be "borrowed" as the curwin.
But this means if two or more non-current windows displayed the buffer, one of them will be arbitrarily selected to be treated differently. this is not desirable.

This branch currently disables all such borrowed-curwin logic, but this likely breaks some useful expectations which should be fixed instead by applying these window checks to all non-current windows (I am looking at you, fix_cursor() ) Done and added tests

[sandersantema]

Fyi this commit seems to break telescope, the list of results is not always shown and only reappears after selecting another entry in the list. I really don't understand enough about either this commit or telescope to say whether this is a new bug caused by this commit or something which should be fixed by telescope, which might rely on old behavior from before this commit. But with the ol' reliable git bisect I could confirm the problem with telescope started with this commit. Is this the right place for a message like this or would it be better fit as a separate issue? Good to know for the future :)

If my explanation of the problem in text wasn't clear enough here are some screenshots; in the neovim repo require('telescope.builtin').find_files() for instance shows the following for the search afad:

As you can see by the counter 25 results should be displayed but aren't.
And only shows the following after selecting result:

[bfredl]

Hmm it seems to behave correctly but there are some missing redraws, leading to the glitches you describe. it seems to depend on the picker type, like find_files is borked but live_grep seems to look just fine.

[petobens]

I reported the same thing in the telescope repo nvim-telescope/telescope.nvim#2667

Also cmp completion seems broken now since we cannot move across the list of candidates in the popup window:

[rhcher]

Meet strange thing with this PR, when I save a buffer, the first line will disappear. I also can't see this change in undo tree.

[bfredl]

The cmp issue is likely the same redraw issue. the actual item is navigated in curbuf, but the popup menu (re-implemented as a float) is not redrawn.

[bfredl]

@rhcher can this also be reproduced with --clean ? if not, I need minimal config to reproduce.

[bfredl]

Locally reverting just this part seems to fix the telescope issue for me:

diff --git a/src/nvim/api/vim.c b/src/nvim/api/vim.c
index b278a21d8e..4179ae40b8 100644
--- a/src/nvim/api/vim.c
+++ b/src/nvim/api/vim.c
@@ -912,17 +912,14 @@ Buffer nvim_create_buf(Boolean listed, Boolean scratch, Error *err)
     goto fail;
   }
 
-  // Only strictly needed for scratch, but could just as well be consistent
-  // and do this now. buffer is created NOW, not when it latter first happen
-  // to reach a window or aucmd_prepbuf() ..
-  buf_copy_options(buf, BCO_ENTER | BCO_NOHELP);
-
   if (scratch) {
-    set_string_option_direct_in_buf(buf, "bufhidden", -1, "hide", OPT_LOCAL, 0);
-    set_string_option_direct_in_buf(buf, "buftype", -1, "nofile", OPT_LOCAL, 0);
-    assert(buf->b_ml.ml_mfp->mf_fd < 0);  // ml_open() should not have opened swapfile already
-    buf->b_p_swf = false;
-    buf->b_p_ml = false;
+    aco_save_T aco;
+    aucmd_prepbuf(&aco, buf);
+    set_option_value("bufhidden", STATIC_CSTR_AS_OPTVAL("hide"), OPT_LOCAL);
+    set_option_value("buftype", STATIC_CSTR_AS_OPTVAL("nofile"), OPT_LOCAL);
+    set_option_value("swapfile", BOOLEAN_OPTVAL(false), OPT_LOCAL);
+    set_option_value("modeline", BOOLEAN_OPTVAL(false), OPT_LOCAL);  // 'nomodeline'
+    aucmd_restbuf(&aco);
   }
   return buf->b_fnum;

I'll debug a bit more, but if I don't find something we can revert just that while keep the bulk of the changes, for now.

[rhcher]

@rhcher can this also be reproduced with --clean ? if not, I need minimal config to reproduce.

That can not be reproduced with --clean, and it related to noice.nvim
I reproduced with this minimal config:

-- DO NOT change the paths and don't remove the colorscheme
local root = vim.fn.fnamemodify("./.repro", ":p")

-- set stdpaths to use .repro
for _, name in ipairs({ "config", "data", "state", "cache" }) do
  vim.env[("XDG_%s_HOME"):format(name:upper())] = root .. "/" .. name
end

-- bootstrap lazy
local lazypath = root .. "/plugins/lazy.nvim"
if not vim.loop.fs_stat(lazypath) then
  vim.fn.system({ "git", "clone", "--filter=blob:none", "https://github.com/folke/lazy.nvim.git", lazypath, })
end
vim.opt.runtimepath:prepend(lazypath)

-- install plugins
local plugins = {
  "folke/tokyonight.nvim",
  {
    "folke/noice.nvim",
    event = "VeryLazy",
    opts = {},
    dependencies  = {
      "MunifTanjim/nui.nvim",
    },
  },
  -- add any other plugins here
}
require("lazy").setup(plugins, {
  root = root .. "/plugins",
})

vim.cmd.colorscheme("tokyonight")

Steps To Reproduce

1. touch test.c
2. add something in test.c file
3. open test.c nvim -u repro.lua
4. the first line disappear
5. If I add first line back, when I save buffer, it also disppear.

I don't know if its an nvim issue or a noice.nvim issue, all I can determine is that it comes after this pr. If you're sure it's an nvim related issue, then I'd be happy to create an issue to track this down.

[bfredl]

correction: the telescope issue was rather about topline. for some reason it is being adjusted differently for a non-current window.

@sandersantema @petobens if you like you could test this branch #24889

[sandersantema]

@bfredl #24889 did the trick! Thanks.

[gegoune]

correction: the telescope issue was rather about topline. for some reason it is being adjusted differently for a non-current window.

if you like you could test this branch #24889

Tested it with telescope and cmp. Telescope issue is fixed, but cmp remains broken.

[SleepySwords]

This seems to make nvim crash for me on line 3220 of undo.c.

This patch seems to fix it for me, idk if this is actually right tho

diff --git a/src/nvim/undo.c b/src/nvim/undo.c
index 552120d4a..52bab2d2a 100644
--- a/src/nvim/undo.c
+++ b/src/nvim/undo.c
@@ -3211,12 +3211,12 @@ u_header_T *u_force_get_undo_header(buf_T *buf)
     // Undo is normally invoked in change code, which already has swapped
     // curbuf.
     // Args are tricky: this means replace empty range by empty range..
-    u_savecommon(curbuf, 0, 1, 1, true);
+    u_savecommon(buf, 0, 1, 1, true);
 
     uhp = buf->b_u_curhead;
     if (!uhp) {
       uhp = buf->b_u_newhead;
-      if (get_undolevel(curbuf) > 0 && !uhp) {
+      if (get_undolevel(buf) > 0 && !uhp) {
         abort();
       }
     }

[bfredl]

@SleepySwords the fix is indeed correct. added to the same PR.

[bfredl]

Unfortunately the first "regression fix" in #24889 was not correct. The (old master) behavior of topline adjustment was invalid and self-inconsistent, but unfortunately some plugins like telescope has come to depend on this incorrect behavior. The added test cases show what I think should be the correct behavior, i e topline is kept on the same line (i e the one with the same contents), even if its number changed. The fix was yet incomplete tho, and there is thus another change now in the PR which might help regressions in other plugins.

Regardless, the behavior currently in 0.9 (and master two days ago) is self-inconsistent (the third added test case, which multiple split windows into the same buffer, already did the "new" correct behavior), and has to be changed.