On Sun, 9 Jul 2023 22:56:23 GMT, Jose Pereda <jper...@openjdk.org> wrote:
>> The reasoning is that this would be the most logical and convenient way for
>> these subscriptions to work. I think mstr2 said it well, but I'll run them
>> down as well:
>>
>> Invalidation subscribers are called when the property actually becomes
>> invalid; since it may currently be valid, calling the subscriber immediately
>> would send the wrong signal. A case could be made to call the subscriber
>> immediately if it is currently invalid, but I think whatever we choose here,
>> it should work the same way as the current
>> `addListener(InvalidationListener)` -- I'm pretty sure this one doesn't call
>> the listener immediately when the property is currently invalid (this is
>> something of a gotcha as well for beginning users of
>> `InvalidationListener`s).
>>
>> Change subscribers require the old value; this is only temporarily available
>> when there's an actual change in the property. Old values are not cached,
>> and can't be cached as this may prevent garbage collection of whatever the
>> old value references.
>>
>> The value listeners are a new breed, and specifically set up for convenient
>> use. They're also the only ones that can send a sensible initial value, and
>> since I'm pretty sure that's almost always what you'd want, this is the
>> default for this type of subscription.
>
> 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 the other way around.
Alternatively, you can always use a change subscriber if that initial call is
not what you want.
There is some precedent as well, ReactFX, which inspired some of these
enhancements, also has/had this mechanism, and they made the same choice for
value type subscriptions.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1257575641
