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

[Andy Goryachev]

On Thu, 27 Oct 2022 17:57:02 GMT, John Hendrikx <jhendrikx 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 28 additional commits since the last revision:
>> 
>>  - Merge remote-tracking branch 'origin/master' into 8290844.skin.install
>>  - 8290844: review comments
>>  - Merge remote-tracking branch 'origin/master' into 8290844.skin.install
>>  - 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
>>  - 8290844: javadoc
>>  - Merge branch 'openjdk:master' into 8290844.skin.install
>>  - 8290844: unit tests
>>  - ... and 18 more: https://git.openjdk.org/jfx/compare/d4b53975...3235d433
>
> modules/javafx.controls/src/main/java/javafx/scene/control/Skin.java line 44:
> 
>> 42:  * <li>installing of the new skin via {@link #install()}
>> 43:  * </ul>
>> 44:  * </ul>
> 
> It says it describes the life cycle of a Skin, but instead goes into detail about the skin replacement mechanism. The lifecycle is much simpler:
> 
> - Instantiation (constructor)
> - Acceptance (#install)
> - Disposal (#dispose)
> 
> I also think that `install` may not be the correct name.  It is like an event that already occurred, and should be past tense (like `invalidated`).  The Control has done its checks and has accepted the new skin. Perhaps `installed` or `accepted`?
> 
> Suggestion:
> 
>  * {@link #install()} method.  A skin has the following life cycle:
>  * <ul>
>  * <li>instantiation: a skin should not make permanent changes to its associated control during this phase
>  * <li>acceptance: a skin was accepted as the new skin for its control and can now change the control as needed
>  * <li>disposal: a skin, which was previously accepted, is about to be unassociated with its control and should undo any permanent changes and clean up any bindings and listeners on the control
>  * </ul>

Not sure if I agree here.  For one thing, Swing ComponentUI uses installUI(), so it's very clear to anyone familiar with the swing.  Secondly, install() installs the skin in its control, i.e. sets various fields and properties on that control instance.

I'd prefer to keep existing install() name.

> modules/javafx.controls/src/main/java/javafx/scene/control/Skin.java line 87:
> 
>> 85:      * Most implementations of Skin
>> 86:      * do not need to implement {@code install} unless they must set one or more
>> 87:      * properties in the corresponding Skinnable.
> 
> I think this might be better:
> 
> Suggestion:
> 
>      * Skins only need to implement {@code install} if they need to make direct changes to the control
>      * like overwriting properties or event handlers. Such skins should ensure these changes are undone in
>      * their {@link #dipose()} method.

Your version is so much better, than you.

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

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