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

[Jose Pereda]

On Mon, 10 Jul 2023 00:17:44 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> Okay, that makes sense for the InvalidationListener. 
>> 
>> However, for the ChangeListener, as I see it, this PR gives two options, over the same listener: you can care only about the newValue, or you can care about both oldValue and newValue, both from the observableValue. These are then passed to the consumer/biConsumer that defines the ChangeListener, but I don't see any reason why this consumer/biconsumer needs to get initialized with any value at all, does it?
>> 
>> Only bindings apply the initial value: when you bind two properties (unidirectionally), one property will get any future values of the other, starting from the initial value. But I don't see that happening to changeListeners: when you add a changeListener to a property, I don't see the action being invoked with the current value of the property at that moment, only when the property does change from the current value.
>> 
>> In fact, if you run this test:
>> 
>> 
>>     private final StringProperty value = new SimpleStringProperty("Initial");
>> 
>>     @Test
>>     void subscribeConsumerShouldCallSubscriberImmediatelyAndAfterEachChange() {
>>         value.addListener((obs, ov, nv) -> System.out.println("changeListener, nv: " + nv));
>>         value.subscribe(nv -> System.out.println("consumer, nv = " + nv));
>>         value.subscribe((ov, nv) -> System.out.println("biconsumer, nv = " + nv));
>>         value.set("A");
>> }
>> 
>> 
>> you'll get:
>> 
>> 
>>     consumer, nv = Initial
>>     changeListener, nv: A
>>     consumer, nv = A
>>     biconsumer, nv = A
>> 
>> 
>> It feels wrong to see the first printout to me.
>> 
>> My point is that I don't see why we need to call the consumer at all before there is any actual change in the property.
>
> I think it would be better to see value listeners as the listener alternative to what `bind` offers, except not limited to properties. The main purpose of these subscriptions is to keep things in sync, not so much to know when something changed. When you can use `bind` that would be ideal, but not all properties can be bound, and not everything you may want to keep in sync is a property.  `SelectionModel` is a good example; its properties can't be bound, but it does have methods to modify the selection:
> 
>      myModelSelection.subscribe(selectionModel::select);  // sync right away thanks to initial call
> 
> The same goes for other state that you may want to sync, like the example I gave in another reply:
> 
> 
> node.sceneProperty()
>   .flatMap(Scene::windowProperty)
>   .flatMap(Window::showingProperty)
>   .orElse(false)
>   .subscribe(this::subscribeOrUnsubscribeDependingOnVisibility);
> 
> It's much nicer if this gets called immediately, to avoid having to duplicate this code to register the initial subscribers (and this code may need to check the visibility first still... was this UI part added to an existing visible UI?  Then register immediately, if it wasn't then do nothing and let the subscriber above handle it... )
> 
> I think calling subscribers immediately when it is possible and makes sense to do so should be the default.  For change subscribers it makes no sense as there is no change to report; for invalidation subscribers, you could again make the case that it may be good to call them immediately if the property isn't currently valid, if it weren't for the fact that `ExpressionHelper` will make a property always valid when you register an `InvalidationListener` (undocumented) -- and since it will always be valid immediately after subscription, there is no need to do that initial call.  (The `ExpressionHelper` making the property valid when a listener gets added may even have been a quick hack, as it will unnecessarily make something valid, which may result in **all** invalidation listeners getting called down the line, while the intention maybe was to only call the newly registered one if the property was currently invalid... too late to change this now though without breaking lots of code.)
> 
> So I realize this may seem inconsistent, but each subscriber type IMHO can stand on its own and can have a different purpose and different semantics, and I think that in the case of a value subscriber, there is a lot more benefit from calling the subscriber immediately than th...

Thanks for the clarifications. 
If I get it right, you talk about `invalidation subscriber` for `subscribe(Runnable)` API, `value subscriber` for `subscribe(Consumer)` API and `change subscriber` for `subscribe(BiConsumer)` API, and that's fine, but for a developer that approaches this new API, there is no indication of that (API naming, javadoc), so that's why I'm a little bit concerned about this different in behaviour of the value vs change subscribers.

As commented before, probably the javadoc for the three `subscribe` methods should be extended a little bit more with a mention of the listener involved, and with a small sample.

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

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