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 trim_selections command #1092

Merged
merged 1 commit into from
Nov 14, 2021
Merged

Conversation

ath3
Copy link
Contributor

@ath3 ath3 commented Nov 13, 2021

No description provided.

static START_REGEX: Lazy<Regex> = Lazy::new(|| Regex::new(r"^\s+\S").unwrap());
static END_REGEX: Lazy<Regex> = Lazy::new(|| Regex::new(r"\S\s+$").unwrap());
Copy link
Member

Choose a reason for hiding this comment

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

I think using regex is overkill. You should be able to fetch the range.fragment() then use skip_while ch.is_whitespace(): https://github.com/helix-editor/helix/blob/master/helix-core/src/movement.rs#L150-L181

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thats what i originally did, then changed to regex cause of the ending whitespaces, good thing i made backup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind, i didnt know about the backward search, seems like trim_selections will be the first one to use it 🕺

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason backwards_skip_while is starting from pos+1, is there a reason for that or should i change it?

Copy link
Member

Choose a reason for hiding this comment

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

I think that was so that the current pos is included once you reverse the direction. chars().rev() would skip pos and instead start with pos-1

Copy link
Member

Choose a reason for hiding this comment

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

Though maybe that's not the case and you can remove that. I see other code simply reversing https://github.com/helix-editor/helix/blob/b824e091a948c076a428fb981cd5be2929378533/helix-core/src/textobject.rs#L19-21

@ath3 ath3 force-pushed the trim-selections branch 2 times, most recently from 0586770 to d6a2080 Compare November 13, 2021 06:18
+ 1;
}

if start > end {
Copy link
Member

Choose a reason for hiding this comment

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

This happens only when the whole range is whitespace right? Those ranges should be removed altogether, you can use ranges = filter_map+collect instead of transform to construct a new selection. (You'll need to check !ranges.is_empty?() and fail because a selection needs at least one range)

let mut end = range.to();

start = movement::skip_while(text, start, |x| x.is_whitespace()).unwrap_or(start);
if end > 0 {
Copy link
Member

@archseer archseer Nov 13, 2021

Choose a reason for hiding this comment

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

Rather than this check for > 0 then -1 I think you have to use prev_grapheme_boundary

Copy link
Member

Choose a reason for hiding this comment

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

If backwards_skip_while returns none then you also end up increasing end by 1 here (end = end+1)

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I think you want to use range.cursor

pub fn cursor(self, text: RopeSlice) -> usize {

This way you have an inclusive range between range.anchor and range.cursor()(you have to check which one is start and which one is end based on orientation though)

@ath3 ath3 force-pushed the trim-selections branch 3 times, most recently from 19f4914 to 4c810ec Compare November 14, 2021 00:09
@pickfire
Copy link
Contributor

What is the use case for this?

@EpocSquadron
Copy link
Contributor

@pickfire this is a feature kakoune includes. One use case is when you have a selection spanning many lines of code at different indentation levels. You use split on newlines A-s to get a selection for each line of code, then want to do something with those lines that won't work if the leading whitespace is present. So trim selections _ let's you get just the code without first collapsing to the first non-empty character gs and re-select the line remainder vgl<esc>.

It's a bit situational but I have used it organically (meaning I didn't reach for it just to practice it's usage but because it was the most natural thing that came to mind to enable what I was doing) in the past.

@ath3 ath3 requested a review from archseer November 14, 2021 03:51
Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

Tested locally, thanks! 🎉

@archseer archseer merged commit 6fa76d9 into helix-editor:master Nov 14, 2021
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.

None yet

5 participants