Do not switch to the unified diff after the comparison was closed - #2854
Conversation
Test Results 54 files 54 suites 59m 45s ⏱️ Results for commit 379a782. ♻️ This comment has been updated with latest results. |
b1276b1 to
2ed603c
Compare
There was a problem hiding this comment.
Pull request overview
Adds a bidirectional view switch between the classic side-by-side compare editor and the unified diff view, allowing users to move back to unified diff without changing preferences/reopening the editor, while reusing the already-prepared compare input.
Changes:
- Introduces a “Show Unified Diff” toolbar action in
TextMergeViewer(only when the prepared compare result qualifies for unified diff). - Refactors unified-diff preparation to extract a reusable “candidate/source” from an already-prepared
CompareEditorInput, and addsswitchToUnifiedDiff(...)to open unified diff and close the compare editor. - Adds regression tests covering eligibility, editor replacement behavior, and action resource completeness; adds a new toolbar icon.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| team/tests/org.eclipse.compare.tests/src/org/eclipse/compare/tests/UnifiedDiffOpenTest.java | Adds tests for unified-diff eligibility, switching behavior, and resource completeness. |
| team/bundles/org.eclipse.compare/icons/full/elcl16/unifieddiff_co.svg | Adds an icon for the new toolbar action. |
| team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/CompareUIPlugin.java | Refactors unified diff source extraction and adds the switch-to-unified-diff job + editor-closing behavior. |
| team/bundles/org.eclipse.compare/compare/org/eclipse/compare/contentmergeviewer/TextMergeViewerResources.properties | Adds label/tooltip/image keys for the new “Show Unified Diff” action. |
| team/bundles/org.eclipse.compare/compare/org/eclipse/compare/contentmergeviewer/TextMergeViewer.java | Contributes the new toolbar action to switch from side-by-side to unified diff when applicable. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
I tested the change on my machine and the button in the two way compare works as described. Great feature, thanks a lot Lars for all your work. +1 |
|
Thanks @tobiasmelcher for the test and feedback. I was planning to hold it off for the next release as this makes the unified diff visible to users. I assume without this, almost nobody will discover it, we have too many settings in Eclipse. EDIT: The question changes, let me know if I should remove the button again, it is already in with #2874. |
|
I just noted that it accidently slip in with #2874, sorry for that. I will use this one to address the valid feedback from Copilot and if someone wants the button for switching between the old compare to the unified compare gone, I will remove it with a new PR. |
2ed603c to
ed681ff
Compare
vogella
changed the title
The switch to the unified diff reads its source in a job and opens the editor afterwards. It looked up the compare editor only to decide whether to close it, so a comparison the user closed in the meantime still brought up a unified diff for an editor that was already gone. The switch now stops when the compare editor can no longer be found, which also covers a page that is closing. The toolbar action test compared the label against the English text. A missing resource key makes Utilities.initAction fall back to the key name, so the assertion is against that instead, which keeps the missing key detected without breaking under a translated label.
ed681ff to
379a782
Compare
|
Can we gate the new button on the EXPERIMENTAL preference? It should only be visible when the experimental checkbox is enabled in the preferences. 
|
Good idea. Yes I can do that. |
|
The switch enablement based on the experimental flag can be found here: #2877 |
The original change of this PR is already on master (7e2e72e, merged along with #2874), so what is left here are the two review findings on it.
The switch to the unified diff reads its source in a job and opens the editor afterwards. It looked up the compare editor only to decide whether to close it, so a comparison the user closed in the meantime still brought up a unified diff for an editor that was already gone. The switch now stops when the compare editor can no longer be found, which also covers a page that is closing, where the old code ran into a logged PartInitException. A regression test covers it; without the fix it fails with an editor appearing after the close.
The toolbar action test compared the label against the English text, which a translation would break. A missing resource key makes Utilities.initAction fall back to the key name rather than to a blank label, so the assertion is against the key name plus a blank check. Removing the key from the properties file still fails the test.
Contributes to #2795