On Thu, 29 May 2025 18:42:10 GMT, Andy Goryachev <ango...@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
