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> 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 <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 >> >
