Unified Diff display — review findings

This issue collects the findings of a code review of the Unified Diff feature introduced in 556f657 ("Add Unified Diff Display in Text Editor as Alternative to Classic 2-Way Compare") and its follow-ups (c45a7a7, cb939ba). The feature lives in team/bundles/org.eclipse.compare/compare/org/eclipse/compare/unifieddiff/ plus the integration in CompareUIPlugin.

The issue is split into two parts:

• Part A — self-contained fixes. Each item includes instructions written for an AI agent (e.g. a Claude Opus instance) so it can be handed off directly.
• Part B — design / architecture items. These need discussion or larger refactoring and are only described.

Part A — Self-contained fixes (with AI instructions)

A2. Hardcoded UTF-8 when reading compare element contents

CompareUIPlugin.java (~line 704): toString(InputStream) reads with StandardCharsets.UTF_8, ignoring IEncodedStreamContentAccessor.getCharset(). Non-UTF-8 files (e.g. Cp1252/Latin-1) produce garbled diff text. The compare framework already has Utilities.readString(...) which honors the element's encoding.

AI instructions:
In CompareUIPlugin.java, replace the private toString(InputStream)/getSourceOf(IStreamContentAccessor) reading logic with a call to the existing org.eclipse.compare.internal.Utilities.readString(...) overload that accepts the IStreamContentAccessor/ITypedElement and resolves the charset via IEncodedStreamContentAccessor. Preserve the current null-handling contract: getSourceOf must still return "" (never null) when the accessor is null or reading fails, and must log via CompareUIPlugin.log(e). Remove the now-unused toString(InputStream) helper and the StandardCharsets import if unused. Compile-check with cd team/bundles/org.eclipse.compare && mvn clean verify -Pbuild-individual-bundles -DskipTests (allow 300s timeout).

A4. Null-safety in UI event handlers

Several handlers assume state that may be gone by the time they run:

• UnifiedDiffManager.dismissCurrentDiff (~line 666): getUnifiedDiffForAnno(anno) can return null → NPE on diff.container. Also throws raw IllegalStateException from a toolbar button handler.
• UnifiedDiffManager.UnifiedDiffMouseMoveListener.mouseMove (~line 1046) and UnifiedDiffPaintListener.paintControl (~line 1249): model.getPosition(anno) returns null once the annotation has been removed → NPE on pos.offset.
• UnifiedDiffCodeMiningProvider setTextEditorActionsActivated (~line 581): PlatformUI.getWorkbench().getActiveWorkbenchWindow() can be null (shutdown, non-UI context) → NPE.

AI instructions:
In UnifiedDiffManager.java and UnifiedDiffCodeMiningProvider.java, add null guards at the four locations above: return early from dismissCurrentDiff when the resolved UnifiedDiff is null; skip the annotation (continue) in mouseMove and paintControl when model.getPosition(anno) returns null; return early from setTextEditorActionsActivated when the active workbench window or active page is null. In dismissCurrentDiff, replace the throw new IllegalStateException("UnifiedDiff not found in container") with a logged error (UnifiedDiffManager.error(...)) and an early return, so a stale toolbar click cannot take down the event loop. Keep the changes minimal — guards only, no behavioral redesign. Compile-check with cd team/bundles/org.eclipse.compare && mvn clean verify -Pbuild-individual-bundles -DskipTests (allow 300s timeout).

A5. Dead code and small cleanups

• NextRunnable.run: the if (nextDiff != null) break; after the preceding break is unreachable/redundant.
• setChecked(false) is called on SWT.PUSH-style actions in the toolbar helpers (addToolbarAction in UnifiedDiffManager) — meaningless for push buttons.
• Misspelled locals in UnifiedDiffManager.open: lDocIgnonerWhitespaceContributor, rDocIgnonreWhitespaceContributor.

AI instructions:
Remove the unreachable if (nextDiff != null) break; in NextRunnable.run. Remove the setChecked(false) calls from push-button actions in both addToolbarAction variants in UnifiedDiffManager (verify no action there is ever created as CHECK style first). Rename the two misspelled locals to leftIgnoreWhitespaceContributor / rightIgnoreWhitespaceContributor. No functional changes. Compile-check with cd team/bundles/org.eclipse.compare && mvn clean verify -Pbuild-individual-bundles -DskipTests (allow 300s timeout).

Part B — Design / architecture items (description only)

B1. Compare computation runs synchronously on the UI thread

CompareUIPlugin.canShowInUnifiedDiff calls input.run(new NullProgressMonitor()) directly on the UI thread. That is the potentially long-running diff preparation the classic path deliberately runs as a background job (openEditorInBackground / canRunAsJob). Large inputs freeze the UI with no progress or cancel. Additionally, when the method returns null and the code falls back to the classic compare editor, the input is computed a second time. The unified-diff eligibility check and content preparation should be integrated into the existing job-based flow.

