RFR: 8290844: Add Skin.install() method [v11]

[Kevin Rushforth]

On Fri, 7 Oct 2022 18:55:56 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> - added Skin.install()
>> - javadoc changes for Skinnable.setSkin(Skin)
>
> Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 24 additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'origin/master' into 8290844.skin.install
>  - 8290844: javadoc
>  - Merge remote-tracking branch 'origin/master' into 8290844.skin.install
>  - 8290844: javadoc
>  - Merge branch 'openjdk:master' into 8290844.skin.install
>  - 8290844: unit tests
>  - Merge remote-tracking branch 'origin/master' into 8290844.skin.install
>  - 8290844: review comments
>  - 8290844: review comments
>  - Merge remote-tracking branch 'origin/master' into 8290844.skin.install
>  - ... and 14 more: https://git.openjdk.org/jfx/compare/c435607c...15731a2c

A few comments left inline.

modules/javafx.controls/src/main/java/javafx/scene/control/Control.java line 227:

> 225:      * To ensure a one-to-one relationship between a {@code Control} and its {@code Skin},
> 226:      * setting the {@link #skinProperty() skin property} first triggers a check of the return value of
> 227:      * {@link Skin#getSkinnable()} against this Control, and throw an {@code IllegalArgumentException}

I think you still need to say "if not null" here. Also, I think "triggers a check of" can be replaced with "checks". Further, "throw" should be "throws", so maybe something more like:


setting the {@link #skinProperty() skin property} to a non-null {@code Skin} first
checks the return value of {@link Skin#getSkinnable()} against this Control, and throws ...

modules/javafx.controls/src/main/java/javafx/scene/control/Control.java line 232:

> 230:      * to complete the process.  Only inside of {@link Skin#install()} should {@code Skin} implementations
> 231:      * set/overwrite properties of their {@code Control} (though commutative operations like add/remove listener
> 232:      * can still be done in the {@code Skin} constructor.

I don't think "commutative" is quite the right term here, although I get what you are saying. This is trying to describe an operation that can be reversed (undone) without side effect. So commutativity is part of it, but not the whole thing. The property we want is:


f(A, A`) == empty
f(A, B, B`) == f(A)
f(A, B, A`) == f(B)


Is there a single term that describes this?

Oh, and you are missing a closing parenthesis.

modules/javafx.controls/src/main/java/javafx/scene/control/Control.java line 237:

> 235:      *
> 236:      * @return the skin property for this control
> 237:      * @throws IllegalArgumentException if {@code (skin != null && skin.getSkinnable() != Control.this)}

Saying `Control.this` is leaking an implementation detail into the spec. The fact that the proposed implementation "just so happens" to do the check in a lambda, causing the implementation to use `Control.this`, is of no relevance to the spec. You should replace `Control.this` with `this`.

modules/javafx.controls/src/main/java/javafx/scene/control/Skin.java line 39:

> 37:  * <li>instantiation
> 38:  * <li>configuration, such as passing of dependencies and parameters
> 39:  * <li>inside of {@link Control#setSkin(Skin)}:

Same comment as earlier: replace "inside of Control.setSkin" with "when the skin is set on a Skinnable".

modules/javafx.controls/src/main/java/javafx/scene/control/Skin.java line 75:

> 73: 
> 74:     /**
> 75:      * Called once by {@link Skinnable#setSkin(Skin)} after the

Same comment as earlier:  replace "Called once by Skinnable.setSkin" with "Called once when the skin is set".

modules/javafx.controls/src/main/java/javafx/scene/control/Skin.java line 80:

> 78:      * required properties and/or event handlers.
> 79:      * <p>
> 80:      * Client code must not call this method.

I think saying "Application code" would be clearer (to remove the ambiguity over who the "client" is).

-------------

PR: https://git.openjdk.org/jfx/pull/845