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(renderer): check if current buffer is loaded #1418

Closed
wants to merge 4 commits into from

Conversation

pysan3
Copy link
Collaborator

@pysan3 pysan3 commented Mar 27, 2024

closes: #1415

I failed to detect one edge case in

Old code was guaranteed that state.bufnr was deleted inside close_old_window function but now only when window_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.

@pysan3
Copy link
Collaborator Author

pysan3 commented Mar 27, 2024

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

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:

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.

Copy link
Contributor

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

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.

@cseickel
Copy link
Contributor

Closing in favor of #1433

@cseickel cseickel closed this Apr 11, 2024
@chaozwn
Copy link

chaozwn commented Apr 12, 2024

Closing in favor of #1433

hi, after test. this issue not fixed. you can test when neo-tree is open, then <C-w>o, reopen neo-tree.
image

@chaozwn
Copy link

chaozwn commented Apr 12, 2024

Closing in favor of #1433

but this pr can fix it.

@cseickel
Copy link
Contributor

Closing in favor of #1433

but this pr can fix it.

That PR was merged to main and it works for me. Can you confirm that it is fixed for you using the main branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Buffer with this name already exists after manually closing neo-tree
3 participants