Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display.withCrLf() is producing wrong line breaks on Windows #1708

Merged

Conversation

Christopher-Hermann
Copy link
Contributor

Replacing all line breaks on windows with CrLf using regex, which is much safer.

Fixes #1557

@Christopher-Hermann
Copy link
Contributor Author

I developed the fix on MacOS, so I had no change to test the fix. Would be nice if someone on windows can retest the fix.

Copy link
Contributor

github-actions bot commented Jan 7, 2025

Test Results

   486 files  ±0     486 suites  ±0   9m 49s ⏱️ -2s
 4 308 tests +1   4 295 ✅ +1   13 💤 ±0  0 ❌ ±0 
16 413 runs  +1  16 305 ✅ +1  108 💤 ±0  0 ❌ ±0 

Results for commit fcaa4c2. ± Comparison against base commit d7febc4.

♻️ This comment has been updated with latest results.

@Christopher-Hermann Christopher-Hermann force-pushed the FixLineBreaks branch 3 times, most recently from acfa734 to 889de2c Compare January 8, 2025 09:51
@deepika-u
Copy link
Contributor

@Christopher-Hermann
I tested this pr with below test case so that it covers all the cases from your fix(regex).

	@Test
	public void test_mixedLfAndCrfl() {
		//Use text control for testing since Display.withCrLf() is package private
		Text text = new Text(shell, SWT.None);

		//text.setText("First Line \n second line \r\n third line");
                //text.setText("\r First \r\r Line \n second \n\n line \r\n third \r\n\r\n line \r");
		text.setText("\n First \r\r Line \n second \n\n line \r\n third \r\n\r\n line \n");
		assertEquals("\r\n First \r\n\r\n Line \r\n second \r\n\r\n line \r\n third \r\n\r\n line \r\n", text.getText());
	}

Your fix works like charm.
It is clean, simple and straight forward. A go for the fix from my side.

Tested on below environment:
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

Replacing all line breaks on windows with CrLf using regex, which is much safer.

Fixes eclipse-platform#1557
@DenisUngemach
Copy link

I have tested the PR on a Text Control on windows, it works.

One thing i want to mention. In this change a single \r will be replaced with a \r\n.
So we have this in the Text widget:

Before:
\r was shown as space
With PR:
\r is replaced with \r\n and shown as new line.

This is a change in the behavior.

Here are the uses:
\n (Line Feed):
on Unix systems
\r\n (Carriage Return + Line Feed):
on Windows-systems
\r (Carriage Return):
was used as new line on old macOS-versions (before OS X)

Please decide which behavior you wish. In my opinion both is fine and this is not that relevant.

@Christopher-Hermann
Copy link
Contributor Author

Christopher-Hermann commented Jan 9, 2025

Before: \r was shown as space With PR: \r is replaced with \r\n and shown as new line.

Good point. Since \r was used for new lines in older versions of macOS, I believe the new behavior is correct. It's likely that the old behavior was incorrect anyway. However, changing the behavior is of course never a good idea.

Any other opinions on that?

@BeckerWdf
Copy link
Contributor

in older versions of macOS

how old are these "older versions" of macOS?

@BeckerWdf
Copy link
Contributor

before OS X

Ah I see: "before OS X". This long time ago. I think we can merge this PR as it is.

@Christopher-Hermann Christopher-Hermann merged commit 950ca8a into eclipse-platform:master Jan 10, 2025
14 checks passed
@Christopher-Hermann Christopher-Hermann deleted the FixLineBreaks branch January 10, 2025 10:03
@jukzi
Copy link
Contributor

jukzi commented Jan 13, 2025

org.eclipse.swt.widgets.Display.withCrLf("\n ".repeat(100_000_000)); seems to be twice as slow now (#1718), can you please fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display.withCrLf() is producing wrong line breaks on Windows
5 participants