Dealing with nested events for a single property in JavaFX

[Daniel Zwolenski]

Just to add to my previous comment, I don't think this is really about
'Event events' but more about property changes and it just so happens that
some Event events are triggered via property changes. I think the JIRA
issue alludes to this too:

The ComboBox onAction event is called only when the value changes, so what
you say in your first paragraph [i.e. <<changing a property from within a
property event>>] is still valid for onAction.

I think this is pretty important to take note of, but Jonathan might
correct me on this if I am wrong.


Interestingly, I just had a look at what Swing does, because surely it's
faced with similar problems.

With raw PropertyChangeListeners, it works exactly like we have it in JFX
now. i.e. nested event handling (aka the "onion" approach) where you can
change the Property from within the propertyChangeListener and it will
immediately change it and handle all listeners for the new change before
finishing off the original change.

Meanwhile with the Document of a JTextField, if I try to change the text
field's value from within a 'update' event, it throws a
"java.lang.IllegalStateException: Attempt to mutate in notification". i.e.
the "wrist-slap" approach.

Funnily enough focus handling is different again. It uses the
requestFocus() method which I guess defers it, kind of like
platform.runLater(). i.e. the "queue" approach.

Not that Swing is the template for success here but I thought it would give
a reference point. Turns out it gives us a reference point for lots of
options, which is probably even more confusing.

How did Swing get away with this then? My guess: I think this sort of
nesting is not a huge problem in traditional apps because the events are
not so interwoven. It's the new Properties and all the interconnected
listeners that have made this far more noticeable in JFX. You can't really
move too far in JFX without triggering a property update which may then
ripple on to any number of listeners.


On Fri, Jun 22, 2012 at 10:36 AM, Richard Bair <richard.bair at oracle.com>wrote:

