On Thu, 9 Jul 2026 18:18:32 GMT, Kevin Rushforth <[email protected]> wrote:
>> Andy Goryachev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> demos
>
> modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/DataFormatHandler.java
> line 45:
>
>> 43: * the implementation if the format contains no styles on its own
>> (for example, in the plain text format case).
>> 44: *
>> 45: * @param input the input data, never null
>
> Now that you take an Object instead of a String, you need to define what
> subtypes are expected and what happens if an unexpected type is passed in.
> Presumably "IllegalArgumentException" or "ClassCastException", but maybe it
> is a no-op?
>
> Some documentation should be on this abstract method. If the answer depends
> on the subclass, then document this with an `@implSpec` section.
documented `ClassCastException`
> modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/EmbeddedImage.java
> line 91:
>
>> 89:
>> 90: /// Private constructor that DOES NOT make a defensive copy of the
>> bytes.
>> 91: private EmbeddedImage(
>
> Minor: We don't usually put javadoc comments on private methods of public
> classes. Suggestion: consider changing this to an ordinary inline comment
> (although I see that this pattern is used elsewhere, so this could be
> deferred).
Eclipse shows javadoc when I click on a name, so it is useful.
Javadoc tool ignores these comments.
> modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/EmbeddedImage.java
> line 224:
>
>> 222: */
>> 223: public EmbeddedImage copy(double targetWidth, double targetHeight,
>> boolean keepAspectRatio) {
>> 224: return new EmbeddedImage(bytes, width, height, targetWidth,
>> targetHeight, keepAspectRatio);
>
> Not an API issue, but this doesn't make a defensive copy. Is the `bytes`
> array ever modified? Or is it effectively deeply immutable?
The only public way of creating an `EmbeddedImage` instance is
`EmbeddedImage.of()` which does make a defensive copy.
Also, the byte array is never exposed to the public.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3554973155
PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3554981614
PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3554995495
