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.

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

Jump to bottom

Implement Helix Support (WIP) #19175

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Implement Helix Support (WIP) #19175

wants to merge 11 commits into from
+473 −9

Conversation

wmstack
Copy link

@wmstack wmstack commented Oct 14, 2024

Closes #4642

  • Added the ability to switch to helix normal mode, with an additional helix visual mode.
  • ctrlh from Insert mode goes to Helix Normal mode. i and a to go back.
  • Need to find a way to perform the helix normal mode selection with w , e , b as a first step. Need to figure out how the mode will interoperate with the VIM mode as the new additions are in the same crate.

@cla-bot cla-bot
Copy link

cla-bot bot commented Oct 14, 2024

We require contributors to sign our Contributor License Agreement, and we don't have @wmstack on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'.

@wmstack
Copy link
Author

wmstack commented Oct 14, 2024 •
edited
Loading

I am trying to understand how to implement the Helix signature motions in normal mode. I think the most helpful piece of the code is this visual motion piece, which I am putting here to study:

zed/crates/vim/src/visual.rs

Lines 119 to 197 in ec5d6e9

pub fn visual_motion(
&mut self,
motion: Motion,
times: Option<usize>,
cx: &mut ViewContext<Self>,
) {
self.update_editor(cx, |vim, editor, cx| {
let text_layout_details = editor.text_layout_details(cx);
if vim.mode == Mode::VisualBlock
&& !matches!(
motion,
Motion::EndOfLine {
display_lines: false
}
)
{
let is_up_or_down = matches!(motion, Motion::Up { .. } | Motion::Down { .. });
vim.visual_block_motion(is_up_or_down, editor, cx, |map, point, goal| {
motion.move_point(map, point, goal, times, &text_layout_details)
})
} else {
editor.change_selections(Some(Autoscroll::fit()), cx, |s| {
s.move_with(|map, selection| {
let was_reversed = selection.reversed;
let mut current_head = selection.head();
// our motions assume the current character is after the cursor,
// but in (forward) visual mode the current character is just
// before the end of the selection.
// If the file ends with a newline (which is common) we don't do this.
// so that if you go to the end of such a file you can use "up" to go
// to the previous line and have it work somewhat as expected.
#[allow(clippy::nonminimal_bool)]
if !selection.reversed
&& !selection.is_empty()
&& !(selection.end.column() == 0 && selection.end == map.max_point())
{
current_head = movement::left(map, selection.end)
}
let Some((new_head, goal)) = motion.move_point(
map,
current_head,
selection.goal,
times,
&text_layout_details,
) else {
return;
};
selection.set_head(new_head, goal);
// ensure the current character is included in the selection.
if !selection.reversed {
let next_point = if vim.mode == Mode::VisualBlock {
movement::saturating_right(map, selection.end)
} else {
movement::right(map, selection.end)
};
if !(next_point.column() == 0 && next_point == map.max_point()) {
selection.end = next_point;
}
}
// vim always ensures the anchor character stays selected.
// if our selection has reversed, we need to move the opposite end
// to ensure the anchor is still selected.
if was_reversed && !selection.reversed {
selection.start = movement::left(map, selection.start);
} else if !was_reversed && selection.reversed {
selection.end = movement::right(map, selection.end);
}
})
});
}
});
}

This in turn runs the visual_block_motion

zed/crates/vim/src/visual.rs

Lines 199 to 305 in ec5d6e9

