git_ui: Fix diff clipboard against selection#51457
git_ui: Fix diff clipboard against selection#51457OmChillure wants to merge 2 commits intozed-industries:mainfrom
Conversation
There was a problem hiding this comment.
Hey @OmChillure 👋
Thank you for picking this one up! Can you confirm whether these changes are ready for review? Testing it out locally seems to work fine but the description states Draft adding tests !! even though tests have also been added.
Also, are you able to add a release note for this? Seems like the template's content was completely removed from the pull request's description 🤔
P.S.: I actually even wonder if it's worth adding this, as it seems that editor: diff clipboard with selection is a bit wonky? If you have, say, 10 lines of text and copy that, then remove 4 or 5 words from the text, editor: diff clipboard with selection doesn't really result in a pleasant way to view that diff...
Hey @dinocosta sorry for this, actually I pushed this changes in two parts, first the main issue fix and then tests , due to which I initially marked it as draft , but when I pushed the tests I marked it "Ready for Review", but forgot to update the description and stuff. I have added it now. thanks |
thanks for bringing this up. This PR mainly focuses on fixing the incorrect behavior when diffing the clipboard against a selection so that the comparison itself works as expected. I agree that the current diff view can look a bit messy for partial edits (like removing a few words across multiple lines). Improving how that diff is presented would probably be better handled separately from this fix. Happy to adjust or drop this change if you feel the command itself isn’t worth supporting. |
|
Hi, thanks for the PR 😁 It looks like there are two separate things this PR is fixing:
The code changes for the side-by-side diff view look broadly good, but I think there are some misunderstanding w.r.t. how multibuffer co-ordinates work that are complicating this PR. Could you split this out into two PRs please? I think we should be good to merge the side-by-side change without too many changes, which will already be a nice improvement😁 Some context on the multibuffer coordinate issue: You have a comment: Ideally, we'd have one code path that handles both singleton and non-singleton multibuffers. |
Thanks for the detailed feedback @cameron1024! I've split this into two PRs as suggested: PR 1 here : Side-by-side diff view support via SplittableEditor - no multibuffer coordinate changes. PR 2 here : Multibuffer coordinate fix with a unified code path for both singleton and non-singleton multibuffers, accounting for expanded deleted diff hunks as you pointed out. @cameron1024 both pr are seperate wthe the required fix |
## Context Fixes the visual selection update in `TextDiffView::open` to properly convert between buffer-local and multibuffer coordinates using `buffer_point_to_anchor`. Previously, raw multibuffer Points were used directly for line expansion, which produced incorrect regions when expanded deleted diff hunks shifted multibuffer row numbers. This provides a single unified code path for both singleton and non-singleton multibuffers, as suggested in [#51457 review feedback](#51457 (comment)). Follow-up to #51457. ## How to Review Small PR - all changes are in `crates/git_ui/src/text_diff_view.rs`. Focus on: - `open()`: The visual selection update block now uses `buffer_point_to_anchor` + `to_point` instead of raw multibuffer coordinate math - No more assumption that multibuffer Points == buffer-local Points - Existing tests validate both singleton and non-singleton multibuffer paths ## Self-Review Checklist - [x] I've reviewed my own diff for quality, security, and reliability - [x] The content is consistent with the UI/UX checklist - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - Fixed incorrect diff region when using "Diff Clipboard with Selection" with expanded diff hunks in the editor.
…industries#51985) ## Context Fixes the visual selection update in `TextDiffView::open` to properly convert between buffer-local and multibuffer coordinates using `buffer_point_to_anchor`. Previously, raw multibuffer Points were used directly for line expansion, which produced incorrect regions when expanded deleted diff hunks shifted multibuffer row numbers. This provides a single unified code path for both singleton and non-singleton multibuffers, as suggested in [zed-industries#51457 review feedback](zed-industries#51457 (comment)). Follow-up to zed-industries#51457. ## How to Review Small PR - all changes are in `crates/git_ui/src/text_diff_view.rs`. Focus on: - `open()`: The visual selection update block now uses `buffer_point_to_anchor` + `to_point` instead of raw multibuffer coordinate math - No more assumption that multibuffer Points == buffer-local Points - Existing tests validate both singleton and non-singleton multibuffer paths ## Self-Review Checklist - [x] I've reviewed my own diff for quality, security, and reliability - [x] The content is consistent with the UI/UX checklist - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - Fixed incorrect diff region when using "Diff Clipboard with Selection" with expanded diff hunks in the editor.
…industries#51985) ## Context Fixes the visual selection update in `TextDiffView::open` to properly convert between buffer-local and multibuffer coordinates using `buffer_point_to_anchor`. Previously, raw multibuffer Points were used directly for line expansion, which produced incorrect regions when expanded deleted diff hunks shifted multibuffer row numbers. This provides a single unified code path for both singleton and non-singleton multibuffers, as suggested in [zed-industries#51457 review feedback](zed-industries#51457 (comment)). Follow-up to zed-industries#51457. ## How to Review Small PR - all changes are in `crates/git_ui/src/text_diff_view.rs`. Focus on: - `open()`: The visual selection update block now uses `buffer_point_to_anchor` + `to_point` instead of raw multibuffer coordinate math - No more assumption that multibuffer Points == buffer-local Points - Existing tests validate both singleton and non-singleton multibuffer paths ## Self-Review Checklist - [x] I've reviewed my own diff for quality, security, and reliability - [x] The content is consistent with the UI/UX checklist - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - Fixed incorrect diff region when using "Diff Clipboard with Selection" with expanded diff hunks in the editor.
…51966) #### Context Switches `TextDiffView` from using `Editor` directly to `SplittableEditor`, enabling side-by-side diff view support for "Diff Clipboard with Selection". The diff view now respects the user's `diff_view_style` setting. Split out from #51457. This PR contains only the `SplittableEditor` wiring. The multibuffer coordinate fix for non-singleton editors will follow in a separate PR. Closes #50912 (partially) #### How to Review Small PR — all changes are in `crates/git_ui/src/text_diff_view.rs`. Focus on: - `new()`: `SplittableEditor::new` replaces `Editor::for_multibuffer`, editor-specific setup goes through `rhs_editor()` - Item trait delegation: `act_as_type`, `for_each_project_item`, `set_nav_history` updated for `SplittableEditor` - Tests: pinned `DiffViewStyle::Unified` and assertions go through `rhs_editor()` #### Self-Review Checklist - [x] I've reviewed my own diff for quality, security, and reliability - [ ] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable #### Video : [Screencast from 2026-03-19 23-11-36.webm](https://github.com/user-attachments/assets/c5a2381d-238d-43ef-ac6f-9994996c0c69) #### Release Notes: - Improved "Diff Clipboard with Selection" to support side-by-side diff view style.
…industries#51985) ## Context Fixes the visual selection update in `TextDiffView::open` to properly convert between buffer-local and multibuffer coordinates using `buffer_point_to_anchor`. Previously, raw multibuffer Points were used directly for line expansion, which produced incorrect regions when expanded deleted diff hunks shifted multibuffer row numbers. This provides a single unified code path for both singleton and non-singleton multibuffers, as suggested in [zed-industries#51457 review feedback](zed-industries#51457 (comment)). Follow-up to zed-industries#51457. ## How to Review Small PR - all changes are in `crates/git_ui/src/text_diff_view.rs`. Focus on: - `open()`: The visual selection update block now uses `buffer_point_to_anchor` + `to_point` instead of raw multibuffer coordinate math - No more assumption that multibuffer Points == buffer-local Points - Existing tests validate both singleton and non-singleton multibuffer paths ## Self-Review Checklist - [x] I've reviewed my own diff for quality, security, and reliability - [x] The content is consistent with the UI/UX checklist - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - Fixed incorrect diff region when using "Diff Clipboard with Selection" with expanded diff hunks in the editor.
…ed-industries#51966) #### Context Switches `TextDiffView` from using `Editor` directly to `SplittableEditor`, enabling side-by-side diff view support for "Diff Clipboard with Selection". The diff view now respects the user's `diff_view_style` setting. Split out from zed-industries#51457. This PR contains only the `SplittableEditor` wiring. The multibuffer coordinate fix for non-singleton editors will follow in a separate PR. Closes zed-industries#50912 (partially) #### How to Review Small PR — all changes are in `crates/git_ui/src/text_diff_view.rs`. Focus on: - `new()`: `SplittableEditor::new` replaces `Editor::for_multibuffer`, editor-specific setup goes through `rhs_editor()` - Item trait delegation: `act_as_type`, `for_each_project_item`, `set_nav_history` updated for `SplittableEditor` - Tests: pinned `DiffViewStyle::Unified` and assertions go through `rhs_editor()` #### Self-Review Checklist - [x] I've reviewed my own diff for quality, security, and reliability - [ ] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable #### Video : [Screencast from 2026-03-19 23-11-36.webm](https://github.com/user-attachments/assets/c5a2381d-238d-43ef-ac6f-9994996c0c69) #### Release Notes: - Improved "Diff Clipboard with Selection" to support side-by-side diff view style.
|
Thanks for the great work @OmChillure Closing this PR. |
fixes #50912
What changed
Testing
Result
Release Notes