On Tue, 18 Apr 2023 00:36:22 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> modules/javafx.base/src/main/java/javafx/beans/Observable.java line 107:
>>
>>> 105: * @since 21
>>> 106: */
>>> 107: default Subscription invalidations(Runnable subscriber) {
>>
>> I think re-using `InvalidationListener` may fit better into the existing
>> JavaFX API:
>>
>> interface Observable {
>> Subscription subscribe(InvalidationListener);
>> }
>>
>>
>> The method can then be overloaded for `ChangeListener`,
>> `ListChangeListener`, `SetChangeListener`, and `MapChangeListener` as well.
>> Doing so allows users to use the same functional interfaces they've used
>> before with the `addListener`/`removeListener` APIs.
>>
>> As an added bonus, we don't need to allocate a wrapper for the listeners.
>
> This would depend I think how you want to approach this. I added three
> variants, invalidations, changes and values. Using the same name
> `subscribe`, you need to make sure the lambda's used all have a different
> arity, or you'll need to add a cast to disambiguate them.
>
> I purposely removed the first `Observable` parameter on all of the variants
> because I think it will be a much better match when you want to register a
> listener, and "pipe" it to some existing code. Now we often have to "strip"
> the unnecessary parameter. I can't do this easily for example:
>
> minimumWidthProperty().addListener(this::requestLayout);
> selectedItem.addListener(getSelectionModel()::select);
> modelStatus.addListener(statusLabel.textProperty()::set);
>
> It must be these now:
>
> minimumWidthProperty().addListener(obs -> this.requestLayout());
> selectedItem.addListener((obs, o, n) -> gelectionModel().select(n));
> modelStatus.addListener((obs, o, n) -> statusLabel.setText(n));
>
> The `values` variant is highly useful for this, as `ChangeListener` has a 2nd
> rarely used parameter (oldValue).
>
> Also I think calling unsubscribe on subscriptions should preferably be the
> only way to remove a listener again; reusing the existing listeners would
> allow you to bypass this with `removeListener`.
These example would probably look like this in the near future:
``` java
minimumWidthProperty().addListener(this::requestLayout());
minimumWidthProperty().addListener(_ -> this.requestLayout());
selectedItem.addListener(getSelectionModel()::select);
selectedItem.addListener((_, _, value) -> gelectionModel().select(value));
modelStatus.addListener(statusLabel.textProperty()::set);
modelStatus.addListener((_, _, value) -> statusLabel.setText(value));
You can always ignore the `Observable` parameter, but you can't bring it back
once it's gone. I have often used this parameter, for example when my listener
handles changes of multiple similar properties. Removing functionality just to
make it look a little prettier is not enough justification in my opinion.
If a subscription shouldn't be removable by passing the listener to
`removeListener`, this can easily be achieved by internally wrapping it into
another listener (just what you did, only with the functional interface being
different). However, this might not be the best solution, as it doubles the
number of listener objects when using the subscription API.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1169386664
