[RDY] support buffer-associated highlighting by plugins

[bfredl]

As discussed in #1767, this aims to implement a variant of matchaddpos, but matches tied to a specific buffer and not the current window.
No actual code for the feature yet, just started with cleanup/refactor of the exisiting code.

[fmoralesc]

@bfredl This will be very appreciated! 👍

[tarruda]

👍

[bfredl]

I first thought, just take take whatever datastructure that stores matchaddpos matches in window_T and replicade that in buffer_T. Unfortunately that will not scale well for a lot of added highlights as currently a single linked list is used to store all matches (regexp or pos based) in a window. For every point in the screen where match highlight can change, the entire list must be traversed, giving O(N*N) complexity where N is the number of position matches. The struct storing a single matchaddpos item is also quite "fat", as it was originaly meant to contain regexp matcher logic and not a single/a few position matches.

So my current plan is instead:

• Use some more efficient structure in buffer_T to store buffer matches. I think the simplest would be an array over every line in the buffer, of pointers to a linked list of highlight entries for that line. (the array can be lazily allocated for whatever prefix of the buffer currently containing matches)
• To avoid excessive surgery in screen.c at this point (before we have a lot more screen tests and such), still reuse the search_hl/matchadd[pos] logic as much as possible. Either just use the "fat" struct anyway (and see if memory consumption is a problem), or reconstruct these for a drawn line at a time, from the compact representation.
• only static position, within single line matches will be supported, in the first iteration of this PR at least. (for regex just continue to use :syntax)

Thoughts/Suggestions/Objections?

[justinmk]

What happens when the text is edited?

[tarruda]

What happens when the text is edited?

I'm not sure if @bfredl plans to cover that, but my guess is that this feature is more useful for read-only buffers.

[bfredl]

Same as for matchaddpos I thought for the first iteration (i e they will get out of sync if lines are added/deleted and not updated). I thought to maybe try supporting more "dynamic" matches, that at least support add/removing lines, as a 2nd step after I get the basics right (and understand the code better :P ) Perhaps the logic for handling manual folds is useful as inspiration, will look into it later.

[justinmk]

Perhaps the logic for handling manual folds is useful as inspiration

Marks also follow line moves (but not column moves, AFAICT).

[bfredl]

After digging yet deeper into screen.c I've realized trying to reuse the matchaddpos logic was a false start. The code feels like "hey lets take the search highlighting code and make it more complicated to let plugins add ad-hoc regex highlighting in a window and while we're at it why not make the same code also do position based highlighting". In contrast, syntax highlighting (not including concealing) has a much cleaner interface to screen.c which basically consists of a single function to get the highlighting at a single character. So contrary to what I said above it would probably be easier and touch existing code much less, to add this as a separate thing.
(Then the pushed refactor in this PR is completely irrelevant to the feature, I might push it separately later if its a welcomed refactor. )

[justinmk]

if its a welcomed refactor.

👍 Looks like you found some clearly redundant blocks

[bfredl]

Yes, should probably go along with some screen tests of hlsearch/matchaddpos ( took some iterations before getting rid of some drawing errors )

[bfredl]

Closest in implemention of exsisting features might be the signs feature. The code for moving signs (they are essentialy treated as marks) to buffer changes doesn't look too complicated, either.

[fmoralesc]

If you figure out a way to move signs when a new line is inserted at the line the sign is at, that would be awesome.

[bfredl]

Now there is a kind-of working prototype, quite messy/inefficient code and such, but now you can do (translate to your api client of choice)
buffer_add_highlight(buffer, srcid, "Statement", 3, 5, 10)
buffer_add_highlight(buffer, srcid, "String", 10, 1, -1)
to highlight column 5-10 (inclusive) on line 3, and entire line 10 as a string. It should respond to removing/inserting lines before, and deleting highlighting lines (though not undoing deletes). Right now srcid can be a positive integer ie to identify the plugin generating the highlight, so that one can delete all highlights with the same source id in a range:
buffer_clear_highlight(buffer, srcid, 1, -1)
(only -1 to mean "to the end of buffer" supported, not back-indexing). Passing srcid=0 to add_highlight will use and return a new unique srcid that then can be reused by the plugin, or pass srcid=-1 if you never intend to delete it with clear_highlight.

I will do some work to cleanup the implementation, but right now I would appreciate feedback on the api design. Would it be a better interface to accept an array of [hl_group, line, start, end] vaules, perhaps? (Alternatively, a client could "stream" a large number of added highlights, by invoking add_highlight as an async notification).
Also, I'm not sure the best way to manage source ids, how do different plugins know they don't use the same value? Perhaps these could be strings translated to id values in a hashtable? Or just return uids from add_highlight, but I think it would be useful for a plugin to say "remove all highlights of this kind"

[bfredl]

Btw, why can't one just do something like apicall("buffer_api_function", arg, arg2) from vimscript? It makes no sense that new core functionality should need both implementing vimscript and new-style api interfaces. (somewhat related: #1446 )

[justinmk]

@bfredl #1446 is definitely related, and I agree, we will move in this direction.

[bfredl]

FWIW, I made a branch of nvim-ipy to test this. There is a bit of "impendance mismatch" right now when implementing terminal output parsing (IPython protocol represents colored output as terminal ANSI sequences), as one for this usecase would want to specify the attributes directly and not a "semantic" highlight group

[tarruda]

@bfredl I'm using this PR into my private branch where I implement buffer terminal emulator for nvim. Except for some minor adjustments, it is working well for me.

[tarruda]

This fixes a bug I introduced in hl_combine_attr during #1820:

diff --git a/src/nvim/syntax.c b/src/nvim/syntax.c
index 26f172c..c9b5b72 100644
--- a/src/nvim/syntax.c
+++ b/src/nvim/syntax.c
@@ -6719,6 +6719,10 @@ int hl_combine_attr(int char_attr, int prim_attr)
     return prim_attr;
   }

+  if (prim_attr == 0) {
+    return char_attr;
+  }
+
   // Find the entry for char_attr
   char_aep = syn_cterm_attr2entry(char_attr);

Without it there may be invalid reads when you call this function(after you rebase)

[bfredl]

@brcolow Thanks, updated!

[bfredl]

On a general note, please comment on the "Files changed" tab rather than a commit if possible; this makes the comment survive rebases and also makes github keep track which comments are addressed or not.

[bfredl]

I think this is starting to get quite RDY, I've added some argument validation and checked it works with multibyte text (it uses byte-based indexing for columns, just like buffer_get_mark).

[brcolow]

@bfredl Will do, thanks (didn't know about that difference).

[bfredl]

I will go ahead and merge this soon, if there is no more comments.

[fmoralesc]

Just in case: clint.py is complaining a bunch on kvec.h.

[bfredl]

yes, I asked about it above.

[justinmk]

don't worry about kvec.h lint errors.

[bfredl]

All test pass except for the kvec linter errors. might be some grammer error left in the docs, but let's merge this.