[External] : Re: Eager Evaluation of Invalidation Listeners

[Kevin Rushforth]

Given that the intention of InvalidationListener was to be a 
light-weight way to listen to properties without causing a binding chain 
to be revalidated, it does seem reasonable to me that the listener is 
only fired on the valid --> invalid transition, which is the specified 
behavior, even if the implementation doesn't currently do that in all cases.

The two related questions then are:

1. Should adding an invalidation listener to property do an immediate 
get(), which will ensure that the property is then valid? This will 
force an eager evaluation of the property and "arm" the property to fire 
an invalidation even the next time it is invalidated.

2. Should adding an invalidation listener to a currently invalid 
property cause the listener to be called (either immediately or the next 
time the object is invalidated)?

I think the ideal answer to both questions is "no", although I share 
John's concern that changing this behavior for InvalidationListeners 
could break applications. So the question is: how likely do we think 
that changing this behavior will break existing applications?

I think it's something we can and should consider changing. Unless there 
are serious concerns, I would support changing this behavior as part of 
avoiding eager evaluation when using invalidation listeners. It would 
need to be well tested (of course), and would need a CSR describing the 
compatibility risk, and should probably get a release note.

Any concerns in doing this?

On the related question, I like the idea of nulling out the current 
value when a property is bound.

-- Kevin


On 9/11/2021 9:41 AM, Nir Lisker wrote:
> 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 
> <mailto: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
>     <https://urldefense.com/v3/__https://github.com/javafxports/openjdk-jfx/pull/110__;!!ACWV5N9M2RV99hQ!bIbtLsC0tg02c9a_lgKnM1Xta2USX8QkKRL4imOUSw8xshJsVquOeulplJR7dfEzQUf6$>
>
>     On Fri, Sep 3, 2021 at 5:35 AM John Hendrikx <hjohn at xs4all.nl
>     <mailto: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
>