On Wed, 14 Jun 2023 19:26:01 GMT, Kevin Rushforth <k...@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
