From 92142c352e7e5cb5806e0dbb21c16e53bf47f31c Mon Sep 17 00:00:00 2001 From: Tobias Melcher Date: Wed, 23 Sep 2026 15:05:48 +0200 Subject: [PATCH] Fix detailed-diff highlight clipping at end of trimmed diff string When a detailed diff ended exactly at the last character of the trimmed diff string (i.e. start + length == trimmedLength), the old >= guard incorrectly reduced the length by the number of stripped trailing newlines, cutting off that many characters from the highlight rectangle. The fix changes the condition to > so the length is only clamped when the range actually overshoots. The same bug existed independently in computeDetailedDiffRanges and createDetailedDiffBackgroundRanges; both now delegate to a shared helper clampDetailedDiffLength in UnifiedDiffText. --- .../UnifiedDiffCodeMiningProvider.java | 27 +++--- .../unifieddiff/internal/UnifiedDiffText.java | 13 +++ .../team/tests/ui/UnifiedDiffTextTest.java | 97 +++++++++++++++++++ 3 files changed, 121 insertions(+), 16 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 3ce35d6f8bf..60bdf744b3f 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 @@ -15,6 +15,7 @@ import static org.eclipse.compare.unifieddiff.internal.UnifiedDiffManager.error; import static org.eclipse.compare.unifieddiff.internal.UnifiedDiffManager.isOverlay; +import static org.eclipse.compare.unifieddiff.internal.UnifiedDiffText.clampDetailedDiffLength; import static org.eclipse.compare.unifieddiff.internal.UnifiedDiffText.countLines; import static org.eclipse.compare.unifieddiff.internal.UnifiedDiffText.mapOffsetToTabExpanded; import static org.eclipse.compare.unifieddiff.internal.UnifiedDiffText.mergeStyleRanges; @@ -759,12 +760,9 @@ static List createDetailedDiffBackgroundRanges(UnifiedDiff diff, int if (detailedDiffStr.trim().length() == 0) { continue; } - if (detailedDiffStart + detailedDiffLength >= trimmedDiffStr.length()) { - int delta = diffStr.length() - trimmedDiffStr.length(); - if (detailedDiffLength <= delta) { - continue; - } - detailedDiffLength -= delta; + detailedDiffLength = clampDetailedDiffLength(detailedDiffStart, detailedDiffLength, trimmedDiffStr.length()); + if (detailedDiffLength <= 0) { + continue; } int expandedStart = mapOffsetToTabExpanded(diffStr, detailedDiffStart, tabWidth); int expandedEnd = mapOffsetToTabExpanded(diffStr, detailedDiffStart + detailedDiffLength, tabWidth); @@ -940,7 +938,6 @@ private static List computeDetailedDiffRanges(UnifiedDiff dif || 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; @@ -949,11 +946,9 @@ private static List computeDetailedDiffRanges(UnifiedDiff dif if (detailedDiffStr.trim().length() == 0) { continue; } - if (detailedDiffStart + detailedDiffLength >= diffStr.length()) { - if (detailedDiffLength <= diffStrDelta) { - continue; - } - detailedDiffLength -= diffStrDelta; + detailedDiffLength = clampDetailedDiffLength(detailedDiffStart, detailedDiffLength, diffStr.length()); + if (detailedDiffLength <= 0) { + continue; } // String#split drops trailing empty strings, so it must not be used to // count lines: a prefix ending with \n starts the next line @@ -1228,19 +1223,19 @@ private int getLineDelimiterLength(Document diffStrDoc, int middleLine) throws B return delim.length(); } - private static class RangeInfo { + public static class RangeInfo { int rangeIndex; int offset; Point position; - RangeInfo(int rangeIndex, int offset, Point position) { + public RangeInfo(int rangeIndex, int offset, Point position) { this.rangeIndex = rangeIndex; this.offset = offset; this.position = position; } } - private Point getPositionForOffset(StyledText tw, GC gc, int offset, String str, List ranges, + public Point getPositionForOffset(StyledText tw, GC gc, int offset, String str, List ranges, RangeInfo rangeInfo) { String sub = str.substring(0, offset); Point result = null; @@ -1268,7 +1263,7 @@ private Point getPositionForOffset(StyledText tw, GC gc, int offset, String str, } } int lfIdx = sub.lastIndexOf("\n"); //$NON-NLS-1$ - if (lfIdx > 0) { + if (lfIdx >= 0) { if (lfIdx == sub.length() - 1 && isLastForCurrentOffset(ranges, i, offset)) { sub = sub.substring(0, lfIdx); } else { 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 eb12952fb74..2c7b996aa2b 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 @@ -162,4 +162,17 @@ private static StyleRange createRange(StyleRange base, int start, int length, Co } return r; } + + /** + * Clamps {@code length} so that {@code start + length} does not exceed + * {@code trimmedEnd}. Returns the clamped length, or {@code <= 0} when the + * range falls entirely outside. + */ + public static int clampDetailedDiffLength(int start, int length, int trimmedEnd) { + int end = start + length; + if (end > trimmedEnd) { + length -= end - trimmedEnd; + } + return length; + } } 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 7cf8bef93c4..d4adbb19ab5 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 @@ -13,21 +13,35 @@ *******************************************************************************/ package org.eclipse.team.tests.ui; +import static org.eclipse.compare.unifieddiff.internal.UnifiedDiffText.clampDetailedDiffLength; import static org.eclipse.compare.unifieddiff.internal.UnifiedDiffText.countLines; import static org.eclipse.compare.unifieddiff.internal.UnifiedDiffText.mapOffsetToTabExpanded; import static org.eclipse.compare.unifieddiff.internal.UnifiedDiffText.mergeStyleRanges; import static org.eclipse.compare.unifieddiff.internal.UnifiedDiffText.replaceTabWithSpaces; import static org.junit.jupiter.api.Assertions.assertAll; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.List; +import org.eclipse.compare.unifieddiff.UnifiedDiffMode; +import org.eclipse.compare.unifieddiff.internal.UnifiedDiffCodeMiningProvider.UnifiedDiffLineHeaderCodeMining; +import org.eclipse.compare.unifieddiff.internal.UnifiedDiffCodeMiningProvider.UnifiedDiffLineHeaderCodeMining.RangeInfo; +import org.eclipse.compare.unifieddiff.internal.UnifiedDiffManager.UnifiedDiff; +import org.eclipse.jface.text.Document; +import org.eclipse.jface.text.Position; 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.GC; +import org.eclipse.swt.graphics.Point; import org.eclipse.swt.widgets.Display; +import org.eclipse.swt.widgets.Shell; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; /** @@ -36,6 +50,7 @@ * painted, so an off-by-one here shows up as a misplaced highlight that is easy * to miss in a manual test. */ +@SuppressWarnings("restriction") public class UnifiedDiffTextTest { private static final Color BACKGROUND_1 = systemColor(SWT.COLOR_RED); @@ -43,6 +58,27 @@ public class UnifiedDiffTextTest { private static final Color FOREGROUND_1 = systemColor(SWT.COLOR_BLUE); private static final Color FOREGROUND_2 = systemColor(SWT.COLOR_YELLOW); + private Shell shell; + private StyledText styledText; + private GC gc; + + @BeforeEach + public void setUp() { + shell = new Shell(Display.getDefault()); + styledText = new StyledText(shell, SWT.NONE); + gc = new GC(styledText); + } + + @AfterEach + public void tearDown() { + if (gc != null && !gc.isDisposed()) { + gc.dispose(); + } + if (shell != null && !shell.isDisposed()) { + shell.dispose(); + } + } + // ---------------------------------------------------------------- countLines /** @@ -221,6 +257,59 @@ public void testMergeStyleRangesWithGapsBetweenForegrounds() { assertTilesForegrounds(result, foregrounds); } + // ------------------------------------------- clampDetailedDiffLength + + @Test + public void testClampDetailedDiffLengthEndExactlyAtTrimmedEnd() { + // diff ends exactly at the last char of the trimmed string — must not clip + assertEquals(4, clampDetailedDiffLength(22, 4, 26)); + } + + @Test + public void testClampDetailedDiffLengthEndBeyondTrimmedEnd() { + // diff overshoots by 1 (e.g. includes a stripped trailing newline) — clip by 1 + assertEquals(4, clampDetailedDiffLength(22, 5, 26)); + } + + @Test + public void testClampDetailedDiffLengthFullyOutside() { + // diff is entirely in the stripped region — result is <= 0 + assertTrue(clampDetailedDiffLength(26, 1, 26) <= 0); + } + + @Test + public void testClampDetailedDiffLengthNoOvershoot() { + // diff well within bounds — unchanged + assertEquals(3, clampDetailedDiffLength(5, 3, 20)); + } + + // ------------------------------------------- getPositionForOffset + + @Test + public void testGetPositionForOffsetResetsXAfterNewlineAtRangeStart() throws Exception { + // Label: "abc\nxyz" — two lines, 3 chars each. + // Range [0, 3): "abc" — first line only, no newline. + // Range [3, 7): "\nxyz" — starts with \n at index 0 (lfIdx == 0). + // Querying the position of offset 7 (end of "xyz") must return x=width("xyz"), + // not x=width("abc")+width("xyz"), because the \n resets the x accumulator. + String str = "abc\nxyz"; + List ranges = List.of(styledRange(0, 3), styledRange(3, 4)); + + Document doc = new Document(str); + UnifiedDiff diff = new UnifiedDiff(doc, 0, str.length(), str, doc, 0, str.length(), str, + List.of(), UnifiedDiffMode.REPLACE_MODE); + UnifiedDiffLineHeaderCodeMining mining = new UnifiedDiffLineHeaderCodeMining( + new Position(0, 1), null, diff, 4, null, null, null); + + gc.setFont(styledText.getFont()); + Point result = mining.getPositionForOffset(styledText, gc, 7, str, ranges, new RangeInfo(-1, -1, null)); + + assertNotNull(result, "position must not be null"); + Point expected = gc.stringExtent("xyz"); + assertEquals(expected.x, result.x, + "x must equal the width of 'xyz' alone, not width('abc')+width('xyz')"); + } + // ------------------------------------------------------------------ helpers /** @@ -292,4 +381,12 @@ private static StyleRange background(int start, int length, Color color) { private static Color systemColor(int id) { return Display.getDefault().getSystemColor(id); } + + private static StyleRange styledRange(int start, int length) { + StyleRange range = new StyleRange(); + range.start = start; + range.length = length; + range.foreground = systemColor(SWT.COLOR_BLUE); + return range; + } }