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-8119521It 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 willbreak existing applications?That's the main question. I tested the JavaFX code with the new behaviorand 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, wemight 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 valuewhen 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 <kevin.rushfo...@oracle.com>wrote:Given that the intention of InvalidationListener was to be a light-weightway to listen to properties without causing a binding chain to berevalidated, it does seem reasonable to me that the listener is only fired on the valid --> invalid transition, which is the specified behavior, evenif 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 immediateget(), which will ensure that the property is then valid? This will forcean 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 theobject is invalidated)? I think the ideal answer to both questions is "no", although I shareJohn's concern that changing this behavior for InvalidationListeners couldbreak 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 thereare serious concerns, I would support changing this behavior as part ofavoiding eager evaluation when using invalidation listeners. It would needto 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 valuewhen 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> 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> wrote:On 02/09/2021 11:57, Nir Lisker wrote:So in orderto make sure that a new interested invalidation listener does notmissthe fact that a property was *already* invalid, the easiestsolutionmight have been to revalidate it upon registration of a newlistenerBut why does an invalidation listener need to know the past state oftheproperty? It is only interested in the valid -> invalid transition. Ifthe property was invalid then the listener (in theory) shouldn'treceiveany events anyway on subsequent invalidations. (I understand that youdon't justify this, I'm posing it as a general question.)Strictly speaking, no, if users are using InvalidationListener correctlythen 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 aninvalidation event always the next time the value changes (and theirlistener will read that value, validating it again), making it act like a light-weight ChangeListener. I know that I probably have written codethat made that assumption, and would in the past not even think twiceabout 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 authorand two reviewers.I suggest that we split the problem into 2: one is the case where theproperty was valid when the listener was attached, and the other iswhenit was invalid.* A valid starting state. In this case attaching a listener shouldn'tneed 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 propertyto become valid.* An invalid starting state. In this case the documentation says thatnothing needs to happen, but get() is called anyway. Here, thedifference 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 designdecision 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 begenerated 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 thepropertyis valid, we can skip the get() call. That solves the valid startingstate issue. The question is what to do if the property is not valid.I also noticed an odd design choice in the implementation ofproperties: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 outdatedvalue (until the property is unbound).Yeah, that might not be a wise decision as that can lead to memory beingreferenced that users might expect to be freed. I didn't see anywheredefined what will happen to the value of the property when it is unbound again. The current implementation will keep its last value (during theunbind 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
