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

textlock umbrella issue #183

Open
jmbuhr opened this issue Oct 21, 2024 · 5 comments · Fixed by #190
Open

textlock umbrella issue #183

jmbuhr opened this issue Oct 21, 2024 · 5 comments · Fixed by #190
Labels
bug Something isn't working

Comments

@jmbuhr
Copy link
Owner

jmbuhr commented Oct 21, 2024

Issue

Currently we call sync_raft on a request to update the otter buffer and forward requests to up-to-date attached servers.

local success = keeper.sync_raft(main_nr, lang)

but this uses the set_lines api, which does not work when textlock is set:

api.nvim_buf_set_lines(otter_nr, 0, -1, false, ls)

This happens for example when a plugin uses the CompleteChanged autocommand and an lsp request is sent from within this context (which then reaches otter.nvim as a request).

References

https://neovim.io/doc/user/api.html#nvim_buf_set_lines()
https://neovim.io/doc/user/eval.html#textlock

Related

#145

Affected issues

#182, #178

@jmbuhr jmbuhr added the bug Something isn't working label Oct 21, 2024
@jmbuhr jmbuhr changed the title async umbrella issue textlock umbrella issue Oct 21, 2024
@TheLeoP
Copy link
Contributor

TheLeoP commented Dec 10, 2024

I had an idea for a naive solution and after some really naive tests it seems to be working. So, I'm putting it in here to see if I may be missing something obvious about why my solution won't work.

If the whole request function is wrapped in vim.schedule_wrap, this error won't happen because will never be in a textlock while changing the otter buffers.

i.e.

        request = vim.schedule_wrap(function(method, params, handler, _)
        -- the body of the request function is still here
        end),

@jmbuhr
Copy link
Owner Author

jmbuhr commented Dec 11, 2024

Great idea! I wonder if it would be enough to only wrap the relevant part of sync_raft, since it can then take advantage of the fact that is doesn't have to run when the buffer hasn't changed.

Do you want to open a PR (with your solution)? Preferably also with an example that doesn't work with the current implementation, but works with your solution.

@TheLeoP
Copy link
Contributor

TheLeoP commented Dec 11, 2024

Great idea! I wonder if it would be enough to only wrap the relevant part of sync_raft, since it can then take advantage of the fact that is doesn't have to run when the buffer hasn't changed.

My first thougt is that it may be messy because:

  • If the schedule is done inside of sync_raft, the bufferts wouldn't be sync when the function exits. They would be synced in the next event loop iteration
  • If the schedule is done in the middle of request, you would need to plut all the code following sync_raft inside of it (because of the reason above) to keep it working

I don't know the codebase and I'm wokirng under the assumption that after a sync_raft everything should be synced or else the following code won't work. This may not be true, but I can't really tell

@jmbuhr
Copy link
Owner Author

jmbuhr commented Dec 11, 2024

I think you are right.

@jmbuhr jmbuhr closed this as completed in 964b012 Dec 17, 2024
@jmbuhr jmbuhr reopened this Dec 25, 2024
@jmbuhr
Copy link
Owner Author

jmbuhr commented Dec 25, 2024

Re-opened due to regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants