jfx 8290844: Add Skin.install() method

added Skin.install()
javadoc changes for Skinnable.setSkin(Skin)

no code changes for Skinnable.setSkin(Skin) yet.

Progress

[ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
[x] Change must not contain extraneous whitespace
[x] Commit message must refer to an issue
[ ] Change requires a CSR request to be approved

Issues

JDK-8290844: Add Skin.install() method
JDK-8292213: Add Skin.install() method (CSR)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/845/head:pull/845
$ git checkout pull/845

Update a local copy of the PR:
$ git checkout pull/845
$ git pull https://git.openjdk.org/jfx pull/845/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 845

View PR using the GUI difftool:
$ git pr show -t 845

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/845.diff

Jul 22 '22 19:07 andy-goryachev-oracle

:wave: Welcome back angorya! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Jul 22 '22 19:07 bridgekeeper[bot]

In a property setter, you cannot do anything other than call property.set(...), since setFoo(...) and fooProperty().set(...) must be equivalent.
As you discovered, ComboBoxPopupControl implements a skin that violates the getSkinnable() specification. But that seems to be completely unnecessary. When I change the implementation to conform to the specification, everything works just as well: all tests pass, and I can run a large application (SceneBuilder) without problems.

Here's the code in ComboBoxPopupControl that I've tested:

private void createPopup() {
    class PopupControlImpl extends PopupControl {
        @Override public Styleable getStyleableParent() {
            return ComboBoxPopupControl.this.getSkinnable();
        }

        PopupControlImpl() {
            setSkin(new Skin<>() {
                @Override public Skinnable getSkinnable() { return PopupControlImpl.this; }
                @Override public Node getNode() { return getPopupContent(); }
                @Override public void dispose() { }
            });
        }
    }

    popup = new PopupControlImpl();
    ....
}

Jul 26 '22 15:07 mstr2

All good points, many thanks! Fixed the setSkin(), changed the javadoc for setSkin() to indicate that this method might throw an IAE.

Jul 26 '22 20:07 andy-goryachev-oracle

Webrevs

Aug 08 '22 20:08 mlbridge[bot]

From a purist perspective, I love this change :) But ..

jumping to the impact analysis (and playing devil's advocate, a bit :)

The changes should be fairly trivial. Only a subset of skins needs to be refactored, and the refactoring itself is trivial.

optimistic 😉 On the bright side: that subset are the harder ones, some being in a hierarchy of skins where every level might do its own evil .. so we can eat our own dog food (dealing with the added life-cyle).

The new API is backwards compatible with the existing code, the customer-developed skins can remain unchanged (thanks to default implementation).

hmm ... probably I'm missing something but I don't see how that would be the case for the subset of core skins that have to be refactored and moved their installation into the install() method. Typically, custom skins do their own stuff on top of what super did at the end of the constructor, assuming that everything is fully installed. Depending on what exactly will be moved into install, their behavior will be broken after the change.

Examples:

  // prepending a listener
  Consumer old = unregisterXXListener(someProperty);
  registerXXListener(someProperty, consumer);
  registerXXListener(someProperty, old);

  // lookup a child added by super 
  Node child = control.lookup(..)
  
  // replace singleton event handlers
  control.setOnXX(...)

  // probably more that I don't remember right now ..

Aug 11 '22 13:08 kleopatra

hmm ... probably I'm missing something but I don't see how that would be the case for the subset of core skins that have to be refactored and moved their installation into the install() method.

You are right - some of the core skins will have to be refactored. As a result, any skin that user derived from those might need to change as well. I shall update the CSR.

This is a price to pay for fixing a design error, I think. Either fix it now, or forever stay with memory leaks and resulting bugs.

Aug 11 '22 19:08 andy-goryachev-oracle

The new API is backwards compatible with the existing code, the customer-developed skins can remain unchanged (thanks to default implementation).

hmm ... probably I'm missing something but I don't see how that would be the case for the subset of core skins that have to be refactored and moved their installation into the install() method. Typically, custom skins do their own stuff on top of what super did at the end of the constructor, assuming that everything is fully installed. Depending on what exactly will be moved into install, their behavior will be broken after the change.

Maybe. As you say, it depends on what is moved from the constructor into install().

Examples:

[snip]

How common do you think this sort of thing is in practice? This seems like a case of the subclass "peeking" at the implementation of its superclass, and then coding in such a way as to rely on that implementation. This does suggest that we ought to take a cautious approach, only overriding and implementing Skin::install for those built-in controls where it is needed to fix bugs. It also suggests that this possible incompatibility ought to be highlighted in a release note (in addition to updating the CSR, which Andy has indicated that he will do).

Aug 11 '22 23:08 kevinrushforth

How common do you think this sort of thing is in practice?

can't do more than guessing (knowing that companies I worked for had done so before I got my hands onto their code) - so would say it's common enough to cause .. well, some distress ;)

