Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] functions to notify a co-process when the current buffer is modified #5269

Closed
wants to merge 21 commits into from

Conversation

phodge
Copy link
Contributor

@phodge phodge commented Aug 29, 2016

Hello, I have been working on an efficient API for having neovim notify a co-process (channel?) any time a buffer is modified (because doing this with TextChanged autocmds is inefficient and difficult). This feature will make it easier to write plugins which scans the buffers contents and updates the display in real-time (e.g. syntax highlighters, linters).

The first commit in this PR contains the help text for 3 new functions and a mini-tutorial, please refer to that commit if you would like to learn how the new feature would work. The last commit contains some TODO items that I have yet to resolve.

I'm interested in collecting feedback with regards to:

  1. Do others think the 3 new functions mentioned in the first commit are a sensible interface?
  2. Are there any reasons why the solution I'm working on won't work?
  3. Do you have any suggestions on how I should resolve the TODO items from the last commit?

Thank you for your thoughtful consideration.
Peter

@marvim marvim added the RFC label Aug 29, 2016
@phodge
Copy link
Contributor Author

phodge commented Aug 29, 2016

Related to:
#719
#1114

was replaced and will always be between `1` and `line('$')`. {int2} tells
how many lines were replaced and will always be at least `1`. For most
small changes {int2} will be `1`, meaning only the line mentioned in {int1}
was changed. {int3} tells how many new lines are replacing the lines that
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{int3} would also be at least 1?

@bfredl
Copy link
Member

bfredl commented Aug 29, 2016

Interesting work! There is a simpler way than adding vimL functions tough, use api methods. Compare nvim_ui_attach in api/ui.c, if the first argument is uint64_t channel_id, then the channel_id is implicitly sent when the api function is called over the channel. If the function is placed in api/buffer.c and called buffer_add_watcher (soon nvim_buf_add_watcher, #4934 ) then for instance it can be called on a python buffer object as vim.current.buffer.api.add_watcher(...)

@ghost
Copy link

ghost commented Aug 29, 2016

When a single channel is watching multiple buffers, is there a way to determine which buffer was modified?


Setting Up~

Use |rpcstart()| to start up your co-process. This will give you a valid
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to repeat this here. It is likely also confusing: I suppose many consumers would be plugins living in a rplugin host, and don't be separate co-processes. (As the notifications are high-throughput we should also add multiplexing logic to e.g. the python-client, so that multiple plugins active in the same buffer can share the same notification stream)

@phodge
Copy link
Contributor Author

phodge commented Aug 30, 2016

There is a simpler way than adding vimL functions tough, use api methods.

Thank you, I will look into this ... I don't have any experience with writing or using api methods but it sounds like a reasonable approach.

When a single channel is watching multiple buffers, is there a way to determine which buffer was modified?

Er, no. I'm embarrassed that I didn't notice this glaring omission myself. I will make sure this is catered for in my next push.

I think it should use zero-based indexing

This seems reasonable.

@bfredl
Copy link
Member

bfredl commented Aug 30, 2016

Also please try to write tests in lua. api/server_notifications_spec.lua might have some useful starting points: channel = nvim('get_api_info')[1] to get the channel to the lua test driver (which is just another rpc client to nvim) and eq(..., next_message()) to assert the next notification from nvim. If you make api functions on buffer objects, use curbufmeths.watch_changes() (or what they will be called) to call them.

@phodge phodge closed this Oct 26, 2016
@phodge phodge deleted the rpcbufwatch branch October 26, 2016 20:28
@jszakmeister jszakmeister removed the RFC label Oct 26, 2016
@justinmk
Copy link
Member

@phodge why close this?

@phodge phodge restored the rpcbufwatch branch October 29, 2016 02:10
@phodge
Copy link
Contributor Author

phodge commented Oct 29, 2016

@justinmk Sorry I think when I deleted my rpcbufwatch branch it closed the PR, and just now I've accidentally restored the old branch because my trackpad is way too sensitive.

@phodge
Copy link
Contributor Author

phodge commented Oct 29, 2016

OK I've won my battle with git rebase and managed to push a redesigned version of this feature. It's quite stable and performance seems good. Unfortunately with a new baby having arrived a few weeks ago and a bunch of other projects piling up, I haven't had time to learn lua and rewrite the tests (although they all pass with a python3 py.test). All feedback is appreciated!

@marvim marvim added the RFC label Oct 29, 2016
@phodge
Copy link
Contributor Author

phodge commented Oct 29, 2016

I've written a simple word highlighting plugin to help with evaluating performance: https://github.com/phodge/vim-hiword

@tjdevries
Copy link
Contributor

tjdevries commented Oct 30, 2016

This would be great for www.github.com/tjdevries/nvim-langserver-shim. There are a couple messages that need to be sent on text changed, text editted, etc. Really cool idea!

src/nvim/eval.c Outdated
@@ -10583,6 +10583,7 @@ static void f_has(typval_T *argvars, typval_T *rettv, FunPtr fptr)
"linebreak",
"lispindent",
"listcmds",
"liveupdate",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally we don't add named features for new nvim-specific features, as nvim will always be compiled with all features (with a few exceptions). Instead a plugin should check for the thing the feature adds, an api function in this case. This is possible in principle by inspecting the api metadata, but maybe we should make this simpler by overloading exist() (exist('+') does not work as not all api functions are vimL functions, like this won't be).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a new commit to revert this addition.

}

