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
