On Tue, 2 Jun 2026 12:35:23 GMT, Petr Štechmüller <[email protected]> wrote:
>> ProxyBuilder is used by the FXML loader to instantiate classes whose >> constructors are annotated with `@NamedArg`. When such a class also exposes >> a read-only `Map` property (e.g. getProperties() — the same pattern used by >> `javafx.scene.Node`), setting child elements under that property in FXML >> caused incorrect behaviour or a runtime error. >> >> **Root cause:** _getReadOnlyProperty()_ always returned an >> `ArrayListWrapper` regardless of the actual **getter** return type. When the >> getter returns a `Map`, an `ArrayListWrapper` is the wrong container and the >> entries are never transferred to the real map on the object. >> >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Petr Štechmüller has updated the pull request incrementally with two > additional commits since the last revision: > > - Use collection instead of List/Set pair > - Update copyright headers Thank you for adding tests! Here some preliminary code review only comments. modules/javafx.fxml/src/main/java/com/sun/javafx/fxml/builder/ProxyBuilder.java line 202: > 200: if (ObservableMap.class.isAssignableFrom(retType)) { > 201: return FXCollections.observableHashMap(); > 202: } Question: the `javafx.collection` package also declares an `ObservableArray` class, should it be handled here? modules/javafx.fxml/src/main/java/com/sun/javafx/fxml/builder/ProxyBuilder.java line 211: > 209: } > 210: if (Collection.class.isAssignableFrom(retType)) { > 211: return new ArrayListWrapper<>(); should it be called `CollectionWrapper` ? or, why have this type at all - the `Set` case returns a `LinkedHashSet` instance, so perhaps this case should return `LinkedList` maybe? modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 145: > 143: @SuppressWarnings("unchecked") > 144: public void add(Object element) { > 145: // If value is a list, add element to it; otherwise, get the > value is a collection? modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 157: > 155: > 156: // Get the list value > 157: collection = > (List<Object>)getProperties().get(defaultPropertyName); should it be `(Collection<Object>)` then? modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 159: > 157: collection = > (List<Object>)getProperties().get(defaultPropertyName); > 158: > 159: // Coerce the element to the list item type collection item type? modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 486: > 484: String stringValue) throws LoadException { > 485: // Split the string and add the values to the list > 486: List<Object> list = > (List<Object>)valueAdapter.get(listPropertyName); should it be `Collection<Object>` ? modules/javafx.fxml/src/test/java/test/com/sun/javafx/fxml/builder/ClassWithReadOnlySet.java line 1: > 1: package test.com.sun.javafx.fxml.builder; The standard copyright header is still missing. modules/javafx.fxml/src/test/java/test/javafx/fxml/ClassWithDefaultListProperty.java line 1: > 1: package test.javafx.fxml; missing copyright header - here and in other files modules/javafx.fxml/src/test/resources/test/javafx/fxml/class_with_default_list_property.fxml line 5: > 3: <?import test.javafx.fxml.ClassWithDefaultListProperty?> > 4: > 5: <ClassWithDefaultListProperty>one,two,three</ClassWithDefaultListProperty> minor: missing newline modules/javafx.fxml/src/test/resources/test/javafx/fxml/class_with_default_set_property.fxml line 5: > 3: <?import test.javafx.fxml.ClassWithDefaultSetProperty?> > 4: > 5: <ClassWithDefaultSetProperty>one,two,three</ClassWithDefaultSetProperty> minor: missing newline (here and in some other files). you can see it in the github diff view. ------------- Changes requested by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/2167#pullrequestreview-4618816968 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3514322116 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3514314971 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3514122351 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3514117233 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3514120848 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3514165513 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3514206549 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3514250167 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3514261773 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3514268586
