RFR: 8290844: Add Skin.install() method [v9]
Kevin Rushforth
kcr at openjdk.org
Tue Oct 4 16:22:46 UTC 2022
On Tue, 4 Oct 2022 15:58:43 GMT, Jeanette Winzenburg <fastegal 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 20 additional commits since the last revision:
>>
>> - Merge branch 'openjdk:master' into 8290844.skin.install
>> - 8290844: unit tests
>> - 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: review comments
>> - 8290844: review comments
>> - Merge remote-tracking branch 'origin/master' into 8290844.skin.install
>> - 8290844: javadoc
>> - ... and 10 more: https://git.openjdk.org/jfx/compare/ae837393...d954aafc
>
> modules/javafx.controls/src/main/java/javafx/scene/control/Skin.java line 43:
>
>> 41: * <li>uninstalling of the old skin via its {@link #dispose()} method
>> 42: * <li>installing of the new skin via {@link #install()}
>> 43: * </ul>
>
> - this interface is unrelated to any specific implementation of a Skinnable, at least replace control.setSkin with skinnable.setSkin
> - technically, it can't be the setSkin method to have any workload (as already discussed and fixed elsewhere)
>
> aside from these nit-pickings .. don't we overspecify here (and in the doc of install)? From scratch implementations of a Skinnable/Skin pair might decide to do the complete wiring outside of the property, something like
>
> Skinnable skinnable = new ..
> Skin skin = new ...
> if (skinnable.getSkin() != null) skinnable.getSkin().dispose();
> skin.install();
> skinnable.setSkin(skin);
I don't think we would want to go out of our way to enable this, so I prefer the tighter definition of the life-cycle that Andy is proposing. It seems better to have the Control always call `dispose` and `install` rather than provide an option where the application would call it.
-------------
PR: https://git.openjdk.org/jfx/pull/845
More information about the openjfx-dev
mailing list