kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

I've known for a while that the quasi-modal mode provided by DIALOG_SHIM
was broken.  The only reason it hasn't reared it's head is because of
the design of most of dialogs.  If you call any dialog derived from
DIALOG_SHIM using ShowQuasiModal() and allow the default button event
handlers (wxID_OK, wx_ID_CANCEL, etc.) defined in wxDialogBase (see the
common/dlgcmn.cpp file in the wxWidgets source) to be called, kicad will
either crash or become stuck in the quasi-modal event loop requiring it
to be killed by the OS.  You can test this for yourself by opening an
HTML_MESSAGE_BOX dialog in the quasi-modal mode and either clicking the
OK button or clicking the close button in the title bar.  The attached
patch fixes these issues.  However, I would like some thorough testing
on all platforms to make sure it doesn't break any other dialog behavior
before I commit it.

This bug is the final straw in our poor practice of directly handling
default button events in our dialog code instead of using the correct
wxDialog methods for transferring data between the controls in the
dialog and the data structures and/or dialog class members.  Therefore,
I am implementing a new policy.  Any new dialogs that directly handle
default button events will be rejected.  As we modify existing dialogs,
we should be fixing them accordingly so they can be shown as
quasi-modal, modal, or mode-less without any modification.  Your help in
this matter will be greatly appreciated.

Cheers,

Wayne

=== modified file 'common/dialog_shim.cpp'
--- common/dialog_shim.cpp	2016-04-02 12:25:44 +0000
+++ common/dialog_shim.cpp	2016-04-08 19:59:22 +0000
@@ -70,6 +70,9 @@
     if( h )
         SetKiway( this, &h->Kiway() );
 
+    Bind( wxEVT_CLOSE_WINDOW, &DIALOG_SHIM::OnCloseWindow, this );
+    Bind( wxEVT_BUTTON, &DIALOG_SHIM::OnButton, this );
+
 #ifdef __WINDOWS__
     // On Windows, the app top windows can be brought to the foreground
     // (at least temporary) in certain circumstances,
@@ -535,3 +538,50 @@
 
     Show( false );
 }
+
+
+void DIALOG_SHIM::OnCloseWindow( wxCloseEvent& aEvent )
+{
+    if( IsQuasiModal() )
+    {
+        EndQuasiModal( wxID_CANCEL );
+        return;
+    }
+
+    // This is manditory to allow wxDialogBase::OnCloseWindow() to be called.
+    aEvent.Skip();
+}
+
+
+void DIALOG_SHIM::OnButton( wxCommandEvent& aEvent )
+{
+    if( IsQuasiModal() )
+    {
+        const int id = aEvent.GetId();
+
+        if( id == GetAffirmativeId() )
+        {
+            EndQuasiModal( id );
+        }
+        else if( id == wxID_APPLY )
+        {
+            if( Validate() )
+                TransferDataFromWindow();
+        }
+        else if( id == GetEscapeId() ||
+                 (id == wxID_CANCEL && GetEscapeId() == wxID_ANY) )
+        {
+            EndQuasiModal( wxID_CANCEL );
+        }
+        else // not a standard button
+        {
+            aEvent.Skip();
+        }
+
+        return;
+    }
+
+    // This is manditory to allow wxDialogBase::OnButton() to be called.
+    aEvent.Skip();
+}
+

=== modified file 'cvpcb/cvpcb_mainframe.cpp'
--- cvpcb/cvpcb_mainframe.cpp	2016-04-08 16:45:11 +0000
+++ cvpcb/cvpcb_mainframe.cpp	2016-04-08 18:57:16 +0000
@@ -725,9 +725,11 @@
         return false;
     }
 
+    {
     wxBusyCursor dummy;  // Let the user know something is happening.
 
     m_FootprintsList.ReadFootprintFiles( fptbl );
+    }
 
     if( m_FootprintsList.GetErrorCount() )
     {

=== modified file 'include/dialog_shim.h'
--- include/dialog_shim.h	2016-01-12 16:33:33 +0000
+++ include/dialog_shim.h	2016-04-08 19:24:35 +0000
@@ -52,6 +52,22 @@
  **/
 class DIALOG_SHIM : public wxDialog, public KIWAY_HOLDER
 {
+    /**
+     * Function OnCloseWindow
+     *
+     * properly handles the wxCloseEvent when in the quasimodal mode when not calling
+     * EndQuasiModal which is possible with any dialog derived from #DIALOG_SHIM.
+     */
+    void OnCloseWindow( wxCloseEvent& aEvent );
+
+    /**
+     * Function OnCloseWindow
+     *
+     * properly handles the default button events when in the quasimodal mode when not
+     * calling EndQuasiModal which is possible with any dialog derived from #DIALOG_SHIM.
+     */
+    void OnButton( wxCommandEvent& aEvent );
+
 public:
 
     DIALOG_SHIM( wxWindow* aParent, wxWindowID id, const wxString& title,