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

[deepika-u]

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]

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[bot]]

Test Results

   486 files  ±0     486 suites  ±0   9m 49s ⏱️ +25s  4 307 tests +1   4 294 ✅ +1   13 💤 ±0  0 ❌ ±0  16 412 runs  +4  16 304 ✅ +4  108 💤 ±0  0 ❌ ±0 

Results for commit 55dc1f37. ± Comparison against base commit 8845e701.

:recycle: This comment has been updated with latest results.

[deepika-u]

@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?

[deepika-u]

@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]

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

[merks]

@deepika-u

Could you please squash this to a single commit?

[deepika-u]

@deepika-u

Could you please squash this to a single commit?

Done.

[deepika-u]

@merks : All review suggestions are implemented, can you take a look please? if it can be approved.

[merks]

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

[deepika-u]

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]

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

[BeckerWdf]

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

[deepika-u]

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]

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]

@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]

@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]

@HeikoKlare

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

[deepika-u]

@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]

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]

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.

[HeikoKlare]

This looks good now and we also have Ed's approval, thus merging.

[deepika-u]

Verified this issue on below build. Eclipse SDK Version: 2025-03 (4.35) Build id: I20250107-1800 OS: Windows 11, v.10.0, x86_64 / win32 Java vendor: Eclipse Adoptium Java runtime version: 23.0.1+11 Java version: 23.0.1

Number of lines: 7

[jukzi]

This PR introduced a regression https://github.com/eclipse-platform/eclipse.platform.swt/issues/1714, please fix. org.eclipse.swt.graphics.TextLayout.getRanges() tries to return only ranges with style!= null. TextLayoutgetStyle(range) for such a range should always return not null.