Skip to content

Conversation

@wmstack
Copy link
Contributor

@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.

Release notes:

  • N/A

@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
Contributor Author

wmstack commented Oct 14, 2024

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:

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

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
Contributor 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
Copy link

cla-bot bot commented Oct 14, 2024

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

@wmstack
Copy link
Contributor Author

wmstack commented Oct 14, 2024

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.

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

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
Contributor 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

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 485a176

@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
Contributor Author

wmstack commented Nov 7, 2024

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.

@wmstack
Copy link
Contributor Author

wmstack commented Nov 12, 2024

@ConradIrwin Well I kind of don't think that the boundary_trail functions should be separated, as both the head and the tail move using the same logic. I have changed the functions to take only the head as input, so as to simplify its mechanics a bit.

However, really, if each function is split into two, we would have 4 functions, and both parts would really look almost identical, they would share much of the same functionality, namely skipping newline characters, and skipping the first boundary even if it lies just after the newlines. The bulk of the code, which determines where to put the new tail at, is also used to move the head forward to start testing for boundaries starting from the correct point.

@BrianTheMessiah
Copy link

BrianTheMessiah commented Dec 3, 2024

https://app.opire.dev/issues/01J6HJZCK02FVD4XG900FY36EE is this bounty still available or has @wmstack claimed it?

@ConradIrwin
Copy link
Member

@BrianTheMessiah I don't think it's been completed yet.

@wmstack I am going to merge this PR, sorry for the delay, I was out last week.

For anyone wanting to try the (very nascent) helix mode out, you can currently access it by creating a key-binding that does ["vim::SwitchMode", "helix_normal"]. A lot more work is needed before we can really expect people to use this though help is welcome :D.

@ConradIrwin ConradIrwin merged commit 8f08787 into zed-industries:main Dec 4, 2024
11 checks passed
@wmstack wmstack deleted the helix-support branch December 4, 2024 06:40
@well1791
Copy link

well1791 commented Dec 4, 2024

is this available on stable release? or preview release? or just by building it locally?

@kranurag7
Copy link

is this available on stable release? or preview release? or just by building it locally?

from the time it was merged which is today, we didn't had any pre-release or stable release of building from main would have it.

@baldwindavid
Copy link
Contributor

Assuming the typical Wednesday Preview Release, it will be in there.

@nabby27
Copy link

nabby27 commented Dec 4, 2024

hI @wmstack I am the creator of the bounty mentioned and seeing your great work and (although the integration is not complete) the issue has already been closed and your PR has been merged, I would be more than happy to pay you the bounty for your effort, you would only need to claim it in Opire and configure your Stripe account to receive the money.

@Siloss
Copy link

Siloss commented Dec 7, 2024

For anyone wanting to try the (very nascent) helix mode out, you can currently access it by creating a key-binding that does ["vim::SwitchMode", "helix_normal"]. A lot more work is needed before we can really expect people to use this though help is welcome :D.

isn't it ["vim::SwitchMode", "HelixNormal"]? Only that option worked for me.

@wmstack
Copy link
Contributor Author

wmstack commented Dec 7, 2024

The way I did the command was:

"bindings": {
      "escape": "vim::HelixNormalAfter"
}

I named it like this because vim::NormalBefore moved the block behind the cursor, while vim::HelixNormalAfter puts the block after the cursor.

@ricardonunez-io
Copy link

Is this still in active development to reach a state of parity or almost-parity with Helix?

Happy to add to the bounty if so.

@freepicheep
Copy link

I tried configuring this in my settings, but still am unable to enter Helix Normal. Can anyone help?

@forivall
Copy link

From my experimentation, this is still very work in progress, but it's promising! My config is here for those who want to use it, although it's missing many of my most common motions and actions. It's good enough for a bit of light editing.

@freepicheep
Copy link

@forivall thanks! That is a great starting place. If it's useful, I immediately added this keybind for the c movement from Helix "c": ["workspace::SendKeystrokes", "d i"] under "context": "vim_mode == helix_normal && !menu".

Is there any way to make Helix normal the default instead of (vim) normal?

@neel04
Copy link

neel04 commented Jan 11, 2025

Is there any way we can manually add some of the more complex keybindings, such as Alt/Option + o for selecting syntactic tokens of code through tree-sitter? And other standard helix keybindings which rely on AST like gw? and proper yanking with y? @forivall

@nyabinary
Copy link

What the remainder of the work needed?

@ConradIrwin
Copy link
Member

Roughly:

  • Creating a setting that enables HelixNormal instead of Normal by default. (Maybe vim: Add ability to change default mode #25067 would be enough)
  • Bundling as many of Helix's bindings as we can reasonably support.
  • Adding more actions to support any bindings that are deemed reasonable, but not implementatable yet.

I think a few people have said they are using helix-like keybindings, so maybe that'd be a good starting point for 2.

@nyabinary
Copy link

Roughly:

* Creating a setting that enables HelixNormal instead of Normal by default. (Maybe [vim: Add ability to change default mode #25067](https://github.com/zed-industries/zed/pull/25067) would be enough)

* Bundling as many of Helix's bindings as we can reasonably support.

* Adding more actions to support any bindings that are deemed reasonable, but not implementatable yet.

I think a few people have said they are using helix-like keybindings, so maybe that'd be a good starting point for 2.

Does Helix have a crate or something we can use instead for keybindings?

@storopoli
Copy link

Does Helix have a crate or something we can use instead for keybindings?

Yes, here: https://github.com/helix-editor/helix/blob/master/helix-term/src/commands.rs

Which is a used in https://github.com/helix-editor/helix/blob/master/helix-term/src/keymap.rs

@nyabinary
Copy link

Pretty afraid Helix support is just going to rot and get eventually removed if someone doesn't step up :P.

@theherk
Copy link
Contributor

theherk commented Apr 23, 2025

I just started testing this out in e15d20a4, and it actually feels really nice. I've never been a big helix / kakoune user, but having dabbled I get the draw. The way this is implemented actually feels like the best of both worlds. Nice work.

@hisirdox
Copy link

@ricardonunez-io

Is this still in active development to reach a state of parity or almost-parity with Helix?

Happy to add to the bounty if so.

if enough people want to use, we can do it.
i am personaly, a heavy neovim user and could work on this

@zetashift
Copy link

@ricardonunez-io

Is this still in active development to reach a state of parity or almost-parity with Helix?
Happy to add to the bounty if so.

if enough people want to use, we can do it. i am personaly, a heavy neovim user and could work on this

Helix user here, the helix bindings in Zed are a bit rough, it doesn't have default keybindings that work well with Zeds multicursors and the keymap of m(match around, inside and surround) is hard to replicate. Also f and t don't seem to be working.

So it depends on what people expect from helix keybindings. Personally I'd already be pretty happy if the match thingy gets parity.

@hisirdox
Copy link

i think the user expects the same use of helix, so they can just migrate to zed
not that it makes sense, or i am right on my view of the situation

@zetashift
Copy link

i think the user expects the same use of helix,

I get what you're saying, but even the Vim mode in Zed has some tiny differences and helix includes a lot out of the box, e.g. jumping to a word in the visible area of the buffer. Users chose helix for different kinds of reasons.

I don't think the first thing to implement is the multicursor feature of helix (s) because there is no real Zed equivalent for that yet. Instead ergonomic keybindings to the current Zed cursor functionality would be a better trade-off.

Instead I'd recommend more keybindings that enable select->act functionality are easy wins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed The user has signed the Contributor License Agreement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Helix keymap