From bca175dc593399b27dbaa5f585c1d2f380a9d0f1 Mon Sep 17 00:00:00 2001 From: Tobias Melcher Date: Wed, 11 Nov 2026 15:17:28 +0100 Subject: [PATCH] Fix unified diff mining removal and footer painting Mark resolved annotations deleted before removing them so the painter clears their reserved line height. Synchronize footer drawing and disposal, and use the measured height to prevent cache invalidation from causing a null pointer exception. --- .../UnifiedDiffCodeMiningProvider.java | 10 ++-- .../internal/UnifiedDiffManager.java | 3 +- .../ui/UnifiedDiffCodeMiningProviderTest.java | 52 +++++++++++++++++++ .../team/tests/ui/UnifiedDiffManagerTest.java | 16 ++++++ 4 files changed, 76 insertions(+), 5 deletions(-) diff --git a/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/unifieddiff/internal/UnifiedDiffCodeMiningProvider.java b/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/unifieddiff/internal/UnifiedDiffCodeMiningProvider.java index 7bcf51683f5..1064e73e9b2 100644 --- a/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/unifieddiff/internal/UnifiedDiffCodeMiningProvider.java +++ b/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/unifieddiff/internal/UnifiedDiffCodeMiningProvider.java @@ -749,7 +749,7 @@ public String getRawLabel() { } @Override - public void dispose() { + public synchronized void dispose() { styleRanges = null; lastRectangle = null; cachedFont = null; @@ -770,7 +770,7 @@ private List styleRanges(String label) { } @Override - public Point draw(GC gc, StyledText textWidget, Color color, int x, int y) { + public synchronized Point draw(GC gc, StyledText textWidget, Color color, int x, int y) { gc.setBackground(this.deletionBackgroundColor); Color c = textWidget.getForeground(); gc.setForeground(c); @@ -797,7 +797,7 @@ public Point draw(GC gc, StyledText textWidget, Color color, int x, int y) { if (ranges.isEmpty()) { // no syntax coloring available; fall back to plain rendering result = super.draw(gc, textWidget, color, x, y); - drawBandBorder(gc, this.borderColor, y, textWidget.getBounds().width, lastRectangle.height, + drawBandBorder(gc, this.borderColor, y, textWidget.getBounds().width, result.y, this.bandBorder); return result; } @@ -809,7 +809,9 @@ public Point draw(GC gc, StyledText textWidget, Color color, int x, int y) { gc.setFont(font); drawStyleRanges(gc, textWidget, ranges, label, rawLabel, styledFonts, x, y, null, whitespace, trailingCarriageReturn, trailingLineFeed); - drawBandBorder(gc, this.borderColor, y, textWidget.getBounds().width, lastRectangle.height, + // Syntax-coloring callbacks can dispose the mining and clear lastRectangle. + // Use the size measured by this draw call rather than the invalidated cache. + drawBandBorder(gc, this.borderColor, y, textWidget.getBounds().width, result.y, this.bandBorder); return result; } diff --git a/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/unifieddiff/internal/UnifiedDiffManager.java b/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/unifieddiff/internal/UnifiedDiffManager.java index 9777291f2bf..7bd961dcaf8 100644 --- a/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/unifieddiff/internal/UnifiedDiffManager.java +++ b/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/unifieddiff/internal/UnifiedDiffManager.java @@ -1672,8 +1672,9 @@ private static void applyDiffRightStr(ITextViewer tv, IAnnotationModel model, Un positions.add(pos); replaceStrings.add(diff.rightStr); } - model.removeAnnotation(lanno); } + // The painter must see deleted headers when it clears their reserved height. + removeAnnotations(model, annos); if (tv instanceof ISourceViewerExtension5 ext) { ext.updateCodeMinings(); } diff --git a/team/tests/org.eclipse.team.tests.core/src/org/eclipse/team/tests/ui/UnifiedDiffCodeMiningProviderTest.java b/team/tests/org.eclipse.team.tests.core/src/org/eclipse/team/tests/ui/UnifiedDiffCodeMiningProviderTest.java index 2ef2306dbb0..465998c7e07 100644 --- a/team/tests/org.eclipse.team.tests.core/src/org/eclipse/team/tests/ui/UnifiedDiffCodeMiningProviderTest.java +++ b/team/tests/org.eclipse.team.tests.core/src/org/eclipse/team/tests/ui/UnifiedDiffCodeMiningProviderTest.java @@ -18,6 +18,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; import static org.junit.jupiter.api.Assumptions.assumeTrue; @@ -49,6 +50,7 @@ import org.eclipse.jface.text.BadLocationException; import org.eclipse.jface.text.Document; import org.eclipse.jface.text.IDocument; +import org.eclipse.jface.text.IRegion; import org.eclipse.jface.text.ITextViewer; import org.eclipse.jface.text.Position; import org.eclipse.jface.text.TextAttribute; @@ -74,6 +76,7 @@ import org.eclipse.jface.text.source.projection.ProjectionAnnotationModel; import org.eclipse.jface.text.source.projection.ProjectionViewer; import org.eclipse.swt.SWT; +import org.eclipse.swt.custom.StyleRange; import org.eclipse.swt.custom.StyledText; import org.eclipse.swt.graphics.Color; import org.eclipse.swt.graphics.Font; @@ -633,6 +636,55 @@ private void draw(UnifiedDiffLineHeaderCodeMining mining, List scannedDo } } + @Test + public void testFooterMiningDisposalDuringPlainDrawingDoesNotLoseHeight() throws Exception { + assertFooterDisposalDuringDrawing(false); + } + + @Test + public void testFooterMiningDisposalDuringStyledDrawingDoesNotLoseHeight() throws Exception { + assertFooterDisposalDuringDrawing(true); + } + + private void assertFooterDisposalDuringDrawing(boolean styled) throws Exception { + switchToDocument(new Document("line 0\nx")); + assertTrue(open("line 0\nchanged\n", MODE).isOK()); + UnifiedDiffFooterCodeMining footer = footerMining(); + UnifiedDiffFooterCodeMining[] painting = new UnifiedDiffFooterCodeMining[1]; + ProjectionViewer paintingViewer = new ProjectionViewer(shell, null, null, false, SWT.V_SCROLL) { + @Override + public List computeStyleRanges(IDocument syntaxDocument, IRegion region) { + // Invalidate the cache after draw measured its size, before it draws the border. + painting[0].dispose(); + return styled ? new ArrayList<>(List.of(new StyleRange(region.getOffset(), region.getLength(), + display.getSystemColor(SWT.COLOR_DARK_BLUE), null))) : new ArrayList<>(); + } + }; + paintingViewer.setDocument(document); + paintingViewer.getTextWidget().setSize(400, 100); + Color deletionColor = new Color(display, 11, 22, 33); + Color detailedDiffColor = new Color(display, 44, 55, 66); + Color borderColor = new Color(display, 77, 88, 99); + painting[0] = new UnifiedDiffFooterCodeMining(document, provider, footer.getUnifiedDiff(), 4, + deletionColor, detailedDiffColor, borderColor, paintingViewer); + Image image = new Image(display, 400, 100); + GC gc = new GC(image); + try { + Point size = painting[0].draw(gc, paintingViewer.getTextWidget(), null, 0, 0); + assertTrue(size.y > 0, "drawing must retain its measured height after cache invalidation"); + assertNull(painting[0].getLastRectangle(), "disposal must have cleared the cached rectangle"); + assertTrue(firstColumnWith(image, borderColor) >= 0, "drawing must still paint the band border"); + } finally { + gc.dispose(); + image.dispose(); + painting[0].dispose(); + paintingViewer.getTextWidget().dispose(); + deletionColor.dispose(); + detailedDiffColor.dispose(); + borderColor.dispose(); + } + } + /** * A word-level footer highlight following bold text must start after the bold * text, not where the base font would put it. Inspects the pixels diff --git a/team/tests/org.eclipse.team.tests.core/src/org/eclipse/team/tests/ui/UnifiedDiffManagerTest.java b/team/tests/org.eclipse.team.tests.core/src/org/eclipse/team/tests/ui/UnifiedDiffManagerTest.java index bef57eb6b6e..603d6a49c67 100644 --- a/team/tests/org.eclipse.team.tests.core/src/org/eclipse/team/tests/ui/UnifiedDiffManagerTest.java +++ b/team/tests/org.eclipse.team.tests.core/src/org/eclipse/team/tests/ui/UnifiedDiffManagerTest.java @@ -23,6 +23,7 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNotSame; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -1127,6 +1128,14 @@ public void testRevertingEarlierDiffRefreshesRemainingCodeMiningPosition() throw List diffs = UnifiedDiffManager.get(viewer()); assertEquals(2, diffs.size(), "the input must produce two revert diffs"); + StyledText tw = viewer().getTextWidget(); + tw.setSize(800, 600); + assertNotNull(waitForLineHeaderAnnotation()); + UnifiedDiffManager.UnifiedDiff laterDiff = diffs.get(1); + lineHeaderMiningFor(laterDiff); + firePaint(tw); + int firstLine = tw.getLineAtOffset(diffs.get(0).leftStart); + assertTrue(tw.getLineVerticalIndent(firstLine) > 0, "the first mining must reserve height before reverting"); viewer().setSelectedRange(diffs.get(0).leftStart, 0); executeEditorCommand("org.eclipse.compare.acceptCurrentDiff"); //$NON-NLS-1$ @@ -1134,9 +1143,16 @@ public void testRevertingEarlierDiffRefreshesRemainingCodeMiningPosition() throw diffs = UnifiedDiffManager.get(viewer()); assertEquals(1, diffs.size(), "reverting the first diff must leave the later diff active"); + assertSame(laterDiff, diffs.get(0), "the surviving band must belong to the later diff"); + assertEquals(0, tw.getLineVerticalIndent(firstLine), "removing the first header must clear its reserved height"); UnifiedDiffLineHeaderCodeMining mining = lineHeaderMiningFor(diffs.get(0)); assertEquals(afterRevert.indexOf("new later replacement"), mining.getPosition().offset, "the remaining code mining must be positioned after the reverted text"); + assertTrue(mining.getLabel().contains("old later"), "the surviving mining must retain the later deletion text"); + firePaint(tw); + assertNotNull(mining.getLastRectangle(), "the surviving mining must be painted"); + assertEquals(tw.getLocationAtOffset(tw.getOffsetAtLine(tw.getLineAtOffset(mining.getPosition().offset))).x, + mining.getLastRectangle().x, "the surviving mining must paint at the line origin"); } @Test