Add paragraph textobject #784
Conversation
| // TODO: add textobject for other types like parenthesis | ||
| // TODO: cancel new ranges if inconsistent surround matches across lines | ||
| // ch if !ch.is_ascii_alphanumeric() => { | ||
| // textobject::textobject_surround(text, range, objtype, ch, count) | ||
| // } |
There was a problem hiding this comment.
@sudormrfbin We may have to tweak the current keymap data format to be able to store extra hooks and process it for infobox to work, at the same time we need to think of what to put on the left side of the infobox since we usually display the original key but now it can be any key.
There was a problem hiding this comment.
I think a better way to do this is to introduce a method to dynamically update the infobox, something like so:
fn select_textobject(cx: &mut Context, objtype: textobject::TextObject) {
let count = cx.count();
cx.editor.set_autoinfo(hashmap!(
"w" => "Word",
"p" => "Paragraph",
"(,{,\",[" => "Surround",
));
cx.on_next_key(move |cx, event| { /* ... */ }
}There was a problem hiding this comment.
I think a better way is to store the key in the context.
There was a problem hiding this comment.
I think a better way is to store the
keyin the context.
I don't quite understand... could you elaborate ? If we have something like the set_autoinfo method we can display arbitrary information in the keys and description. For example for surround we can show (,{,",[ and not just a single key.
There was a problem hiding this comment.
I mean make key part of Context so that autoinfo can use the current flow with some tweak to allow a wildcard, then on keypress set the Context key so that the function can know what key is pressed.
helix-core/src/textobject.rs
Outdated
| @@ -45,6 +49,54 @@ fn find_word_boundary(slice: RopeSlice, mut pos: usize, direction: Direction) -> | |||
| pos | |||
| } | |||
|
|
|||
| fn find_paragraph_boundary(slice: RopeSlice, mut pos: usize, direction: Direction) -> usize { | |||
| // if pos is at non-empty line ending or when going forward move one character left | |||
| if (!char_is_line_ending(slice.char(pos.saturating_sub(1).min(slice.len_chars()))) | |||
There was a problem hiding this comment.
Regarding adding/subtracting from pos: keep in mind that CRLF is a possible line ending as well, and is actually two characters. I don't know if that actually matters here, but I'm wondering if this code might do weird things in documents with CRLF line endings. We might need to replace some of these with calls to helix_core::graphemes::prev/next_grapheme_boundary().
(In fact, as a general rule, it's best to work in terms of graphemes first, and then switch to char index addition/subtraction only as an optimization in places where we can prove it's correct. Direct manipulation of char indices is usually incorrect.)
There was a problem hiding this comment.
I believe both \r and \n is considered newlines right? Here I just want to check the previous character, it doesn't really matter what kind of line ending is it as long as it's line ending.
But I think the next part needs reworking if you talk about CRLF because I do + 1 for characters when newlines are there, it is probably wrong.
There was a problem hiding this comment.
I switched some parts to use graphemes already but I am not sure how to switch the iterator below to use grapheme.
| match direction { | ||
| Direction::Forward if pos + count == slice.len_chars() => slice.len_chars(), | ||
| // subtract by 1 due to extra \n when going forward | ||
| Direction::Forward if prev_line_ending => pos + count.saturating_sub(1), | ||
| Direction::Forward => pos + count, | ||
| // iterator exhausted so it should be 0 | ||
| Direction::Backward if pos.saturating_sub(count) == 0 => 0, | ||
| // subtract by 2 because it starts with \n and have 2 extra \n when going backwards | ||
| Direction::Backward if prev_line_ending => pos.saturating_sub(count.saturating_sub(2)), | ||
| // subtract by 1 due to extra \n when going backward | ||
| Direction::Backward => pos.saturating_sub(count.saturating_sub(1)), | ||
| } |
There was a problem hiding this comment.
@cessen Thanks for the review, after you remind me about CRLF I think this part have potential issues. I guess I need to iterate through them rather then just subtract or add here then.
There was a problem hiding this comment.
This part is good, but the part above this haven't figure out how to switch it yet. #784 (comment)
Also, I am still fixing on the goto next paragraph, haven't figured that out yet.
pickfire
force-pushed
the
paragraph
branch
2 times, most recently
from
311253c
to
e8649c9
Compare
| @@ -333,8 +338,18 @@ impl Command { | |||
| surround_add, "Surround add", | |||
| surround_replace, "Surround replace", | |||
| surround_delete, "Surround delete", | |||
| select_textobject_around, "Select around object", | |||
| select_textobject_inner, "Select inside object", | |||
| select_textobject_around_word, "Word", | |||
There was a problem hiding this comment.
I think it would be easier to review if the other textobject changes were a separate PR (I quite like it btw, the infobox works with it too !)
There was a problem hiding this comment.
Yeah, I didn't separate it out because I am lazy to split it out.
Temporarily disable other textobject besides word and paragraph since it integrates nicely with infobox. The behavior is based on vim and kakoune but with some differences, it can select whitespace only paragraph like vim and it can select backward from the end of the file like kakoune.
|
Closing this as I am gonna redo the implementation, current version is too complicated, I am probably going to base the behavior off kakoune and take some code from there. |
Temporarily disable other textobject besides word and paragraph since it
integrates nicely with infobox. The behavior is based on vim and kakoune
but with some differences, it can select whitespace only paragraph like
vim and it can select backward from the end of the file like kakoune.