From 07a665f4fd9ce703d31e4afaf4a4aa27c649d58d Mon Sep 17 00:00:00 2001 From: Jeff Young Date: Sat, 21 Jul 2018 21:23:13 +0100 Subject: [PATCH] Improve validation of symbol fields editor. Don't beep when inserting character from focused grid cell (but before editor is opened). Handle reference validation separately from name validation and separately from user field value validation. The old way of setting the fieldId on the validator wasn't working because the validator gets copied. Run validation when leaving cell. Don't just check for empty (particularly for fields that CAN be empty). Fixes: lp:1782917 * https://bugs.launchpad.net/kicad/+bug/1782917 (cherry picked from commit 79e04de) --- common/validators.cpp | 9 +- common/widgets/grid_text_button_helpers.cpp | 48 +++++++ .../dialog_edit_component_in_schematic.cpp | 13 +- .../dialog_edit_libentry_fields_in_lib.cpp | 11 +- eeschema/fields_grid_table.cpp | 118 ++++++++++-------- eeschema/fields_grid_table.h | 15 +-- eeschema/sch_validators.cpp | 30 ++--- eeschema/sch_validators.h | 9 +- include/widgets/grid_text_button_helpers.h | 1 + 9 files changed, 162 insertions(+), 92 deletions(-) diff --git a/common/validators.cpp b/common/validators.cpp index 94e8a0c861..28db81c135 100644 --- a/common/validators.cpp +++ b/common/validators.cpp @@ -41,12 +41,12 @@ GRID_CELL_TEXT_EDITOR::GRID_CELL_TEXT_EDITOR() : wxGridCellTextEditor() } -void GRID_CELL_TEXT_EDITOR::SetValidator(const wxValidator& validator) +void GRID_CELL_TEXT_EDITOR::SetValidator( const wxValidator& validator ) { // keep our own copy because wxGridCellTextEditor's is annoyingly private m_validator.reset( static_cast( validator.Clone() ) ); - wxGridCellTextEditor::SetValidator( validator ); + wxGridCellTextEditor::SetValidator( *m_validator ); } @@ -58,8 +58,11 @@ void GRID_CELL_TEXT_EDITOR::StartingKey( wxKeyEvent& event ) m_validator.get()->ProcessEvent( event ); } - if( !event.WasProcessed() ) + if( event.GetSkipped() ) + { wxGridCellTextEditor::StartingKey( event ); + event.Skip( false ); + } } diff --git a/common/widgets/grid_text_button_helpers.cpp b/common/widgets/grid_text_button_helpers.cpp index 8608f757be..b35fa48cf6 100644 --- a/common/widgets/grid_text_button_helpers.cpp +++ b/common/widgets/grid_text_button_helpers.cpp @@ -60,6 +60,54 @@ void GRID_CELL_TEXT_BUTTON::SetSize( const wxRect& aRect ) } +void GRID_CELL_TEXT_BUTTON::StartingKey( wxKeyEvent& event ) +{ + // Note: this is a copy of wxGridCellTextEditor's StartingKey() + + // Since this is now happening in the EVT_CHAR event EmulateKeyPress is no + // longer an appropriate way to get the character into the text control. + // Do it ourselves instead. We know that if we get this far that we have + // a valid character, so not a whole lot of testing needs to be done. + + wxTextEntry* textEntry = dynamic_cast( Combo() ); + int ch; + + bool isPrintable; + +#if wxUSE_UNICODE + ch = event.GetUnicodeKey(); + if ( ch != WXK_NONE ) + isPrintable = true; + else +#endif // wxUSE_UNICODE + { + ch = event.GetKeyCode(); + isPrintable = ch >= WXK_SPACE && ch < WXK_START; + } + + switch (ch) + { + case WXK_DELETE: + // Delete the initial character when starting to edit with DELETE. + textEntry->Remove(0, 1); + break; + + case WXK_BACK: + // Delete the last character when starting to edit with BACKSPACE. + { + const long pos = textEntry->GetLastPosition(); + textEntry->Remove(pos - 1, pos); + } + break; + + default: + if ( isPrintable ) + textEntry->WriteText(static_cast(ch)); + break; + } +} + + void GRID_CELL_TEXT_BUTTON::BeginEdit( int aRow, int aCol, wxGrid* aGrid ) { auto evtHandler = static_cast( m_control->GetEventHandler() ); diff --git a/eeschema/dialogs/dialog_edit_component_in_schematic.cpp b/eeschema/dialogs/dialog_edit_component_in_schematic.cpp index fd2028946c..9e59857d83 100644 --- a/eeschema/dialogs/dialog_edit_component_in_schematic.cpp +++ b/eeschema/dialogs/dialog_edit_component_in_schematic.cpp @@ -357,7 +357,7 @@ bool DIALOG_EDIT_COMPONENT_IN_SCHEMATIC::Validate() if( !SCH_COMPONENT::IsReferenceStringValid( m_fields->at( REFERENCE ).GetText() ) ) { - DisplayErrorMessage( nullptr, _( "References must start with a letter." ) ); + DisplayErrorMessage( this, _( "References must start with a letter." ) ); m_delayedFocusColumn = FDC_VALUE; m_delayedFocusRow = REFERENCE; @@ -544,14 +544,17 @@ void DIALOG_EDIT_COMPONENT_IN_SCHEMATIC::OnOKButtonClick( wxCommandEvent& event void DIALOG_EDIT_COMPONENT_IN_SCHEMATIC::OnGridCellChanging( wxGridEvent& event ) { - if( event.GetString().IsEmpty() ) - { - DisplayErrorMessage( this, _( "Field value cannot be empty." ) ); - event.Veto(); + wxGridCellEditor* editor = m_grid->GetCellEditor( event.GetRow(), event.GetCol() ); + wxControl* control = editor->GetControl(); + if( control && control->GetValidator() && !control->GetValidator()->Validate( control ) ) + { + event.Veto(); m_delayedFocusRow = event.GetRow(); m_delayedFocusColumn = event.GetCol(); } + + editor->DecRef(); } diff --git a/eeschema/dialogs/dialog_edit_libentry_fields_in_lib.cpp b/eeschema/dialogs/dialog_edit_libentry_fields_in_lib.cpp index 1efd636ecd..360195d779 100644 --- a/eeschema/dialogs/dialog_edit_libentry_fields_in_lib.cpp +++ b/eeschema/dialogs/dialog_edit_libentry_fields_in_lib.cpp @@ -322,14 +322,17 @@ void DIALOG_EDIT_LIBENTRY_FIELDS_IN_LIB::OnEditSpiceModel( wxCommandEvent& event void DIALOG_EDIT_LIBENTRY_FIELDS_IN_LIB::OnGridCellChanging( wxGridEvent& event ) { - if( event.GetCol() == FDC_NAME && event.GetString().IsEmpty() ) - { - DisplayErrorMessage( this, _( "Fields must have a name." ) ); - event.Veto(); + wxGridCellEditor* editor = m_grid->GetCellEditor( event.GetRow(), event.GetCol() ); + wxControl* control = editor->GetControl(); + if( control && control->GetValidator() && !control->GetValidator()->Validate( control ) ) + { + event.Veto(); m_delayedFocusRow = event.GetRow(); m_delayedFocusColumn = event.GetCol(); } + + editor->DecRef(); } diff --git a/eeschema/fields_grid_table.cpp b/eeschema/fields_grid_table.cpp index 3d21d0e741..50b7ea36e9 100644 --- a/eeschema/fields_grid_table.cpp +++ b/eeschema/fields_grid_table.cpp @@ -48,17 +48,23 @@ FIELDS_GRID_TABLE::FIELDS_GRID_TABLE( DIALOG_SHIM* aDialog, bool aInLibEdit, m_userUnits( aDialog->GetUserUnits() ), m_part( aPart ), m_inLibEdit( aInLibEdit ), - m_valueValidator( aInLibEdit, REFERENCE ) + m_fieldNameValidator( aInLibEdit, FIELD_NAME ), + m_referenceValidator( aInLibEdit, REFERENCE ) { - // Build the column attributes. + // Build the various grid cell attributes. m_readOnlyAttr = new wxGridCellAttr; m_readOnlyAttr->SetReadOnly( true ); - m_valueColAttr = new wxGridCellAttr; - GRID_CELL_TEXT_EDITOR* textEditor = new GRID_CELL_TEXT_EDITOR(); - textEditor->SetValidator( m_valueValidator ); - m_valueColAttr->SetEditor( textEditor ); + m_fieldNameAttr = new wxGridCellAttr; + GRID_CELL_TEXT_EDITOR* nameEditor = new GRID_CELL_TEXT_EDITOR(); + nameEditor->SetValidator( m_fieldNameValidator ); + m_fieldNameAttr->SetEditor( nameEditor ); + + m_referenceAttr = new wxGridCellAttr; + GRID_CELL_TEXT_EDITOR* referenceEditor = new GRID_CELL_TEXT_EDITOR(); + referenceEditor->SetValidator( m_referenceValidator ); + m_referenceAttr->SetEditor( referenceEditor ); m_footprintAttr = new wxGridCellAttr; m_footprintAttr->SetEditor( new GRID_CELL_FOOTPRINT_EDITOR( aDialog ) ); @@ -66,33 +72,33 @@ FIELDS_GRID_TABLE::FIELDS_GRID_TABLE( DIALOG_SHIM* aDialog, bool aInLibEdit, m_urlAttr = new wxGridCellAttr; m_urlAttr->SetEditor( new GRID_CELL_URL_EDITOR( aDialog ) ); - m_boolColAttr = new wxGridCellAttr; - m_boolColAttr->SetRenderer( new wxGridCellBoolRenderer() ); - m_boolColAttr->SetEditor( new wxGridCellBoolEditor() ); - m_boolColAttr->SetAlignment( wxALIGN_CENTER, wxALIGN_BOTTOM ); + m_boolAttr = new wxGridCellAttr; + m_boolAttr->SetRenderer( new wxGridCellBoolRenderer() ); + m_boolAttr->SetEditor( new wxGridCellBoolEditor() ); + m_boolAttr->SetAlignment( wxALIGN_CENTER, wxALIGN_BOTTOM ); wxArrayString vAlignNames; vAlignNames.Add( _( "Top" ) ); vAlignNames.Add( _( "Center" ) ); vAlignNames.Add( _( "Bottom" ) ); - m_vAlignColAttr = new wxGridCellAttr; - m_vAlignColAttr->SetEditor( new wxGridCellChoiceEditor( vAlignNames ) ); - m_vAlignColAttr->SetAlignment( wxALIGN_CENTER, wxALIGN_BOTTOM ); + m_vAlignAttr = new wxGridCellAttr; + m_vAlignAttr->SetEditor( new wxGridCellChoiceEditor( vAlignNames ) ); + m_vAlignAttr->SetAlignment( wxALIGN_CENTER, wxALIGN_BOTTOM ); wxArrayString hAlignNames; hAlignNames.Add( _( "Left" ) ); hAlignNames.Add(_( "Center" ) ); hAlignNames.Add(_( "Right" ) ); - m_hAlignColAttr = new wxGridCellAttr; - m_hAlignColAttr->SetEditor( new wxGridCellChoiceEditor( hAlignNames ) ); - m_hAlignColAttr->SetAlignment( wxALIGN_CENTER, wxALIGN_BOTTOM ); + m_hAlignAttr = new wxGridCellAttr; + m_hAlignAttr->SetEditor( new wxGridCellChoiceEditor( hAlignNames ) ); + m_hAlignAttr->SetAlignment( wxALIGN_CENTER, wxALIGN_BOTTOM ); wxArrayString orientationNames; orientationNames.Add( _( "Horizontal" ) ); orientationNames.Add(_( "Vertical" ) ); - m_orientationColAttr = new wxGridCellAttr; - m_orientationColAttr->SetEditor( new wxGridCellChoiceEditor( orientationNames ) ); - m_orientationColAttr->SetAlignment( wxALIGN_CENTER, wxALIGN_BOTTOM ); + m_orientationAttr = new wxGridCellAttr; + m_orientationAttr->SetEditor( new wxGridCellChoiceEditor( orientationNames ) ); + m_orientationAttr->SetAlignment( wxALIGN_CENTER, wxALIGN_BOTTOM ); } @@ -100,13 +106,14 @@ template FIELDS_GRID_TABLE::~FIELDS_GRID_TABLE() { m_readOnlyAttr->DecRef(); - m_boolColAttr->DecRef(); - m_valueColAttr->DecRef(); + m_fieldNameAttr->DecRef(); + m_boolAttr->DecRef(); + m_referenceAttr->DecRef(); m_footprintAttr->DecRef(); m_urlAttr->DecRef(); - m_vAlignColAttr->DecRef(); - m_hAlignColAttr->DecRef(); - m_orientationColAttr->DecRef(); + m_vAlignAttr->DecRef(); + m_hAlignAttr->DecRef(); + m_orientationAttr->DecRef(); } @@ -177,24 +184,35 @@ wxGridCellAttr* FIELDS_GRID_TABLE::GetAttr( int aRow, int aCol, wxGridCellAtt return m_readOnlyAttr; } else - return nullptr; + { + m_fieldNameAttr->IncRef(); + return m_fieldNameAttr; + } case FDC_VALUE: - // This field is the lib name and the default value when loading this component in - // schematic. The value is now not editable here (in this dialog) because changing - // it is equivalent to create a new component or alias. This is handled in libedit, - // not in this dialog. - if( m_inLibEdit && aRow == VALUE ) + if( aRow == REFERENCE ) { - m_readOnlyAttr->IncRef(); - return m_readOnlyAttr; + m_referenceAttr->IncRef(); + return m_referenceAttr; } - // For power symbols, the value is not editable, because value and pin - // name must be same and can be edited only in library editor - else if( m_part && m_part->IsPower() ) + else if( aRow == VALUE ) { - m_readOnlyAttr->IncRef(); - return m_readOnlyAttr; + if( m_inLibEdit ) + { + // This field is the lib name and the default value when loading this component + // in schematic. The value is now not editable here (in this dialog) because + // changing it is equivalent to create a new component or alias. This is handled + // in libedit, not in this dialog. + m_readOnlyAttr->IncRef(); + return m_readOnlyAttr; + } + else if( m_part && m_part->IsPower() ) + { + // For power symbols, the value is not editable, because value and pin name must + // be the same and can be edited only in library editor. + m_readOnlyAttr->IncRef(); + return m_readOnlyAttr; + } } else if( aRow == FOOTPRINT ) { @@ -206,15 +224,7 @@ wxGridCellAttr* FIELDS_GRID_TABLE::GetAttr( int aRow, int aCol, wxGridCellAtt m_urlAttr->IncRef(); return m_urlAttr; } - else - { - // Some fields have different validation semantics. Make sure the - // validator knows what it's validating. - m_valueValidator.SetFieldId( aRow ); - - m_valueColAttr->IncRef(); - return m_valueColAttr; - } + return nullptr; case FDC_TEXT_SIZE: case FDC_POSX: @@ -222,22 +232,22 @@ wxGridCellAttr* FIELDS_GRID_TABLE::GetAttr( int aRow, int aCol, wxGridCellAtt return nullptr; case FDC_H_ALIGN: - m_hAlignColAttr->IncRef(); - return m_hAlignColAttr; + m_hAlignAttr->IncRef(); + return m_hAlignAttr; case FDC_V_ALIGN: - m_vAlignColAttr->IncRef(); - return m_vAlignColAttr; + m_vAlignAttr->IncRef(); + return m_vAlignAttr; case FDC_ORIENTATION: - m_orientationColAttr->IncRef(); - return m_orientationColAttr; + m_orientationAttr->IncRef(); + return m_orientationAttr; case FDC_SHOWN: case FDC_ITALIC: case FDC_BOLD: - m_boolColAttr->IncRef(); - return m_boolColAttr; + m_boolAttr->IncRef(); + return m_boolAttr; default: wxFAIL; diff --git a/eeschema/fields_grid_table.h b/eeschema/fields_grid_table.h index f191fe5817..e63e3aa464 100644 --- a/eeschema/fields_grid_table.h +++ b/eeschema/fields_grid_table.h @@ -30,7 +30,6 @@ #include #include - class FIELDS_GRID_TRICKS : public GRID_TRICKS { public: @@ -96,16 +95,18 @@ private: LIB_PART* m_part; bool m_inLibEdit; - SCH_FIELD_VALIDATOR m_valueValidator; + SCH_FIELD_VALIDATOR m_fieldNameValidator; + SCH_FIELD_VALIDATOR m_referenceValidator; wxGridCellAttr* m_readOnlyAttr; - wxGridCellAttr* m_valueColAttr; + wxGridCellAttr* m_fieldNameAttr; + wxGridCellAttr* m_referenceAttr; wxGridCellAttr* m_footprintAttr; wxGridCellAttr* m_urlAttr; - wxGridCellAttr* m_boolColAttr; - wxGridCellAttr* m_vAlignColAttr; - wxGridCellAttr* m_hAlignColAttr; - wxGridCellAttr* m_orientationColAttr; + wxGridCellAttr* m_boolAttr; + wxGridCellAttr* m_vAlignAttr; + wxGridCellAttr* m_hAlignAttr; + wxGridCellAttr* m_orientationAttr; }; diff --git a/eeschema/sch_validators.cpp b/eeschema/sch_validators.cpp index f7cb396b1b..58cfc1da08 100644 --- a/eeschema/sch_validators.cpp +++ b/eeschema/sch_validators.cpp @@ -30,12 +30,12 @@ #include #include -SCH_FIELD_VALIDATOR::SCH_FIELD_VALIDATOR( bool aIsCmplibEditor, - int aFieldId, wxString* aValue ) : + +SCH_FIELD_VALIDATOR::SCH_FIELD_VALIDATOR( bool aIsLibEditor, int aFieldId, wxString* aValue ) : wxTextValidator( wxFILTER_EXCLUDE_CHAR_LIST, aValue ) { m_fieldId = aFieldId; - m_isLibEditor = aIsCmplibEditor; + m_isLibEditor = aIsLibEditor; // Fields cannot contain carriage returns, line feeds, or tabs. wxString excludes( "\r\n\t" ); @@ -49,7 +49,7 @@ SCH_FIELD_VALIDATOR::SCH_FIELD_VALIDATOR( bool aIsCmplibEditor, long style = GetStyle(); // The reference and value fields cannot be empty. - if( aFieldId == REFERENCE || aFieldId == VALUE ) + if( aFieldId == REFERENCE || aFieldId == VALUE || aFieldId == FIELD_NAME ) style |= wxFILTER_EMPTY; SetStyle( style ); @@ -82,19 +82,20 @@ bool SCH_FIELD_VALIDATOR::Validate( wxWindow *aParent ) switch( m_fieldId ) { - case REFERENCE: fieldName = _( "reference designator" ); break; - case VALUE: fieldName = _( "value" ); break; - case FOOTPRINT: fieldName = _( "footprint" ); break; - case DATASHEET: fieldName = _( "data sheet" ); break; - default: fieldName = _( "user defined" ); break; + case FIELD_NAME: fieldName = _( "field name" ); break; + case REFERENCE: fieldName = _( "reference field" ); break; + case VALUE: fieldName = _( "value field" ); break; + case FOOTPRINT: fieldName = _( "footprint field" ); break; + case DATASHEET: fieldName = _( "datasheet field" ); break; + default: fieldName = _( "user defined field" ); break; }; - wxString errorMsg; + wxString msg; // We can only do some kinds of validation once the input is complete, so // check for them here: if( HasFlag( wxFILTER_EMPTY ) && val.empty() ) - errorMsg.Printf( _( "The %s field cannot be empty." ), fieldName ); + msg.Printf( _( "The %s cannot be empty." ), fieldName ); else if( HasFlag( wxFILTER_EXCLUDE_CHAR_LIST ) && ContainsExcludedCharacters( val ) ) { wxArrayString whiteSpace; @@ -124,15 +125,14 @@ bool SCH_FIELD_VALIDATOR::Validate( wxWindow *aParent ) else wxCHECK_MSG( false, true, wxT( "Invalid illegal character in field validator." ) ); - errorMsg.Printf( _( "The %s field cannot contain %s characters." ), fieldName, badChars ); + msg.Printf( _( "The %s cannot contain %s characters." ), fieldName, badChars ); } - if ( !errorMsg.empty() ) + if ( !msg.empty() ) { m_validatorWindow->SetFocus(); - wxMessageBox( errorMsg, _( "Field Validation Error" ), - wxOK | wxICON_EXCLAMATION, aParent ); + wxMessageBox( msg, _( "Field Validation Error" ), wxOK | wxICON_EXCLAMATION, aParent ); return false; } diff --git a/eeschema/sch_validators.h b/eeschema/sch_validators.h index 6ebdbd5917..e97bfd51a9 100644 --- a/eeschema/sch_validators.h +++ b/eeschema/sch_validators.h @@ -30,9 +30,12 @@ #ifndef _SCH_VALIDATORS_H_ #define _SCH_VALIDATORS_H_ - #include + +#define FIELD_NAME -1 + + /** * class SCH_FILED_VALIDATOR * @@ -50,7 +53,7 @@ class SCH_FIELD_VALIDATOR : public wxTextValidator bool m_isLibEditor; public: - SCH_FIELD_VALIDATOR( bool aIsCmplibEditor, int aFieldId, wxString* aValue = NULL ); + SCH_FIELD_VALIDATOR( bool aIsLibEditor, int aFieldId, wxString* aValue = NULL ); SCH_FIELD_VALIDATOR( const SCH_FIELD_VALIDATOR& aValidator ); @@ -66,8 +69,6 @@ public: * @return true if the text in the control is valid otherwise false. */ virtual bool Validate( wxWindow *aParent ) override; - - void SetFieldId( int aFieldId ) { m_fieldId = aFieldId; } }; diff --git a/include/widgets/grid_text_button_helpers.h b/include/widgets/grid_text_button_helpers.h index ff556b3d2d..c644a52e5d 100644 --- a/include/widgets/grid_text_button_helpers.h +++ b/include/widgets/grid_text_button_helpers.h @@ -42,6 +42,7 @@ public: void SetSize( const wxRect& aRect ) override; + void StartingKey( wxKeyEvent& event ) override; void BeginEdit( int aRow, int aCol, wxGrid* aGrid ) override; bool EndEdit( int , int , const wxGrid* , const wxString& , wxString *aNewVal ) override; void ApplyEdit( int aRow, int aCol, wxGrid* aGrid ) override;