8357393: RichTextArea: fails to properly save text attributes #1813
Conversation
|
👋 Welcome back angorya! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
andy-goryachev-oracle
changed the title
|
/csr |
|
@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-8357393 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
Webrevs
|
There was a problem hiding this comment.
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.
| @@ -428,7 +428,7 @@ private static void initAccessor() { | |||
| StyleAttributeMapHelper.setAccessor(new StyleAttributeMapHelper.Accessor() { | |||
| @Override | |||
| public StyleAttributeMap filterAttributes(StyleAttributeMap ss, boolean isParagraph) { | |||
| return ss.filterAttributes(isParagraph); | |||
| return (ss == null ? null : ss.filterAttributes(isParagraph)); | |||
There was a problem hiding this comment.
You could skip the null check if you disallowed a null map.
| * @param ss the style attribute map | ||
| * @return the instance | ||
| * @return the instance of StyleAttributeMap, or null |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I've recorded this in the feedback document - this might be worth doing.
|
@Ziad-Mid Can you be the second reviewer? /reviewers 2 |
|
@kevinrushforth |
You are absolutely right: we must provide a mechanism for versioning as well as properly document the format. The draft spec is but it is guaranteed to change to enable versioning (and addition of tab stop attributes in #1800) |
Fixing a bug that breaks proper saving 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
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1813/head:pull/1813$ git checkout pull/1813Update a local copy of the PR:
$ git checkout pull/1813$ git pull https://git.openjdk.org/jfx.git pull/1813/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1813View PR using the GUI difftool:
$ git pr show -t 1813Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1813.diff
Using Webrev
Link to Webrev Comment