-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
align selections #1101
Conversation
helix-term/src/keymap.rs
Outdated
@@ -601,6 +601,7 @@ impl Default for Keymaps { | |||
// "q" => record_macro, | |||
// "Q" => replay_macro, | |||
|
|||
"&" => align_lines, | |||
// & align selections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// & align selections |
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 |
@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. |
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. |
Here's something that works for me: select all the columns then use |
Updated the code, shall work well with select_regex, but maybe not with split_selection without move cursor. |
Co-authored-by: Ivan Tham <[email protected]>
helix-term/src/commands.rs
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this 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 🎉
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 Awesome work btw @QiBaobin 🎉 |
Select above lines,
&
and input\|
would turn them to2&
and input\|
would turn them to3&
and input\|
would turn them to