[vim/vim] Add support for prop_add_list() (#8751)

[Yegappan Lakshmanan]

Closes #8675.

---

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

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

Commit Summary

• Add support for prop_add_list()

File Changes

• M runtime/doc/textprop.txt (31)
• M runtime/doc/usr_41.txt (1)
• M src/evalfunc.c (3)
• M src/proto/textprop.pro (1)
• M src/testdir/test_textprop.vim (27)
• M src/textprop.c (233)

Patch Links:

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

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

[Yegappan Lakshmanan]

@yegappan pushed 1 commit.

• 5de649d Fix test failure

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

View it on GitHub or unsubscribe.

[codecov[bot]]

Codecov Report

Merging #8751 (bb04068) into master (92f05f2) will decrease coverage by 87.68%.
The diff coverage is 0.00%.

❗ Current head bb04068 differs from pull request most recent head 5de649d. Consider uploading reports for the commit 5de649d to get more accurate results

@@             Coverage Diff             @@

##           master    #8751       +/-   ##

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

- Coverage   90.14%    2.46%   -87.69%     

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

  Files         151      149        -2     

  Lines      170590   165370     -5220     

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

- Hits       153780     4072   -149708     

- Misses      16810   161298   +144488     

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

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

Impacted Files	Coverage Δ
src/evalfunc.c	0.00% <ø> (-96.38%)	⬇️
src/textprop.c	0.00% <0.00%> (-96.82%)	⬇️
src/float.c	0.00% <0.00%> (-99.22%)	⬇️
src/gui_gtk_f.c	0.00% <0.00%> (-97.54%)	⬇️
src/sound.c	0.00% <0.00%> (-97.12%)	⬇️
src/crypt_zip.c	0.00% <0.00%> (-97.06%)	⬇️
src/match.c	0.00% <0.00%> (-96.98%)	⬇️
src/sha256.c	0.00% <0.00%> (-96.94%)	⬇️
src/evalbuffer.c	0.00% <0.00%> (-96.92%)	⬇️
src/cmdhist.c	0.00% <0.00%> (-96.76%)	⬇️
... and 139 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 92f05f2...5de649d. Read the comment docs.

[Yegappan Lakshmanan]

Hi,

On Thu, Aug 12, 2021 at 10:58 PM Yegappan Lakshmanan <vim-dev...@256bit.org> wrote:

Closes #8675.

---

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

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

Commit Summary

• Add support for prop_add_list()

The current implementation of the prop_add_list() function supports adding

a single text property to multiple locations in a buffer. Should we modify

this to support adding multiple text properties in a single call?

prop_add_list([[{prop1}, [<list of locations>]], [{prop2}, [<list of locations>], ...])

- Yegappan

[vim-dev ML]

Hi,

On Thu, Aug 12, 2021 at 10:58 PM Yegappan Lakshmanan <

***@***.***> wrote:

> Closes #8675 <https://github.com/vim/vim/issues/8675>.
> ------------------------------

> You can view, comment on, or merge this pull request online at:
>
> https://github.com/vim/vim/pull/8751
> Commit Summary
>

> - Add support for prop_add_list()

>
>
> The current implementation of the prop_add_list() function supports adding
a single text property to multiple locations in a buffer. Should we modify
this to support adding multiple text properties in a single call?

prop_add_list([[{prop1}, [<list of locations>]], [{prop2}, [<list of
locations>], ...])

- Yegappan

[Yegappan Lakshmanan]

@yegappan pushed 1 commit.

• 4ada76b Add vim9 test for prop_add_list()

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

View it on GitHub or unsubscribe.

[Dominique Pellé]

@dpelle commented on this pull request.

---

In src/textprop.c:

> +	linenr_T    start_lnum,
+	colnr_T	    start_col,
+	dict_T	    *dict,
+	buf_T	    *default_buf,
+	typval_T    *dict_arg)
+{
+    linenr_T	end_lnum;
+    colnr_T	end_col;
+    char_u	*type_name;
+    buf_T	*buf = default_buf;
+    int		id = 0;
+
+    if (dict == NULL || dict_find(dict, (char_u *)"type", -1) == NULL)
+    {
+	emsg(_("E965: missing property type name"));
+	return;

Indentation is wrong here. It's also wrong in several other lines below (e.g. line 389)

---

In src/testdir/test_textprop.vim:

> @@ -339,6 +339,33 @@ func Test_prop_add()
   bwipe!
 endfunc
 
+" Test for the prop_add_list() function
+func Test_prop_add_list()
+  new
+  call AddPropTypes()
+  call setline(1, ['one one one', 'two two two', 'six six six'])
+  call prop_add_list(#{type: 'one', id: 2}, [[1, 1, 3], [2, 5, 7], [3, 9, 11]])
+  call assert_equal([#{id: 2, col: 1, type_bufnr: 0, end: 1, type: 'one',
+        \ length: 2, start: 1}], prop_list(1))
+  call assert_equal([#{id: 2, col: 5, type_bufnr: 0, end: 1, type: 'one',
+        \ length: 2, start: 1}], prop_list(2))
+  call assert_equal([#{id: 2, col: 9, type_bufnr: 0, end: 1, type: 'one',
+        \ length: 2, start: 1}], prop_list(3))
+  call assert_fails('call prop_add_list([1, 2], [[1, 1, 3]])', 'E1206:')
+  call assert_fails('call prop_add_list({}, {})', 'E1211:')

Also test with:

• test_null_list()?
• a list that contains something else that a lists e.g. [1, 'a']

—
You are receiving this because you commented.

[bfrg]

I find it awkward that multiline text-properties cannot be handled with this function. Is there really such a drastic slowdown when the function is changed to prop_add_list({props}, [[{lnum}, {col}, {end-lnum}, {end-col}], ...])?

—
You are receiving this because you commented.

[Yegappan Lakshmanan]

Hi,

Thanks for the comments. See below.

On Fri, Aug 13, 2021 at 12:37 AM Dominique Pellé <vim-dev...@256bit.org> wrote:

@dpelle commented on this pull request.

---

In src/textprop.c:

> +	linenr_T    start_lnum,
+	colnr_T	    start_col,
+	dict_T	    *dict,
+	buf_T	    *default_buf,
+	typval_T    *dict_arg)
+{
+    linenr_T	end_lnum;
+    colnr_T	end_col;
+    char_u	*type_name;
+    buf_T	*buf = default_buf;
+    int		id = 0;
+
+    if (dict == NULL || dict_find(dict, (char_u *)"type", -1) == NULL)
+    {
+	emsg(_("E965: missing property type name"));
+	return;

Indentation is wrong here. It's also wrong in several other lines below (e.g. line 389)

I checked the indentation in the file and it looks correct to me.

 

---

In src/testdir/test_textprop.vim:

> @@ -339,6 +339,33 @@ func Test_prop_add()
   bwipe!
 endfunc
 
+" Test for the prop_add_list() function
+func Test_prop_add_list()
+  new
+  call AddPropTypes()
+  call setline(1, ['one one one', 'two two two', 'six six six'])
+  call prop_add_list(#{type: 'one', id: 2}, [[1, 1, 3], [2, 5, 7], [3, 9, 11]])
+  call assert_equal([#{id: 2, col: 1, type_bufnr: 0, end: 1, type: 'one',
+        \ length: 2, start: 1}], prop_list(1))
+  call assert_equal([#{id: 2, col: 5, type_bufnr: 0, end: 1, type: 'one',
+        \ length: 2, start: 1}], prop_list(2))
+  call assert_equal([#{id: 2, col: 9, type_bufnr: 0, end: 1, type: 'one',
+        \ length: 2, start: 1}], prop_list(3))
+  call assert_fails('call prop_add_list([1, 2], [[1, 1, 3]])', 'E1206:')
+  call assert_fails('call prop_add_list({}, {})', 'E1211:')

Also test with:

• test_null_list()?

I have added a new test for this.

 

• a list that contains something else that a lists e.g. [1, 'a']

There is already a test for using a List with non-number items a few lines below.

Regards,

Yegappan 

[vim-dev ML]

Hi,

Thanks for the comments. See below.

On Fri, Aug 13, 2021 at 12:37 AM Dominique Pellé ***@***.***>
wrote:

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

>
> > + linenr_T start_lnum,
> + colnr_T start_col,
> + dict_T *dict,
> + buf_T *default_buf,
> + typval_T *dict_arg)
> +{
> + linenr_T end_lnum;
> + colnr_T end_col;
> + char_u *type_name;
> + buf_T *buf = default_buf;
> + int id = 0;
> +
> + if (dict == NULL || dict_find(dict, (char_u *)"type", -1) == NULL)
> + {
> + emsg(_("E965: missing property type name"));
> + return;
>
> Indentation is wrong here. It's also wrong in several other lines below
> (e.g. line 389)
>

I checked the indentation in the file and it looks correct to me.

> ------------------------------
>
> In src/testdir/test_textprop.vim
> <https://github.com/vim/vim/pull/8751#discussion_r688307163>:

>
> > @@ -339,6 +339,33 @@ func Test_prop_add()
> bwipe!
> endfunc
>
> +" Test for the prop_add_list() function
> +func Test_prop_add_list()
> + new
> + call AddPropTypes()
> + call setline(1, ['one one one', 'two two two', 'six six six'])
> + call prop_add_list(#{type: 'one', id: 2}, [[1, 1, 3], [2, 5, 7], [3, 9, 11]])
> + call assert_equal([#{id: 2, col: 1, type_bufnr: 0, end: 1, type: 'one',
> + \ length: 2, start: 1}], prop_list(1))
> + call assert_equal([#{id: 2, col: 5, type_bufnr: 0, end: 1, type: 'one',
> + \ length: 2, start: 1}], prop_list(2))
> + call assert_equal([#{id: 2, col: 9, type_bufnr: 0, end: 1, type: 'one',
> + \ length: 2, start: 1}], prop_list(3))
> + call assert_fails('call prop_add_list([1, 2], [[1, 1, 3]])', 'E1206:')
> + call assert_fails('call prop_add_list({}, {})', 'E1211:')
>
> Also test with:
>

