8370140: RichTextArea: line endings #1944

andy-goryachev-oracle · 2025-10-22T20:35:36Z

Adds control of line endings (newline separators) in StyledTextModel, RichTextArea, and CodeArea.

The impacted areas are:

saving to plain text
copying to plain text
IME

This feature is implemented as a regular field in the StyledTextModel (since it is ultimately an attribute of the model), with convenience setter and getter in the RichTextArea.

NOTES

some dependency on 8368478: RichTextArea: add IME support #1938 , resolved.

Progress

Change must not contain extraneous whitespace
Change requires CSR request JDK-8370973 to be approved
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

JDK-8370140: RichTextArea: line endings (Enhancement - P4)
JDK-8370973: RichTextArea: line endings (CSR)

Reviewers

Jayathirth D V (@jayathirthrao - Committer) 🔄 Re-review required (review applies to 4a3e1e32)
Kevin Rushforth (@kevinrushforth - Reviewer) 🔄 Re-review required (review applies to 4a3e1e32)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1944/head:pull/1944
$ git checkout pull/1944

Update a local copy of the PR:
$ git checkout pull/1944
$ git pull https://git.openjdk.org/jfx.git pull/1944/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1944

View PR using the GUI difftool:
$ git pr show -t 1944

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1944.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-10-22T20:36:12Z

👋 Welcome back angorya! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-10-22T20:37:33Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

andy-goryachev-oracle · 2025-10-22T21:44:00Z

/reviewers 2
/csr

openjdk · 2025-10-22T21:45:27Z

@andy-goryachev-oracle
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

openjdk · 2025-10-22T21:46:00Z

@andy-goryachev-oracle has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@andy-goryachev-oracle please create a CSR request for issue JDK-8370140 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

mlbridge · 2025-10-22T21:49:03Z

Webrevs

kevinrushforth · 2025-10-29T16:58:05Z

Reviewers: @kevinrushforth @jayathirthrao

jayathirthrao

Code change looks good to me. I have minor suggestions.
Also played around with demos everything works fine.

jayathirthrao · 2025-11-04T10:59:19Z

...xt/src/main/java/com/sun/jfx/incubator/scene/control/richtext/StringBuilderStyledOutput.java

