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

fix: schedule setting buffer modifiable #18

Merged
merged 2 commits into from
Apr 14, 2024
Merged

Conversation

b0o
Copy link
Contributor

@b0o b0o commented Apr 12, 2024

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:~~

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 7e00951. 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
Copy link
Contributor

mobily commented Apr 14, 2024

@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
Copy link
Contributor

mobily commented Apr 14, 2024

@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
Copy link
Contributor Author

b0o commented Apr 14, 2024

Sure, done. Thanks for testing!

@mobily
Copy link
Contributor

mobily commented Apr 14, 2024

excellent, thank you! ❤️

@mobily mobily merged commit 8cd0493 into grapp-dev:main Apr 14, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: Buffer is not 'modifiable'
2 participants