[lmi] UpdateUI problems in main_wx.cpp

[Vadim Zeitlin]

 Hello,

 This is another not really urgent thing but I just got very tired of
looking at the UI updating-related mess in main_wx.cpp (plenty of "WX !!"
comments and commented out code) and so decided to fix the problem. And
unless I'm missing something, this is surprisingly simple. Let me explain
my understanding of what the problem was and how it can be fixed and please
correct me if I'm wrong:

 The root of the problem was described in this comment AFAICS:

// Required for toolbar enablement. Although '.xrc' files use the same
// xrc Id:
//   <object class="wxMenuItem" name="wxID_SAVE">
//   <object class="tool"       name="wxID_SAVE">
// the UponMenuOpen() handler above doesn't handle toolbar enablement,
// even when the menu is pulled down; and, OTOH, this function alone
// doesn't handle menuitem enablement.

While it's true that EVT_MENU_OPEN() handler can't do anything for the
toolbars, I was surprised to read that the EVT_UPDATE_UI() handler wasn't
enough to handle the menu item, which should clearly be the case. And it
would have been, if the EVT_MENU_OPEN() handler, which is also used for
other things, called event.Skip(). But it didn't and so the default wx
handler for opening the menus wasn't executed and so it didn't update the
menu items state correctly: this is done only when the menu is opened (and
not during the idle time, as with the toolbars) as an optimization, to
avoid updating the menu items needlessly if they're not currently visible
anyhow.

 So the solution is to allow the base EVT_MENU_OPEN() handler to run and
then all the other workarounds become unnecessary. The patch below is a bit
long but mostly it just removes comments and code not needed any more:

=== modified file 'main_wx.cpp'
--- main_wx.cpp 2008-08-02 03:47:24 +0000
+++ main_wx.cpp 2008-08-07 19:26:10 +0000
@@ -119,10 +119,6 @@
 // using the XRCID here prevents the menu command from working, but
 // either one makes toolbar enablement work correctly.
 //
-// WX !! However, menu enablement still doesn't seem to work with the
-// EVT_UPDATE_UI(wxID_SAVE,...) handler here; that seems to require
-// the EVT_MENU_OPEN handler.
-//
 BEGIN_EVENT_TABLE(Skeleton, wxApp)
     EVT_DROP_FILES(                                    Skeleton::UponDropFiles 
                   )
     EVT_BUTTON(wxID_HELP                              ,Skeleton::UponHelp      
                   )
@@ -154,14 +150,7 @@
     EVT_MENU(XRCID("window_tile_vertically"          
),Skeleton::UponWindowTileVertically         )
     EVT_MENU_OPEN(                                     Skeleton::UponMenuOpen  
                   )
     EVT_TIMER(wxID_ANY                                ,Skeleton::UponTimer     
                   )
-// TODO ?? expunge
-//  EVT_UPDATE_UI(wxID_ANY                            ,Skeleton::UponUpdateUI  
                   )
-    EVT_UPDATE_UI(wxID_SAVE                           
,Skeleton::UponUpdateFileSave               )
-// TODO ?? expunge
-// Enabling this line prevents the menuitem from performing its required
-// action, whether or not the EVT_UPDATE_UI(wxID_SAVE...) handler is also
-// present.
-//  EVT_UPDATE_UI(XRCID("wxID_SAVE"                  
),Skeleton::UponUpdateFileSave               )
+    EVT_UPDATE_UI(wxID_SAVE                           
,Skeleton::UponUpdateUISave                 )
 
 // TODO ?? There has to be a better way.
 /*
@@ -692,6 +681,13 @@
             return false;
             }
 
+        // window creation is complete, connect the update UI handlers which
+        // wouldn't work well if the creation had failed (e.g. because we
+        // couldn't load the XRC files)
+        Connect(wxID_SAVE,
+                wxEVT_UPDATE_UI,
+                wxUpdateUIEventHandler(Skeleton::UponUpdateUISave));
+
         frame_->Show(true);
         SetTopWindow(frame_);
 
@@ -722,10 +718,10 @@
     return true;
 }
 
-// TODO ?? Should this call Skip()?
-
-void Skeleton::UponMenuOpen(wxMenuEvent&)
+void Skeleton::UponMenuOpen(wxMenuEvent& event)
 {
+    event.Skip(); // let the default handler be executed as well
+
     int child_frame_count = 0;
     wxWindowList wl = frame_->GetChildren();
     for(wxWindowList::const_iterator i = wl.begin(); i != wl.end(); ++i)
@@ -751,29 +747,6 @@
             {
             window_previous->Enable(1 < child_frame_count);
             }
-
-        // Needed for (xrc) menu enablement: a
-        //   EVT_UPDATE_UI(XRCID("wxID_SAVE"),Skeleton::UponUpdateFileSave)
-        // handler fails to update enablement for that menu item.
-        // However, enablement of an item with the same ID on the
-        // toolbar apparently requires the EVT_UPDATE_UI technique.
-        wxDocument* doc = doc_manager_->GetCurrentDocument();
-        wxMenuItem* file_save = child_frame->GetMenuBar()->FindItem
-            (XRCID("wxID_SAVE")
-            );
-        if(file_save)
-            {
-            file_save->Enable(doc && doc->IsModified());
-            }
-/*
-        wxMenuItem* file_save_as = child_frame->GetMenuBar()->FindItem
-            (XRCID("wxID_SAVEAS")
-            );
-        if(file_save_as)
-            {
-            file_save_as->Enable(true);
-            }
-*/
         }
     // (else) Parent menu enablement could be handled here, but, for
     // now at least, none is required.
