We have e.g. "LiveUpdateStart" and "LiveUpdate", which fits imho, but opinions are welcome.

Events should have a prefix to distinguish builtin events from events sent by user code (rpcnotify(..., "theevent")). Unless there is a strong reason for CamelCase I think events should follow the general API convention, i e underscores and lower case. Also is "live" really good in the event name? All events about current state is "live"... It would better if the name signifies the actual change, that some lines of a buffer changed. Something in the roundabouts of nvim_buf_lines_changed I would expect.

Ok, how about

• LiveUpdateStart -> nvim_buf_update_start
• LiveUpdate -> nvim_buf_update
• LiveUpdateTick -> nvim_buf_update_tick
• LiveUpdateEnd -> nvim_buf_update_end

Or should there be a function to request the full buffer separately?

No, this would make it more easy to write racy code, subscription and full buffer send should be the same method call when applicable. I would rather have a separate method to stop subscription.

My idea would be to allow this to be configured by the plugin (i.e. introduce methods nvim_get_channel_capacity and nvim_set_channel_capacity), and keep the current default. I'd also think this should be another PR (I'd do it), so unless someone pipes up, I'll not include this here.

I think we should attempt at reducing copying and allocation before we consider to add user configuration for memory limits.

No, this would make it more easy to write racy code, subscription and full buffer send should be the same method call when applicable. I would rather have a separate method to stop subscription.

Ok, I will introduce a second function.

I think we should attempt at reducing copying and allocation before we consider to add user configuration for memory limits.

I was afraid you were going to say something like this :D Are you positive a channel capacity of 10MB can be made sufficient to transfer a 1GB file in a sane number of notifications? Do you have something particular in mind?

I'm thinking of event names that maybe contains "lines" to mirror get_lines/set_lines. Also I would replace numreplaced with last_line, to follow the notion of set_lines (the numbers should be the same as a set_lines call that uses strict indexing, and nonnegative indicies)

How do you mean chunking solve the allocation problem? If the client hasn't read the first chunk when nvim wants to send the second, what happens?

I think it makes far more sense to block the channel when the buffer fills, encourage clients to drain the buffer as fast as they can (thus moving the buffer size explosion to the client), and have a user setting for client timeout (so users who want neovim to block for multiple second sometimes need to enable it themselves), than it does for clients to ask neovim for more buffer size.

this mode is still helped greatly by copy on write, which may be able to remove the need for large channel buffer sizes

I'm thinking of event names that maybe contains "lines" to mirror get_lines/set_lines

We'd have to change that if we ever want to send partial lines though. That's why I'm kinda leaning towards not having line in the name. Otoh, if the return values change, a new name might not be the worst thing...

How do you mean chunking solve the allocation problem? If the client hasn't read the first chunk when nvim wants to send the second, what happens?

I really don't know, I was hoping something along the lines of "chunk it up and hope the client fetches the chunks fast enough".

Also I would replace numreplaced with last_line, to follow the notion of set_lines

Will do.

I really don't know, I was hoping something along the lines of "chunk it up and hope the client fetches the chunks fast enough".

I'm sorry to be "that one" but "chunking" doesn't say me anything unless you specify what decision rule nvim will apply at the chunk boundary (that it couldn't apply otherwise)

I'm sorry to be "that one" but "chunking" doesn't say me anything unless you specify what decision rule nvim will apply at the chunk boundary (that it couldn't apply otherwise)

No, worries, I'm really just "handwaving" in the worst possible way here. Note that the LiveUpdateStart notification already has a parameter more to notify that client that more lines are to be expected. That way, neovim can send several LiveUpdateStart notifications, each containing a set of lines so the message size is always <10MB.

Limiting chunk size surely has other benefits as it allows interleaving a smaller unrelated message without waiting for the full 1GB. But that is a separate problem (head-of-line problem).

I will investigate if memline can support COW-references without going insane (that would mean zero allocation of line contents whenever the line is still in undo history, even if it was changed), until then (in this PR) I think we should just increase the memory limit.

I will investigate if memline can support COW-references without going insane (that would mean zero allocation of line contents whenever the line is still in undo history, even if it was changed), until then (in this PR) I think we should just increase the memory limit.

Sounds good :) To what should I increase it? I went with 2GB locally, but I'm unsure if there are consequences of just using a large value.

Let's go for that unless someone protests. Alternatively, one possibility could be to make the memory quota shared among all channels, and at OOM close the one with largest memory consumption...

@KillTheMule Thanks for picking this up! I haven't had energy to keep this going after so long, so it'd be absolutely awesome if you can get it across the finish line.

WRT to renaming events from "LiveUpdate" to "[something else]", remember to make the name something that is easy to google. :-)

