patch 9.1.0175: wrong window positions with 'winfix{width,height}'

Problem:  winframe functions incorrectly recompute window positions if
          the altframe wasn't adjacent to the closed frame, which is
          possible if adjacent windows had 'winfix{width,height}' set.

Solution: recompute for windows within the parent of the altframe and
          closed frame. Skip this (as before) if the altframe was
          top/left, but only if adjacent to the closed frame, as
          positions won't change in that case. Also correct the return
          value documentation for win_screenpos. (Sean Dewar)

The issue revealed itself after removing the win_comp_pos call below
winframe_restore in win_splitmove. Similarly, wrong positions could result from
windows closed in other tabpages, as win_free_mem uses winframe_remove (at least
until it is entered later, where enter_tabpage calls win_comp_pos).

NOTE: As win_comp_pos handles only curtab, it's possible via other means for
positions in non-current tabpages to be wrong (e.g: after changing 'laststatus',
'showtabline', etc.). Given enter_tabpage recomputes it, maybe it's intentional
as an optimization? Should probably be documented in win_screenpos then, but I
won't address that here.

closes: #14191

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 Mar 12
+*builtin.txt*	For Vim version 9.1.  Last change: 2024 Mar 13
 
 
 		  VIM REFERENCE MANUAL	  by Bram Moolenaar
@@ -10829,8 +10829,7 @@ win_screenpos({nr})					*win_screenpos()*
 		[1, 1], unless there is a tabline, then it is [2, 1].
 		{nr} can be the window number or the |window-ID|.  Use zero
 		for the current window.
-		Returns [0, 0] if the window cannot be found in the current
-		tabpage.
+		Returns [0, 0] if the window cannot be found.
 
 		Can also be used as a |method|: >
 			GetWinid()->win_screenpos()

src/testdir/test_window_cmd.vim

@@ -218,11 +218,11 @@ func Test_window_split_edit_bufnr()
   %bw!
 endfunc
 
-func s:win_layout_info() abort
+func s:win_layout_info(tp = tabpagenr()) abort
   return #{
