WIP: revisit dynamic signcolumn #9788

blueyed · 2019-03-25T18:07:56Z

No description provided.

This goes well together with the previous commit. Vim uses this internally since it has a hard width of 2 cells for the signcolumn, but that has changed in Neovim now.

src/nvim/buffer.c

blueyed · 2019-03-25T18:10:06Z

src/nvim/buffer.c

            }
        }

        if (nr_signs > 0 && buf->b_signcols_max < same) {
            buf->b_signcols_max = same;
+            buf->b_signcols_cells = cells;
        }


This loop should be made clearer probably.

blueyed · 2019-03-25T18:10:47Z

src/nvim/edit.c

@@ -5899,7 +5899,7 @@ comp_textwidth (
      textwidth -= 1;
    }
    textwidth -= curwin->w_p_fdc;
-    textwidth -= win_signcol_count(curwin);
+    textwidth -= win_signcol_width(curwin);


Should/could address the buffer value directly probably for performance reasons.

blueyed · 2019-03-25T18:11:19Z

src/nvim/ex_cmds.c

-            if (cells == 1) {
-              STRCPY(sp->sn_text + len - 1, " ");
-            }
+            sp->sn_text = vim_strnsave(arg, p - arg);


This is in a separate commit - not strictly necessary, but allows for having signs only take one cell after all.

blueyed · 2019-03-25T18:12:12Z

src/nvim/option.c

