[vim/vim] Extend prop_list() to return properties across multiple lines (PR #9138)

[Yegappan Lakshmanan]

---

You can view, comment on, or merge this pull request online at:

  https://github.com/vim/vim/pull/9138

Commit Summary

• 1c9a1fa Extend prop_list() to return properties across multiple lines

File Changes

(3 files)

• M runtime/doc/textprop.txt (11)
• M src/testdir/test_textprop.vim (73)
• M src/textprop.c (113)

Patch Links:

• https://github.com/vim/vim/pull/9138.patch
• https://github.com/vim/vim/pull/9138.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.

[Yegappan Lakshmanan]

@yegappan pushed 1 commit.

• b1ac4b4 Extend prop_list() to return properties across multiple lines

—
You are receiving this because you are subscribed to this thread.

View it on GitHub.

[codecov[bot]]

Codecov Report

Merging #9138 (b1ac4b4) into master (b1b163e) will decrease coverage by 1.74%.
The diff coverage is 95.55%.

@@            Coverage Diff             @@

##           master    #9138      +/-   ##

==========================================

- Coverage   90.14%   88.40%   -1.75%     

==========================================

  Files         151      150       -1     

  Lines      169319   167732    -1587     

==========================================

- Hits       152637   148276    -4361     

- Misses      16682    19456    +2774     

Flag	Coverage Δ
huge-clang-none	?
huge-gcc-none	?
huge-gcc-testgui	88.36% <95.55%> (-0.01%)	⬇️
huge-gcc-unittests	2.45% <0.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/textprop.c	96.70% <95.55%> (-0.13%)	⬇️
src/libvterm/src/rect.h	0.00% <0.00%> (-96.56%)	⬇️
src/libvterm/src/mouse.c	0.00% <0.00%> (-48.28%)	⬇️
src/libvterm/src/state.c	49.10% <0.00%> (-40.86%)	⬇️
src/libvterm/include/vterm.h	0.00% <0.00%> (-37.50%)	⬇️
src/libvterm/src/pen.c	49.85% <0.00%> (-34.81%)	⬇️
src/libvterm/src/keyboard.c	54.73% <0.00%> (-33.69%)	⬇️
src/mouse.c	58.59% <0.00%> (-30.81%)	⬇️
src/libvterm/src/encoding.c	45.54% <0.00%> (-27.73%)	⬇️
src/libvterm/src/vterm.c	48.57% <0.00%> (-18.89%)	⬇️
... and 75 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1b163e...b1ac4b4. Read the comment docs.

[Bram Moolenaar]

Thanks for working on this.
The "type" option currently only supports one type name. The request was to use a pattern. Maybe that is a bit slow (each type ID needs to be looked up to find the name to match with). An alternative would be to pass a list of type names.

[Yegappan Lakshmanan]

@yegappan pushed 1 commit.

• 77c2369 Extend prop_list() to return properties across multiple lines

—
You are receiving this because you are subscribed to this thread.

View it on GitHub.

[Yegappan Lakshmanan]

@yegappan pushed 1 commit.

• 3931d03 Fix memory leak

—
You are receiving this because you are subscribed to this thread.

View it on GitHub.

[Yegappan Lakshmanan]

Hi Bram,

On Tue, Nov 16, 2021 at 8:51 AM Bram Moolenaar <vim-dev...@256bit.org> wrote:

Thanks for working on this.
The "type" option currently only supports one type name. The request was to use a pattern. Maybe that is a bit slow (each type ID needs to be looked up to find the name to match with). An alternative would be to pass a list of type names.

I have updated the PR to support specifying a list of property type names and identifiers

to prop_list().

- Yegappan

 

[vim-dev ML]

Hi Bram,

On Tue, Nov 16, 2021 at 8:51 AM Bram Moolenaar ***@***.***>

wrote:

> Thanks for working on this.
> The "type" option currently only supports one type name. The request was
> to use a pattern. Maybe that is a bit slow (each type ID needs to be looked
> up to find the name to match with). An alternative would be to pass a list
> of type names.
>
>
>
I have updated the PR to support specifying a list of property type names
and identifiers
to prop_list().

- Yegappan

[Bram Moolenaar]

Thanks for the update. I'll leave this open for a couple of days to await any comments.

—
You are receiving this because you commented.

[bfrg]

How do we get all text-properties of a specific buffer?

In the examples you use line('$') but the function doesn't accept a buffer number. Currently the only way to obtain the number of lines in a buffer is through getbufinfo(bufnr)[0].linecount but this is a bit heavy considering how much data getbufinfo() could return. So wouldn't it be more efficient to accept '$' directly in end_lnum?

For example: prop_list(1, {'bufnr': 4, 'end_lnum': '$', 'id': [123]}).

—
You are receiving this because you commented.

[Bram Moolenaar]

> How do we get all text-properties of a specific buffer?
>
> In the examples you use `line('$')` but the function doesn't accept a
> buffer number. Currently the only way to obtain the number of lines in
> a buffer is through `getbufinfo(bufnr)[0].linecount` but this is a bit
> heavy considering how much data `getbufinfo()` could return. So

> wouldn't it be more efficient to accept `'$'` directly in `end_lnum`?
>
> For example: `prop_list(1, {'bufnr': 4, 'end_lnum': '$', 'id': [123]})`.

We try to stay away from arguments with mixed types. Accepting both a
number and a string makes it harder to give good error messages.

Getting the number of lines in a buffer is a generic thing. line()
already takes an optional window ID argument. It's logical to also
accept a buffer number.

line({expr}, {winid}, {bufnr})

However, that is ambiguous, the {winid} would be ignored when using the
{bufnr}. Perhaps it's better to use:

line({expr}, {id}, {id-is-bufnr})

When the third argument is omitted or FALSE, the {id} is used as the
{winid} like before. When it is TRUE then {id} is used as {bufnr}.
So for your example you can use: line('$', bufnr, 1).
How about that?

An alternative would be to add a bufline() function. Advantage is that
we can drop the window related items, since a buffer by itself does not
have a cursor position or visible lines.

--
There are three kinds of people: Those who can count & those who can't.

/// 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 ///

[bfrg]

Hmm, Bram's comment didn't make it to github for some reasons. I thought emails were automatically mirrored.

I like the suggestion to add a new function getline() (or similar), since it could also be used in other places.

—
You are receiving this because you commented.

[Yegappan Lakshmanan]

@yegappan pushed 1 commit.

• 19558a8 If end_lnum is greater than the last buffer line, use the last buffer line

—
You are receiving this because you are subscribed to this thread.

View it on GitHub.

[Yegappan Lakshmanan]

Hi,

On Thu, Nov 18, 2021 at 4:49 AM bfrg <vim-dev...@256bit.org> wrote:

How do we get all text-properties of a specific buffer?

Thanks for the feedback. I have updated the PR to use the last buffer line number if

the end_lnum is greater than the last buffer line number. So you can use a very large

value (9999999) for end_lnum.

- Yegappan

[vim-dev ML]

Hi,

[Yegappan Lakshmanan]

Hi Bram,

On Thu, Nov 18, 2021 at 5:57 AM Bram Moolenaar <Br...@moolenaar.net> wrote:
>
>
> > How do we get all text-properties of a specific buffer?
> >
> > In the examples you use `line('$')` but the function doesn't accept a
> > buffer number. Currently the only way to obtain the number of lines in
> > a buffer is through `getbufinfo(bufnr)[0].linecount` but this is a bit
> > heavy considering how much data `getbufinfo()` could return. So
> > wouldn't it be more efficient to accept `'$'` directly in `end_lnum`?
> >
> > For example: `prop_list(1, {'bufnr': 4, 'end_lnum': '$', 'id': [123]})`.
>
> We try to stay away from arguments with mixed types. Accepting both a
> number and a string makes it harder to give good error messages.
>

The existing buffer related functions appendbufline(),
deletebufline(), getbufline()
and setbufline() already accept "$" to refer to the last line in a buffer.
I can modify prop_list() to also accept '$' for 'end_lnum'.

Regards,
Yegappan

[Yegappan Lakshmanan]

@yegappan pushed 1 commit.

• b95a536 Accept $ to refer to the last buffer line

—
You are receiving this because you are subscribed to this thread.

View it on GitHub.

[Bram Moolenaar]

> Hmm, Bram's comment didn't make it to github for some reasons. I
> thought emails were automatically mirrored.

I usually get two messages, one from github and one from the maillist.
If I respond to the wrong one it won't go to github.

> I like the suggestion to add a new function `getline()` (or similar),
> since it could also be used in other places.

getline() already exists, that is why I suggested bufline(). But using
a negative number would be simpler. -1 for the last line number.

--
Proofread carefully to see if you any words out.

[Bram Moolenaar]

> On Thu, Nov 18, 2021 at 5:57 AM Bram Moolenaar <Br...@moolenaar.net> wrote:
> >
> >
> > > How do we get all text-properties of a specific buffer?
> > >
> > > In the examples you use `line('$')` but the function doesn't accept a
> > > buffer number. Currently the only way to obtain the number of lines in
> > > a buffer is through `getbufinfo(bufnr)[0].linecount` but this is a bit
> > > heavy considering how much data `getbufinfo()` could return. So
> > > wouldn't it be more efficient to accept `'$'` directly in `end_lnum`?
> > >
> > > For example: `prop_list(1, {'bufnr': 4, 'end_lnum': '$', 'id': [123]})`.
> >
> > We try to stay away from arguments with mixed types. Accepting both a
> > number and a string makes it harder to give good error messages.
> >
>
> The existing buffer related functions appendbufline(),
> deletebufline(), getbufline()
> and setbufline() already accept "$" to refer to the last line in a buffer.
> I can modify prop_list() to also accept '$' for 'end_lnum'.

Yeah, we used to do this in the past, but now that we have added more
type checking it's probably better to stop doing it like this.

Using a very large number works, although it's not the nicest.
Perhaps we could use -1, as a negative number is relative to the end?
So -2 would be last-but-one line. Might be useful?
It's at least existing, see :help list-index.

--
Be nice to your kids... they'll be the ones choosing your nursing home.

[Yegappan Lakshmanan]

@yegappan pushed 1 commit.

• 60532fd Support negative numbers for end_lnum

—
You are receiving this because you are subscribed to this thread.

View it on GitHub.

[Yegappan Lakshmanan]

@yegappan pushed 4 commits.

• 53a156a Extend prop_list() to return properties across multiple lines
• feca84b If end_lnum is greater than the last buffer line, use the last buffer line
• e286439 Accept $ to refer to the last buffer line
• c248987 Support negative numbers for end_lnum

—
You are receiving this because you are subscribed to this thread.

View it on GitHub.

[Yegappan Lakshmanan]

Hi Bram,

On Thu, Nov 18, 2021 at 9:12 AM Bram Moolenaar <Br...@moolenaar.net> wrote:
>
>
> > On Thu, Nov 18, 2021 at 5:57 AM Bram Moolenaar <Br...@moolenaar.net> wrote:
> > >
> > >
> > > > How do we get all text-properties of a specific buffer?
> > > >
> > > > In the examples you use `line('$')` but the function doesn't accept a
> > > > buffer number. Currently the only way to obtain the number of lines in
> > > > a buffer is through `getbufinfo(bufnr)[0].linecount` but this is a bit
> > > > heavy considering how much data `getbufinfo()` could return. So
> > > > wouldn't it be more efficient to accept `'$'` directly in `end_lnum`?
> > > >
> > > > For example: `prop_list(1, {'bufnr': 4, 'end_lnum': '$', 'id': [123]})`.
> > >
> > > We try to stay away from arguments with mixed types. Accepting both a
> > > number and a string makes it harder to give good error messages.
> > >
> >
> > The existing buffer related functions appendbufline(),
> > deletebufline(), getbufline()
> > and setbufline() already accept "$" to refer to the last line in a buffer.
> > I can modify prop_list() to also accept '$' for 'end_lnum'.
>
> Yeah, we used to do this in the past, but now that we have added more
> type checking it's probably better to stop doing it like this.
>
> Using a very large number works, although it's not the nicest.
> Perhaps we could use -1, as a negative number is relative to the end?
> So -2 would be last-but-one line. Might be useful?
> It's at least existing, see :help list-index.
>

I have updated the PR to support negative numbers for end_lnum to refer
to a line from the end of the buffer.

- Yegappan

[Ben Jackson]

Ping @bstaletic

—
You are receiving this because you commented.

[Ben Jackson]

@puremourning commented on this pull request.

---

In src/textprop.c:

>  
-	    if (d == NULL)
-		break;
-	    mch_memmove(&prop, text + textlen + i * sizeof(textprop_T),
-							   sizeof(textprop_T));
-	    prop_fill_dict(d, &prop, buf);
-	    list_append_dict(rettv->vval.v_list, d);
+	    if (di->di_tv.vval.v_list == NULL)
+		return;
+
+	    prop_ids = get_prop_ids_from_list(di->di_tv.vval.v_list,
+		    &prop_ids_len);
+	    if (prop_ids == NULL)
+		return;

does this leak prop_types? There seem to be a few early returns that might leak one of prop_types or prop_ids. Worth double-checking ?

—
You are receiving this because you commented.

[Bram Moolenaar]

The name of the item for the list of types is "type". Now that it is a list it would be better to call this "types" or "typelist".
I think "types" is the best.

—
You are receiving this because you commented.

[Yegappan Lakshmanan]

@yegappan pushed 1 commit.

• ee9fea9 Fix memory leaks on error. Rename 'type' to 'types' and 'id' to 'ids'. Add more tests

—
You are receiving this because you are subscribed to this thread.

View it on GitHub.

[Yegappan Lakshmanan]

Hi,

On Fri, Nov 19, 2021 at 2:05 AM Ben Jackson <vim-dev...@256bit.org> wrote:

@puremourning commented on this pull request.

---

In src/textprop.c:

>  
-	    if (d == NULL)
-		break;
-	    mch_memmove(&prop, text + textlen + i * sizeof(textprop_T),
-							   sizeof(textprop_T));
-	    prop_fill_dict(d, &prop, buf);
-	    list_append_dict(rettv->vval.v_list, d);
+	    if (di->di_tv.vval.v_list == NULL)
+		return;
+
+	    prop_ids = get_prop_ids_from_list(di->di_tv.vval.v_list,
+		    &prop_ids_len);
+	    if (prop_ids == NULL)
+		return;

does this leak prop_types? There seem to be a few early returns that might leak one of prop_types or prop_ids. Worth double-checking ?

Yes. Thanks for catching this. The tests didn't detect this leak because only

'types' or 'ids' item is used in the test.  I have updated the PR with a fix for this

and added additional tests.

Regards,

Yegappan

[vim-dev ML]

Hi,

On Fri, Nov 19, 2021 at 2:05 AM Ben Jackson ***@***.***>
wrote:

> ***@***.**** commented on this pull request.
> ------------------------------
>
> In src/textprop.c
> <https://github.com/vim/vim/pull/9138#discussion_r753048618>:

>
> >
> - if (d == NULL)
> - break;
> - mch_memmove(&prop, text + textlen + i * sizeof(textprop_T),
> - sizeof(textprop_T));
> - prop_fill_dict(d, &prop, buf);
> - list_append_dict(rettv->vval.v_list, d);
> + if (di->di_tv.vval.v_list == NULL)
> + return;
> +
> + prop_ids = get_prop_ids_from_list(di->di_tv.vval.v_list,
> + &prop_ids_len);
> + if (prop_ids == NULL)
> + return;
>
> does this leak prop_types? There seem to be a few early returns that might
> leak one of prop_types or prop_ids. Worth double-checking ?
>
>
> Yes. Thanks for catching this. The tests didn't detect this leak because
only
'types' or 'ids' item is used in the test. I have updated the PR with a
fix for this
and added additional tests.

Regards,
Yegappan

—
You are receiving this because you are subscribed to this thread.

Reply to this email directly, view it on GitHub.

[Yegappan Lakshmanan]

Hi Bram,

On Fri, Nov 19, 2021 at 4:25 AM Bram Moolenaar
<vim-dev...@256bit.org> wrote:
>
> The name of the item for the list of types is "type". Now that it is a list it would be better to call this "types" or "typelist".
> I think "types" is the best.
>

I have updated the PR to rename "type" to "types" and "id" to "ids".

- Yegappan

[vim-dev ML]

[Yegappan Lakshmanan]

@yegappan pushed 1 commit.

• b6f0c85 Extend prop_list() to return properties across multiple lines

—
You are receiving this because you are subscribed to this thread.

View it on GitHub.

[Yegappan Lakshmanan]

@yegappan pushed 1 commit.

• 55fae8e Add Vim9 tests

[Yegappan Lakshmanan]

@yegappan pushed 1 commit.

• 149e0ff Extend prop_list() to return properties across multiple lines

[Yegappan Lakshmanan]

@yegappan pushed 1 commit.

• 039c66b Extend prop_list() to return properties across multiple lines

[Bram Moolenaar]

Closed #9138 via e021662.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub.