On Tue, 18 Apr 2023 00:54:53 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> 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.
Yeah I'm aware they want to add this, but it doesn't really invalidate my point
as that syntax is terrible still. Method references are far preferred whenever
possible.
I stand by my point that using the observable parameter is a rare and quite
advanced use case, where these API's would be intended to make it easier and
safer to use method references and lambda's.
When dealing with multiple listeners that require the `Observable`, you are
likely also dealing with adding and removing that single listener in some
dynamic fashion. Subscriptions don't work well with this as you'd need to
track these, unlike `removeListener` which you can just pass the single
listener reference.
Here's one of the only examples in JavaFX that uses the observable parameter
(if you know of others, I'd be interested to see those as well):
private final ChangeListener<Boolean> paneFocusListener = new
ChangeListener<>() {
@Override public void changed(ObservableValue<? extends Boolean>
observable, Boolean oldValue, Boolean newValue) {
if (newValue) {
final ReadOnlyBooleanProperty focusedProperty =
(ReadOnlyBooleanProperty) observable;
final TitledPane tp = (TitledPane)
focusedProperty.getBean();
focus(accordion.getPanes().indexOf(tp));
}
}
};
And its management code:
private final ListChangeListener<TitledPane> panesListener = c -> {
while (c.next()) {
if (c.wasAdded()) {
for (final TitledPane tp: c.getAddedSubList()) {
tp.focusedProperty().addListener(paneFocusListener);
}
} else if (c.wasRemoved()) {
for (final TitledPane tp: c.getRemoved()) {
tp.focusedProperty().removeListener(paneFocusListener);
}
}
}
};
To do this with `Subscription::unsubscribe` you'd need to track the
`Subscriptions`... something like:
private final Map<Property<?>, Subscription> subscriptions = new
HashMap<>();
private final ListChangeListener<TitledPane> panesListener = c -> {
while (c.next()) {
if (c.wasAdded()) {
for (final TitledPane tp: c.getAddedSubList()) {
subscriptions.put(tp.focusedProperty(),
tp.focusedProperty().changes(paneFocusListener));
}
} else if (c.wasRemoved()) {
for (final TitledPane tp: c.getRemoved()) {
subscriptions.remove(tp.focusedProperty()).unsubscribe();
}
}
}
};
That more than likely goes against the reason for using a single listener in
the first place. Even if `removeListener` could be used still (a question that
would need to be answered as it could limit our options later on), you'd be
creating a `Subscription` only to discard it. Now, I can imagine there might
be use cases that don't need this kind of dynamic listener management but still
need the observable parameter, but that would only make the use of that
parameter in combination with subscriptions even more rare, and you'd still
need the current API for some cases.
This is why I would really advocate for a simpler API, focusing on the use
cases most often encountered by users of properties and controls, instead of
cluttering these to also try to cater for control and property authors that
(when dealing with performance optimizations that require the observable) are
likely to need the existing methods anyway.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1172255804