This does suggest that we ought to take a cautious approach, only overriding and implementing Skin::install for those built-in controls where it is needed to fix bugs. It also suggests that this possible incompatibility ought to be highlighted in a release note (in addition to updating the CSR, which Andy has indicated that he will do).

Agreed. The part open to discussion is "needed to fix bugs". So far, we have identified (Andy, correct me if I'm wrong - you are nearer to the current state than I am :) only one JDK-8268877 that can't be handled without some kind of api change (which in that particular case might be a marker interface like UIResource in Swing to allow skins to differentiate between control- and skin installed handlers :). There probably are more in skins that are not yet cleaned.

Aug 12 '22 14:08 kleopatra

only one JDK-8268877 that can't be handled without some kind of api change

There might be more, for example MenuButtonSkinBase:96, MenuButtonSkinBase:105, MenuButtonSkin:108. I don't think we have a bug filed for those (yet) - I was planning to go over core skins after we get this PR integrated. Any time a singleton is being set, or conditionally set, we have this problem.

Aug 12 '22 14:08 andy-goryachev-oracle

only overriding and implementing Skin::install for those built-in controls where it is needed to fix bugs. It also suggests that this possible incompatibility ought to be highlighted in a release note (in addition to updating the CSR, which Andy has indicated that he will do).

Umbrella ticket JDK-8241364 provides a list which might be incomplete (I think there are issues in MenuBaseSkin/Base as well).

Aug 12 '22 15:08 andy-goryachev-oracle

did a bit of experimenting, trying to eat and keep the cake at the same time :) Or in terms of the new skin api: have a nice new life-cycle without breaking existing skins

The idea:

let skin have another new method supportsInstall with a default to false
let our concrete skins implement that to test against their exact class
let constructors of concrete skins call install if supportsInstall returns false
custom skins will see the completely installed super if not refactored, or have moved their own installation code into install and overridden supportsInstall to return true

A quick PoC is in my fork off Andy's PR - obviously very raw :) - for TextFieldSkin, tested in SkinCleanupTest: note the uncommented ignores for JDK-8268877 which pass due to Andy's work on install. Directly below those is a test against a custom text skin which expects super to be fully installed at the end of the constructor.

Have a nice weekend, everybody, whenever it begins in your timezone!

Aug 12 '22 17:08 kleopatra

A quick PoC is in my fork off Andy's PR

@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.

Aug 12 '22 18:08 andy-goryachev-oracle

hmm .. with the singleton handler problem (f.i. JDK-8268877) out off the way (a skin must not set such a handler), it seems we are very near to a solution without a problem: none of our skins would want to really implement install (due to the high risk of breaking existing custom skins).

Nevertheless, the idea is good and a step into the right direction. But would be a bit weird not implementing it ourselfves .. what about refactor all our skins (move the complete installation into install, including listeners, children, mutations) and - circling back to the supportsInstall :) - implement the concrete subclasses to self-install in the constructor, if that marker returns false? All our skins extend from SkinBase, so the method might reside there (could be protected) if Skin should remain skinny ..

