fix(terminal): improve cursor refresh handling (#32596)

Problem: terminal mode cursor refresh logic has too many edge cases where it
fails when events change curbuf.

Solution: change the logic. Introduce cursor_visible to TerminalState to more
reliably track if terminal mode has changed busy. Move visibility handling to
refresh_cursor and move its call in refresh_terminal to terminal_check to avoid
temporarily changed curbufs from influencing cursor state.

This has the effect of "debouncing" shape/visibility updates to once per
terminal state tick (with the final attributes taking effect, as expected). I
think this is OK, but as a result it may also be warranted to update when
redrawing during the same state tick (e.g: from events executing :redraw); this
can be added later, if wanted.

Also move previous tests to a more appropriate place.

src/nvim/terminal.c

@@ -106,10 +106,11 @@
 typedef struct {
   VimState state;
   Terminal *term;
-  int save_rd;              // saved value of RedrawingDisabled
+  int save_rd;          ///< saved value of RedrawingDisabled
   bool close;
-  bool got_bsl;             // if the last input was <C-\>
+  bool got_bsl;         ///< if the last input was <C-\>
-  bool got_bsl_o;           // if left terminal mode with <c-\><c-o>
+  bool got_bsl_o;       ///< if left terminal mode with <c-\><c-o>
+  bool cursor_visible;  ///< cursor's current visibility; ensures matched busy_start/stop UI events
 } TerminalState;
 
 #ifdef INCLUDE_GENERATED_DECLARATIONS
@@ -673,6 +674,7 @@ bool terminal_enter(void)
   assert(buf->terminal);  // Should only be called when curbuf has a terminal.
   TerminalState s[1] = { 0 };
   s->term = buf->terminal;
+  s->cursor_visible = true;  // Assume visible; may change via refresh_cursor later.
   stop_insert_mode = false;
 
   // Ensure the terminal is properly sized. Ideally window size management
@@ -685,10 +687,6 @@ bool terminal_enter(void)
   State = MODE_TERMINAL;
   mapped_ctrl_c |= MODE_TERMINAL;  // Always map CTRL-C to avoid interrupt.
   RedrawingDisabled = false;
-  if (!s->term->cursor.visible) {
-    // Hide cursor if it should be hidden. Do so right after setting State, before events.
-    ui_busy_start();
-  }
 
   // Disable these options in terminal-mode. They are nonsense because cursor is
   // placed at end of buffer to "follow" output. #11072
@@ -715,10 +713,7 @@ bool terminal_enter(void)
   curwin->w_p_so = 0;
   curwin->w_p_siso = 0;
 
-  // Update the cursor shape table and flush changes to the UI
+  s->term->pending.cursor = true;  // Update the cursor shape table
-  s->term->pending.cursor = true;
-  refresh_cursor(s->term);
-
   adjust_topline(s->term, buf, 0);  // scroll to end
   showmode();
   curwin->w_redr_status = true;  // For mode() in statusline. #8323
@@ -739,8 +734,8 @@ bool terminal_enter(void)
   }
   State = save_state;
   RedrawingDisabled = s->save_rd;
-  if (!s->term->cursor.visible) {
+  if (!s->cursor_visible) {
-    // If cursor was hidden, show it again. Do so right after restoring State, before events.
+    // If cursor was hidden, show it again. Do so right after restoring State.
     ui_busy_stop();
   }
 
@@ -805,10 +800,13 @@ static void terminal_check_cursor(void)
 //   0 if the main loop must exit
 static int terminal_check(VimState *state)
 {
+  TerminalState *const s = (TerminalState *)state;
+
   if (stop_insert_mode) {
     return 0;
   }
 
+  assert(s->term == curbuf->terminal);
   terminal_check_cursor();
   validate_cursor(curwin);
 
@@ -835,6 +833,7 @@ static int terminal_check(VimState *state)
   }
 
   setcursor();
+  refresh_cursor(s->term, &s->cursor_visible);
   ui_flush();
   return 1;
 }
@@ -937,23 +936,9 @@ static int terminal_execute(VimState *state, int key)
   }
   if (s->term != curbuf->terminal) {
     // Active terminal buffer changed, flush terminal's cursor state to the UI
-    curbuf->terminal->pending.cursor = true;
-
-    if (!s->term->cursor.visible) {
-      // If cursor was hidden, show it again
-      ui_busy_stop();
-    }
-
-    if (!curbuf->terminal->cursor.visible) {
-      // Hide cursor if it should be hidden
-      ui_busy_start();
-    }
-
-    invalidate_terminal(s->term, s->term->cursor.row, s->term->cursor.row + 1);
-    invalidate_terminal(curbuf->terminal,
-                        curbuf->terminal->cursor.row,
-                        curbuf->terminal->cursor.row + 1);
     s->term = curbuf->terminal;
+    s->term->pending.cursor = true;
+    invalidate_terminal(s->term, -1, -1);
   }
   return 1;
 }
