RFR: 8375588: Enhanced property metadata [v2]

Wed Jan 21 11:17:48 UTC 2026

On Wed, 21 Jan 2026 01:07:15 GMT, Michael Strauß <mstrauss at openjdk.org> 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