// add the buffer to the active array and send the full buffer contents now
uint64_t *newlist = xmalloc((active_size + 2) * sizeof channel_id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should use kvec_t for this, it implements a resizable array for us. See lib/kvec.h for implementation, and you can find example i e bufhl_vec_T lineinfo in buffer.c (type defined in bufhl_defs.h)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a new commit that changes buf->liveupdate_channels to a kvec_t(uint64_t).

@@ -7,6 +7,7 @@ local deprecated_aliases = {
nvim_buf_get_number="buffer_get_number",
nvim_buf_get_option="buffer_get_option",
nvim_buf_get_var="buffer_get_var",
nvim_buf_live_updates="buffer_live_updates",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you use master version of neovim python-client (release will be made soon) this will not be needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to leave this backwards-compatibility in since I'd like to try and have live updates merged soon, and I want it to work with current python clients.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is for older functions that was part of 0.1.5 as 0.1.6 changed the naming convention and this has nothing to be backwards-compatible to. python-client 0.1.11 will be released soon, well before this is merged. But we will need to add helper functions in python-client to make it feasible to use this in rplugins (if different python plugins subscribe to the same buffer).

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method names ("LiveUpdate" etc) should be rationalized with other method names used by nvim. See discussion starting at https://gitter.im/neovim/neovim?at=58160c8a482c168b22d0369b

args.items = xcalloc(sizeof(Object), args.size);

// the first argument is always the buffer number
args.items[0] = INTEGER_OBJ(buf->handle);
Copy link

@ghost ghost Nov 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Buffer should be sent using EXT encoding. See discussion starting at https://gitter.im/neovim/neovim?at=581609f85a1cfa016e61e784

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which means BUFFER_OBJ should be used instead of INTEGER_OBJ.

while (buf->liveupdate_channels[active_size] != LIVEUPDATE_NONE) {
if (buf->liveupdate_channels[active_size] == channel_id) {
size_t size = kv_size(buf->liveupdate_channels);
if (size) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this if can just be removed

KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request Feb 9, 2018
KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request Feb 9, 2018
KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request Feb 12, 2018
KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request Feb 12, 2018
KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request Feb 12, 2018
KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request Feb 17, 2018
KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request Feb 17, 2018
KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request Feb 17, 2018
@jcjolley
Copy link

jcjolley commented Apr 20, 2018

Thank you for all the work that has been done towards this! This issue is important for https://github.com/Chillee/VSCodeNeovim, and I'm excited to see all the progress that has been made.

KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request Apr 24, 2018
KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request Apr 24, 2018
KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request Apr 24, 2018
KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request Apr 27, 2018
KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request Apr 27, 2018
KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request Apr 27, 2018
KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request May 10, 2018
KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request May 10, 2018
KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request May 10, 2018
KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request May 18, 2018
KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request May 18, 2018
KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request May 18, 2018
KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request May 23, 2018
KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request May 23, 2018
KillTheMule pushed a commit to KillTheMule/neovim that referenced this pull request May 23, 2018
@justinmk
Copy link
Member

justinmk commented Jun 6, 2018

This will be addressed by #7917

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet