On Thu, 11 Aug 2022 22:47:28 GMT, Kevin Rushforth <[email protected]> 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 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/45c8c4bc...647ecd6c > > 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(); > } Good point! Actually, unbind() is sufficient, as it does the required check. From javadoc: _If the Property is not bound, calling this method has no effect._ Is there another reason to call isBound(), perhaps for clarity? > 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). ok > 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. thank you ------------- PR: https://git.openjdk.org/jfx/pull/845