-    public void setLineSeparator(String s) {
-        newline = s;
+    public StringBuilderStyledOutput(LineEnding lineEnding) {
+        sb = new StringBuilder(1024);


Its better to name this constant.

jayathirthrao · 2025-11-04T11:00:44Z

...ator.richtext/src/main/java/com/sun/jfx/incubator/scene/control/richtext/util/RichUtils.java

+            return System.getProperty("line.separator");
+        }
+        return switch(v) {
+        case CR -> "\r";


Missing indentation.

kevinrushforth

I looked over the proposed API and left some inline comments. I'll repeat my two main points here:

Rather than a Nullable enum, create an explicit enum value for "use the line.separator property"
Why is the new lineEnding property on the CodeArea subclass and not on RichTextArea?

kevinrushforth · 2025-11-04T19:06:21Z

...es/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/LineEnding.java

+ *
+ * @since 26
+ */
+public enum LineEnding {


Please add an explicit enum value that means "use the value of System.getProperty("line.separator")". Relying on null to mean this is not clean for two reasons: 1) it is more difficult to document (case in point, it isn't documented here); and 2) using an enum that might be null in a switch statement without first checking for null will cause an NPE.

Possible names: NATIVE? LINE_SEPARATOR? something else?

Another possible name: DEFAULT

I'm not sure which one I like best.

PLATFORM_DEFAULT maybe?

Maybe. Or maybe SYSTEM_DEFAULT (since it comes from Java's system properties)?

kevinrushforth · 2025-11-04T19:09:54Z

...es/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/LineEnding.java

+    /** Classic Mac OS */
+    CR,


I question the need for this enum value. CR was only used prior to macOS 10.0, which is so old as to be uninteresting without a good reason (for comparison, the very first version of Java for macOS was for macOS 10.5). Do you have a use case in mind?

Yes, the use case is for application developers to have full control of the line endings. This might be useful for programming editors that deal with legacy formats.

kevinrushforth · 2025-11-04T19:10:20Z

...es/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/LineEnding.java

+    /** Windows */
+    CRLF,
+    /** macOS/Unix */
+    LF


Since this shows up in the javadoc-generated API docs, it might be worth expanding this a bit.

kevinrushforth · 2025-11-04T19:19:44Z

modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/CodeArea.java

+     *
+     * @return the line ending property
+     * @since 26
+     * @defaultValue null


I can't think of a good reason for this to be a Nullable property. Instead, define a new enum value that means "use the value of the line.separator system property". It seems much cleaner (I don't like relying on null for out of band enum values, since it will cause an NPE if you switch on it without checking for null first).

Separately, should "use line.separator" should be the default value or should it use "\n" instead. Probably using "line.separator" is a better choice, which is what this PR does. The reason I bring this up is so that we make a conscious decision.

kevinrushforth · 2025-11-04T20:40:36Z

modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/CodeArea.java

+     * @since 26
+     * @defaultValue null
+     */
+    public final ObjectProperty<LineEnding> lineEndingProperty() {


Shouldn't this be a property of RichTextArea?

No, RichTextArea might have a model that has no concept of line ending, or has its own idea of line ending - that's why it's an attribute of the model rather than the property in the control.

CodeArea, on the other hand, deals with plain text as an underlying data, so line ending is a property of the control, just like font and tabSize.

Unlike font and tabSize, the lineEnding property is used when exporting to an external format where you want line endings, which also applies to RichTextArea. This is done implicitly by various operations (copy/paste/export) or explicitly when calling getText. I realize that getText is not currently a public method in RichTextArea, but I understand is likely to be proposed in the future. Once you add that method to the public API it will seem odd that users of CodeArea set the property in the control, but users of RichTextArea itself would need to set it in the model.

Do you still think it belongs in the CodeArea subclass? I suppose it can be moved to the superclass later, if needed (without breaking anything), but I at least want you to think through all the issues.

Actually, I thought of a bigger concern. Having a writable attribute in both the control and the model is not going to give coherent results in the case where you have more than one view on the same model. So you might need to rethink the idea of having a writable property on the view (the control) while also having state (via a writable attribute) on the model.

Good point!
What do you think we should do then?

I should probably move the setter and getter to the RichTextArea class then, and remove the property altogether.

Any custom UI can always create its own property for line ending and deal with the model directly.

Making the property in the model be the primary and having the RTA setter/getter forward to the model makes it a lot like setUndoRedoEnable, which seems like a good choice. The state is only in one place (the model) with convenience methods in the control.

kevinrushforth · 2025-11-05T18:38:01Z

modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/CodeArea.java

+     * @since 26
+     * @defaultValue null
+     */
+    public final ObjectProperty<LineEnding> lineEndingProperty() {


Unlike font and tabSize, the lineEnding property is used when exporting to an external format where you want line endings, which also applies to RichTextArea. This is done implicitly by various operations (copy/paste/export) or explicitly when calling getText. I realize that getText is not currently a public method in RichTextArea, but I understand is likely to be proposed in the future. Once you add that method to the public API it will seem odd that users of CodeArea set the property in the control, but users of RichTextArea itself would need to set it in the model.

Do you still think it belongs in the CodeArea subclass? I suppose it can be moved to the superclass later, if needed (without breaking anything), but I at least want you to think through all the issues.

kevinrushforth · 2025-11-05T18:39:47Z

...es/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/LineEnding.java

+    /** macOS/Unix line ending, sequence of CR/LF (0x0d 0x0a)*/
+    LF,
+    /** Platform default line ending, using the value of {@code System.getProperty("line.separator")} */
+    SYSTEM_DEFAULT;


or maybe just SYSTEM? Now that I look at it, I'm not sure we need the "DEFAULT" suffix (nor the word "default" in the description).

kevinrushforth · 2025-11-05T18:43:27Z

...es/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/LineEnding.java

+ * @since 26
+ */
+public enum LineEnding {
+    /** Classic Mac OS line ending, ASCII CR (0x0d) */


I would say "Legacy" rather than "Classic", and maybe add a sentence that this was used on Mac systems prior to macOS 10.

kevinrushforth · 2025-11-05T18:44:14Z

...es/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/LineEnding.java

+    /** Windows line ending, ASCII LF (0x0a) */
+    CRLF,


Can you add a . at the end of the sentence (here and for all the enums)?

kevinrushforth · 2025-11-05T18:57:11Z

...ncubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledOutput.java

     */
    public static StyledOutput forPlainText() {
-        return new StringBuilderStyledOutput();
+        return new StringBuilderStyledOutput(null);


Null is not allowed. Pass LineEnding.SYSTEM_DEFAULT.

kevinrushforth · 2025-11-05T18:58:23Z

modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/CodeArea.java

+                @Override
+                protected void invalidated() {
+                    LineEnding v = get();
+                    if(v == null) {


Minor: space after if

kevinrushforth · 2025-11-05T18:59:12Z

modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/CodeArea.java

+                protected void invalidated() {
+                    LineEnding v = get();
+                    if(v == null) {
+                        set(old);


You also need to unbind if bound.

kevinrushforth

Looks good with a couple fairly minor comments inline.

kevinrushforth · 2025-11-06T19:47:53Z

...es/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/LineEnding.java

+    /** Legacy Mac OS line ending, ASCII CR (0x0d). */
    CR,
-    /** Windows */
+    /** Windows line ending, ASCII LF (0x0a). */


The "ASCII LF (0x0a)" bit belongs to the LF enum value, not here.

kevinrushforth · 2025-11-06T19:48:54Z

...es/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/LineEnding.java

    CRLF,
-    /** macOS/Unix */
-    LF
+    /** macOS/Unix line ending, sequence of CR/LF (0x0d 0x0a). */


The The "CR/LF (0x0d 0x0a)" bit belongs to the CRLF enum value, not here. While you are at it, maybe add "ASCII" before "CR/LF" for consistency?

kevinrushforth · 2025-11-06T19:53:28Z

...bator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledTextModel.java

+        if (lineEnding == null) {
+            lineEnding = LineEnding.SYSTEM;
+        }


This isn't needed, right? lineEnding can never be null as long as you initialize it to LineEnding.SYSTEM , since you check and throw on an attempt to set it to null.

tried to do a lazy init, but it's not that essential.

kevinrushforth

Looks good. I left one minor comment, but will approve as is. Will reapprove if you decide to make changes.

kevinrushforth · 2025-11-06T21:09:43Z

...cubator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/support/RTUtil.java

+        ByteArrayOutputStream out = new ByteArrayOutputStream();
+        try {
+            try {


Minor: you don't need two levels of try here. Even better, you can use try with resources instead of adding finally clause.

line endings

f8f3d58

tests

fd279ec

andy-goryachev-oracle mentioned this pull request Oct 22, 2025

8369085: RichTextArea SELECT_PARAGRAPH to include line separator #1940

Closed

3 tasks

andy-goryachev-oracle marked this pull request as ready for review October 22, 2025 21:43

openjdk bot added the rfr Ready for review label Oct 22, 2025

openjdk bot added the csr Need approved CSR to integrate pull request label Oct 22, 2025

kevinrushforth self-requested a review October 23, 2025 16:01

Merge remote-tracking branch 'origin/master' into 8370140.line.separator

48b85fc

kevinrushforth mentioned this pull request Oct 29, 2025

8368478: RichTextArea: add IME support #1938

Closed

3 tasks

andy-goryachev-oracle added 4 commits October 30, 2025 12:45

Merge branch 'master' into 8370140.line.separator

2d7dbbb

since

7c3b107

Merge branch 'master' into 8370140.line.separator

b9a496e

get text

994e678

jayathirthrao approved these changes Nov 4, 2025

View reviewed changes

kevinrushforth reviewed Nov 4, 2025

View reviewed changes

andy-goryachev-oracle added 5 commits November 4, 2025 13:41

Merge branch 'master' into 8370140.line.separator

158e4b1

non-nullable

25373a9

fixed tests

5d8eaf0

Merge branch 'master' into 8370140.line.separator

0cf0ed6

code area test

a27a441

kevinrushforth reviewed Nov 5, 2025

View reviewed changes

andy-goryachev-oracle added 2 commits November 5, 2025 12:30

review comments

cfbc542

removed property

35a75cc

kevinrushforth reviewed Nov 6, 2025

View reviewed changes

andy-goryachev-oracle added 2 commits November 6, 2025 12:03

review comments

37864df

Merge remote-tracking branch 'origin/master' into 8370140.line.separator

285cf40

kevinrushforth approved these changes Nov 6, 2025

View reviewed changes

kevinrushforth requested a review from jayathirthrao November 6, 2025 21:19

andy-goryachev-oracle added 2 commits November 6, 2025 13:20

try

90c1110

rm property

4a3e1e3

kevinrushforth approved these changes Nov 6, 2025

View reviewed changes

jayathirthrao approved these changes Nov 7, 2025

View reviewed changes

Merge branch 'master' into 8370140.line.separator

4eb1546

andy-goryachev-oracle requested review from jayathirthrao and kevinrushforth November 7, 2025 17:02

8370140: RichTextArea: line endings #1944

Are you sure you want to change the base?

8370140: RichTextArea: line endings #1944

Uh oh!

Conversation

andy-goryachev-oracle commented Oct 22, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

NOTES

Progress

Issues

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Oct 22, 2025

Uh oh!

openjdk bot commented Oct 22, 2025

Uh oh!

andy-goryachev-oracle commented Oct 22, 2025

Uh oh!

openjdk bot commented Oct 22, 2025

Uh oh!

openjdk bot commented Oct 22, 2025

Uh oh!

mlbridge bot commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

kevinrushforth commented Oct 29, 2025

Uh oh!

jayathirthrao 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!

kevinrushforth 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!

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!

Choose a reason for hiding this comment

Uh oh!

kevinrushforth Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

andy-goryachev-oracle commented Oct 22, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 22, 2025 •

edited

Loading

kevinrushforth Nov 5, 2025 •

edited

Loading