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

[John Hendrikx]

On Thu, 27 Oct 2022 17:16:02 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> - added Skin.install()
>> - javadoc changes for Skinnable.setSkin(Skin)
>
> 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/f0ef8135...3235d433

I've read through all the docs and code, and added a few suggestions.

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

> 226:      * setting the skin property to a non-null {@code Skin} first checks
> 227:      * the return value of {@link Skin#getSkinnable()} against this Control,
> 228:      * and throws an {@code IllegalArgumentException} if it is not the same.

Suggestion:

     * To ensure a one-to-one relationship between a {@code Control} and its {@code Skin},
     * skins which were not created for this control are rejected with an {@code IllegalArgumentException}.

No need to emphasize this is only in the not null case, as null skins are documented to be allowed.

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

> 259:                     throw new IllegalArgumentException("Skin does not correspond to this Control");
> 260:                 }
> 261:             }

Suggestion:

            if (skin != null && skin.getSkinnable() != Control.this) {
                unbind();
                set(oldValue);
                throw new IllegalArgumentException("Skin does not correspond to this Control");
            }

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>

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

> 76:      * previous skin, if any, has been uninstalled via its {@link #dispose()} method.
> 77:      * This method allows a Skin to register listeners, add child nodes, set
> 78:      * required properties and/or event handlers.

I don't see how it is relevant for this interface to know what happens to the previous skin.  The interface should simply describe what is expected, and not go into detail here.

Suggestion:

     * Called when the {@link Skin} is accepted for its control. The skin can now
     * safely make changes to its associated control, like registering listeners, adding 
     * child nodes and modifying properties and event handlers.
     *
     * <p>When the control calls this method, it guarantees a future call to {@link #dispose} 
     * when the skin is removed.

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.

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

> 95:      * This method allows a {@link Skin} to implement any logic necessary to clean up itself after
> 96:      * the {@code Skin} is no longer needed. It may be used to release native resources.
> 97:      * The methods {@link #getSkinnable()} and {@link #getNode()}

I don't think this is correct "Disconnects the skin from its skinnable" -- the control does this.  I also think "clean up" covers the "native resources" bit, and not something that should be mentioned in an interface description.

May I suggest:

> Called when a previously installed skin is about to be removed from its associated control. This allows the skin to do clean up, like removing listeners and bindings, and undo any permanent changes to its control.  After this method completes, {@link #getSkinnable()} and {@link #getNode()} should return {@code null}.

modules/javafx.controls/src/main/java/javafx/scene/control/Skinnable.java line 41:

> 39:      * The Skin is responsible for rendering this {@code Skinnable}. From the
> 40:      * perspective of the {@code Skinnable}, the {@code Skin} is a black box.
> 41:      * It listens and responds to changes in state in a {@code Skinnable}.

I think the start is incorrect (in both old and new version).

It should either be "The skin **that** is responsible for" or "The skin responsible for".

Suggestion:

     * The Skin responsible for rendering this {@code Skinnable}. From the
     * perspective of the {@code Skinnable}, the {@code Skin} is a black box.
     * It listens and responds to changes in state of its {@code Skinnable}.

modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlTest.java line 816:

> 814:         SkinStub skin = new SkinStub(new ControlStub());
> 815:         c.setSkin(skin);
> 816:     }

Is Junit 5 availabe for this project?  If so I'd recommend writing:
Suggestion:

    @Test
    public void skinMustCorrespondToControl() {
        SkinStub skin = new SkinStub(new ControlStub());
        assertThrows(IllegalArgumentException.class, () -> c.setSkin(skin));
    }

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

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