editor: Fix git-hunk toggling for adjacent hunks#43187
editor: Fix git-hunk toggling for adjacent hunks#43187kubkon merged 3 commits intozed-industries:mainfrom
Conversation
Commentary
|
| // why do we need to subtract 1 from the start offset? | ||
| // let start = start.saturating_sub_usize(1); |
There was a problem hiding this comment.
This change is questionable, since it seems to break some existing behaviour when attempting to collapse a diff-hunk when the cursor is a line below the expanded hunk.
Prior to this change, the cursor could be at the beginning of the line below an expanded hunk, and the toggle-selected-hunks command would collapse the hunk above the cursor.
After this change, when the cursor is a line below an expanded hunk, and the cursor is a line above a deletion hunk, the toggle-selected-hunks command will expand the deletion hunk.
Here's a screen recording of the current behaviour with the changes in this PR:zed-pr-toggle-hunk-behaviour-fix-v1.mp4 |
This change adds a failing test to the editor test suite for a scenario where toggling neighboring git-hunks does work as expected because the second hunk is not targeted when the first hunk is already expanded
# 50-character subject line # # 72-character wrapped longer description. # # * Maybe try answering these questions: * # * What did I change? * # * Why did I change it? * # * How did I change it? *
99c3d29 to
0960a0f
Compare
|
Thanks for the PR! I had a closer look and the proposed change does not seem the right solution as it regresses other behaviour plus makes it generally inconsistent. However, I did discuss that with the team and we have an idea how to approach this. With that in mind, I'm gonna go ahead and close the PR and hopefully get a fix up ASAP. |
|
Thanks for taking a look! Glad to hear there was discussion, hope this was helpful for kickstarting the real solution, eager to learn more about it when it’s ready. feel free to tag me in the next pr, happy to follow along and help out if needed 👍 |
|
I've done some more digging and I agree with you that subtracting 1 from multibuffer offset makes little sense and it doesn't seem to regress anything (anything that we have coverage for). It also keeps things consistent, whereas trying to special-case this would inevitably result in inconsistent handling depending if we are dealing with single empty cursor selection or more elaborate selections. I am therefore inclined to merge this PR with some minor cleanups and rebase on main (already done that locally, will push in a minute). If we regress something that we don't have coverage for, we'll revert and add a new test case and whatnot. |
|
awesome! thanks @kubkon 🙌 |
Closes zed-industries#42934 Release Notes: - Fix toggling adjacent git-diff hunks based on the reported behaviour in zed-industries#42934 --------- Co-authored-by: Jakub Konka <kubkon@jakubkonka.com>
Closes zed-industries#42934 Release Notes: - Fix toggling adjacent git-diff hunks based on the reported behaviour in zed-industries#42934 --------- Co-authored-by: Jakub Konka <kubkon@jakubkonka.com>
Closes zed-industries#42934 Release Notes: - Fix toggling adjacent git-diff hunks based on the reported behaviour in zed-industries#42934 --------- Co-authored-by: Jakub Konka <kubkon@jakubkonka.com>
Closes #42934
Release Notes: