vim-patch:9.1.0118: Use different restoration strategy in win_splitmove

Problem:  saving and restoring all frames to split-move is overkill now
          that WinNewPre is not fired when split-moving.
Solution: defer the flattening of frames until win_split_ins begins
          reorganising them, and attempt to restore the layout by
          undoing our changes. (Sean Dewar)

https://github.com/vim/vim/commit/704966c2545897dfcf426dd9ef946aeb6fa80c38

Adjust winframe_restore to account for Nvim's horizontal separators when the
global statusline is in use. Add a test.

4 files changed, 152 insertions, 151 deletions

diff --git a/src/nvim/api/win_config.c b/src/nvim/api/win_config.c
index c308cadb7c..978d8515c8 100644
--- a/src/nvim/api/win_config.c
+++ b/src/nvim/api/win_config.c
@@ -260,7 +260,7 @@ Window nvim_open_win(Buffer buffer, Boolean enter, Dict(win_config) *config, Err
     int flags = win_split_flags(fconfig.split, parent == NULL) | WSP_NOENTER;
 
     if (parent == NULL) {
-      wp = win_split_ins(0, flags, NULL, 0);
+      wp = win_split_ins(0, flags, NULL, 0, NULL);
     } else {
       tp = win_find_tabpage(parent);
       switchwin_T switchwin;
@@ -268,7 +268,7 @@ Window nvim_open_win(Buffer buffer, Boolean enter, Dict(win_config) *config, Err
       const int result = switch_win(&switchwin, parent, tp, true);
       assert(result == OK);
       (void)result;
-      wp = win_split_ins(0, flags, NULL, 0);
+      wp = win_split_ins(0, flags, NULL, 0, NULL);
       restore_win(&switchwin, true);
     }
     if (wp) {
@@ -495,11 +495,11 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err)
           // If the frame doesn't have a parent, the old frame
           // was the root frame and we need to create a top-level split.
           int dir;
-          new_curwin = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp);
+          new_curwin = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, NULL);
         } else if (n_frames == 2) {
           // There are two windows in the frame, we can just rotate it.
           int dir;
-          neighbor = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp);
+          neighbor = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, NULL);
           new_curwin = neighbor;
         } else {
           // There is only one window in the frame, we can't split it.
@@ -511,7 +511,7 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err)
         parent = neighbor;
       } else {
         int dir;
-        new_curwin = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp);
+        new_curwin = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, NULL);
       }
       win_remove(win, win_tp == curtab ? NULL : win_tp);
       // move to neighboring window if we're moving the current window to a new tabpage
@@ -531,7 +531,7 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err)
     int flags = win_split_flags(fconfig.split, parent == NULL) | WSP_NOENTER;
 
     if (parent == NULL) {
-      if (!win_split_ins(0, flags, win, 0)) {
+      if (!win_split_ins(0, flags, win, 0, NULL)) {
         // TODO(willothy): What should this error message say?
         api_set_error(err, kErrorTypeException, "Failed to split window");
         return;
@@ -542,7 +542,7 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err)
       const int result = switch_win(&switchwin, parent, win_find_tabpage(parent), true);
       (void)result;
       assert(result == OK);
-      win_split_ins(0, flags, win, 0);
+      win_split_ins(0, flags, win, 0, NULL);
       restore_win(&switchwin, true);
     }
     if (HAS_KEY_X(config, width)) {
diff --git a/src/nvim/window.c b/src/nvim/window.c
index b1135d59fc..2d0010ad6c 100644
--- a/src/nvim/window.c
+++ b/src/nvim/window.c
@@ -981,16 +981,19 @@ int win_split(int size, int flags)
     clear_snapshot(curtab, SNAP_HELP_IDX);
   }
 
-  return win_split_ins(size, flags, NULL, 0) == NULL ? FAIL : OK;
+  return win_split_ins(size, flags, NULL, 0, NULL) == NULL ? FAIL : OK;
 }
 
 /// 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.
+/// When "to_flatten" is not NULL: flatten this frame before reorganising frames;
+/// remains unflattened on failure.
+///
 /// On failure, if "new_wp" was not NULL, no changes will have been made to the
 /// window layout or sizes.
 /// @return  NULL for failure, or pointer to new window
