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 <nlis...@gmail.com
<mailto:nlis...@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 <hj...@xs4all.nl
<mailto:hj...@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
