Re: [lmi] Suspected wx-2.8.9 regression

[Vaclav Slavik]

Hi,

sorry for the detail, it took me much longer than I expected to
understand what's happening; I think I do now. To compensate for that, I
don't understand how could it work before. As a reminder, here's what
happens:

On Sun, 2008-12-28 at 02:31 +0000, Greg Chicares wrote:
> Start lmi
> File | New | Illustration
> Go to "Payments" tab
> In "Dumpin" field, type a negative number like -1
> Hit Tab
> 
> A messagebox appears:
> 
> Assertion '!unit_test_refocus_event_pending_' failed.
> [file /lmi/src/lmi/mvc_controller.cpp, line 670]
> 
> To see the desired behavior: do the same thing, but hit
> Enter instead of Tab: in that case,
>   "-1 is too low: 0 is the lower limit."

The "only" problem is the assertion failure, if it weren't for this (and
one other) LMI_ASSERT line, validation error would still show up in the
static control at the bottom.

This code ensures that focus doesn't leave a control with invalid data.
Because focus change cannot be prevented in wx, it changes focus back to
the control that had it before. The sequence of actions goes like this:

  1. UponChildFocus() detects validation error and posts
  2. it posts wxEVT_REFOCUS_INVALID_CONTROL to the pending events queue
     (i.e. to be processed at idle time, right after emptying "real"
     events queue)
  3. eventually, UponRefocusInvalidControl() is called to handle this
     event and set focus back to the last focused control (i.e. the
     one that failed validation)

To check that the code behaves as expected, there are test asserts: on
validation error, UponChildFocus() sets unit_test_refocus_event_pending_
= true and UponRefocusInvalidControl() resets it back to false when its
done. UponChildFocus() checks the flag to verify that the app isn't
already in the process of refocusing.

This last bit is what fails: MvcController receives *two*
wxChildFocusEvents for the "Dumpin" control before it processes the
wxEVT_REFOCUS_INVALID_CONTROL posted in reaction to the first one of
them. I believe this is a bug in this testing code, not wx 2.8.9: wx
doesn't guarantee that pending events are processed during
wxChildFocusEvent propagation. On the contrary, it is reasonable to
expect that they are _not_, because wx consistently propagates events
with direct ProcessEvent() calls and not wxPostEvent() internally.

Let me explain in details how is wxChildFocusEvent event propagated. As
an example, let's use this hierarchy: 

  MvcController
     input_notebook
        payment_panel
           External1035ExchangeAmount

When focus changes to External1035ExchangeAmount, wxMSW sends
wxChildFocusEvent to the control that gained focus (i.e.
External1035ExchangeAmount). Because wxChildFocusEvent is a
wxCommandEvent, it gets propagated upwards until a handler is found for
it _and_ it doesn't call Skip() on the event. As it happens, all
wxChildFocusEvent handlers involved do call Skip() and so the event is
always propagated up to MvcController.

But there's an *additional* mechanism for wxChildFocusEvent propagation:
this event is meant to inform a controls container (say, wxPanel) that
its *immediate* child obtained focus. So in our example, payment_panel
should receive an event informing it that External1035ExchangeAmount got
focus, but input_notebook should also receive a notification that
*payment_panel* got focus. wxControlContainer's event handler
synthesizes a new wxChildFocusEvent for this purpose.

Here's the full sequence of events propagation in our example:

* wxChildFocusEvent(External1035ExchangeAmount) is sent to
  External1035ExchangeAmount
* External1035ExchangeAmount has no handler, so the event goes to
  its parent, payment_panel
* payment_panel is wxControlContainer and has a handler, which is called
    * this handler creates a new wxChildFocusEvent(payment_panel) and
      sends it to its parent
    * input_notebook has no handler, event goes to its parent
    * MvcController::UponChildFocus() is called, with win=payment_panel
        * wxEVT_REFOCUS_INVALID_CONTROL is added to pending events list
* the original event was Skip()ed by wxControlContainer's handler, so
  its propagation continues to input_notebook
* input_notebook has no handler, event goes to its parent
* MvcController::UponChildFocus() is called *again*, with
  win=External1035ExchangeAmount
    * another wxEVT_REFOCUS_INVALID_CONTROL is posted
    * therefore, LMI_ASSERT() in it fails, because
      wxEVT_REFOCUS_INVALID_CONTROL wasn't handled yet
...
* some time later, events queue is emptied and pending events are
  processed
    * MvcController::UponRefocusInvalidControl() is called
    * MvcController::UponRefocusInvalidControl() is called again
  
In short, UponChildFocus() incorrectly assumes it will be called exactly
once per focus change.

BTW, on a related note, there's the following comment above
UponChildFocus():

        /// WX !! It seems surprising that calling GetWindow() on the
        /// wxChildFocusEvent argument doesn't return the same thing as
        /// FindFocus(): instead, it returns a pointer to the notebook
        tab.
        
This is intended and documented behavior of wxChildFocusEvent:

        A child focus event is sent to a (parent-)window when one of its
        child windows gains focus, so that the window could restore the
        focus back to its corresponding child if it loses it now and
        regains later.
        
        Notice that child window is the direct child of the window
        receiving event. Use FindFocus to retrieve the window which is
        actually getting focus. 
        
(This makes sense in light of the above description of how are
wxChildFocusEvents propagated.)

> appears in the static control at the bottom of the dialog.
> That's the way it worked for Tab as well as Enter, with some
> earlier version of wx-2.8.x (I can't easily say exactly which
> wx version, but suspect it was approximately wx-2.8.3).

This is the confusing part for me, because I couldn't find any related
changes -- as far as I can tell, the logic is unchanged in wx since May
2002 and since forever in LMI CVS. It could also be explained by changes
in XRC code (the intermediary "payment_panel" window between the
"Dumpin" control and MvcController causes emission of one more
wxChildFocusEvent), but again, nothing changed in this respect in CVS.
Maybe there was some wxYield() somewhere (in wx or LMI) that isn't there
anymore, that's the only thing I can think of.


A possible fix is to filter out wxChildFocusEvents other than the first
one, like this:

Index: mvc_controller.cpp
===================================================================
RCS file: /sources/lmi/lmi/mvc_controller.cpp,v
retrieving revision 1.24
diff -u -u -r1.24 mvc_controller.cpp
--- mvc_controller.cpp  27 Dec 2008 02:56:50 -0000      1.24
+++ mvc_controller.cpp  3 Jan 2009 00:52:11 -0000
@@ -640,6 +640,12 @@
 
     wxWindow* new_focused_window = FindFocus();
 
+    // wxChildFocusEvent is sent for every window in the hierarchy from
+    // new_focused_window up to this MvcController. But we only want to
+    // handle the event once, so ignore all but the "deepest" one.
+    if(event.GetWindow() != new_focused_window)
+        return;
+
     // Do nothing if focus hasn't changed. This case arises when
     // another application is activated, and then this application
     // is reactivated.


Another would be to manually Connect() EVT_KILL_FOCUS handler to all
controls within the MvcController (e.g. in UponInitDialog()). 

Regards,
Vaclav