On Tue, 11 Oct 2022 18:50:54 GMT, Andy Goryachev <ango...@openjdk.org> 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<Boolean> noMouseAndInstalled = installed .flatMap(b -> b ? control.onMousePressedProperty() : installed) .map(x -> false) // ---+ use case for isNotEmpty .orElse(true); // --/ Or (if `isNotEmpty` already exists): ObservableValue<Boolean> 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
