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

[Jeanette Winzenburg]

On Mon, 15 Aug 2022 15:37:15 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>>> A quick PoC is in [my fork off Andy's PR](https://github.com/kleopatra/jfx/tree/exp-skin-install-supportsInstall)
>> 
>> @kleopatra : 
>> Thank you so much for your effort to research the alternatives.
>> 
>> The main issue that necessitates creation of install() and moving it outside of the skin's constructor is the fact that certain code just cannot be inside of the skin's constructor - because the old skin is still attached.  So it must be called after the old skin has been removed in setSkin().  I don't think there is a way around it.
>> 
>> Those user-defined skins (and the affected core skins) that modify singletons or rely on internals of the superclass - must be refactored.  The others, which only add listeners and children nodes, can remain unchanged.
>
>> 
>> @kleopatra : Thank you so much for your effort to research the alternatives.
>> 
> 
> thinking about alternatives is our job, isn't it :)
> 
>> The main issue that necessitates creation of install() and moving it outside of the skin's constructor is the fact that certain code just cannot be inside of the skin's constructor - because the old skin is still attached. So it must be called after the old skin has been removed in setSkin(). I don't think there is a way around it.
> 
> There might be such code, but I doubt that there is anything (at least nothing validly installed) in our skins. Haven't looked into the install children that are defined in the control itself (like f.i. editors in the combo/spinner et al).
> 
>> 
>> Those user-defined skins (and the affected core skins) that modify singletons or rely on internals of the superclass - must be refactored. 
> 
> As long as custom classes play by the rules (aka: don't violate the spec) we'll have a hard time imposing code changes onto custom classes. Or if we do, we might see us in the middle of many pointed fingers. Like (from [kinds of compatibilty](https://wiki.openjdk.org/display/csr/Kinds+of+Compatibility))
> 
>> While an end-user may not care why a program does not work with a newer version of a library, what contracts are being followed or broken should determine which party has the onus for fixing the problem

> I'd like to move forward with this PR. Despite many insightful suggestions from @kleopatra , I don't think there is a way around this problem except to create a new method. 

Not at the end of my insight - in particular, since you seem to circle back to your very first assumption, despite out lengthy debate in [JDK-8268877](https://bugs.openjdk.org/browse/JDK-8268877) ;)

Fact is that not one of our current skins will have an install different from the empty default implementation. So I agree - there is no reason for any bridging api (as f.i. supportsInstall), and retract that suggestion.

But that also means, that there is _no_ problem in our _current_ skin that _requires_ such new api.

We identified a single pattern that will, though: given a skin wants to set a default value to a control's property only if that value is __not__ set by the control itself, that is something like

      if (control.getXX() == null) {
              control.setXX(myDefault);
      }

then it's not possible to safely install and cleanup that value with the current api.

But: turned out that we currently don't have such a case :) We have 

1. XX being a singleton event handler: it's a bug of the skin to set the singleton handler, instead it should add a handler
2. for all other XX, the value is set unconditionally, no matter its current value

If we want to change 2. to doing so conditionally (only if not set by the control/client code) - which we both consider sane behavior - then we _require_ some new API.

-------------

PR: https://git.openjdk.org/jfx/pull/845