On Wed, 21 Jan 2026 01:07:15 GMT, Michael Strauß <[email protected]> wrote:
>> Implementation of [enhanced property >> metadata](https://gist.github.com/mstr2/2fec0303fc440b8eaeb126befc76eb5c). >> >> ### New API >> This PR includes the following API additions: >> >> 1. `ReadOnlyProperty.getDeclaringClass()` and its default implementation. >> 2. The `javafx.beans.property.AttachedProperty` interface. >> 3. New constructors for all `Simple<*>Property` and `ReadOnly<*>Wrapper` >> classes, accepting the declaring class of the property. >> >> The declaring class is stored in a new field in the `Simple<*>Property` >> classes. If a legacy constructor is used that doesn't specify the declaring >> class, the `ReadOnlyProperty.getDeclaringClass()` default implementation is >> called the first time the `Simple<*>Property.getDeclaringClass()` method is >> called, and its result is stored for future retrieval. >> >> ### Testing >> For testing, this PR also includes the >> `test.util.property.PropertyMetadataVerifier` tool. It systematically tests >> all public and protected properties of a class, and ensures conformance to >> the following rules: >> * `ReadOnlyProperty.getBean()` returns the object instance of the enclosing >> class, or the target object instance if the property is an attached property. >> * `ReadOnlyProperty.getName()` returns the name of the property, which must >> correspond to the name of the property getter (excluding the word >> "Property"). >> * `ReadOnlyProperty.getDeclaringClass()` returns the enclosing class of the >> property getter. >> * The declaring class of a `Simple<*>Property` or `ReadOnly<*>Wrapper` must >> be specified in the constructor, not resolved at runtime. >> * `getBean()`, `getName()`, and `getDeclaringClass()` must not be overridden >> in subclasses of `Simple<*>Property` or `ReadOnly<*>Wrapper`. >> * An instance property does not implement `AttachedProperty`. >> * An instance property has a parameterless property getter. >> * An attached property implements `AttachedProperty`. >> * An attached property has a static single-argument property getter that >> accepts the target object. >> * `AttachedProperty.getTargetClass()` returns the class of the single >> parameter of the static property getter. >> * A property getter does not return an instance of `ReadOnly<*>Wrapper`, it >> returns the result of calling `ReadOnly<*>Wrapper.getReadOnlyProperty()`. >> >> Many properties in existing JavaFX classes violate the >> `PropertyMetadataVerifier` rules in some way or shape. This PR won't address >> these issues, this will be done in a future cleanup PR. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > ReadOnlyProperty.getDeclaringClass() tests Hm, perhaps now that the idea here has had some time to sink in, perhaps the initial version with attachedness directly specified on the `ReadOnlyProperty` interface was better (also in light of the hoops we now need to jump through in the wrappers). Unless of course you think this *is* the better solution. It seems to me that since we're already adding some weight to all the `Simple` property classes, and now additional mini-classes just to implement `AttachedProperty`, a solution where there is one or two default methods on `ReadOnlyProperty` is lighter, even if they may seem out of place (like I said, the idea needed some time to sink in, and there may be a good enough case to have this on simply **all** properties, UI related or not). Best however to see what other reviewers think. All in all, I think this is a good infrastructural change to build upon for making all static properties attached (I'm especially excited for `HBox`, `VBox` and `GridPane`), and I assume there will be follow-up PR's (where I could perhaps help) to modify these classes, as well and supporting these to be styleable by CSS. The included `HeaderBar` changes may be better done separately. Also are you sure this will work for styleable properties as well? modules/javafx.base/src/main/java/javafx/beans/property/AttachedProperty.java line 46: > 44: * Returns the class of objects to which this property can be > attached. > 45: * > 46: * @return the target class If this can ever return `null` (pretty sure it can), then I think that should be documented so there is no need to guess. modules/javafx.base/src/main/java/javafx/beans/property/ReadOnlyBooleanWrapper.java line 84: > 82: > 83: /** > 84: * The constructor of {@code ReadOnlyBooleanWrapper} nit: Here and in a lot of other places, missing punctuation at end of sentence of main javadoc comment. modules/javafx.base/src/main/java/javafx/beans/property/ReadOnlyBooleanWrapper.java line 163: > 161: return > ((AttachedProperty)ReadOnlyBooleanWrapper.this).getTargetClass(); > 162: } > 163: } This is clumsier than I had hoped. modules/javafx.base/src/main/java/javafx/beans/property/ReadOnlyProperty.java line 59: > 57: String getName(); > 58: > 59: /** Should we add docs to `getBean()` as well? If I understand correctly, the declaring class can be say `HBox` for an attached property, and the bean instance would then be a child of the hbox. The documentation for `getBean` then seems outdated, or could be clarified in light of this? As it stands, one might think that `getBean().getClass() == getDeclaringClass()` (and it often is). modules/javafx.base/src/main/java/javafx/beans/property/SimpleBooleanProperty.java line 150: > 148: this.name = (name == null) ? DEFAULT_NAME : name; > 149: } > 150: } nit: I see this was already the case for existing, but just saying, I don't like the code duplication in the constructors. modules/javafx.base/src/test/java/test/util/property/PropertyMetadataVerifier.java line 172: > 170: } > 171: } else if > (isSimplePropertyOrDerivedClass(propertyClass)) { > 172: fail(("%s#getDeclaringClass() must not be overridden > in %s, pass %s " nit: unnecessary parentheses here modules/javafx.base/src/test/java/test/util/property/PropertyMetadataVerifier.java line 236: > 234: if (isSimplePropertyClass(propertyClass)) { > 235: return; > 236: } else { nit: no need for `else` modules/javafx.graphics/src/main/java/javafx/scene/layout/HeaderBar.java line 234: > 232: } > 233: > 234: class PropertyImpl extends SimpleObjectProperty<HeaderDragType> > implements AttachedProperty { This seems to be primarily an infrastructure PR, we should consider to leave these specific changes out. I think there are many classes that could be adjusted, for which we could create a few grouped PR's. I also noticed there is basically no users yet of the new getTargetClass method, I assume that was done deliberately as well. ------------- PR Review: https://git.openjdk.org/jfx/pull/2015#pullrequestreview-3686471870 PR Review Comment: https://git.openjdk.org/jfx/pull/2015#discussion_r2711969585 PR Review Comment: https://git.openjdk.org/jfx/pull/2015#discussion_r2711960784 PR Review Comment: https://git.openjdk.org/jfx/pull/2015#discussion_r2712042459 PR Review Comment: https://git.openjdk.org/jfx/pull/2015#discussion_r2712031904 PR Review Comment: https://git.openjdk.org/jfx/pull/2015#discussion_r2712054919 PR Review Comment: https://git.openjdk.org/jfx/pull/2015#discussion_r2711944565 PR Review Comment: https://git.openjdk.org/jfx/pull/2015#discussion_r2711948269 PR Review Comment: https://git.openjdk.org/jfx/pull/2015#discussion_r2712072320
