Fixing buffer menu having stale items with terminal / cmdline window

[ychin]

Currently, when using special buffers like terminals / command-line window / quickfix / location list, these buffers will get added to the "Buffers" menu, but they don't get removed when the buffers are gone, leaving stale menu items. Fix buffer menu to be more robust.

1. Currently the buffer menu works by using the buffer name to add/remove the entries, but it's error-prone because the buffer could have been renamed under-the-hood. While it uses BufFilePre/Post autocommands to handle normal buffer renames, it doesn't work for the command-line window (accessible via q:) which gets renamed without sending the autocommand. Instead, change the menus to cached a dictionary a bufnum -> menu name, so it will always know how to remove a buffer from itself.
2. Add BufFilePre/Post autocommands to command-line windows when it changes the buffer name to "[Command Line]".
3. Add BufFilePre/Post autocommands to terminal windows when it changes the buffer name to the command, e.g. "!/bin/zsh".

Either (1) or (2)+(3) will fix the issue, but just doing all of them as this seems like the right thing to do (2 + 3) also means the menu items show the correct names instead of just saying "[No Name]".

This doesn't fix the following which needs to be fixed later:

1. Quickfix and Location List don't send BufDeleted autocmds. This leads to them also leaving stale buffer menu items as there's no way for the script to know that those buffers are gone.

[ychin]

I didn't add any tests, as I'm not quite sure how testing runtime files (menu.vim) should work, as they tend to go in at a later time. Otherwise I could add a test that first loads in menu.vim, then makes cmdline-window and terminal windows and test that the buffer menu grows and shrink correctly.

[vim-ml]

Hi,

On Sun, Mar 15, 2020 at 1:49 PM Yee Cheng Chin ***@***.***> wrote: I didn't add any tests, as I'm not quite sure how testing runtime files ( menu.vim) should work, as they tend to go in at a later time. Otherwise I could add a test that first loads in menu.vim, then makes cmdline-window and terminal windows and test that the buffer menu grows and shrink correctly.

You can use the Test_load_menu() function in testdir/test_menu.vim as an example. This test loads menu.vim and runs a basic test. - Yegappan

[ychin]

Ok. I added a basic test that will load the buffer menu and add/remove command-line window and terminals, and confirm that the Buffers menu will add and remove the item correctly.

I can confirm that the test passes, and fails without my change.

Edit: This test does require the runtime menu.vim file to go in at the same time, as it's testing the menu.vim file as much as the .c ones.

[brammool]

Thanks for looking into this and making a patch.
I don't use this menu, I hope some others can try it out and comment.

We could just omit any kind of special buffers from the buffers menu, it's unlikely the user will want to select them. Except perhaps terminal buffers?

[ychin]

I personally don't use the Buffers menu as well, but the stale menu item was causing some integration errors on MacVim-side, but also it's annoying to have stale items floating around.

We could just omit any kind of special buffers from the buffers menu, it's unlikely the user will want to select them. Except perhaps terminal buffers?

I think this could work for quickfix (and I also don't think there is that much value in using :buffer with quickfix buffers except in esoteric cases that the Buffers menu is not a good fit for), but I'm not sure about command-line windows because there isn't an easy way to tell that a buffer is a command-line window. What this means is we could just keep the current patch that improves the way we manage the Buffers menu, while adding special logic to prevent adding quickfix buffers to the menu (by checking &buftype == 'quickfix'). This would solve all stale items.

I think there's still an inconsistency (bug?) that quickfix buffers generates BufCreate events but not BufDelete though.

Also, how do you feel about (2) / (3) (adding BufFilePre/Post events to command-line windows and terminal windows)? I guess the docs make it sound like it should only be for file-based buffers, but otherwise you have no way to know if a buffer has changed its buffer name.

[brammool]

this was included with patch 8.2.0413

[ychin]

Hi, I think this commit introduced a bug where buffer menus now doesn't include any buffers at all (until refresh happens) due to how the BMCanAdd function works. I filed #5864 to fix this.

This is also another bug where we attempt to check buftype to detect a special buffer. This actually doesn't work because buftype is set after we get the BufCreate autocommand. I think it's ok for now because at least it is not a regression compared to before.