pub fn visual_block_motion(
&mut self,
preserve_goal: bool,
editor: &mut Editor,
cx: &mut ViewContext<Editor>,
mut move_selection: impl FnMut(
&DisplaySnapshot,
DisplayPoint,
SelectionGoal,
) -> Option<(DisplayPoint, SelectionGoal)>,
) {
let text_layout_details = editor.text_layout_details(cx);
editor.change_selections(Some(Autoscroll::fit()), cx, |s| {
let map = &s.display_map();
let mut head = s.newest_anchor().head().to_display_point(map);
let mut tail = s.oldest_anchor().tail().to_display_point(map);
let mut head_x = map.x_for_display_point(head, &text_layout_details);
let mut tail_x = map.x_for_display_point(tail, &text_layout_details);
let (start, end) = match s.newest_anchor().goal {
SelectionGoal::HorizontalRange { start, end } if preserve_goal => (start, end),
SelectionGoal::HorizontalPosition(start) if preserve_goal => (start, start),
_ => (tail_x.0, head_x.0),
};
let mut goal = SelectionGoal::HorizontalRange { start, end };
let was_reversed = tail_x > head_x;
if !was_reversed && !preserve_goal {
head = movement::saturating_left(map, head);
}
let Some((new_head, _)) = move_selection(map, head, goal) else {
return;
};
head = new_head;
head_x = map.x_for_display_point(head, &text_layout_details);
let is_reversed = tail_x > head_x;
if was_reversed && !is_reversed {
tail = movement::saturating_left(map, tail);
tail_x = map.x_for_display_point(tail, &text_layout_details);
} else if !was_reversed && is_reversed {
tail = movement::saturating_right(map, tail);
tail_x = map.x_for_display_point(tail, &text_layout_details);
}
if !is_reversed && !preserve_goal {
head = movement::saturating_right(map, head);
head_x = map.x_for_display_point(head, &text_layout_details);
}
let positions = if is_reversed {
head_x..tail_x
} else {
tail_x..head_x
};
if !preserve_goal {
goal = SelectionGoal::HorizontalRange {
start: positions.start.0,
end: positions.end.0,
};
}
let mut selections = Vec::new();
let mut row = tail.row();
loop {
let laid_out_line = map.layout_row(row, &text_layout_details);
let start = DisplayPoint::new(
row,
laid_out_line.closest_index_for_x(positions.start) as u32,
);
let mut end =
DisplayPoint::new(row, laid_out_line.closest_index_for_x(positions.end) as u32);
if end <= start {
if start.column() == map.line_len(start.row()) {
end = start;
} else {
end = movement::saturating_right(map, start);
}
}
if positions.start <= laid_out_line.width {
let selection = Selection {
id: s.new_selection_id(),
start: start.to_point(map),
end: end.to_point(map),
reversed: is_reversed,
goal,
};
selections.push(selection);
}
if row == head.row() {
break;
}
if tail.row() > head.row() {
row.0 -= 1
} else {
row.0 += 1
}
}
s.select(selections);
})
}

@wmstack
Copy link
Author

wmstack commented Oct 14, 2024

@cla-bot check

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Oct 14, 2024
@cla-bot cla-bot
Copy link

cla-bot bot commented Oct 14, 2024

The cla-bot has been summoned, and re-checked this pull request!

@wmstack
Copy link
Author

wmstack commented Oct 14, 2024 •
edited
Loading

Okay, I have implemented some semblance of Helix's w, e and b. The only one that doesn't work as intended is the e motion. It is supposed to go the the letter at the end of the word, with the tail at the old head's position. It falls short by one letter.

@maan2003
Copy link
Contributor

@wmstack I had implemented lot of helix bindings in my old PR #13768

@ConradIrwin
Copy link
Member

Thanks for this! I'm struggling to decide what to do with Helix: there's clearly a lot of demand for it, but I know that there's enough to do in vim to keep me busy forever :D.

As mentioned before, I think it'd be ok to merge a WIP helix mode in such a way that it's not impacting the overall user/developer experience, and I think this PR is a reasonable approach (#17575 goes a completely different way, and I also think that is reasonable - but this seems less intrusive somehow).

Back to your questions:

One thing that causes problems in vim (and probably will in helix too) is that a collapsed selection in zed is represented as the position before the character that the block cursor is under; whereas an expanded selection spans from before the first character to after the last.

In vim you can only be in normal mode when selections are collapsed, but in helix, normal mode may have expanded or collapsed selections.

zed/crates/vim/src/visual.rs

Lines 142 to 193 in 56163b1

let was_reversed = selection.reversed;
let mut current_head = selection.head();
// our motions assume the current character is after the cursor,
// but in (forward) visual mode the current character is just
// before the end of the selection.
// If the file ends with a newline (which is common) we don't do this.
// so that if you go to the end of such a file you can use "up" to go
// to the previous line and have it work somewhat as expected.
#[allow(clippy::nonminimal_bool)]
if !selection.reversed
&& !selection.is_empty()
&& !(selection.end.column() == 0 && selection.end == map.max_point())
{
current_head = movement::left(map, selection.end)
}
let Some((new_head, goal)) = motion.move_point(
map,
current_head,
selection.goal,
times,
&text_layout_details,
) else {
return;
};
selection.set_head(new_head, goal);
// ensure the current character is included in the selection.
if !selection.reversed {
let next_point = if vim.mode == Mode::VisualBlock {
movement::saturating_right(map, selection.end)
} else {
movement::right(map, selection.end)
};
if !(next_point.column() == 0 && next_point == map.max_point()) {
selection.end = next_point;
}
}
// vim always ensures the anchor character stays selected.
// if our selection has reversed, we need to move the opposite end
// to ensure the anchor is still selected.
if was_reversed && !selection.reversed {
selection.start = movement::left(map, selection.start);
} else if !was_reversed && selection.reversed {
selection.end = movement::right(map, selection.end);
}
})
is probably the logic you need to copy (conditional on whether the selections are expanded or collapsed).

