vim-patch:9.1.2097: TabClosedPre may be triggered twice for the same tab page

Problem:  TabClosedPre may be triggered twice for the same tab page when
          closing another tab page in BufWinLeave (after 9.1.1211).
Solution: Store whether TabClosedPre was triggered in tabpage_T
          (zeertzjq).

Also fix the inconsistency that :tabclose! triggers TabClosedPre after
a failed :tabclose, but :close! doesn't even if there is only one window
in the tab page.

closes: vim/vim#19211

9168a04e0c

src/nvim/buffer_defs.h

@@ -841,9 +841,10 @@ struct tabpage_S {
   win_T *tp_firstwin;         ///< first window in this Tab page
   win_T *tp_lastwin;          ///< last window in this Tab page
   int64_t tp_old_Rows_avail;  ///< ROWS_AVAIL when Tab page was left
-  int64_t tp_old_Columns;        ///< Columns when Tab page was left, -1 when
-                                 ///< calling win_new_screen_cols() postponed
+  int64_t tp_old_Columns;     ///< Columns when Tab page was left, -1 when
+                              ///< calling win_new_screen_cols() postponed
   OptInt tp_ch_used;          ///< value of 'cmdheight' when frame size was set
+  bool tp_did_tabclosedpre;   ///< whether TabClosedPre was triggered
 
   diff_T *tp_first_diff;
   buf_T *(tp_diffbuf[DB_COUNT]);

src/nvim/ex_docmd.c

@@ -5150,7 +5150,9 @@ void tabpage_close(int forceit)
     return;
   }
 
-  trigger_tabclosedpre(curtab, true);
+  trigger_tabclosedpre(curtab);
+  curtab->tp_did_tabclosedpre = true;
+  tabpage_T *const save_curtab = curtab;
 
   // First close all the windows but the current one.  If that worked then
   // close the last window in this tab, that will close it.
@@ -5163,6 +5165,11 @@ void tabpage_close(int forceit)
   if (ONE_WINDOW) {
     ex_win_close(forceit, curwin, NULL);
   }
+  if (curtab == save_curtab) {
+    // When closing the tab page failed, reset tp_did_tabclosedpre so that
+    // TabClosedPre behaves consistently on next :close vs :tabclose.
+    curtab->tp_did_tabclosedpre = false;
+  }
 }
 
 /// Close tab page "tp", which is not the current tab page.
@@ -5178,7 +5185,8 @@ void tabpage_close_other(tabpage_T *tp, int forceit)
     return;
   }
 
-  trigger_tabclosedpre(tp, true);
+  trigger_tabclosedpre(tp);
+  tp->tp_did_tabclosedpre = true;
 
   // Limit to 1000 windows, autocommands may add a window while we close
   // one.  OK, so I'm paranoid...
@@ -5187,11 +5195,21 @@ void tabpage_close_other(tabpage_T *tp, int forceit)
     win_T *wp = tp->tp_lastwin;
     ex_win_close(forceit, wp, tp);
 
-    // Autocommands may delete the tab page under our fingers and we may
-    // fail to close a window with a modified buffer.
-    if (!valid_tabpage(tp) || tp->tp_lastwin == wp) {
+    // Autocommands may delete the tab page under our fingers.
+    if (!valid_tabpage(tp)) {
       break;
     }
+    // We may fail to close a window with a modified buffer.
+    if (tp->tp_lastwin == wp) {
+      done = 1000;
+      break;
+    }
+  }
+  if (done >= 1000) {
+    // When closing the tab page failed, reset tp_did_tabclosedpre so that
+    // TabClosedPre behaves consistently on next :close vs :tabclose.
+    tp->tp_did_tabclosedpre = false;
+    return;
   }
 }
 

src/nvim/window.c

@@ -3110,12 +3110,9 @@ static void do_autocmd_winclosed(win_T *win)
   recursive = false;
 }
 
-/// directly is true if the window is closed by ':tabclose' or ':tabonly'.
-/// This allows saving the session before closing multi-window tab.
-void trigger_tabclosedpre(tabpage_T *tp, bool directly)
+void trigger_tabclosedpre(tabpage_T *tp)
 {
   static bool recursive = false;
-  static bool skip = false;
   tabpage_T *ptp = curtab;
 
   // Quickly return when no TabClosedPre autocommands to be executed or
@@ -3124,17 +3121,8 @@ void trigger_tabclosedpre(tabpage_T *tp, bool directly)
     return;
   }
 
