Re: [vim/vim] Allow listener_add mechanism to provide unbuffered notifications. (PR #18295)
Paul Ollis
@paul-ollis pushed 1 commit.
- 63b0a08 Implement unbuffered listener feature.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis pushed 1 commit.
- c5143c6 Implement unbuffered listener feature.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis pushed 1 commit.
- 6cdbb29 Implement unbuffered listener feature.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis pushed 1 commit.
- 71e9d01 Chasing a clean CI run.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra commented on this pull request.
> Add a callback function that will be invoked when changes have
been made to buffer {buf}.
{buf} refers to a buffer name or number. For the accepted
values, see |bufname()|. When {buf} is omitted the current
buffer is used.
Returns a unique ID that can be passed to |listener_remove()|.
- The {callback} is invoked with five arguments:
+ If the {buf} already has registered callbacks then the
+ equivalent of >
+ listener_flush({buf})
+< is performed before the new callback is added.
+
+ When {unbuffered} is |FALSE| or omitted, the {callback} is
+ invoked with five arguments:
This is a bit strange. Wouldn't it make more sense to have the callback signature always the same? So always call it with the arguments (bufnr, start, end, added and changes), and then just detail that changes is a single change for an unbuffered callback?
In src/testdir/test_listener.vim:
> @@ -375,7 +524,7 @@ endfunc
func Test_remove_listener_in_callback()
new
let s:ID = listener_add('Listener')
- func Listener(...)
+ func! Listener(...)
I think you can remove the !
In src/change.c:
> + * will be discarded in the event that all listeners were removed.
+ *
+ */
+ static void
+clean_listener_list(buf_T *buf, listener_T **list, bool all)
+{
+ listener_T *prev;
+ listener_T *lnr;
+ listener_T *next;
+
+ prev = NULL;
+ for (lnr = *list; lnr != NULL; lnr = next)
+ {
+ next = lnr->lr_next;
+ if (all || lnr->lr_id == 0)
+ {
style: you can drop braces around single statements
In src/change.c:
> @@ -250,6 +347,10 @@ f_listener_add(typval_T *argvars, typval_T *rettv)
free_callback(&callback);
return;
}
+ if (argvars[2].v_type != VAR_UNKNOWN)
+ {
style: you can drop braces around single line statements.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis commented on this pull request.
> Add a callback function that will be invoked when changes have
been made to buffer {buf}.
{buf} refers to a buffer name or number. For the accepted
values, see |bufname()|. When {buf} is omitted the current
buffer is used.
Returns a unique ID that can be passed to |listener_remove()|.
- The {callback} is invoked with five arguments:
+ If the {buf} already has registered callbacks then the
+ equivalent of >
+ listener_flush({buf})
+< is performed before the new callback is added.
+
+ When {unbuffered} is |FALSE| or omitted, the {callback} is
+ invoked with five arguments:
Thanks for reviewing this PR. I will be away from my development PC for a few days, so fixes will be a bit delayed.
I though of doing it that way at first, but changed my mind. There were 2
reasons for my decision:
-
Providing a single entry list, which added a single extra value not
already present in the other arguments seemed strange to me. -
Performance. I try to avoid premature optimisation, but specifically
creating a list that would likely be ignored seemed like unnecessary
CPU load.
I am just providing my reasoning. I am happy to go with your preferred option.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis commented on this pull request.
In src/testdir/test_listener.vim:
> @@ -375,7 +524,7 @@ endfunc
func Test_remove_listener_in_callback()
new
let s:ID = listener_add('Listener')
- func Listener(...)
+ func! Listener(...)
I added this because I got an error about the function already existing.
Thinking about it, I should have investigated a bit more as to why. I will do so.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis commented on this pull request.
In src/change.c:
> + * will be discarded in the event that all listeners were removed.
+ *
+ */
+ static void
+clean_listener_list(buf_T *buf, listener_T **list, bool all)
+{
+ listener_T *prev;
+ listener_T *lnr;
+ listener_T *next;
+
+ prev = NULL;
+ for (lnr = *list; lnr != NULL; lnr = next)
+ {
+ next = lnr->lr_next;
+ if (all || lnr->lr_id == 0)
+ {
Agreed.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Paul Ollis
> @@ -250,6 +347,10 @@ f_listener_add(typval_T *argvars, typval_T *rettv)
free_callback(&callback);
return;
}
+ if (argvars[2].v_type != VAR_UNKNOWN)
+ {
Agreed.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra commented on this pull request.
> Add a callback function that will be invoked when changes have
been made to buffer {buf}.
{buf} refers to a buffer name or number. For the accepted
values, see |bufname()|. When {buf} is omitted the current
buffer is used.
Returns a unique ID that can be passed to |listener_remove()|.
- The {callback} is invoked with five arguments:
+ If the {buf} already has registered callbacks then the
+ equivalent of >
+ listener_flush({buf})
+< is performed before the new callback is added.
+
+ When {unbuffered} is |FALSE| or omitted, the {callback} is
+ invoked with five arguments:
I understand, but I am more concerned with a consistent function interface.
Others opinions? @yegappan @zeertzjq @dkearns ?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan commented on this pull request.
> Add a callback function that will be invoked when changes have
been made to buffer {buf}.
{buf} refers to a buffer name or number. For the accepted
values, see |bufname()|. When {buf} is omitted the current
buffer is used.
Returns a unique ID that can be passed to |listener_remove()|.
- The {callback} is invoked with five arguments:
+ If the {buf} already has registered callbacks then the
+ equivalent of >
+ listener_flush({buf})
+< is performed before the new callback is added.
+
+ When {unbuffered} is |FALSE| or omitted, the {callback} is
+ invoked with five arguments:
I agree. The callback function signature should remain consistent between the two cases.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.