Re: [External] : Re: Eager Evaluation of Invalidation Listeners

[Michael Strauß]

1) The specification of Observable states that: "An implementation of
{@code Observable} may support lazy evaluation [...]"

It seems to me that this is an optional feature, and a user of this
API must be aware that any particular Observable implementation may
not support it, or that it chooses to implement a custom strategy to
reduce the number of invalidation events ("[...] should strive to
generate as few events as possible").

This means that a user of this API must expect that any particular
Observable implementation may generate an invalidation event for every
single change, or for changes in particular situations (for example,
after an InvalidationListener was added).

Let me summarize that as follows: Any well-behaved listener must
tolerate any lazy evaluation strategy.

I think this is important, because the "problem" that is being
discussed here can't, by definition, cause incorrect behavior for
users of the API.

2) Currently, adding an InvalidationListener validates the property.
This has implications on what I would see as "local reasoning" vs.
"global reasoning": developers can reason about their code in
isolation, and will generally get it right. Changing it means that
whether some listener code works can be dependent on other parts of
the system (parts that might validate the property, such that the
listener code will "accidentally" pick up the next change to the
property).

>From an API standpoint, I would consider this an unfortunate situation
because in many scenarios it will "just work" and not prompt any
further consideration from developers, even though the code that set
up the listener was incorrectly implemented. That's because a correct
implementation would be similar to the following "protocol":

prop.addListener(observable -> { ... prop.getValue(); ... });
prop.getValue();

Any code that doesn't use this protocol to set up an
InvalidationListener is now potentially incorrectly implemented, and
accidental validation will obscure this defect in most cases. Sure, we
can include this information in the specification of Observable, but
this is so easy to get wrong that even with explicitly mentioning it,
developers will routinely get it wrong. This makes for brittle
codebases and obscure defects, where changes to one part of a system
can unexpectedly and silently affect other parts of the system.

We can avoid all of that by changing the specification as follows: If
an Observable implementation supports lazy evaluation, it must not
elide an invalidation event after new listeners have been added.



On Thu, Oct 14, 2021 at 6:39 PM Nir Lisker  wrote:
>
> 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ß  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
>> 

Re: [External] : Re: Eager Evaluation of Invalidation Listeners

[Nir Lisker]

>
> As mentioned already, if we make any change in this area, it will need
> to be very well-tested, and even then we risk breaking applications.
>

I can make the change and can fix some of the tests (which will probably
mean deleting some of them since they explicitly rely on this behavior).
Some tests could be in areas of the code I'm not familiar with.
As for breaking applications, I'm not sure what we're supposed to do. Ask
people if they misused InvalidationListener?



On Fri, Oct 15, 2021 at 2:12 AM Kevin Rushforth 
wrote:

> That's a good idea to dig into the history further. I looked through the
> closed-source repo, and the behavior of calling get to validate the
> observable when adding an InvalidationListener for bindings has been
> there pretty much from the beginning, even before the creation of the
> ExpressionHelper class (in July 2011).
>
> I looked at when InvaldationListeners were added to the Bindings classes
> (in March 2011), and the initial implementation of that method calls
> get() from the InvalidationListener. That was the commit that also added
> the `testAddingListenerWillAlwaysReceiveInvalidationEvent` test methods.
>
> So this was clearly a deliberate design choice from the beginning. I can
> see the rationale for it, but in retrospect it seems an unfortunate choice.
>
> As mentioned already, if we make any change in this area, it will need
> to be very well-tested, and even then we risk breaking applications.
>
> -- Kevin
>
>
> On 10/6/2021 12:39 AM, John Hendrikx wrote:
> > Is it possible to dig in the history of ExpressionHelper a bit
> > further? In git it seems limited to 9 years ago, but in JIRA I found
> > this bug report:
> >
> > https://bugs.openjdk.java.net/browse/JDK-8119521
> >
> > It describes an issue where an InvalidationListener is not working
> > correctly (according to the reporter) and where #get must be called to
> > make it behave as expected.  But it turns out this was already fixed
> > -- this specific fix might have been the one that introduced the #get
> > call in ExpressionHelper.
> >
> > On 06/10/2021 04:38, Nir Lisker wrote:
> >> I would also answer "no" to both points. This is also what the docs say.
> >>
> >> So the question is: how likely do we think that changing this
> >> behavior will
> >>> break existing applications?
> >>>
> >>
> >> That's the main question. I tested the JavaFX code with the new behavior
> >> and some tests break immediately, though a few I've looked at seemed
> >> to be
> >> testing the invalidation listener behavior itself (in their own
> >> context). I
> >> don't know what would break outside of the tests. If we go this
> >> route, we
> >> might want to create PRs to fix the tests before we actually change
> >> the behavior (in contrast to doing it all in the same PR). I think that
> >> tests fail in several modules and it might require several people to fix
> >> these tests depending on their areas of expertise. Then we would need to
> >> test runtime behavior to see what breaks outside of the tests.
> >>
> >> In my own codebase nothing breaks, but it's relatively small.
> >>
> >> On the related question, I like the idea of nulling out the current
> >> value
> >>> when a property is bound.
> >>>
> >>
> >> I can pick up from where the older PR stopped. It's a simple change.
> >>
> >> On Wed, Oct 6, 2021 at 3:15 AM Kevin Rushforth
> >> 
> >> 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
> 

Re: [External] : Re: Eager Evaluation of Invalidation Listeners

[Kevin Rushforth]

That's a good idea to dig into the history further. I looked through the 
closed-source repo, and the behavior of calling get to validate the 
observable when adding an InvalidationListener for bindings has been 
there pretty much from the beginning, even before the creation of the 
ExpressionHelper class (in July 2011).


