Corrected TextLayout to produce right number of lines when '\r\n' sequence comes. #1320

deepika-u · 2024-07-03T13:01:20Z

Corrected TextLayout to produce right number of lines when '\r\n' sequence comes.

Fixes #184

@sravanlakkimsetti
@niraj-modi
@merks
Can one of you review this fix please.

deepika-u · 2024-07-03T13:03:37Z

Anyone testing this pr can use the below snippet.
simple test case is with text1.
complex test case is with text2.

package org.eclipse.swt.bugs4;

/*
 * TextLayout example snippet: text with underline and strike through
 *
 * For a list of all SWT example snippets see
 * http://www.eclipse.org/swt/snippets/
 *
 * @since 3.1
 */
import org.eclipse.swt.*;
import org.eclipse.swt.graphics.*;
import org.eclipse.swt.widgets.*;

public class Issue184 {

	private static FontData[] getFontData(Font font, int style) {
		FontData[] fontDatas = font.getFontData();
		for (FontData fontData : fontDatas) {
			fontData.setStyle(style);
		}
		return fontDatas;
	}

	public static void main(String[] args) {
		Display display = new Display();
		final Shell shell = new Shell(display, SWT.SHELL_TRIM | SWT.DOUBLE_BUFFERED);
		shell.setText("Underline, Strike Out");
		//String text = "Here is 1.\r\n\r\nsome text that is 2.\r\nunderlined or 3.\rstruck out or 4.\nboth.";
		String text1 = "0123456789\r\n\r\n0123456789\r\n0123456789\r0123456789\n0123456789";
		String text2 = "\r\n0123456789\r\n\r\n01\r\n456789\r\n0123456789\r0123456789\n0123456789";

		FontData[] fontData = getFontData(shell.getFont(), SWT.BOLD);
		Font font = new Font(shell.getDisplay(), fontData);

		FontData[] fontData1 = getFontData(shell.getFont(), SWT.ITALIC | SWT.BOLD);
		Font font1 = new Font(shell.getDisplay(), fontData1);

		FontData[] fontData2 = getFontData(shell.getFont(), SWT.BOLD);
		Font font2 = new Font(shell.getDisplay(), fontData2);

		final TextLayout layout = new TextLayout(display);
		//layout.setText(text1);
		layout.setText(text2);

		TextStyle style1 = new TextStyle(font, null, null);
		//layout.setStyle(style1, 0, 16);
		layout.setStyle(style1, 5, 16);

		TextStyle style2 = new TextStyle(font1, null, null);
		layout.setStyle(style2, 21, 24);

		TextStyle style3 = new TextStyle(font2, null, null);
		layout.setStyle(style3, 27, 31);
		//layout.setStyle(style3, 25, 29);

		int[] offsets = layout.getLineOffsets();
		System.out.println(offsets.length);
		shell.addListener(SWT.Paint, event -> {
			Point point = new Point(10, 10);
			int width = shell.getClientArea().width - 2 * point.x;
			layout.setWidth(width);
			layout.draw(event.gc, point.x, point.y);
		});
		shell.setSize(400, 300);
		shell.open();
		while (!shell.isDisposed()) {
			if (!display.readAndDispatch())
				display.sleep();
		}
		layout.dispose();
		display.dispose();
	}
}

github-actions · 2024-07-03T13:09:25Z

Test Results

383 files ±0 383 suites ±0 5m 3s ⏱️ -17s
4 287 tests +1 4 272 ✅ +1 15 💤 ±0 0 ❌ ±0
12 153 runs +3 12 067 ✅ +3 86 💤 ±0 0 ❌ ±0

Results for commit a3510b6. ± Comparison against base commit 4bf1af8.

♻️ This comment has been updated with latest results.

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/TextLayout.java

laeubi · 2024-07-04T05:45:24Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/TextLayout.java

