Should this use/implement namespaces, like bufhl/eol-annotations? I haven't thought this through, but seems to make sense to me as it is a "text decoration" in the broader sense.

qsort(sign_line_numbers, (size_t)nr_signs, sizeof(linenr_T), linenr_compare);

Hmm, this is n log(n) in number of signs. But as all sign-operations already are O(n) due to linked-list, it probably doesn't make things much worse... We could change signs to use the kbtree representation of bufhl/extmark later.

Should we allow for signs adders to specify for each added sign, whether it should be removed on line deletion?

No. Pick a reasonable default.

Should the specification for sign removal on line deletion be accessible to plugin developers via API or be more user-facing via configuration?

Is that something that needs to be decided now? Would be better to get this PR in a mergeable state, and leave other enhancements for the future.

cc @teto

Is that something that needs to be decided now? Would be better to get this PR in a mergeable state, and leave other enhancements for the future.

There are so many possibilities regarding the columns that it would be good to define the scope beforehand (which is also what stopped me from pushing this forward), also anticipating on externalising columns ? Like can we merge different columns (folds/signs) ? shouldn't we put a limit on the number of signs ?
@ZyX-I had proposed an API for columns IIRC.

can we merge different columns (folds/signs) ? shouldn't we put a limit on the number of signs ?

Those examples don't seem like decisions that matter right now. "merging columns" is an enhancement that doesn't change the shape of the current work here. And limits can be decided later.

So my suggestion @da-x is to add some tests here and let's plan it for 0.3.3 (and please, anyone who cares about this functionality: review this PR). Otherwise let's forget it, because it's a lot of wasted effort to review PRs that don't get merged.

@justinmk to get this forward, I suggest to keep the removal of all signs excepts marks on line deletion by default (i.e. keep marks because those are usually manually added by users), and sort signs by id before drawing them. If this sounds reasonable I can implement and move to testing.

Sounds good to me.

One question that might be out of scope: what happens if line is deleted then restored by undo? @timeyyy does extmarks address that case (for marks, not signs)?

After more checking, marks are already removed by deleted lines, so there's no need to preserved the signs for them. Seems that undo restores them, vim-signature plugin, responsible for the mark's signs, restores the signs as well.

I've added the change for sort by Id.

@justinmk extmark does restore marks after delete/undo. Changing sign implementation from linked list to extmark data structure is a low-hanging fruit after extmark is merged.

It came to my attention that we may need to reverse the sort order for signcolumn=yes, or to let this be customizable, otherwise the ALE signs will not be visible and possibly annoy users. Due to previous issues with the interop of these plugins ALE puts the default sign Id above that of gitgutter and other plugins by default, see dense-analysis/ale#569. This is done with no effect, though, because presently there's no sort mechanism. It also turns out that gitgutter at its current version avoids putting signs altogether if it detects other signs present... I have an open PR in vim-gitgutter to customize this behavior. airblade/vim-gitgutter#565

To sum it, turns out plugins really "fought over each other" due to this non-determinism of the "who gets to put his sign in the column". I'm glad we are beginning to sort that out :)

I propose the following enhanced semantics for signcolumn:

• yes, no, auto - preserve current behavior
• max-N - like auto, but up to N (0-9) signs. max-1 is equivalent to auto. max-0 is equivalent to no.
• fixed-N - like yes, always have N-sized column, with up to N signs to draw in it. fixed-1 is equivalent to yes. max-0 is equivalent to no.

In addition, a new setting:

						*'signpriority'* *'spri'*
'signpriority' 'spri'	string	(default "")
			local to window
	Some lines may have more signs to display than what the sign column
	can support.  The following is a list of comma separated prefixes that
	give priority to sign types, for the purpose of determining which
	signs will be displayed, and in what order. For example:

	`:set signpriority=ALE,GitGutter`

	In the example above, all sign types that do not start with either
	`ALE` nor `GitGutter` will be given priority 0, where `ALE` and
	`GitGutter` signs will be given priority 2, and 1 respectively.

As for sorting, we would do so by sign type. However, once we are activating it with the PR's qsort changes, insert order is lost. So to completely preserve the current behavior, we can add a monotonic uint64_t to the signlist_T struct, and use as the second key when sorting. In time, all plugins that implement signs, can instruct their users to configure signpriority according to their taste. Plugins can still determine ordering between their own signs by insertion, as it was before.

Hopefully I have not missed anything, and having this ends "sign war" between plugins, and makes the meaning of the sign Id number private to each plugin.

@da-x

• regarding signcolumn:
  • max-N:
    • the usual Vim way to have sub-options is with colon, not hyphen.
    • call it auto:N instead of max-N
  • fixed-N: call it yes:N instead
