I disagree with this interpretation. Observable says
> Implementations in this library mark themselves as invalid when the first > invalidation event occurs. They do not generate anymore invalidation events > until their value is recomputed and valid again. And ObservableValue says An ObservableValue generates two types of events: ... An invalidation event > is generated if the current value is not valid anymore. Implementations in this library mark themselves as invalid when the first > invalidation event occurs. They do not generate any more invalidation > events until their value is recomputed and valid again. It's clear that the validity is with respect to the value, not the listener. There is 1 value and it is either valid or invalid. If we want to define validity on a per-listener basis, the docs would need to be changed too. I don't know how much sense it makes practically because I don't think anyone used them with this intention in mind. It could be a middleground to bridge the current "negligence" with the stricter docs, but it's a more fundamental change conceptually. On Wed, Oct 6, 2021 at 8:02 PM Michael Strauß <michaelstr...@gmail.com> wrote: > The documentation of `Observable` states that "an implementation [...] > may support lazy evaluation" and that "implementations [...] should > strive to generate as few events as possible". > This seems to me like it would be within spec to fire an invalidation > event for every single change. It would also be within spec to fire > redundant invalidation events only in certain scenarios (like adding a > listener). > > The problem could also be approached from a different angle: what does > it mean for a property to be "valid"? > As implemented, the "valid" state is an opaque and unknowable > implementation detail. We could re-define "valid" to mean: valid from > the perspective of an InvalidationListener. > A newly-added InvalidationListener wouldn't know that the property is > invalid (and has no way of knowing), and can therefore reasonably > assume that, from its perspective, the property is valid. It would > receive an invalidation event when the property value is changed. > From the perspective of pre-existing listeners, however, the property > could well have been invalid all the time, so they won't receive an > invalidation event. > > On Wed, Oct 6, 2021 at 2:16 AM Kevin Rushforth > <kevin.rushfo...@oracle.com> wrote: > > > > 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 > > > > > >
