From 05f922c60c8f10e3eea38349e828ec7ed18d5233 Mon Sep 17 00:00:00 2001 From: Lars Vogel Date: Wed, 2 Sep 2026 23:45:56 +0200 Subject: [PATCH] Paint unified diff decorations only for the damaged lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The unified diff paint listener queried StyledText for the bounds of every line of every hunk on each repaint. With inlined annotations the widget has variable line heights, so each query lays out lines that are not on screen, and a repaint costs O(document): on a 1.7 MB file opening a diff blocked the UI for seconds and each page scroll for a good part of a second. Clip the loop to the lines inside the paint event's damaged rectangle, so a repaint is proportional to what is visible. The line header code mining also re-derived the line range of every detailed diff from copies of the hunk text while drawing. Resolve these ranges once when the mining is created, off the UI thread, and reuse the split lines and Document across the detailed diffs of one hunk. Assisted-by: multiple AI agents and layers of automated tooling 🤖 --- .../UnifiedDiffCodeMiningProvider.java | 85 ++++++++++++------- .../internal/UnifiedDiffManager.java | 11 ++- .../unifieddiff/internal/UnifiedDiffText.java | 10 ++- .../team/tests/ui/UnifiedDiffTextTest.java | 14 +++ 4 files changed, 85 insertions(+), 35 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 143a2a68eec..0e545cd993b 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 @@ -359,6 +359,8 @@ public static class UnifiedDiffLineHeaderCodeMining extends LineHeaderCodeMining private final int tabWidth; private ITextViewer viewer; + private final List detailedDiffRanges; + private Rectangle lastRectangle; private List backgrounds; private List foregrounds; @@ -380,9 +382,47 @@ public UnifiedDiffLineHeaderCodeMining(Position position, ICodeMiningProvider pr this.diff = diff; this.tabWidth = tabWidth; this.viewer = viewer; + this.detailedDiffRanges = computeDetailedDiffRanges(diff); ((MouseClickConsumer) getAction()).setCodeMining(this); } + /** + * Line range of a detailed diff, resolved once so painting does not scan + * the hunk text. + */ + private record DetailedDiffRange(String diffStr, int start, int length, int fromLine, int toLine) { + } + + private static List computeDetailedDiffRanges(UnifiedDiff diff) { + boolean useRight = diff.mode.equals(UnifiedDiffMode.OVERLAY_MODE) + || diff.mode.equals(UnifiedDiffMode.OVERLAY_READ_ONLY_MODE) + || diff.mode.equals(UnifiedDiffMode.REVERT_MODE); + String fullDiffStr = useRight ? diff.rightStr : diff.leftStr; + String diffStr = removeTrailingNewLines(fullDiffStr); + int diffStrDelta = fullDiffStr.length() - diffStr.length(); + List result = new ArrayList<>(); + for (var detailedDiff : diff.detailedDiffs) { + String detailedDiffStr = useRight ? detailedDiff.rightStr : detailedDiff.leftStr; + int detailedDiffStart = useRight ? detailedDiff.rightStart : detailedDiff.leftStart; + int detailedDiffLength = useRight ? detailedDiff.rightLength : detailedDiff.leftLength; + if (detailedDiffStr.trim().length() == 0) { + continue; + } + if (detailedDiffStart + detailedDiffLength >= diffStr.length()) { + if (detailedDiffLength <= diffStrDelta) { + continue; + } + detailedDiffLength -= diffStrDelta; + } + // String#split drops trailing empty strings, so it must not be used to + // count lines: a prefix ending with \n starts the next line + int fromLine = countLines(diffStr, detailedDiffStart); + int toLine = countLines(diffStr, detailedDiffStart + detailedDiffLength); + result.add(new DetailedDiffRange(diffStr, detailedDiffStart, detailedDiffLength, fromLine, toLine)); + } + return result; + } + private static final class ForegroundInfo { final int x; @@ -636,37 +676,16 @@ public Point draw(GC gc, StyledText textWidget, Color color, int x, int y) { // draw darker background for detailed diff gc.setBackground(this.detailedDiffColor); backgrounds = new ArrayList<>(); - for (var detailedDiff : this.diff.detailedDiffs) { - String diffStr = this.diff.leftStr; - String detailedDiffStr = detailedDiff.leftStr; - int detailedDiffStart = detailedDiff.leftStart; - int detailedDiffLength = detailedDiff.leftLength; - if (diff.mode.equals(UnifiedDiffMode.OVERLAY_MODE) - || diff.mode.equals(UnifiedDiffMode.OVERLAY_READ_ONLY_MODE) - || diff.mode.equals(UnifiedDiffMode.REVERT_MODE)) { - diffStr = this.diff.rightStr; - detailedDiffStr = detailedDiff.rightStr; - detailedDiffStart = detailedDiff.rightStart; - detailedDiffLength = detailedDiff.rightLength; - } - if (detailedDiffStr.trim().length() == 0) { - continue; - } - int diffStrLength = diffStr.length(); - diffStr = removeTrailingNewLines(diffStr); - if (detailedDiffStart + detailedDiffLength >= diffStr.length()) { - int diffStrDelta = diffStrLength - diffStr.length(); - if (detailedDiffLength <= diffStrDelta) { - continue; - } - detailedDiffLength -= diffStrDelta; - } + String[] diffLines = null; + Document diffStrDoc = null; + for (var range : this.detailedDiffRanges) { + String diffStr = range.diffStr(); + int detailedDiffStart = range.start(); + int detailedDiffLength = range.length(); + int fromLine = range.fromLine(); + int toLine = range.toLine(); try { var rangeInfo = new RangeInfo(-1, -1, null); - // String#split drops trailing empty strings, so it must not be used to - // count lines: a prefix ending with \n starts the next line - int fromLine = countLines(diffStr.substring(0, detailedDiffStart)); - int toLine = countLines(diffStr.substring(0, detailedDiffStart + detailedDiffLength)); if (fromLine == toLine) { int starty = getYForLine(fromLine - 1, y, gc, textWidget); Point start = getPositionForOffset(textWidget, gc, detailedDiffStart, diffStr, ranges, @@ -685,11 +704,13 @@ public Point draw(GC gc, StyledText textWidget, Color color, int x, int y) { } } else { // mark first line until end - String[] lines = diffStr.split("\n"); //$NON-NLS-1$ - String firstLine = lines[fromLine - 1]; + if (diffLines == null) { + diffLines = diffStr.split("\n"); //$NON-NLS-1$ + diffStrDoc = new Document(diffStr); + } + String firstLine = diffLines[fromLine - 1]; int starty = getYForLine(fromLine - 1, y, gc, textWidget); int idx = getOffsetAtLine(diffStr, detailedDiffStart); - var diffStrDoc = new Document(diffStr); int fromLineOffset; try { fromLineOffset = diffStrDoc.getLineOffset(fromLine - 1); 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 cb5a6012edd..21fee40f26e 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 @@ -1316,6 +1316,10 @@ void dispose() { @Override public void paintControl(PaintEvent e) { Rectangle bounds = this.w.getBounds(); + // Only the damaged lines: getTextBounds lays out every line it is asked for, + // so touching every hunk would make each repaint O(document). + int firstDamagedLine = this.w.getLineIndex(e.y); + int lastDamagedLine = this.w.getLineIndex(e.y + e.height); Iterator it = this.model.getAnnotationIterator(); while (it.hasNext()) { Annotation anno = it.next(); @@ -1338,8 +1342,11 @@ public void paintControl(PaintEvent e) { } posLength = pos.length; } - int fromLine = this.w.getLineAtOffset(posOffset); - int toLine = this.w.getLineAtOffset(posOffset + posLength); + int fromLine = Math.max(this.w.getLineAtOffset(posOffset), firstDamagedLine); + int toLine = Math.min(this.w.getLineAtOffset(posOffset + posLength), lastDamagedLine + 1); + if (fromLine >= toLine) { + continue; + } e.gc.setBackground(this.additionBackgroundColor); for (int lineNr = fromLine; lineNr < toLine; lineNr++) { String line = this.w.getLine(lineNr); diff --git a/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/unifieddiff/internal/UnifiedDiffText.java b/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/unifieddiff/internal/UnifiedDiffText.java index c46e4279524..eb12952fb74 100644 --- a/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/unifieddiff/internal/UnifiedDiffText.java +++ b/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/unifieddiff/internal/UnifiedDiffText.java @@ -36,8 +36,16 @@ public static int countLines(String str) { if (str == null) { return 0; } + return countLines(str, str.length()); + } + + /** + * Returns the number of lines of the first {@code end} characters of the + * given string, like {@link #countLines(String)} on that prefix. + */ + public static int countLines(String str, int end) { int result = 1; - for (int i = 0; i < str.length(); i++) { + for (int i = 0; i < end; i++) { if (str.charAt(i) == '\n') { result++; } diff --git a/team/tests/org.eclipse.team.tests.core/src/org/eclipse/team/tests/ui/UnifiedDiffTextTest.java b/team/tests/org.eclipse.team.tests.core/src/org/eclipse/team/tests/ui/UnifiedDiffTextTest.java index 9b97d70bc44..7cf8bef93c4 100644 --- a/team/tests/org.eclipse.team.tests.core/src/org/eclipse/team/tests/ui/UnifiedDiffTextTest.java +++ b/team/tests/org.eclipse.team.tests.core/src/org/eclipse/team/tests/ui/UnifiedDiffTextTest.java @@ -63,6 +63,20 @@ public void testCountLinesForDetailedDiffStartOffsets() { () -> assertEquals(2, countLines("first\r\nsecond"), "CRLF is one delimiter")); } + /** + * The prefix overload counts the same lines as the copied substring would. + */ + @Test + public void testCountLinesOfPrefixMatchesSubstring() { + String diff = "first\nsecond\r\nthird\n"; + assertAll( // + () -> assertEquals(1, countLines(diff, 0), "empty prefix is one line"), // + () -> assertEquals(1, countLines(diff, 5), "prefix ending before the delimiter"), // + () -> assertEquals(2, countLines(diff, 6), "prefix ending after the delimiter"), // + () -> assertEquals(3, countLines(diff, 14), "CRLF is one delimiter"), // + () -> assertEquals(countLines(diff), countLines(diff, diff.length()), "whole string")); + } + // ------------------------------------------------------------ tab expansion @Test