fix(column): modifying a sign should update placed signs (#29750)

Problem:  Modifying a sign no longer updates already placed signs.
Solution: Loop over (newly-exposed) placed decorations when modifying a
          sign definition. Update placed decor if it belongs to the sign
          that is modified.

4 files changed, 76 insertions, 16 deletions

diff --git a/src/nvim/decoration.c b/src/nvim/decoration.c
index 0cf02d96da..cdb78e9eb5 100644
--- a/src/nvim/decoration.c
+++ b/src/nvim/decoration.c
@@ -31,9 +31,6 @@
 # include "decoration.c.generated.h"
 #endif
 
-// TODO(bfredl): These should maybe be per-buffer, so that all resources
-// associated with a buffer can be freed when the buffer is unloaded.
-kvec_t(DecorSignHighlight) decor_items = KV_INITIAL_VALUE;
 uint32_t decor_freelist = UINT32_MAX;
 
 // Decorations might be requested to be deleted in a callback in the middle of redrawing.
@@ -292,7 +289,7 @@ static void decor_free_inner(DecorVirtText *vt, uint32_t first_idx)
   while (idx != DECOR_ID_INVALID) {
     DecorSignHighlight *sh = &kv_A(decor_items, idx);
     if (sh->flags & kSHIsSign) {
-      xfree(sh->sign_name);
+      XFREE_CLEAR(sh->sign_name);
     }
     sh->flags = 0;
     if (sh->url != NULL) {
diff --git a/src/nvim/decoration.h b/src/nvim/decoration.h
index 86d4de79f0..1b595fb86f 100644
--- a/src/nvim/decoration.h
+++ b/src/nvim/decoration.h
@@ -77,6 +77,9 @@ typedef struct {
 } DecorState;
 
 EXTERN DecorState decor_state INIT( = { 0 });
+// TODO(bfredl): These should maybe be per-buffer, so that all resources
+// associated with a buffer can be freed when the buffer is unloaded.
+EXTERN kvec_t(DecorSignHighlight) decor_items INIT( = KV_INITIAL_VALUE);
 
 #ifdef INCLUDE_GENERATED_DECLARATIONS
 # include "decoration.h.generated.h"
diff --git a/src/nvim/sign.c b/src/nvim/sign.c
index fbd25493ef..9b2516ed83 100644
--- a/src/nvim/sign.c
+++ b/src/nvim/sign.c
@@ -404,19 +404,13 @@ static int sign_define_by_name(char *name, char *icon, char *text, char *linehl,
                                char *culhl, char *numhl, int prio)
 {
   cstr_t *key;
-  sign_T **sp = (sign_T **)pmap_put_ref(cstr_t)(&sign_map, name, &key, NULL);
+  bool new_sign = false;
+  sign_T **sp = (sign_T **)pmap_put_ref(cstr_t)(&sign_map, name, &key, &new_sign);
 
-  if (*sp == NULL) {
+  if (new_sign) {
     *key = xstrdup(name);
     *sp = xcalloc(1, sizeof(sign_T));
     (*sp)->sn_name = (char *)(*key);
-  } else {
-    // Signs may already exist, a redraw is needed in windows with a non-empty sign list.
-    FOR_ALL_WINDOWS_IN_TAB(wp, curtab) {
-      if (buf_has_signs(wp->w_buffer)) {
-        redraw_buf_later(wp->w_buffer, UPD_NOT_VALID);
-      }
-    }
   }
 
   // Set values for a defined sign.
@@ -441,6 +435,28 @@ static int sign_define_by_name(char *name, char *icon, char *text, char *linehl,
     }
   }
 
+  // Update already placed signs and redraw if necessary when modifying a sign.
+  if (!new_sign) {
+    bool did_redraw = false;
+    for (size_t i = 0; i < kv_size(decor_items); i++) {
+      DecorSignHighlight *sh = &kv_A(decor_items, i);
+      if (sh->sign_name && strcmp(sh->sign_name, name) == 0) {
+        memcpy(sh->text, (*sp)->sn_text, SIGN_WIDTH * sizeof(schar_T));
+        sh->hl_id = (*sp)->sn_text_hl;
+        sh->line_hl_id = (*sp)->sn_line_hl;
+        sh->number_hl_id = (*sp)->sn_num_hl;
+        sh->cursorline_hl_id = (*sp)->sn_cul_hl;
+        if (!did_redraw) {
+          FOR_ALL_WINDOWS_IN_TAB(wp, curtab) {
+            if (buf_has_signs(wp->w_buffer)) {
+              redraw_buf_later(wp->w_buffer, UPD_NOT_VALID);
+            }
+          }
+          did_redraw = true;
+        }
+      }
+    }
+  }
   return OK;
 }
 
diff --git a/test/functional/legacy/signs_spec.lua b/test/functional/legacy/signs_spec.lua
index 614673ee3c..61eba1e559 100644
--- a/test/functional/legacy/signs_spec.lua
+++ b/test/functional/legacy/signs_spec.lua
@@ -1,13 +1,14 @@
 -- Tests for signs
 
 local n = require('test.functional.testnvim')()
+local Screen = require('test.functional.ui.screen')
 
-local clear, command, expect = n.clear, n.command, n.expect
+local clear, command, exec, expect, feed = n.clear, n.command, n.exec, n.expect, n.feed
 
 describe('signs', function()
-  setup(clear)
+  before_each(clear)
 
-  it('is working', function()
+  it('are working', function()
     command('sign define JumpSign text=x')
     command([[exe 'sign place 42 line=2 name=JumpSign buffer=' . bufnr('')]])
     -- Split the window to the bottom to verify :sign-jump will stay in the current
@@ -21,4 +22,47 @@ describe('signs', function()
 
       2]])
   end)
+
+  -- oldtest: Test_sign_cursor_position()
+  it('are drawn correctly', function()
+    local screen = Screen.new(75, 6)
+    screen:attach()
+    exec([[
+      call setline(1, [repeat('x', 75), 'mmmm', 'yyyy'])
+      call cursor(2,1)
+      sign define s1 texthl=Search text==>
+      redraw
+      sign place 10 line=2 name=s1
+    ]])
+    screen:expect([[
+      {7:  }xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx|
+      {7:  }xx                                                                       |
+      {10:=>}^mmmm                                                                     |
+      {7:  }yyyy                                                                     |
+      {1:~                                                                          }|
+                                                                                 |
+    ]])
+
+    -- Change the sign text
+    command('sign define s1 text=-)')
+    screen:expect([[
+      {7:  }xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx|
+      {7:  }xx                                                                       |
+      {10:-)}^mmmm                                                                     |
+      {7:  }yyyy                                                                     |
+      {1:~                                                                          }|
+                                                                                 |
+    ]])
+
+    -- update cursor position calculation
+    feed('lh')
+    command('sign unplace 10')
+    screen:expect([[
+      xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx|
+      ^mmmm                                                                       |
+      yyyy                                                                       |
+      {1:~                                                                          }|*2
+                                                                                 |
+    ]])
+  end)
 end)