Fix replaceTextRange logic for unfocused StyledText with variable line height #2318

tobiasmelcher · 2025-07-16T11:20:11Z

Updated productive code to prevent IllegalArgumentException when replacing text in a StyledText control with variable line height and no focus, by adding boundary checks.
Added a regression test to validate the fix: ensures no exception is thrown when replacing the full text range under these conditions.

tobiasmelcher · 2025-07-16T11:21:12Z

@HeikoKlare could you please take a look and review? Do you think that the boundary checks do make sense?

github-actions · 2025-07-16T11:30:04Z

Test Results

539 files - 7 539 suites - 7 35m 19s ⏱️ + 6m 3s
4 359 tests - 53 4 344 ✅ - 51 14 💤 - 3 1 ❌ +1
16 668 runs - 50 16 543 ✅ - 48 124 💤 - 3 1 ❌ +1

For more details on these failures, see this check.

Results for commit 49fa012. ± Comparison against base commit bef3ad7.

This pull request removes 54 and adds 1 tests. Note that renamed tests count towards both.

AllWin32Tests ImageWin32Tests ‑ testDisposeDrawnImageBeforeRequestingTargetForOtherZoom
AllWin32Tests ImageWin32Tests ‑ testDrawImageAtDifferentZooms(boolean)[1] true
AllWin32Tests ImageWin32Tests ‑ testDrawImageAtDifferentZooms(boolean)[2] false
AllWin32Tests ImageWin32Tests ‑ testImageDataForDifferentFractionalZoomsShouldBeDifferent
AllWin32Tests ImageWin32Tests ‑ testImageShouldHaveDimesionAsPerZoomLevel
AllWin32Tests ImageWin32Tests ‑ testRetrieveImageDataAtDifferentZooms(boolean)[1] true
AllWin32Tests ImageWin32Tests ‑ testRetrieveImageDataAtDifferentZooms(boolean)[2] false
AllWin32Tests TestTreeColumn ‑ test_ColumnOrder
AllWin32Tests Test_org_eclipse_swt_dnd_DND ‑ testByteArrayTransfer
AllWin32Tests Test_org_eclipse_swt_dnd_DND ‑ testFileTransfer
…

org.eclipse.swt.tests.junit.Test_org_eclipse_swt_custom_StyledText ‑ test_replaceTextRangeWithVariableHeight

♻️ This comment has been updated with latest results.

BeckerWdf

A don't know that code good enough to really be able to judge on this.

BeckerWdf · 2025-07-21T06:27:35Z

@HeikoKlare: Do you feel able to review this?

mickaelistria

The general idea of adding checks to prevent error is fine.
But I find the repetition of a similar check everywhere is making the code less clear. I've put a few suggestions inline, which I believe can make algorithms slightly clearer to understand, refining the loops instead of adding conditions here and there.

mickaelistria · 2025-07-21T07:21:43Z

