Re: [lmi] wx-2.9.5 tab navigation

[Vadim Zeitlin]

On Tue, 23 Jul 2013 20:58:54 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2013-07-23 15:25Z, Vadim Zeitlin wrote:
GC> [...]
GC> > fixed it in http://trac.wxwidgets.org/changeset/74588
GC> 
GC> Thanks, that works.
GC> 
GC> Here's a regression that I hadn't noticed before:
GC>   File | New | Illustration
GC>   change to "Solve" notebook page
GC>   in "Solve for" groupbox, select "Specified amount"
GC>   change to "Face" notebook page
GC> The contents of these three textcontrols are highlighted:
GC>   "cap"
GC>   "offset"
GC>   "Specified amount"
GC> By "highlighted", I mean that the textcontrol has its usual color,
GC> but all of the text it contains is in reverse video: here, text is
GC> white foreground with blue background.

[Warning: this is a long post, if you are just interested in the apparently
 best solution to the problem above, please search for "Finally" and skip
 all the rest -- but I think it would still be worth reading all of it,
 because I'm not sure if the proposed solution is really the best one]


 Sorry for returning to this so late, but I've finally got around to
debugging this problem again and this time was able to find the explanation
for it: the problem is that MvcController::UponChildFocus() is called in
response to the text control on the "Face" page getting focus, but the
Validate() method called from it results in this very same control being
disabled. I.e. we disable the text control which is in process of getting
focus and, clearly, this just messes up some internal invariants of the
native control and results in an inconsistent internal state and wrong
display.


 Unfortunately, I don't have any perfect solution to this. My first idea
was to ensure that we never give focus to a control that could become
disabled as the result of validation that occurs because of this. But I
don't see any really reliable way of doing it. We can, of course, call
Validate() from the page changed handler, which would take care of the
problem in this particular case, but I don't think this is the only
situation in which this problem can appear. And I don't see any really
general solution for this. But if there is one, and it's just my lack of
knowledge of LMI controller code that prevents me from finding it, this
would probably be the best solution -- but right now I just don't have it.


 Second idea was to postpone the validation until after the change of focus
is completed. This can be achieved by the following simple patch

----------------------------------------------------------------------------
diff --git a/mvc_controller.cpp b/mvc_controller.cpp
index 5541f69..f2c1459 100644
--- a/mvc_controller.cpp
+++ b/mvc_controller.cpp
@@ -679,6 +679,18 @@ void MvcController::UponChildFocus(wxChildFocusEvent& 
event)
         return;
         }
 
+    // Schedule the validation of the window that has just lost focus in the
+    // near future (during the next event loop iteration), as we can't do this
+    // now because it is forbidden to change focus from a focus event handler
+    // and, even worse, we could decide to disable the window which is in
+    // process of getting focus, which would result in some really undesirable
+    // consequences, such as text controls keeping their selection while being
+    // disabled.
+    CallAfter(&MvcController::AfterChildFocus, new_focused_window);
+}
+
+void MvcController::AfterChildFocus(wxWindow* new_focused_window)
+{
     if(Validate())
         {
         if(new_focused_window)
diff --git a/mvc_controller.hpp b/mvc_controller.hpp
index 4dac68a..749e326 100644
--- a/mvc_controller.hpp
+++ b/mvc_controller.hpp
@@ -453,6 +453,8 @@ class MvcController
     void UponRefocusInvalidControl (wxCommandEvent&      );
     void UponUpdateUI              (wxUpdateUIEvent&     );
 
+    void AfterChildFocus(wxWindow*);
+
     // wxWindow overrides.
     virtual bool Validate();
 
----------------------------------------------------------------------------
and does indeed solve the problem but at a price: when entering an invalid
value in some control (e.g. "a" in the "cap" text control on the "Face"
page) you can now actually see the focus briefly switching to the next
control and then jumping back to the control with the invalid contents.
This is practically unnoticeable when using the mouse but quite obvious
when using TAB to change focus as the contents of the next control becomes
briefly selected when it gains focus and then immediately deselected again
when the focus is reset back, and the resulting flashing is definitely
annoying. I'm not sure if we can really live with this, but, on balance,
I'd say that this is still better than the current situation in which the
controls just get stuck in an impossible state.

 If you do decide to apply this solution, please notice that CallAfter()
was only added in wxWidgets 2.9.5, so LMI would either have to require at
least this version of wxWidgets (of course, my hope is that it will just
require 3.0 in a few days) or we'd have to use some preprocessor guards.
And also, if this solution is used, then wxEVT_REFOCUS_INVALID_CONTROL
becomes completely unnecessary as we can call RefocusLastFocusedWindow()
directly from AfterChildFocus() with this [fragment of a] diff:

----------------------------------------------------------------------------
@@ -693,13 +688,7 @@ void MvcController::AfterChildFocus(wxWindow* 
new_focused_window)
         }
     else
         {
-        LMI_ASSERT(!unit_test_refocus_event_pending_);
-        if(!unit_test_under_way_)
-            {
-            wxCommandEvent event0(wxEVT_REFOCUS_INVALID_CONTROL);
-            wxPostEvent(this, event0);
-            }
-        unit_test_refocus_event_pending_ = true;
+        RefocusLastFocusedWindow();
         }
 }
