`TabClosed` callbacks break if `expand("<afile>")` is called prior

[NGPONG]

Is this an expected behavior or a bug? Because after exploring a little deeper, I found that closing multiple panels is done via TabClosed event callbacks, defined here

https://github.com/sindrets/diffview.nvim/blob/86bf6182e2ea767c21711de8a3c396d9b635d970/lua/diffview/init.lua#L44-L50

However, the 'TabClosed' event is specified to be non-recursive (event cannot trigger itself) (I didn't find this in the :h tabclosed, but I found this logic in the neovim source code static void do_autocmd_winclosed(win_T *win)), so the end result is that the TabClosed event will not be triggered again when the close operation is performed a second time and the remaining panels will not be closed further down.

Now, I got around this by forcing the event to fire again by calling vim.api.nvim_exec_autocmds in the TabClosed callback, but I'm not sure if that's the right solution, or if the result (only two panels closed) is what you expect.

A small addition, if this is a bug, can you try to let me fix it? I really like this plugin and would like to give some modest contribution.

[sindrets]

Just because this is terminology that often confuses users of vim, and because I'm confused by the issue you're describing; I'm gonna include the summary from :h windows-intro:

Summary:
   A buffer is the in-memory text of a file.
   A window is a viewport on a buffer.
   A tab page is a collection of windows.

Please clarify: are you talking about vim tabs (the visible collection of windows), or are you talking about vim windows?

DiffviewClose or tabclose will only close a maximum of two tabs

No, :tabclose will only ever close one tab page. You can specify which one with :tabclose [count].

If you're actually talking about windows, and you're saying that :tabclose doesn't close all the windows in the current tab, then that would be unusual. In this case I would ask that you try to reproduce with a minimal config. You can find a template here.

A small addition, if this is a bug, can you try to let me fix it? I really like this plugin and would like to give some modest contribution.

I really appreciate the incentive!

[NGPONG]

Hi @sindrets Thanks for your reply!

No, :tabclose will only ever close one tab page. You can specify which one with :tabclose [count].

You are absolutely right, after some testing I found that it is caused by the logic in the callbacks of other plugins that also register the TabClosed event, where the core logic is vim.fn.expand('<afile>').

When vim.fn.expand('<afile>') is called(inside another TabClosed callback) before trigger diffview's TabClosed event, then the file field(state.file) in diffview's callback function's argument is expanded to an absolute path. So we'll get a nil result when using this absolute path to convert to a integer, which leads to the behavior of closing the second panel(as the title i said)(or should call it a tabpage) because execute M.close(nil) -- in diffview/init.lua:48 again.

I was able to determine that this was an problem with neovim(But I don't have any clue as to why this is happening), as I tried disabling all my cfg and only kept the following code to check the output:

--require 'native'
--require 'plugins'

local logger = require 'utils.log'

vim.api.nvim_create_autocmd("TabClosed", {
  group = vim.api.nvim_create_augroup('a', { clear = true, }),
  pattern = "*",
  callback = function(state)
    vim.fn.expand('<afile>')
    logger .info(state)
  end,
})

vim.api.nvim_create_autocmd("TabClosed", {
  group = vim.api.nvim_create_augroup('b', { clear = true, }),
  pattern = "*",
  callback = function(state)
    logger .info(state)
  end,
})

So I got the results that I expected

[INFO  Tue Jun  6 02:07:16 2023] _:9: {
  buf = 3,
  event = "TabClosed",
  file = "3", 
  group = 5,
  id = 3,
  match = "3"
}
[INFO  Tue Jun  6 02:07:16 2023] _:17: {
  buf = 3,
  event = "TabClosed",
  file = "/home/ngpong/neovim/3", <----------------
  group = 6,
  id = 4,
  match = "3"
}

Now I seem to be at an impasse, as the problem does seem to be somewhat outside of the current project, so should I launch a new issuse for the neovim project to continue exploring the problem (as this may stretch the timeline for solving it), or can we avoid the problem by some means in this project first.

After consulting the autocmd documentation, it is explained in the TabClosed event as:

After closing a tab page. <afile> expands to the tab page number.

So I think the effect of expanding <afile> is consistent with the state.file used in the original code (I'm not quite sure if I'm going wrong again, so correct me if I'm wrong, I'd appreciate it!) ), can we use 'vim.fn.expand('')' instead of 'args.file', which looks like this.

au("TabClosed", {
    group = M.augroup.
    pattern = "*".
    callback = function(_)
      M.close(vim.fn.expand('<afile>'))
    end.
  })

If my assumptions above are correct, please do give me a chance to try it out, thanks!

[sindrets]

Good job figuring this out. This is very strange behavior and indeed an upstream issue. You should open an issue there.

But it seems trivial to work around it in this plugin in the meantime. We just need to use state.match instead of state.file. That field seems to be unaffected by this bug (I tested with vim.fn.expand("<amatch>") and it didn't demonstrate the same strange behavior). If you wish to have a go at it, please go ahead.

[NGPONG]

https://github.com/vim/vim/blob/59f7038536a370d771758dc34036cc1424be7421/src/autocmd.c#L1984

https://github.com/vim/vim/blob/8060687905bdadc46abb68ee6d40e5660e352297/src/ex_docmd.c#L10652-L10659

https://github.com/vim/vim/blob/8060687905bdadc46abb68ee6d40e5660e352297/src/ex_docmd.c#L10623-L10640

The initialization logic for '<afile>'(autocmd_fname) and '<amatch>'(autocmd_match) is found in the three places I mentioned above. I didn't have enough time to preview it carefully, but my intuition tells me that the processing logic of '<afile>' and '<amatch>' is extremely similar in some cases. So I think there may be some unknown risks in the temporary replacement of '<amatch>'.

If you wish to have a go at it, please go ahead.

One of the immediate priorities is to send a new issuse for upstream and continue to explore this problem(neovim/neovim#23941). I wouldn't be reckless enough to add unknown risks to your project, and I'm not sure that this solution will destroy the ideas and programming design you have in this project. So before I start I would like to ask your opinion again, which I respect very much. @sindrets

[NGPONG]

Hmm, it looks like this bug has been around for a while neovim/neovim#18964. But thankfully it's making some headway now. neovim/neovim#23943

[sindrets]

Very nice, it's now been fixed upstream. However, while I see that the fix is scheduled for backporting to v0.9, we aim to support 2 versions behind the current stable release. I suggest we go through with implementing the workaround of using match instead when vim.fn.has("nvim-0.9.2") ~= 1.

[NGPONG]

I suggest we go through with implementing the workaround of using match instead when vim.fn.has("nvim-0.9.2") ~= 1.

Wise move! I'm working on it.