@@ -1078,53 +1051,10 @@
     wxSafeShowMessage("Terminating due to unhandled exception.", "Fatal 
error");
 }
 
-// TODO ?? Should this call Skip()?
-
-// Required for toolbar enablement. Although '.xrc' files use the same
-// xrc Id:
-//   <object class="wxMenuItem" name="wxID_SAVE">
-//   <object class="tool"       name="wxID_SAVE">
-// the UponMenuOpen() handler above doesn't handle toolbar enablement,
-// even when the menu is pulled down; and, OTOH, this function alone
-// doesn't handle menuitem enablement.
-//
-void Skeleton::UponUpdateFileSave(wxUpdateUIEvent& event)
+void Skeleton::UponUpdateUISave(wxUpdateUIEvent& event)
 {
     wxDocument* doc = doc_manager_->GetCurrentDocument();
     event.Enable(doc && doc->IsModified());
-
-    // Setting the event's Id to the xrc Id fails to handle menu
-    // enablement--that is, this does not work:
-//    event.SetId(XRCID("wxID_SAVE"));
-//    event.Enable(doc && doc->IsModified());
-}
-
-// TODO ?? An unsuccessful experiment.
-void Skeleton::UponUpdateInapplicable(wxUpdateUIEvent& e)
-{
-// This handler seems to override mdi childrens'.
-//    e.Enable(0 != frame_->GetChildren().GetCount());
-
-/*
-This doesn't undo that override.
-    e.Enable(false);
-    if(0 != frame_->GetChildren().GetCount())
-        {
-        e.Skip();
-        }
-*/
-
-// Presumably we need to use ProcessEvent(), somehow.
-}
-
-// TODO ?? Should this be expunged?
-
-// WX !! It seems that a function like this should be able to handle
-// all toolbar and menu enablement. But it appears that a much more
-// complex implementation is required for wxID_SAVE.
-//
-void Skeleton::UponUpdateUI(wxUpdateUIEvent&)
-{
 }
 
 void Skeleton::UponWindowCascade(wxCommandEvent&)

=== modified file 'main_wx.hpp'
--- main_wx.hpp 2008-06-28 02:00:43 +0000
+++ main_wx.hpp 2008-08-07 18:24:08 +0000
@@ -107,8 +107,7 @@
 
     void UponTimer                        (wxTimerEvent&);
     void UponUpdateInapplicable           (wxUpdateUIEvent&);
-    void UponUpdateFileSave               (wxUpdateUIEvent&);
-    void UponUpdateUI                     (wxUpdateUIEvent&);
+    void UponUpdateUISave                 (wxUpdateUIEvent&);
     void UponWindowCascade                (wxCommandEvent&);
     void UponWindowNext                   (wxCommandEvent&);
     void UponWindowPrevious               (wxCommandEvent&);

Notice that this contains my earlier patch to Connect() the "Save" UI
update handler dynamically to fix a crash which happened if the XRC
couldn't be loaded otherwise (bug #17757).

 I am not sure if I should submit this as a single patch or wait until the
Connect() fix is applied and submit the rest of the patch then?


 The only remaining related problem is this part of Skeleton class event
table definition:

// TODO ?? There has to be a better way.
/*
    EVT_UPDATE_UI(XRCID("edit_cell"            
),Skeleton::UponUpdateInapplicable)
... 13 more event handlers snipped ...
    EVT_UPDATE_UI(XRCID("column_width_fixed"   
),Skeleton::UponUpdateInapplicable)
*/

Unfortunately I don't have any satisfactory solution to it. There is
EVT_UPDATE_UI_RANGE() but nothing guarantees that the numeric values of the
corresponding ids are going to be consecutive, strictly speaking (even if
this is very likely in practice). And I don't think there is any way to
define a consecutive range of ids in the XRC. I hope Vaclav would correct
me if I'm wrong.

 Regards,
VZ