...les/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/StyledText.java

 			while (lineIndex < lineCount) {
 				if (delta <= 0) break;
-				delta -= renderer.getCachedLineHeight(lineIndex++);
+				if (lineIndex >= 0 && lineIndex < lineCount) {


This lineIndex < lineCount doesn't have to be repeated.
Also the condition lineIndex >= 0 can be skipped if we initialize lineIndex to Math.max(0, topIndex)

done with 9ff6acd

mickaelistria · 2025-07-21T07:27:38Z

...les/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/StyledText.java

+				int topIndexHeight = 0;
+				if (topIndex >= 0 && topIndex < lineCount) {
+					topIndexHeight = renderer.getCachedLineHeight(topIndex);
+				}
+				topIndexY = -topIndexHeight - delta;


Wouldn't it be clearer to write it like

if (topIndex >= 0 && topIndex < lineCount) { topIndexY = -renderer.getCachedLineHeight(topIndex); } topIndexY -= delta;

done with 9ff6acd

mickaelistria · 2025-07-21T07:32:09Z

...les/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/StyledText.java

 		return topIndexY + topMargin;
 	int height = topIndexY;
 	if (lineIndex > topIndex) {
 		for (int i = topIndex; i < lineIndex; i++) {


Can't we use

for (int i = Math.max(topIndex, 0); i < Math.min(lineIndex, lineCount); i++) {

here and get rid of next conditions?

done with 9ff6acd

HeikoKlare · 2025-07-21T17:00:21Z

Sorry, I think I am not the best to review here as I am not that deep into the StyledText. Thanks to @mickaelistria for jumping in and reviewing! In case you need a second look from my side, please ping me again.

tobiasmelcher · 2025-07-28T07:26:57Z

thanks a lot @mickaelistria for reviewing the code. Do you think that we can merge this change?

mickaelistria

(sorry for nitpicking, but I have sad memories of fighting with this code and can't resist the discussion)
I have put some comments inline about possible ways to make code clearer.

mickaelistria · 2025-07-28T13:54:01Z

...les/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/StyledText.java

 		}
 	} else {
 		for (int i = topIndex - 1; i >= lineIndex; i--) {
+			if (i < 0 || i >= lineCount) {


This condition could also be merged in the loop, just like above (with the trick that te loop is reversed here)

for (int i - Math.min(topIndex - 1, lineCount); i >= Math.max(0, lineIndex); i--) {

or, if you prefer expressive names

int lastLineToConsider = Math.min(topIndex - 1, lineCount); int firstLineToConsider = Math.max(0, lineIndex); for (int i = lastLineToConsider; i >= firstLineToConsider; i--) { ...

which actually isn't so interesting to get in reverse order, it could easily be

for (int i = firstLineToConsider, i <= lastLineToConsider; i++)

done with 4d9d7e6; looks like that you were testing me with int lastLineToConsider = Math.min(topIndex - 1, lineCount); if I understood the algorithm :-) But luckily, the test detected the off-by-one error.

mickaelistria · 2025-07-28T17:57:18Z

Great, thanks!
I am also wondering whether it would make things clearer to create a simple boolean lineExists(int lineNumber) { return lineNumber >= 0 && lineNumber < lineCount); } method to reuse here and there. What do you think?

tobiasmelcher · 2025-07-29T08:31:49Z

Great, thanks! I am also wondering whether it would make things clearer to create a simple boolean lineExists(int lineNumber) { return lineNumber >= 0 && lineNumber < lineCount); } method to reuse here and there. What do you think?

done with [95618eb] - could you please check? Did I catch all locations? I hope I did not introduce any error with this change.

mickaelistria · 2025-07-29T11:05:49Z

...les/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/StyledText.java

 			}
-			if (lineIndex == 0 || -delta + renderer.getCachedLineHeight(lineIndex) <= clientAreaHeight - topMargin - bottomMargin) {
+			int lineHeight = 0;
+			if (lineIndex >= 0 && lineIndex < lineCount) {


could be lineExists(lineIndex)?

Thanks a lot. Sorry my fault, I was searching for all occurrences of "lineCount" but somehow missed this location. Code updated with [99ae962].

mickaelistria · 2025-07-29T11:06:05Z

...les/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/StyledText.java

-				int lineHeight = renderer.getCachedLineHeight(lineIndex - 1);
+				int previousLineIndex = lineIndex - 1;
+				int lineHeight = 0;
+				if (previousLineIndex >= 0 && previousLineIndex < lineCount) {


could be lineExists(lineIndex)?

Thanks a lot. Sorry my fault, I was searching for all occurrences of "lineCount" but somehow missed this location. Code updated with [99ae962].

mickaelistria · 2025-07-29T11:06:11Z

...les/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/StyledText.java

 			} else {
 				topIndex = lineIndex - 1;
-				topIndexY = -renderer.getCachedLineHeight(topIndex) - delta;
+				if (topIndex >= 0 && topIndex < lineCount) {


could be lineExists(lineIndex)?

Thanks a lot. Sorry my fault, I was searching for all occurrences of "lineCount" but somehow missed this location. Code updated with [99ae962].

mickaelistria · 2025-07-29T11:06:16Z

...les/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/StyledText.java

+				lineIndex++;
+			}
+			int lineHeight = 0;
+			if (lineIndex >= 0 && lineIndex < lineCount) {


could be lineExists(lineIndex)?

Thanks a lot. Sorry my fault, I was searching for all occurrences of "lineCount" but somehow missed this location. Code updated with [99ae962].

mickaelistria · 2025-07-29T16:36:37Z

Thanks, let's merge it now!

mickaelistria · 2025-07-29T16:49:33Z

@tobiasmelcher Can you please squash those commits as a single one and push --force on your branch to update the PR?

height - Updated productive code to prevent IllegalArgumentException when replacing text in a StyledText control with variable line height and no focus, by adding boundary checks. - Added a regression test to validate the fix: ensures no exception is thrown when replacing the full text range under these conditions. Fixes: eclipse-platform#2302

tobiasmelcher · 2025-07-30T11:25:49Z

test error seems not to be related to this change

Error: Tests run: 388, Failures: 1, Errors: 0, Skipped: 6, Time elapsed: 105.7 s <<< FAILURE! -- in
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser
Error: org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser.test_setUrl_remote[browser flags: 0] -- Time elapsed: >> 37.11 s <<< FAILURE!

mickaelistria · 2025-07-30T12:02:47Z

Thank you!

BeckerWdf · 2025-08-08T09:28:42Z

Thanks @tobiasmelcher for fixing and thanks @mickaelistria for reviewing and merging.

tobiasmelcher requested review from BeckerWdf and HeikoKlare July 16, 2025 11:21

tobiasmelcher force-pushed the styled_text_index_out_of_bounds branch from 747348b to 4b1f261 Compare July 16, 2025 15:26

BeckerWdf reviewed Jul 17, 2025

View reviewed changes

mickaelistria reviewed Jul 21, 2025

View reviewed changes

laeubi mentioned this pull request Jul 23, 2025

The code mining does not reset line vertical indentation when editor auto inserts multiple lines of code eclipse-platform/eclipse.platform.ui#3121

Closed

2 tasks

tobiasmelcher force-pushed the styled_text_index_out_of_bounds branch from 9ff6acd to 8fc5cf8 Compare July 28, 2025 08:45

mickaelistria reviewed Jul 28, 2025

View reviewed changes

mickaelistria reviewed Jul 29, 2025

View reviewed changes

tobiasmelcher force-pushed the styled_text_index_out_of_bounds branch from 99ae962 to 49fa012 Compare July 30, 2025 11:01

mickaelistria merged commit c0dbf00 into eclipse-platform:master Jul 30, 2025
15 of 17 checks passed

BeckerWdf assigned tobiasmelcher Aug 8, 2025

Fix replaceTextRange logic for unfocused StyledText with variable line height #2318

Fix replaceTextRange logic for unfocused StyledText with variable line height #2318

Uh oh!

Conversation

tobiasmelcher commented Jul 16, 2025

Uh oh!

tobiasmelcher commented Jul 16, 2025

Uh oh!

github-actions bot commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

BeckerWdf left a comment

Choose a reason for hiding this comment

Uh oh!

BeckerWdf commented Jul 21, 2025

Uh oh!

mickaelistria left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeikoKlare commented Jul 21, 2025

Uh oh!

tobiasmelcher commented Jul 28, 2025

Uh oh!

mickaelistria left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mickaelistria commented Jul 28, 2025

Uh oh!

tobiasmelcher commented Jul 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mickaelistria commented Jul 29, 2025

Uh oh!

mickaelistria commented Jul 29, 2025

Uh oh!

tobiasmelcher commented Jul 30, 2025

Uh oh!

Uh oh!

mickaelistria commented Jul 30, 2025

Uh oh!

BeckerWdf commented Aug 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

github-actions bot commented Jul 16, 2025 •

edited

Loading