vim: Fix visual mode entry at line end near trailing newline#50709
vim: Fix visual mode entry at line end near trailing newline#50709dinocosta merged 12 commits intozed-industries:mainfrom
Conversation
|
We require contributors to sign our Contributor License Agreement, and we don't have @SkandaBhat 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'. |
|
@cla-bot check |
|
We require contributors to sign our Contributor License Agreement, and we don't have @SkandaBhat 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'. |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
dinocosta
left a comment
There was a problem hiding this comment.
Hey @SkandaBhat 👋
Thank you so much for picking this one up! I can reproduce the bug you’re working on, but after looking at the changes, I’m not sure Vim::switch_mode is the right place to handle this 🤔
From what I can tell, this bug only appears when using Helix mode, not Vim mode itself. It seems tied to how the line selection is calculated when the cursor is on the newline character, which isn’t possible in Vim mode unless you’re already in a visual mode.
With that in mind, I wonder if the fix should instead live in how we handle the vim::HelixSelectLine action and its motion. Testing in Helix, if the cursor is at the last position of the line and you press x, the whole line is selected but the cursor stays put. In Zed’s implementation, x selects the whole line but then moves the cursor to the first column of the next line, which suggests that’s where the fix needs to go.
One simple way to see this mismatch is to select multiple lines with x and then press A to start inserting at the end of the last selected line. In Helix, if you select three lines, the cursor ends up at the end of the last selected line; in Zed’s implementation, it lands on the line below 🫠
Let me know if you have any questions or would like more guidance. Happy to help!
|
@dinocosta thanks for the detailed feedback! Your suggestions make sense, and I've fixed them. I tested alongside helix and select and insert works the same as helix.
Note: I had to add a separate handler for shift a for helix, not sure if that's the right approach. I also added a couple of tests to verify if cursor behavior is as expected! |
* The changes in `vim::helix::Vim::helix_select_lines` didn't actually affect the behavior of `x`, so I'm reverthing those, as they were a bit confusing whether they were related with the changes at all. * Add documentation to `vim::helix::Vim::helix_insert_end_of_line`, calling out why that is different from the default vim's implementation. * Refactor bits from the `test_helix_select_lines_does_not_over_advance_when_next_line_is_empty` test to `test_helix_select_lines`. The final assertions regarding selections on an empty last line are not actually useful, as the issue with last line selection is concerned with the selection layout when the selection's head is the max point, so not with the calculation logic, meaning it doesn't catch the bug, so much so that I was still able to see it while testing these changes. * Rename `test_helix_shift_a_after_x_stays_on_selected_line` to `test_helix_insert_end_of_line`.
dinocosta
left a comment
There was a problem hiding this comment.
Hey @SkandaBhat 👋
I've updated your branch to be up to date with main, fixing an existing conflict, and ended up cleaning it up a bit, as it was now concerned with shift-a behavior after using x to select a whole line, then the issue it was associated with.
As such, I'm going to update the description so it doesn't actually mention that it closes #35616 , as that seems to be a little bit more tricky that what initially thought .
Thanks! 🙂
…ustries#50709) In Helix, selecting a line with `x` creates a selection from column 0 of the current row to column 0 of the next row. The default `InsertEndOfLine` uses the selection head (which is on the next row) to find the line end, placing the cursor on the wrong line. This commit introduces a new `HelixInsertEndOfLine`, mapped by default to `shift-a` when Helix mode is enabled, that moves left from the head first to land on the correct line. Release Notes: - Fixed `shift-a` in Helix select mode placing the cursor on the wrong line after selecting with `x` --------- Co-authored-by: SkandaBhat <9384046+SkandaBhat@users.noreply.github.com> Co-authored-by: dino <dinojoaocosta@gmail.com>
There's an edge case while entering Visual mode where movement::right jumps to the next line on the second last line, and when the last line is a trailing newline.
before fix:
after fix:
Before you mark this PR as ready for review, make sure that you have:
Release Notes:
shift-ain Helix select mode placing the cursor on the wrong line after selecting withx