-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fix panic on close last buffer (#2367) #2658
Fix panic on close last buffer (#2367) #2658
Conversation
In certain circumstances it was possible to cause a panic when closing buffers due to some mishandling of view document history. A change has been made to delete removed documents from the history of accessed documents for each view. The ensures we don't attempt to jump to a deleted document by mistake.
helix-view/src/editor.rs
Outdated
@@ -826,6 +826,9 @@ impl Editor { | |||
Some(Action::Close(view.id)) | |||
} | |||
} else { | |||
// documents also need be removed from the view "document access history" | |||
// so we don't accidentally try to jump back to them after they have been deleted | |||
view.docs_access_history.retain(|doc| doc != &doc_id); |
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.
Let's fix this in https://github.com/helix-editor/helix/blob/955a6cd540ca0483859b98512d7404b5f028b8d4/helix-view/src/view.rs#L64-L66=
The method is already called on line 818
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.
Ah wait, that's not actually possible since it's a method on view.jumps
. Perhaps we should add a method to View
that cleans up both jumps and access history. Any place calling jumps.remove
should then call this new method instead
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.
Ok, I'll have a go at doing it
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.
@archseer I've moved the cleanup into a view.remove_document
function in commit edd5a38, take a look when you have a moment.
We could also take this approach further and add an Editor.remove_document
function which would look like:
impl Editor {
fn remove_document(&mut self, doc_id: &DocumentId) {
for view in self.tree.views_mut() {
view.remove_document(&doc_id);
}
self.documents.remove(&doc_id);
}
}
e728afc
to
edd5a38
Compare
There's one more occurrence of view.jumps.remove in the file that needs to be changed to use the new function |
I've now switched the last occurrence of |
In certain circumstances it was possible to cause a panic when closing
buffers due to some mishandling of view document history.
A change has been made to delete removed documents from the history of
accessed documents for each view. The ensures we don't attempt to jump
to a deleted document by mistake.