-win_T *win_split_ins(int size, int flags, win_T *new_wp, int dir)
+win_T *win_split_ins(int size, int flags, win_T *new_wp, int dir, frame_T *to_flatten)
 {
   win_T *wp = new_wp;
 
@@ -1261,6 +1264,11 @@ win_T *win_split_ins(int size, int flags, win_T *new_wp, int dir)
     CLEAR_FIELD(wp->w_border_adj);
   }
 
+  // Going to reorganize frames now, make sure they're flat.
+  if (to_flatten != NULL) {
+    frame_flatten(to_flatten);
+  }
+
   bool before;
   frame_T *curfrp;
 
@@ -1915,38 +1923,29 @@ int win_splitmove(win_T *wp, int size, int flags)
     return FAIL;
   }
 
-  frame_T *frp = NULL;
+  frame_T *unflat_altfr = NULL;
   if (wp->w_floating) {
     win_remove(wp, NULL);
   } else {
-    // Undoing changes to frames if splitting fails is complicated.
-    // Save a full snapshot to restore instead.
-    frp = make_full_snapshot();
-
-    // Remove the window and frame from the tree of frames.
-    winframe_remove(wp, &dir, NULL);
+    // Remove the window and frame from the tree of frames.  Don't flatten any
+    // frames yet so we can restore things if win_split_ins fails.
+    winframe_remove(wp, &dir, NULL, &unflat_altfr);
     win_remove(wp, NULL);
     last_status(false);  // may need to remove last status line
     win_comp_pos();  // recompute window positions
   }
 
   // Split a window on the desired side and put "wp" there.
