Validate new footprint / save as footprint info before exiting dialog.

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

eeschema/symbol_editor/symbol_edit_frame.h

@@ -415,9 +415,6 @@ private:
     // Set up the tool framework
     void setupTools();
 
-    EDA_LIST_DIALOG* buildSaveAsDialog( const wxString& aSymbolName,
-                                        const wxString& aLibraryPreselect );
-
     void saveSymbolAs();
 
     /**

include/pcb_base_frame.h

@@ -270,9 +270,11 @@ public:
      *       footprint is placed on a board and a netlist is read.
      *
      * @param aFootprintName is the name of the new footprint in library.
+     * @param aLibName optional, if specified is the library for the new footprint
      * @param aQuiet prevents user dialogs from being shown
      */
-    FOOTPRINT* CreateNewFootprint( const wxString& aFootprintName, bool aQuiet = false );
+    FOOTPRINT* CreateNewFootprint( const wxString& aFootprintName, const wxString& aLibName,
+                                   bool aQuiet );
 
     /**
      * Places \a aFootprint at the current cursor position and updates footprint coordinates

pcbnew/footprint_edit_frame.h

@@ -332,9 +332,6 @@ protected:
     /// protected so only friend PCB::IFACE::CreateWindow() can act as sole factory.
     FOOTPRINT_EDIT_FRAME( KIWAY* aKiway, wxWindow* aParent );
 
-    EDA_LIST_DIALOG* buildSaveAsDialog( const wxString& aFootprintName,
-                                        const wxString& aLibraryPreselect );
-
     /**
      * Make sure the footprint info list is loaded (with a progress dialog) and then initialize
      * the footprint library tree.

pcbnew/footprint_libraries_utils.cpp

@@ -931,16 +931,21 @@ bool FOOTPRINT_EDIT_FRAME::SaveFootprintToBoard( bool aAddNew )
 }
 
 
-static int ID_SAVE_AS_NAME     = 4172;
 static int ID_MAKE_NEW_LIBRARY = 4173;
 
 
-EDA_LIST_DIALOG* FOOTPRINT_EDIT_FRAME::buildSaveAsDialog( const wxString& aFootprintName,
+class SAVE_AS_DIALOG : public EDA_LIST_DIALOG
-                                                          const wxString& aLibraryPreselect )
+{
+public:
+    SAVE_AS_DIALOG( FOOTPRINT_EDIT_FRAME* aParent, const wxString& aFootprintName,
+                    const wxString& aLibraryPreselect,
+                    std::function<bool( wxString libName, wxString fpName )> aValidator ) :
+            EDA_LIST_DIALOG( aParent, _( "Save Footprint As" ), false ),
+            m_validator( std::move( aValidator ) )
     {
         COMMON_SETTINGS*           cfg = Pgm().GetCommonSettings();
-    PROJECT_FILE&              project = Kiway().Prj().GetProjectFile();
+        PROJECT_FILE&              project = aParent->Prj().GetProjectFile();
-    FP_LIB_TABLE*              tbl = Prj().PcbFootprintLibs();
+        FP_LIB_TABLE*              tbl = aParent->Prj().PcbFootprintLibs();
         std::vector<wxString>      nicknames = tbl->GetLogicalLibs();
         wxArrayString              headers;
         std::vector<wxArrayString> itemsToDisplay;
@@ -973,49 +978,68 @@ EDA_LIST_DIALOG* FOOTPRINT_EDIT_FRAME::buildSaveAsDialog( const wxString& aFootp
                 itemsToDisplay.push_back( item );
             }
         }
+        initDialog( headers, itemsToDisplay, aLibraryPreselect );
 
-    EDA_LIST_DIALOG* dlg = new EDA_LIST_DIALOG( this, _( "Save Footprint As" ), headers,
+        SetListLabel( _( "Save in library:" ) );
-                                                itemsToDisplay, aLibraryPreselect, false );
+        SetOKLabel( _( "Save" ) );
-
-    dlg->SetListLabel( _( "Save in library:" ) );
-    dlg->SetOKLabel( _( "Save" ) );
 
         wxBoxSizer* bNameSizer = new wxBoxSizer( wxHORIZONTAL );
 
-    wxStaticText* label = new wxStaticText( dlg, wxID_ANY, _( "Name:" ) );
+        wxStaticText* label = new wxStaticText( this, wxID_ANY, _( "Name:" ) );
         bNameSizer->Add( label, 0, wxALIGN_CENTER_VERTICAL|wxTOP|wxBOTTOM|wxLEFT, 5 );
 
-    wxTextCtrl* nameTextCtrl = new wxTextCtrl( dlg, ID_SAVE_AS_NAME, aFootprintName );
+        m_fpNameCtrl = new wxTextCtrl( this, wxID_ANY, aFootprintName );
-    bNameSizer->Add( nameTextCtrl, 1, wxALIGN_CENTER_VERTICAL|wxALL, 5 );
+        bNameSizer->Add( m_fpNameCtrl, 1, wxALIGN_CENTER_VERTICAL|wxALL, 5 );
 
         wxTextValidator nameValidator( wxFILTER_EXCLUDE_CHAR_LIST );
         nameValidator.SetCharExcludes( FOOTPRINT::StringLibNameInvalidChars( false ) );
-    nameTextCtrl->SetValidator( nameValidator );
+        m_fpNameCtrl->SetValidator( nameValidator );
 
-    wxButton* newLibraryButton = new wxButton( dlg, ID_MAKE_NEW_LIBRARY, _( "New Library..." ) );
+        wxButton* newLibraryButton = new wxButton( this, ID_MAKE_NEW_LIBRARY, _( "New Library..." ) );
-    dlg->m_ButtonsSizer->Prepend( 80, 20 );
+        m_ButtonsSizer->Prepend( 80, 20 );
-    dlg->m_ButtonsSizer->Prepend( newLibraryButton, 0, wxALIGN_CENTER_VERTICAL|wxLEFT|wxRIGHT, 10 );
+        m_ButtonsSizer->Prepend( newLibraryButton, 0, wxALIGN_CENTER_VERTICAL|wxLEFT|wxRIGHT, 10 );
 
-    dlg->GetSizer()->Prepend( bNameSizer, 0, wxEXPAND|wxTOP|wxLEFT|wxRIGHT, 5 );
+        GetSizer()->Prepend( bNameSizer, 0, wxEXPAND|wxTOP|wxLEFT|wxRIGHT, 5 );
 
-    dlg->Bind( wxEVT_BUTTON,
+        Bind( wxEVT_BUTTON,
-            [dlg]( wxCommandEvent& )
+                [this]( wxCommandEvent& )
                 {
-                dlg->EndModal( ID_MAKE_NEW_LIBRARY );
+                    EndModal( ID_MAKE_NEW_LIBRARY );
                 }, ID_MAKE_NEW_LIBRARY );
 
         // Move nameTextCtrl to the head of the tab-order
-    if( dlg->GetChildren().DeleteObject( nameTextCtrl ) )
+        if( GetChildren().DeleteObject( m_fpNameCtrl ) )
-        dlg->GetChildren().Insert( nameTextCtrl );
+            GetChildren().Insert( m_fpNameCtrl );
 
-    dlg->SetInitialFocus( nameTextCtrl );
+        SetInitialFocus( m_fpNameCtrl );
 
-    dlg->Layout();
+        SetupStandardButtons();
-    dlg->GetSizer()->Fit( dlg );
 
-    return dlg;
+        Layout();
+        GetSizer()->Fit( this );
+
+        Centre();
     }
 
+    wxString GetFPName()
+    {
+        wxString footprintName = m_fpNameCtrl->GetValue();
+        footprintName.Trim( true );
+        footprintName.Trim( false );
+        return footprintName;
+    }
+
+protected:
+    bool TransferDataFromWindow() override
+    {
+        return m_validator( GetTextSelection(), GetFPName() );
+    }
+
+private:
+    wxTextCtrl*                                              m_fpNameCtrl;
+    std::function<bool( wxString libName, wxString fpName )> m_validator;
+};
+
 
 bool FOOTPRINT_EDIT_FRAME::SaveFootprintAs( FOOTPRINT* aFootprint )
 {
@@ -1030,14 +1054,56 @@ bool FOOTPRINT_EDIT_FRAME::SaveFootprintAs( FOOTPRINT* aFootprint )
     wxString footprintName = aFootprint->GetFPID().GetLibItemName();
     bool     updateValue = aFootprint->GetValue() == footprintName;
     bool     done = false;
-
+    bool     footprintExists = false;
-    std::unique_ptr<EDA_LIST_DIALOG> dlg;
 
     while( !done )
     {
-        dlg.reset( buildSaveAsDialog( footprintName, libraryName ) );
+        SAVE_AS_DIALOG dlg( this, footprintName, libraryName,
+                [&]( const wxString& newLib, const wxString& newName )
+                {
+                    if( newLib.IsEmpty() )
+                    {
+                        wxMessageBox( _( "A library must be specified." ) );
+                        return false;
+                    }
 
-        int ret = dlg->ShowModal();
+                    if( newName.IsEmpty() )
+                    {
+                        wxMessageBox( _( "Footprint must have a name." ) );
+                        return false;
+                    }
+
+                    // Legacy libraries are readable, but modifying legacy format is not allowed
+                    // So prompt the user if he try to add/replace a footprint in a legacy lib
+                    const FP_LIB_TABLE_ROW* row = Prj().PcbFootprintLibs()->FindRow( libraryName );
+                    wxString                libPath = row->GetFullURI();
+                    IO_MGR::PCB_FILE_T      piType = IO_MGR::GuessPluginTypeFromLibPath( libPath );
+
+                    if( piType == IO_MGR::LEGACY )
+                    {
+                        DisplayInfoMessage( this, INFO_LEGACY_LIB_WARN_EDIT );
+                        return false;
+                    }
+
+                    footprintExists = tbl->FootprintExists( libraryName, footprintName );
+
+                    if( footprintExists )
+                    {
+                        wxString msg = wxString::Format( _( "Footprint %s already exists in %s." ),
+                                                         footprintName,
+                                                         libraryName );
+
+                        KIDIALOG errorDlg( this, msg, _( "Confirmation" ),
+                                           wxOK | wxCANCEL | wxICON_WARNING );
+                        errorDlg.SetOKLabel( _( "Overwrite" ) );
+
+                        return errorDlg.ShowModal() == wxID_OK;
+                    }
+
+                    return true;
+                } );
+
+        int ret = dlg.ShowModal();
 
         if( ret == wxID_CANCEL )
         {
@@ -1045,6 +1111,8 @@ bool FOOTPRINT_EDIT_FRAME::SaveFootprintAs( FOOTPRINT* aFootprint )
         }
         else if( ret == wxID_OK )
         {
+            footprintName = dlg.GetFPName();
+            libraryName = dlg.GetTextSelection();
             done = true;
         }
         else if( ret == ID_MAKE_NEW_LIBRARY )
@@ -1054,54 +1122,11 @@ bool FOOTPRINT_EDIT_FRAME::SaveFootprintAs( FOOTPRINT* aFootprint )
         }
     }
 
-    libraryName = dlg->GetTextSelection();
-
-    if( libraryName.IsEmpty() )
-    {
-        DisplayError( this, _( "No library specified.  Footprint could not be saved." ) );
-        return false;
-    }
-
-    footprintName = static_cast<wxTextCtrl*>( dlg->FindWindow( ID_SAVE_AS_NAME ) )->GetValue();
-    footprintName.Trim( true );
-    footprintName.Trim( false );
-
-    if( footprintName.IsEmpty() )
-    {
-        DisplayError( this, _( "No footprint name specified.  Footprint could not be saved." ) );
-        return false;
-    }
-
     aFootprint->SetFPID( LIB_ID( libraryName, footprintName ) );
 
     if( updateValue )
         aFootprint->SetValue( footprintName );
 
-    // Legacy libraries are readable, but modifying legacy format is not allowed
-    // So prompt the user if he try to add/replace a footprint in a legacy lib
-    wxString    libfullname = Prj().PcbFootprintLibs()->FindRow( libraryName )->GetFullURI();
-    IO_MGR::PCB_FILE_T  piType = IO_MGR::GuessPluginTypeFromLibPath( libfullname );
-
-    if( piType == IO_MGR::LEGACY )
-    {
-        DisplayInfoMessage( this, INFO_LEGACY_LIB_WARN_EDIT );
-        return false;
-    }
-
-    bool footprintExists = tbl->FootprintExists( libraryName, footprintName );
-
-    if( footprintExists )
-    {
-        wxString msg = wxString::Format( _( "Footprint %s already exists in %s." ),
-                                         footprintName,
-                                         libraryName );
-        KIDIALOG chkdlg( this, msg, _( "Confirmation" ), wxOK | wxCANCEL | wxICON_WARNING );
-        chkdlg.SetOKLabel( _( "Overwrite" ) );
-
-        if( chkdlg.ShowModal() == wxID_CANCEL )
-            return false;
-    }
-
     if( !SaveFootprintInLibrary( aFootprint, libraryName ) )
         return false;
 
@@ -1150,9 +1175,42 @@ bool FOOTPRINT_EDIT_FRAME::RevertFootprint()
 }
 
 
-FOOTPRINT* PCB_BASE_FRAME::CreateNewFootprint( const wxString& aFootprintName, bool aQuiet )
+class NEW_FP_DIALOG : public WX_TEXT_ENTRY_DIALOG
 {
+public:
+    NEW_FP_DIALOG( PCB_BASE_FRAME* aParent, const wxString& aName, int aFootprintType,
+                   std::function<bool( wxString newName )> aValidator ) :
+            WX_TEXT_ENTRY_DIALOG( aParent, _( "Enter footprint name:" ), _( "New Footprint" ),
+                                  aName, _( "Footprint type:" ),
+                                  { _( "Through hole" ), _( "SMD" ), _( "Other" ) },
+                                  aFootprintType ),
+            m_validator( std::move( aValidator ) )
+    { }
+
+    wxString GetFPName()
+    {
+        wxString name = m_textCtrl->GetValue();
+        name.Trim( true ).Trim( false );
+        return name;
+    }
+
+protected:
+    bool TransferDataFromWindow() override
+    {
+        return m_validator( GetFPName() );
+    }
+
+private:
+    std::function<bool( wxString newName )> m_validator;
+};
+
+
+FOOTPRINT* PCB_BASE_FRAME::CreateNewFootprint( const wxString& aFootprintName,
+                                               const wxString& aLibName, bool aQuiet )
+{
+    FP_LIB_TABLE* tbl = Prj().PcbFootprintLibs();
     wxString      footprintName = aFootprintName;
+    wxString      msg;
 
     // Static to store user preference for a session
     static int footprintType = 1;
@@ -1161,15 +1219,36 @@ FOOTPRINT* PCB_BASE_FRAME::CreateNewFootprint( const wxString& aFootprintName, b
     // Ask for the new footprint name
     if( footprintName.IsEmpty() && !aQuiet )
     {
-        WX_TEXT_ENTRY_DIALOG dlg( this, _( "Enter footprint name:" ), _( "New Footprint" ),
+        NEW_FP_DIALOG dlg( this, footprintName, footprintType,
-                                  footprintName, _( "Footprint type:" ),
+                [&]( wxString newName )
-                                  { _( "Through hole" ), _( "SMD" ), _( "Other" ) },
+                {
-                                  footprintType );
+                    if( newName.IsEmpty() )
+                    {
+                        wxMessageBox( _( "Footprint must have a name." ) );
+                        return false;
+                    }
+
+                    if( !aLibName.IsEmpty() && tbl->FootprintExists( aLibName, newName ) )
+                    {
+                        msg = wxString::Format( _( "Footprint '%s' already exists in library '%s'." ),
+                                                newName, aLibName );
+
+                        KIDIALOG errorDlg( this, msg, _( "Confirmation" ),
+                                           wxOK | wxCANCEL | wxICON_WARNING );
+                        errorDlg.SetOKLabel( _( "Overwrite" ) );
+
+                        return errorDlg.ShowModal() == wxID_OK;
+                    }
+
+                    return true;
+                } );
+
         dlg.SetTextValidator( FOOTPRINT_NAME_VALIDATOR( &footprintName ) );
 
         if( dlg.ShowModal() != wxID_OK )
             return nullptr;    //Aborted by user
 
+        footprintName = dlg.GetFPName();
         footprintType = dlg.GetChoice();
 
         switch( footprintType )
@@ -1180,17 +1259,6 @@ FOOTPRINT* PCB_BASE_FRAME::CreateNewFootprint( const wxString& aFootprintName, b
         }
     }
 
-    footprintName.Trim( true );
-    footprintName.Trim( false );
-
-    if( footprintName.IsEmpty() )
-    {
-        if( !aQuiet )
-            DisplayInfoMessage( this, _( "No footprint name defined." ) );
-
-        return nullptr;
-    }
-
     // Creates the new footprint and add it to the head of the linked list of footprints
     FOOTPRINT* footprint = new FOOTPRINT( GetBoard() );
 

pcbnew/microwave/microwave_footprint.cpp

@@ -192,7 +192,7 @@ FOOTPRINT* MICROWAVE_TOOL::createBaseFootprint( const wxString& aValue,
 {
     PCB_EDIT_FRAME& editFrame = *getEditFrame<PCB_EDIT_FRAME>();
 
-    FOOTPRINT* footprint = editFrame.CreateNewFootprint( aValue, true );
+    FOOTPRINT* footprint = editFrame.CreateNewFootprint( aValue, wxEmptyString, true );
 
     footprint->SetAttributes( FP_EXCLUDE_FROM_POS_FILES | FP_EXCLUDE_FROM_BOM );
 

pcbnew/microwave/microwave_inductor.cpp

@@ -407,7 +407,7 @@ FOOTPRINT* MICROWAVE_TOOL::createMicrowaveInductor( MICROWAVE_INDUCTOR_PATTERN&
     if( ( cmpdlg.ShowQuasiModal() != wxID_OK ) || msg.IsEmpty() )
         return nullptr;    //  Aborted by user
 
-    FOOTPRINT* footprint = editFrame->CreateNewFootprint( msg, true );
+    FOOTPRINT* footprint = editFrame->CreateNewFootprint( msg, wxEmptyString, true );
 
     footprint->SetFPID( LIB_ID( wxEmptyString, wxT( "mw_inductor" ) ) );
     footprint->SetAttributes( FP_EXCLUDE_FROM_POS_FILES | FP_EXCLUDE_FROM_BOM );

pcbnew/tools/footprint_editor_control.cpp

@@ -151,7 +151,8 @@ bool FOOTPRINT_EDITOR_CONTROL::Init()
 int FOOTPRINT_EDITOR_CONTROL::NewFootprint( const TOOL_EVENT& aEvent )
 {
     LIB_ID     selected = m_frame->GetTreeFPID();
-    FOOTPRINT* newFootprint = m_frame->CreateNewFootprint( wxEmptyString );
+    wxString   libraryName = selected.GetUniStringLibNickname();
+    FOOTPRINT* newFootprint = m_frame->CreateNewFootprint( wxEmptyString, libraryName, false );
 
     if( !newFootprint )
         return 0;
@@ -396,43 +397,69 @@ int FOOTPRINT_EDITOR_CONTROL::DuplicateFootprint( const TOOL_EVENT& aEvent )
 }
 
 
+class RENAME_DIALOG : public wxTextEntryDialog
+{
+public:
+    RENAME_DIALOG( wxWindow* aParent, const wxString& aName,
+                   std::function<bool( wxString newName )> aValidator ) :
+            wxTextEntryDialog( aParent, _( "New name:" ), _( "Change Footprint Name" ), aName ),
+            m_validator( std::move( aValidator ) )
+    { }
+
+    wxString GetFPName()
+    {
+        wxString name = m_textctrl->GetValue();
+        name.Trim( true ).Trim( false );
+        return name;
+    }
+
+protected:
+    bool TransferDataFromWindow() override
+    {
+        return m_validator( GetFPName() );
+    }
+
+private:
+    std::function<bool( wxString newName )> m_validator;
+};
+
+
 int FOOTPRINT_EDITOR_CONTROL::RenameFootprint( const TOOL_EVENT& aEvent )
 {
     FP_LIB_TABLE* tbl = m_frame->Prj().PcbFootprintLibs();
     LIB_ID        fpID = m_frame->GetTreeFPID();
     wxString      libraryName = fpID.GetLibNickname();
     wxString      oldName = fpID.GetLibItemName();
-    wxString      newName = oldName;
+    wxString      msg;
-    bool          done = false;
 
-    while( !done )
+    RENAME_DIALOG dlg( m_frame, oldName,
+            [&]( wxString newName )
             {
-        wxTextEntryDialog dlg( m_frame, _( "New name:" ), _( "Change Footprint Name" ), newName );
+                if( newName.IsEmpty() )
+                {
+                    wxMessageBox( _( "Footprint must have a name." ) );
+                    return false;
+                }
+
+                if( tbl->FootprintExists( libraryName, newName ) )
+                {
+                    msg = wxString::Format( _( "Footprint '%s' already exists in library '%s'." ),
+                                            newName, libraryName );
+
+                    KIDIALOG errorDlg( m_frame, msg, _( "Confirmation" ),
+                                       wxOK | wxCANCEL | wxICON_WARNING );
+                    errorDlg.SetOKLabel( _( "Overwrite" ) );
+
+                    return errorDlg.ShowModal() == wxID_OK;
+                }
+
+                return true;
+            } );
 
     if( dlg.ShowModal() != wxID_OK )
         return 0;   // canceled by user
 
-        newName = dlg.GetValue();
+    wxString   newName = dlg.GetFPName();
-        newName.Trim( true ).Trim( false );
-
-        if( newName.IsEmpty() )
-        {
-            DisplayErrorMessage( m_frame, _( "Footprint name cannot be empty." ) );
-        }
-        else if( tbl->FootprintExists( libraryName, newName ) )
-        {
-            DisplayErrorMessage( m_frame, wxString::Format( _( "Footprint name '%s' already "
-                                                               "in use in library '%s'." ),
-                                                            UnescapeString( newName ),
-                                                            libraryName ) );
-            newName = oldName;
-        }
-        else
-        {
-            done = true;
-        }
-    }
-
     FOOTPRINT* footprint = nullptr;
 
     if( fpID == m_frame->GetLoadedFPID() )