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

[Andy Goryachev]

On Thu, 11 Aug 2022 22:47:28 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> 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 13 additional commits since the last revision:
>> 
>>  - 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
>>  - 8290844: validate input
>>  - 8290844: illegal argument exception
>>  - 8290844: illegal argument exception
>>  - ... and 3 more: https://git.openjdk.org/jfx/compare/45c8c4bc...647ecd6c
>
> modules/javafx.controls/src/main/java/javafx/scene/control/Control.java line 245:
> 
>> 243:             if (skin != null) {
>> 244:                 if (skin.getSkinnable() != Control.this) {
>> 245:                     throw new IllegalArgumentException("There must be 1:1 relationship between Skin and Skinnable");
> 
> I wonder an error message like "Skin was not created for this Skinnable" or "Skin does not correspond to this Skinnable" would be clearer?
> 
> For the implementation, you should also unbind, if needed, before throwing the exception (we really should have made this a read-only property so it couldn't be bound, since a Skin is a "use once" object).
> 
> 
>                 if (isBound()) {
>                     unbind();
>                 }

Good point!
Actually, unbind() is sufficient, as it does the required check.  From javadoc: _If the Property is not bound, calling this method has no effect._
Is there another reason to call isBound(), perhaps for clarity?

> modules/javafx.controls/src/main/java/javafx/scene/control/Skin.java line 36:
> 
>> 34:  * <p>
>> 35:  * A Skin implementation should generally avoid modifying its control outside of
>> 36:  * {@link #install()} method.  The recommended life cycle of a Skin implementation
> 
> The life-cycle is what the Control (Skinnable) does regardless of the implementation of the Skin, so I would drop the word "recommended" (the recommendation for the Skin implementation is the previous sentence to avoid modifying the control outside the install method).

ok

> modules/javafx.controls/src/main/java/javafx/scene/control/Skin.java line 78:
> 
>> 76:     /**
>> 77:      * Called by {@link Skinnable#setSkin(Skin)} on a pristine control, or after the
>> 78:      * previous skin has been uninstalled via its {@link #dispose()} method.
> 
> Suggestion: You might be able to simplify this a bit by saying: "Called by {@link Skinnable#setSkin(Skin)}, after the previous skin, if any, has been uninstalled." I don't have a strong opinion on this.

thank you

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

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