> I don't recursion is the right phrase, but we fire the new event
> immediately. The problem with this is that two guys might be listening to
> the same event. The first guy is notified and changes the state of the
> thing, causing a new event to fire. The second guy gets this new event
> first, and reacts to it. The second guy then gets the first event and
> reacts to it. From his point of view, the second event happened first, and
> the first second. So we end up with a hopeless situation for the poor guy
> in a loosely coupled app. He doesn't know that he handled events out of
> ordered, and without violating encapsulation, he can't know.
>
> Unless of course we have a timestamp on the event object and throw away
> events that occurred after the one he most recently handled.
>
> Richard
>
> On Jun 21, 2012, at 5:17 PM, steve.x.northover at oracle.com wrote:
>
> > Exactly what is happening now?  Are we recursing ?  If so, although
> normally unwanted, I believe this is *correct*.  It allows very smart
> people to make a change, ignore the event caused by it and keep going by
> stacking a boolean state within a listener.  If we queue, then how do they
> do this?
> >
> > Steve
> >
> > On 21/06/2012 7:38 PM, Daniel Zwolenski wrote:
> >> Jonathan's second JIRA (http://javafx-jira.kenai.com/browse/RT-17772)
> uses
> >> a TextProperty with a ChangeListener, i.e. it is not limited to 'Event
> >> events'.
> >>
> >>
> >> On Fri, Jun 22, 2012 at 9:30 AM, Richard Bair<richard.bair at oracle.com
> >wrote:
> >>
> >>> No I believe the value would still be "true" immediately. And to be
> clear
> >>> I'm talking about Event events here, not change notification
> listeners. So
> >>> ActionEvent, MouseEvent, etc.
> >>>
> >>> But that does give pause to consider listeners as well. I don't think
> you
> >>> can queue those up. So specifically I think this issue is about
> whether:
> >>>
> >>> node.fireEvent(newEvent)
> >>>
> >>> is layered or queued, without having to be placed on the actual
> EventQueue
> >>> (ie: runLater).
> >>>
> >>> Richard
> >>>
> >>> On Jun 21, 2012, at 4:18 PM, Daniel Zwolenski wrote:
> >>>
> >>> It's not totally clear: I assume you are talking about a queue per
> >>> property? Or do you mean one big queue for all properties?
> >>>
> >>> In either case I think we introduce a potential problem with this.
> Let's
> >>> say we have:
> >>>
> >>>   BooleanProperty one;
> >>>   BooleanProperty two;
> >>>   two.bind(one);
> >>>
> >>> Then in normal code we do this:
> >>>
> >>>   one.set(true);
> >>>   two.get(); // value is 'true' as a result of binding
> >>>
> >>> But if we do the exact same code in a callback it has a different
> result:
> >>>
> >>>   one.onValueChanged() {
> >>>     one.set(true);
> >>>     two.get(); // value is 'false' as event hasn't been processed yet
> >>>   }
> >>>
> >>> With a queue per property the problem is limited to only when you are
> >>> listening on your own value and just maybe we could get away with that
> (I'm
> >>> not convinced though, 'feels' dangerous). With one big queue it is
> really
> >>> nasty as it would totally change the semantics of code depending on
> whether
> >>> it was being called from within a property callback or not.
> >>>
> >>>
> >>>
> >>> On Fri, Jun 22, 2012 at 8:54 AM, Richard Bair<richard.bair at oracle.com
> >wrote:
> >>>
> >>>> Right, so what it comes down to is: do we layer events, or queue
> events?
> >>>> RunLater is wrong because we might insert a pulse between events on
> the
> >>>> queue so you will render things incorrectly for some brief period of
> time.
> >>>>
> >>>> It seems like handling events sequentially as they occur is the right
> >>>> semantic, rather than the "onion" approach where partway through
> handling
> >>>> event 1 we issue event 2. The problem here is that event 2 is
> handled, and
> >>>> then the rest of event 1 is handled, which now has the wrong state
> (if it
> >>>> was carrying state related to that first event.
> >>>>
> >>>> Of course by queuing events it is possible people will end up with
> >>>> handling so many events that the app thread is always busy and not
> >>>> processing the queue -- but then they can get into infinite recursion
> and
> >>>> all kinds of other issues, so I don't see that as a problem but just
> the
> >>>> nature of the beast.
> >>>>
> >>>> Richard
> >>>>
> >>>> On Jun 21, 2012, at 3:47 PM, Jonathan Giles wrote:
> >>>>
> >>>>> The point is: can we have a better solution by considering changing
> the
> >>>> event API such that it queues events, rather than do them immediately
> as
> >>>> they are fired.
> >>>>> -- Jonathan
> >>>>>
> >>>>> On 22/06/2012 10:46 a.m., Slavko Scekic wrote:
> >>>>>> So, the point is: be careful? :)
> >>>>>>
> >>>>>> On Fri, Jun 22, 2012 at 12:39 AM, Jonathan Giles<
> >>>> jonathan.giles at oracle.com<mailto:jonathan.giles at oracle.com>>  wrote:
> >>>>>>    runLater is wrong more often than it is right - so you're right -
> >>>>>>    I don't think we should consider that approach (or requiring
> >>>>>>    people to have to know to do this)
> >>>>>>
> >>>>>>    Saying that you should error out when someone tries to update a
> >>>>>>    property in the event callback is also wrong, I think. There are
> >>>>>>    legitimate use cases, for example as a last ditch form of
> >>>>>>    validation: if the value is outside the allowable range, change
> it
> >>>>>>    back to something that is valid. Alternatively, when focus is
> >>>>>>    given to a node, request a focus change on another node. I
> imagine
> >>>>>>    our own code would blow up if we took this approach :-)
> >>>>>>
> >>>>>>    -- Jonathan
> >>>>>>
> >>>>>>
> >>>>>>    On 22/06/2012 10:31 a.m., Daniel Zwolenski wrote:
> >>>>>>
> >>>>>>        Similar to the ConcurrentModification thing you get with
> >>>>>>        Collections right? Could it be handled in a similar way, i.e.
> >>>>>>        throw an error if someone tries to update the property they
> >>>>>>        are modifying while in the update callback for that property?
> >>>>>>        As you say, it's user error, so slapping them on the wrists
> is
> >>>> ok.
> >>>>>>        The runLater one feels like it could cause its own problems
> to
> >>>>>>        me. The 'single' threadedness of JFX is part of it's design.
> >>>>>>        It gives me deterministic behaviour, this feels like it could
> >>>>>>        open up small cracks in that. Obviously we wouldn't get
> >>>>>>        concurrency/deadlock issues but I suspect we could get things
> >>>>>>        in a non-deterministic order as a result of this (e.g. if
> >>>>>>        another thread does a runLater somewhere else in the code at
> >>>>>>        the same time this runLater is being added). Could end up
> that
> >>>>>>        my property change is overwritten sometimes but not others,
> >>>>>>        etc (I'm going more off gut feel here though than concrete
> >>>>>>        examples).
> >>>>>>
> >>>>>>
> >>>>>>        On Fri, Jun 22, 2012 at 8:20 AM, Jonathan Giles
> >>>>>>        <jonathan.giles at oracle.com<mailto:jonathan.giles at oracle.com>
> >>>>>>        <mailto:jonathan.giles at oracle.com
> >>>>>>        <mailto:jonathan.giles at oracle.com>>>  wrote:
> >>>>>>
> >>>>>>           Hi all,
> >>>>>>
> >>>>>>           I'm going to keep this brief as I'm fairly comprehensively
> >>>>>>           underwater on the bug count.
> >>>>>>
> >>>>>>           Recently I've found a pattern of bug that, well, I'm
> fairly
> >>>>>>        sure
> >>>>>>           is due to user error, but is not obvious at all (and as
> such
> >>>> it
> >>>>>>           leads to bug reports). In the last week, I've encountered
> >>>> this
> >>>>>>           issue twice. The basic issue is that of listening to an
> >>>>>>        event (for
> >>>>>>           example, a focus change event), and reacting in such a way
> >>>>>>        as to
> >>>>>>           modify the state of this property (which results in
> another
> >>>>>>        event
> >>>>>>           being fired). The end result is non-deterministic (but
> often
> >>>>>>           broken behavior). Interestingly, it has in both my cases
> >>>>>>           manifested itself as something that works once, and then
> >>>> fails
> >>>>>>           after that forever more.
> >>>>>>
> >>>>>>           In both cases, after much code digging and debugging
> >>>> (although
> >>>>>>           today was made much easier by the same issue last week), I
> >>>>>>        believe
> >>>>>>           the issue can be worked around simply by wrapping the
> change
> >>>> to
> >>>>>>           the property state (in the event callback) with a
> >>>>>>           Platform.runLater(new Runnable() { ...}). This forces the
> >>>>>>        second
> >>>>>>           property update to happen after the first event has
> finished
> >>>>>>           firing (at some point in the future).
> >>>>>>
> >>>>>>           However, this isn't a great solution - we're forcing the
> >>>>>>        event to
> >>>>>>           fire at a later date where the state may have already
> >>>>>>        changed. The
> >>>>>>           better solution, in my opinion, is to improve the event
> >>>> system
> >>>>>>           such that it knows whether an event is already firing, and
> >>>>>>        if so
> >>>>>>           it will queue up the event to run after the current one
> has
> >>>>>>           finished. I would be interested in hearing whether anyone
> >>>>>>        else has
> >>>>>>           encountered this kind of bug, or whether they have better
> >>>>>>        suggestions.
> >>>>>>
> >>>>>>           You can see two examples of this bug in the code attached
> >>>> here
> >>>>>>           (where the first example is for ComboBox where the value
> is
> >>>>>>           updated in the onAction callback....which is called when
> >>>> value
> >>>>>>           changes):
> >>>>>>
> >>>>>>        http://javafx-jira.kenai.com/browse/RT-22478
> >>>>>>        http://javafx-jira.kenai.com/browse/RT-17772
> >>>>>>
> >>>>>>           -- Jonathan
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
>
>