WRT chunking ... when the LiveUpdateStart message is being generated, you could count the number of characters in each line as it is added to the array, and start a new chunk when the number of characters goes over some predefined limit. I don't know what the limit should be, but you could set it low enough that you never hit the channel buffer limits. And if there's a single line of code that's too many characters even on its own, you could just disable LiveUpdates and return an error message saying Line X too many characters for live updates. I think most users would be OK with having LiveUpdate-dependent plugins turn themselves off on the rare occasion they're editing a big file.

Thanks again for taking this on - I look forward to a future version of neovim with "Merged #7917" in the release notes. 👍

WRT chunking ...

Yeah, that's basically what I meant to say :) But since the buffer capacity is 10MB right now, sending a 1GB file would probably produce a lot of overhead. We'll just go with upping the capacity for now.

@bfredl So, for naming, we have the API functions, which are called nvim_buf_live_updates_start etc. right now, and the C functions with names like liveupdate_register. Should there be some similarity? I'll make an overview over the functions of this PR, so it's easier to think about :)

I made the changes @bfredl requested to the LiveUpdate event.

Here are the C functions we have (from liveupdate.h), and if applicable the corresponding API function, and the events sent:

• Functions:
  • liveupdate_register -> API nvim_buf_live_updates_start
  • liveupdate_unregister -> API nvim_buf_live_updates_stop
  • liveupdate_unregister_all
  • liveupdate_send_changes
  • liveupdate_send_tick
• Events:
  • LiveUpdateStart
  • LiveUpdate
  • LiveUpdateTick
  • LiveUpdateEnd

So how about we replace that with

• Functions:
  • buffer_updates_register -> API nvim_buf_updates_start
  • buffer_updates_unregister -> API nvim_buf_updates_stop
  • buffer_updates_unregister_all
  • buffer_updates_send_changes
  • buffer_updates_send_tick
• Events:
  • nvim_buf_updates_start
  • nvim_buf_update
  • nvim_buf_update_tick
  • nvim_buf_updates_end

Just throwing out ideas, get me something better (or what you like better, I don't mind), and I'll put it in :)

Also, this fails on windows with Vim(diffthis):E97: Cannot create diffs, does anyone know what's up with this?

Third, this fails on windows with test\functional\helpers.lua:93: Failed to evaluate expression, does anyone know those things.

@janlazo You seem to know a lot about windows things, could you have a look? Unfortunately, I can't test on windows locally...

(e) justin says on gitter:

the "cannot create diffs" issue could be related to the $TMPDIR change i pushed today (which hopefully fixes it)
regarding the "third" problem, what is the value of where
try this instead: eval("serverstart('"..where.."')")

when possible, use ' in VimL, it avoids escaping issues (forces all chars to be literal)

yeah, on windows with \ slashes, \ in a " string will break. so use '

QB/Travis failures unrelated. AV failure seems geniune, but I have not idea how to go about it, I could not reproduce it manually. I'll do some desperate debugging attempts, but hopefully someone can lend me a hand.

Other than that, I think what's needed is a decision about naming. That's it :)

@chemzqm In general attachment is on a channel basis (same as UI). Rather the host should allow multiple plugins to share a buffer attachment. I might prototype this in the python host, to show the general pattern. This will have other benefits: in any sync context buf[line:range] (python syntax, but the idea should work the same) can use the cached value in the host instead of needing to send an additional request.

