From 46d71f0d23a519fa79cfd47ab264c02084dcc4e0 Mon Sep 17 00:00:00 2001 From: Jeff Young Date: Tue, 16 Mar 2021 14:28:01 +0000 Subject: [PATCH] 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 | 55 ++++------ include/widgets/paged_dialog.h | 6 +- pcbnew/dialogs/dialog_board_setup.cpp | 3 - .../dialogs/panel_setup_text_and_graphics.cpp | 29 +---- .../dialogs/panel_setup_text_and_graphics.h | 2 - .../dialogs/panel_setup_tracks_and_vias.cpp | 101 ++---------------- 6 files changed, 29 insertions(+), 167 deletions(-) diff --git a/common/widgets/paged_dialog.cpp b/common/widgets/paged_dialog.cpp index e22cfc7075..e81373a7b0 100644 --- a/common/widgets/paged_dialog.cpp +++ b/common/widgets/paged_dialog.cpp @@ -36,7 +36,7 @@ std::map g_lastPage; std::map 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 ), - nullptr, this ); + wxCommandEventHandler( PAGED_DIALOG::OnResetButton ), nullptr, + 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 ), - nullptr, this ); + wxCommandEventHandler( PAGED_DIALOG::OnResetButton ), nullptr, + 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 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( ctrl ) ) { textCtrl->SetSelection( -1, -1 ); diff --git a/include/widgets/paged_dialog.h b/include/widgets/paged_dialog.h index 0ab82be743..8abaeb3ced 100644 --- a/include/widgets/paged_dialog.h +++ b/include/widgets/paged_dialog.h @@ -45,7 +45,7 @@ private: std::vector 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 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; diff --git a/pcbnew/dialogs/dialog_board_setup.cpp b/pcbnew/dialogs/dialog_board_setup.cpp index 4f3daa01d9..a8c293817e 100644 --- a/pcbnew/dialogs/dialog_board_setup.cpp +++ b/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 ); diff --git a/pcbnew/dialogs/panel_setup_text_and_graphics.cpp b/pcbnew/dialogs/panel_setup_text_and_graphics.cpp index f1a1be47bf..bf51b5d327 100644 --- a/pcbnew/dialogs/panel_setup_text_and_graphics.cpp +++ b/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 ] = - wxSize( getGridValue( i, COL_TEXT_WIDTH ), getGridValue( i, COL_TEXT_HEIGHT ) ); + m_BrdSettings->m_TextSize[ i ] = wxSize( getGridValue( i, COL_TEXT_WIDTH ), + 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 ) ); diff --git a/pcbnew/dialogs/panel_setup_text_and_graphics.h b/pcbnew/dialogs/panel_setup_text_and_graphics.h index 1d36b50b63..1f6b016767 100644 --- a/pcbnew/dialogs/panel_setup_text_and_graphics.h +++ b/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 diff --git a/pcbnew/dialogs/panel_setup_tracks_and_vias.cpp b/pcbnew/dialogs/panel_setup_tracks_and_vias.cpp index 70a71622cf..66f92a26ea 100644 --- a/pcbnew/dialogs/panel_setup_tracks_and_vias.cpp +++ b/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() ) - 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() ) + if( !dpWidth.IsEmpty() && dpGap.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;