Aug 15 '22 15:08 kleopatra

@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)

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

Aug 15 '22 15:08 kleopatra

@kleopatra :

Please take a look at MenubuttonSkin:108 In there, we are setting an onAction handler, but only if it's not null. This particular code cannot distinguish between onAction handler set by the user prior to any skin installed, and the handler installed by a previous skin in the case of replacing an existing skin.

How do you propose to fix it?

Even if we invent some kind of inner class to be able to do instanceOf, there is still a possibility of replacing a user-defined skin, where the old skin would set and remove its handler and we'll end up without a handler once new skin is installed.

No amount of hacks can fix a bad design, it seems.

Aug 15 '22 15:08 andy-goryachev-oracle

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. Thank you.

Aug 23 '22 18:08 andy-goryachev-oracle

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 ;)

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

XX being a singleton event handler: it's a bug of the skin to set the singleton handler, instead it should add a handler
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.

Aug 24 '22 15:08 kleopatra

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

But we do! (I this one of my earlier comments did not got lost by Jira, sorry).

In TextInputControlSkin : 334, we have

control.setInputMethodRequests(new ExtendedInputMethodRequests()

this sets a property with a complex object (not a listener). the constructor cannot distinguish between this property set by the user, or by the still attached skin. So if the user wants to set a custom input method requests it will get overwritten.

There is no way around it.

Aug 24 '22 15:08 andy-goryachev-oracle

the constructor cannot distinguish between this property set by the user

true, but currently it doesn't even try to.

So IMO that's not a problematic pattern (as already pointed out in our recent debate): the install sets whatever property unconditionally - to safely cleanup behind itself, the old skin will simply check if the current value is the one it installed itself, if not leave it alone (which it currently doesn't but that's a bug that can be solved without new api).

Aug 24 '22 15:08 kleopatra

@kleopatra , this will fail in the case of changing skins - 0. user sets the property (U)

skinA() sets the property (A)
skinB() sets the property (B)
skinA.dispose() does not reset property to U because it's now == B
skinB.dispose() fails to set the property to U because it does not know the initial value

Aug 24 '22 15:08 andy-goryachev-oracle

trying again: the current state of a (arbitrarily complex property)

 // constructor setting a property unconditionally
 control.setYY(myYY);

 // dispose
 if (getSkinnable().getYY() == myYY) getSkinnable().setYY(myYY);

We agree that setting the property unconditionally is suboptimal, a better (yet to be implemented, RFE required :) way would be to only do so if the control doesn't have one set

  // constructor setting a property as default, only if the control doesn't have its own
  // not possible - can't differentiate control-set from skin-set
  if (control.getYY() == null) control.setYY(myYY);

  // dispose is the same
  if (getSkinnable().getYY() == myYY) getSkinnable().setYY(null);

Implementating that pattern - in a future RFE - requires new API, f.i. separating instantiation from install

  // constructor does nothing related to YY

  // install
  // the constraint getSkinnable().getSkin() == null guarantees that no skin installed property is left
  if (control.getYY() == null) control.setYY(myYY);

Aug 24 '22 15:08 kleopatra

ahh .. at least I seem to see now where we talked past each other:

     YY userYY;
     // install
     userYY = control.getYY();
     control.setYY(myYY);

     // dispose: reset to previous user installed YY
     if (getSkinnable().getYY() == myYY) getSkinnable().setYY(userYY);

not sure if that's anything we want to do, and also not a pattern currently used anywhere in our skins

Anyway, off for today, need food and family :)

Aug 24 '22 16:08 kleopatra

even with the code you just provided, it will fail as described earlier. the problem, as i see it, is that dispose() of the previous skin is executed after the new skin is attached. literally nothing can be done to work around the issues caused by this design mistake, but to fix it outright - dispose() the old skin first, then install the new one.

then, and only then, all the requirements concerning user-settable properties and clean uninstallation can be met.

