On Wed, 21 Jan 2026 10:37:12 GMT, John Hendrikx <[email protected]> wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> ReadOnlyProperty.getDeclaringClass() tests
>
> 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.
It never returns `null`, as an attached property must have a target class. I
mean it can return `null`, but that would be an implementation bug.
> 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.
I think it's okay as it's a very localized implementation detail. You'll
probably forget that it even exists very soon and move on.
> 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).
`getBean().getClass() == getDeclaringClass()` is only true if the property is
declared in the most derived class. For most JavaFX properties, this would not
be the case. In any case, I've updated the documentation a bit.
> 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.
I've changed to telescoping constructor invocation where possible.
> 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
They are not unnecessary, they group the string concatenation on which the
`formatted()` method is called.
> 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.
I included this as a small demonstration for an attached property
implementation.
I don't think that there will be users of `getTargetClass()` in the JavaFX
framework, at least at the moment (it didn't exist before, so why would there
be users).
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2015#discussion_r2714106046
PR Review Comment: https://git.openjdk.org/jfx/pull/2015#discussion_r2714115188
PR Review Comment: https://git.openjdk.org/jfx/pull/2015#discussion_r2714108925
PR Review Comment: https://git.openjdk.org/jfx/pull/2015#discussion_r2714116347
PR Review Comment: https://git.openjdk.org/jfx/pull/2015#discussion_r2714103282
PR Review Comment: https://git.openjdk.org/jfx/pull/2015#discussion_r2714122151
