Expunge update UI event handler from paged dialog object.

Use the book control page changing event to update any pages prior to
them being shown.  When the validation fails when changing pages, the
page change is vetoed until the invalid condition is fixed by the user.

Fixes: https://gitlab.com/kicad/code/kicad/-/issues/5049

Fixes: https://gitlab.com/kicad/code/kicad/-/issues/10139

common/dialogs/panel_hotkeys_editor.cpp

@@ -70,7 +70,7 @@ static wxSearchCtrl* CreateTextFilterBox( wxWindow* aParent, const wxString& aDe
 
 PANEL_HOTKEYS_EDITOR::PANEL_HOTKEYS_EDITOR( EDA_BASE_FRAME* aFrame, wxWindow* aWindow,
                                             bool aReadOnly ) :
-        RESETTABLE_PANEL( aWindow, wxID_ANY, wxDefaultPosition, default_dialog_size ),
+        RESETTABLE_PANEL( aWindow, wxID_ANY, wxDefaultPosition, wxDefaultSize ),
         m_frame( aFrame ),
         m_readOnly( aReadOnly ),
         m_hotkeyStore()
@@ -106,18 +106,6 @@ PANEL_HOTKEYS_EDITOR::PANEL_HOTKEYS_EDITOR( EDA_BASE_FRAME* aFrame, wxWindow* aW
 }
 
 
-bool PANEL_HOTKEYS_EDITOR::Show( bool show )
-{
-    if( show && m_hotkeyStore.GetSections().empty() )
-    {
-        m_hotkeyStore.Init( m_actions, m_readOnly );
-        return m_hotkeyListCtrl->TransferDataToControl();
-    }
-
-    return RESETTABLE_PANEL::Show( show );
-}
-
-
 void PANEL_HOTKEYS_EDITOR::ResetPanel()
 {
     m_hotkeyListCtrl->ResetAllHotkeys( true );
@@ -147,7 +135,6 @@ void PANEL_HOTKEYS_EDITOR::installButtons( wxSizer* aSizer )
         }
     };
 
