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

Scrolling view keeps selections #1420

Merged
merged 1 commit into from Jan 3, 2022

Conversation

k2d222
Copy link
Contributor

@k2d222 k2d222 commented Jan 2, 2022

Fixes #917.

  • Scrolling will never change secondary selections.
  • Primary selection is replaced with a single char selection when the cursor gets out of view.

This is the same behavior as in Kakoune. I believe in vim / nvim it would grow / shrink the main selection instead of deleting it.

Copy link
Member

@dead10ck dead10ck left a comment

Choose a reason for hiding this comment

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

Nice, thank you!

// TODO: only manipulate main selection
doc.set_selection(view.id, Selection::single(anchor, head));
// replace primary selection with an empty selection at cursor pos
let prim_sel = Range::new(anchor, head);
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to clone, or is it possible just to mutate the primary range that we already got from above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Selections are currently not mutably accessible outside Document. I believe this protects the Integrity of the selections, e.g. not having overlapping selections (see:

pub fn ensure_invariants(self, text: RopeSlice) -> Self {
)

Ranges and Selections are pretty lightweight too: Range is 3 usize and Selection is 1 vec of Ranges + 1 usize

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but Range: Copy, and Selection::primary returns an owned Range, which is its own copy, if I'm not mistaken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure to get your point. Currently, Document::set_selection is the only way to modify selections and it asks for a owned Selection, so a copy of Selection is required anyway. The Ranges inside Selections are not mutable in any way so a copy or a new range is also required.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I see what you mean now: Selection::replace takes a mut self, so cloning is necessary. Never mind me.

@archseer
Copy link
Member

archseer commented Jan 3, 2022

Thanks! 🎉

@archseer archseer merged commit dbaed0b into helix-editor:master Jan 3, 2022
@k2d222 k2d222 deleted the scroll-keep-selection branch January 13, 2022 16:14
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.

Scrolling view using view mode or mouse removes additional cursors
3 participants