On Mon, 10 Apr 2023 19:18:42 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> John Hendrikx has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Move Subscription method for invalidations to Observable
>>
>> - moved Subscription class to the Observable package
>
> 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`.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1169379284
