RFR: JDK-8304439: Subscription based listeners [v2]

[John Hendrikx]

On Wed, 14 Jun 2023 19:26:01 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> 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.foc...
>
> I think the three newly added methods are a good choice. I wonder if we can some up with better names, though. without some verb like "add" or "subscribe" in the name, the name doesn't really indicate that it is adding a new listener to the observable.

I agree that the chosen names `invalidation`, `changes` and `values` are a bit terse.  The whole signature (without reading docs) should make it clear you are creating a subscription, but perhaps we can do better.  The use of `addListener` can be ruled out as it would conflict with the existing method due to having Lambda's with the same arity (the `values` listener would conflict with `addListener(InvalidationListener)`.  Also, an `add` method would probably have users expecting a corresponding `remove` method.

A few ideas listed here:

| invalidation | values | changes |
|---|---|---|
|`subscribe(Runnable)`(*)|`subscribe(Consumer)`(*)|`subscribe(BiConsumer)`(*)|
|`subscribeInvalidations(Runnable)`|`subscribeValues(Consumer)`|`subscribeChanges(BiConsumer)`|
|`invalidationsTo(Runnable)`|`valuesTo(Consumer)`|`changesTo(BiConsumer)`|

(*) May limit future listener types that have same arity, but can still be a good choice

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1232029010