On Thu, 11 Aug 2022 23:03:49 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/1c437875...647ecd6c > > 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". thank you. > 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"? @throws IllegalArgumentException if {@link Skin#getSkinnable()} returns a different {@code Skinnable} is this better? ------------- PR: https://git.openjdk.org/jfx/pull/845
