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

If switching away from an empty scratch buffer, remove it #935

Merged
merged 3 commits into from
Oct 31, 2021

Conversation

Omnikar
Copy link
Contributor

@Omnikar Omnikar commented Oct 30, 2021

https://matrix.to/#/!zMuVRxoqjyxyjSEBXc:matrix.org/$UF70A_vWmDH5EawsTQLnyLVp0qIqokNwVI4aRFXdEd4?via=matrix.org&via=tchncs.de&via=nixos.dev

When I use Helix, I tend to start it with no arguments and then space f and open a file. As a result, the scratch buffer which opened when I first started the editor remains in the buffer list, and this has been annoying me. I'd prefer if, when switching buffers, if a scratch buffer has no path and is not modified (meaning that it is empty), that it be automatically removed (given that no open view currently displays the buffer).

I had to do some stuff to get around borrow errors.

helix-view/src/editor.rs Outdated Show resolved Hide resolved
&& !self
.tree
.traverse()
.any(|(_, view)| view.doc == doc.id && view.id != view!(self).id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about first changing the view.doc and view.offset (L263/264)? This way you don't need to check for view.id != view!(self).id) here and we can extract this traversal into a method

Copy link
Contributor Author

@Omnikar Omnikar Oct 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would require using view_mut! twice: once to set view.doc and view.offset, and once after borrowing self.tree for the check, which I suppose isn't necessarily wrong, but I just don't like it. We could alternatively do this:

self.tree.traverse().filter(|(_, view)| view.doc == doc.id).count() > 1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see. We want to at least get rid of the view! calls inside any, since that costs as much as a view_mut but would be called per iteration here. On L235 we already borrow view so you should be able to reuse that:

  • change L235 to let (view, doc) = current_ref!(self)
  • remove L241 (let doc = doc!(self);)
  • change any to .any(|(_, v)| v.doc == doc.id && v.id != view.id);

&& !self
.tree
.traverse()
.any(|(_, view)| view.doc == doc.id && view.id != view!(self).id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see. We want to at least get rid of the view! calls inside any, since that costs as much as a view_mut but would be called per iteration here. On L235 we already borrow view so you should be able to reuse that:

  • change L235 to let (view, doc) = current_ref!(self)
  • remove L241 (let doc = doc!(self);)
  • change any to .any(|(_, v)| v.doc == doc.id && v.id != view.id);

@@ -238,9 +238,28 @@ impl Editor {
self.documents[view.doc].selection(view.id).clone(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the change to current_ref, this can now be simplified to

Suggested change
self.documents[view.doc].selection(view.id).clone(),
doc.selection(view.id).clone(),

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to also move this jump definition into if !remove_empty_scratch. That way we're only constructing and cloning the selection if we actually need the jump. To do that we would need to use view_mut! twice, but that's cheaper than a clone

@archseer archseer merged commit 2f8ad7f into helix-editor:master Oct 31, 2021
@Omnikar Omnikar deleted the remove-empty-scratch branch October 31, 2021 00:44
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.

2 participants