-  if (win_split_ins(size, flags, wp, dir) == NULL) {
+  if (win_split_ins(size, flags, wp, dir, unflat_altfr) == NULL) {
     win_append(wp->w_prev, wp);
     if (!wp->w_floating) {
-      // Restore the previous layout from the snapshot.
-      xfree(wp->w_frame);
-      restore_full_snapshot(frp);
-
-      // Vertical separators to the left may have been lost.  Restore them.
-      frp = wp->w_frame;
-      if (frp->fr_parent->fr_layout == FR_ROW && frp->fr_prev != NULL) {
-        frame_add_vsep(frp->fr_prev);
-      }
+      // win_split_ins doesn't change sizes or layout if it fails to insert an
+      // existing window, so just undo winframe_remove.
+      winframe_restore(wp, dir, unflat_altfr);
+      win_comp_pos();  // recompute window positions
     }
     return FAIL;
   }
-  clear_snapshot_rec(frp);
 
   // If splitting horizontally, try to preserve height.
   // Note that win_split_ins autocommands may have immediately made "wp" floating!
@@ -3065,7 +3064,7 @@ static win_T *win_free_mem(win_T *win, int *dirp, tabpage_T *tp)
   if (!win->w_floating) {
     // Remove the window and its frame from the tree of frames.
     frame_T *frp = win->w_frame;
-    wp = winframe_remove(win, dirp, tp);
+    wp = winframe_remove(win, dirp, tp, NULL);
     xfree(frp);
   } else {
     *dirp = 'h';  // Dummy value.
@@ -3146,9 +3145,11 @@ void win_free_all(void)
 ///
 /// @param dirp  set to 'v' or 'h' for direction if 'ea'
 /// @param tp    tab page "win" is in, NULL for current
+/// @param unflat_altfr if not NULL, set to pointer of frame that got
+///                     the space, and it is not flattened
 ///
 /// @return      a pointer to the window that got the freed up space.
-win_T *winframe_remove(win_T *win, int *dirp, tabpage_T *tp)
+win_T *winframe_remove(win_T *win, int *dirp, tabpage_T *tp, frame_T **unflat_altfr)
   FUNC_ATTR_NONNULL_ARG(1, 2)
 {
   assert(tp == NULL || tp != curtab);
@@ -3236,56 +3237,110 @@ win_T *winframe_remove(win_T *win, int *dirp, tabpage_T *tp)
     frame_comp_pos(frp2, &row, &col);
   }
 
-  if (frp2->fr_next == NULL && frp2->fr_prev == NULL) {
-    // There is no other frame in this list, move its info to the parent
-    // and remove it.
-    frp2->fr_parent->fr_layout = frp2->fr_layout;
-    frp2->fr_parent->fr_child = frp2->fr_child;
-    frame_T *frp;
-    FOR_ALL_FRAMES(frp, frp2->fr_child) {
-      frp->fr_parent = frp2->fr_parent;
-    }
-    frp2->fr_parent->fr_win = frp2->fr_win;
-    if (frp2->fr_win != NULL) {
-      frp2->fr_win->w_frame = frp2->fr_parent;
+  if (unflat_altfr == NULL) {
+    frame_flatten(frp2);
+  } else {
+    *unflat_altfr = frp2;
+  }
+
+  return wp;
+}
+
+/// Flatten "frp" into its parent frame if it's the only child, also merging its
+/// list with the grandparent if they share the same layout.
+/// Frees "frp" if flattened; also "frp->fr_parent" if it has the same layout.
+/// "frp" must be valid in the current tabpage.
+static void frame_flatten(frame_T *frp)
+  FUNC_ATTR_NONNULL_ALL
+{
+  if (frp->fr_next != NULL || frp->fr_prev != NULL) {
+    return;
+  }
+
+  // There is no other frame in this list, move its info to the parent
+  // and remove it.
+  frp->fr_parent->fr_layout = frp->fr_layout;
+  frp->fr_parent->fr_child = frp->fr_child;
+  frame_T *frp2;
+  FOR_ALL_FRAMES(frp2, frp->fr_child) {
+    frp2->fr_parent = frp->fr_parent;
+  }
+  frp->fr_parent->fr_win = frp->fr_win;
+  if (frp->fr_win != NULL) {
+    frp->fr_win->w_frame = frp->fr_parent;
+  }
+  frp2 = frp->fr_parent;
+  if (topframe->fr_child == frp) {
+    topframe->fr_child = frp2;
+  }
+  xfree(frp);
+
+  frp = frp2->fr_parent;
+  if (frp != NULL && frp->fr_layout == frp2->fr_layout) {
+    // The frame above the parent has the same layout, have to merge
+    // the frames into this list.
+    if (frp->fr_child == frp2) {
+      frp->fr_child = frp2->fr_child;
+    }
+    assert(frp2->fr_child);
+    frp2->fr_child->fr_prev = frp2->fr_prev;
+    if (frp2->fr_prev != NULL) {
+      frp2->fr_prev->fr_next = frp2->fr_child;
+    }
+    for (frame_T *frp3 = frp2->fr_child;; frp3 = frp3->fr_next) {
+      frp3->fr_parent = frp;
+      if (frp3->fr_next == NULL) {
+        frp3->fr_next = frp2->fr_next;
+        if (frp2->fr_next != NULL) {
+          frp2->fr_next->fr_prev = frp3;
+        }
+        break;
+      }
     }
-    frp = frp2->fr_parent;
     if (topframe->fr_child == frp2) {
       topframe->fr_child = frp;
     }
     xfree(frp2);
+  }
+}
 
-    frp2 = frp->fr_parent;
-    if (frp2 != NULL && frp2->fr_layout == frp->fr_layout) {
-      // The frame above the parent has the same layout, have to merge
-      // the frames into this list.
-      if (frp2->fr_child == frp) {
-        frp2->fr_child = frp->fr_child;
-      }
-      assert(frp->fr_child);
-      frp->fr_child->fr_prev = frp->fr_prev;
-      if (frp->fr_prev != NULL) {
-        frp->fr_prev->fr_next = frp->fr_child;
-      }
-      frame_T *frp3;
-      for (frp3 = frp->fr_child;; frp3 = frp3->fr_next) {
-        frp3->fr_parent = frp2;
-        if (frp3->fr_next == NULL) {
-          frp3->fr_next = frp->fr_next;
-          if (frp->fr_next != NULL) {
-            frp->fr_next->fr_prev = frp3;
-          }
-          break;
-        }
-      }
-      if (topframe->fr_child == frp) {
-        topframe->fr_child = frp2;
-      }
-      xfree(frp);
+/// Undo changes from a prior call to winframe_remove, also restoring lost
+/// vertical separators and statuslines.
+/// Caller must ensure no other changes were made to the layout or window sizes!
+static void winframe_restore(win_T *wp, int dir, frame_T *unflat_altfr)
+  FUNC_ATTR_NONNULL_ALL
+{
+  frame_T *frp = wp->w_frame;
+
+  // Put "wp"'s frame back where it was.
+  if (frp->fr_prev != NULL) {
+    frame_append(frp->fr_prev, frp);
+  } else {
+    frame_insert(frp->fr_next, frp);
+  }
+
+  // Vertical separators to the left may have been lost.  Restore them.
+  if (wp->w_vsep_width == 0 && frp->fr_parent->fr_layout == FR_ROW && frp->fr_prev != NULL) {
+    frame_add_vsep(frp->fr_prev);
+  }
+
+  // Statuslines or horizontal separators above may have been lost.  Restore them.
+  if (frp->fr_parent->fr_layout == FR_COL && frp->fr_prev != NULL) {
+    if (global_stl_height() == 0 && wp->w_status_height == 0) {
+      frame_add_statusline(frp->fr_prev);
+    } else if (wp->w_hsep_height == 0) {
+      frame_add_hsep(frp->fr_prev);
     }
   }
 
-  return wp;
+  // Restore the lost room that was redistributed to the altframe.
+  if (dir == 'v') {
+    frame_new_height(unflat_altfr, unflat_altfr->fr_height - frp->fr_height,
+                     unflat_altfr == frp->fr_next, false);
+  } else if (dir == 'h') {
+    frame_new_width(unflat_altfr, unflat_altfr->fr_width - frp->fr_width,
+                    unflat_altfr == frp->fr_next, false);
+  }
 }
 
 /// If 'splitbelow' or 'splitright' is set, the space goes above or to the left
@@ -7248,23 +7303,23 @@ void reset_lnums(void)
 void make_snapshot(int idx)
 {
   clear_snapshot(curtab, idx);
-  make_snapshot_rec(topframe, &curtab->tp_snapshot[idx], false);
+  make_snapshot_rec(topframe, &curtab->tp_snapshot[idx]);
 }
 
-static void make_snapshot_rec(frame_T *fr, frame_T **frp, bool snap_wins)
+static void make_snapshot_rec(frame_T *fr, frame_T **frp)
 {
   *frp = xcalloc(1, sizeof(frame_T));
   (*frp)->fr_layout = fr->fr_layout;
   (*frp)->fr_width = fr->fr_width;
   (*frp)->fr_height = fr->fr_height;
   if (fr->fr_next != NULL) {
-    make_snapshot_rec(fr->fr_next, &((*frp)->fr_next), snap_wins);
+    make_snapshot_rec(fr->fr_next, &((*frp)->fr_next));
   }
   if (fr->fr_child != NULL) {
-    make_snapshot_rec(fr->fr_child, &((*frp)->fr_child), snap_wins);
+    make_snapshot_rec(fr->fr_child, &((*frp)->fr_child));
   }
-  if (fr->fr_layout == FR_LEAF && (snap_wins || fr->fr_win == curwin)) {
-    (*frp)->fr_win = fr->fr_win;
+  if (fr->fr_layout == FR_LEAF && fr->fr_win == curwin) {
+    (*frp)->fr_win = curwin;
   }
 }
 
@@ -7381,80 +7436,6 @@ static win_T *restore_snapshot_rec(frame_T *sn, frame_T *fr)
   return wp;
 }
 
-/// Return a snapshot of all frames in the current tabpage and which windows are
-/// in them.
-/// Use clear_snapshot_rec to free the snapshot.
-static frame_T *make_full_snapshot(void)
-{
-  frame_T *frp;
-  make_snapshot_rec(topframe, &frp, true);
-  return frp;
-}
-
-/// Restore all frames in the full snapshot "sn" for the current tabpage.
-/// Caller must ensure that the screen size didn't change, no windows with frames
-/// in the snapshot were freed, and windows with frames not in the snapshot are
-/// removed from their frames!
-/// Doesn't restore changed window vertical separators.
-/// Frees the old frames.  Don't call clear_snapshot_rec on "sn" afterwards!
-static void restore_full_snapshot(frame_T *sn)
-{
-  if (sn == NULL) {
-    return;
-  }
-
-  clear_snapshot_rec(topframe);
-  restore_full_snapshot_rec(sn);
-  curtab->tp_topframe = topframe = sn;
-  last_status(false);
-
-  // If the amount of space available changed, first try setting the sizes of
-  // windows with 'winfix{width,height}'. If that doesn't result in the right
-  // size, forget about that option.
-  if (topframe->fr_width != Columns) {
-    frame_new_width(topframe, Columns, false, true);
-    if (!frame_check_width(topframe, Columns)) {
-      frame_new_width(topframe, Columns, false, false);
-    }
-  }
-  if (topframe->fr_height != ROWS_AVAIL) {
-    frame_new_height(topframe, (int)ROWS_AVAIL, false, true);
-    if (!frame_check_height(topframe, (int)ROWS_AVAIL)) {
-      frame_new_height(topframe, (int)ROWS_AVAIL, false, false);
-    }
-  }
-
-  win_comp_pos();
-}
-
-static void restore_full_snapshot_rec(frame_T *sn)
-{
-  if (sn == NULL) {
-    return;
-  }
-
-  if (sn->fr_child != NULL) {
-    sn->fr_child->fr_parent = sn;
-  }
-  if (sn->fr_next != NULL) {
-    sn->fr_next->fr_parent = sn->fr_parent;
-    sn->fr_next->fr_prev = sn;
-  }
-  if (sn->fr_win != NULL) {
-    sn->fr_win->w_frame = sn;
-    // Assume for now that all windows have statuslines, so last_status in restore_full_snapshot
-    // doesn't resize frames to fit any missing statuslines.
-    sn->fr_win->w_status_height = STATUS_HEIGHT;
-    sn->fr_win->w_hsep_height = 0;
-
-    // Resize window to fit the frame.
-    frame_new_height(sn, sn->fr_height, false, false);
-    frame_new_width(sn, sn->fr_width, false, false);
-  }
-  restore_full_snapshot_rec(sn->fr_child);
-  restore_full_snapshot_rec(sn->fr_next);
-}
-
 /// Check that "topfrp" and its children are at the right height.
 ///
 /// @param  topfrp  top frame pointer
diff --git a/src/nvim/winfloat.c b/src/nvim/winfloat.c
index 8fe0315230..cddc4ed9dd 100644
--- a/src/nvim/winfloat.c
+++ b/src/nvim/winfloat.c
@@ -57,7 +57,7 @@ win_T *win_new_float(win_T *wp, bool last, WinConfig fconfig, Error *err)
       return NULL;
     }
     int dir;
-    winframe_remove(wp, &dir, NULL);
+    winframe_remove(wp, &dir, NULL, NULL);
     XFREE_CLEAR(wp->w_frame);
     win_comp_pos();  // recompute window positions
     win_remove(wp, NULL);
diff --git a/test/functional/ui/statusline_spec.lua b/test/functional/ui/statusline_spec.lua
index fee4b64d44..000e2b1b04 100644
--- a/test/functional/ui/statusline_spec.lua
+++ b/test/functional/ui/statusline_spec.lua
@@ -11,6 +11,7 @@ local exec = helpers.exec
 local exec_lua = helpers.exec_lua
 local eval = helpers.eval
 local sleep = vim.uv.sleep
+local pcall_err = helpers.pcall_err
 
 local mousemodels = { 'extend', 'popup', 'popup_setpos' }
 
@@ -474,6 +475,25 @@ describe('global statusline', function()
                                                                   |
     ]])
   end)
+
+  it('horizontal separators unchanged when failing to split-move window', function()
+    exec([[
+      botright split
+      let &winwidth = &columns
+      let &winminwidth = &columns
+    ]])
+    eq('Vim(wincmd):E36: Not enough room', pcall_err(command, 'wincmd L'))
+    command('mode')
+    screen:expect([[
+                                                                  |
+      {1:~                                                           }|*5
+      ────────────────────────────────────────────────────────────|
+      ^                                                            |
+      {1:~                                                           }|*6
+      {2:[No Name]                                 0,0-1          All}|
+                                                                  |
+    ]])
+  end)
 end)
 
 it('statusline does not crash if it has Arabic characters #19447', function()