RFR: 8357393: RichTextArea: fails to properly save text attributes [v2]

[Kevin Rushforth]

On Thu, 29 May 2025 18:42:10 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Fixing a bug that breaks proper serialization of character attributes.
>> 
>> This, unfortunately, makes a breaking change in the RichTextArea native format [0].
>> 
>> ## References
>> 
>> [0] https://github.com/andy-goryachev-oracle/Test/blob/main/doc/RichTextArea/RichTextArea_DataFormat_V2.md
>
> Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'origin/master' into 8357393.attr.ser
>  - javadoc
>  - Merge remote-tracking branch 'origin/master' into 8357393.attr.ser
>  - test
>  - tests
>  - fixed attribute serialization

The fix looks reasonable to me. I left a suggestion about whether we really need to allow null, but that's preexisting and could be considered in a follow-up.

Another thing that needs to be done in a follow-up is that you will need to add versioning to the internal rich text format. While this is still an incubating API we can break backward compatibility, but at some point, we will need a format that can evolve compatibility with newer runtimes able to read older files.

@Ziad-Mid Can you be the second reviewer?

modules/jfx.incubator.richtext/src/main/java/com/sun/jfx/incubator/scene/control/richtext/StyleAttributeMapHelper.java line 57:

> 55:      *
> 56:      * @param ss the style attribute map
> 57:      * @return the instance of StyleAttributeMap, or null

Is there a good reason to allow null here? Unless there is a semantic difference between null and the empty map, it might be easier to make this non-nullable. This could be done later, since the fact that it can return null is preexisting.

modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyleAttributeMap.java line 431:

> 429:             @Override
> 430:             public StyleAttributeMap filterAttributes(StyleAttributeMap ss, boolean isParagraph) {
> 431:                 return (ss == null ? null : ss.filterAttributes(isParagraph));

You could skip the null check if you disallowed a null map.

-------------

Marked as reviewed by kcr (Lead).

PR Review: https://git.openjdk.org/jfx/pull/1813#pullrequestreview-2902448757
PR Comment: https://git.openjdk.org/jfx/pull/1813#issuecomment-2946418313
PR Review Comment: https://git.openjdk.org/jfx/pull/1813#discussion_r2130475589
PR Review Comment: https://git.openjdk.org/jfx/pull/1813#discussion_r2130460632