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

Add shrink equivalent of extend_to_line_bounds #2450

Merged

Conversation

EpocSquadron
Copy link
Contributor

Implements #2002, with a difference in that using the command when the selection spans only a single line will not result in conditional logic.

I believe there's some holes in the logic that could open to a panic so marking as draft until I can address.

@EpocSquadron EpocSquadron force-pushed the epocsquadron/shrink-to-full-lines branch from 8bfe5ee to ac84862 Compare May 15, 2022 00:45
@EpocSquadron
Copy link
Contributor Author

I fixed the potential for panic.

I originally had this so that the shrink would leave the cursor on the character before the EOL, but this led to some interesting bugs where empty lines would be trimmed from the selection even though in spirit the whole line is part of the selection. Including the EOL in the selection resolves this, while also mirroring the behavior of extend_to_line_bounds which includes the EOL character at the end of the selection as well. This makes the command perfectly symmetric. If you still desire to trim empty lines, that can now be accomplished by chaining combinations of _, X and this new command.

@EpocSquadron EpocSquadron marked this pull request as ready for review May 15, 2022 00:57
@the-mikedavis
Copy link
Member

I just gave this a try and it looks good: I can't find any edge cases.

I think A-x is good for now since it's the kakoune binding but it could be reconsidered later like in #2130

Comment on lines +1944 to +1979
doc.set_selection(
view.id,
doc.selection(view.id).clone().transform(|range| {
let text = doc.text();

let (start_line, end_line) = range.line_range(text.slice(..));

// Do nothing if the selection is within one line to prevent
// conditional logic for the behavior of this command
if start_line == end_line {
return range;
}

let mut start = text.line_to_char(start_line);

// line_to_char gives us the start position of the line, so
// we need to get the start position of the next line. In
// the editor, this will correspond to the cursor being on
// the EOL whitespace charactor, which is what we want.
let mut end = text.line_to_char((end_line + 1).min(text.len_lines()));

if start != range.from() {
start = text.line_to_char((start_line + 1).min(text.len_lines()));
}

if end != range.to() {
end = text.line_to_char(end_line);
}

if range.anchor <= range.head {
Range::new(start, end)
} else {
Range::new(end, start)
}
}),
);
Copy link
Contributor

@pickfire pickfire May 16, 2022

Choose a reason for hiding this comment

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

It would be better to move this into selection variable to reduce one layer of nesting.

@the-mikedavis the-mikedavis added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 18, 2022
@archseer archseer merged commit 0c05447 into helix-editor:master May 22, 2022
@EpocSquadron EpocSquadron deleted the epocsquadron/shrink-to-full-lines branch May 22, 2022 12:15
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 15, 2022
* Add shrink equivalent of extend_to_line_bounds

* Add a check for being past rope end in end position calc

* Include the EOL character in calculations

* Bind to `A-x` for now

* Document new keybind
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 15, 2022
* Add shrink equivalent of extend_to_line_bounds

* Add a check for being past rope end in end position calc

* Include the EOL character in calculations

* Bind to `A-x` for now

* Document new keybind
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants