Go back to validate-on-OK, but stop doing design checks.

If we keep doing design validations at two different places, they can
only start to drift apart.  Board Setup should validate for malformed
data ONLY; everything else is the business of DRC.

This also fixes a bug where implementation validations would not allow
OK in the dialog, but wouldn't put up a message to say why.  We now use
the InfoBar impl from Jon's commit for that.

common/widgets/paged_dialog.cpp

@@ -36,7 +36,7 @@ std::map<wxString, wxString> g_lastPage;
 std::map<wxString, wxString> g_lastParentPage;
 
 
-PAGED_DIALOG::PAGED_DIALOG( wxWindow* aParent, const wxString& aTitle, bool aUseReset,
+PAGED_DIALOG::PAGED_DIALOG( wxWindow* aParent, const wxString& aTitle, bool aShowReset,
                             const wxString& aAuxiliaryAction ) :
         DIALOG_SHIM( aParent, wxID_ANY, aTitle, wxDefaultPosition, wxDefaultSize,
                      wxDEFAULT_DIALOG_STYLE | wxRESIZE_BORDER ),
@@ -64,7 +64,7 @@ PAGED_DIALOG::PAGED_DIALOG( wxWindow* aParent, const wxString& aTitle, bool aUse
 
     m_buttonsSizer = new wxBoxSizer( wxHORIZONTAL );
 
-    if( aUseReset )
+    if( aShowReset )
     {
         m_resetButton = new wxButton( this, wxID_ANY, _( "Reset to Defaults" ) );
         m_buttonsSizer->Add( m_resetButton, 0, wxRIGHT|wxLEFT, 5 );
@@ -73,12 +73,12 @@ PAGED_DIALOG::PAGED_DIALOG( wxWindow* aParent, const wxString& aTitle, bool aUse
     if( !aAuxiliaryAction.IsEmpty() )
     {
         m_auxiliaryButton = new wxButton( this, wxID_ANY, aAuxiliaryAction );
-        m_buttonsSizer->Add( m_auxiliaryButton, 0, wxRIGHT|wxLEFT, 5 );
+        m_buttonsSizer->Add( m_auxiliaryButton, 0, wxALIGN_CENTER_VERTICAL|wxRIGHT|wxLEFT, 5 );
     }
 
     m_buttonsSizer->AddStretchSpacer();
 
-    auto sdbSizer = new wxStdDialogButtonSizer();
+    wxStdDialogButtonSizer* sdbSizer = new wxStdDialogButtonSizer();
     wxButton* sdbSizerOK = new wxButton( this, wxID_OK );
     sdbSizer->AddButton( sdbSizerOK );
     wxButton* sdbSizerCancel = new wxButton( this, wxID_CANCEL );
@@ -96,14 +96,18 @@ PAGED_DIALOG::PAGED_DIALOG( wxWindow* aParent, const wxString& aTitle, bool aUse
     m_hash_key = aTitle;
 
     if( m_auxiliaryButton )
+    {
         m_auxiliaryButton->Connect( wxEVT_COMMAND_BUTTON_CLICKED,
                                     wxCommandEventHandler( PAGED_DIALOG::OnAuxiliaryAction ),
                                     nullptr, this );
+    }
 
     if( m_resetButton )
+    {
         m_resetButton->Connect( wxEVT_COMMAND_BUTTON_CLICKED,
-                                wxCommandEventHandler( PAGED_DIALOG::OnResetButton ),
+                                wxCommandEventHandler( PAGED_DIALOG::OnResetButton ), nullptr,
-                                nullptr, this );
+                                this );
+    }
 
     m_treebook->Connect( wxEVT_TREEBOOK_PAGE_CHANGED,
                          wxBookCtrlEventHandler( PAGED_DIALOG::OnPageChange ), NULL, this );
@@ -164,14 +168,18 @@ PAGED_DIALOG::~PAGED_DIALOG()
     g_lastParentPage[ m_title ] = lastParentPage;
 
     if( m_auxiliaryButton )
+    {
         m_auxiliaryButton->Disconnect( wxEVT_COMMAND_BUTTON_CLICKED,
                                        wxCommandEventHandler( PAGED_DIALOG::OnAuxiliaryAction ),
                                        nullptr, this );
+    }
 
     if( m_resetButton )
+    {
         m_resetButton->Disconnect( wxEVT_COMMAND_BUTTON_CLICKED,
-                                   wxCommandEventHandler( PAGED_DIALOG::OnResetButton ),
+                                   wxCommandEventHandler( PAGED_DIALOG::OnResetButton ), nullptr,
-                                   nullptr, this );
+                                   this );
+    }
 
     m_treebook->Disconnect( wxEVT_TREEBOOK_PAGE_CHANGED,
                             wxBookCtrlEventHandler( PAGED_DIALOG::OnPageChange ), NULL, this );
@@ -290,35 +298,6 @@ void PAGED_DIALOG::SetError( const wxString& aMessage, wxWindow* aPage, wxWindow
 }
 
 
-void PAGED_DIALOG::AddAuxiliaryAction( const wxString& aTitle, const wxString& aTooltip,
-                                       std::function<void( wxCommandEvent& )> aHandler )
-{
-    // Insert before standard button sizer and flex spacer
-    const int idx = m_buttonsSizer->GetItemCount() - 2;
-
-    wxButton* button = new wxButton( this, wxID_ANY, aTitle );
-    button->SetToolTip( aTooltip );
-    button->Bind( wxEVT_BUTTON, aHandler );
-    m_buttonsSizer->Insert( idx, button, 0, wxRIGHT | wxLEFT, 5 );
-}
-
-
-void PAGED_DIALOG::OnValidate( wxCommandEvent& aEvent )
-{
-    for( size_t i = 0; i < m_treebook->GetPageCount(); ++i )
-    {
-        wxWindow* page = m_treebook->GetPage( i );
-
-        // Display first warning
-        if( !page->Validate() )
-            break;
-    }
-
-    if( !m_errorMessage.IsEmpty() )
-        m_infoBar->ShowMessage( m_errorMessage, wxICON_WARNING );
-}
-
-
 void PAGED_DIALOG::OnUpdateUI( wxUpdateUIEvent& event )
 {
     // Handle an error.  This is delayed to OnUpdateUI so that we can change the focus
@@ -332,6 +311,8 @@ void PAGED_DIALOG::OnUpdateUI( wxUpdateUIEvent& event )
         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 );

include/widgets/paged_dialog.h

@@ -45,7 +45,7 @@ private:
     std::vector<bool> m_macHack;
 
 public:
-    PAGED_DIALOG( wxWindow* aParent, const wxString& aTitle, bool aUseReset = false,
+    PAGED_DIALOG( wxWindow* aParent, const wxString& aTitle, bool aShowReset,
                   const wxString& aAuxiliaryAction = wxEmptyString );
     ~PAGED_DIALOG() override;
 
@@ -61,9 +61,6 @@ public:
     void SetError( const wxString& aMessage, wxWindow* aPage, wxWindow* aCtrl, int aRow = -1,
                    int aCol = -1 );
 
-    void AddAuxiliaryAction( const wxString& aTitle, const wxString& aTooltip,
-                             std::function<void( wxCommandEvent& )> aHandler );
-
 protected:
     void finishInitialization();
 
@@ -76,7 +73,6 @@ protected:
     void OnResetButton( wxCommandEvent& aEvent );
     void OnUpdateUI( wxUpdateUIEvent& event );
     void OnPageChange( wxBookCtrlEvent& event );
-    void OnValidate( wxCommandEvent& aEvent );
 
     wxTreebook* m_treebook;
     wxButton*   m_auxiliaryButton;

pcbnew/dialogs/dialog_board_setup.cpp

@@ -103,9 +103,6 @@ DIALOG_BOARD_SETUP::DIALOG_BOARD_SETUP( PCB_EDIT_FRAME* aFrame ) :
     for( size_t i = 0; i < m_treebook->GetPageCount(); ++i )
    	    m_macHack.push_back( true );
 
-    AddAuxiliaryAction( "Validate Settings", "Check for problems with board settings",
-                        std::bind( &DIALOG_BOARD_SETUP::OnValidate, this, _1 ) );
-
 	// Connect Events
 	m_treebook->Connect( wxEVT_TREEBOOK_PAGE_CHANGED,
                          wxBookCtrlEventHandler( DIALOG_BOARD_SETUP::OnPageChange ), NULL, this );

pcbnew/dialogs/panel_setup_text_and_graphics.cpp

@@ -1,7 +1,7 @@
 /*
  * This program source code file is part of KiCad, a free EDA CAD application.
  *
- * Copyright (C) 2018 KiCad Developers, see change_log.txt for contributors.
+ * Copyright (C) 2018-2021 KiCad Developers, see change_log.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
@@ -162,32 +162,11 @@ int PANEL_SETUP_TEXT_AND_GRAPHICS::getGridValue( int aRow, int aCol )
 }
 
 
-bool PANEL_SETUP_TEXT_AND_GRAPHICS::Validate()
+bool PANEL_SETUP_TEXT_AND_GRAPHICS::TransferDataFromWindow()
 {
     if( !m_grid->CommitPendingChanges() )
         return false;
 
-    // Test text parameters.
-    for( int row : { ROW_SILK, ROW_COPPER, ROW_FAB, ROW_OTHERS } )
-    {
-        int textSize = std::min( getGridValue( row, COL_TEXT_WIDTH ),
-                                 getGridValue( row, COL_TEXT_HEIGHT ) );
-
-        if( getGridValue( row, COL_TEXT_THICKNESS ) > textSize / 4 )
-        {
-            wxString msg = _( "Text will not be readable with a thickness greater than "
-                              "1/4 its width or height." );
-            m_Parent->SetError( msg, this, m_grid, row, COL_TEXT_THICKNESS );
-            return false;
-        }
-    }
-
-    return true;
-}
-
-
-bool PANEL_SETUP_TEXT_AND_GRAPHICS::TransferDataFromWindow()
-{
     for( int i = 0; i < ROW_COUNT; ++i )
     {
         m_BrdSettings->m_LineThickness[ i ] = getGridValue( i, COL_LINE_THICKNESS );
@@ -195,8 +174,8 @@ bool PANEL_SETUP_TEXT_AND_GRAPHICS::TransferDataFromWindow()
         if( i == ROW_EDGES || i == ROW_COURTYARD )
             continue;
 
-        m_BrdSettings->m_TextSize[ i ] =
+        m_BrdSettings->m_TextSize[ i ] = wxSize( getGridValue( i, COL_TEXT_WIDTH ),
-                wxSize( getGridValue( i, COL_TEXT_WIDTH ), getGridValue( i, COL_TEXT_HEIGHT ) );
+                                                 getGridValue( i, COL_TEXT_HEIGHT ) );
         m_BrdSettings->m_TextThickness[ i ] = getGridValue( i, COL_TEXT_THICKNESS );
         m_BrdSettings->m_TextItalic[ i ] =
                 wxGridCellBoolEditor::IsTrueValue( m_grid->GetCellValue( i, COL_TEXT_ITALIC ) );

pcbnew/dialogs/panel_setup_text_and_graphics.h

@@ -55,8 +55,6 @@ public:
     bool TransferDataFromWindow() override;
 
     void ImportSettingsFrom( BOARD* aBoard );
-
-    bool Validate() override;
 };
 
 #endif //PANEL_SETUP_TEXT_AND_GRAPHICS_H

pcbnew/dialogs/panel_setup_tracks_and_vias.cpp

@@ -149,6 +149,8 @@ bool PANEL_SETUP_TRACKS_AND_VIAS::TransferDataFromWindow()
         return false;
     }
 
+    // Test ONLY for malformed data.  Design rules and constraints are the business of DRC.
+
     for( int row = 0; row < m_trackWidthsGrid->GetNumberRows();  ++row )
     {
         msg = m_trackWidthsGrid->GetCellValue( row, TR_WIDTH_COL );
@@ -226,125 +228,34 @@ bool PANEL_SETUP_TRACKS_AND_VIAS::Validate()
     }
 
     wxString msg;
-    int minViaAnnulus = m_ConstraintsPanel->m_viaMinAnnulus.GetValue();
-    int minViaDia = m_ConstraintsPanel->m_viaMinSize.GetValue();
-    int minThroughHole = m_ConstraintsPanel->m_throughHoleMin.GetValue();
-    int minTrackWidth = m_ConstraintsPanel->m_trackMinWidth.GetValue();
-    int minClearance = m_ConstraintsPanel->m_minClearance.GetValue();
-
-    // Test tracks
-    for( int row = 0; row < m_trackWidthsGrid->GetNumberRows();  ++row )
-    {
-        wxString tvalue = m_trackWidthsGrid->GetCellValue( row, TR_WIDTH_COL );
-
-        if( tvalue.IsEmpty() )
-            continue;
-
-        if( ValueFromString( m_Frame->GetUserUnits(), tvalue ) < minTrackWidth )
-        {
-            msg.Printf( _( "Track width less than minimum track width (%s)." ),
-                        StringFromValue( m_Frame->GetUserUnits(), minTrackWidth, true ) );
-            m_Parent->SetError( msg, this, m_trackWidthsGrid, row, TR_WIDTH_COL );
-            return false;
-        }
-    }
 
     // Test vias
     for( int row = 0; row < m_viaSizesGrid->GetNumberRows();  ++row )
     {
         wxString viaDia = m_viaSizesGrid->GetCellValue( row, VIA_SIZE_COL );
-
-        if( viaDia.IsEmpty() )
-            continue;
-
-        if( ValueFromString( m_Frame->GetUserUnits(), viaDia ) < minViaDia )
-        {
-            msg.Printf( _( "Via diameter less than minimum via diameter (%s)." ),
-                        StringFromValue( m_Frame->GetUserUnits(), minViaDia, true ) );
-            m_Parent->SetError( msg, this, m_viaSizesGrid, row, VIA_SIZE_COL );
-            return false;
-        }
-
         wxString viaDrill = m_viaSizesGrid->GetCellValue( row, VIA_DRILL_COL );
 
-        if( viaDrill.IsEmpty() )
+        if( !viaDia.IsEmpty() && viaDrill.IsEmpty() )
         {
             msg = _( "No via hole size defined." );
             m_Parent->SetError( msg, this, m_viaSizesGrid, row, VIA_DRILL_COL );
             return false;
         }
 
-        if( ValueFromString( m_Frame->GetUserUnits(), viaDrill ) < minThroughHole )
-        {
-            msg.Printf( _( "Via hole diameter less than minimum through hole diameter (%s)." ),
-                        StringFromValue( m_Frame->GetUserUnits(), minThroughHole, true ) );
-            m_Parent->SetError( msg, this, m_viaSizesGrid, row, VIA_DRILL_COL );
-            return false;
-        }
-
-        if( ValueFromString( m_Frame->GetUserUnits(), viaDrill )
-                >= ValueFromString( m_Frame->GetUserUnits(), viaDia ) )
-        {
-            msg = _( "Via hole diameter larger than via diameter." );
-            m_Parent->SetError( msg, this, m_viaSizesGrid, row, VIA_DRILL_COL );
-            return false;
-        }
-
-        if( ( ValueFromString( m_Frame->GetUserUnits(), viaDia )
-                - ValueFromString( m_Frame->GetUserUnits(), viaDrill ) ) / 2 < minViaAnnulus )
-        {
-            msg.Printf( _( "Diameter and hole leave via annulus less than minimum (%s)." ),
-                        StringFromValue( m_Frame->GetUserUnits(), minViaAnnulus, true ) );
-            m_Parent->SetError( msg, this, m_viaSizesGrid, row, VIA_SIZE_COL );
-            return false;
-        }
     }
 
     // Test diff pairs
     for( int row = 0; row < m_diffPairsGrid->GetNumberRows();  ++row )
     {
-        wxString tvalue = m_diffPairsGrid->GetCellValue( row, 0 );
+        wxString dpWidth = m_diffPairsGrid->GetCellValue( row, 0 );
+        wxString dpGap = m_diffPairsGrid->GetCellValue( row, 1 );
 
-        if( tvalue.IsEmpty() )
+        if( !dpWidth.IsEmpty() && dpGap.IsEmpty() )
-            continue;
-
-        if( ValueFromString( m_Frame->GetUserUnits(), tvalue ) < minTrackWidth )
-        {
-            msg.Printf( _( "Differential pair track width less than minimum track width (%s)." ),
-                        StringFromValue( m_Frame->GetUserUnits(), minTrackWidth, true ) );
-            m_Parent->SetError( msg, this, m_diffPairsGrid, row, 0 );
-            return false;
-        }
-
-        wxString gap = m_diffPairsGrid->GetCellValue( row, 1 );
-
-        if( gap.IsEmpty() )
         {
             msg = _( "No differential pair gap defined." );
             m_Parent->SetError( msg, this, m_diffPairsGrid, row, 1 );
             return false;
         }
-
-        if( ValueFromString( m_Frame->GetUserUnits(), gap ) < minClearance )
-        {
-            msg.Printf( _( "Differential pair gap less than minimum clearance (%s)." ),
-                        StringFromValue( m_Frame->GetUserUnits(), minClearance, true ) );
-            m_Parent->SetError( msg, this, m_diffPairsGrid, row, 1 );
-            return false;
-        }
-
-        wxString viaGap = m_diffPairsGrid->GetCellValue( row, 2 );
-
-        if( viaGap.IsEmpty() )
-            continue;
-
-        if( ValueFromString( m_Frame->GetUserUnits(), viaGap ) < minClearance )
-        {
-            msg.Printf( _( "Differential pair via gap less than minimum clearance (%s)." ),
-                        StringFromValue( m_Frame->GetUserUnits(), minClearance, true ) );
-            m_Parent->SetError( msg, this, m_diffPairsGrid, row, 2 );
-            return false;
-        }
     }
 
     return true;