----------------------------------------------------------------------------
I am not sure about what should be done with unit_test_refocus_event_pending_ 
however as I don't see where is it used, there doesn't seem to be any
matches for it in the LMI code other than in mvc_controller.?pp files
themselves.


 Finally, there is a third idea, which I don't like very much, but which
seems to work fine in practice: let's just postpone disabling the control
which currently has focus, as this patch does:
----------------------------------------------------------------------------
diff --git a/mvc_controller.cpp b/mvc_controller.cpp
index 5541f69..52253e9 100644
--- a/mvc_controller.cpp
+++ b/mvc_controller.cpp
@@ -266,12 +266,43 @@ void MvcController::ConditionallyEnable()
         }
 }
 
+void MvcController::DoEnableControl
+    (wxWindow* control
+    ,bool      control_should_be_enabled
+    )
+{
+    control->Enable(control_should_be_enabled);
+}
+
 void MvcController::ConditionallyEnableControl
     (std::string const& name
     ,wxWindow&          control
     )
 {
-    control.Enable(ModelReference<datum_base>(name).is_enabled());
+    bool const control_should_be_enabled = 
ModelReference<datum_base>(name).is_enabled();
+    if(FindFocus() == &control)
+        {
+            // If the control is in process of gaining focus, disabling it
+            // right now could result in the native control getting into an
+            // inconsistent internal state (notably, we could have a disabled
+            // text control still showing selection under MSW when this was
+            // still done). We cannot really know if we are in the process of
+            // gaining focus or already have it since a long time, so for
+            // safety we have to assume that it is always dangerous to disable
+            // the focused control and delay doing it until it finishes fully
+            // receiving focus.
+            //
+            // Also notice that we have to pass "control" by pointer here due
+            // to a deficiency in CallAfter() implementation which would
+            // attempt to copy the non-copyable wxWindow class if we passed it
+            // by reference here.
+            CallAfter(&MvcController::DoEnableControl,
+                      &control, control_should_be_enabled);
+        }
+    else
+        {
+            DoEnableControl(&control, control_should_be_enabled);
+        }
 }
 
 void MvcController::ConditionallyEnableItems
diff --git a/mvc_controller.hpp b/mvc_controller.hpp
index 4dac68a..d9907e9 100644
--- a/mvc_controller.hpp
+++ b/mvc_controller.hpp
@@ -430,6 +430,7 @@ class MvcController
     void ConditionallyEnable();
     void ConditionallyEnableControl(std::string const&, wxWindow&);
     void ConditionallyEnableItems  (std::string const&, wxWindow&);
+    void DoEnableControl(wxWindow*, bool);
     wxWindow& CurrentPage() const;
     wxStaticText& DiagnosticsWindow() const;
     void EnsureOptimalFocus();
----------------------------------------------------------------------------
This fixes the originally reported problem without any obvious side effects
but just looks like a hack to me. This is, however, not a really convincing
argument against it, so even though I dislike it, this is what I recommend
to use right now. Notice that the remarks about CallAfter() above still
apply to this solution as well.


 The useful part of this email stops here, but I can't finish without
saying that, in my humble but firm opinion, the "design requirement of this
MVC framework that focus be retained in the offending control if validation
fails" is fundamentally flawed. It is problematic from the implementation
point of view as the sole existence of wxEVT_REFOCUS_INVALID_CONTROL (and
all the extra complexity it entails) shows. And the latest bug also
convincingly proves that the current implementation is very fragile as it
was broken by a perfectly unrelated (and, on its own, totally correct)
change in the details of wxMSW implementation. But, worse, this design is
also problematic from the UI point of view as forcing the user to enter a
valid value into the control is just very user-unfriendly, the user might
want to leave some value temporarily invalid, e.g. because he forgot or is
otherwise unsure about the value of this control right now but would like
to enter the value that he does have in mind into the next control. And
there is really no good reason to break the user data entry flow like this.
A typical form validation algorithm would clearly mark the control with the
invalid value, e.g. by changing its background colour to red or maybe by
showing a small error icon near it or in any other clearly visible way,
and, of course, disable the "OK" button on the dialog, but would let the
user do things in his or her own order.

 So irrespectively of how this bug should be fixed, I strongly believe that
the behaviour of the validation should be changed to prevent the user from
feeling caged by the constraints imposed by the program -- this is not a
good feeling to cultivate.


 But I realize that this is unlikely to change in the near future, so for
now I think that either the small patch above, introduced by the paragraph
starting with "Finally", should be applied, or, if you think that the
flashing of the text is not too bad, the second approach should be used.
In any case, I think this is something that must be fixed at LMI level and
I don't see this behaviour as a problem in wxWidgets as we indicate that
the focus shouldn't be changed by wxEVT_{KILL,SET}_FOCUS handlers, yet this
is what the current code (indirectly) does as disabling the window does
change the focus too (but I'll update wxWidgets documentation to mention
this as it's definitely not obvious). So I don't think this should prevent
the use of wxWidgets 3.0 in LMI -- but please let me know if you disagree
and, more generally, if you'd like me to do anything more about this bug.

 Thanks in advance,
VZ