On Thu, 27 Oct 2022 17:37:11 GMT, John Hendrikx <jhendr...@openjdk.org> 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 28 additional >> commits since the last revision: >> >> - Merge remote-tracking branch 'origin/master' into 8290844.skin.install >> - 8290844: review comments >> - Merge remote-tracking branch 'origin/master' into 8290844.skin.install >> - 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 >> - 8290844: javadoc >> - Merge branch 'openjdk:master' into 8290844.skin.install >> - 8290844: unit tests >> - ... and 18 more: https://git.openjdk.org/jfx/compare/81a0647c...3235d433 > > modules/javafx.controls/src/main/java/javafx/scene/control/Control.java line > 228: > >> 226: * setting the skin property to a non-null {@code Skin} first checks >> 227: * the return value of {@link Skin#getSkinnable()} against this >> Control, >> 228: * and throws an {@code IllegalArgumentException} if it is not the >> same. > > Suggestion: > > * 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}. > > No need to emphasize this is only in the not null case, as null skins are > documented to be allowed. i like that, thank you. > modules/javafx.controls/src/main/java/javafx/scene/control/Control.java line > 261: > >> 259: throw new IllegalArgumentException("Skin does not >> correspond to this Control"); >> 260: } >> 261: } > > Suggestion: > > if (skin != null && skin.getSkinnable() != Control.this) { > unbind(); > set(oldValue); > throw new IllegalArgumentException("Skin does not correspond > to this Control"); > } i prefer to have one statement per line. ------------- PR: https://git.openjdk.org/jfx/pull/845
