On Mon, 1 Jun 2026 19:08:11 GMT, Andy Goryachev <[email protected]> wrote:
>> Petr Štechmüller has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Make sure instantiation works also when ProxyBuilder is not used
>
> modules/javafx.fxml/src/main/java/com/sun/javafx/fxml/builder/ProxyBuilder.java
> line 190:
>
>> 188: // This is used to support read-only collection/map property.
>> 189: private Object getReadOnlyProperty(String propName) {
>> 190: Method getter = findGetter(propName);
>
> I wonder if this is the right place for this logic.
>
> It looks like the `ArrayListWrapper` is used as a marker container type (the
> comment seems to be still valid), and the actual logic is handled in
> `getUserValue()`. I wonder if even an AtomicReference or a simple holder
> type would work just as well, with the logic handling different types moved
> to where it's actually used?
>
> (this private method getReadOnlyProperty() seems completely unnecessary in
> the first place)
Mostly wondering the same - why do we have `ArrayListWrapper` but no wrapper
classes for the other collections? Maybe we can unify / simplify it more.
I did some tests with `@NamedArg` and it really seems to be buggy with more
advanced usecases like this one
> modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 127:
>
>> 125: // a default property that is a list
>> 126: boolean collection;
>> 127: if (value instanceof List<?> || value instanceof Set<?>) {
>
> shouldn't this be `instanceof Collection` then?
> (and I don't think generics are needed for `instanceof` since it's runtime
> check and type is erased)
Without `<?>`, we will usually get a warning. I think its okay to keep. But I'm
also wondering if we should check for the generic `Collection` class
> modules/javafx.fxml/src/test/java/test/javafx/fxml/RT_8203870Test.java line
> 10:
>
>> 8: import static org.junit.jupiter.api.Assertions.*;
>> 9:
>> 10: public class RT_8203870Test {
>
> We don't use RT-* prefix anymore; typically, we try to make the names more
> descriptive, like `TestFXMLoaderReadOnlyProperties` or something like that,
> your call. Same goes for the resource files, probably.
+1, otherwise its hard to guess what happens. If you want you can still write
javadoc and reference the JDK ticket
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3336651342
PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3336636453
PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3336644684
