Merge pull request #8790 from bfredl/hlattr

pass highlight attrs per value and thread-safely to TUI thread

10 files changed, 148 insertions, 92 deletions

diff --git a/runtime/doc/ui.txt b/runtime/doc/ui.txt
index 9d10756e23..51af11a2cd 100644
--- a/runtime/doc/ui.txt
+++ b/runtime/doc/ui.txt
@@ -89,13 +89,18 @@ Global Events							    *ui-global*
 	`cursor_shape`:	"block", "horizontal", "vertical"
 	`cell_percentage`: Cell % occupied by the cursor.
 	`blinkwait`, `blinkon`, `blinkoff`: See |cursor-blinking|.
-	`hl_id`:	Cursor highlight group.
-	`hl_lm`:	Cursor highlight group if 'langmap' is active.
+	`attr_id`:	Cursor attribute id (defined by `hl_attr_define`)
+	`attr_id_lm`:	Cursor attribute id for when 'langmap' is active.
 	`short_name`:	Mode code name, see 'guicursor'.
 	`name`:		Mode descriptive name.
 	`mouse_shape`:	(To be implemented.)
 
 	Some keys are missing in some modes.
+	
+	The following keys are deprecated:
+
+	`hl_id`:	Use `attr_id` instead.
+	`hl_lm`:	Use `attr_id_lm` instead.
 
 ["option_set", name, value]
 	UI-related option changed, where `name` is one of:
