vim-patch:9.1.0557: moving in the buffer list doesn't work as documented (#29653)

Problem:  moving in the buffer list doesn't work as documented
          (SenileFelineS)
Solution: Skip non-help buffers, when run from normal buffers, else
          only move from help buffers to the next help buffer (LemonBoy)

As explained in the help section for :bnext and :bprev the commands
should jump from help buffers to help buffers (and from regular ones to
regular ones).

fixes: vim/vim#4478
closes: vim/vim#15198

893eeeb445

Co-authored-by: LemonBoy <thatlemon@gmail.com>

runtime/doc/news.txt

@@ -49,6 +49,9 @@ EDITOR
   now appear left of lower priority signs.
 • |hl-CurSearch| now behaves the same as Vim and no longer updates on every
   cursor movement.
+• Moving in the buffer list using |:bnext| and similar commands behaves as
+  documented and skips help buffers if run from a non-help buffer, otherwise
+  it moves to another help buffer.
 
 VIM SCRIPT
 

src/nvim/buffer.c

@@ -942,6 +942,21 @@ static void free_buffer_stuff(buf_T *buf, int free_flags)
 void goto_buffer(exarg_T *eap, int start, int dir, int count)
 {
   const int save_sea = swap_exists_action;
+  bool skip_help_buf;
+
+  switch (eap->cmdidx) {
+  case CMD_bnext:
+  case CMD_sbnext:
+  case CMD_bNext:
+  case CMD_bprevious:
+  case CMD_sbNext:
+  case CMD_sbprevious:
+    skip_help_buf = true;
+    break;
+  default:
+    skip_help_buf = false;
+    break;
+  }
 
   bufref_T old_curbuf;
   set_bufref(&old_curbuf, curbuf);
@@ -949,8 +964,9 @@ void goto_buffer(exarg_T *eap, int start, int dir, int count)
   if (swap_exists_action == SEA_NONE) {
     swap_exists_action = SEA_DIALOG;
   }
-  do_buffer(*eap->cmd == 's' ? DOBUF_SPLIT : DOBUF_GOTO,
+  (void)do_buffer_ext(*eap->cmd == 's' ? DOBUF_SPLIT : DOBUF_GOTO, start, dir, count,
-            start, dir, count, eap->forceit);
+                      (eap->forceit ? DOBUF_FORCEIT : 0) |
+                      (skip_help_buf ? DOBUF_SKIPHELP : 0));
 
   if (swap_exists_action == SEA_QUIT && *eap->cmd == 's') {
     cleanup_T cs;
@@ -1202,10 +1218,10 @@ static int empty_curbuf(bool close_others, int forceit, int action)
 ///
 /// @param dir  FORWARD or BACKWARD
 /// @param count  buffer number or number of buffers
-/// @param forceit  true for :...!
+/// @param flags  see @ref dobuf_flags_value
 ///
 /// @return  FAIL or OK.
-int do_buffer(int action, int start, int dir, int count, int forceit)
+static int do_buffer_ext(int action, int start, int dir, int count, int flags)
 {
   buf_T *buf;
   buf_T *bp;
@@ -1257,8 +1273,12 @@ int do_buffer(int action, int start, int dir, int count, int forceit)
           buf = lastbuf;
         }
       }
-      // don't count unlisted buffers
+      // Don't count unlisted buffers.
-      if (unload || buf->b_p_bl) {
+      // Avoid non-help buffers if the starting point was a non-help buffer and
+      // vice-versa.
+      if (unload
+          || (buf->b_p_bl
+              && ((flags & DOBUF_SKIPHELP) == 0 || buf->b_help == bp->b_help))) {
         count--;
         bp = NULL;              // use this buffer as new starting point
       }
@@ -1284,7 +1304,9 @@ int do_buffer(int action, int start, int dir, int count, int forceit)
     return FAIL;
   }
 
-  if (action == DOBUF_GOTO && buf != curbuf && !check_can_set_curbuf_forceit(forceit)) {
+  if (action == DOBUF_GOTO
+      && buf != curbuf
+      && !check_can_set_curbuf_forceit((flags & DOBUF_FORCEIT) ? true : false)) {
     // disallow navigating to another buffer when 'winfixbuf' is applied
     return FAIL;
   }
@@ -1310,7 +1332,7 @@ int do_buffer(int action, int start, int dir, int count, int forceit)
       return FAIL;
     }
 
-    if (!forceit && bufIsChanged(buf)) {
+    if ((flags & DOBUF_FORCEIT) == 0 && bufIsChanged(buf)) {
       if ((p_confirm || (cmdmod.cmod_flags & CMOD_CONFIRM)) && p_write) {
         dialog_changed(buf, false);
         if (!bufref_valid(&bufref)) {
@@ -1330,7 +1352,7 @@ int do_buffer(int action, int start, int dir, int count, int forceit)
       }
     }
 
-    if (!forceit && buf->terminal && terminal_running(buf->terminal)) {
+    if (!(flags & DOBUF_FORCEIT) && buf->terminal && terminal_running(buf->terminal)) {
       if (p_confirm || (cmdmod.cmod_flags & CMOD_CONFIRM)) {
         if (!dialog_close_terminal(buf)) {
           return FAIL;
@@ -1358,7 +1380,7 @@ int do_buffer(int action, int start, int dir, int count, int forceit)
       }
     }
     if (bp == NULL && buf == curbuf) {
-      return empty_curbuf(true, forceit, action);
+      return empty_curbuf(true, (flags & DOBUF_FORCEIT), action);
     }
 
     // If the deleted buffer is the current one, close the current window
@@ -1516,7 +1538,7 @@ int do_buffer(int action, int start, int dir, int count, int forceit)
   if (buf == NULL) {
     // Autocommands must have wiped out all other buffers.  Only option
     // now is to make the current buffer empty.
-    return empty_curbuf(false, forceit, action);
+    return empty_curbuf(false, (flags & DOBUF_FORCEIT), action);
   }
 
   // make "buf" the current buffer
@@ -1537,7 +1559,7 @@ int do_buffer(int action, int start, int dir, int count, int forceit)
   }
 
   // Check if the current buffer may be abandoned.
-  if (action == DOBUF_GOTO && !can_abandon(curbuf, forceit)) {
+  if (action == DOBUF_GOTO && !can_abandon(curbuf, (flags & DOBUF_FORCEIT))) {
     if ((p_confirm || (cmdmod.cmod_flags & CMOD_CONFIRM)) && p_write) {
       bufref_T bufref;
       set_bufref(&bufref, buf);
@@ -1567,6 +1589,11 @@ int do_buffer(int action, int start, int dir, int count, int forceit)
   return OK;
 }
 
+int do_buffer(int action, int start, int dir, int count, int forceit)
+{
+  return do_buffer_ext(action, start, dir, count, forceit ? DOBUF_FORCEIT : 0);
+}
+
 /// Set current buffer to "buf".  Executes autocommands and closes current
 /// buffer.
 ///

src/nvim/buffer.h

@@ -39,7 +39,7 @@ enum bln_values {
   BLN_NOCURWIN = 128,  ///< buffer is not associated with curwin
 };
 
-/// Values for action argument for do_buffer()
+/// Values for action argument for do_buffer_ext() and close_buffer()
 enum dobuf_action_values {
   DOBUF_GOTO   = 0,  ///< go to specified buffer
   DOBUF_SPLIT  = 1,  ///< split window and go to specified buffer
@@ -48,7 +48,7 @@ enum dobuf_action_values {
   DOBUF_WIPE   = 4,  ///< delete specified buffer(s) really
 };
 
-/// Values for start argument for do_buffer()
+/// Values for start argument for do_buffer_ext()
 enum dobuf_start_values {
   DOBUF_CURRENT = 0,  ///< "count" buffer from current buffer
   DOBUF_FIRST   = 1,  ///< "count" buffer from first buffer
@@ -56,6 +56,13 @@ enum dobuf_start_values {
   DOBUF_MOD     = 3,  ///< "count" mod. buffer from current buffer
 };
 
+/// Values for flags argument of do_buffer_ext()
+enum dobuf_flags_value {
+  DOBUF_FORCEIT  = 1,  ///< :cmd!
+  DOBUF_SKIPHELP = 4,  ///< skip or keep help buffers depending on b_help of the
+                       ///< starting buffer
+};
+
 /// flags for buf_freeall()
 enum bfa_values {
   BFA_DEL          = 1,  ///< buffer is going to be deleted

test/old/testdir/test_buffer.vim

@@ -126,6 +126,52 @@ func Test_buflist_browse()
   %bwipe!
 endfunc
 
+" Test for :bnext and :bprev when called from help and non-help buffers.
+func Test_bnext_bprev_help()
+  %bwipe!
+
+  e XHelp1 | set bt=help
+  let b1 = bufnr()
+  e Xbuf1
+  let b2 = bufnr()
+
+  " There's only one buffer of each type.
+  b XHelp1
+  bnext | call assert_equal(b1, bufnr())
+  bprev | call assert_equal(b1, bufnr())
+  b Xbuf1
+  bnext | call assert_equal(b2, bufnr())
+  bprev | call assert_equal(b2, bufnr())
+
+  " Add one more buffer of each type.
+  e XHelp2 | set bt=help
+  let b3 = bufnr()
+  e Xbuf2
+  let b4 = bufnr()
+
+  " Help buffer jumps to help buffer.
+  b XHelp1
+  bnext | call assert_equal(b3, bufnr())
+  bnext | call assert_equal(b1, bufnr())
+  bprev | call assert_equal(b3, bufnr())
+  bprev | call assert_equal(b1, bufnr())
+
+  " Regular buffer jumps to regular buffer.
+  b Xbuf1
+  bnext | call assert_equal(b4, bufnr())
+  bnext | call assert_equal(b2, bufnr())
+  bprev | call assert_equal(b4, bufnr())
+  bprev | call assert_equal(b2, bufnr())
+
+  " :brewind and :blast are not affected by the buffer type.
+  b Xbuf2
+  brewind | call assert_equal(b1, bufnr())
+  b XHelp1
+  blast   | call assert_equal(b4, bufnr())
+
+  %bwipe!
+endfunc
+
 " Test for :bdelete
 func Test_bdelete_cmd()
   %bwipe!