Adding multiple virtual text properties using the prop_add_list() function

[Yegappan Lakshmanan]

Hi,

Currently the prop_add_list() function doesn't support adding multiple virtual
text properties. This functionality is useful when applying the inlay
hints reply
received from a LSP server.

Should we extend the prop_add_list() function to support the "text" value in the
second argument? The second argument (list of items) needs to support the
following:

[{lnum}, {col}, {end-lnum}, {end-col}, "text"]

Should we make the "text_align", "text_padding_left" and "text_wrap" properties
common to all the text properties or support a dict for the list element:

[{lnum}, {col}, {end-lnum}, {end-col}, {'text': value, 'text_align': value,
'text_padding_left':
value, 'text'wrap': value}]

Regards,
Yegappan

[Bram Moolenaar]

Yegappan wrote:

> Currently the prop_add_list() function doesn't support adding multiple
> virtual text properties. This functionality is useful when applying
> the inlay hints reply received from a LSP server.

What is against calling prop_add() for each one?

prop_add_list() is intended to add the same type of property and a list
of positions. I don't expect you want to add the same text in a list of
positions? Unless it's some kind of error or marker.

> Should we extend the prop_add_list() function to support the "text"
> value in the second argument? The second argument (list of items)
> needs to support the following:
>
> [{lnum}, {col}, {end-lnum}, {end-col}, "text"]
>
> Should we make the "text_align", "text_padding_left" and "text_wrap"
> properties common to all the text properties or support a dict for the
> list element:
>
> [{lnum}, {col}, {end-lnum}, {end-col}, {'text': value, 'text_align': value,
> 'text_padding_left':
> value, 'text'wrap': value}]

This quickly gets complicated, since depending on what you are adding
some fields will be the same for all properties and some will be
different. I doubt the complexity is justified by calling one function
for multiple text properties instead of calling prop_add() for each one.

Is there a virtual text property with several fields that are the same
(padding, align, etc.) and only differ in position and text?

--
Although the scythe isn't pre-eminent among the weapons of war, anyone who
has been on the wrong end of, say, a peasants' revolt will know that in
skilled hands it is fearsome.
-- (Terry Pratchett, Mort)

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[Yegappan Lakshmanan]

Hi Bram,

On Sat, Nov 26, 2022 at 1:25 PM Bram Moolenaar <Br...@moolenaar.net> wrote:
>
>
> Yegappan wrote:
>
> > Currently the prop_add_list() function doesn't support adding multiple
> > virtual text properties. This functionality is useful when applying
> > the inlay hints reply received from a LSP server.
>
> What is against calling prop_add() for each one?
>

When adding a large number of text properties for inlay hints to a source file
with many lines, calling prop_add() multiple times may not be efficient.

>
> prop_add_list() is intended to add the same type of property and a list
> of positions. I don't expect you want to add the same text in a list of
> positions? Unless it's some kind of error or marker.
>

When adding the text properties for inlay hints, the text will be different
for each position.

>
> > Should we extend the prop_add_list() function to support the "text"
> > value in the second argument? The second argument (list of items)
> > needs to support the following:
> >
> > [{lnum}, {col}, {end-lnum}, {end-col}, "text"]
> >
> > Should we make the "text_align", "text_padding_left" and "text_wrap"
> > properties common to all the text properties or support a dict for the
> > list element:
> >
> > [{lnum}, {col}, {end-lnum}, {end-col}, {'text': value, 'text_align': value,
> > 'text_padding_left':
> > value, 'text'wrap': value}]
>
> This quickly gets complicated, since depending on what you are adding
> some fields will be the same for all properties and some will be
> different. I doubt the complexity is justified by calling one function
> for multiple text properties instead of calling prop_add() for each one.
>
> Is there a virtual text property with several fields that are the same
> (padding, align, etc.) and only differ in position and text?
>

No. The LSP specification for inlay hints is at:

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#inlayHint

The padding is specified for each inlay hint item.

Regards,
Yegappan

[Bram Moolenaar]

It also mentions a tooltip. I suppose we don't support that (yet).

Since this is an optimization, and the choices for implementation are
not directly clear, we should do this later perhaps. Using prop_add()
now should make clear how it works, even when it's a little bit slower.
Perhaps we might actually want to make it work better before making it
more efficient.

--
Keep America beautiful. Swallow your beer cans.

[bfrg]

Personally, I haven't measured a significant difference between prop_add() and
prop_add_list() for normal text-properties. For signs, on the other hand,
there's a significant difference between sign_place() in a for-loop and
sign_place_list().

But what I've noticed is that calling prop_remove() can be very slow when
removing over a thousand virtual text-properties.

For comparison: with normal text-properties, we can group them under a given
text-prop ID and use that ID in prop_remove() to remove a set of "related"
text-properties:

prop_remove({
    'id': group_id,
    'bufnr': some_bufnr,
    'type': 'my_prop_type',
    'both': true,
    'all': true
})

This is very efficient, even when removing 10'000 text-properties with the same
ID.

However, with virtual text-properties we cannot group them under the same ID
(why?), instead each virtual text will be assigned a unique ID. Thus, we need
to cache every ID returned by prop_add() and then remove them individually in a
for-loop, something like:

var virt_IDs: list<number>
# fill virt_IDs with IDs returned by prop_add() …

for i in virt_IDs
    prop_remove({
        'id': i,
        'bufnr': some_bufnr
    })
endfor

Since the IDs are unique within a buffer, we don't have to specify `type` or
`both` in `prop_remove()`. Nevertheless, removing virtual text this way is very
slow when several hundred properties need to be removed.

To give you an example, on my 10 year old laptop calling prop_remove() in a
for-loop to remove 10'000 text-properties takes over 2 seconds. If I remove the
'id' entry in prop_remove() and match by 'type' it takes only a few
milliseconds. It's also significantly faster when I add 'lnum' to prop_remove(),
but the problem is that lnum isn't known when removing the virtual-text since
the lines might have shifted.

I think removing a set of virtual text-properties is very common, for example,

when `align`, `wrap` or `text_padding_left` need to be toggled. The only way to

accomplish this is by removing the old ones and re-adding them with a different

'align', 'wrap' etc. value.

[Bram Moolenaar]

> Personally, I haven't measured a significant difference between prop_add()
> and prop_add_list() for normal text-properties. For signs, on the
> other hand, there's a significant difference between sign_place() in a
> for-loop and sign_place_list().
>
> But what I've noticed is that calling prop_remove() can be very slow when
> removing over a thousand virtual text-properties.
>
> For comparison: with normal text-properties, we can group them under a given
> text-prop ID and use that ID in prop_remove() to remove a set of "related"
> text-properties:
>
> prop_remove({
> 'id': group_id,
> 'bufnr': some_bufnr,
> 'type': 'my_prop_type',
> 'both': true,
> 'all': true
> })
>
> This is very efficient, even when removing 10'000 text-properties with
> the same ID.
>
> However, with virtual text-properties we cannot group them under the same ID
> (why?),

The text of the virtual text property is stored in an array, the ID is
used to lookup the text, thus it has to be unique.

> instead each virtual text will be assigned a unique ID. Thus, we need
> to cache every ID returned by prop_add() and then remove them
> individually in a for-loop, something like:
>
> var virt_IDs: list<number>
> # fill virt_IDs with IDs returned by prop_add() …
>
> for i in virt_IDs
> prop_remove({
> 'id': i,
> 'bufnr': some_bufnr
> })
> endfor
>
> Since the IDs are unique within a buffer, we don't have to specify
> `type` or `both` in `prop_remove()`. Nevertheless, removing virtual
> text this way is very slow when several hundred properties need to be
> removed.
>
> To give you an example, on my 10 year old laptop calling prop_remove() in a
> for-loop to remove 10'000 text-properties takes over 2 seconds. If I remove
> the 'id' entry in prop_remove() and match by 'type' it takes only a
> few milliseconds. It's also significantly faster when I add 'lnum' to
> prop_remove(), but the problem is that lnum isn't known when removing
> the virtual-text since the lines might have shifted.

Can you not delete all properties with a given type? Considering that
defining a type is not much work, you could use the type to group
properties together. You can define different type names with the same
highlighting. The only condition is that you know beforehand which
virtual text properties you want to remove at the same time.

> I think removing a set of virtual text-properties is very common, for
> example, when `align`, `wrap` or `text_padding_left` need to be
> toggled. The only way to accomplish this is by removing the old ones
> and re-adding them with a different 'align', 'wrap' etc. value.

--
In Joseph Heller's novel "Catch-22", the main character tries to get out of a
war by proving he is crazy. But the mere fact he wants to get out of the war
only shows he isn't crazy -- creating the original "Catch-22".