• regarding signpriority:
  • Let's not creep into this scope for this PR. Similar to Multiple sign columns; remove signs on line deletion #9295 (comment) , signpriority is not something that needs to be figured out to make forward progress here, is it?
  • Where are "sign types" defined? It might sense to use namespaces: [RFC] make namespaces explicit (intitially for bufhl and virttext) #6920

@justinmk ok, I'll keep it for an additional PR :)

Sign types are currently a global list of names, controlled with sign define. Each sign has its own generated unique Id, so it's already quite similar to namespaces, and IMHO it makes less sense to add another Id on top of it. Also, I don't see how the sort order is going to be determined that way.

There may be something else to keep for an additional PR, for which namespaces may be fitting. I noticed that sign Ids from different plugins can conflict, and the commands to manage them are not supportive of avoiding that. The sign place and sign unplace commands: da-x@2997e00 .

Alternatively to that change, we can add an optional ns= to the sign place and sign unplace, or we can somehow associate a namespace Id with the current plugin so we would not need to change the commands. The last suggestion is a bit off-topic, though. I like somewhat, that per vimL source file, plugin writers could simply stick a nvim_set_script_namespace(<string>) call, and it would set a hidden script variable that commands such as sign can implicitly pick up. I am not sure how current Neovim code is easy to modify for that, though.

BTW, currently these commands don't behave according to the documentation - sign place {id} name={name} file={fname} is supposed to modify the type of the sign if it exists, where due to the logic first added in d9f5cd6 and later revised in 6e99287, it can result in multiple signs having the same Id on the same line in the same buffer. Bug can be triggered (on Neovim 0.3.1) with a loop doing this command sequence (with Name as a valid sign type): exe "sign place 1001 line=1 name=Name buffer=".bufnr("%"), exe "sign place 1002 line=1 name=Name buffer=".bufnr("%").

Trying to cram namespaces, either explicitly or implicitly (how would nvim_set_script_namespace() work with lua or RPC?) into the :sign command probably doesn't make any sense. For that we could instead have a proper nvim_buf_add_sign() API function (which would be simply cleared by nvim_buf_clear_namespace, no need to "unplace" anything).

Is this waiting on feedback or what's left to do?

@justinmk I've being using this branch for awhile and noticed some bugs that need fixing. I'll push when I get it stabled. In another PR I'll take @bfredl 's suggestion regarding nvim_buf_add_sign().

Rebased, and added a commit: multi signcols: no need to sort signs in buf_addsign.

ToDo:

• Implement @justinmk 's "regarding signcolumn" suggestion.
• Fixes for win_signcol_count() impl per @bfredl 's review.

CI failures

I merged this into master and pushed it here, to check CI again.

• Added a note at :help nvim-features.
• Added a test for yes:9 before the auto:4 test, so that unchanged=true can be removed (makes the screen test stronger).
• Note for reference: 24d7513 (merged recently, after the last update to this PR) added "sign ordering", and a prev member on the signlist_T struct.

Merged. Thanks for following through with this @da-x .

I squashed the commits (preserving the commit messages) since most of the intermediate commits were fixups.

I've noticed that with set signcolumn=auto:3 and only two signs I am seeing 3 columns: "~ ⚠" (gitgutter and Neomake). Shouldn't that be 2?
(sign place shows two only for that line)

Oh, that's due to it using 2 as a multiplier (via win_signcol_width - https://github.com/blueyed/neovim/blob/8138e0be874f112a9800325bd275c6dee3e22b9e/src/nvim/screen.c#L1536-L1549)

I think it should not really use a space after each sign, but rather only a single space at the end maybe.

But just using 1 in win_signcol_width (i.e. resulting in two columns for 2 signs) looks quite good (if you like it condensed).
I can see that there might be wide-glyph signs etc, but having 6 columns with 3 signs seems quite "wide".

I think win_signcol_width could maybe be used directly, returning return MAX(2, win_signcol_count(wp) + 1). But it is not straight-forward with https://github.com/blueyed/neovim/blob/8138e0be874f112a9800325bd275c6dee3e22b9e/src/nvim/screen.c#L2705-L2739, which explicitly handles "filling spaces".

From an API point of view win_signcol_width is supposed to return the width of the whole "column", which includes all signs, and not the width of each sign in there I think.

I'll give this a shot - drawing all signs at once in win_line.

win_signcol_width was added in 0848add - and its purpose appears to be like I've understood it.

OTOH sign text is always 2 chars long.. the help says that it must fit into 1-2 cells. Does it make sense to trim a trailing space here?

=> #9788