On Thu, 5 Sep 2024 16:53:23 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> changes per review
>
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 9043:
>
>> 9041: }
>> 9042:
>> 9043: return result;
>
> minor suggestion:
>
> List<CssMetaData<? extends Styleable, ?>> subMetadata =
> metadata.getSubProperties();
> if (subMetadata != null) {
> for (int i = 0, max = subMetadata.size(); i < max; ++i) {
> result = collectTransitionTimers(property, result);
> }
> }
> return result;
I usually like flatter code more than nested code, but I don't really mind one
way or the other in this particular situation.
> modules/javafx.graphics/src/main/java/javafx/scene/layout/BackgroundConverter.java
> line 109:
>
>> 107: image = img;
>> 108: } else {
>> 109: throw new
>> IllegalArgumentException("convertedValues");
>
> would it make sense to make this exception message more meaningful to help
> diagnose the issue? for example, what is expected and what is encountered.
>
> (this comment applies to every other converter)
I've reworded the message to include the unexpected type. Note that the
existing code will just throw `ClassCastException` in similar circumstances, so
we probably don't need to get super detailed for this particular situation. If
we want to have really detailed exception messages, the entire method should be
refactored.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1746024861
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1746022540
