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

align selections #1101

Merged
merged 9 commits into from
Nov 23, 2021
Merged

align selections #1101

merged 9 commits into from
Nov 23, 2021

Conversation

QiBaobin
Copy link
Contributor

Screen Shot 2021-11-14 at 10 01 06 PM

Select above lines, & and input \| would turn them to

Screen Shot 2021-11-14 at 10 01 25 PM

2& and input \| would turn them to
Screen Shot 2021-11-14 at 10 01 38 PM

3& and input \| would turn them to
Screen Shot 2021-11-14 at 10 01 52 PM

@@ -601,6 +601,7 @@ impl Default for Keymaps {
// "q" => record_macro,
// "Q" => replay_macro,

"&" => align_lines,
// & align selections
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// & align selections

@archseer
Copy link
Member

Rather than using a regex for the delimiter, this should align selections in the same line: http://kakoune.org/video/align.webm

^ it selects the whole line, splits into sub selections, moves cursors to the start of the selection, then & aligns the cursors.

https://github.com/mawww/kakoune/blob/e7100dc87434d933fd8268e5bf70080b627750c5/src/normal.cc#L1615-L1701

@QiBaobin
Copy link
Contributor Author

@archseer , I tried to implement using selections, one problem is that we the cursor is not at the selection we want(in another word, it's at a part of the separators), it would make cursor as a selection too.

@archseer
Copy link
Member

What do you mean? Can you show me an example?

In general we want commands to always take advantage of multiple selections, it's more idiomatic for helix.

@QiBaobin
Copy link
Contributor Author

QiBaobin commented Nov 15, 2021

When I used split_selections with \| after selecting two lines, I got below selections, as you can see the selections in the first line doesn't make sense.

Screen Shot 2021-11-15 at 9 58 24 AM

Just checked select_regex with. [^|]+, it seems to work in this case.
Screen Shot 2021-11-15 at 10 02 10 AM

@archseer
Copy link
Member

Here's something that works for me: select all the columns then use s\w<enter>, mi|, _ and then alt-; to jump to the start.

https://asciinema.org/a/q7Jd8EgPQ6OVMZ4aAWCTGTjJt

@QiBaobin
Copy link
Contributor Author

Updated the code, shall work well with select_regex, but maybe not with split_selection without move cursor.

@QiBaobin QiBaobin changed the title align lines align selections Nov 15, 2021
QiBaobin and others added 2 commits November 19, 2021 13:28
Co-authored-by: Ivan Tham <pickfire@riseup.net>
Comment on lines 687 to 689
2 => s.insert_str(s.len() / 2, trimed), // center align
3 => s.push_str(trimed), // right align
1 => s.insert_str(0, trimed), // left align
Copy link
Contributor

@pickfire pickfire Nov 20, 2021

Choose a reason for hiding this comment

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

Suggested change
2 => s.insert_str(s.len() / 2, trimed), // center align
3 => s.push_str(trimed), // right align
1 => s.insert_str(0, trimed), // left align
1 => s.insert_str(0, trimed), // left align
2 => s.insert_str(s.len() / 2, trimed), // center align
3 => s.push_str(trimed), // right align

Also, I think it's better to create an enum or reuse an existing one (if possible) for this, so we don't need unimplemented!.

Also, I think it is better to use https://doc.rust-lang.org/std/primitive.slice.html#method.swap_with_slice swap_with_slice rather than having multiple allocations. We can just swap it twice and it should move all the whitespaces. The alternative is to use String::with_capacity and push it manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

swap_with_slice need two mut str or string, but the trimmed or the fragment is not own by us, to use it we have make another string, which is same thing, right?

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.

Great work! Tested locally 🎉

@archseer archseer merged commit 21143e8 into helix-editor:master Nov 23, 2021
@sudormrfbin
Copy link
Member

We also need some docs about this in the book in the usage section, especially since the workflow might be confusing at first (using cursors to align) and left, right, center alignment are controlled by counts before &. Bonus points for a gif :)

Awesome work btw @QiBaobin 🎉

@QiBaobin QiBaobin deleted the align branch November 24, 2021 00:36
@QiBaobin QiBaobin mentioned this pull request Feb 16, 2022
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