grapp-dev / nui-components.nvim

A feature-rich and highly customizable library for creating user interfaces in Neovim.
https://nui-components.grapp.dev
MIT License
303 stars 6 forks source link

fix: schedule setting buffer modifiable #18

Closed b0o closed 5 months ago

b0o commented 5 months ago

Fixes #17.

Outdated I'm mostly following the existing pattern in the `Component:modify_buffer_content`, but I'm wondering if a better solution would be to just move the `self:set_buffer_option("modifiable", true)` into the upper `vim.schedule` callback, instead of wrapping the whole thing in an additional one:~~ ```lua function Component:modify_buffer_content(modify_fn) vim.schedule(function() self:set_buffer_option("modifiable", true) modify_fn() vim.schedule(function() self:set_buffer_option("modifiable", false) end) end) end ``` I've tested the above and it seems to work just as well, but I imagine there was a reason `self:set_buffer_option("modifiable", true)` was outside of the first `schedule` callback in the first place?

Update: After more testing, the bug was still occurring in 7e00951598ad4246f16d9c7fcad5941c7bb73327. I force-pushed a new commit which sets modifiable inside the first vim.schedue callback, right before calling modify_fn(). This seems to work 100% of the time.

However, I question whether it's necessary for the self:set_buffer_option("modifiable", false) to be inside of its own schedule callback? I have tested this and it works just as well, and seems to be the more straightforward, "atomic", solution:

function Component:modify_buffer_content(modify_fn)
  vim.schedule(function()
    self:set_buffer_option("modifiable", true)
    modify_fn()
    self:set_buffer_option("modifiable", false)
  end)
end

Out of curiosity, what's the reasoning for changing modifiable in a separate schedule callback? It seem like it might be predisposed to race conditions.

mobily commented 5 months ago

@b0o after testing your solution in my sandbox code, which includes complex cases and each component, it seems there are no issues. Ready to merge, thank you!

mobily commented 5 months ago

@b0o BTW, I have been testing the following:

function Component:modify_buffer_content(modify_fn)
  vim.schedule(function()
    self:set_buffer_option("modifiable", true)
    modify_fn()
    self:set_buffer_option("modifiable", false)
  end)
end

Could you update your PR, please?

b0o commented 5 months ago

Sure, done. Thanks for testing!

mobily commented 5 months ago

excellent, thank you! ❤️