helix: Fix insert line above/below with selection#46492
helix: Fix insert line above/below with selection#46492dinocosta merged 7 commits intozed-industries:mainfrom
Conversation
f28fe7e to
53c7a94
Compare
|
Had a thought on this. Maybe Still mulling it over, but I might attempt one of these approaches and see where it gets me. |
|
Ok, got most of the way through refactoring the cursor offset boolean to live on Selection before deciding that it was way more trouble than it was worth, and didn't even solve the real problem with InsertLineAbove, which was that it was considering the selection head for the new cursor position rather than the tail as it should. So I got to rip out the plumbing and duplicate logic that was bugging me, and the fix is now much simpler. |
5c0367f to
7a35f31
Compare
|
Since this no longer needs the cursor offset boolean, I've rebased it to not depend on the PR that fixes the visible cursor offset. |
|
Ugh, still a bug with cursor placement post-insert-after. I think I need to write some actual tests for this. |
73c935a to
137cb0f
Compare
aa86d99 to
fadf351
Compare
fadf351 to
f67bafe
Compare
f67bafe to
337f733
Compare
337f733 to
6cdf5ca
Compare
6cdf5ca to
1ad1804
Compare
1ad1804 to
aed38e1
Compare
|
@kubkon Can I get a review on this? |
dinocosta
left a comment
There was a problem hiding this comment.
Hey @jrobsonchase 👋
Thank you for picking this one up and trying to improve it!
I've tested your changes locally and it fixes when x is used to select lines, but it doesn't seem to tackle the scenario where HelixSelect mode is used, as in, making a selection using v. I believe that's already not working today, so it's not like it breaks anything, just though I should point that out as it's mentioned in the original issue 🙂
|
Oh, huh, good catch! Seems like insert line above/below simply does nothing when in helix select mode currently, both in this branch and on stable. I'll see if I can get that fixed! |
|
Alright, added a test and fixed it, but not convinced that I haven't broken something else in the process that just isn't tested. Going to run it myself for a day or two to see if I notice anything amiss. |
|
Been running this for a few days and haven't noticed anything amiss! Should be good to go. |
Setting the selection with `set_state` instead of using
`simulate_keystrokes` appears to not have been correctly setting the
selection in a state to trigger the path where the previous row had to
be selected (`!selection.is_empty() && selection.end.column == 0`), so
this commit refactors the test to ensure that, if we change the code in
`vim::normal::Vim::insert_line_below` from:
```
.map(|selection| {
if !selection.is_empty() && selection.end.column == 0 {
selection.end.row.saturating_sub(1)
} else {
selection.end.row
}
})
```
back to:
```
.map(|selection| selection.end.row)
```
The test actually fails.
bd68978 to
78d2cea
Compare
dinocosta
left a comment
There was a problem hiding this comment.
Thank you for tackling the provided feedback @jrobsonchase ! 🙌
I'm going to merge this as it fixes the reported issue but, if you wish, you can also open a different Pull Request tackling the scenario where there's multiple cursors in the same line, where Helix actually inserts the same number of lines as cursors.
For example, given the following state:
ˇline ˇone
Pressing o would insert two lines below the one where the cursors are:
line one
ˇ
ˇ
Which Zed doesn't yet support 🙂
In addition to the visual offset of the cursor as addressed in the #46311, this also fixes its behavior. Even though the cursor displays at the end of the line per #46311, it's treated as at the beginning of the next line since selections are end-exclusive. This caused the
vim: insert line above/belowcommands to behave strangely, either leaving the cursor at the end of the first line of the selection after inserting a line above, or inserting a line below the next line.The fix for each case is slightly different. The "above" case switches from
move_cursors_withtomove_withfor tighter control over which part of the selection the cursor gets collapsed to. Inmove_cursors_with, it's always the head/end, but when inserting a line above the selection, we actually want to use the tail/start.The "below" case checks for a non-empty selection ending at column zero, which is semantically a "to the end of the row" selection. In this situation, the "previous row" is actually the row before the selected row, not the row of the end of the selection.
Closes #43210
Release Notes: