On Wed, 8 Jul 2026 08:45:24 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: > > Fix ProxyBuilder to correctly handle read-only Observable Integer/Float > array with @NamedArg some comments and suggestions, 1 question, looks good otherwise! modules/javafx.fxml/src/main/java/com/sun/javafx/fxml/BeanAdapter.java line 655: > 653: > 654: /** > 655: * Determines the type of a list or set item. Determines the type of a `Collection`. modules/javafx.fxml/src/main/java/com/sun/javafx/fxml/builder/ProxyBuilder.java line 501: > 499: int[] ints = new int[list.size()]; > 500: for (int idx = 0; idx < list.size(); idx++) { > 501: ints[idx] = BeanAdapter.coerce(list.get(idx), > Integer.class).intValue(); `.intValue()` is not needed modules/javafx.fxml/src/main/java/com/sun/javafx/fxml/builder/ProxyBuilder.java line 514: > 512: float[] floats = new float[list.size()]; > 513: for (int idx = 0; idx < list.size(); idx++) { > 514: floats[idx] = BeanAdapter.coerce(list.get(idx), > Float.class).floatValue(); `.floatValue()` is not needed modules/javafx.fxml/src/main/java/com/sun/javafx/fxml/builder/ProxyBuilder.java line 592: > 590: strsMap.put(propName, new Getter(m, retType)); > 591: } else if (Map.class.isAssignableFrom(retType) && > argType.length == 0 > 592: && !strsMap.containsKey(propName)) { Why is this condition needed (only here?)? modules/javafx.fxml/src/main/java/com/sun/javafx/fxml/builder/ProxyBuilder.java line 664: > 662: if (argStr instanceof Map) { > 663: to.putAll((Map) argStr); > 664: } Suggestion: Map<?, ?> to = (Map<?, ?>) ModuleHelper.invoke(method, obj, new Object[]{}); if (argStr instanceof Map argMap) { to.putAll(argMap); } avoid most of the warnings- modules/javafx.fxml/src/test/java/test/javafx/fxml/ClassWithDefaultPropertyTest.java line 39: > 37: ClassWithDefaultListProperty classWithDefaultProperty = > fxmlLoader.load(); > 38: > 39: Assertions.assertEquals(1, > classWithDefaultProperty.getItems().size()); I would rather recommend to do `import static org.junit.jupiter.api.Assertions.*;` and remove `Assertions` from here (also makes this shorter). modules/javafx.fxml/src/test/java/test/javafx/fxml/ClassWithDefaultPropertyTest.java line 41: > 39: Assertions.assertEquals(1, > classWithDefaultProperty.getItems().size()); > 40: // Coma separated values are currently not supported in default > list properties > 41: // Assertions.assertEquals(3, > classWithDefaultProperty.getItems().size()); maybe remove? I don't think this is something that can be supported without breaking compatibility. ------------- PR Review: https://git.openjdk.org/jfx/pull/2167#pullrequestreview-4677813648 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3564189430 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3564187335 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3564187736 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3564195850 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3564192671 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3564203782 PR Review Comment: https://git.openjdk.org/jfx/pull/2167#discussion_r3564205723
