[vim/vim] Proposal: sign_setlist (#4557)

[Paul Jolly]

Is your feature request related something that is currently hard to do? Please describe.

Per https://groups.google.com/d/msg/vim_dev/aW2buOgZ6qI/fpxkp-V8AQAJ

In https://github.com/leitzler/govim (master branch) Pontus is adding support to govim for signs that correspond to quickfix entries.

This is done by calling sign_place/sign_unplace as required to get the signs for a buffer in the correct state.

If there are lots of errors in a file, then there can be a large number of calls to sign_place/sign_unplace made by govim in rapid succession.

Describe the solution you'd like

A function:

sign_setlist({bfnr}, {list} [, {action}])

that sets the signs for buffer {bufnr} to {list}. {action} is similar to setqflist in that the caller can specify whether to create, replace or add to existing signs.

Describe alternatives you've considered

The only apparent alternative at the moment is to call sign_getplaced and then make the necessary calls to sign_place/sign_unplace; over a channel this becomes expensive.

Additional context

n/a

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

[Yegappan Lakshmanan]

Hi,

On Tue, Jun 18, 2019 at 2:37 AM Paul Jolly <vim-dev...@256bit.org> wrote:
>
> Is your feature request related something that is currently hard to do? Please describe.
>
> Per https://groups.google.com/d/msg/vim_dev/aW2buOgZ6qI/fpxkp-V8AQAJ
>
> In https://github.com/leitzler/govim (master branch) Pontus is adding support to govim for signs that correspond to quickfix entries.
>
> This is done by calling sign_place/sign_unplace as required to get the signs for a buffer in the correct state.
>
> If there are lots of errors in a file, then there can be a large number of calls to sign_place/sign_unplace made by govim in rapid succession.
>
> Describe the solution you'd like
>
> A function:
>
> sign_setlist({bfnr}, {list} [, {action}])
>
> that sets the signs for buffer {bufnr} to {list}. {action} is similar to setqflist in that the caller can specify whether to create, replace or add to existing signs.
>

Instead of adding a new function for placing multiple signs, what
about modifying the
existing sign_place() function to accept a List argument. Each list
item is a dictionary
specifying the attributes of a sign. We can do a similar change to the
sign_unplace()
function to remove a placed sign.

Thinking about this some more, we should modify the existing
sign_place() function
to always accept a List argument. This will simplify the function. This won't be
backward compatible. But the sign functions were recently added. So I am not
sure how many plugins have started using the sign_place() function.

- Yegappan

[vim-dev ML]

Hi,

On Tue, Jun 18, 2019 at 2:37 AM Paul Jolly <vim-dev...@256bit.org> wrote:
>

> Is your feature request related something that is currently hard to do? Please describe.
>
> Per https://groups.google.com/d/msg/vim_dev/aW2buOgZ6qI/fpxkp-V8AQAJ
>
> In https://github.com/leitzler/govim (master branch) Pontus is adding support to govim for signs that correspond to quickfix entries.
>
> This is done by calling sign_place/sign_unplace as required to get the signs for a buffer in the correct state.
>
> If there are lots of errors in a file, then there can be a large number of calls to sign_place/sign_unplace made by govim in rapid succession.
>
> Describe the solution you'd like
>
> A function:
>
> sign_setlist({bfnr}, {list} [, {action}])
>
> that sets the signs for buffer {bufnr} to {list}. {action} is similar to setqflist in that the caller can specify whether to create, replace or add to existing signs.
>

Instead of adding a new function for placing multiple signs, what
about modifying the
existing sign_place() function to accept a List argument. Each list
item is a dictionary
specifying the attributes of a sign. We can do a similar change to the
sign_unplace()
function to remove a placed sign.

Thinking about this some more, we should modify the existing
sign_place() function
to always accept a List argument. This will simplify the function. This won't be
backward compatible. But the sign functions were recently added. So I am not
sure how many plugins have started using the sign_place() function.

- Yegappan

>

[Paul Jolly]

Thinking about this some more, we should modify the existing sign_place() function to always accept a List argument

Just to say, I don't have any strong feelings either way. I'll defer to those with more experience of the Vim API and how changes are made.

—
You are receiving this because you commented.

[Bram Moolenaar]

> > Thinking about this some more, we should modify the existing
> > sign_place() function to always accept a List argument
>
> Just to say, I don't have any strong feelings either way. I'll defer
> to those with more experience of the Vim API and how changes are made.

The current sign_place() function is basically mimicking ":sign place".
One of the quircks is that it takes a file name, not a buffer number.
It also has an optional {id}, where zero means it's not passed in.

Adding a new function that takes a list of dictionaries could be a good
alternative. We then do need to validate the entries to check if the
required keys are included. It will then be possible to either pass a
buffer number or a file name.

--
Facepalm reply #9: "Speed up, you can drive 80 here" "Why, the cars behind us
are also driving 60"

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

—
You are receiving this because you commented.

[Yegappan Lakshmanan]

Hi Bram,

On Sat, Jun 29, 2019 at 5:57 AM Bram Moolenaar
<vim-dev...@256bit.org> wrote:
>
>
> > > Thinking about this some more, we should modify the existing
> > > sign_place() function to always accept a List argument
> >
> > Just to say, I don't have any strong feelings either way. I'll defer
> > to those with more experience of the Vim API and how changes are made.
>
> The current sign_place() function is basically mimicking ":sign place".

> One of the quirks is that it takes a file name, not a buffer number.
>

The sign_place() function accepts either a buffer name or a buffer
number. The {expr} argument to sign_place() is same as the {expr}
argument to bufname().

>
> It also has an optional {id}, where zero means it's not passed in.
>

Yes. Converting sign_place() to accept a List of dicts will make this
simpler.

>
> Adding a new function that takes a list of dictionaries could be a good
> alternative. We then do need to validate the entries to check if the
> required keys are included. It will then be possible to either pass a
> buffer number or a file name.
>

I have the changes for this ready to go (including the check for existence
of the required keys). I will merge my local changes with the recent
changes to sign.c/evalfunc.c and create a pull request.

- Yegappan

[vim-dev ML]

[Yegappan Lakshmanan]

Hi,

I have created PR #4602 (https://github.com/vim/vim/pull/4602) for this.
I have simplified the sign_place(), sign_unplace() and sign_getplaced()
functions. Took some time to make these changes as the sign tests heavily
use these functions. Note that this change is not backward compatible.

- Yegappan

[vim-dev ML]

[Bram Moolenaar]

Yegappan wrote:

> > > > > Thinking about this some more, we should modify the existing
> > > > > sign_place() function to always accept a List argument
> > > >
> > > > Just to say, I don't have any strong feelings either way. I'll defer
> > > > to those with more experience of the Vim API and how changes are made.
> > >
> > > The current sign_place() function is basically mimicking ":sign place".
> > > One of the quirks is that it takes a file name, not a buffer number.
> > >
> >
> > The sign_place() function accepts either a buffer name or a buffer
> > number. The {expr} argument to sign_place() is same as the {expr}
> > argument to bufname().
> >
> > >
> > > It also has an optional {id}, where zero means it's not passed in.
> > >
> >
> > Yes. Converting sign_place() to accept a List of dicts will make this
> > simpler.
> >
> > >
> > > Adding a new function that takes a list of dictionaries could be a good
> > > alternative. We then do need to validate the entries to check if the
> > > required keys are included. It will then be possible to either pass a
> > > buffer number or a file name.
> > >
> >
> > I have the changes for this ready to go (including the check for existence
> > of the required keys). I will merge my local changes with the recent
> > changes to sign.c/evalfunc.c and create a pull request.
> >
>
> I have created PR #4602 (https://github.com/vim/vim/pull/4602) for this.
> I have simplified the sign_place(), sign_unplace() and sign_getplaced()
> functions. Took some time to make these changes as the sign tests heavily
> use these functions. Note that this change is not backward compatible.

The sign functions were added more than half a year ago. I don't think
a change that is not backward compatible will be appreciated.

I think for sign_define() we can support both the old arguments and a
list argument. for sign_getplaced() we don't need changes. For
place/unplace add a new "list" function:

sign_define({name} [, {dict}])
sign_define({list})

sign_getplaced([{expr} [, {dict}]])

sign_place({id}, {group}, {name}, {expr} [, {dict}])
sign_placelist({list})

sign_unplace({group} [, {dict}])
sign_unplacelist({list})

Does that sound reasonable?

--
An extraordinary TALL KNIGHT in all black (possibly John with Mike on his
shoulders) walks out from the dark trees. He is extremely fierce and
gruesome countenance. He walks towards KING ARTHUR and PATSY, who are
wazzing like mad. (Salopian slang, meaning very scared. almost to the
point of wetting oneself, e.g. before an important football match or
prior to a postering. Salopian slang meaning a beating by the school
praeposters. Sorry about the Salopian slant to this stage direction - Ed.)
"Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD

[vim-dev ML]

[Yegappan Lakshmanan]

Hi Bram,

Instead of adding the new sign_placelist() and sign_unplacelist() functions,
how about modifying the existing sign_place() and sign_unplace() functions
to also accept a {list} argument (similar to the sign_define() function above)?
So the new set of functions will be like below:

sign_define({name} [, {dict}])
sign_define({list})

sign_getplaced([{expr} [, {dict}]])

sign_place({id}, {group}, {name}, {expr} [, {dict}])

sign_place({list})

sign_unplace({group} [, {dict}])

sign_unplace({list})

- Yegappan

[vim-dev ML]

[Paul Jolly]

My only comment here would be that sign_setlist as proposed in my original description not only handles setting of signs but also unsetting. That is, it ensures the buffer's sign list reflects the argument. Because in the context of my example I have the list of errors for a buffer (the same list that populates the quickfix window) and I simply want to say "now ensure the signs for this buffer are this list X", just as I call setqflist to update the quickfix window.

So from a selfish perspective I'd prefer not to have to keep track of/derive the delta to be applied, I'd like to just call a function that does all of that for me.

—
You are receiving this because you commented.

[Bram Moolenaar]

Hmm, it would work, of course, but is it better? I don't think so. The
documentation for sign_place() would be two complete different sections,
one explaining the first set of arguments and one explaining the {list}
argument. That doesn't make it simpler for the user.

It's true that having both sign_place() and sign_placelist() makes the
list of functions longer, but since the meaning is obvious I don't think
it matters much.

--
'I generally avoid temptation unless I can't resist it."
-- Mae West

[Bram Moolenaar]

> My only comment here would be that `sign_setlist` as proposed in my
> original description not only handles setting of signs but also
> unsetting. That is, it ensures the buffer's sign list reflects the
> argument. Because in the context of my example I have the list of
> errors for a buffer (the same list that populates the quickfix window)
> and I simply want to say "now ensure the signs for this buffer are
> this list X", just as I call `setqflist` to update the quickfix
> window.
>
> So from a selfish perspective I'd prefer not to have to keep track
> of/derive the delta to be applied, I'd like to just call a function
> that does all of that for me.

That's just one use case. It's easy enough to clear all signs and then
add a list of signs. Since we need a function to add a list and a
function clear all signs, also having a function for setting a list
would duplicate functionality.


--
Proverb: A nightingale that forgets the lyrics is a hummingbird.

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

—
You are receiving this because you commented.

[vim-dev ML]

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

—

[Paul Jolly]

That's just one use case.

Yes, indeed: quite accept that I'm only speaking about my particular use case here.

The reason behind me being attracted to this approach is that it makes it simpler from a channel-based plugin to do everything with a single call (reduces round trips), which also means less flicker for the user. As I understand it, with two calls (clear then set) there is potential (likely?) for flickering.

—
You are receiving this because you commented.

[Bram Moolenaar]

Paul Jolly wrote:

> > That's just one use case.
>
> Yes, indeed: quite accept that I'm only speaking about my particular
> use case here.
>
> The reason behind me being attracted to this approach is that it makes
> it simpler from a channel-based plugin to do everything with a single
> call (reduces round trips), which also means less flicker for the
> user. As I understand it, with two calls (clear then set) there is
> potential (likely?) for flickering.

It would do the same thing, only with one function instead of two.
So long as Vim keeps busy there will be no redraw. If you pause, then
yes. Why would there be an extra round trip? The function calls can be
in one message, right?

--
Futility Factor: No experiment is ever a complete failure - it can always
serve as a negative example.

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

—
You are receiving this because you commented.

[Paul Jolly]

The function calls can be in one message, right?

Very true.

—
You are receiving this because you commented.

[Yegappan Lakshmanan]

Hi Bram,

On Mon, Jul 1, 2019 at 1:38 PM Bram Moolenaar <Br...@moolenaar.net> wrote:
>
> >

> > I have created PR #4602 (https://github.com/vim/vim/pull/4602) for this.
> > I have simplified the sign_place(), sign_unplace() and sign_getplaced()
> > functions. Took some time to make these changes as the sign tests heavily
> > use these functions. Note that this change is not backward compatible.
>
> The sign functions were added more than half a year ago. I don't think
> a change that is not backward compatible will be appreciated.
>
> I think for sign_define() we can support both the old arguments and a
> list argument. for sign_getplaced() we don't need changes. For
> place/unplace add a new "list" function:
>
> sign_define({name} [, {dict}])
> sign_define({list})
>
> sign_getplaced([{expr} [, {dict}]])
>
> sign_place({id}, {group}, {name}, {expr} [, {dict}])
> sign_placelist({list})
>
> sign_unplace({group} [, {dict}])
> sign_unplacelist({list})
>
> Does that sound reasonable?
>

I have created PR #4636 that implements the above functions.

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

- Yegappan

[vim-dev ML]

[Bram Moolenaar]

Thanks! It looks good. I'll wait a couple of days to give others a
chance to comment.

--
ARTHUR: Shut up! Will you shut up!
DENNIS: Ah, now we see the violence inherent in the system.
ARTHUR: Shut up!
DENNIS: Oh! Come and see the violence inherent in the system!
HELP! HELP! I'm being repressed!
The Quest for the Holy Grail (Monty Python)

[vim-dev ML]

[Yegappan Lakshmanan]

Hi all,

On Tue, Jul 9, 2019 at 12:53 PM Bram Moolenaar <Br...@moolenaar.net> wrote:
>
> Yegappan wrote:
>
> > On Mon, Jul 1, 2019 at 1:38 PM Bram Moolenaar <Br...@moolenaar.net> wrote:
> > >
> > > >
> > > > I have created PR #4602 (https://github.com/vim/vim/pull/4602) for this.
> > > > I have simplified the sign_place(), sign_unplace() and sign_getplaced()
> > > > functions. Took some time to make these changes as the sign tests heavily
> > > > use these functions. Note that this change is not backward compatible.
> > >
> > > The sign functions were added more than half a year ago. I don't think
> > > a change that is not backward compatible will be appreciated.
> > >
> > > I think for sign_define() we can support both the old arguments and a
> > > list argument. for sign_getplaced() we don't need changes. For
> > > place/unplace add a new "list" function:
> > >
> > > sign_define({name} [, {dict}])
> > > sign_define({list})
> > >
> > > sign_getplaced([{expr} [, {dict}]])
> > >
> > > sign_place({id}, {group}, {name}, {expr} [, {dict}])
> > > sign_placelist({list})
> > >
> > > sign_unplace({group} [, {dict}])
> > > sign_unplacelist({list})
> > >
> > > Does that sound reasonable?
> > >
> >
> > I have created PR #4636 that implements the above functions.
> >
> > https://github.com/vim/vim/pull/4636
>
> Thanks! It looks good. I'll wait a couple of days to give others a
> chance to comment.
>

I am including the documentation update from the PR below.
Let me know if you have any comments on the new functionality.

- Yegappan

--------------------------------------------------------------------------------------------------

sign_define({name} [, {dict}])
sign_define({list})

Define a new sign named {name} or modify the attributes of an
existing sign. This is similar to the |:sign-define| command.

Prefix {name} with a unique text to avoid name collisions.
There is no {group} like with placing signs.

The {name} can be a String or a Number. The optional {dict}
argument specifies the sign attributes. The following values
are supported:
icon full path to the bitmap file for the sign.
linehl highlight group used for the whole line the
sign is placed in.
text text that is displayed when there is no icon
or the GUI is not being used.
texthl highlight group used for the text item

If the sign named {name} already exists, then the attributes
of the sign are updated.

The one argument {list} can be used to define a list of signs.
Each list item is a dictionary with the above items in {dict}
and a 'name' item for the sign name.

Returns 0 on success and -1 on failure. When the one argument
{list} is used, then returns a List of values one for each
defined sign.

Examples:
call sign_define("mySign", {"text" : "=>", "texthl" :
\ "Error", "linehl" : "Search"})
call sign_define([{'name' : 'sign1', 'text' : '=>'},
\ {'name' : 'sign2', 'text' : '!!'}])

sign_undefine([{name}])
sign_undefine({list})
Deletes a previously defined sign {name}. This is similar to
the |:sign-undefine| command. If {name} is not supplied, then
deletes all the defined signs.

The one argument {list} can be used to undefine a list of
signs. Each list item is the name of a sign.

Returns 0 on success and -1 on failure. For the one argument
{list} call, returns a list of values one for each undefined
sign.

Examples:
" Delete a sign named mySign
call sign_undefine("mySign")

" Delete signs 'sign1' and 'sign2'
call sign_undefine(["sign1", "sign2"])

" Delete all the signs
call sign_undefine()


sign_placelist({list})
Place one or more signs. This is similar to the
|sign_place()| function. The {list} argument specifies the
List of signs to place. Each list item is a dict with the
following sign attributes:
buffer buffer name or number. For the
accepted values, see |bufname()|.
group sign group. {group} functions as a
namespace for {id}, thus two groups
can use the same IDs. If not specified
or set to an empty string, then the
global group is used. See
|sign-group| for more information.
id sign identifier. If not specified or
zero, then a new unique identifier is
allocated. Otherwise the specified
number is used. See |sign-identifier|
for more information.
lnum line number in the buffer {expr} where
the sign is to be placed. For the
accepted values, see |line()|.
name name of the sign to place. See
|sign_define()| for more information.
priority priority of the sign. When multiple
signs are placed on a line, the sign
with the highest priority is used. If
not specified, the default value of 10
is used. See |sign-priority| for more
information.

If {id} refers to an existing sign, then the existing sign is
modified to use the specified {name} and/or {priority}.

Returns a List of sign identifiers. If failed to place a
sign, the corresponding list item is set to -1.

Examples:
" Place sign s1 with id 5 at line 20 and id 10 at line
" 30 in buffer a.c
let [n1, n2] = sign_place([{'id' : 5, 'name' : 's1',
\ 'buffer' : 'a.c', 'lnum' : 20},
\ {'id' : 10, 'name' : 's1',
\ 'buffer' : 'a.c', 'lnum' : 30}])

" Place sign s1 in buffer a.c at line 40 and 50
" with auto-generated identifiers
let [n1, n2] = sign_place([{'name' : 's1',
\ 'buffer' : 'a.c', 'lnum' : 40},
\ {'name' : 's1', 'buffer' : 'a.c',
\ 'lnum' : 50}])

sign_unplacelist({list})
Remove previously placed signs from one or more buffers. This
is similar to the |sign_unplace()| function.

The {list} argument specifies the List of signs to remove.
Each list item is a dict with the following sign attributes:
buffer buffer name or number. For the accepted
values, see |bufname()|. If not specified,
then the specified sign is removed from all
the buffers.
group sign group name. If not specified or set to an
empty string, then the global sign group is
used. If set to '*', then all the groups
including the global group are used.
id sign identifier. If not specified, then all
the signs in the specified group are removed.

Returns a List where an entry is set to 0 if the corresponding
sign was successfully removed or -1 on failure.

Example: >
" Remove sign with id 10 from buffer a.vim and sign
" with id 20 from buffer b.vim
call sign_unplace([{'id' : 10, 'buffer' : "a.vim"},
\ {'id' : 20, 'buffer' : 'b.vim'}])

[vim-dev ML]

[Paul Jolly]

The function calls can be in one message, right?

Very true.

As a follow up to this point (and hopefully not straying too far off topic on this thread), we now have a first cut of batching calls in govim (which could easily be extended to include the other channel functions if required):

myitcv/govim@bcba0c8

(which was then accompanied by a later bug fix myitcv/govim@2848a08)

The approach works well.

That said, the changes in this will likely still be a benefit I'd assume? Because we move from multiple function calls to two calls with list arguments.

—
You are receiving this because you commented.

[Yegappan Lakshmanan]

Hi,

On Tue, Jun 18, 2019 at 2:37 AM Paul Jolly <vim-dev...@256bit.org> wrote:
>
> Is your feature request related something that is currently hard to do? Please describe.
>
> Per https://groups.google.com/d/msg/vim_dev/aW2buOgZ6qI/fpxkp-V8AQAJ
>
> In https://github.com/leitzler/govim (master branch) Pontus is adding support to govim for signs that correspond to quickfix entries.
>
> This is done by calling sign_place/sign_unplace as required to get the signs for a buffer in the correct state.
>
> If there are lots of errors in a file, then there can be a large number of calls to sign_place/sign_unplace made by govim in rapid succession.
>
> Describe the solution you'd like
>
> A function:
>
> sign_setlist({bfnr}, {list} [, {action}])
>
> that sets the signs for buffer {bufnr} to {list}. {action} is similar to setqflist in that the caller can specify whether to create, replace or add to existing signs.
>

> Describe alternatives you've considered
>
> The only apparent alternative at the moment is to call sign_getplaced and then make the necessary calls to sign_place/sign_unplace; over a channel this becomes expensive.
>
> Additional context
>
> n/a

The support for this has now been merged in patch 8.1.1682.
Let me know if there are any issues with the functionality.

- Yegappan

[vim-dev ML]

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

[Christian Brabandt]

Closed #4557.

—
You are receiving this because you commented.