-
     if( ADVANCED_CFG::GetCfg().m_HotkeysDumper )
     {
         // Add hotkeys dumper (does not need translation, it's a dev tool only)
@@ -171,7 +158,10 @@ void PANEL_HOTKEYS_EDITOR::installButtons( wxSizer* aSizer )
 
 bool PANEL_HOTKEYS_EDITOR::TransferDataToWindow()
 {
-    // Deferred to Show() for performance when opening preferences
+    m_hotkeyStore.Init( m_actions, m_readOnly );
+
+    if( !m_hotkeyListCtrl->TransferDataToControl() )
+        false;
 
     return true;
 }
@@ -179,12 +169,12 @@ bool PANEL_HOTKEYS_EDITOR::TransferDataToWindow()
 
 bool PANEL_HOTKEYS_EDITOR::TransferDataFromWindow()
 {
-    if( !m_hotkeyListCtrl->TransferDataFromControl() )
-        return false;
-
     if( m_readOnly )
         return true;
 
+    if( !m_hotkeyListCtrl->TransferDataFromControl() )
+        return false;
+
     WriteHotKeyConfig( m_actions );
 
     return true;

common/widgets/paged_dialog.cpp

@@ -46,10 +46,7 @@ PAGED_DIALOG::PAGED_DIALOG( wxWindow* aParent, const wxString& aTitle, bool aSho
         m_resetButton( nullptr ),
         m_cancelButton( nullptr ),
         m_title( aTitle ),
-        m_dirty( false ),
+        m_dirty( false )
-        m_errorCtrl( nullptr ),
-        m_errorRow( 0 ),
-        m_errorCol( 0 )
 {
     auto mainSizer = new wxBoxSizer( wxVERTICAL );
     SetSizer( mainSizer );
@@ -100,21 +97,17 @@ PAGED_DIALOG::PAGED_DIALOG( wxWindow* aParent, const wxString& aTitle, bool aSho
 
     if( m_auxiliaryButton )
     {
-        m_auxiliaryButton->Connect( wxEVT_COMMAND_BUTTON_CLICKED,
+        m_auxiliaryButton->Bind( wxEVT_COMMAND_BUTTON_CLICKED, &PAGED_DIALOG::OnAuxiliaryAction,
-                                    wxCommandEventHandler( PAGED_DIALOG::OnAuxiliaryAction ),
+                                 this );
-                                    nullptr, this );
     }
 
     if( m_resetButton )
     {
-        m_resetButton->Connect( wxEVT_COMMAND_BUTTON_CLICKED,
+        m_resetButton->Bind( wxEVT_COMMAND_BUTTON_CLICKED, &PAGED_DIALOG::OnResetButton, this );
-                                wxCommandEventHandler( PAGED_DIALOG::OnResetButton ), nullptr,
-                                this );
     }
 
-    m_treebook->Connect( wxEVT_TREEBOOK_PAGE_CHANGED,
+    m_treebook->Bind( wxEVT_TREEBOOK_PAGE_CHANGED, &PAGED_DIALOG::OnPageChange, this );
-                         wxBookCtrlEventHandler( PAGED_DIALOG::OnPageChange ), nullptr, this );
+    m_treebook->Bind( wxEVT_TREEBOOK_PAGE_CHANGING, &PAGED_DIALOG::OnPageChanging, this );
-    Connect( wxEVT_UPDATE_UI, wxUpdateUIEventHandler( PAGED_DIALOG::OnUpdateUI ), nullptr, this );
 }
 
 
@@ -170,22 +163,17 @@ PAGED_DIALOG::~PAGED_DIALOG()
 
     if( m_auxiliaryButton )
     {
-        m_auxiliaryButton->Disconnect( wxEVT_COMMAND_BUTTON_CLICKED,
+        m_auxiliaryButton->Unbind( wxEVT_COMMAND_BUTTON_CLICKED, &PAGED_DIALOG::OnAuxiliaryAction,
-                                       wxCommandEventHandler( PAGED_DIALOG::OnAuxiliaryAction ),
+                                   this );
-                                       nullptr, this );
     }
 
     if( m_resetButton )
     {
-        m_resetButton->Disconnect( wxEVT_COMMAND_BUTTON_CLICKED,
+        m_resetButton->Unbind( wxEVT_COMMAND_BUTTON_CLICKED, &PAGED_DIALOG::OnResetButton, this );
-                                   wxCommandEventHandler( PAGED_DIALOG::OnResetButton ), nullptr,
-                                   this );
     }
 
-    m_treebook->Disconnect( wxEVT_TREEBOOK_PAGE_CHANGED,
+    m_treebook->Unbind( wxEVT_TREEBOOK_PAGE_CHANGED, &PAGED_DIALOG::OnPageChange, this );
-                            wxBookCtrlEventHandler( PAGED_DIALOG::OnPageChange ), nullptr, this );
+    m_treebook->Unbind( wxEVT_TREEBOOK_PAGE_CHANGING, &PAGED_DIALOG::OnPageChanging, this );
-    Disconnect( wxEVT_UPDATE_UI, wxUpdateUIEventHandler( PAGED_DIALOG::OnUpdateUI ),
-                nullptr, this );
 }
 
 
@@ -234,7 +222,7 @@ bool PAGED_DIALOG::TransferDataToWindow()
         }
     }
 
-    m_treebook->SetSelection( (unsigned) std::max( 0, lastPageIndex ) );
+    m_treebook->ChangeSelection( (unsigned) std::max( 0, lastPageIndex ) );
 
     return true;
 }
@@ -258,15 +246,13 @@ bool PAGED_DIALOG::TransferDataFromWindow()
 
         if( !page->TransferDataFromWindow() )
         {
+            m_treebook->ChangeSelection( i );
             ret = false;
             break;
         }
     }
 #endif
 
-    if( !ret && !m_errorMessage.IsEmpty() )
-        m_infoBar->ShowMessage( m_errorMessage, wxICON_WARNING );
-
     return ret;
 }
 
