On Thu, 11 Aug 2022 22:19:39 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> - added Skin.install() >> - javadoc changes for Skinnable.setSkin(Skin) >> >> no code changes for Skinnable.setSkin(Skin) yet. > > 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 13 additional > commits since the last revision: > > - 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 > - 8289397: added space > - 8290844: skin.install > - 8290844: whitespace > - 8290844: validate input > - 8290844: illegal argument exception > - 8290844: illegal argument exception > - ... and 3 more: https://git.openjdk.org/jfx/compare/9063b6c9...647ecd6c I left a few comments / questions / suggestions on the docs. modules/javafx.controls/src/main/java/javafx/scene/control/Control.java line 245: > 243: if (skin != null) { > 244: if (skin.getSkinnable() != Control.this) { > 245: throw new IllegalArgumentException("There must be > 1:1 relationship between Skin and Skinnable"); I wonder an error message like "Skin was not created for this Skinnable" or "Skin does not correspond to this Skinnable" would be clearer? For the implementation, you should also unbind, if needed, before throwing the exception (we really should have made this a read-only property so it couldn't be bound, since a Skin is a "use once" object). if (isBound()) { unbind(); } modules/javafx.controls/src/main/java/javafx/scene/control/Skin.java line 36: > 34: * <p> > 35: * A Skin implementation should generally avoid modifying its control > outside of > 36: * {@link #install()} method. The recommended life cycle of a Skin > implementation The life-cycle is what the Control (Skinnable) does regardless of the implementation of the Skin, so I would drop the word "recommended" (the recommendation for the Skin implementation is the previous sentence to avoid modifying the control outside the install method). modules/javafx.controls/src/main/java/javafx/scene/control/Skin.java line 78: > 76: /** > 77: * Called by {@link Skinnable#setSkin(Skin)} on a pristine control, > or after the > 78: * previous skin has been uninstalled via its {@link #dispose()} > method. Suggestion: You might be able to simplify this a bit by saying: "Called by {@link Skinnable#setSkin(Skin)}, after the previous skin, if any, has been uninstalled." I don't have a strong opinion on this. modules/javafx.controls/src/main/java/javafx/scene/control/Skin.java line 80: > 78: * previous skin has been uninstalled via its {@link #dispose()} > method. > 79: * This method allows a Skin to register listeners, add child nodes, > set > 80: * required properties and/or event handlers. It might be good to add a sentence at the end saying: "The default implementation of this method does nothing". 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 When isn't it a 1-to-1 relationship? Is it just for the special case of `PopupControl`? 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"? 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). ------------- PR: https://git.openjdk.org/jfx/pull/845