If you'd like to pair on this, please book time here: https://calendly.com/conradirwin/pairing (@maan2003 let me know if you want to join too, and I'll send you the link).

@ConradIrwin ConradIrwin self-assigned this Oct 15, 2024
@baldwindavid
Copy link
Contributor

baldwindavid commented Oct 16, 2024 •
edited
Loading

I'm struggling to decide what to do with Helix: there's clearly a lot of demand for it, but I know that there's enough to do in vim to keep me busy forever :D.

I use and like Helix a lot, but part of its allure is the opinionated and constrained package. Dumping some Helix concepts into Zed doesn't really make it Helix and potentially adds a lot of expectations and confusion in terms of maintenance and feature parity. I wonder if it's possible to codify the name of the feature being added for clarity and to limit expectations.

From what I can tell the desire is mostly for the object-verb grammar as opposed to verb-object. Object-verb is not even a Helix thing; it's lifted from Kakoune. At any rate, I wonder if this could be slotted into the existing vim settings with something like:

"vim_mode": true,
"vim": {
  "grammar": "helix (or kakoune)" // default "vim" (or vi if I'm being so pedantic :))
},

or

"vim_mode": true,
"vim": {
  "grammar": "object_verb" // default "verb_object"
},

If that doesn't prove to be enough then perhaps in the future a more full-blown Helix/Kak extension similar to VSCode's Dance emerges down the road.

@wmstack
Copy link
Author

wmstack commented Oct 20, 2024

Okay word movements w e and b are working as expected:

W.E.and.B.motions.mov

@zed-industries-bot
Copy link

zed-industries-bot commented Oct 21, 2024 •
edited
Loading

Warnings
⚠️

This PR is missing release notes.

Please add a "Release Notes" section that describes the change:

Release Notes:

- Added/Fixed/Improved ...

If your change is not user-facing, you can use "N/A" for the entry:

Release Notes:

- N/A

Generated by 🚫 dangerJS against 3ccb000

@ConradIrwin
Copy link
Member

Nice!

Before I merge this I'd like to talk things through with you.

Also, I think we should:

  • add a test
  • remove the HelixVisual placeholder
  • remove the ctrl-h keybinding and add something to the docs explaining how people can try this,

@wmstack
Copy link
Author

wmstack commented Nov 7, 2024 •
edited
Loading

Apologies for the delay. When I first submitted the pull request, I had upcoming university exams that required my focus. After completing those exams, I found it challenging to regain the motivation to continue with this. Thanks for your patience.

I added a test for next word start, and I discovered that it wasn't actually working as intended for the word before a new line it selected the newline character too. So I fixed that.

Also, I am aware of a problem where when moving one character right when there is a selection, a character is skipped (as can be seen in the video above). This probably happens for the reason you mentioned before. However, as far as I am aware, the word motions are all correct, but I haven't tested them extensively and there are a number of edges cases.

I have booked a meeting as I am interested to discuss the next steps. I feel like the issue should be open until a significant number of helix commands are functional.

dunxen
dunxen reviewed Nov 7, 2024
View reviewed changes
assets/keymaps/vim.json Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dunxen dunxen dunxen left review comments

Assignees

@ConradIrwin ConradIrwin

Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Helix keymap
6 participants
@wmstack @maan2003 @ConradIrwin @baldwindavid @zed-industries-bot @dunxen