@bfredl I've tested that this could happen for different node plugins

let buffer = null

async function getLines () {
  let {nvim} = this
  buffer = await nvim.buffer
  await buffer.request('nvim_buf_attach', [buffer, false, {}])
}

async function unbind() {
  await buffer.request('nvim_buf_detach', [buffer])
}

module.exports = (plugin) => {
  plugin.registerCommand('LiveListen', getLines.bind(plugin), {sync: false})
  plugin.registerCommand('LiveunListen', unbind.bind(plugin), {sync: false})
  plugin.nvim.on('notification', (name, args) => {
    if (name === 'nvim_buf_lines_event') {
      let [buf, tick, firstline, lastline, linedata, more] = args
      console.error(`changes: ${firstline} ${lastline}`)
    }
  })
}

let buffer = null

async function getLines () {
  let {nvim} = this
  buffer = await nvim.buffer
  await buffer.request('nvim_buf_attach', [buffer, false, {}])
}

module.exports = (plugin) => {
  plugin.registerCommand('LiveNotify', getLines.bind(plugin), {sync: false})
  plugin.nvim.on('notification', (name, args) => {
    if (name === 'nvim_buf_lines_event') {
      let [buf, tick, firstline, lastline, linedata, more] = args
      console.error(`notify: ${firstline} ${lastline}`)
    }
  })
}

Detach called in one plugin would cause notification not working in other plugins.

There is a detach event nvim_buf_detach_event, but how could I attach event listener to the buffer again if it was triggered by other plugin calling detach?

There is a detach event nvim_buf_detach_event, but how could I attach event listener to the buffer again if it was triggered by other plugin calling detach?

Well the point is that the plugin won't call the nvim methods directly, rather helper functions in the host. The host can keep refcounting on the buffer, and only call detach when the number of plugins using it goes to zero. I think I have an old unfinished branch for this in python, seems like a good time for me to revive it....

@bfredl But there would some other problem for a host to manage the count of attach and detach.
For example, a plugin may call attach on different user event(the attach could be called more than one time), and it would expect one detach call to detach the events.
So it requires something like plugin id to make the detach work properly when one plugin call attach multiply times.

@chemzqm That sounds to me more like an "accidental" problem (change the helper functions to expose the most convenient interface to plugins) rather than a "fundamental" problem (that helper function solution won't work at all)

My thought for python host would be to just reify the subscription rather than introduce plugin identity, so something like

id = thebuf.attach(self.callback, options...)

and the plugin could later do thebuf.detach(id) or alternative attach would return an object with a .detach(). So detach is called the same number times as attach (or not at all, if subscription is persistent) Won't something like this make sense for nodejs as well?

The alternative for me, if we want nvim core to be more involved, would be to introduce some kind of sub-channel routing mechanism, where each nvim_..._event will have an adress handle (or list of such) as first parameter. (but even then I think host will be involved, because we will want to multicast the some updates to different addresses on a channel, to not duplicate all buffer updates). To me this seems overblown for this PR as it stands, but maybe this will be useful with further expansions, like individual plugin only caring about extmarks in its own namespace.

Wait, am I missing something? Registering for updats happens via channel_id. Am I wrong in thinking that every remote plugin gets its own channel? So what I think you're describing shouldn't happen, @chemzqm . I can't read your code, but maybe I can cook up a test for that.

Or am I off with something here?

(e) Btw, calling attach multiple times is equivalent to calling it once.

(ee) Actually this test tests the scenario, doesn't it? We're detaching here, but still receive notifications here.

@KillTheMule remote plugins that use the registration mechanism will all run in a host shared among all such plugins in the same language. My thought was to add a thin layer in the host to make sure multiple plugins can subscribe and only get callbacks when their buffers has an event.

Oh, ok, so there even are docs for this feature :) Imho, that would be the job of the host indeed.

Nicely done. Thank you @phodge @KillTheMule @bfredl @chemzqm and @ZyX-I .

I made some superficial changes in the merge-commit.

I began to look into python host integration: neovim/pynvim#340