On Mon, 10 Jul 2023 00:17:44 GMT, John Hendrikx <[email protected]> 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
