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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

wants to merge 1 commit into from

Conversation

Evgeni-Kramerov
Copy link

Fixes issue #1557

Changes functions so line breaks display correctly in Windows.

Fixes #1557

@Evgeni-Kramerov
Display.withCrLf() is producing wrong line breaks on Windows eclipse-…
a2045d4 
…platform#1557

Changes functions so line breaks display correctly in Windows.

Fixes eclipse-platform#1557
@BeckerWdf
Copy link
Contributor

can you please add the test-case provided in #1557

To Reproduce
See Test case

@test
public void testMixedLfAndCrflOnWindows() {
if (!SWT.getPlatform().equals("win32")) {
return;
}

  String act = Display.withCrLf("First Line \n second line \r\n third line");

  assertEquals("First Line \r\n second line \r\n third line", act);

}

@github-actions GitHub Actions
Copy link
Contributor

Test Results

   482 files   -  1     482 suites   - 1   8m 18s ⏱️ - 1m 37s
 4 060 tests  - 35   4 037 ✅  - 48   4 💤  - 3  3 ❌ ±0  16 🔥 +16 
16 138 runs   - 35  16 032 ✅  - 48  87 💤  - 3  3 ❌ ±0  16 🔥 +16 

For more details on these failures and errors, see this check.

Results for commit a2045d4. ± Comparison against base commit 435fe7e.

This pull request removes 35 tests. 
org.eclipse.swt.graphics.ImageWin32Tests ‑ testImageShouldHaveDimesionAsPerZoomLevel
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testByteArrayTransfer
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testFileTransfer
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testHtmlTransfer
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromCopiedImage
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImage
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImageData
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImageDataFromImage
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testRtfTransfer
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testTextTransfer
…

@Christopher-Hermann
Copy link
Contributor

There are failing test that are related to this change. Looks like some edge cases which results in out of bounds exception
String index out of range: -1
java.lang.StringIndexOutOfBoundsException: String index out of range: -1
at java.base/java.lang.StringLatin1.charAt(StringLatin1.java:48)
at java.base/java.lang.String.charAt(String.java:1517)
at org.eclipse.swt.widgets.Display.withCrLf(Display.java:5231)
at org.eclipse.swt.widgets.Text.setText(Text.java:2305)
at org.eclipse.swt.tests.junit.Test_org_eclipse_swt_custom_CTabFolder.createTabFolder(Test_org_eclipse_swt_custom_CTabFolder.java:140)

Christopher-Hermann
Christopher-Hermann reviewed Oct 29, 2024
View reviewed changes
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Display.java
}
}
return result.toString ();
return result.toString();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not took a detailed look in the original coding, but according to ChatGTP, we could just use a regex to normalize the String. See the coding:

/**
     * Normalize line breaks in a string to Windows line breaks (\r\n)
     *
     * @param input The input string
     * @return The string with normalized line breaks
     */
    public static String normalizeLineBreaks(String input) {
        // Replace \r\n, \r, or \n with \r\n
        return input.replaceAll("(\r\n|\r|\n)", "\r\n");
    }

This seems pretty easy and clean. Maybe you can give it a try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Christopher-Hermann Christopher-Hermann Christopher-Hermann left review comments

Assignees

@Evgeni-Kramerov Evgeni-Kramerov

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Display.withCrLf() is producing wrong line breaks on Windows
3 participants
@Evgeni-Kramerov @BeckerWdf @Christopher-Hermann