diff --git a/src/nvim/api/ui.c b/src/nvim/api/ui.c
index d0db43c588..37d34c5843 100644
--- a/src/nvim/api/ui.c
+++ b/src/nvim/api/ui.c
@@ -351,12 +351,8 @@ static void remote_ui_hl_attr_define(UI *ui, Integer id, HlAttrs rgb_attrs,
   Array args = ARRAY_DICT_INIT;
 
   ADD(args, INTEGER_OBJ(id));
-
-  Dictionary rgb_hl = hlattrs2dict(&rgb_attrs, true);
-  ADD(args, DICTIONARY_OBJ(rgb_hl));
-
-  Dictionary cterm_hl = hlattrs2dict(&cterm_attrs, false);
-  ADD(args, DICTIONARY_OBJ(cterm_hl));
+  ADD(args, DICTIONARY_OBJ(hlattrs2dict(rgb_attrs, true)));
+  ADD(args, DICTIONARY_OBJ(hlattrs2dict(cterm_attrs, false)));
 
   if (ui->ui_ext[kUIHlState]) {
     ADD(args, ARRAY_OBJ(copy_array(info)));
@@ -372,21 +368,12 @@ static void remote_ui_highlight_set(UI *ui, int id)
   Array args = ARRAY_DICT_INIT;
   UIData *data = ui->data;
 
-  HlAttrs attrs = HLATTRS_INIT;
 
   if (data->hl_id == id) {
     return;
   }
   data->hl_id = id;
-
-  if (id != 0) {
-    HlAttrs *aep = syn_attr2entry(id);
-    if (aep) {
-      attrs = *aep;
-    }
-  }
-
-  Dictionary hl = hlattrs2dict(&attrs, ui->rgb);
+  Dictionary hl = hlattrs2dict(syn_attr2entry(id), ui->rgb);
 
   ADD(args, DICTIONARY_OBJ(hl));
   push_call(ui, "highlight_set", args);
@@ -524,8 +511,7 @@ static void remote_ui_cmdline_show(UI *ui, Array args)
     Array new_item = ARRAY_DICT_INIT;
     int attr = (int)item.items[0].data.integer;
     if (attr) {
-      HlAttrs *aep = syn_attr2entry(attr);
-      Dictionary rgb_attrs = hlattrs2dict(aep, ui->rgb ? kTrue : kFalse);
+      Dictionary rgb_attrs = hlattrs2dict(syn_attr2entry(attr), ui->rgb);
       ADD(new_item, DICTIONARY_OBJ(rgb_attrs));
     } else {
       ADD(new_item, DICTIONARY_OBJ((Dictionary)ARRAY_DICT_INIT));
diff --git a/src/nvim/cursor_shape.c b/src/nvim/cursor_shape.c
index b45e7002f7..cf79005a37 100644
--- a/src/nvim/cursor_shape.c
+++ b/src/nvim/cursor_shape.c
@@ -64,6 +64,9 @@ Array mode_style_array(void)
       PUT(dic, "blinkoff", INTEGER_OBJ(cur->blinkoff));
       PUT(dic, "hl_id", INTEGER_OBJ(cur->id));
       PUT(dic, "id_lm", INTEGER_OBJ(cur->id_lm));
+      PUT(dic, "attr_id", INTEGER_OBJ(cur->id ? syn_id2attr(cur->id) : 0));
+      PUT(dic, "attr_id_lm", INTEGER_OBJ(cur->id_lm ? syn_id2attr(cur->id_lm)
+                                                    : 0));
     }
     PUT(dic, "name", STRING_OBJ(cstr_to_string(cur->full_name)));
     PUT(dic, "short_name", STRING_OBJ(cstr_to_string(cur->name)));
@@ -258,15 +261,30 @@ char_u *parse_shape_opt(int what)
 /// @return -1 in case of failure, else the matching SHAPE_ID* integer
 int cursor_mode_str2int(const char *mode)
 {
-  for (int current_mode = 0; current_mode < SHAPE_IDX_COUNT; current_mode++) {
-    if (strcmp(shape_table[current_mode].full_name, mode) == 0) {
-      return current_mode;
+  for (int mode_idx = 0; mode_idx < SHAPE_IDX_COUNT; mode_idx++) {
+    if (strcmp(shape_table[mode_idx].full_name, mode) == 0) {
+      return mode_idx;
     }
   }
   WLOG("Unknown mode %s", mode);
   return -1;
 }
 
+/// Check if a syntax id is used as a cursor style.
+bool cursor_mode_uses_syn_id(int syn_id)
+{
+  if (*p_guicursor == NUL) {
+    return false;
+  }
+  for (int mode_idx = 0; mode_idx < SHAPE_IDX_COUNT; mode_idx++) {
+    if (shape_table[mode_idx].id == syn_id
+        || shape_table[mode_idx].id_lm == syn_id) {
+      return true;
+    }
+  }
+  return false;
+}
+
 
 /// Return the index into shape_table[] for the current mode.
 int cursor_get_mode_idx(void)
diff --git a/src/nvim/highlight.c b/src/nvim/highlight.c
index 0b39ba442e..a104137d9e 100644
--- a/src/nvim/highlight.c
+++ b/src/nvim/highlight.c
@@ -135,11 +135,8 @@ int hl_get_ui_attr(int idx, int final_id, bool optional)
 
   int syn_attr = syn_id2attr(final_id);
   if (syn_attr != 0) {
-    HlAttrs *aep = syn_attr2entry(syn_attr);
-    if (aep) {
-      attrs = *aep;
-      available = true;
-    }
+    attrs = syn_attr2entry(syn_attr);
+    available = true;
   }
   if (optional && !available) {
     return 0;
@@ -232,42 +229,33 @@ int hl_combine_attr(int char_attr, int prim_attr)
     return id;
   }
 
-  HlAttrs *char_aep, *spell_aep;
-  HlAttrs new_en = HLATTRS_INIT;
+  HlAttrs char_aep = syn_attr2entry(char_attr);
+  HlAttrs spell_aep = syn_attr2entry(prim_attr);
 
+  // start with low-priority attribute, and override colors if present below.
+  HlAttrs new_en = char_aep;
 
-  // Find the entry for char_attr
-  char_aep = syn_attr2entry(char_attr);
+  new_en.cterm_ae_attr |= spell_aep.cterm_ae_attr;
+  new_en.rgb_ae_attr |= spell_aep.rgb_ae_attr;
 
-  if (char_aep != NULL) {
-    // Copy all attributes from char_aep to the new entry
-    new_en = *char_aep;
+  if (spell_aep.cterm_fg_color > 0) {
+    new_en.cterm_fg_color = spell_aep.cterm_fg_color;
   }
 
-  spell_aep = syn_attr2entry(prim_attr);
-  if (spell_aep != NULL) {
-    new_en.cterm_ae_attr |= spell_aep->cterm_ae_attr;
-    new_en.rgb_ae_attr |= spell_aep->rgb_ae_attr;
-
-    if (spell_aep->cterm_fg_color > 0) {
-      new_en.cterm_fg_color = spell_aep->cterm_fg_color;
-    }
-
-    if (spell_aep->cterm_bg_color > 0) {
-      new_en.cterm_bg_color = spell_aep->cterm_bg_color;
-    }
+  if (spell_aep.cterm_bg_color > 0) {
+    new_en.cterm_bg_color = spell_aep.cterm_bg_color;
+  }
 
-    if (spell_aep->rgb_fg_color >= 0) {
-      new_en.rgb_fg_color = spell_aep->rgb_fg_color;
-    }
+  if (spell_aep.rgb_fg_color >= 0) {
+    new_en.rgb_fg_color = spell_aep.rgb_fg_color;
+  }
 
-    if (spell_aep->rgb_bg_color >= 0) {
-      new_en.rgb_bg_color = spell_aep->rgb_bg_color;
-    }
+  if (spell_aep.rgb_bg_color >= 0) {
+    new_en.rgb_bg_color = spell_aep.rgb_bg_color;
+  }
 
-    if (spell_aep->rgb_sp_color >= 0) {
-      new_en.rgb_sp_color = spell_aep->rgb_sp_color;
-    }
+  if (spell_aep.rgb_sp_color >= 0) {
+    new_en.rgb_sp_color = spell_aep.rgb_sp_color;
   }
 
   id = get_attr_entry((HlEntry){ .attr = new_en, .kind = kHlCombine,
@@ -280,44 +268,41 @@ int hl_combine_attr(int char_attr, int prim_attr)
 }
 
 /// Get highlight attributes for a attribute code
-HlAttrs *syn_attr2entry(int attr)
+HlAttrs syn_attr2entry(int attr)
 {
   if (attr <= 0 || attr >= (int)kv_size(attr_entries)) {
     // invalid attribute code, or the tables were cleared
-    return NULL;
+    return HLATTRS_INIT;
   }
-  return &(kv_A(attr_entries, attr).attr);
+  return kv_A(attr_entries, attr).attr;
 }
 
 /// Gets highlight description for id `attr_id` as a map.
 Dictionary hl_get_attr_by_id(Integer attr_id, Boolean rgb, Error *err)
 {
-  HlAttrs *aep = NULL;
   Dictionary dic = ARRAY_DICT_INIT;
 
   if (attr_id == 0) {
     return dic;
   }
 
-  aep = syn_attr2entry((int)attr_id);
-  if (!aep) {
+  if (attr_id <= 0 || attr_id >= (int)kv_size(attr_entries)) {
     api_set_error(err, kErrorTypeException,
                   "Invalid attribute id: %" PRId64, attr_id);
     return dic;
   }
 
-  return hlattrs2dict(aep, rgb);
+  return hlattrs2dict(syn_attr2entry((int)attr_id), rgb);
 }
 
 /// Converts an HlAttrs into Dictionary
 ///
 /// @param[in] aep data to convert
 /// @param use_rgb use 'gui*' settings if true, else resorts to 'cterm*'
-Dictionary hlattrs2dict(const HlAttrs *aep, bool use_rgb)
+Dictionary hlattrs2dict(HlAttrs ae, bool use_rgb)
 {
-  assert(aep);
   Dictionary hl = ARRAY_DICT_INIT;
-  int mask  = use_rgb ? aep->rgb_ae_attr : aep->cterm_ae_attr;
+  int mask  = use_rgb ? ae.rgb_ae_attr : ae.cterm_ae_attr;
 
   if (mask & HL_BOLD) {
     PUT(hl, "bold", BOOLEAN_OBJ(true));
@@ -344,24 +329,24 @@ Dictionary hlattrs2dict(const HlAttrs *aep, bool use_rgb)
   }
 
   if (use_rgb) {
-    if (aep->rgb_fg_color != -1) {
-      PUT(hl, "foreground", INTEGER_OBJ(aep->rgb_fg_color));
+    if (ae.rgb_fg_color != -1) {
+      PUT(hl, "foreground", INTEGER_OBJ(ae.rgb_fg_color));
     }
 
-    if (aep->rgb_bg_color != -1) {
-      PUT(hl, "background", INTEGER_OBJ(aep->rgb_bg_color));
+    if (ae.rgb_bg_color != -1) {
+      PUT(hl, "background", INTEGER_OBJ(ae.rgb_bg_color));
     }
 
-    if (aep->rgb_sp_color != -1) {
-      PUT(hl, "special", INTEGER_OBJ(aep->rgb_sp_color));
+    if (ae.rgb_sp_color != -1) {
+      PUT(hl, "special", INTEGER_OBJ(ae.rgb_sp_color));
     }
   } else {
-    if (cterm_normal_fg_color != aep->cterm_fg_color) {
-      PUT(hl, "foreground", INTEGER_OBJ(aep->cterm_fg_color - 1));
+    if (cterm_normal_fg_color != ae.cterm_fg_color) {
+      PUT(hl, "foreground", INTEGER_OBJ(ae.cterm_fg_color - 1));
     }
 
-    if (cterm_normal_bg_color != aep->cterm_bg_color) {
-      PUT(hl, "background", INTEGER_OBJ(aep->cterm_bg_color - 1));
+    if (cterm_normal_bg_color != ae.cterm_bg_color) {
+      PUT(hl, "background", INTEGER_OBJ(ae.cterm_bg_color - 1));
     }
   }
 
diff --git a/src/nvim/screen.c b/src/nvim/screen.c
index 43ab9cd356..bcfef89cc2 100644
--- a/src/nvim/screen.c
+++ b/src/nvim/screen.c
@@ -2424,12 +2424,12 @@ win_line (
   if (wp->w_p_cul && lnum == wp->w_cursor.lnum
       && !(wp == curwin && VIsual_active)) {
     int cul_attr = win_hl_attr(wp, HLF_CUL);
-    HlAttrs *aep = syn_attr2entry(cul_attr);
+    HlAttrs ae = syn_attr2entry(cul_attr);
 
     // We make a compromise here (#7383):
     //  * low-priority CursorLine if fg is not set
     //  * high-priority ("same as Vim" priority) CursorLine if fg is set
-    if (aep->rgb_fg_color == -1 && aep->cterm_fg_color == 0) {
+    if (ae.rgb_fg_color == -1 && ae.cterm_fg_color == 0) {
       line_attr_lowprio = cul_attr;
     } else {
       if (line_attr != 0 && !(State & INSERT) && bt_quickfix(wp->w_buffer)
diff --git a/src/nvim/syntax.c b/src/nvim/syntax.c
index d7c23742ba..3cb998b805 100644
--- a/src/nvim/syntax.c
+++ b/src/nvim/syntax.c
@@ -16,6 +16,7 @@
 #include "nvim/ascii.h"
 #include "nvim/syntax.h"
 #include "nvim/charset.h"
+#include "nvim/cursor_shape.h"
 #include "nvim/eval.h"
 #include "nvim/ex_cmds2.h"
 #include "nvim/ex_docmd.h"
@@ -7228,7 +7229,6 @@ static void set_hl_attr(int idx)
   HlAttrs at_en = HLATTRS_INIT;
   struct hl_group     *sgp = HL_TABLE() + idx;
 
-
   at_en.cterm_ae_attr = sgp->sg_cterm;
   at_en.cterm_fg_color = sgp->sg_cterm_fg;
   at_en.cterm_bg_color = sgp->sg_cterm_bg;
@@ -7241,6 +7241,11 @@ static void set_hl_attr(int idx)
   at_en.rgb_sp_color = sgp->sg_rgb_sp_name ? sgp->sg_rgb_sp : -1;
 
   sgp->sg_attr = hl_get_syn_attr(idx+1, at_en);
+
+  // a cursor style uses this syn_id, make sure its atribute is updated.
+  if (cursor_mode_uses_syn_id(idx+1)) {
+    ui_mode_info_set();
+  }
 }
 
 /// Lookup a highlight group name and return its ID.
diff --git a/src/nvim/tui/tui.c b/src/nvim/tui/tui.c
index 508d25cd3b..df14ddf988 100644
--- a/src/nvim/tui/tui.c
+++ b/src/nvim/tui/tui.c
@@ -874,7 +874,7 @@ static cursorentry_T decode_cursor_entry(Dictionary args)
       r.blinkon = (int)value.data.integer;
     } else if (strequal(key, "blinkoff")) {
       r.blinkoff = (int)value.data.integer;
-    } else if (strequal(key, "hl_id")) {
+    } else if (strequal(key, "attr_id")) {
       r.id = (int)value.data.integer;
     }
   }
@@ -942,13 +942,10 @@ static void tui_set_mode(UI *ui, ModeShape mode)
   TUIData *data = ui->data;
   cursorentry_T c = data->cursor_shapes[mode];
 
-  if (c.id != 0 && ui->rgb) {
-    int attr = syn_id2attr(c.id);
-    if (attr > 0) {
-      HlAttrs *aep = syn_attr2entry(attr);
-      UNIBI_SET_NUM_VAR(data->params[0], aep->rgb_bg_color);
-      unibi_out_ext(ui, data->unibi_ext.set_cursor_color);
-    }
+  if (c.id != 0 && c.id < (int)kv_size(data->attrs) && ui->rgb) {
+    int color = kv_A(data->attrs, c.id).rgb_bg_color;
+    UNIBI_SET_NUM_VAR(data->params[0], color);
+    unibi_out_ext(ui, data->unibi_ext.set_cursor_color);
   }
 
   int shape;
diff --git a/src/nvim/ui.c b/src/nvim/ui.c
index ef68b804ba..07aa032a50 100644
--- a/src/nvim/ui.c
+++ b/src/nvim/ui.c
@@ -55,6 +55,7 @@ static int row = 0, col = 0;
 static bool pending_cursor_update = false;
 static int busy = 0;
 static int mode_idx = SHAPE_IDX_N;
+static bool pending_mode_info_update = false;
 static bool pending_mode_update = false;
 
 #if MIN_LOG_LEVEL > DEBUG_LOG_LEVEL
@@ -368,10 +369,7 @@ void ui_add_linewrap(int row)
 
 void ui_mode_info_set(void)
 {
-  Array style = mode_style_array();
-  bool enabled = (*p_guicursor != NUL);
-  ui_call_mode_info_set(enabled, style);
-  api_free_array(style);
+  pending_mode_info_update = true;
 }
 
 int ui_current_row(void)
@@ -391,6 +389,13 @@ void ui_flush(void)
     ui_call_grid_cursor_goto(1, row, col);
     pending_cursor_update = false;
   }
+  if (pending_mode_info_update) {
+    Array style = mode_style_array();
+    bool enabled = (*p_guicursor != NUL);
+    ui_call_mode_info_set(enabled, style);
+    api_free_array(style);
+    pending_mode_info_update = false;
+  }
   if (pending_mode_update) {
     char *full_name = shape_table[mode_idx].full_name;
     ui_call_mode_change(cstr_as_string(full_name), mode_idx);
diff --git a/test/functional/ui/cursor_spec.lua b/test/functional/ui/cursor_spec.lua
index 812c095add..d1cddbe06a 100644
--- a/test/functional/ui/cursor_spec.lua
+++ b/test/functional/ui/cursor_spec.lua
@@ -28,6 +28,8 @@ describe('ui/cursor', function()
         name = 'normal',
         hl_id = 0,
         id_lm = 0,
+        attr = {},
+        attr_lm = {},
         mouse_shape = 0,
         short_name = 'n' },
       [2] = {
@@ -39,6 +41,8 @@ describe('ui/cursor', function()
         name = 'visual',
         hl_id = 0,
         id_lm = 0,
+        attr = {},
+        attr_lm = {},
         mouse_shape = 0,
         short_name = 'v' },
       [3] = {
@@ -50,6 +54,8 @@ describe('ui/cursor', function()
         name = 'insert',
         hl_id = 0,
         id_lm = 0,
+        attr = {},
+        attr_lm = {},
         mouse_shape = 0,
         short_name = 'i' },
       [4] = {
@@ -61,6 +67,8 @@ describe('ui/cursor', function()
         name = 'replace',
         hl_id = 0,
         id_lm = 0,
+        attr = {},
+        attr_lm = {},
         mouse_shape = 0,
         short_name = 'r' },
       [5] = {
@@ -72,6 +80,8 @@ describe('ui/cursor', function()
         name = 'cmdline_normal',
         hl_id = 0,
         id_lm = 0,
+        attr = {},
+        attr_lm = {},
         mouse_shape = 0,
         short_name = 'c' },
       [6] = {
@@ -83,6 +93,8 @@ describe('ui/cursor', function()
         name = 'cmdline_insert',
         hl_id = 0,
         id_lm = 0,
+        attr = {},
+        attr_lm = {},
         mouse_shape = 0,
         short_name = 'ci' },
       [7] = {
@@ -94,6 +106,8 @@ describe('ui/cursor', function()
         name = 'cmdline_replace',
         hl_id = 0,
         id_lm = 0,
+        attr = {},
+        attr_lm = {},
         mouse_shape = 0,
         short_name = 'cr' },
       [8] = {
@@ -105,6 +119,8 @@ describe('ui/cursor', function()
         name = 'operator',
         hl_id = 0,
         id_lm = 0,
+        attr = {},
+        attr_lm = {},
         mouse_shape = 0,
         short_name = 'o' },
       [9] = {
@@ -116,6 +132,8 @@ describe('ui/cursor', function()
         name = 'visual_select',
         hl_id = 0,
         id_lm = 0,
+        attr = {},
+        attr_lm = {},
         mouse_shape = 0,
         short_name = 've' },
       [10] = {
@@ -155,6 +173,8 @@ describe('ui/cursor', function()
         name = 'showmatch',
         hl_id = 0,
         id_lm = 0,
+        attr = {},
+        attr_lm = {},
         short_name = 'sm' },
       }
 
@@ -179,6 +199,7 @@ describe('ui/cursor', function()
     end)
 
     -- Change the cursor style.
+    helpers.command('hi Cursor guibg=DarkGray')
     helpers.command('set guicursor=n-v-c:block,i-ci-ve:ver25,r-cr-o:hor20'
       ..',a:blinkwait700-blinkoff400-blinkon250-Cursor/lCursor'
       ..',sm:block-blinkwait175-blinkoff150-blinkon175')
@@ -194,7 +215,10 @@ describe('ui/cursor', function()
         if m.blinkoff then m.blinkoff = 400 end
         if m.blinkwait then m.blinkwait = 700 end
       end
-      if m.hl_id then m.hl_id = 49 end
+      if m.hl_id then
+          m.hl_id = 49
+          m.attr = {background = Screen.colors.DarkGray}
+      end
       if m.id_lm then m.id_lm = 50 end
     end
 
@@ -205,6 +229,26 @@ describe('ui/cursor', function()
       eq('normal', screen.mode)
     end)
 
+    -- Change hl groups only, should update the styles
+    helpers.command('hi Cursor guibg=Red')
+    helpers.command('hi lCursor guibg=Green')
+
+    -- Update the expected values.
+    for _, m in ipairs(expected_mode_info) do
+      if m.hl_id then
+          m.attr = {background = Screen.colors.Red}
+      end
+      if m.id_lm then
+          m.attr_lm = {background = Screen.colors.Green}
+      end
+    end
+    -- Assert the new expectation.
+    screen:expect(function()
+      eq(expected_mode_info, screen._mode_info)
+      eq(true, screen._cursor_style_enabled)
+      eq('normal', screen.mode)
+    end)
+
     -- Another cursor style.
     meths.set_option('guicursor', 'n-v-c:ver35-blinkwait171-blinkoff172-blinkon173'
       ..',ve:hor35,o:ver50,i-ci:block,r-cr:hor90,sm:ver42')
diff --git a/test/functional/ui/screen.lua b/test/functional/ui/screen.lua
index c40b2210ff..872a9d3200 100644
--- a/test/functional/ui/screen.lua
+++ b/test/functional/ui/screen.lua
@@ -386,6 +386,17 @@ end
 
 function Screen:_handle_mode_info_set(cursor_style_enabled, mode_info)
   self._cursor_style_enabled = cursor_style_enabled
+  for _, item in pairs(mode_info) do
+      -- attr IDs are not stable, but their value should be
+      if item.attr_id ~= nil then
+        item.attr = self._attr_table[item.attr_id][1]
+        item.attr_id = nil
+      end
+      if item.attr_id_lm ~= nil then
+        item.attr_lm = self._attr_table[item.attr_id_lm][1]
+        item.attr_id_lm = nil
+      end
+  end
   self._mode_info = mode_info
 end