@@ -1291,17 +1276,8 @@ static int term_settermprop(VTermProp prop, VTermValue *val, void *data)
     break;
 
   case VTERM_PROP_CURSORVISIBLE:
-    if (is_focused(term)) {
-      if (!val->boolean && term->cursor.visible) {
-        // Hide the cursor
-        ui_busy_start();
-      } else if (val->boolean && !term->cursor.visible) {
-        // Unhide the cursor
-        ui_busy_stop();
-      }
-      invalidate_terminal(term, -1, -1);
-    }
     term->cursor.visible = val->boolean;
+    invalidate_terminal(term, -1, -1);
     break;
 
   case VTERM_PROP_TITLE: {
@@ -2042,23 +2018,35 @@ static void refresh_terminal(Terminal *term)
   }
   linenr_T ml_before = buf->b_ml.ml_line_count;
 
-  // refresh_ functions assume the terminal buffer is current
+  // Some refresh_ functions assume the terminal buffer is current. Don't call refresh_cursor here,
+  // as we don't want a terminal that was possibly made temporarily current influencing the cursor.
   aco_save_T aco;
   aucmd_prepbuf(&aco, buf);
   refresh_size(term, buf);
   refresh_scrollback(term, buf);
   refresh_screen(term, buf);
-  refresh_cursor(term);
   aucmd_restbuf(&aco);
 
   int ml_added = buf->b_ml.ml_line_count - ml_before;
   adjust_topline(term, buf, ml_added);
 }
 
-static void refresh_cursor(Terminal *term)
+static void refresh_cursor(Terminal *term, bool *cursor_visible)
   FUNC_ATTR_NONNULL_ALL
 {
-  if (!is_focused(term) || !term->pending.cursor) {
+  if (!is_focused(term)) {
+    return;
+  }
+  if (term->cursor.visible != *cursor_visible) {
+    *cursor_visible = term->cursor.visible;
+    if (*cursor_visible) {
+      ui_busy_stop();
+    } else {
+      ui_busy_start();
+    }
+  }
+
+  if (!term->pending.cursor) {
     return;
   }
   term->pending.cursor = false;
@@ -2166,6 +2154,7 @@ static void adjust_scrollback(Terminal *term, buf_T *buf)
 // Refresh the scrollback of an invalidated terminal.
 static void refresh_scrollback(Terminal *term, buf_T *buf)
 {
+  assert(buf == curbuf);  // TODO(seandewar): remove this condition
   int width, height;
   vterm_get_size(term->vt, &height, &width);
 

test/functional/terminal/cursor_spec.lua

@@ -149,6 +149,73 @@ describe(':terminal cursor', function()
                                                           |*5
         {3:-- TERMINAL --}                                    |
       ]])
+
+      -- Cursor is hidden; now request to show it while in a TermLeave autocmd.
+      -- Process events (via :sleep) to handle the escape sequence now.
+      command([[autocmd TermLeave * ++once call chansend(&channel, "\e[?25h") | sleep 1m]])
+      feed([[<C-\><C-N>]]) -- Exit terminal mode; cursor should not remain hidden
+      screen:expect([[
+        tty ready                                         |
+        ^                                                  |
+                                                          |*5
+      ]])
+
+      command('bwipeout! | let chan = nvim_open_term(0, {})')
+      feed('i')
+      -- Hide the cursor, switch to a non-terminal buffer, then show the cursor; it shouldn't remain
+      -- hidden after we're kicked out of terminal mode in the new buffer.
+      -- Must ensure these actions happen within the same terminal_execute call. The stream is
+      -- internal, so polling the event loop isn't necessary (terminal_receive is directly called).
+      command([[call chansend(chan, "\e[?25l") | new floob | call chansend(chan, "\e[?25h")]])
+      screen:expect([[
+        ^                                                  |
+        {4:~                                                 }|
+        {5:floob                                             }|
+                                                          |*2
+        {18:[Scratch]                                         }|
+                                                          |
+      ]])
+
+      feed('<C-W>pi')
+      screen:expect([[
+                                                          |
+        {4:~                                                 }|
+        {1:floob                                             }|
+        ^                                                  |
+                                                          |
+        {17:[Scratch]                                         }|
+        {3:-- TERMINAL --}                                    |
+      ]])
+    end)
+
+    it('becomes visible on TermLeave if hidden immediately by events #32456', function()
+      skip(is_os('win'), '#31587')
+      -- Reproducing the issue is quite fragile; it's easiest done in a lone test case like this
+      -- with no prior commands.
+      feed([[<C-\><C-N>]])
+      screen:expect([[
+        tty ready                                         |
+        ^                                                  |
+                                                          |*5
+      ]])
+
+      -- Hide the cursor such that the escape sequence is processed as a side effect of showmode in
+      -- terminal_enter handling events (skip_showmode -> char_avail -> vpeekc -> os_breakcheck).
+      -- This requires a particular set of actions; :startinsert repros better than feed('i') here.
+      hide_cursor()
+      command('mode | startinsert')
+      screen:expect([[
+        tty ready                                         |
+                                                          |*5
+        {3:-- TERMINAL --}                                    |
+      ]])
+
+      feed([[<C-\><C-N>]])
+      screen:expect([[
+        tty ready                                         |
+        ^                                                  |
+                                                          |*5
+      ]])
     end)
   end)
 
