From 3e431d0d392d852ee87ff892173be7971207799a Mon Sep 17 00:00:00 2001 From: Wayne Stambaugh Date: Fri, 13 Dec 2019 16:51:59 -0500 Subject: [PATCH] Symbol editor: fix inherited symbol editing bug. The library manager update part function was orphaning the root symbol of derived symbols when the root symbol was edited. Re-parent the inherited symbols when updating a root symbol. Update the mandatory field attributes whenever the parent is set for derived parts. This will show the fields with the correct attributes in the symbol editor and viewer. Using std::unique_ptr to hold the current symbols was deleting the pointer on symbol selection changes causing the pointer in the library manager buffer to be stale in some cases. Revert back to using a simple pointer and manual clean up as required. Prevent derived symbols from being saved to a different library to prevent orphaned symbols. Fixes kicad/code/kicad#3649 --- eeschema/class_libentry.cpp | 43 ++++++++++++++++--- .../dialogs/dialog_edit_component_in_lib.cpp | 5 ++- eeschema/libedit/lib_edit_frame.cpp | 12 ++++-- eeschema/libedit/lib_edit_frame.h | 4 +- eeschema/libedit/lib_manager.cpp | 22 +++++++++- eeschema/libedit/libedit.cpp | 18 +++++--- eeschema/libedit/libedit_undo_redo.cpp | 8 ++-- eeschema/libedit/symbedit.cpp | 6 +-- 8 files changed, 92 insertions(+), 26 deletions(-) diff --git a/eeschema/class_libentry.cpp b/eeschema/class_libentry.cpp index ac63cf08d8..fdf480fc7c 100644 --- a/eeschema/class_libentry.cpp +++ b/eeschema/class_libentry.cpp @@ -96,9 +96,6 @@ LIB_PART::LIB_PART( const wxString& aName, LIB_PART* aParent, PART_LIB* aLibrary m_showPinNumbers = true; m_showPinNames = true; - if( aParent ) - m_parent = aParent->SharedPtr(); - // Add the MANDATORY_FIELDS in RAM only. These are assumed to be present // when the field editors are invoked. m_drawings[LIB_FIELD_T].reserve( 4 ); @@ -107,8 +104,12 @@ LIB_PART::LIB_PART( const wxString& aName, LIB_PART* aParent, PART_LIB* aLibrary m_drawings[LIB_FIELD_T].push_back( new LIB_FIELD( this, FOOTPRINT ) ); m_drawings[LIB_FIELD_T].push_back( new LIB_FIELD( this, DATASHEET ) ); - SetLib( aLibrary ); SetName( aName ); + + if( aParent ) + SetParent( aParent ); + + SetLib( aLibrary ); } @@ -120,7 +121,6 @@ LIB_PART::LIB_PART( const LIB_PART& aPart, PART_LIB* aLibrary ) : m_library = aLibrary; m_name = aPart.m_name; - m_parent = aPart.m_parent; m_FootprintList = wxArrayString( aPart.m_FootprintList ); m_unitCount = aPart.m_unitCount; m_unitsLocked = aPart.m_unitsLocked; @@ -143,6 +143,11 @@ LIB_PART::LIB_PART( const LIB_PART& aPart, PART_LIB* aLibrary ) : newItem->SetParent( this ); m_drawings.push_back( newItem ); } + + PART_SPTR parent = aPart.m_parent.lock(); + + if( parent ) + SetParent( parent.get() ); } @@ -260,9 +265,37 @@ void LIB_PART::SetName( const wxString& aName ) void LIB_PART::SetParent( LIB_PART* aParent ) { if( aParent ) + { m_parent = aParent->SharedPtr(); + + // Inherit the parent mandatory field attributes. + for( int id=0; idGetField( id ); + + wxASSERT( parentField ); + + wxString name = field->GetText(); + + *field = *parentField; + + if( id == VALUE ) + field->SetText( name ); + else if( id == DATASHEET && !GetDocFileName().IsEmpty() ) + field->SetText( GetDocFileName() ); + + field->SetParent( this ); + } + } else + { m_parent.reset(); + } } diff --git a/eeschema/dialogs/dialog_edit_component_in_lib.cpp b/eeschema/dialogs/dialog_edit_component_in_lib.cpp index 2add8156c0..24eec9ae2c 100644 --- a/eeschema/dialogs/dialog_edit_component_in_lib.cpp +++ b/eeschema/dialogs/dialog_edit_component_in_lib.cpp @@ -153,8 +153,9 @@ bool DIALOG_EDIT_COMPONENT_IN_LIBRARY::TransferDataToWindow() // Push a copy of each field into m_fields m_libEntry->GetFields( *m_fields ); - // Copy the data sheet field from the old alias document file name. - m_fields->at( DATASHEET ).SetText( m_libEntry->GetDocFileName() ); + // Copy the data sheet field from the old alias document file name if it's not empty. + if( !m_libEntry->GetDocFileName().IsEmpty() ) + m_fields->at( DATASHEET ).SetText( m_libEntry->GetDocFileName() ); // The Y axis for components in lib is from bottom to top while the screen axis is top // to bottom: we must change the y coord sign for editing diff --git a/eeschema/libedit/lib_edit_frame.cpp b/eeschema/libedit/lib_edit_frame.cpp index 1eb147fda9..fc03450b9c 100644 --- a/eeschema/libedit/lib_edit_frame.cpp +++ b/eeschema/libedit/lib_edit_frame.cpp @@ -105,6 +105,7 @@ LIB_EDIT_FRAME::LIB_EDIT_FRAME( KIWAY* aKiway, wxWindow* aParent ) : SetShowElectricalType( true ); m_FrameSize = ConvertDialogToPixels( wxSize( 500, 350 ) ); // default in case of no prefs + m_my_part = nullptr; m_treePane = nullptr; m_libMgr = nullptr; m_unit = 1; @@ -367,7 +368,10 @@ void LIB_EDIT_FRAME::SetCurPart( LIB_PART* aPart ) { m_toolManager->RunAction( EE_ACTIONS::clearSelection, true ); - m_my_part.reset( aPart ); + if( m_my_part ) + delete m_my_part; + + m_my_part = aPart; // select the current component in the tree widget if( m_my_part ) @@ -507,7 +511,7 @@ LIB_PART* LIB_EDIT_FRAME::getTargetPart() const return alias; } - return m_my_part.get(); + return m_my_part; } @@ -639,7 +643,7 @@ bool LIB_EDIT_FRAME::backupFile( const wxFileName& aOriginalFile, const wxString void LIB_EDIT_FRAME::storeCurrentPart() { if( m_my_part && !GetCurLib().IsEmpty() && GetScreen()->IsModify() ) - m_libMgr->UpdatePart( m_my_part.get(), GetCurLib() ); // UpdatePart() makes a copy + m_libMgr->UpdatePart( m_my_part, GetCurLib() ); // UpdatePart() makes a copy } @@ -702,7 +706,7 @@ void LIB_EDIT_FRAME::RebuildView() GetRenderSettings()->m_ShowUnit = m_unit; GetRenderSettings()->m_ShowConvert = m_convert; GetRenderSettings()->m_ShowDisabled = m_my_part && m_my_part->IsAlias(); - GetCanvas()->DisplayComponent( m_my_part.get() ); + GetCanvas()->DisplayComponent( m_my_part ); GetCanvas()->GetView()->HideWorksheet(); GetCanvas()->GetView()->ClearHiddenFlags(); diff --git a/eeschema/libedit/lib_edit_frame.h b/eeschema/libedit/lib_edit_frame.h index 9730a5ccb0..b17ae2dafe 100644 --- a/eeschema/libedit/lib_edit_frame.h +++ b/eeschema/libedit/lib_edit_frame.h @@ -49,7 +49,7 @@ class LIB_MANAGER; */ class LIB_EDIT_FRAME : public SCH_BASE_FRAME { - std::unique_ptr< LIB_PART > m_my_part; // a part I own, it is not in any library, but a copy + LIB_PART* m_my_part; // a part I own, it is not in any library, but a copy // could be. wxComboBox* m_unitSelectBox; // a ComboBox to select a unit to edit (if the part // has multiple units) @@ -149,7 +149,7 @@ public: * * This is a LIB_PART that I own, it is at best a copy of one in a library. */ - LIB_PART* GetCurPart() { return m_my_part.get(); } + LIB_PART* GetCurPart() { return m_my_part; } /** * Take ownership of aPart and notes that it is the one currently being edited. diff --git a/eeschema/libedit/lib_manager.cpp b/eeschema/libedit/lib_manager.cpp index 9eb45142fb..d3c99b0ca4 100644 --- a/eeschema/libedit/lib_manager.cpp +++ b/eeschema/libedit/lib_manager.cpp @@ -390,6 +390,26 @@ bool LIB_MANAGER::UpdatePart( LIB_PART* aPart, const wxString& aLibrary ) if( partBuf ) { + if( partBuf->GetPart()->IsRoot() && libBuf.HasDerivedSymbols( aPart->GetName() ) ) + { + // Reparent derived symbols. + for( auto entry : libBuf.GetBuffers() ) + { + if( entry->GetPart()->IsRoot() ) + continue; + + if( entry->GetPart()->GetParent().lock() == partBuf->GetPart()->SharedPtr() ) + { + if( !libBuf.DeleteBuffer( entry ) ) + return false; + + LIB_PART* reparentedPart = new LIB_PART( *entry->GetPart() ); + reparentedPart->SetParent( partCopy ); + libBuf.CreateBuffer( reparentedPart, new SCH_SCREEN( &m_frame.Kiway() ) ); + } + } + } + libBuf.UpdateBuffer( partBuf, partCopy ); } else // New entry @@ -761,7 +781,7 @@ LIB_MANAGER::LIB_BUFFER& LIB_MANAGER::getLibraryBuffer( const wxString& aLibrary } else if( !buf.GetPart( part->GetName() ) ) { - buf.CreateBuffer( new LIB_PART( *part, nullptr ), new SCH_SCREEN( &m_frame.Kiway() ) ); + buf.CreateBuffer( new LIB_PART( *part ), new SCH_SCREEN( &m_frame.Kiway() ) ); } } diff --git a/eeschema/libedit/libedit.cpp b/eeschema/libedit/libedit.cpp index b26a0ee04d..47f5059683 100644 --- a/eeschema/libedit/libedit.cpp +++ b/eeschema/libedit/libedit.cpp @@ -251,7 +251,7 @@ bool LIB_EDIT_FRAME::LoadOneLibraryPartAux( LIB_PART* aEntry, const wxString& aL m_toolManager->RunAction( ACTIONS::zoomFitScreen, true ); updateTitle(); RebuildSymbolUnitsList(); - SetShowDeMorgan( GetCurPart()->Flatten()->HasConversion() ); + SetShowDeMorgan( GetCurPart()->HasConversion() ); SyncToolbars(); // Display the document information based on the entry selected just in @@ -464,6 +464,16 @@ void LIB_EDIT_FRAME::savePartAs() return; } + // @todo Either check the selecteced library to see if the parent symbol name is in + // the new library and/or copy the parent symbol as well. This is the lazy + // solution to ensure derived parts do not get orphaned. + if( part->IsAlias() && new_lib != old_lib ) + { + DisplayError( this, _( "Derived symbols must be save in the same library\n" + "that the parent symbol exists." ) ); + return; + } + wxString new_name = nameTextCtrl->GetValue(); new_name.Trim( true ); new_name.Trim( false ); @@ -490,9 +500,7 @@ void LIB_EDIT_FRAME::savePartAs() m_libMgr->UpdatePart( &new_part, new_lib ); SyncLibraries( false ); m_treePane->GetLibTree()->SelectLibId( LIB_ID( new_lib, new_part.GetName() ) ); - - if( isCurrentPart( old_lib_id ) ) - LoadPart( new_name, new_lib, m_unit ); + LoadPart( new_name, new_lib, m_unit ); } } @@ -517,7 +525,7 @@ void LIB_EDIT_FRAME::UpdateAfterSymbolProperties( wxString* aOldName, wxArrayStr m_my_part->SetName( *aOldName ); } else - m_libMgr->UpdatePartAfterRename( m_my_part.get(), *aOldName, lib ); + m_libMgr->UpdatePartAfterRename( m_my_part, *aOldName, lib ); } // Reselect the renamed part diff --git a/eeschema/libedit/libedit_undo_redo.cpp b/eeschema/libedit/libedit_undo_redo.cpp index 1b6f93d3e8..6718141428 100644 --- a/eeschema/libedit/libedit_undo_redo.cpp +++ b/eeschema/libedit/libedit_undo_redo.cpp @@ -75,7 +75,7 @@ void LIB_EDIT_FRAME::GetComponentFromRedoList() // Store the current part in the undo buffer PICKED_ITEMS_LIST* undoCommand = new PICKED_ITEMS_LIST(); - LIB_PART* oldPart = m_my_part.get(); + LIB_PART* oldPart = m_my_part; oldPart->SetFlags( UR_TRANSIENT ); ITEM_PICKER undoWrapper( oldPart, undoRedoType ); undoCommand->PushItem( undoWrapper ); @@ -85,7 +85,7 @@ void LIB_EDIT_FRAME::GetComponentFromRedoList() // which calls delete . // is now put in undo list and is owned by this list // Just set the current part to the part which come from the redo list - m_my_part.reset( part ); + m_my_part = part; if( undoRedoType == UR_LIB_RENAME ) { @@ -123,7 +123,7 @@ void LIB_EDIT_FRAME::GetComponentFromUndoList() // Store the current part in the redo buffer PICKED_ITEMS_LIST* redoCommand = new PICKED_ITEMS_LIST(); - LIB_PART* oldPart = m_my_part.get(); + LIB_PART* oldPart = m_my_part; oldPart->SetFlags( UR_TRANSIENT ); ITEM_PICKER redoWrapper( oldPart, undoRedoType ); redoCommand->PushItem( redoWrapper ); @@ -133,7 +133,7 @@ void LIB_EDIT_FRAME::GetComponentFromUndoList() // which calls delete . // is now put in redo list and is owned by this list. // Just set the current part to the part which come from the undo list - m_my_part.reset( part ); + m_my_part = part; if( undoRedoType == UR_LIB_RENAME ) { diff --git a/eeschema/libedit/symbedit.cpp b/eeschema/libedit/symbedit.cpp index a38ef989c1..39370e4421 100644 --- a/eeschema/libedit/symbedit.cpp +++ b/eeschema/libedit/symbedit.cpp @@ -109,7 +109,7 @@ void LIB_EDIT_FRAME::LoadOneSymbol() wxCHECK_RET( alias, "Invalid symbol." ); - SaveCopyInUndoList( m_my_part.get() ); + SaveCopyInUndoList( m_my_part ); LIB_PART* first = alias; LIB_ITEMS_CONTAINER& drawList = first->GetDrawItems(); @@ -129,7 +129,7 @@ void LIB_EDIT_FRAME::LoadOneSymbol() LIB_ITEM* newItem = (LIB_ITEM*) item.Clone(); - newItem->SetParent( m_my_part.get() ); + newItem->SetParent( m_my_part ); m_my_part->AddDrawItem( newItem ); item.ClearSelected(); } @@ -189,7 +189,7 @@ void LIB_EDIT_FRAME::SaveOneSymbol() plugin->CreateSymbolLib( fn.GetFullPath(), &nodoc_props ); // The part gets flattened by the LIB_PART copy constructor. - LIB_PART* saved_part = new LIB_PART( *m_my_part.get() ); + LIB_PART* saved_part = new LIB_PART( *m_my_part ); plugin->SaveSymbol( fn.GetFullPath(), saved_part, &nodoc_props ); } catch( const IO_ERROR& ioe )