-        \ layout: winlayout(),
-        \ pos_sizes: range(1, winnr('$'))
-        \            ->map({_, nr -> win_getid(nr)->getwininfo()[0]})
+        \ layout: winlayout(a:tp),
+        \ pos_sizes: range(1, tabpagewinnr(a:tp, '$'))
+        \            ->map({_, nr -> win_getid(nr, a:tp)->getwininfo()[0]})
         \            ->map({_, wininfo -> #{id: wininfo.winid,
         \                                   row: wininfo.winrow,
         \                                   col: wininfo.wincol,
@@ -2210,4 +2210,69 @@ func Test_win_gotoid_splitmove_textlock_cmdwin()
            \ .. ":call assert_equal('', win_gettype(winnr('#')))\<CR>", 'ntx')
 endfunc
 
+func Test_winfixsize_positions()
+  " Check positions are correct when closing a window in a non-current tabpage
+  " causes non-adjacent window to fill the space due to 'winfix{width,height}'.
+  tabnew
+  vsplit
+  wincmd |
+  split
+  set winfixheight
+  split foo
+  tabfirst
+
+  bwipe! foo
+  " Save actual values before entering the tabpage.
+  let info = s:win_layout_info(2)
+  tabnext
+  " Compare it with the expected value (after win_comp_pos) from entering.
+  call assert_equal(s:win_layout_info(), info)
+
+  $tabnew
+  split
+  split
+  wincmd k
+  belowright vsplit
+  set winfixwidth
+  belowright vsplit foo
+  tabprevious
+
+  bwipe! foo
+  " Save actual values before entering the tabpage.
+  let info = s:win_layout_info(3)
+  tabnext
+  " Compare it with the expected value (after win_comp_pos) from entering.
+  call assert_equal(s:win_layout_info(), info)
+
+  " Check positions unchanged when failing to move a window, if 'winfix{width,
+  " height}' would otherwise cause a non-adjacent window to fill the space.
+  %bwipe
+  call assert_fails('execute "split|"->repeat(&lines)', 'E36:')
+  wincmd p
+  vsplit
+  set winfixwidth
+  vsplit
+  set winfixwidth
+  vsplit
+  vsplit
+  set winfixwidth
+  wincmd p
+
+  let info = s:win_layout_info()
+  call assert_fails('wincmd J', 'E36:')
+  call assert_equal(info, s:win_layout_info())
+
+  only
+  call assert_fails('execute "vsplit|"->repeat(&columns)', 'E36:')
+  belowright split
+  set winfixheight
+  belowright split
+
+  let info = s:win_layout_info()
+  call assert_fails('wincmd H', 'E36:')
+  call assert_equal(info, s:win_layout_info())
+
+  %bwipe
+endfunc
+
 " vim: shiftwidth=2 sts=2 expandtab

src/version.c

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

src/window.c

@@ -3493,6 +3493,7 @@ winframe_remove(
     frame_T	*frp, *frp2, *frp3;
     frame_T	*frp_close = win->w_frame;
     win_T	*wp;
+    int		row, col;
 
     /*
      * If there is only one window there is nothing to remove.
@@ -3500,6 +3501,12 @@ winframe_remove(
     if (tp == NULL ? ONE_WINDOW : tp->tp_firstwin == tp->tp_lastwin)
 	return NULL;
 
+    // Save the position of the containing frame (which will also contain the
+    // altframe) before we remove anything, to recompute window positions later.
+    wp = frame2win(frp_close->fr_parent);
+    row = wp->w_winrow;
+    col = wp->w_wincol;
+
     /*
      * Remove the window from its frame.
      */
@@ -3584,15 +3591,10 @@ winframe_remove(
 	*dirp = 'h';
     }
 
-    // If rows/columns go to a window below/right its positions need to be
-    // updated.  Can only be done after the sizes have been updated.
-    if (frp2 == frp_close->fr_next)
-    {
-	int row = win->w_winrow;
-	int col = win->w_wincol;
-
-	frame_comp_pos(frp2, &row, &col);
-    }
+    // If the altframe wasn't adjacent and left/above, resizing it will have
+    // changed window positions within the parent frame.  Recompute them.
+    if (frp2 != frp_close->fr_prev)
+	frame_comp_pos(frp_close->fr_parent, &row, &col);
 
     if (unflat_altfr == NULL)
 	frame_flatten(frp2);
@@ -3666,8 +3668,6 @@ frame_flatten(frame_T *frp)
 winframe_restore(win_T *wp, int dir, frame_T *unflat_altfr)
 {
     frame_T	*frp = wp->w_frame;
-    int		row = wp->w_winrow;
-    int		col = wp->w_wincol;
 
     // Put "wp"'s frame back where it was.
     if (frp->fr_prev != NULL)
@@ -3691,19 +3691,23 @@ winframe_restore(win_T *wp, int dir, frame_T *unflat_altfr)
     {
 	frame_new_height(unflat_altfr, unflat_altfr->fr_height - frp->fr_height,
 		unflat_altfr == frp->fr_next, FALSE);
-	row += frp->fr_height;
     }
     else if (dir == 'h')
     {
 	frame_new_width(unflat_altfr, unflat_altfr->fr_width - frp->fr_width,
 		unflat_altfr == frp->fr_next, FALSE);
-	col += frp->fr_width;
     }
 
-    // If rows/columns went to a window below/right, its positions need to be
-    // restored.  Can only be done after the sizes have been updated.
-    if (unflat_altfr == frp->fr_next)
-	frame_comp_pos(unflat_altfr, &row, &col);
+    // Recompute window positions within the parent frame to restore them.
+    // Positions were unchanged if the altframe was adjacent and left/above.
+    if (unflat_altfr != frp->fr_prev)
+    {
+	win_T	*topleft = frame2win(frp->fr_parent);
+	int	row = topleft->w_winrow;
+	int	col = topleft->w_wincol;
+
+	frame_comp_pos(frp->fr_parent, &row, &col);
+    }
 }
 
 /*