B2. Deferred document edits race with the user (runAfterRepaintFinished)

UnifiedDiffManager.runAfterRepaintFinished is a paint-listener + Display.timerExec(100) construction used by the Accept/Undo paths. Document Positions are captured, annotations removed, and the actual document.replace(...) happens ≥100 ms later. If the user types in that window the captured offsets are stale and the replace corrupts the document; if the widget never repaints (obscured/minimized) the action silently never applies. This needs a deterministic mechanism (e.g. removing the line-header minings via the code-mining API and applying edits synchronously) instead of a timing heuristic.

B3. REPLACE_MODE applies each diff as an individual document.replace

No DocumentRewriteSession and no compound undo: N diffs produce N undo steps and N document/reconcile events, and a BadLocationException mid-loop leaves the document half-transformed (the catch logs and continues while subsequent deltas are then wrong). Wrap the batch in a rewrite session and a single undoable operation, and abort cleanly on failure.

B4. Fallback path can leave a stray editor open / swallows "no differences"

openUnifiedDiffInEditor opens the real file editor first and only then attempts UnifiedDiff...open(). If that fails or the part is not an ITextEditor, the method returns false and the classic compare editor opens as well — the just-opened text editor stays behind. Related: when both sides are identical, open() still installs toolbar and listeners and returns OK, so the user sees an editor with a floating toolbar and no "no differences" feedback (the classic path shows a dialog).

B5. Thread-safety of the code mining provider

Colors in UnifiedDiffCodeMiningProvider.provideCodeMinings are only (re)created when Display.getCurrent() != null; a first invocation from a background thread leaves them null and minings later fail in draw (gc.setBackground(null) throws). The static UnifiedDiffManager.diffsByViewer HashMap is also read from CompletableFuture.supplyAsync (common pool) without synchronization.

B6. Reflection into AbstractTextEditor.setActionActivation

The overlay editor activates/deactivates editor actions via setAccessible(true) reflection on a private platform method. Fragile against platform evolution and JPMS tightening; since this code is being contributed to the platform, a supported hook should be introduced instead.

B7. getEditorId has a persistent side effect

It calls IDE.overrideDefaultEditorAssociation(...) while merely computing an editor id, mutating the input's editor association as a by-product. The override should either be intentional and documented, or removed.

B8. Performance

• getAllAnnotationsForUnifiedDiff walks the entire annotation model; clearAll, AcceptAllRunnable, HideAllDiffsRunnable call it once per diff → O(diffs × annotations). The mouse-move listener also walks every annotation (with getTextBounds calls) on every mouse move. Use IAnnotationModelExtension2.getAnnotationIterator(offset, length, ...) or maintain a diff→annotations map.
• computeStyleRanges copies the entire document prefix into a fresh Document per mining computation — O(document size) per diff, re-triggered on cache invalidation (font change, disposal).
• canShowInUnifiedDiff creates a Shell and a full content merge viewer just to obtain one IDocumentMergerInput adapter; the viewer's listeners on the input are never explicitly disposed.

B9. UnifiedDiffManager is a static god class

All state lives in a static Map<ITextViewer, ...> plus half a dozen StyledText.setData keys, with cleanup depending on a web of self-deregistering listeners. A per-viewer instance object (created in open(), disposed with the widget) would eliminate the static map, the setData keys, and most of the "is this listener already registered?" checks, and would make the lifecycle auditable and testable.

B10. Duplication

The left/right branches of openUnifiedDiffInEditor are ~40 nearly identical lines each; createLineHeaderCodeMinings has three near-identical mode branches; there are two copies of addToolbarAction; drawToolbarForOneDiff/drawToolBarForAllDiffs share most of their body; the inline "Undo" action in drawToolbarForOneDiff duplicates dismissCurrentDiff logic.

B11. UnifiedDiffMode should be an enum; UnifiedDiff data class hygiene

UnifiedDiffMode is a hand-rolled type-safe enum, which forces long if/else .equals(...) chains; a real enum enables switch and exhaustiveness checking. UnifiedDiff exposes public mutable fields that are mutated in place (leftStart += delta in REPLACE mode), making the objects' meaning state-dependent.

B12. Test coverage and API readiness

~190 test lines cover ~3,000 lines of feature code, and the hairiest logic (line/offset geometry in draw, mergeStyleRanges, mapOffsetToTabExpanded, tab expansion) is locked inside UI classes — extracting these pure functions would make defects like the split("\n") line-count off-by-one unit-testable. The package is exported x-internal:=true (good); before graduating UnifiedDiff/UnifiedDiffMode to real API they need javadoc, @since tags, and the B11 enum decision. Also note: a comment in draw() states the rendering depends on eclipse.platform.ui PR #3651 — a cross-repo version dependency not expressed in the bundle manifest.