> - test_null_list()?

>
> I have added a new test for this.


>

> - a list that contains something else that a lists e.g. [1, 'a']

>
>
> There is already a test for using a List with non-number items a few lines
below.

Regards,
Yegappan

[Yegappan Lakshmanan]

Hi,

On Fri, Aug 13, 2021 at 5:18 AM bfrg <vim-dev...@256bit.org> wrote:
>
> I find it awkward that multiline text-properties cannot be handled with this function.
> Is there really such a drastic slowdown when the function is changed to
> prop_add_list({props}, [[{lnum}, {col}, {end-lnum}, {end-col}], ...])?
>

Adding one more item to the List will not slow down the function.
The user now needs to pass a four item List instead of a three item
List. If the common case is single line text properties, then in most
use cases, lnum and end-lnum will be the same.

Based on Bram's comment, I thought if multi-line text property is needed,
then prop_add() should be used?

- Yegappan

[vim-dev ML]

Hi,

[Yegappan Lakshmanan]

@yegappan pushed 1 commit.

• 7a770ac Add test for null arguments

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

View it on GitHub or unsubscribe.

[Bram Moolenaar]

Even though it's very likely that prop_add_list() will only be used for
single line items, and one can always fall back to calling prop_add()
multiple times, there is the argument of what users expect. It's a bit
of a surprise if something is left out for unclear reasons. Especially
when adding it doesn't really have a negative effect.

