On Fri, 9 Sep 2022 20:47:56 GMT, Andy Goryachev <[email protected]> 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 16 additional > commits since the last revision: > > - 8290844: review comments > - Merge remote-tracking branch 'origin/master' into 8290844.skin.install > - 8290844: review comments > - 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 > - ... and 6 more: https://git.openjdk.org/jfx/compare/6f6de14f...7d5db78b The API looks good. I left a few more minor comments on the docs, and then I think it's ready. The implementation looks good, too. So I think the only thing missing are unit tests. modules/javafx.controls/src/main/java/javafx/scene/control/Control.java line 226: > 224: * <p> > 225: * To ensure a one-to-one relationship between a {@code Control} and > its {@code Skin}, > 226: * {@link Control#setSkin(Skin)} method will check the return value > of {@link Skin#getSkinnable()} Suggestion: either "the setSkin method" or "setSkin" (without "method"). modules/javafx.controls/src/main/java/javafx/scene/control/Control.java line 231: > 229: * A skin may be null. > 230: * > 231: * @return the skin property for this control I recommend adding an `@throws` clause here. Something like: * @throws IllegalArgumentException if {@code skin != null && skin != getSkinnable()} although the javadoc tool currently doesn't do anything with an `@throws` tag in a property (I'll file a follow-up bug for this). modules/javafx.controls/src/main/java/javafx/scene/control/Control.java line 252: > 250: unbind(); > 251: set(oldValue); > 252: throw new IllegalArgumentException("Skin does not > correspond to this Skinnable"); Minor: it might be clearer to say "this Control" here (but `Skinnable` is not wrong) modules/javafx.controls/src/main/java/javafx/scene/control/Skin.java line 83: > 81: * > 82: * @implNote > 83: * Most implementations of Skin in the <code>javafx.controls</code> > module Minor: `{@code some text}` is preferred over `<code>some text</code>` in API docs. modules/javafx.controls/src/main/java/javafx/scene/control/Skin.java line 84: > 82: * @implNote > 83: * Most implementations of Skin in the <code>javafx.controls</code> > module > 84: * do not need to implement {@link Skin#install()} unless they must > set one or more Since this doc is already in the `install` method, there is no need for a link. It can be replaced with: `{@code install}` ------------- PR: https://git.openjdk.org/jfx/pull/845
