Skip to content

Improve consistency of repeat_last_motion #14478

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Improve consistency of repeat_last_motion #14478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
651a413
Improve consistency of repeat_last_motion
Sep 21, 2025
e11a4f9
remove unused imports
Sep 21, 2025
d46810a
get rid of the should_extend method
Sep 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions helix-core/src/movement.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,7 +634,7 @@ pub fn move_parent_node_end(
text: RopeSlice,
selection: Selection,
dir: Direction,
movement: Movement,
extend: bool,
) -> Selection {
selection.transform(|range| {
let start_from = text.char_to_byte(range.from()) as u32;
Expand Down Expand Up @@ -674,21 +674,21 @@ pub fn move_parent_node_end(
}
};

if movement == Movement::Move {
// preserve direction of original range
if range.direction() == Direction::Forward {
Range::new(end_head, end_head + 1)
} else {
Range::new(end_head + 1, end_head)
}
} else {
if extend {
// if we end up with a forward range, then adjust it to be one past
// where we want
if end_head >= range.anchor {
end_head += 1;
}

Range::new(range.anchor, end_head)
} else {
// preserve direction of original range
if range.direction() == Direction::Forward {
Range::new(end_head, end_head + 1)
} else {
Range::new(end_head + 1, end_head)
}
}
})
}
Expand Down
63 changes: 40 additions & 23 deletions helix-term/src/commands.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1545,7 +1545,7 @@ fn find_char_line_ending(
doc.set_selection(view.id, selection);
}

fn find_char(cx: &mut Context, direction: Direction, inclusive: bool, extend: bool) {
fn find_char(cx: &mut Context, direction: Direction, inclusive: bool) {
// TODO: count is reset to 1 before next key so we move it into the closure here.
// Would be nice to carry over.
let count = cx.count();
Expand All @@ -1559,7 +1559,13 @@ fn find_char(cx: &mut Context, direction: Direction, inclusive: bool, extend: bo
code: KeyCode::Enter,
..
} => {
find_char_line_ending(cx, count, direction, inclusive, extend);
find_char_line_ending(
cx,
count,
direction,
inclusive,
cx.editor.mode == Mode::Select,
);
return;
}

Expand All @@ -1575,12 +1581,22 @@ fn find_char(cx: &mut Context, direction: Direction, inclusive: bool, extend: bo
};
let motion = move |editor: &mut Editor| {
match direction {
Direction::Forward => {
find_char_impl(editor, &find_next_char_impl, inclusive, extend, ch, count)
}
Direction::Backward => {
find_char_impl(editor, &find_prev_char_impl, inclusive, extend, ch, count)
}
Direction::Forward => find_char_impl(
editor,
&find_next_char_impl,
inclusive,
ch,
count,
editor.mode == Mode::Select,
),
Direction::Backward => find_char_impl(
editor,
&find_prev_char_impl,
inclusive,
ch,
count,
editor.mode == Mode::Select,
),
};
};

Expand All @@ -1595,9 +1611,9 @@ fn find_char_impl<F, M: CharMatcher + Clone + Copy>(
editor: &mut Editor,
search_fn: &F,
inclusive: bool,
extend: bool,
char_matcher: M,
count: usize,
extend: bool,
) where
F: Fn(RopeSlice, M, usize, usize, bool) -> Option<usize> + 'static,
{
Expand Down Expand Up @@ -1663,35 +1679,35 @@ fn find_prev_char_impl(
}

fn find_till_char(cx: &mut Context) {
find_char(cx, Direction::Forward, false, false);
find_char(cx, Direction::Forward, false);
}

fn find_next_char(cx: &mut Context) {
find_char(cx, Direction::Forward, true, false)
find_char(cx, Direction::Forward, true)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm this sort of breaks the methods: find and extend now are identical and behave implicitly according to the mode. The whole point was to have two distinct commands so the behavior is more predictable when writing macros

Copy link
Author

@sudonym1 sudonym1 Sep 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, that does stick out like sore thumb. I flip flopped a bit on what I thought made sense here. Options that occurred to me are:

  1. the extend flavors first explicitly enter select mode. If you are already in select mode, then this is a no-op.
  2. the extend flavors get dropped, and macros are expected to explicitly enter a state, or operate within the existing state.
  3. The extend flavors stick around a human readable name, but with no real effect.

IMO the most consistent thing to do is option 2 above, but it will also break custom key maps which is why I left option 3 in place.

}

fn extend_till_char(cx: &mut Context) {
find_char(cx, Direction::Forward, false, true)
find_char(cx, Direction::Forward, false)
}

fn extend_next_char(cx: &mut Context) {
find_char(cx, Direction::Forward, true, true)
find_char(cx, Direction::Forward, true)
}

fn till_prev_char(cx: &mut Context) {
find_char(cx, Direction::Backward, false, false)
find_char(cx, Direction::Backward, false)
}

fn find_prev_char(cx: &mut Context) {
find_char(cx, Direction::Backward, true, false)
find_char(cx, Direction::Backward, true)
}

fn extend_till_prev_char(cx: &mut Context) {
find_char(cx, Direction::Backward, false, true)
find_char(cx, Direction::Backward, false)
}

fn extend_prev_char(cx: &mut Context) {
find_char(cx, Direction::Backward, true, true)
find_char(cx, Direction::Backward, true)
}

fn repeat_last_motion(cx: &mut Context) {
Expand Down Expand Up @@ -5515,8 +5531,9 @@ fn select_prev_sibling(cx: &mut Context) {
select_sibling_impl(cx, object::select_prev_sibling)
}

fn move_node_bound_impl(cx: &mut Context, dir: Direction, movement: Movement) {
fn move_node_bound_impl(cx: &mut Context, dir: Direction) {
let motion = move |editor: &mut Editor| {
let extend = editor.mode == Mode::Select;
let (view, doc) = current!(editor);

if let Some(syntax) = doc.syntax() {
Expand All @@ -5528,7 +5545,7 @@ fn move_node_bound_impl(cx: &mut Context, dir: Direction, movement: Movement) {
text,
current_selection.clone(),
dir,
movement,
extend,
);

doc.set_selection(view.id, selection);
Expand All @@ -5539,19 +5556,19 @@ fn move_node_bound_impl(cx: &mut Context, dir: Direction, movement: Movement) {
}

pub fn move_parent_node_end(cx: &mut Context) {
move_node_bound_impl(cx, Direction::Forward, Movement::Move)
move_node_bound_impl(cx, Direction::Forward)
}

pub fn move_parent_node_start(cx: &mut Context) {
move_node_bound_impl(cx, Direction::Backward, Movement::Move)
move_node_bound_impl(cx, Direction::Backward)
}

pub fn extend_parent_node_end(cx: &mut Context) {
move_node_bound_impl(cx, Direction::Forward, Movement::Extend)
move_node_bound_impl(cx, Direction::Forward)
}

pub fn extend_parent_node_start(cx: &mut Context) {
move_node_bound_impl(cx, Direction::Backward, Movement::Extend)
move_node_bound_impl(cx, Direction::Backward)
}

fn select_all_impl<F>(editor: &mut Editor, select_fn: F)
Expand Down