I looked at when InvaldationListeners were added to the Bindings classes 
(in March 2011), and the initial implementation of that method calls 
get() from the InvalidationListener. That was the commit that also added 
the `testAddingListenerWillAlwaysReceiveInvalidationEvent` test methods.


So this was clearly a deliberate design choice from the beginning. I can 
see the rationale for it, but in retrospect it seems an unfortunate choice.


As mentioned already, if we make any change in this area, it will need 
to be very well-tested, and even then we risk breaking applications.


-- Kevin


On 10/6/2021 12:39 AM, John Hendrikx wrote:
Is it possible to dig in the history of ExpressionHelper a bit 
further? In git it seems limited to 9 years ago, but in JIRA I found 
this bug report:


    https://bugs.openjdk.java.net/browse/JDK-8119521

It describes an issue where an InvalidationListener is not working 
correctly (according to the reporter) and where #get must be called to 
make it behave as expected.  But it turns out this was already fixed 
-- this specific fix might have been the one that introduced the #get 
call in ExpressionHelper.


On 06/10/2021 04:38, Nir Lisker wrote:

I would also answer "no" to both points. This is also what the docs say.

So the question is: how likely do we think that changing this 
behavior will

break existing applications?



That's the main question. I tested the JavaFX code with the new behavior
and some tests break immediately, though a few I've looked at seemed 
to be
testing the invalidation listener behavior itself (in their own 
context). I
don't know what would break outside of the tests. If we go this 
route, we

might want to create PRs to fix the tests before we actually change
the behavior (in contrast to doing it all in the same PR). I think that
tests fail in several modules and it might require several people to fix
these tests depending on their areas of expertise. Then we would need to
test runtime behavior to see what breaks outside of the tests.

In my own codebase nothing breaks, but it's relatively small.

On the related question, I like the idea of nulling out the current 
value

when a property is bound.



I can pick up from where the older PR stopped. It's a simple change.

On Wed, Oct 6, 2021 at 3:15 AM Kevin Rushforth 


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

Re: [External] : Re: Eager Evaluation of Invalidation Listeners

[Kevin Rushforth]

I agree with Nir on this point. I don't see the middle ground as being 
all that useful in practice, just more complicated, and the 
specification change would be odd.


So I think we either go with the proposed change (and it would need to 
be well-tested), or leave it as-is, where every new invalidation 
listener validates the object being listened to. It would be an easy 
call if it weren't for the concern about potentially breaking 
applications that have baked in assumptions on the existing behavior.


-- Kevin


On 10/14/2021 9:39 AM, Nir Lisker wrote:

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ß > 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
mailto: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 

Re: [External] : Re: Eager Evaluation of Invalidation Listeners

[Nir Lisker]

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ß 
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
>  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  > > > 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  > > > wrote:
> > >
> > >
> > >
> > > On 02/09/2021 11:57, Nir Lisker wrote:
> > > > So in order
> > > > to make sure that a new interested invalidation 

Re: [External] : Re: Eager Evaluation of Invalidation Listeners

[Michael Strauß]

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

Re: [External] : Re: Eager Evaluation of Invalidation Listeners

[John Hendrikx]

Is it possible to dig in the history of ExpressionHelper a bit further? 
In git it seems limited to 9 years ago, but in JIRA I found this bug report:


https://bugs.openjdk.java.net/browse/JDK-8119521

It describes an issue where an InvalidationListener is not working 
correctly (according to the reporter) and where #get must be called to 
make it behave as expected.  But it turns out this was already fixed -- 
this specific fix might have been the one that introduced the #get call 
in ExpressionHelper.


On 06/10/2021 04:38, Nir Lisker wrote:

I would also answer "no" to both points. This is also what the docs say.

So the question is: how likely do we think that changing this behavior will

break existing applications?



That's the main question. I tested the JavaFX code with the new behavior
and some tests break immediately, though a few I've looked at seemed to be
testing the invalidation listener behavior itself (in their own context). I
don't know what would break outside of the tests. If we go this route, we
might want to create PRs to fix the tests before we actually change
the behavior (in contrast to doing it all in the same PR). I think that
tests fail in several modules and it might require several people to fix
these tests depending on their areas of expertise. Then we would need to
test runtime behavior to see what breaks outside of the tests.

In my own codebase nothing breaks, but it's relatively small.

On the related question, I like the idea of nulling out the current value

when a property is bound.



I can pick up from where the older PR stopped. It's a simple change.

On Wed, Oct 6, 2021 at 3:15 AM Kevin Rushforth 
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  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  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 

Re: [External] : Re: Eager Evaluation of Invalidation Listeners

[Nir Lisker]

I would also answer "no" to both points. This is also what the docs say.

So the question is: how likely do we think that changing this behavior will
> break existing applications?
>

That's the main question. I tested the JavaFX code with the new behavior
and some tests break immediately, though a few I've looked at seemed to be
testing the invalidation listener behavior itself (in their own context). I
don't know what would break outside of the tests. If we go this route, we
might want to create PRs to fix the tests before we actually change
the behavior (in contrast to doing it all in the same PR). I think that
tests fail in several modules and it might require several people to fix
these tests depending on their areas of expertise. Then we would need to
test runtime behavior to see what breaks outside of the tests.

In my own codebase nothing breaks, but it's relatively small.

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

I can pick up from where the older PR stopped. It's a simple change.

On Wed, Oct 6, 2021 at 3:15 AM Kevin Rushforth 
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  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  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 

Re: [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 > 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 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