-
Notifications
You must be signed in to change notification settings - Fork 251
fix(renderer): check if current buffer is loaded #1418
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
Conversation
Umm I might've done something wrong with git history. Only the last commit is what is related to this PR. |
vim.api.nvim_buf_delete(bufnr, { force = true }) | ||
end | ||
end) | ||
if window_existed then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it matter if the window existed? Why should a buffer be allowed to remain if the window was closed by some other means, but not if it was closed by this function?
Also, why is it always scheduled even though it usually should be deleted synchronously?
I think the code from 3.22 was already correct and would fix this issue and we should just go back to that:
neo-tree.nvim/lua/neo-tree/ui/renderer.lua
Lines 167 to 175 in 8afbb06
if bufnr > 0 and vim.api.nvim_buf_is_valid(bufnr) then | |
state.bufnr = nil | |
local success, err = pcall(vim.api.nvim_buf_delete, bufnr, { force = true }) | |
if not success and err:match("E523") then | |
vim.schedule_wrap(function() | |
vim.api.nvim_buf_delete(bufnr, { force = true }) | |
end)() | |
end | |
end |
Whatever the other edge case was which the changes were trying to fix should be handled another way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have confirmed that the old code works correctly and the bug in #1415 was the direct result of changing that code. I'm going to close this because reverting that code is the real fix.
@@ -1010,6 +1013,9 @@ M.acquire_window = function(state) | |||
vim.api.nvim_win_set_buf(state.winid, state.bufnr) | |||
else | |||
close_old_window() | |||
if state.bufnr and vim.api.nvim_buf_is_valid(state.bufnr) then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
M.close
is already deleting the buffer. Why are we double deleting? Is it because of your other change which will only sometimes delete the old buffer? It seems like these two changes are in conflict with another.
Also, if i remember correctly, the original code from a few months ago already did what we are slowly getting back to, which is to immediately attempt to delete the buffer and then schedule a delete for later only if we get a specific error.
I think the right place to make this fix is in the M.close()
code, we shouldn't be deleting the buffer multiple times and from different locations.
Closing in favor of #1433 |
hi, after test. this issue not fixed. you can test when neo-tree is open, then |
but this pr can fix it. |
That PR was merged to |
closes: #1415
I failed to detect one edge case in
Old code was guaranteed that
state.bufnr
was deleted insideclose_old_window
function but now only whenwindow_existed
.Since you changed the code so that buffer might survive even when window is closed a couple months ago, we needed to delete the old remaining buffer regardless of
window_existed
for this specific if-cases.