RFR: 8290844: Add Skin.install() method [v15]

Andy Goryachev angorya at openjdk.org
Thu Oct 27 22:22:29 UTC 2022


On Thu, 27 Oct 2022 17:37:11 GMT, John Hendrikx <jhendrikx at 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


More information about the openjfx-dev mailing list