@@ -358,55 +425,50 @@ describe(':terminal cursor', function()
     eq(error_hl_id, screen._mode_info[terminal_mode_idx].hl_id)
   end)
 
-  it('restores visibility on TermLeave #32456', function()
+  it('uses the correct attributes', function()
-    skip(is_os('win'), '#31587')
+    feed([[<C-\><C-N>]])
-    feed([[<C-\><C-N>]]) -- Exit terminal mode
+    command([[
-    screen:expect([[
+      bwipeout!
-      tty ready                                         |
+      let chan1 = nvim_open_term(0, {})
-      ^                                                  |
+      vnew
-                                                        |*5
+      let chan2 = nvim_open_term(0, {})
     ]])
-
-    tt.hide_cursor()
-    -- :startinsert repros the issue more reliably than feed('i')
-    command('mode | startinsert')
-    screen:expect([[
-      tty ready                                         |
-                                                        |*5
-      {3:-- TERMINAL --}                                    |
-    ]])
-
-    feed([[<C-\><C-N>]]) -- Exit terminal mode
-    screen:expect([[
-      tty ready                                         |
-      ^                                                  |
-                                                        |*5
-    ]])
-
     feed('i')
     screen:expect([[
-      tty ready                                         |
+      ^                         │                        |
-                                                        |*5
+                               │                        |*4
+      {17:[Scratch]                 }{18:[Scratch]               }|
       {3:-- TERMINAL --}                                    |
     ]])
+    eq('block', screen._mode_info[terminal_mode_idx].cursor_shape)
+    eq(500, screen._mode_info[terminal_mode_idx].blinkon)
+    eq(500, screen._mode_info[terminal_mode_idx].blinkoff)
 
-    -- Cursor currently hidden; request to show it while in a TermLeave autocmd.
+    -- Modify cursor in the non-current terminal; should not affect this cursor.
-    -- Process events (via :sleep) to handle the escape sequence immediately.
+    command([[call chansend(chan1, "\e[4 q")]])
-    command([[autocmd TermLeave * ++once call chansend(b:terminal_job_id, "\e[?25h") | sleep 1m]])
+    screen:expect_unchanged()
-    feed([[<C-\><C-N>]]) -- Exit terminal mode
+    eq('block', screen._mode_info[terminal_mode_idx].cursor_shape)
-    screen:expect([[
+    eq(500, screen._mode_info[terminal_mode_idx].blinkon)
-      tty ready                                         |
+    eq(500, screen._mode_info[terminal_mode_idx].blinkoff)
-      ^                                                  |
-                                                        |*5
-    ]])
 
-    feed('i')
+    -- Modify cursor in the current terminal.
+    command([[call chansend(chan2, "\e[6 q")]])
+    screen:expect_unchanged()
+    eq('vertical', screen._mode_info[terminal_mode_idx].cursor_shape)
+    eq(0, screen._mode_info[terminal_mode_idx].blinkon)
+    eq(0, screen._mode_info[terminal_mode_idx].blinkoff)
+
+    -- Check the cursor in the other terminal reflects our changes from before.
+    command('wincmd p')
     screen:expect([[
-      tty ready                                         |
+                               │^                        |
-      ^                                                  |
+                               │                        |*4
-                                                        |*4
+      {18:[Scratch]                 }{17:[Scratch]               }|
       {3:-- TERMINAL --}                                    |
     ]])
+    eq('horizontal', screen._mode_info[terminal_mode_idx].cursor_shape)
+    eq(0, screen._mode_info[terminal_mode_idx].blinkon)
+    eq(0, screen._mode_info[terminal_mode_idx].blinkoff)
   end)
 end)