On Tue, 11 Oct 2022 18:50:54 GMT, Andy Goryachev wrote:
>>> You are right, @kleopatra - this executeOnceWhenPropertyIsNonNull() is
>>> installing a listener. perhaps we should add a similar functionality to
>>> LambdaMultiplePropertyChangeListenerHandler and actually install a
>>> WeakListener instead.
>>
>> - yes, we need weakListeners (to wait for a not-null value to add the event
>> handler) _and_ remove both the weakListener and the event handlers in dispose
>> - adding functionality to the LambdaHandler sounds like a good idea, but
>> needs some thinking and should be done in a separate issue which also adds
>> delegate methods to expose the new functionality in SkinBase for use in sub
>> classes
>>
>> For this issue, you might simply inline the utility method and register the
>> listeners using skinbase api (didn't try, it's your issue :), see
>> TableRowSkin for a similar pattern
>>
>> This conditional adding of handlers/listeners is needed if we have to
>> support path properties (also f.i. when listening to selectedXX in any of
>> the skins with selectionModels as properties). Which basically should be
>> supported by base - but probably without any chance to ever get them ;)
>
> ListenerHelper - replacement for LambdaMultiplePropertyChangeListenerHandler .
> Please take a look at https://github.com/openjdk/jfx/pull/908
> Sure, but I think this should be done with a clear vision of where we're
> going, and not just as a collection of ad-hoc helper methods. There are even
> more problems that a new API should at least consider: Some skins change
> their behavior depending on the configuration of their associated control at
> the time when the skin is installed. For example, `MenuButtonSkinBase` adds a
> `MOUSE_PRESSED` handler only if the control didn't specify one for its
> onMousePressed property:
>
> ```
> if (control.getOnMousePressed() == null) {
> control.addEventHandler(MouseEvent.MOUSE_PRESSED, ...
> ```
>
> But what happens if the `onMousePressed` property is set after the skin is
> installed? Rather than a one-time check, cases like these should probably be
> a dynamic expression:
>
> ```
> when control.onMousePressedProperty().isNotNull() and skin.isInstalled()
> control.addEventHandler
> otherwise
> control.removeEventHandler
> ```
I think you meant `isNull()` instead of `isNotNull()`:
when control.onMousePressedProperty().isNull() and skin.isInstalled()
control.addEventHandler
otherwise
control.removeEventHandler
So... I was curious, and was checking to see if this can be done with fluent
bindings these days, and I can get quite close. First assume we have a
convenient property in all skins (or everything that is `SkinBase`) that
reflects whether or not the skin is currently installed or active:
BooleanProperty installed = new SimpleBooleanProperty();
You can then create a condition:
ObservableValue noMouseAndInstalled = installed
.flatMap(b -> b ? control.onMousePressedProperty() : installed)
.map(x -> false) // ---+ use case for isNotEmpty
.orElse(true);// --/
Or (if `isNotEmpty` already exists):
ObservableValue noMouseAndInstalled = installed
.flatMap(b -> b ? control.onMousePressedProperty() : installed)
.isNotEmpty();
It's then a simple matter of adding a listener to this result to either add or
remove the event handler:
installed
.flatMap(b -> b ? control.onMousePressedProperty() : installed)
.isNotEmpty();
.addListener((obs, old, active) -> {
if(active) control.addEventHandler(...);
else control.removeEventHandler(...);
});
One of the nice things here is that when `installed` is `false`, the listener
on `control.onMousePressedProperty()` is automatically removed. This is
because the `flatMap` will switch to listening to `installed`. This should
mean that it will automatically be eligible for GC when `installed` becomes
`false`.
The second nice thing is that you can do this in the constructor already, no
need to do this in the `install` callback. If you do it in the callback, then
make sure to do it before `installed` is set to `true` as otherwise the change
listener won't fire for the first change (another reason to have value
listeners...)
If this becomes a common idiom, we could introduce a short circuiting `and`
construct so the `flatMap` can be expressed easier (the short circuiting part
will ensure the onMousePressedProperty won't be observed if installed is
`false`):
installed
.and(control.onMousePressedProperty().isNotEmpty())
.addListener( ... );
-
PR: https://git.openjdk.org/jfx/pull/906