So we could make it [lnum, col, end_lnum, end_col]. Since this would be
filled in by a script, setting end_lnum equal to lnum won't be much
hassle, right?

--
Vi beats Emacs to death, and then again!
http://linuxtoday.com/stories/5764.html

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

[vim-dev ML]

> On Fri, Aug 13, 2021 at 5:18 AM bfrg ***@***.***> wrote:
> >
> > I find it awkward that multiline text-properties cannot be handled with this function.
> > Is there really such a drastic slowdown when the function is changed to
> > prop_add_list({props}, [[{lnum}, {col}, {end-lnum}, {end-col}], ...])?
> >
>
> Adding one more item to the List will not slow down the function.
> The user now needs to pass a four item List instead of a three item
> List. If the common case is single line text properties, then in most
> use cases, lnum and end-lnum will be the same.
>
> Based on Bram's comment, I thought if multi-line text property is needed,
> then prop_add() should be used?

Even though it's very likely that prop_add_list() will only be used for
single line items, and one can always fall back to calling prop_add()
multiple times, there is the argument of what users expect. It's a bit
of a surprise if something is left out for unclear reasons. Especially
when adding it doesn't really have a negative effect.

So we could make it [lnum, col, end_lnum, end_col]. Since this would be
filled in by a script, setting end_lnum equal to lnum won't be much
hassle, right?

