Enable syntax coloring for footer minings in unified diff

[tobiasmelcher]

Footer minings are used when a diff sits at the end of a document that has no trailing newline — in that case there is no following line to anchor a line-header mining, so a DocumentFooterCodeMining is created instead.

These footer minings were missing the syntax coloring that line-header minings already provide: they fell back to a plain super.draw() call, rendering all text in the default foreground color without any keyword highlighting or font styling.

This change brings footer minings to parity with line-header minings:

• Syntax coloring is applied via the same computeStyleRanges path.
• Bold/italic font styles are handled via transformFontStyleToFont.
• The rendering loop is the shared drawStyleRanges helper used by the header.
• The word-level detailed-diff highlighting is now painted inline in the footer band — the detailedDiffColor rectangles are filled behind the syntax-colored text (the same "fill background, then draw transparent text" order the header uses), so the darker word-level highlight shows in the footer band itself, not only in the double-click overlay.
• A shared MouseClickConsumer (introduced via the new IUnifiedDiffCodeMining interface) gives footer minings the same double-click overlay behaviour the header already had.

The footer reaches this parity by reusing the existing createDetailedDiffBackgroundRanges helper to locate the word-level ranges rather than duplicating the header's geometric background-painting code. The footer label is now trailing-newline-stripped like the header's, so the overlay no longer needs a compensating stripTrailing(), and the syntax-coloring ranges are cached so repaints skip recomputation.

Assisted by Claude (Anthropic).

[github-actions]

Test Results

    54 files  ±0      54 suites  ±0   57m 25s ⏱️ -22s
 4 836 tests +3   4 814 ✅ +3   22 💤 ±0  0 ❌ ±0 
12 399 runs  +9  12 245 ✅ +8  154 💤 +1  0 ❌ ±0 

Results for commit 88fefb2. ± Comparison against base commit 5df6d33.

♻️ This comment has been updated with latest results.

[vogella]

You have two dead constructor parameters. UnifiedDiffFooterCodeMining takes Color detailedDiffColor and never stores or uses it, and it takes Consumer action which it silently discards in favour of new MouseClickConsumer(viewer).

UnifiedDiffFooterCodeMining.draw() lines 621-692 are a copy of the header loop at 1093-1157. The only difference is that the header also appends ForegroundInfo entries to its foregrounds cache. Use Consumer as parameter.

Also the test testFooterMiningStyleRangesUseTheSameLogicAsTheHeader does not test the change, it only checks for is non-empty;

[vogella]

Please squash your commits into one.

[vogella]

I think you added an unrelated change: catch (BadLocationException | NullPointerException e)

The NPE does not come from this file: the test builds new ProjectionViewer(shell, null, null, false, SWT.V_SCROLL) and never calls configure(...), so fPresentationReconciler is null, and SourceViewer.computeStyleRanges dereferences it unguarded at reconciler.getRepairer(partitioningRegion.getType()).

I think the real defaultg is a missing null guard in SourceViewer.computeStyleRanges in eclipse.platform.ui. Catching bare NPE here also silents NPEs thrown by real presentation repairers.

I fixed it there, please have a look eclipse-platform/eclipse.platform.ui#4394

[tobiasmelcher]

Thanks a lot for the careful review, @vogella — much appreciated!

I've amended the review-feedback commit with the following changes:

• Footer font-handle leak: UnifiedDiffFooterCodeMining now mirrors the header's font-change handling. It tracks the base font and disposes the derived styled fonts when the editor font changes, instead of only at dispose(), so the native font handles are no longer leaked on a font change.
• Narrowed the NullPointerException catch in computeStyleRanges: the null document case is now handled with an explicit guard, and the remaining NPE catch is scoped to just the SourceViewer.computeStyleRanges call (whose presentation reconciler cannot be checked from the outside). Any genuine NPE elsewhere in that method now propagates instead of being silently swallowed.
• Clarified the footer test: renamed and reworded it to describe what it actually verifies — that draw() runs to completion and degrades gracefully when the viewer offers no syntax coloring.

The compare bundle builds cleanly and all tests pass locally. Please let me know if anything else should be adjusted.

[vogella]

Yes, see #2949 (comment)

[tobiasmelcher]

Thanks again, @vogella — you're absolutely right on both points.

On the NullPointerException: I've removed the bare catch (... | NullPointerException). As you diagnosed, the NPE originates in SourceViewer.computeStyleRanges dereferencing a null fPresentationReconciler, not in this file, and catching it here would also have swallowed genuine NPEs from real presentation repairers. computeStyleRanges now catches only BadLocationException again, plus an explicit if (originalDocument == null) return ...; guard for the document-null case. Thank you for fixing the real cause upstream in eclipse-platform/eclipse.platform.ui#4394 — that's clearly the right place for it.

The footer test previously relied on that unguarded NPE path (it used a bare ProjectionViewer with no configure(...)). I've now given the test viewer a minimal presentation reconciler, so it exercises the real syntax-coloring path instead of depending on the missing null guard.

On squashing: done — the branch is now a single commit.

All tests pass locally. Thanks for the thorough review!

[vogella]

Stepping away from my laptop now, please feel free to merge eclipse-platform/eclipse.platform.ui#4394 once it is green. I think this change should wait until eclipse-platform/eclipse.platform.ui#4394 is in master.

