8290844: Add Skin.install() method

Reviewed-by: kcr, aghaisas

modules/javafx.controls/src/main/java/javafx/scene/control/Control.java

@@ -222,8 +222,19 @@ private static Class<?> loadClass(final String className, final Object instance)
      * {@code Skin}. Every {@code Skin} maintains a back reference to the
      * {@code Control} via the {@link Skin#getSkinnable()} method.
      * <p>
+     * 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}.
+     * Then, {@link Skin#dispose()} is called on the old skin, disconnecting
+     * it from the corresponding {@code Control}.  And finally, {@link Skin#install()} is invoked
+     * to complete the process.  Only inside of {@link Skin#install()} should {@code Skin} implementations
+     * set/overwrite properties of their {@code Control} (though some operations like adding/removing a listener
+     * can still be done in the {@code Skin} constructor).
+     * <p>
      * A skin may be null.
+     *
      * @return the skin property for this control
+     * @throws IllegalArgumentException if {@code (skin != null && skin.getSkinnable() != this)}
      */
     @Override public final ObjectProperty<Skin<?>> skinProperty() { return skin; }
     @Override public final void setSkin(Skin<?> value) {
@@ -239,6 +250,15 @@ private static Class<?> loadClass(final String className, final Object instance)
 
         @Override protected void invalidated() {
             Skin<?> skin = get();
+            // check whether the skin is for right control
+            if (skin != null) {
+                if (skin.getSkinnable() != Control.this) {
+                    unbind();
+                    set(oldValue);
+                    throw new IllegalArgumentException("Skin does not correspond to this Control");
+                }
+            }
+
             // Collect the name of the currently installed skin class. We do this
             // so that subsequent updates from CSS to the same skin class will not
             // result in reinstalling the skin
@@ -288,6 +308,11 @@ private static Class<?> loadClass(final String className, final Object instance)
                 }
             }
 
+            // let the new skin modify this control
+            if (skin != null) {
+                skin.install();
+            }
+
             // clear out the styleable properties so that the list is rebuilt
             // next time they are requested.
             styleableProperties = null;

modules/javafx.controls/src/main/java/javafx/scene/control/PopupControl.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -263,6 +263,11 @@ public void set(Skin<?> v) {
                 bridge.getChildren().clear();
             }
 
+            // let the new skin modify this control
+            if (skin != null) {
+                skin.install();
+            }
+
             // calling NodeHelper.reapplyCSS() as the styleable properties may now
             // be different, as we will now be able to return styleable properties
             // belonging to the skin. If NodeHelper.reapplyCSS() is not called, the

modules/javafx.controls/src/main/java/javafx/scene/control/Skin.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -28,9 +28,20 @@
 import javafx.scene.Node;
 
 /**
- * Base class for defining the visual representation of user interface controls
- * by defining a scene graph of nodes to represent the skin.
- * A user interface control is abstracted behind the {@link Skinnable} interface.
+ * An interface for defining the visual representation of user interface controls.
+ * <p>
+ * A Skin implementation should generally avoid modifying its control outside of
+ * {@link #install()} method.  The life cycle of a Skin implementation
+ * is as follows:
+ * <ul>
+ * <li>instantiation
+ * <li>configuration, such as passing of dependencies and parameters
+ * <li>when the skin is set on a {@link Skinnable}:
+ * <ul>
+ * <li>uninstalling of the old skin via its {@link #dispose()} method
+ * <li>installing of the new skin via {@link #install()}
+ * </ul>
+ * </ul>
  *
  * @param <C> A subtype of Skinnable that the Skin represents. This allows for
  *      Skin implementation to access the {@link Skinnable} implementation,
@@ -61,12 +72,31 @@ public interface Skin<C extends Skinnable> {
     public Node getNode();
 
     /**
-     * Called by a Skinnable when the Skin is replaced on the Skinnable. This method
-     * allows a Skin to implement any logic necessary to clean up itself after
-     * the Skin is no longer needed. It may be used to release native resources.
-     * The methods {@link #getSkinnable()} and {@link #getNode()}
-     * should return null following a call to dispose. Calling dispose twice
-     * has no effect.
+     * Called once when {@code Skin} is set.  This method is called after the previous skin,
+     * if any, has been uninstalled via its {@link #dispose()} method.
+     * The skin can now safely make changes to its associated control, like registering listeners,
+     * adding child nodes, and modifying properties and event handlers.
+     * <p>
+     * Application code must not call this method.
+     * <p>
+     * The default implementation of this method does nothing.
+     *
+     * @implNote
+     * 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 #dispose()} method.
+     *
+     * @since 20
+     */
+    default public void install() { }
+
+    /**
+     * 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 changes
+     * to the control's properties.
+     * After this method completes, {@link #getSkinnable()} and {@link #getNode()} should return {@code null}.
+     * <p>
+     * Calling {@link #dispose()} more than once has no effect.
      */
     public void dispose();
 }

modules/javafx.controls/src/main/java/javafx/scene/control/Skinnable.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -36,29 +36,23 @@
  */
 public interface Skinnable {
     /**
-     * Skin is responsible for rendering this {@code Control}. From the
-     * perspective of the {@code Control}, the {@code Skin} is a black box.
-     * It listens and responds to changes in state in a {@code Control}.
+     * The {@code 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}.
      * <p>
-     * There is a one-to-one relationship between a {@code Control} and its
-     * {@code Skin}. Every {@code Skin} maintains a back reference to the
-     * {@code Control}.
+     * Some implementations of {@code Skinnable} define a one-to-one relationship between {@code Skinnable}
+     * and its {@code Skin}. Every {@code Skin} maintains a back reference to the
+     * {@code Skinnable}.  When required, this relationship is enforced when the {@code Skin} is set,
+     * throwing an {@code IllegalArgumentException} if the return value of {@link Skin#getSkinnable()}
+     * is not the same as this {@code Skinnable}.
      * <p>
      * A skin may be null.
      *
-     * @return the skin property for this control
+     * @return the skin property for this Skinnable
      */
     public ObjectProperty<Skin<?>> skinProperty();
 
-    /**
-     * Sets the skin that will render this {@link Control}
-     * @param value the skin value for this control
-     */
     public void setSkin(Skin<?> value);
 
-    /**
-     * Returns the skin that renders this {@link Control}
-     * @return the skin for this control
-     */
     public Skin<?> getSkin();
 }

modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -787,6 +787,36 @@ private void enableLogging() {
         assertNull(c.getSkin());
     }
 
+    /** verifies that Control.setSkin() calls Skin.install() JDK-8290844 */
+    @Test
+    public void setSkinCallsInstall() {
+        class SkinWithInstall extends SkinStub {
+            public boolean installed;
+
+            public SkinWithInstall(Control c) {
+                super(c);
+            }
+
+            @Override
+            public void install() {
+                installed = true;
+            }
+        }
+
+        SkinWithInstall skin = new SkinWithInstall(c);
+        c.setSkin(skin);
+        assertTrue("Control.setSkin() must call Skin.install()", skin.installed);
+    }
+
+    /** Verifies that an IllegalArgumentException is thrown when setting skin for an unrelated control JDK-8290844 */
+    @Test
+    public void skinMustCorrespondToControl() {
+        SkinStub skin = new SkinStub(new ControlStub());
+        assertThrows(IllegalArgumentException.class, () -> {
+            c.setSkin(skin);
+        });
+    }
+
     @Test public void skinPropertyHasBeanReference() {
         assertSame(c, c.skinProperty().getBean());
     }