Eager Evaluation of Invalidation Listeners

[Nir Lisker]

I think that we need your input on this to move it forward.

On Fri, Sep 3, 2021 at 7:49 AM Nir Lisker <nlisker at gmail.com> wrote:

> so the value field should perhaps be nulled out when bound.
>
>
> There was a PR for something like that in the old repo:
> https://github.com/javafxports/openjdk-jfx/pull/110
>
> On Fri, Sep 3, 2021 at 5:35 AM John Hendrikx <hjohn at xs4all.nl> wrote:
>
>>
>>
>> On 02/09/2021 11:57, Nir Lisker wrote:
>> >     So in order
>> >     to make sure that a new interested invalidation listener does not
>> miss
>> >     the fact that a property was *already* invalid, the easiest solution
>> >     might have been to revalidate it upon registration of a new listener
>> >
>> >
>> > But why does an invalidation listener need to know the past state of the
>> > property? It is only interested in the valid -> invalid transition. If
>> > the property was invalid then the listener (in theory) shouldn't receive
>> > any events anyway on subsequent invalidations. (I understand that you
>> > don't justify this, I'm posing it as a general question.)
>>
>> Strictly speaking, no, if users are using InvalidationListener correctly
>> then this is definitely correct behavior. I'm not really advocating a
>> change, and I'd even prefer that it be brought in line with the
>> documentation.
>>
>> I think however that many users are not using it correctly and expect an
>> invalidation event always the next time the value changes (and their
>> listener will read that value, validating it again), making it act like
>> a light-weight ChangeListener. I know that I probably have written code
>> that made that assumption, and would in the past not even think twice
>> about replacing a change with an invalidation listener or vice versa if
>> that happened to be a better fit. Which is sort of what happened as well
>> in the bidirectional binding PR, and the issue slipped past the author
>> and two reviewers.
>>
>> > I suggest that we split the problem into 2: one is the case where the
>> > property was valid when the listener was attached, and the other is when
>> > it was invalid.
>> > * A valid starting state. In this case attaching a listener shouldn't
>> > need to do anything. A subsequent invalidation event will be sent
>> > regardless. Currently, it is calling get() redundantly.
>>
>> True, the call to get is redundant in this case. Ugly too, calling get
>> and discarding its result, while the intention is to force the property
>> to become valid.
>>
>> > * An invalid starting state. In this case the documentation says that
>> > nothing needs to happen, but get() is called anyway. Here, the
>> > difference is that a subsequent invalidation event is sent in one case
>> > and not in the other. The only way to advance here is to make a design
>> > decision on what should happen, at least that's how I see it.
>>
>> The docs are even more specific I think, they say no more events will be
>> generated until it becomes valid -- it doesn't leave any option open
>> that it could generate events if it wanted to.
>>
>> > As to the implementation of a possible solution, suppose we add the
>> > isValid method. Upon attaching an invalidation listener, if the property
>> > is valid, we can skip the get() call. That solves the valid starting
>> > state issue. The question is what to do if the property is not valid.
>> >
>> > I also noticed an odd design choice in the implementation of properties:
>> > the value field does not update if the property is bound, instead, the
>> > result of the binding is returned and the value field holds an outdated
>> > value (until the property is unbound).
>>
>> Yeah, that might not be a wise decision as that can lead to memory being
>> referenced that users might expect to be freed. I didn't see anywhere
>> defined what will happen to the value of the property when it is unbound
>> again. The current implementation will keep its last value (during the
>> unbind it will take the last value and assign it to its own value
>> field), so the value field should perhaps be nulled out when bound.
>>
>> --John
>>
>