Enable syntax coloring for footer minings in unified diff - #2949
tobiasmelcher wants to merge 1 commit into
Conversation
|
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; |
|
Please squash your commits into one. |
|
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 |
da60a32 to
7429bc8
Compare
|
Thanks a lot for the careful review, @vogella — much appreciated! I've amended the review-feedback commit with the following changes:
The compare bundle builds cleanly and all tests pass locally. Please let me know if anything else should be adjusted. |
|
Yes, see #2949 (comment) |
Line-header minings already applied syntax coloring via StyleRange computation. Footer minings (used when the diff sits at the end of a document without a trailing newline) were missing this: they fell back to a plain super.draw() call for the text. They now use the same computeStyleRanges path, transformFontStyleToFont for bold/italic, and the same rendering loop as the header. A shared MouseClickConsumer (via the new IUnifiedDiffCodeMining interface) also gives footer minings the same double-click overlay behaviour the header already had.
7429bc8 to
d2569a9
Compare
|
Thanks again, @vogella — you're absolutely right on both points. On the The footer test previously relied on that unguarded NPE path (it used a bare On squashing: done — the branch is now a single commit. All tests pass locally. Thanks for the thorough review! |
|
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. |
| static final class ForegroundInfo { | ||
|
|
||
| final int x; | ||
| final int y; | ||
| final String str; | ||
| final Font font; | ||
| final Color background; | ||
| final Color foreground; | ||
|
|
||
| ForegroundInfo(int x, int y, String str, Font font, Color background, Color foreground) { | ||
| this.x = x; | ||
| this.y = y; | ||
| this.str = str; | ||
| this.font = font; | ||
| this.background = background; | ||
| this.foreground = foreground; | ||
| } | ||
| } |
There was a problem hiding this comment.
Would it make sense to convert this class to a record? Could be more compact.
3 participants
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
DocumentFooterCodeMiningis 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:
computeStyleRangespath.transformFontStyleToFont.MouseClickConsumer(introduced via the newIUnifiedDiffCodeMininginterface) gives footer minings the same double-click overlay behaviour the header already had.Assisted by Claude (Anthropic).