Aug 24 '22 16:08 andy-goryachev-oracle

Dear @kleopatra :

Are we in agreement that the issue with setting a field (that cannot be reimplemented via add/remove listener) cannot be fixed without adding install() method, or should I provide a clearer example?

I'd like to move this PR forward, if possible.

Thank you.

Aug 31 '22 15:08 andy-goryachev-oracle

the problem, as i see it, is that dispose() of the previous skin is executed after the new skin is attached. literally nothing can be done to work around the issues caused by this design mistake, but to fix it outright - dispose() the old skin first, then install the new one.

then, and only then, all the requirements concerning user-settable properties and clean uninstallation can be met.

I agree with this conclusion. What might help is a concrete example of an application that would hit this bug, and an explanation of how the app would fail. That way we'll know that this new API is enabling the fixing of actual (as opposed to theoretical) problems.

Sep 01 '22 21:09 kevinrushforth

What might help is a concrete example of an application that would hit this bug, and an explanation of how the app would fail.

Let's consider this scenario: an application sets a custom input method on a control, followed by the user loading a new skin via a menu (i.e. after the default skin has been installed).

// 1. control is created control = new Control();

// 2. user wants to provide a custom input method control.setInputMethodRequests(USER1);

// 3. Stage.show applies CSS and creates a default skin control.setInputMethodRequests(SKIN1);

// 4. user loads a new L&F via menu

// 5. new skin is created, constructor sets the control property while the old skin is still attached new UserSkin(control); control.setInputMethodRequests(SKIN2);

// 6. user calls setSkin() to install the new skin oldSkin.dispose() control.setInputMethodRequests(null); // current implementation

The problem here is that the skin code is unable to decide in step 5 whether to overwrite the input method or keep the current value - because it does not know whether the current value was set by the user (step 2) or the old skin (step 3). Furthermore, if we were to add any kind of conditional logic to step 3 and 5, the problem moves down the line to the dispose(0 method in step 6. since at that moment it is unclear whether the current value was set by the user or by the new skin.

Sep 02 '22 21:09 andy-goryachev-oracle

that's basically the problem we identified as not solvable by current api, though I would formulate slightly differently: there are two bugs in our current implementation, one in step 3 and one in step 6

// bug in step 3: should be replacing the property only if not set be the user
if  (control.getInputMethodRequest() == USER1) { // no means to detect user vs. skin installed
         // do nothing
}  else {
        control.setInputMethodRequest(SKIN1);
}

// bug in step 6: nulling unconditionally in dispose is wrong, should only do if installed by the skin
if (control.getInputMethodRequest() == SKIN1) control.setInputMethodRequest(null);

Yes: there is no way to differentiate USER1 from SKIN1 in step 5 without new API :) There are options besides a new life-cycle, but I agree that adding a life-cycle state is the most clean.

Sep 03 '22 10:09 kleopatra

Thank you! I am glad we are on the same wavelength, @kleopatra . Could we get this PR approved? Do you think we should expand the Skin javadoc?

Sep 06 '22 15:09 andy-goryachev-oracle

Could we get this PR approved?

The PR isn't ready to be approved, but the API changes and javadoc are ready to be reviewed.

Do you think we should expand the Skin javadoc?

Quite possibly. Related to this, I wonder whether we want to add an @implNote indicating that most implementations of Skin in the javafx.controls module do not implement Skin::install unless noted? I hesitate to document what is effectively an implementation detail, but sometimes that's what we use @implNote for in cases where a subclass might want to know.

Sep 06 '22 15:09 kevinrushforth

This looks good now, although I want to do a little more testing before approving it.

The next step is to update the CSR with the latest doc changes. For the existing methods and class docs that have been modified, please show the diffs to make it easier to see what is changed.

Sep 29 '22 18:09 kevinrushforth

jfx jfx copied to clipboard

8290844: Add Skin.install() method

Progress

Issues

Reviewing

Webrevs

jfx
jfx copied to clipboard