-  // Skip if the event have been triggered by ':tabclose' recently
-  if (skip) {
-    skip = false;
-    return;
-  }
-
   if (valid_tabpage(tp)) {
     goto_tabpage_tp(tp, false, false);
-    if (directly) {
-      skip = true;
-    }
   }
   recursive = true;
   window_layout_lock();
@@ -3143,10 +3131,10 @@ void trigger_tabclosedpre(tabpage_T *tp, bool directly)
   recursive = false;
   // tabpage may have been modified or deleted by autocmds
   if (valid_tabpage(ptp)) {
-    // try to recover the tappage first
+    // try to recover the tabpage first
     goto_tabpage_tp(ptp, false, false);
   } else {
-    // fall back to the first tappage
+    // fall back to the first tabpage
     goto_tabpage_tp(first_tabpage, false, false);
   }
 }
@@ -3211,8 +3199,8 @@ bool win_close_othertab(win_T *win, int free_buf, tabpage_T *tp, bool force)
     }
   }
 
-  if (tp->tp_firstwin == tp->tp_lastwin) {
-    trigger_tabclosedpre(tp, false);
+  if (tp->tp_firstwin == tp->tp_lastwin && !tp->tp_did_tabclosedpre) {
+    trigger_tabclosedpre(tp);
     // autocmd may have freed the window already.
     if (!win_valid_any_tab(win)) {
       return false;

test/old/testdir/test_autocmd.vim

@@ -4704,6 +4704,64 @@ func Test_autocmd_TabClosedPre()
   call assert_equal([1, 2], g:tabpagenr_pre)
   call assert_equal([2, 3], g:tabpagenr_post)
 
+  " Test failing to close tab page
+  let g:tabpagenr_pre = []
+  let g:tabpagenr_post = []
+  let t:testvar = 1
+  call setline(1, 'foo')
+  setlocal bufhidden=wipe
+  tabnew
+  let t:testvar = 2
+  tabnew
+  let t:testvar = 3
+  call setline(1, 'bar')
+  setlocal bufhidden=wipe
+  tabnew
+  let t:testvar = 4
+  call setline(1, 'baz')
+  setlocal bufhidden=wipe
+  new
+  call assert_fails('tabclose', 'E445:')
+  call assert_equal([4], g:tabpagenr_pre)
+  call assert_equal([], g:tabpagenr_post)
+  " :tabclose! after failed :tabclose should trigger TabClosedPre again.
+  tabclose!
+  call assert_equal([4, 4], g:tabpagenr_pre)
+  call assert_equal([3], g:tabpagenr_post)
+  call assert_fails('tabclose', 'E37:')
+  call assert_equal([4, 4, 3], g:tabpagenr_pre)
+  call assert_equal([3], g:tabpagenr_post)
+  " The same for :close! if the tab page only has one window.
+  close!
+  call assert_equal([4, 4, 3, 3], g:tabpagenr_pre)
+  call assert_equal([3, 2], g:tabpagenr_post)
+  " Also test with :close! after failed :tabonly.
+  call assert_fails('tabonly', 'E37:')
+  call assert_equal([4, 4, 3, 3, 1], g:tabpagenr_pre)
+  call assert_equal([3, 2], g:tabpagenr_post)
+  tabprevious | close!
+  call assert_equal([4, 4, 3, 3, 1, 1], g:tabpagenr_pre)
+  call assert_equal([3, 2, 2], g:tabpagenr_post)
+  %bwipe!
+
+  " Test closing another tab page in BufWinLeave
+  let g:tabpagenr_pre = []
+  let g:tabpagenr_post = []
+  split
+  let t:testvar = 1
+  tabnew
+  let t:testvar = 2
+  tabnew Xsomebuf
+  let t:testvar = 3
+  new
+  autocmd BufWinLeave Xsomebuf ++once ++nested tabclose 1
+  tabclose
+  " TabClosedPre should not be triggered for tab page 3 twice.
+  call assert_equal([3, 1], g:tabpagenr_pre)
+  " When tab page 1 was closed, tab page 3 was still the current tab page.
+  call assert_equal([3, 2], g:tabpagenr_post)
+  %bwipe!
+
   func ClearAutocmdAndCreateTabs()
     au! TabClosedPre
     bw!