@@ -281,51 +267,22 @@ void PAGED_DIALOG::SetError( const wxString& aMessage, const wxString& aPageName
 void PAGED_DIALOG::SetError( const wxString& aMessage, wxWindow* aPage, wxWindow* aCtrl,
                              int aRow, int aCol )
 {
-    for( size_t i = 0; i < m_treebook->GetPageCount(); ++i )
+    if( aCtrl )
     {
-        if( m_treebook->GetPage( i ) == aPage )
+        m_infoBar->ShowMessageFor( aMessage, 10000, wxICON_WARNING );
-        {
-            m_treebook->SetSelection( i );
-            break;
-        }
-    }
 
-    // Once the page has been changed we want to wait for it to update before displaying
+        if( wxTextCtrl* textCtrl = dynamic_cast<wxTextCtrl*>( aCtrl ) )
-    // the error dialog.  So store the rest of the error info and wait for OnUpdateUI.
-    m_errorMessage = aMessage;
-    m_errorCtrl = aCtrl;
-    m_errorRow = aRow;
-    m_errorCol = aCol;
-}
-
-
-void PAGED_DIALOG::OnUpdateUI( wxUpdateUIEvent& event )
-{
-    // Handle an error.  This is delayed to OnUpdateUI so that we can change the focus
-    // even when the original validation was triggered from a killFocus event, and so
-    // that the corresponding notebook page can be shown in the background when triggered
-    // from an OK.
-    if( m_errorCtrl )
-    {
-        // We will re-enter this routine when the error dialog is displayed, so make
-        // sure we don't keep putting up more dialogs.
-        wxWindow* ctrl = m_errorCtrl;
-        m_errorCtrl = nullptr;
-
-        m_infoBar->ShowMessageFor( m_errorMessage, 10000, wxICON_WARNING );
-
-        if( wxTextCtrl* textCtrl = dynamic_cast<wxTextCtrl*>( ctrl ) )
         {
             textCtrl->SetSelection( -1, -1 );
             textCtrl->SetFocus();
             return;
         }
 
-        if( wxStyledTextCtrl* scintilla = dynamic_cast<wxStyledTextCtrl*>( ctrl ) )
+        if( wxStyledTextCtrl* scintilla = dynamic_cast<wxStyledTextCtrl*>( aCtrl ) )
         {
-            if( m_errorRow > 0 )
+            if( aRow > 0 )
             {
-                int pos = scintilla->PositionFromLine( m_errorRow - 1 ) + ( m_errorCol - 1 );
+                int pos = scintilla->PositionFromLine( aRow - 1 ) + ( aCol - 1 );
                 scintilla->GotoPos( pos );
             }
 
@@ -333,40 +290,41 @@ void PAGED_DIALOG::OnUpdateUI( wxUpdateUIEvent& event )
             return;
         }
 
-        if( wxGrid* grid = dynamic_cast<wxGrid*>( ctrl ) )
+        if( wxGrid* grid = dynamic_cast<wxGrid*>( aCtrl ) )
         {
             grid->SetFocus();
-            grid->MakeCellVisible( m_errorRow, m_errorCol );
+            grid->MakeCellVisible( aRow, aCol );
-            grid->SetGridCursor( m_errorRow, m_errorCol );
+            grid->SetGridCursor( aRow, aCol );
 
             grid->EnableCellEditControl( true );
             grid->ShowCellEditControl();
             return;
         }
     }
-
-    if( m_treebook->GetCurrentPage()->GetChildren().IsEmpty() )
-    {
-        unsigned next = m_treebook->GetSelection() + 1;
-
-        // Use ChangeSelection() here because SetSelection() generates page change events which
-        // creates an infinite wxUpdateUIEvent loop.
-        if( next < m_treebook->GetPageCount() )
-            m_treebook->ChangeSelection( next );
-    }
 }
 
 
 void PAGED_DIALOG::OnPageChange( wxBookCtrlEvent& event )
 {
+    // Use the first sub-page when a tree level node is selected.
+    if( m_treebook->GetCurrentPage()->GetChildren().IsEmpty() )
+    {
+        unsigned next = m_treebook->GetSelection() + 1;
+
+        if( next < m_treebook->GetPageCount() )
+            m_treebook->ChangeSelection( next );
+    }
+
     size_t page = event.GetSelection();
 
+    // NB: dynamic_cast doesn't work over Kiway.
+    wxWindow* panel = m_treebook->GetPage( page );
+
+    wxCHECK( panel, /* void */ );
+
     // Enable the reset button only if the page is re-settable
     if( m_resetButton )
     {
-        // NB: dynamic_cast doesn't work over Kiway.
-        wxWindow* panel = m_treebook->GetPage( page );
-
         if( panel && panel->GetWindowStyle() & wxRESETTABLE )
         {
             m_resetButton->SetLabel( wxString::Format( _( "Reset %s to Defaults" ),
@@ -385,6 +343,13 @@ void PAGED_DIALOG::OnPageChange( wxBookCtrlEvent& event )
         m_resetButton->GetParent()->Layout();
     }
 
+    wxSizeEvent evt( panel->GetSize() );
+
+    panel->ProcessWindowEvent( evt );
+
+    // @todo Test to see if this macOS hack is still necessary now that a psuedo size event is
+    //       processed above.
+
     // Work around an OSX bug where the wxGrid children don't get placed correctly until
     // the first resize event
 #ifdef __WXMAC__
@@ -401,6 +366,26 @@ void PAGED_DIALOG::OnPageChange( wxBookCtrlEvent& event )
 }
 
 
+void PAGED_DIALOG::OnPageChanging( wxBookCtrlEvent& aEvent )
+{
+    int currentPage = aEvent.GetOldSelection();
+
+    if( currentPage == wxNOT_FOUND )
+        return;
+
+    wxWindow* page = m_treebook->GetPage( currentPage );
+
+    wxCHECK( page, /* void */ );
+
+    // If there is a validation error on the current page, don't allow the page change.
+    if( !page->Validate() || !page->TransferDataFromWindow() )
+    {
+        aEvent.Veto();
+        return;
+    }
+}
+
+
 void PAGED_DIALOG::OnResetButton( wxCommandEvent& aEvent )
 {
     int sel = m_treebook->GetSelection();

include/panel_hotkeys_editor.h

@@ -1,7 +1,7 @@
 /*
  * This program source code file is part of KiCad, a free EDA CAD application.
  *
- * Copyright (C) 2004-2020 KiCad Developers, see AUTHORS.TXT for contributors.
+ * Copyright (C) 2004-2021 KiCad Developers, see AUTHORS.TXT for contributors.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -55,8 +55,6 @@ public:
         return _( "Reset all hotkeys to the built-in KiCad defaults" );
     }
 
-    bool Show( bool show ) override;
-
 private:
     /**
      * Install the button panel (global reset/default, import/export)
@@ -79,7 +77,8 @@ private:
     void ImportHotKeys();
 
     /**
-     * Dumps all actions and their hotkeys to a text file for inclusion in documentation.
+     * Dump all actions and their hotkeys to a text file for inclusion in documentation.
+     *
      * The format is asciidoc-compatible table rows.
      * This function is hidden behind an advanced config flag and not intended for users.
      */

include/widgets/paged_dialog.h

@@ -56,8 +56,8 @@ protected:
     void OnCancel( wxCommandEvent& event );
     virtual void OnAuxiliaryAction( wxCommandEvent& event ) { event.Skip(); }
     void OnResetButton( wxCommandEvent& aEvent );
-    void OnUpdateUI( wxUpdateUIEvent& event );
     void OnPageChange( wxBookCtrlEvent& event );
+    void OnPageChanging( wxBookCtrlEvent& aEvent );
 
     wxTreebook* m_treebook;
     wxButton*   m_auxiliaryButton;
@@ -70,11 +70,6 @@ private:
 
     bool        m_dirty;
 
-    wxString    m_errorMessage;
-    wxWindow*   m_errorCtrl;    // the control associated with m_errorMessage
-    int         m_errorRow;     // the row if m_errorCtrl is a grid
-    int         m_errorCol;     // the column if m_errorCtrl is a grid
-
     wxBoxSizer* m_buttonsSizer;
 
     std::vector<bool> m_macHack;