@@ -3035,6 +3035,8 @@ did_set_string_option (
    // 'signcolumn'
    if (check_opt_strings(*varp, p_scl_values, false) != OK) {
      errmsg = e_invarg;
+    } else {
+      curwin->w_buffer->b_signcols_max = -1;


Invalidates the cache - could need a comment maybe. But it is done like this in many places already.

probably need #define SIGNCOLS_UNKNOWN -1 in sign_defs.h by now.

blueyed · 2019-03-25T18:13:13Z

src/nvim/option.c

+  if (cols) {
+    return MAX(cols, wp->w_buffer->b_signcols_cells);
+  }
+  return 0;


This was quick and dirty.
Any idea how to improve this? (not having two functions)

blueyed · 2019-03-25T18:25:11Z

src/nvim/screen.c

-                          p_extra = extra;
-                          p_extra[n_extra] = NUL;
+                          n_extra = (int)STRLEN(p_extra);
+                          sign_cells -= mb_string2cells(p_extra);


The previous use of extra was not clear to me. Using just p_extra and n_extra seems to be good, no?
/cc @teto (via 0848add).

src/nvim/screen.c

justinmk · 2019-03-25T18:56:12Z

src/nvim/buffer_defs.h

@@ -781,6 +781,7 @@ struct file_buffer {
  signlist_T *b_signlist;       // list of signs to draw
  int b_signcols_max;           // cached maximum number of sign columns
  int b_signcols;               // last calculated number of sign columns
+  int b_signcols_cells;         // number of cells used for sign columns


why do we need another field (and thus all the associated housekeeping)? why can't this replace b_signcols?

count != cols, max/b_signcols holds the count.
https://github.com/blueyed/neovim/blob/6d5f8c7addcc4b276a436531e75cb3b620fe513e/src/nvim/buffer.c#L5163-L5167
But it certainly should at least use "cols" (effective).
I might be missing something here though.

justinmk · 2019-03-25T18:58:41Z

cc @da-x

blueyed · 2019-03-25T22:04:40Z

So one test is failing, but I cannot reproduce it (likely because the real UI does not allow to move the cursor there?!):

[  ERROR   ] test/functional/ui/sign_spec.lua @ 118: Signs :sign place multiple signs #9295
test/functional/ui/screen.lua:560: Row 4 did not match.
Expected:
  |{1:>>}XX{6:  1 }a                                            |
  |XX{1:>>}{6:  2 }b                                            |
  |{1:>>}WW{6:  3 }c                                            |
  |*{2:    }{6:  4 }^                                             |
  …
Actual:
  |{1:>>}XX{6:  1 }a                                            |
  |XX{1:>>}{6:  2 }b                                            |
  |{1:>>}WW{6:  3 }c                                            |
  |*{2:    }{6:  ^4 }                                             |

This is meant to be more robust in case signs are not two chars long. (neovim/neovim#9788)

blueyed · 2019-05-22T02:17:19Z

Closing for now.

blueyed added 4 commits March 25, 2019 12:43

minor: improve doc/comments with 36762a0

8138e0b

win_signcol_count: do not call buf_signcols unnecessarily

13ba0ca

WIP: revisit dynamic signcolumn

c83da12

Ref: neovim#9295 (comment)

ex_sign: do not append trailing space to sign text

7219fdd

This goes well together with the previous commit. Vim uses this internally since it has a hard width of 2 cells for the signcolumn, but that has changed in Neovim now.

This was referenced Mar 25, 2019

minor: improve doc/comments with 36762a0 #9786

Closed

Multiple sign columns; remove signs on line deletion #9295

Closed

blueyed commented Mar 25, 2019

View reviewed changes

fixup! WIP: revisit dynamic signcolumn [ci skip]

dd99958

justinmk reviewed Mar 25, 2019

View reviewed changes

src/nvim/screen.c Outdated Show resolved Hide resolved

justinmk reviewed Mar 25, 2019

View reviewed changes

blueyed added 3 commits March 25, 2019 20:12

fixup! fixup! WIP: revisit dynamic signcolumn [ci skip]

3d345a3

handle null text

e4a72ea

win_signcol_width: min 2, not adding one always (?) [ci skip]

dd35274

blueyed force-pushed the signcols branch from 3f99858 to dd35274 Compare March 25, 2019 21:04

blueyed added 2 commits March 25, 2019 22:35

fixup! win_signcol_width: min 2, not adding one always (?) [ci skip]

94929bf

move [ci skip]

6d5f8c7

blueyed added a commit to neomake/neomake that referenced this pull request Mar 28, 2019

tests: neomake#signs#RedefineErrorSign: split sign

22a45a6

This is meant to be more robust in case signs are not two chars long. (neovim/neovim#9788)

blueyed mentioned this pull request Mar 28, 2019

tests: neomake#signs#RedefineErrorSign: split sign neomake/neomake#2346

Merged

blueyed added a commit to neomake/neomake that referenced this pull request Mar 28, 2019

tests: neomake#signs#RedefineErrorSign: split sign (#2346)

36c91c3

This is meant to be more robust in case signs are not two chars long. (neovim/neovim#9788)

blueyed mentioned this pull request Apr 8, 2019

set a maximum number of signs per line / collapse marks neomake/neomake#2349

Closed

justinmk mentioned this pull request May 11, 2019

extra space in signcolumn when using unicode character like ⚠ #9997

Closed

blueyed closed this May 22, 2019

blueyed deleted the signcols branch May 22, 2019 02:18

blueyed mentioned this pull request Jun 22, 2019

sign column: variable width #990

Closed

ghost mentioned this pull request Oct 16, 2020

signcolumn values are not multiples of 1 and column width must be at least 2 #13098

Closed

ivomac mentioned this pull request May 7, 2025

Add option to replace hardcoded sign width #33882

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

WIP: revisit dynamic signcolumn #9788

WIP: revisit dynamic signcolumn #9788

Uh oh!

blueyed commented Mar 25, 2019

Uh oh!

Uh oh!

blueyed Mar 25, 2019

Uh oh!

blueyed Mar 25, 2019

Uh oh!

blueyed Mar 25, 2019

Uh oh!

blueyed Mar 25, 2019

Uh oh!

justinmk Mar 25, 2019 •

edited

Loading

Uh oh!

blueyed Mar 25, 2019

Uh oh!

blueyed Mar 25, 2019

Uh oh!

Uh oh!

justinmk Mar 25, 2019

Uh oh!

blueyed Mar 25, 2019

Uh oh!

justinmk commented Mar 25, 2019

Uh oh!

blueyed commented Mar 25, 2019

Uh oh!

blueyed commented May 22, 2019

Uh oh!

Uh oh!

Uh oh!

WIP: revisit dynamic signcolumn #9788

WIP: revisit dynamic signcolumn #9788

Uh oh!

Conversation

blueyed commented Mar 25, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk Mar 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk commented Mar 25, 2019

Uh oh!

blueyed commented Mar 25, 2019

Uh oh!

blueyed commented May 22, 2019

Uh oh!

Uh oh!

justinmk Mar 25, 2019 •

edited

Loading