[RFC] autocmds: add WinClosed command #9066

maurelio1234 · 2018-09-29T18:46:30Z

test/functional/autocmd/autocmd_spec.lua

justinmk · 2018-09-30T14:27:49Z

Also need to test for "recursion", i.e. the case where WinClosed event causes another window to close. Usually we just disallow this, e.g. by skipping apply_autocmd if a static int recursive function-scoped variable is >0.

WinClosed ... Before closing a window. Just after WinLeave and BufWinLeave.

If it's before closing, it should be named WinClosePre.

Is there a reason for doing it before closing (that implies it's possible the window does not close for some reason)? Will users also want a "after closing" event?

* Add test to make sure event just fires once * Update documentation to clarify when event is fired

maurelio1234 · 2018-10-06T10:18:55Z

Also need to test for "recursion", i.e. the case where WinClosed event causes another window to close. Usually we just disallow this, e.g. by skipping apply_autocmd if a static int recursive function-scoped variable is >0.

I will think about that

test for recursion

maurelio1234 · 2018-10-06T10:30:11Z

If it's before closing, it should be named WinClosePre.

Is there a reason for doing it before closing (that implies it's possible the window does not close for some reason)? Will users also want a "after closing" event?

I clarified the text in my last commit, it is called after closing just before freeing resources (just like BufUnload). At least in the way it is coded now it is called when we know it is going to be closed.
I do not see the point of having a Pre/Post pair of events, looking at the current list of events, Pre/Post pairs seem to be useful when you are performing some operation to a somehow persistent resource (a file a register, a set of windows), so that you can change the resource before performing the operation.
I am still not sure if WinClosed is a good name for the event :) WinClose seems more in line with how other events are named. What do you think about that

maurelio1234 · 2018-10-17T18:31:00Z

Hi @justinmk,

I just added a test for the recursive case (https://github.com/neovim/neovim/pull/9066/files#diff-bdc18dc18b088a1cc092d1fce6face23R65) and the second level event is not fired.

Could you check if I understood your remark about recursive events correctly?

It seems to break ci tests, even though they pass on my system. Most of other tests call bufwinnr directly instead of eval'ing it.

maurelio1234 · 2018-10-21T12:15:07Z

The remaining failed builds seem to be unrelated to my PR

justinmk · 2018-10-21T16:27:48Z

This failure is related (or it exposes an existing bug):

[ RUN      ] autocmds: :WinClosed events are not recursive in different tab: FAIL
...d/neovim/neovim/test/functional/autocmd/autocmd_spec.lua:104: Expected objects to be the same.
Passed in:
(nil)
Expected:
(number) -1

stack traceback:
	...d/neovim/neovim/test/functional/autocmd/autocmd_spec.lua:104: in function <...d/neovim/neovim/test/functional/autocmd/autocmd_spec.lua:92>

==================== File /home/travis/build/neovim/neovim/build/log/ubsan.13158 ====================
= ../src/nvim/highlight.c:150:12: runtime error: member access within null pointer of type 'win_T' (aka 'struct window_S')
=     #0 0xe056c8 in update_window_hl /home/travis/build/neovim/neovim/build/../src/nvim/highlight.c:150:12
=     #1 0x147706b in update_screen /home/travis/build/neovim/neovim/build/../src/nvim/screen.c:391:9
=     #2 0x64383e in nvim_command /home/travis/build/neovim/neovim/build/../src/nvim/api/vim.c:60:3
=     #3 0x5831f8 in handle_nvim_command /home/travis/build/neovim/neovim/build/src/nvim/auto/api/private/dispatch_wrappers.generated.h:1968:3
=     #4 0x10b1b27 in on_request_event /home/travis/build/neovim/neovim/build/../src/nvim/msgpack_rpc/channel.c:365:19
=     #5 0xa7bba7 in multiqueue_process_events /home/travis/build/neovim/neovim/build/../src/nvim/event/multiqueue.c:150:7
=     #6 0x116f29b in nv_event /home/travis/build/neovim/neovim/build/../src/nvim/normal.c:8019:3
=     #7 0x111d180 in normal_execute /home/travis/build/neovim/neovim/build/../src/nvim/normal.c:1137:3
=     #8 0x17970a9 in state_enter /home/travis/build/neovim/neovim/build/../src/nvim/state.c:73:26
=     #9 0x10d7dc4 in normal_enter /home/travis/build/neovim/neovim/build/../src/nvim/normal.c:467:3
=     #10 0xebe84f in main /home/travis/build/neovim/neovim/build/../src/nvim/main.c:595:3
=     #11 0x7f71329cef44 in __libc_start_main /build/eglibc-ripdx6/eglibc-2.19/csu/libc-start.c:287
=     #12 0x44e69b in _start (/home/travis/build/neovim/neovim/build/bin/nvim+0x44e69b)
= 
= SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/nvim/highlight.c:150:12 in 
=====================================================================================================
test/helpers.lua:149: assertion failed!

stack traceback:
	test/helpers.lua:149: in function 'check_logs'
	test/functional/helpers.lua:756: in function <test/functional/helpers.lua:752>

justinmk · 2018-10-21T16:44:07Z

Likely that failure is because recursion isn't prevented. Look at do_autocmd_dirchanged for the typical way to prevent recursion. If recursive is true do not trigger the WinClose event. There is no reasonable need for a WinClose event which closes another window to cause another WinClose event. We will simply document this: at :help WinClose add a line that says Recursion is ignored., like :help DirChanged.

I do not see the point of having a Pre/Post pair of events,

But as mentioned in #3338 QuitPre already exists. Having QuitPre and WinClosed is pretty confusing ...

runtime/doc/autocmd.txt

src/nvim/auevents.lua

src/nvim/window.c

justinmk · 2018-10-21T17:09:41Z

src/nvim/window.c

+
+    // If autocommands closed the window, there's nothing else to do
+    if (!win_valid(win)) {
+      return FAIL;


Can you explain this? How could autocommands close the window during a WinClosed event for that very window? If we disable recursion (see other comments), perhaps this case is avoided.

Yes, I'm trying to avoid recursion. But as I said before, I do not see where/how is recursive defined.

You need to introduce a function-local static int recursive, see do_autocmd_dirchanged for example.

src/nvim/window.c

justinmk · 2018-10-21T17:15:47Z

src/nvim/window.c

+                   win->w_buffer->b_fname, false, win->w_buffer);
+
+    // If autocommands closed the window, there's nothing else to do
+    if (!win_valid_any_tab(win)) {


same comments as for the win_close case.

The same comment as before

test/functional/autocmd/autocmd_spec.lua

justinmk · 2018-10-21T17:19:26Z

This is looking pretty good, just need to figure out these last questions :)

WinClosed is not a command

maurelio1234 · 2018-10-21T21:12:12Z

This failure is related (or it exposes an existing bug):

Yes, it was introduced by the commits just after that comment... I saw I was missing some cases. Still working on them.

maurelio1234 · 2018-10-21T21:15:16Z

Likely that failure is because recursion isn't prevented. Look at do_autocmd_dirchanged for the typical way to prevent recursion. If recursive is true do not trigger the WinClose event. There is no reasonable need for a WinClose event which closes another window to cause another WinClose event. We will simply document this: at :help WinClose add a line that says Recursion is ignored., like :help DirChanged.

I got what you mean now! I will test it :)

maurelio1234 · 2018-10-21T22:03:54Z

I did some tests... but I'm not sure about which kinds of recursion you want to forbid here.
The problem is, there are two code paths for closing a window, either it is on the current tab or on another tab, what if closing the current window triggers a window closing on another tab, which triggers a the first window closing...

should we forbid it?

justinmk · 2018-10-21T22:10:54Z

what if closing the current window triggers a window closing on another tab, which triggers a the first window closing...

Then only the first WinClose is triggered. No recursion (call it "cascading" if that helps).

The problem is, there are two code paths for closing a window, either it is on the current tab or on another tab,

We need a new function (see do_autocmd_textyankpost for example).

Name the function do_autocmd_winclosed.
Manage the static in recursive variable there.
This function will call apply_autocmds(EVENT_WINCLOSED ....
win_close and win_close_othertab will call do_autocmd_winclosed instead of calling apply_autocmds(EVENT_WINCLOSED ... directly.

maurelio1234 · 2018-11-14T00:14:19Z

Hello, I just pushed a first take on that. The only missing point for me is the role of the field w_closing in windows... there's already a protection against a window causing itself to close recursively. I guess this field is not necessary anymore.

justinmk · 2018-11-15T04:19:26Z

still a ASAN failure. search the travis log for "AddressSanitizer" .

It looks like ASAN errors are not related to the new autocmd but to the bdelete ex cmd that seems to break related to the deleted buffer.

To make sure it won't try to use already freed resources on an autocmd.

This reverts commit bebfd6b.

We may be on a different tab after autocmd runs

maurelio1234 · 2018-12-01T22:03:08Z

Hello the ASAN error should be fixed now.
Some builds seem to be red on a missing tool (texi2pdf)

justinmk · 2020-01-13T07:24:52Z

continued at #11331 (you will still get author credit)

autocmds: add WinClosed command

c943800

marvim added the RFC label Sep 29, 2018

Marcos ALMEIDA added 2 commits September 29, 2018 22:16

clint: fix errors

7d703ff

help: Add WinClosed event to vim_diff.txt

62bac63

justinmk reviewed Sep 30, 2018

View reviewed changes

test/functional/autocmd/autocmd_spec.lua Outdated Show resolved Hide resolved

Marcos ALMEIDA added 3 commits October 6, 2018 09:17

typo: event name changed

dfc1d0e

autocmd: only fire once, just before freeing mem

c05ff0b

* Add test to make sure event just fires once * Update documentation to clarify when event is fired

typo: fixe WinClose in comments

a151563

Marcos ALMEIDA added 3 commits October 17, 2018 20:11

WinClosed: triggered when on a different buffer

365b5a8

test: WinClosed events are not called recursivelly

e272bce

refacto: use eq instead of assert.same

63bb1d4

Marcos ALMEIDA added 7 commits October 17, 2018 21:49

test: WinClosed are not recursive in the same window

57fbf14

clint: fix too long lines

3630604

clint: fix indent and whitespace

d3250a8

test: using funcs.bufwinnr instead of eval

11f4c84

It seems to break ci tests, even though they pass on my system. Most of other tests call bufwinnr directly instead of eval'ing it.

lint: fix whitespaces

c49c630

lint: fix whitespace

1e35114

win_close: fail if called recursively

27f417b

WinClosed: avoid recursive calls in another tab

f8f2a13