patch 9.1.0117: Stop split-moving from firing WinNew and WinNewPre au…

…tocommands

Problem:  win_splitmove fires WinNewPre and possibly WinNew when moving
          windows, even though no new windows are created.
Solution: don't fire WinNew and WinNewPre when inserting an existing
          window, even if it isn't the current window. Improve the
          accuracy of related documentation. (Sean Dewar)

Likewise, before this patch, WinClosed was not fired anyway (even for :wincmd
H/J/K/L, which also didn't fire WinNew, but did still fire WinNewPre), despite
documentation saying windows are "closed". Note that :wincmd T actually indeed
works by creating a new window (and closing the old one), unlike the others.

This also fixes issues where WinNewPre is fired when split-moving while curwin
doesn't yet have a frame or entry in the window list, causing many things to not
work (it's not considered valid at that point). This was guaranteed when using
:wincmd H/J/K/L.

Because WinNewPre is no longer fired when split-moving, this makes restoring the
previous window layout on failure easier, as we can be sure that frames are not
resized from WinNewPre autocommands if win_split_ins fails. This allows us to
use a different strategy in the following commit.

--

In my opinion, this leaves questions about the current usefulness of WinNewPre.
A motivation described in #10635 states how creating a new window can steal room
from other windows, and how WinNewPre will be useful for detecting that, but
this is also true when inserting an existing window, which now doesn't fire it.
Maybe the autocommand should be changed to have a better name?

There are also other issues I found with the current implementation of WinNewPre
that need addressing:

- it allows switching windows and tabpages, which can cause incorrect windows to
  be split/moved, and big problems when switching tabpages.

- it fires before win_split_ins checks for room, before it makes any changes to
  window sizes or before it considers allocating a new window. This should be
  changed or documented.

I hope to address some of this stuff in a different PR, if possible.

related: #14038

Signed-off-by: Sean Dewar <6256228+seandewar@users.noreply.github.com>
Signed-off-by: Christian Brabandt <cb@256bit.org>

runtime/doc/builtin.txt

@@ -1,4 +1,4 @@
-*builtin.txt*	For Vim version 9.1.  Last change: 2024 Feb 12
+*builtin.txt*	For Vim version 9.1.  Last change: 2024 Feb 20
 
 
 		  VIM REFERENCE MANUAL	  by Bram Moolenaar
@@ -10762,10 +10762,10 @@ win_screenpos({nr})					*win_screenpos()*
 			GetWinid()->win_screenpos()
 <
 win_splitmove({nr}, {target} [, {options}])		*win_splitmove()*
-		Move the window {nr} to a new split of the window {target}.
-		This is similar to moving to {target}, creating a new window
-		using |:split| but having the same contents as window {nr}, and
-		then closing {nr}.
+		Temporarily switch to window {target}, then move window {nr}
+		to a new split adjacent to {target}.
+		Unlike commands such as |:split|, no new windows are created
+		(the |window-ID| of window {nr} is unchanged after the move).
 
 		Both {nr} and {target} can be window numbers or |window-ID|s.
 		Both must be in the current tab page.

runtime/doc/windows.txt

@@ -1,4 +1,4 @@
-*windows.txt*   For Vim version 9.1.  Last change: 2022 Nov 27
+*windows.txt*   For Vim version 9.1.  Last change: 2024 Feb 20
 
 
 		  VIM REFERENCE MANUAL    by Bram Moolenaar
@@ -519,35 +519,33 @@ horizontally split windows.  CTRL-W H does it the other way around.
 
 						*CTRL-W_K*
 CTRL-W K	Move the current window to be at the very top, using the full
-		width of the screen.  This works like closing the current
-		window and then creating another one with ":topleft split",
-		except that the current window contents is used for the new
-		window.
+		width of the screen.  This works like `:topleft split`, except
+		it is applied to the current window and no new window is
+		created.
 
 						*CTRL-W_J*
 CTRL-W J	Move the current window to be at the very bottom, using the
-		full width of the screen.  This works like closing the current
-		window and then creating another one with ":botright split",
-		except that the current window contents is used for the new
-		window.
+		full width of the screen.  This works like `:botright split`,
+		except it is applied to the current window and no new window
+		is created.
 
 						*CTRL-W_H*
 CTRL-W H	Move the current window to be at the far left, using the
-		full height of the screen.  This works like closing the
-		current window and then creating another one with
-		`:vert topleft split`, except that the current window contents
-		is used for the new window.
+		full height of the screen.  This works like
+		`:vert topleft split`, except it is applied to the current
+		window and no new window is created.
 
 						*CTRL-W_L*
 CTRL-W L	Move the current window to be at the far right, using the full
-		height of the screen.  This works like closing the
-		current window and then creating another one with
-		`:vert botright split`, except that the current window
-		contents is used for the new window.
+		height of the screen.  This works like `:vert botright split`,
+		except it is applied to the current window and no new window
+		is created.
 
 						*CTRL-W_T*
 CTRL-W T	Move the current window to a new tab page.  This fails if
 		there is only one window in the current tab page.
+		This works like `:tab split`, except the previous window is
+		closed.
 		When a count is specified the new tab page will be opened
 		before the tab page with this index.  Otherwise it comes after
 		the current tab page.

src/testdir/test_window_cmd.vim

@@ -1104,6 +1104,18 @@ func Test_win_splitmove()
   leftabove split b
   leftabove vsplit c
   leftabove split d
+
+  " win_splitmove doesn't actually create or close any windows, so expect an
+  " unchanged winid and no WinNew/WinClosed events, like :wincmd H/J/K/L.
+  let s:triggered = []
+  augroup WinSplitMove
+    au!
+    au WinNewPre * let s:triggered += ['WinNewPre']
+    au WinNew * let s:triggered += ['WinNew', win_getid()]
+    au WinClosed * let s:triggered += ['WinClosed', str2nr(expand('<afile>'))]
+  augroup END
+  let winid = win_getid()
+
   call assert_equal(0, win_splitmove(winnr(), winnr('l')))
   call assert_equal(bufname(winbufnr(1)), 'c')
   call assert_equal(bufname(winbufnr(2)), 'd')
@@ -1126,6 +1138,11 @@ func Test_win_splitmove()
   call assert_equal(bufname(winbufnr(3)), 'a')
   call assert_equal(bufname(winbufnr(4)), 'd')
   call assert_fails('call win_splitmove(winnr(), winnr("k"), test_null_dict())', 'E1297:')
+  call assert_equal([], s:triggered)
+  call assert_equal(winid, win_getid())
+
+  unlet! s:triggered
+  au! WinSplitMove
   only | bd
 
   call assert_fails('call win_splitmove(winnr(), 123)', 'E957:')

src/version.c

@@ -704,6 +704,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    117,
 /**/
     116,
 /**/

src/window.c

@@ -935,6 +935,8 @@ win_split(int size, int flags)
  * When "new_wp" is NULL: split the current window in two.
  * When "new_wp" is not NULL: insert this window at the far
  * top/left/right/bottom.
+ * On failure, if "new_wp" was not NULL, no changes will have been made to the
+ * window layout or sizes.
  * Return FAIL for failure, OK otherwise.
  */
     int
@@ -964,7 +966,8 @@ win_split_ins(
     // Do not redraw here, curwin->w_buffer may be invalid.
     ++RedrawingDisabled;
 
-    trigger_winnewpre();
+    if (new_wp == NULL)
+	trigger_winnewpre();
 
     if (flags & WSP_TOP)
 	oldwin = firstwin;
@@ -1444,7 +1447,7 @@ win_split_ins(
     /*
      * make the new window the current window
      */
-    (void)win_enter_ext(wp, WEE_TRIGGER_NEW_AUTOCMDS
+    (void)win_enter_ext(wp, (new_wp == NULL ? WEE_TRIGGER_NEW_AUTOCMDS : 0)
 		    | WEE_TRIGGER_ENTER_AUTOCMDS | WEE_TRIGGER_LEAVE_AUTOCMDS);
     if (flags & WSP_VERT)
 	p_wiw = i;