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

[Kevin Rushforth]

On Fri, 9 Sep 2022 20:47:56 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> - added Skin.install()
>> - javadoc changes for Skinnable.setSkin(Skin)
>> 
>> no code changes for Skinnable.setSkin(Skin) yet.
>
> 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 16 additional commits since the last revision:
> 
>  - 8290844: review comments
>  - 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
>  - 8290844: javadoc
>  - Merge remote-tracking branch 'origin/master' into 8290844.skin.install
>  - 8289397: added space
>  - 8290844: skin.install
>  - 8290844: whitespace
>  - ... and 6 more: https://git.openjdk.org/jfx/compare/6f6de14f...7d5db78b

The API looks good. I left a few more minor comments on the docs, and then I think it's ready.

The implementation looks good, too. So I think the only thing missing are unit tests.

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

> 224:      * <p>
> 225:      * To ensure a one-to-one relationship between a {@code Control} and its {@code Skin},
> 226:      * {@link Control#setSkin(Skin)} method will check the return value of {@link Skin#getSkinnable()}

Suggestion: either "the setSkin method" or "setSkin" (without "method").

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

> 229:      * A skin may be null.
> 230:      *
> 231:      * @return the skin property for this control

I recommend adding an `@throws` clause here. Something like:


     * @throws IllegalArgumentException if {@code skin != null && skin != getSkinnable()}


although the javadoc tool currently doesn't do anything with an `@throws` tag in a property (I'll file a follow-up bug for this).

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

> 250:                     unbind();
> 251:                     set(oldValue);
> 252:                     throw new IllegalArgumentException("Skin does not correspond to this Skinnable");

Minor: it might be clearer to say "this Control" here (but `Skinnable` is not wrong)

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

> 81:      *
> 82:      * @implNote
> 83:      * Most implementations of Skin in the <code>javafx.controls</code> module

Minor: `{@code some text}` is preferred over `<code>some text</code>` in API docs.

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

> 82:      * @implNote
> 83:      * Most implementations of Skin in the <code>javafx.controls</code> module
> 84:      * do not need to implement {@link Skin#install()} unless they must set one or more

Since this doc is already in the `install` method, there is no need for a link. It can be replaced with: `{@code install}`

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

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