--
Vi beats Emacs to death, and then again!
http://linuxtoday.com/stories/5764.html

/// Bram Moolenaar -- ***@***.*** -- 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]

@yegappan pushed 1 commit.

• 4a78b78 Support end line number

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

View it on GitHub or unsubscribe.

[Yegappan Lakshmanan]

Hi Bram,

On Fri, Aug 13, 2021 at 8:49 AM Bram Moolenaar <Br...@moolenaar.net> wrote:
>
>
> > On Fri, Aug 13, 2021 at 5:18 AM bfrg <vim-dev...@256bit.org> wrote:
> > >
> > > I find it awkward that multiline text-properties cannot be handled with this function.
> > > Is there really such a drastic slowdown when the function is changed to
> > > prop_add_list({props}, [[{lnum}, {col}, {end-lnum}, {end-col}], ...])?
> > >
> >
> > Adding one more item to the List will not slow down the function.
> > The user now needs to pass a four item List instead of a three item
> > List. If the common case is single line text properties, then in most
> > use cases, lnum and end-lnum will be the same.
> >
> > Based on Bram's comment, I thought if multi-line text property is needed,
> > then prop_add() should be used?
>
> Even though it's very likely that prop_add_list() will only be used for
> single line items, and one can always fall back to calling prop_add()
> multiple times, there is the argument of what users expect. It's a bit
> of a surprise if something is left out for unclear reasons. Especially
> when adding it doesn't really have a negative effect.
>
> So we could make it [lnum, col, end_lnum, end_col]. Since this would be
> filled in by a script, setting end_lnum equal to lnum won't be much
> hassle, right?
>

I have updated the PR with the support for specifying the end line number.

Should we also support specifying multiple text properties in a single call?

prop_add_list([[{prop1}, [<locations]], [{prop2}, [<locations>]], ....])

Regards,
Yegappan

[vim-dev ML]

Hi Bram,

On Fri, Aug 13, 2021 at 8:49 AM Bram Moolenaar ***@***.***> wrote:

[Bram Moolenaar]

That adds more dictionaries, which is less efficient and results in a
mix of types, which is more complicated. I think we should stick to
using one {props} argument that applies to the list of locations.
If we really need more, we could add another function.

--
TIM: Too late.
ARTHUR: What?
TIM: There he is!
[They all turn, and see a large white RABBIT lollop a few yards out of the
cave. Accompanied by terrifying chord and jarring metallic monster noise.]
"Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\

[vim-dev ML]

Yegappan wrote:

> On Fri, Aug 13, 2021 at 8:49 AM Bram Moolenaar ***@***.***> wrote:

/// Bram Moolenaar -- ***@***.*** -- 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,

I think I didn't provide a clear description. I meant, to add multiple
text properties in several locations, instead of calling the
prop_add_list() function multiple times (one for each text property
and a list of locations), we can change the prop_add_list() function
to accept a single List. In this List, each item is a List with the text
property {prop} and a list of locations. This can help in reducing
the number of calls to prop_add_list().

Regards,
Yegappan

[vim-dev ML]

Hi Bram,

On Fri, Aug 13, 2021 at 10:46 AM Bram Moolenaar ***@***.***> wrote:
>
>
> Yegappan wrote:
>

> > On Fri, Aug 13, 2021 at 8:49 AM Bram Moolenaar ***@***.***> wrote:

[Yegappan Lakshmanan]

@yegappan pushed 1 commit.

• c74acf4 Add support for prop_add_list()

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

View it on GitHub or unsubscribe.

[Bram Moolenaar]

I understand. This is starting to look a lot like a sequence of calls
to prop_add(). That takes away lot of the "added value" of the
function. The idea is to add properties with the same attributes in a
list of positions, thus avoiding to list the properties multiple times.

Potential users of this function: Please comment.

--
If "R" is Reverse, how come "D" is FORWARD?

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\

[vim-dev ML]

Yegappan wrote:

> > > On Fri, Aug 13, 2021 at 8:49 AM Bram Moolenaar ***@***.***> wrote:

/// Bram Moolenaar -- ***@***.*** -- 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]

@yegappan pushed 1 commit.

• 329fedb Add support for prop_add_list()

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

View it on GitHub or unsubscribe.

[Bram Moolenaar]

Since there are no recent comments I'll include this now.

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

[Bram Moolenaar]

Closed #8751 via ccfb7c6.