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

[Kevin Rushforth]

On Fri, 12 Aug 2022 17:39:35 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> modules/javafx.controls/src/main/java/javafx/scene/control/Skinnable.java line 59:
>> 
>>> 57:      * {@code Skin}, this method may check the return value of
>>> 58:      * {@link Skin#getSkinnable()} against this Skinnable,
>>> 59:      * and may throw an {@code IllegalArgumentException} if it is not the same.
>> 
>> Since most implementations of `Skinnable` will do this check, we probably want to strengthen this statement. I like the language you added to the `@throws` tag, which says that it will throw an exception if the Skin does not "correspond to" this Skinnable. Is there a way to work this in, by defining what we mean to "correspond to"?
>
> @throws IllegalArgumentException if {@link Skin#getSkinnable()} returns a different {@code Skinnable}
> 
> is this better?

As discussed elsewhere, only the property method should have javadoc comments, so this should be moved up to the property method, and the existing javadoc comments for this method should be deleted.

I think we need a clearer statement here, given that there are only two top-level classes that implement `Skinnable`, `Control` and `PopupControl`, and thus override this method. `Control` enforces the 1-to-1 relationship by throwing an IAE if the `Skinnable` of the `Skin` doesn't match the `Control`, while `PopupControl` does not.

So maybe something a little more definitive, like this?


     * To ensure a one-to-one relationship between a {@code Skinnable} and its
     * {@code Skin}, some implementations of this method will check the return value of
     * {@link Skin#getSkinnable()} against this Skinnable,
     * and throw an {@code IllegalArgumentException} if it is not the same.


And then the exception could be changed to say....


     * @throws IllegalArgumentException if {@code skin.getSkinnable() != this}, for
     * implementations that enforce this restriction


This information would then be repeated in the `Control` class, but without the qualification.

>> modules/javafx.controls/src/main/java/javafx/scene/control/Skinnable.java line 64:
>> 
>>> 62:      * @throws IllegalArgumentException if {@code Skin} does not correspond to this {@code Skinnable}
>>> 63:      */
>>> 64:     public void setSkin(Skin<?> value);
>> 
>> Generally docs for properties should go on just the property and not the setter or getter. The docs will be copied and cross linked by the javadoc tool. Can you try that, and generate a javadoc, and see if that holds in this case (we don't have many cases where a property is in an interface).
>
> So javadoc tool ignores the interface, resulting in Control.setSkin(skin) method inheriting the property's description.  
> 
> Curiously, eclipse does show the interface's version, which I think helps more than having three identical descriptions for the property, its getter and setter.
> 
> What would you recommend we do here?

I recommend that we follow the existing practice of only putting the docs on the `xxxProperty` method, which includes removing the docs from `{set|get}Skin` in `Skinnable`, `Control`, and `PopupControl`. I checked it and it does what I would expect (except for a minor issue in that the `@throws` doesn't generate any docs, but since the Description indicates the conditions under which it will throw IAE, that's not a big problem; I'll file an RFE against the javadoc tool).

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

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