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

[Kevin Rushforth]

On Tue, 4 Oct 2022 09:25:00 GMT, Johan Vos <jvos 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 20 additional commits since the last revision:
>> 
>>  - 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
>>  - 8290844: review comments
>>  - 8290844: review comments
>>  - Merge remote-tracking branch 'origin/master' into 8290844.skin.install
>>  - 8290844: javadoc
>>  - ... and 10 more: https://git.openjdk.org/jfx/compare/b8fb4e8d...d954aafc
>
> modules/javafx.controls/src/main/java/javafx/scene/control/Control.java line 229:
> 
>> 227:      * against this Control, and throw an {@code IllegalArgumentException} if it is not the same.
>> 228:      * <p>
>> 229:      * A skin may be null.
> 
> This breaks the 1-1 relationship mentioned above, so it's probably best to mention this as an exception to the 1-1 rule.

Maybe something like `will check the return value of {@link Skin#getSkinnable()}, if the Skin is not {@code null}, against this Control...`?

> 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
> 
> If this is only targeting controls in the javafx.controls module, it is not relevant to developers using JavaFX.

It might be best to remove `in the <code>javafx.controls</code> module`, since this advice is relevant for third-party controls as well.

> modules/javafx.controls/src/main/java/javafx/scene/control/Skinnable.java line 43:
> 
>> 41:      * It listens and responds to changes in state in a {@code Skinnable}.
>> 42:      * <p>
>> 43:      * There is typically a one-to-one relationship between a {@code Skinnable} and its
> 
> The word "typically" doesn't describe when this applies and when not. From the previous comments, I get a better understanding (PopupControl), but developers reading the JavaDoc won't see that.

Good point. We have a tension between "pure OO" and giving clear advice. In the case of `Skin`, there are only two direct subclasses, so we could call that out here and say that `Control` enforces a 1-to-1 relationship, but `PopupControl` does not. Alternatively, we could change this to say that "Some implementations of Skinnable define a 1-to-1 relationship...". That would key developers to look for that in the the documentation of the implementing class.

Which way do you think is best?

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

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