[tobiasmelcher]

Do you have further feedback? Or are there any objections? I plan to merge this change tomorrow.

[vogella]

1. The test still doesn't check the coloring.

testFooterMiningStyleRangesUseTheSameLogicAsTheHeader asserts that lastRectangle is set after drawing. But draw() sets lastRectangle (diff line 151) before it decides whether to color. The test would pass even if the plain fallback ran. Also he test reconciler uses new TextAttribute(null), so there is no color at all. A token with a real foreground color, plus a check on the ranges or the drawn pixels, would actually test the change.

2. The footer still doesn't fully match the header. createDetailedDiffBackgroundRanges returns an empty list for the footer. So the footer, and the overlay you get by double-clicking it, never shows the word-level diff highlighting that the header shows. The PR description says "parity", which isn't quite true. It's fine to leave this out of scope, but it's worth one sentence in the description.

[tobiasmelcher]

Thanks again, @vogella — both points are now addressed:

1. Test now checks the coloring. The test viewer uses a presentation reconciler with a real foreground color, and the test asserts the resulting ranges carry a non-null foreground (so the syntax-coloring path ran, not the plain fallback) and that the word-level detailed-diff background range is present.
2. Footer parity is real. createDetailedDiffBackgroundRanges now returns the word-level ranges, and the footer paints them inline behind the syntax-colored text — so the footer band (and its double-click overlay) shows the same darker word-level highlight the header does. Verified manually.

The compare bundle builds cleanly and all tests pass locally. Assisted by Claude.

[vogella]

A few more change request:

1. createDetailedDiffBackgroundRanges now exists twice, once in the footer and once in the header, with identical code. Please move it into one static helper that takes the diff, tabWidth and the color.
2. fillDetailedDiffBackgrounds measures with gc.stringExtent in the base font, but drawStyleRanges draws bold and italic ranges with derived fonts. When a bold keyword comes before the changed word on the same line, the highlight ends up left of the word. The header avoids this because getPositionForOffset(..., ranges, ...) uses the style ranges. Can the footer reuse that code?
3. The test doesn't check what draw() paints. It calls createDetailedDiffBackgroundRanges directly, and anyMatch(r -> r.background != null) can't fail because every returned range has a background. Also, can we avoid the public getStyleRanges() test hook in production code?
4. Minor: the test imports are unsorted. The header overlay used to call stripTrailing() and no longer does, so trailing spaces on the last line now show in the overlay.

[tobiasmelcher]

A few more change request:

1. createDetailedDiffBackgroundRanges now exists twice, once in the footer and once in the header, with identical code. Please move it into one static helper that takes the diff, tabWidth and the color.
2. fillDetailedDiffBackgrounds measures with gc.stringExtent in the base font, but drawStyleRanges draws bold and italic ranges with derived fonts. When a bold keyword comes before the changed word on the same line, the highlight ends up left of the word. The header avoids this because getPositionForOffset(..., ranges, ...) uses the style ranges. Can the footer reuse that code?
3. The test doesn't check what draw() paints. It calls createDetailedDiffBackgroundRanges directly, and anyMatch(r -> r.background != null) can't fail because every returned range has a background. Also, can we avoid the public getStyleRanges() test hook in production code?
4. Minor: the test imports are unsorted. The header overlay used to call stripTrailing() and no longer does, so trailing spaces on the last line now show in the overlay.

1. createDetailedDiffBackgroundRanges existed twice. Done — it is now a single static
helper taking the diff, the tab width and the color. Both minings and the double-click
overlay call it.

2. fillDetailedDiffBackgrounds measured in the base font. Fixed: the footer now measures
with the same fonts drawStyleRanges uses, so a bold keyword before the changed word no
longer pushes the highlight to the left.

I could not reuse getPositionForOffset for this. I tried, and it broke the rendering: the
highlight cascaded diagonally down and to the right. That helper is written for the header's
coordinate space and relies on the header walking its ranges line by line; the footer has
neither, and the differences are not something a parameter can bridge. So the footer got a
small measuring helper of its own instead.

As a result the header is untouched by this change — the earlier attempt to make
getPositionForOffset shared is reverted, which removes about 110 lines of diff.

3. The test did not check what draw() paints. Rewritten. The public getStyleRanges()
test hook is gone. The test now paints into an image and inspects the pixels, and asserts the
highlight starts right of where the base-font measurement would have placed it — so it fails
if the bug comes back. A second test covers the multi-line case.

I do have some doubts whether these pixel-based tests are stable across all platforms. We will
see that in the central test run.

4. Minor. Test imports sorted; the overlay calls stripTrailing() again.

[vogella]

Before:

After:

LGTM

[HannesWell]

@tobiasmelcher I see you pushed a very similar branch to the upstream repo:
https://github.com/eclipse-platform/eclipse.platform/tree/d031119/unified-diff-footer-styling

If it can be deleted, please just do so.

[tobiasmelcher]

@tobiasmelcher I see you pushed a very similar branch to the upstream repo: https://github.com/eclipse-platform/eclipse.platform/tree/d031119/unified-diff-footer-styling

If it can be deleted, please just do so.

@HannesWell sorry, my fault. I just deleted the branch.