@@ -2952,8 +2952,11 @@ StyleItem[] merge (long items, int itemCount) {
 			linkBefore = false;
 		}
 		char ch = segmentsText.charAt(start);
+		if(ch == '\r') {
+			ch = segmentsText.charAt(start+1);


is a boundcheck required here maybe? Just assume a single \r at end of line would read bejond the end

i am checking on further if the string in full is just a "\r", what needs to be done.

If you agree to my changes, i can squash the commits.

deepika-u · 2024-07-10T13:43:56Z

@niraj-modi @merks
Can you also please review this?

@laeubi : Do you have any additional comments on this fix? or are we good to merge this?

merks

I don't have time for a more detailed for the next days. But some comments generally follow a for or if keyword with a space.

merks · 2024-07-10T14:38:59Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/TextLayout.java

-	String text, segmentsText;
-	int lineSpacingInPoints;
+	String text, segmentsText, originalText;
+	int lineSpacingInPoints, previousCountNextLine = 0, previousEnd = -1;


Initializing to 0 is pointless and actually generates more byte code.

Updated initialization as part of constructor.

merks · 2024-07-10T14:42:44Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/TextLayout.java

-			case '\t':
-				item.tab = true;
-				break;
+		if(ch == '\r' && (start+1 < end)) {


The formatting of if( is poor. The () around the < condition is redundant and odd that it's not used for the == test

corrected it.

merks · 2024-07-10T14:43:23Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/TextLayout.java

+		loop = previousEnd+1;
+	else
+		loop = previousEnd;
+	for(int i = loop; i < end; i++) {


Here for( is not nice format.

corrected it.

elsazac · 2024-07-16T02:32:45Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/TextLayout.java

+	this.originalText = text;
+	String[] strings = text.split("\r");
+	text = "";
+	for(int i = 0; i < strings.length; i++)


The formatting of this for loop could also be improved.

Wittmaxi · 2024-07-16T07:54:47Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/TextLayout.java

+	this.originalText = text;
+	String[] strings = text.split("\r");
+	text = "";
+	for (int i = 0; i < strings.length; i++)


This can be done using a map reduction. Anyway, please add braces to the for loop

reference

It's this a complicated and poorly performance way of doing test.replace("\r", "")?

@merks yes, looks like it. Your approach is the best to me!

let me test and update here back.

Thanks @merks for your feedback, after updating this i found a better way of fixing the issue. So new change is updated, you may please review now.

merks · 2024-07-16T08:06:09Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/TextLayout.java

+	else
+		loop = previousEnd;
+	for (int i = loop; i < end; i++) {
+		if (originalText.charAt(i) == '\r' && originalText.charAt(i+1) =='\n')


Is i + 1 necessarily < end?

let me check it.

you are correct, when string ends with \r, it is resulting OOB exception. So corrected it.

deepika-u · 2024-08-06T06:47:05Z

@niraj-modi
Can you also please review this? so that it can be merged before M3.

@merks @laeubi @Wittmaxi : Do you have any additional comments on this fix? or are we good to merge this?

deepika-u · 2024-08-09T11:07:10Z

@merks @laeubi @Wittmaxi
Can one of you review the changes please.

merks · 2024-08-09T11:23:04Z

@deepika-u

Could you please squash this to a single commit?

deepika-u · 2024-08-12T08:57:29Z

@deepika-u

Could you please squash this to a single commit?

Done.

merks · 2024-09-04T09:24:20Z

Wouldn't it be better for there to be actual tests that run with every build?

deepika-u · 2024-09-04T11:54:45Z

Wouldn't it be better for there to be actual tests that run with every build?

Apart from this manual test(tests/org.eclipse.swt.tests/ManualTests/org/eclipse/swt/tests/manual/Issue184_CarriageReturnHandled.java) which is already added, please let me know what else needs to be done, so that this can be taken forward. Can you point me to a sample kind of instance? to refer.

deepika-u · 2024-10-07T11:27:04Z

@merks : I have added some junits for the fix. Can you check now please?

BeckerWdf · 2024-10-07T13:57:18Z

is this only an issue on Windows? May the macOS and Linux implementation have the same issue?

deepika-u · 2024-10-08T04:54:04Z

is this only an issue on Windows? May the macOS and Linux implementation have the same issue?

@BeckerWdf : Atleast from the parent #184 => this is marked as only windows issue and i have implemented the fix only for windows. In the past i remember vaguely but when tested with mac, the behavior on mac Vs behavior on windows (before the fix) were different. So took it up as windows specific issue.

BeckerWdf · 2024-10-08T09:29:22Z

is this only an issue on Windows? May the macOS and Linux implementation have the same issue?

@BeckerWdf : Atleast from the parent #184 => this is marked as only windows issue and i have implemented the fix only for windows. In the past i remember vaguely but when tested with mac, the behavior on mac Vs behavior on windows (before the fix) were different. So took it up as windows specific issue.

Thanks.

deepika-u · 2024-10-14T05:31:42Z

@merks :
I have added some junits for this fix. Can you make some time to move this forward? I know you are super busy these days but yeah please need your inputs on this.

deepika-u · 2024-10-21T05:10:28Z

@merks
We have crossed the M2 milestone for 4.34 and entered into M3 already. Can you find some time to check on this please so that atleast for this release, its available for customers.

merks

It looks okay to me.

That being said, I would prefer that at least one other person deeply involved in SWT reviews this too.

merks · 2024-10-21T13:48:20Z

@HeikoKlare

Do you know who else might be good to review this?

deepika-u · 2024-10-24T14:12:17Z

@sratz @jukzi @laeubi @HeikoKlare
Can one of you review this please.
@merks thanks alot for your time.
But he feels if anyone else also reviews it would be great. - Thanks alot in advance, hoping this would go in 4.34.

HeikoKlare

I have not worked on TextLayout so far. Thus, unfortunately, I cannot say with high confidence whether the overall change is okay. Still, from what I understand, the conceptual change and the code for it looks sound to me. I've tested the proposed change with the snippet in the original issue and it indeed fixes it.

I have remarks regarding the automated and manual test. In particular the automated test does not seem to be an adequate regression test, as the same test succeeds on master, thus it does not seem to cover the addressed issue.

HeikoKlare · 2024-10-25T10:17:15Z

...lipse.swt.tests/ManualTests/org/eclipse/swt/tests/manual/Issue184_CarriageReturnHandled.java

@@ -0,0 +1,68 @@
+package org.eclipse.swt.tests.manual;


Please add missing copyright header

HeikoKlare · 2024-10-25T10:17:56Z

...lipse.swt.tests/ManualTests/org/eclipse/swt/tests/manual/Issue184_CarriageReturnHandled.java

+		TextStyle style3 = new TextStyle(font2, null, null);
+		layout.setStyle(style3, 21, 24); // iri in bold
+
+		int[] offsets = layout.getLineOffsets();


This variable is unused and thus produces a warning. It should be safe to just delete this line of code.

HeikoKlare · 2024-10-25T10:19:44Z

....tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_TextLayout.java

@@ -547,6 +547,60 @@ public void test_setStyle() {
 	assertNull(layout.getStyle(4));
 	assertNull(layout.getStyle(5));
 	layout.dispose();
+
+	layout = new TextLayout (display);


The test with these changes succeeds on master, so it's not a regression test for the changes made in this PR. Can you adapt the test so that it serves as a regression test for the addressed issue?

Here we have not changed the behavior of the api's. The original behavior of the api's still work the same. We are internally detecting, correcting and passing the corrected content in specific cases only. We dint even add new api's. It is only the look and feel by which you can make out if some extra lines are expected or not expected varies. - is my understanding.

Okay, I mistakenly expected the added test to be a regression test for the fix because of #1320 (comment)

Still, this change is no refactoring but affects the behavior of public APIs (e.g., TextLayout#draw(...) behaves differently). I guess that, e.g., also TextLayout#getBounds() now behaves differently, so wouldn't it be possible to have a regression test comparing the bounds of two configurations that should actually result in the bounds but did not do before this change (or vice versa)?

@HeikoKlare
As per your suggestion i have added new junit tests with TextLayout#getLineCount() and TextLayout#getBounds().
Since this change is only with respect to windows, i have excluded mac, gtk for the tests to run.

I guess this makes it all complete as we discussed. If i missed out any, please do let me know.

Can you review it and take it forward please?

Please hold on, let me cross check and update you again.

Hi @HeikoKlare
Sorry for the confusion caused.
I am successfull in adding only TextLayout#getLineCount() junit. I think we can go ahead with this pr now.

I have tried even TextLayout#getBounds() but had to withdraw as the behavior is not consistent across.
With the sample input taken,

On master, getBounds() is returning {0, 0, 30, 220} where width and height are 30, 220 respectively. [junit is failing]

On the branch where i committed my changes(fix_for_issue184), getBounds() is returning {0, 0, 43, 140}. [junit is passing]

To cross check, i have pulled the pr 1320 to local, getBounds() is returning {0, 0, 43, 140}. [junit is passing]

But on the build machine, getBounds() is returning {0, 0, 34, 105}. [junit is failing because it is getting 34 but expecting 43]. -> https://github.com/eclipse-platform/eclipse.platform.swt/actions/runs/12312890737/job/34365830603?pr=1320
Neither able to understand the reason for the failure(to fix) nor able to replicate the problem locally to debug further.

Can you please check now?

HeikoKlare · 2024-10-25T10:26:36Z

And one additional remark: since the TextLayout is very essential and we do not seem to have completely confident reviews so far, I am not sure whether we should bring this kind of change in M3. Maybe it makes more sense to bring in the next M1 to have better implicit testing until it is finally released? The issue has been there for quite a long time without higher complains that I am not sure whether the gain outweighs the risk of regression.

deepika-u · 2024-10-25T12:17:26Z

And one additional remark: since the TextLayout is very essential and we do not seem to have completely confident reviews so far, I am not sure whether we should bring this kind of change in M3. Maybe it makes more sense to bring in the next M1 to have better implicit testing until it is finally released? The issue has been there for quite a long time without higher complains that I am not sure whether the gain outweighs the risk of regression.

Thanks for your opinion. Since it is already tested now by various people, i feel we are safe to merge this. Considering the M3 deadline is 8th Nov(2 weeks far and not away like 2days or so) and we can give it a try. If something is breaking we'll revert it immediately and rectify to make it to next M1? Atleast this risk we can take to improve SWT - i feel so in particularly such long standing issues when they passed all the stages of fixing, review, successfully verified.
This is a windows specific change and not on common code in particluar.

sequence comes. Added some junit tests as well. Updated getStyle() and setStyle() accordingly. Added failing and success junits again. Fixes eclipse-platform#184

deepika-u requested a review from niraj-modi as a code owner July 3, 2024 13:01

elsazac reviewed Jul 4, 2024

View reviewed changes

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/TextLayout.java Outdated Show resolved Hide resolved

laeubi reviewed Jul 4, 2024

View reviewed changes

deepika-u force-pushed the fix_for_issue184 branch 2 times, most recently from 89bd04a to aae8e16 Compare July 9, 2024 04:48

merks reviewed Jul 10, 2024

View reviewed changes

deepika-u force-pushed the fix_for_issue184 branch 3 times, most recently from 0740eef to 26a2d76 Compare July 15, 2024 07:57

elsazac reviewed Jul 16, 2024

View reviewed changes

deepika-u force-pushed the fix_for_issue184 branch 2 times, most recently from e60de1d to 223b000 Compare July 16, 2024 06:15

Wittmaxi reviewed Jul 16, 2024

View reviewed changes

merks reviewed Jul 16, 2024

View reviewed changes

deepika-u force-pushed the fix_for_issue184 branch from 223b000 to 4f760cf Compare July 26, 2024 14:23

deepika-u force-pushed the fix_for_issue184 branch 2 times, most recently from 79bb600 to 52e92e2 Compare August 6, 2024 06:44

deepika-u force-pushed the fix_for_issue184 branch 3 times, most recently from 2e2b765 to 4d7e7e8 Compare August 9, 2024 06:18

deepika-u force-pushed the fix_for_issue184 branch from 4d7e7e8 to 48b7ea7 Compare August 9, 2024 11:10

deepika-u force-pushed the fix_for_issue184 branch from 48b7ea7 to e500d29 Compare August 12, 2024 08:54

deepika-u force-pushed the fix_for_issue184 branch from e500d29 to d53e86e Compare August 13, 2024 14:15

deepika-u force-pushed the fix_for_issue184 branch from c378aa1 to b5dc868 Compare September 4, 2024 09:14

deepika-u force-pushed the fix_for_issue184 branch 2 times, most recently from 8b4764a to 63a46d7 Compare October 7, 2024 11:16

deepika-u force-pushed the fix_for_issue184 branch from 63a46d7 to 048b496 Compare October 14, 2024 05:27

deepika-u force-pushed the fix_for_issue184 branch from 048b496 to e05f6fc Compare October 21, 2024 05:06

merks approved these changes Oct 21, 2024

View reviewed changes

HeikoKlare reviewed Oct 25, 2024

View reviewed changes

deepika-u force-pushed the fix_for_issue184 branch 2 times, most recently from c368f7f to 514957c Compare October 25, 2024 11:43

deepika-u force-pushed the fix_for_issue184 branch 7 times, most recently from 63bcef0 to 1756000 Compare December 13, 2024 10:43

Corrected TextLayout to produce right number of lines when '\r\n'

a3510b6

sequence comes. Added some junit tests as well. Updated getStyle() and setStyle() accordingly. Added failing and success junits again. Fixes eclipse-platform#184

deepika-u force-pushed the fix_for_issue184 branch from 1756000 to a3510b6 Compare December 13, 2024 10:43

Corrected TextLayout to produce right number of lines when '\r\n' sequence comes. #1320

Are you sure you want to change the base?

Corrected TextLayout to produce right number of lines when '\r\n' sequence comes. #1320

Conversation

deepika-u commented Jul 3, 2024

deepika-u commented Jul 3, 2024

github-actions bot commented Jul 3, 2024 • edited Loading

Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepika-u commented Jul 10, 2024

merks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepika-u Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepika-u commented Aug 6, 2024

deepika-u commented Aug 9, 2024

merks commented Aug 9, 2024

deepika-u commented Aug 12, 2024 • edited Loading

merks commented Sep 4, 2024

deepika-u commented Sep 4, 2024

deepika-u commented Oct 7, 2024

BeckerWdf commented Oct 7, 2024

deepika-u commented Oct 8, 2024

BeckerWdf commented Oct 8, 2024

deepika-u commented Oct 14, 2024 • edited Loading

deepika-u commented Oct 21, 2024

merks left a comment

Choose a reason for hiding this comment

merks commented Oct 21, 2024 • edited Loading

deepika-u commented Oct 24, 2024

HeikoKlare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepika-u Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

HeikoKlare commented Oct 25, 2024

deepika-u commented Oct 25, 2024 • edited Loading

github-actions bot commented Jul 3, 2024 •

edited

Loading

deepika-u Jul 18, 2024 •

edited

Loading

deepika-u commented Aug 12, 2024 •

edited

Loading

deepika-u commented Oct 14, 2024 •

edited

Loading

merks commented Oct 21, 2024 •

edited

Loading

deepika-u Dec 13, 2024 •

edited

Loading

deepika-u commented Oct 25, 2024 •

edited

Loading