On Fri, 31 Oct 2025 18:48:57 GMT, Andy Goryachev <[email protected]> wrote:
>> 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), and as a property in the
>> `CodeArea`.
>>
>> NOTE:
>> - some dependency on #1938 , resolved.
>
> Andy Goryachev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> get text
I looked over the proposed API and left some inline comments. I'll repeat my
two main points here:
1. Rather than a Nullable enum, create an explicit enum value for "use the
`line.separator` property"
2. Why is the new lineEnding property on the `CodeArea` subclass and not on
`RichTextArea`?
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/CodeArea.java
line 466:
> 464: * @return the line ending property
> 465: * @since 26
> 466: * @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.
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/CodeArea.java
line 468:
> 466: * @defaultValue null
> 467: */
> 468: public final ObjectProperty<LineEnding> lineEndingProperty() {
Shouldn't this be a property of RichTextArea?
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/LineEnding.java
line 33:
> 31: * @since 26
> 32: */
> 33: 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?
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/LineEnding.java
line 35:
> 33: public enum LineEnding {
> 34: /** Classic Mac OS */
> 35: 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?
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/LineEnding.java
line 39:
> 37: CRLF,
> 38: /** macOS/Unix */
> 39: LF
Since this shows up in the javadoc-generated API docs, it might be worth
expanding this a bit.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1944#pullrequestreview-3418252114
PR Review Comment: https://git.openjdk.org/jfx/pull/1944#discussion_r2491791031
PR Review Comment: https://git.openjdk.org/jfx/pull/1944#discussion_r2491987923
PR Review Comment: https://git.openjdk.org/jfx/pull/1944#discussion_r2491756972
PR Review Comment: https://git.openjdk.org/jfx/pull/1944#discussion_r2491766155
PR Review Comment: https://git.openjdk.org/jfx/pull/1944#discussion_r2491767288
