On Fri, 29 May 2026 07:27:55 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 one > additional commit since the last revision: > > Make sure instantiation works also when ProxyBuilder is not used Some preliminary code review comments, I did not test the solution (yet). modules/javafx.fxml/src/main/java/com/sun/javafx/fxml/builder/ProxyBuilder.java line 41: > 39: import java.util.HashSet; > 40: import java.util.LinkedHashMap; > 41: import java.util.LinkedHashSet; please update the copyright year in all the modified files, as per tradition. 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) 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) modules/javafx.fxml/src/test/java/test/com/sun/javafx/fxml/builder/ClassWithReadOnlySet.java line 1: > 1: package test.com.sun.javafx.fxml.builder; missing copyright header (here and in some other files) 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. modules/javafx.fxml/src/test/java/test/javafx/fxml/RT_8203870Test.java line 25: > 23: assertEquals(2, widget.getObservableMap().size()); > 24: > 25: // assertEquals(3, widget.getNames().length); is there a reason to keep commented out lines? modules/javafx.fxml/src/test/java/test/javafx/fxml/RT_8203870Test.java line 45: > 43: // assertEquals(3, widget.getRatios().length); > 44: } > 45: minor: extra newlines modules/javafx.fxml/src/test/resources/test/javafx/fxml/rt_8203870_a.fxml line 1: > 1: <?xml version="1.0" encoding="UTF-8"?> needs a copyright header (check other .fxml files for an example) modules/javafx.fxml/src/test/resources/test/javafx/fxml/rt_8203870_a.fxml line 55: > 53: </observableMap> > 54: > 55: </ClassWithCollections> minor: missing newline (also in ..._b.fxml) ------------- PR Review: https://git.openjdk.org/jfx/pull/2167#pullrequestreview-4403696449 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3336335774 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3336531125 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3336392140 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3336415321 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3336435096 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3336438779 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3336437171 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3336443873 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3336331277
