vim: Fix Helix mode subword motions to select traversed text#45760
vim: Fix Helix mode subword motions to select traversed text#45760dinocosta merged 13 commits intozed-industries:mainfrom
Conversation
|
Thanks for this! |
dinocosta
left a comment
There was a problem hiding this comment.
Hey @maxmalkin, thanks for tackling this!
I’m not entirely sure how, or whether, this is actually supported in Helix, but it’s worth noting that the current implementation uses only _ as the word boundary. In the past, some folks have complained that it doesn’t quite behave like Vim’s CamelCaseMotion.
For example, #48276 updated Vim mode’s implementation to also treat $ and = as word-boundary characters, so I figured I’d flag that 🙂
Head branch was pushed to by a user without write access
|
Hi @dinocosta! That's a good catch, it is better in terms of UX to have |
There was a problem hiding this comment.
Thank you for tackling the suggested changes @maxmalkin ! 🙂
It seems that there's now a failing test, helix::test::test_subword_motions, let me know if you're able to see what's going on there, otherwise I can try and take a look. I've also merged main into your branch to ensure it stays up to date.
P.S.: Also added a release note which was missing from the PR's description
|
Hey @dinocosta ! I'm having some issues with |
dinocosta
left a comment
There was a problem hiding this comment.
Hey @maxmalkin !
Unfortunately it seems your latest commit – 0407183 – doesn't actually fix the failing test, as it actually deleted assertions from existing tests.
Let me know if you're still unable to run tests on your end and I can try and take a look 🙂
|
Hi @dinocosta! Looks like it's still unable to run tests on my end, I would appreciate it if you could take a look! Thanks! |
* Extract `vim::motion::is_next_subword_start` * Update `vim::motion::next_subword_start` to use `vim::motion::is_next_subword_start` * Update helix's handling of `NextSubwordStart` to also leverage `vim::motion::is_next_subword_start` * Introduce new `vim::helix::test::test_next_subword_start` test to mirror the behavior that we also have on vim, with the exception that vim's implementation takes multiple empty lines into consideration, which Helix does not do right now, but I don't think it's breaking for these changes
I've only now just found that there's actually support for subword motions in Helix, helix-editor/helix#8147 . As such, it's probably better to try and mimic those as closely as possible. In turn, this means we likely can't re-use the vim motion's implementation of `is_next_subword_start`, so, this commit reverts that change and introduces a Helix specific `is_next_subword_start` implementation that is now being used. The tests have also been cross-checked and updated in order to match the behavior in Helix.
* Revert the changes that introduced `vim::helix::is_next_subword_start`, turns out we actually don't need that * Update both `vim::helix::test::test::test_next_subword_end` and `vim::helix::test::test::test_previous_subword_start` to ensure that the behavior matches the existing Helix's subword motions * The `PreviousSubwordEnd` tests have not been updated yet, as it seems Helix doesn't actually offer that specific motion, so I still need to figure out how it should actually behave.
* Add `vim::helix::Vim::is_previous_subword_start` * Add `vim::helix::Vim::is_previous_subword_end` * Remove `vim::helix::test::test_subword_motions` as that attempted to test all motions in a single test and it's easier to reason about and fix motions if they're split into their own tests, which are now: * `vim::helix::test::test_next_subword_start` * `vim::helix::test::test_next_subword_end` * `vim::helix::test::test_previous_subword_start` * `vim::helix::test::test_previous_subword_end` The behavior in this test has been checked against Helix's subword commands, namely: * `move_next_sub_word_start` * `move_prev_sub_word_start` * `move_next_sub_word_end` * `move_prev_sub_word_end`
* Introduce simple `vim::helix::Vim::subword_boundary_start` and `vim::helix::Vim::subword_boundary_end` in order to replace `vim::helix::Vim::is_previous_subword_start`, `vim::helix::Vim::is_previous_subword_end`,`vim::helix::Vim::is_subword_boundary_left` and `vim::helix::Vim::is_subword_boundary_right`. * Remove leftover `dbg!` calls.
Closes #41767
Release Notes:
In Helix mode, word motions select the text they traverse, but subword motions were only moving the cursor without selecting. This was because
helix_move_cursorhad explicit handling for word motions usinghelix_find_range_forwardandhelix_find_range_backward, but subword motions fell back tohelix_move_and_collapsehandler which collapses the selection.Changes and Tests:
is_subword_boundary_rightandis_subword_boundary_leftfunctions for subword-specific boundary detectionhelix_move_cursortest_subword_motionsRelease Notes: