feat: vi copy mode features for '*' / '#' / 'o' #8099
Conversation
fmgornick
changed the title
alacritty/src/config/bindings.rs
Outdated
| /// Search forward for highlighted region or word under cursor. | ||
| SearchRegionForward, | ||
| /// Search backward for highlighted region or word under cursor. | ||
| SearchRegionBackward, |
There was a problem hiding this comment.
I find the name "region" extremely confusing here, why did you name it that way? We already have semantic bindings, iirc they're just called "…SemanticWord…"
There was a problem hiding this comment.
There's no SemanticWord binding, but there are SemanticDir motions. I agree region is a bit weird but the only time we search for the semantic word is if we're in normal mode. In the case of visual mode we want to search for the highlighted selection, so i chose "region" as a sort of catch-all
There was a problem hiding this comment.
does SearchCursorForward and SearchCursorBackward make more sense?
alacritty/src/event.rs
Outdated
| Some(_) => { | ||
| let text = term.selection_to_string().filter(|s| !s.is_empty()); | ||
| self.clear_selection(); | ||
| text | ||
| }, |
There was a problem hiding this comment.
Does this require vi mode? I feel like in theory it should work just fine outside of it. The selection itself can be used as origin in both modes.
alacritty/src/event.rs
Outdated
| let escape_chars = term.semantic_escape_chars(); | ||
| let mut iter = term.grid().iter_from(cursor_point); | ||
|
|
||
| // if inside word, move left to start | ||
| while !escape_chars.contains(iter.cell().c) { | ||
| iter.prev(); | ||
| } | ||
| if iter.point() != cursor_point { | ||
| iter.next(); | ||
| } | ||
|
|
||
| // if not in word, move right to start | ||
| while escape_chars.contains(iter.cell().c) | ||
| && iter.point().column != term.grid().last_column() | ||
| { | ||
| iter.next(); | ||
| } | ||
| if escape_chars.contains(iter.cell().c) { | ||
| return; | ||
| } | ||
|
|
||
| iter.prev(); | ||
| Some( | ||
| iter.take_while(|cell| !escape_chars.contains(cell.c)) | ||
| .map(|cell| cell.c) | ||
| .collect(), | ||
| ) | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Haven't looked at this too closely, but this looks very much like our existing semantic search implementation. We already have logic for semantic search, I don't think we need to implement it again.
There was a problem hiding this comment.
I initially tried using the semantic_search functions, but there are the edge cases where the cursor is on the first letter of a word and we search backwards to the previous word and similarly with the right end, when i had a solution with that it just looked messy and was about the same length anyway. I can give it another shot tho!
alacritty/src/event.rs
Outdated
| // escape regex search characters | ||
| let mut regex = "".to_string(); | ||
| text.chars().for_each(|c| { | ||
| if ".*+^$?\\()[]{}|".contains(c) { | ||
| regex.push('\\'); | ||
| } | ||
| regex.push(c); | ||
| }); |
There was a problem hiding this comment.
Allocation can be trivially improved here.
alacritty_terminal/src/selection.rs
Outdated
| /// Swap ends of highlighted region, returning new end of region location. | ||
| pub fn swap_ends(&mut self) -> Point { | ||
| let (start, end) = (self.region.start, self.region.end); | ||
| self.region.start = Anchor::new(end.point, end.side); | ||
| self.region.end = Anchor::new(start.point, start.side); | ||
| start.point | ||
| } |
There was a problem hiding this comment.
What is the motivation of this function? Why are we randomly swapping things here? I don't think we want to actually mutate the selection?
There was a problem hiding this comment.
when moving the cursor to the other side of a region, we also need to make the selection range go in the other direction, so we must swap our anchors. otherwise moving the cursor from the end of the selection to the beginning will empty the selection. Also just doing mem::swap(&mut start, &mut end) doesn't work.
|
sorry for taking so long! i sat on your comments for a while and i think this addresses most of them...
|
There was a problem hiding this comment.
I have no interest in investing further reviews into this for now since it clearly requires a lot of guidance. I don't want to outright close this since the search_cursor function is basically the only thing that needs significant improvement, but I don't see that getting better without doing it myself.
fmgornick
force-pushed
the
vi-features
branch
4 times, most recently
from
69f1a69
to
864bcb6
Compare
- '*': search forward for current visual selection if in visual
mode, or search for word under cursor in normal mode.
- '#': search backward for current visual selection if in visual
mode, or search for word under cursor in normal mode.
- 'o': toggle cursor position between beggining and end of text
selection in visual mode.
i apologize, for i have been a silly goose 😔🤚, fixed |
Hi! I saw this issue and thought it'd be an easy way to hopefully start contributing to one of my fav projects! I added the 3 new commands and updated the scdoc files accordingly. I briefly describe the behavior with these new changes below...
Upon pressing
*in visual mode, search for the highlighted text in the region, and in normal mode, grab the first semantic word at/after the cursor point in the same line, searching for that.#similarly searches backwards.This matches any word containing the search pattern, unlike vim which matches the exact pattern. I tried getting the search to behave exactly like vim, but wrapping the region in "\<\>" didn't work, nor did using the negative lookahead/lookbehind operators (
(?!),(?<!)), but i figured this was prolly good enough.Also added the visual
okeybinding, which will move the cursor to the other side of a visual region. Works as expected for all visual modes. Contemplated adding theOkey for switching the column of a visual block, but i doubt anyone even knows about this key, let alone has use for it.