Skip to content

Commit 7429bc8

Browse files
tobiasmelchertobiasmelcher
committed
Address PR review feedback
- Remove two dead constructor parameters from UnifiedDiffFooterCodeMining (Consumer<MouseEvent> action and Color detailedDiffColor) - Extract the shared render loop into a static drawStyleRanges() method used by both header and footer, with a Consumer<ForegroundInfo> parameter that is null for the footer and foregrounds::add for the header - Fix testFooterMiningStyleRangesUseTheSameLogicAsTheHeader to call draw() with a real GC and assert lastRectangle is set, proving the render path ran
1 parent 99b598e commit 7429bc8

2 files changed

Lines changed: 136 additions & 153 deletions

File tree

‎team/bundles/org.eclipse.compare/compare/org/eclipse/compare/unifieddiff/internal/UnifiedDiffCodeMiningProvider.java‎

Lines changed: 117 additions & 143 deletions
Original file line numberDiff line numberDiff line change
@@ -316,8 +316,7 @@ private ICodeMining createMining(IDocument doc, UnifiedDiff diff, int offset, in
316316
throws BadLocationException {
317317
int end = doc.getLength();
318318
if (offset >= end && !startsLine(doc, end)) {
319-
return new UnifiedDiffFooterCodeMining(doc, this, null, diff, tabWidth, this.deletionBackgroundColor,
320-
this.detailedDiffColor, tv);
319+
return new UnifiedDiffFooterCodeMining(doc, this, diff, tabWidth, this.deletionBackgroundColor, tv);
321320
}
322321
// a position must not reach beyond the document, otherwise the annotation model
323322
// silently drops it
@@ -540,10 +539,10 @@ public static class UnifiedDiffFooterCodeMining extends DocumentFooterCodeMining
540539
private List<StyleRange> styleRanges;
541540
private final HashMap<Font, Map<Integer, Font>> styledFonts = new HashMap<>();
542541
private Rectangle lastRectangle;
542+
private Font cachedFont;
543543

544544
public UnifiedDiffFooterCodeMining(IDocument document, ICodeMiningProvider provider,
545-
Consumer<MouseEvent> action, UnifiedDiff diff, int tabWidth, Color deletionBackgroundColor,
546-
Color detailedDiffColor, ITextViewer viewer) {
545+
UnifiedDiff diff, int tabWidth, Color deletionBackgroundColor, ITextViewer viewer) {
547546
super(document, provider, new MouseClickConsumer(viewer));
548547
this.deletionBackgroundColor = deletionBackgroundColor;
549548
this.viewer = viewer;
@@ -585,9 +584,14 @@ public String getLabel() {
585584
public void dispose() {
586585
styleRanges = null;
587586
lastRectangle = null;
587+
cachedFont = null;
588+
clearStyledFonts();
589+
super.dispose();
590+
}
591+
592+
private void clearStyledFonts() {
588593
styledFonts.forEach((font, map) -> map.forEach((style, f) -> f.dispose()));
589594
styledFonts.clear();
590-
super.dispose();
591595
}
592596

593597
private List<StyleRange> styleRanges(String label) {
@@ -597,17 +601,19 @@ private List<StyleRange> styleRanges(String label) {
597601
return styleRanges;
598602
}
599603

600-
private StyleRange transformFontStyleToFont(Font baseFont, StyleRange styleRange) {
601-
return UnifiedDiffCodeMiningProvider.transformFontStyleToFont(styledFonts, baseFont, styleRange);
602-
}
603-
604604
@Override
605605
public Point draw(GC gc, StyledText textWidget, Color color, int x, int y) {
606606
gc.setBackground(this.deletionBackgroundColor);
607607
Color c = textWidget.getForeground();
608608
gc.setForeground(c);
609609
Font font = textWidget.getFont();
610610
gc.setFont(font);
611+
if (cachedFont != null && (cachedFont.isDisposed() || !cachedFont.equals(font))) {
612+
// font might have been changed in the meantime - drop the derived fonts
613+
// keyed on the old base font so their native handles are not leaked
614+
clearStyledFonts();
615+
}
616+
cachedFont = font;
611617
// first run to get width and height for label
612618
// change from https://github.com/eclipse-platform/eclipse.platform.ui/pull/3651
613619
// is required so that background correctly drawn with line spacing > 0
@@ -627,81 +633,132 @@ public Point draw(GC gc, StyledText textWidget, Color color, int x, int y) {
627633
}
628634

629635
gc.setFont(font);
630-
int textWidgetLineHeight = textWidget.getLineHeight();
631-
int cx = x;
632-
int cy = y;
633-
for (StyleRange range : ranges) {
634-
String sub = label.substring(range.start, range.start + range.length);
635-
if (sub.trim().length() > 0) {
636-
if (range.background != null) {
637-
gc.setBackground(range.background);
638-
}
639-
if (range.foreground != null) {
640-
gc.setForeground(range.foreground);
641-
}
642-
Font currentFont = gc.getFont();
643-
var rangeWithFont = transformFontStyleToFont(currentFont, range);
644-
if (rangeWithFont.font != null) {
645-
gc.setFont(rangeWithFont.font);
646-
}
647-
String[] lines = sub.split("\n"); //$NON-NLS-1$
648-
if (lines.length > 1) {
649-
for (int i = 0; i < lines.length; i++) {
650-
String line = lines[i].replace("\r", ""); //$NON-NLS-1$ //$NON-NLS-2$
651-
gc.drawString(line, cx, cy, true);
652-
Point p = gc.stringExtent(line);
653-
if (i < lines.length - 1) {
654-
cy += textWidgetLineHeight + textWidget.getLineSpacing();
655-
cx = x;
656-
} else {
657-
if (sub.endsWith("\n")) { //$NON-NLS-1$
658-
cy += textWidgetLineHeight + textWidget.getLineSpacing();
659-
cx = x;
660-
} else {
661-
cx += p.x;
662-
}
663-
}
636+
drawStyleRanges(gc, textWidget, ranges, label, styledFonts, x, y, null);
637+
return result;
638+
}
639+
}
640+
641+
static final class ForegroundInfo {
642+
643+
final int x;
644+
final int y;
645+
final String str;
646+
final Font font;
647+
final Color background;
648+
final Color foreground;
649+
650+
ForegroundInfo(int x, int y, String str, Font font, Color background, Color foreground) {
651+
this.x = x;
652+
this.y = y;
653+
this.str = str;
654+
this.font = font;
655+
this.background = background;
656+
this.foreground = foreground;
657+
}
658+
}
659+
660+
/**
661+
* Draws the syntax-colored label using the given style ranges, advancing the
662+
* cursor position range by range. The {@code onForeground} consumer is called
663+
* for each drawn segment and may be {@code null}; the header mining uses it to
664+
* populate its foreground cache so subsequent repaints skip this path.
665+
*/
666+
static void drawStyleRanges(GC gc, StyledText textWidget, List<StyleRange> ranges, String label,
667+
HashMap<Font, Map<Integer, Font>> styledFonts, int x, int y,
668+
Consumer<ForegroundInfo> onForeground) {
669+
Font font = gc.getFont();
670+
int textWidgetLineHeight = textWidget.getLineHeight();
671+
int cx = x;
672+
int cy = y;
673+
for (StyleRange range : ranges) {
674+
String sub = label.substring(range.start, range.start + range.length);
675+
if (sub.trim().length() > 0) {
676+
if (range.background != null) {
677+
gc.setBackground(range.background);
678+
}
679+
if (range.foreground != null) {
680+
gc.setForeground(range.foreground);
681+
}
682+
Font currentFont = gc.getFont();
683+
var rangeWithFont = transformFontStyleToFont(styledFonts, currentFont, range);
684+
if (rangeWithFont.font != null) {
685+
gc.setFont(rangeWithFont.font);
686+
}
687+
String[] lines = sub.split("\n"); //$NON-NLS-1$
688+
if (lines.length > 1) {
689+
for (int i = 0; i < lines.length; i++) {
690+
String line = lines[i].replace("\r", ""); //$NON-NLS-1$ //$NON-NLS-2$
691+
gc.drawString(line, cx, cy, true);
692+
if (onForeground != null) {
693+
onForeground.accept(new ForegroundInfo(cx - x, cy - y, line, gc.getFont(),
694+
gc.getBackground(), gc.getForeground()));
664695
}
665-
} else {
666-
gc.drawString(sub, cx, cy, true);
667-
Point p = gc.stringExtent(sub);
668-
if (sub.endsWith("\n")) { //$NON-NLS-1$
696+
Point p = gc.stringExtent(line);
697+
if (i < lines.length - 1) {
669698
cy += textWidgetLineHeight + textWidget.getLineSpacing();
670699
cx = x;
671700
} else {
672-
cx += p.x;
701+
if (sub.endsWith("\n")) { //$NON-NLS-1$
702+
cy += textWidgetLineHeight + textWidget.getLineSpacing();
703+
cx = x;
704+
} else {
705+
cx += p.x;
706+
}
673707
}
674708
}
675-
gc.setFont(currentFont);
676709
} else {
677-
int lfCount = 0;
678-
if (sub.contains("\n")) { //$NON-NLS-1$
679-
lfCount = sub.split("\n", -1).length - 1; //$NON-NLS-1$
680-
sub = sub.substring(sub.lastIndexOf("\n") + 1); //$NON-NLS-1$
710+
gc.drawString(sub, cx, cy, true);
711+
if (onForeground != null) {
712+
onForeground.accept(new ForegroundInfo(cx - x, cy - y, sub, gc.getFont(),
713+
gc.getBackground(), gc.getForeground()));
681714
}
682715
Point p = gc.stringExtent(sub);
683-
if (lfCount > 0) {
684-
cy += lfCount * (textWidgetLineHeight + textWidget.getLineSpacing());
716+
if (sub.endsWith("\n")) { //$NON-NLS-1$
717+
cy += textWidgetLineHeight + textWidget.getLineSpacing();
685718
cx = x;
719+
} else {
720+
cx += p.x;
686721
}
687-
cx += p.x;
688722
}
723+
gc.setFont(currentFont);
724+
} else {
725+
int lfCount = 0;
726+
if (sub.contains("\n")) { //$NON-NLS-1$
727+
lfCount = sub.split("\n", -1).length - 1; //$NON-NLS-1$
728+
sub = sub.substring(sub.lastIndexOf("\n") + 1); //$NON-NLS-1$
729+
}
730+
Point p = gc.stringExtent(sub);
731+
if (lfCount > 0) {
732+
cy += lfCount * (textWidgetLineHeight + textWidget.getLineSpacing());
733+
cx = x;
734+
}
735+
cx += p.x;
689736
}
690-
return result;
691737
}
738+
gc.setFont(font);
692739
}
693740

694741
private static List<StyleRange> computeStyleRanges(ITextViewer v, int offset, String source) {
695742
List<StyleRange> result = new ArrayList<>();
696743
if (!(v instanceof SourceViewer sv)) {
697744
return result;
698745
}
746+
IDocument originalDocument = sv.getDocument();
747+
if (originalDocument == null) {
748+
return result;
749+
}
699750
try {
700-
IDocument originalDocument = sv.getDocument();
701751
String prefix = originalDocument.get(0, offset /* diff.leftStart */);
702752
IDocument document = new Document(prefix + source);
703753
IRegion damage = new Region(prefix.length(), source.length());
704-
result = sv.computeStyleRanges(document, damage);
754+
try {
755+
result = sv.computeStyleRanges(document, damage);
756+
} catch (NullPointerException e) {
757+
// SourceViewer.computeStyleRanges dereferences its presentation
758+
// reconciler without a null check; a viewer configured without one
759+
// (e.g. a plain projection viewer) has no syntax coloring to offer
760+
return result;
761+
}
705762
int startOffset = prefix.length();
706763
for (StyleRange next : result) {
707764
if (next.start < startOffset) {
@@ -788,26 +845,6 @@ private static List<DetailedDiffRange> computeDetailedDiffRanges(UnifiedDiff dif
788845
return result;
789846
}
790847

791-
private static final class ForegroundInfo {
792-
793-
final int x;
794-
final int y;
795-
final String str;
796-
final Font font;
797-
final Color background;
798-
final Color foreground;
799-
800-
public ForegroundInfo(int x, int y, String str, Font font, Color background, Color foreground) {
801-
this.x = x;
802-
this.y = y;
803-
this.str = str;
804-
this.font = font;
805-
this.background = background;
806-
this.foreground = foreground;
807-
}
808-
809-
}
810-
811848
@Override
812849
public String getLabel() {
813850
return this.unifiedDiffLabel;
@@ -1092,70 +1129,7 @@ public Point draw(GC gc, StyledText textWidget, Color color, int x, int y) {
10921129
}
10931130
foregrounds = new ArrayList<>();
10941131
gc.setFont(cachedFont);
1095-
int textWidgetLineHeight = textWidget.getLineHeight();
1096-
int cx = x;
1097-
int cy = y;
1098-
for (StyleRange range : ranges) {
1099-
String sub = label.substring(range.start, range.start + range.length);
1100-
if (sub.trim().length() > 0) {
1101-
if (range.background != null) {
1102-
gc.setBackground(range.background);
1103-
}
1104-
if (range.foreground != null) {
1105-
gc.setForeground(range.foreground);
1106-
}
1107-
Font currentFont = gc.getFont();
1108-
var rangeWithFont = transformFontStyleToFont(currentFont, range);
1109-
if (rangeWithFont.font != null) {
1110-
gc.setFont(rangeWithFont.font);
1111-
}
1112-
String[] lines = sub.split("\n"); //$NON-NLS-1$
1113-
if (lines.length > 1) {
1114-
for (int i = 0; i < lines.length; i++) {
1115-
String line = lines[i].replace("\r", ""); //$NON-NLS-1$ //$NON-NLS-2$
1116-
gc.drawString(line, cx, cy, true);
1117-
foregrounds.add(new ForegroundInfo(cx - x, cy - y, line, gc.getFont(), gc.getBackground(),
1118-
gc.getForeground()));
1119-
Point p = gc.stringExtent(line);
1120-
if (i < lines.length - 1) {
1121-
cy += textWidgetLineHeight + textWidget.getLineSpacing();
1122-
cx = x;
1123-
} else {
1124-
if (sub.endsWith("\n")) { //$NON-NLS-1$
1125-
cy += textWidgetLineHeight + textWidget.getLineSpacing();
1126-
cx = x;
1127-
} else {
1128-
cx += p.x;
1129-
}
1130-
}
1131-
}
1132-
} else {
1133-
gc.drawString(sub, cx, cy, true);
1134-
foregrounds.add(new ForegroundInfo(cx - x, cy - y, sub, gc.getFont(), gc.getBackground(),
1135-
gc.getForeground()));
1136-
Point p = gc.stringExtent(sub);
1137-
if (sub.endsWith("\n")) { //$NON-NLS-1$
1138-
cy += textWidgetLineHeight + textWidget.getLineSpacing();
1139-
cx = x;
1140-
} else {
1141-
cx += p.x;
1142-
}
1143-
}
1144-
gc.setFont(currentFont);
1145-
} else {
1146-
int lfCount = 0;
1147-
if (sub.contains("\n")) { //$NON-NLS-1$
1148-
lfCount = sub.split("\n", -1).length - 1; //$NON-NLS-1$
1149-
sub = sub.substring(sub.lastIndexOf("\n") + 1); //$NON-NLS-1$
1150-
}
1151-
Point p = gc.stringExtent(sub);
1152-
if (lfCount > 0) {
1153-
cy += lfCount * (textWidgetLineHeight + textWidget.getLineSpacing());
1154-
cx = x;
1155-
}
1156-
cx += p.x;
1157-
}
1158-
}
1132+
drawStyleRanges(gc, textWidget, ranges, label, styledFonts, x, y, foregrounds::add);
11591133
return result;
11601134
}
11611135

‎team/tests/org.eclipse.team.tests.core/src/org/eclipse/team/tests/ui/UnifiedDiffCodeMiningProviderTest.java‎

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@
5959
import org.eclipse.swt.widgets.Display;
6060
import org.eclipse.swt.widgets.Shell;
6161
import org.eclipse.text.undo.DocumentUndoManagerRegistry;
62+
import org.eclipse.swt.graphics.GC;
6263
import org.junit.jupiter.api.AfterEach;
6364
import org.junit.jupiter.api.BeforeEach;
6465
import org.junit.jupiter.api.Test;
@@ -388,12 +389,15 @@ public void testFooterMiningIsCreatedWhenDocumentHasNoTrailingNewline() throws E
388389
}
389390

390391
/**
391-
* The footer mining computes style ranges using the same {@code computeStyleRanges}
392-
* path as the line-header mining. With a plain viewer that has no presentation
393-
* reconciler the result is empty — the same fallback the header takes.
392+
* The footer mining draws through the shared {@code draw()} entry point that
393+
* the line-header mining uses. This viewer has no presentation reconciler, so
394+
* {@code computeStyleRanges} yields no ranges and {@code draw()} degrades to
395+
* the plain-rendering fallback: it must run to completion without throwing and
396+
* set {@code lastRectangle}, showing the footer survives a viewer that offers
397+
* no syntax coloring.
394398
*/
395399
@Test
396-
public void testFooterMiningStyleRangesUseTheSameLogicAsTheHeader() throws Exception {
400+
public void testFooterMiningDrawsWithoutSyntaxColoring() throws Exception {
397401
switchToDocument(new Document("line 0\nline 1 changed"));
398402

399403
IStatus status = UnifiedDiffManager.open(viewer, document, model, null, "line 0\nline 1\n", MODE, null, null,
@@ -408,13 +412,18 @@ public void testFooterMiningStyleRangesUseTheSameLogicAsTheHeader() throws Excep
408412
}
409413
}
410414
assertNotNull(footer, "a footer mining must be present");
415+
assertThat(footer.getLastRectangle()).as("lastRectangle starts null before the first draw").isNull();
411416

412-
// With no presentation reconciler the viewer returns no style ranges — the
413-
// footer must behave identically to the header: accept the empty result
414-
// without error, not substitute some other rendering.
415-
assertThat(footer.getLabel().trim())
416-
.as("the footer mining shows the diff content")
417-
.isNotEmpty();
417+
GC gc = new GC(shell);
418+
try {
419+
footer.draw(gc, viewer.getTextWidget(), null, 0, 0);
420+
} finally {
421+
gc.dispose();
422+
}
423+
424+
assertThat(footer.getLastRectangle())
425+
.as("draw() must set lastRectangle — it stayed null, so draw() bailed out before rendering")
426+
.isNotNull();
418427
}
419428

420429
// ------------------------------------------------------------------ helpers

0 commit comments

Comments
 (0)