Fix bugs in sheet duplicate & copy/paste.

Don't edit sheet during AddToScreenAndUndoList() call.  If it's
cancelled and we delete the item, callers will still own pointers
to the freed memory.  Do it in New and Paste instead.

eeschema/operations_on_items_lists.cpp

@@ -183,11 +183,13 @@ SCH_ITEM* DuplicateItem( SCH_ITEM* aItem, bool doClone )
 
     if( newItem->Type() == SCH_COMPONENT_T )
     {
-        for( SCH_PIN& pin : static_cast<SCH_COMPONENT*>( newItem )->GetPins() )
+        SCH_COMPONENT* component = (SCH_COMPONENT*) newItem;
+
+        for( SCH_PIN& pin : component->GetPins() )
             pin.ClearFlags( SELECTED | HIGHLIGHTED | BRIGHTENED );
 
         std::vector<SCH_FIELD*> fields;
-        static_cast<SCH_COMPONENT*>( newItem )->GetFields( fields, false );
+        component->GetFields( fields, false );
 
         for( SCH_FIELD* field : fields )
             field->ClearFlags( SELECTED | HIGHLIGHTED | BRIGHTENED );
@@ -195,7 +197,9 @@ SCH_ITEM* DuplicateItem( SCH_ITEM* aItem, bool doClone )
 
     if( newItem->Type() == SCH_SHEET_T )
     {
-        for( SCH_SHEET_PIN& pin : static_cast<SCH_SHEET*>( newItem )->GetPins() )
+        SCH_SHEET* sheet = (SCH_SHEET*) newItem;
+
+        for( SCH_SHEET_PIN& pin : sheet->GetPins() )
             pin.ClearFlags( SELECTED | HIGHLIGHTED | BRIGHTENED );
     }
 

eeschema/sch_edit_frame.cpp

@@ -1203,29 +1203,7 @@ void SCH_EDIT_FRAME::AddItemToScreenAndUndoList( SCH_ITEM* aItem, bool aUndoAppe
 
     if( aItem->IsNew() )
     {
-        // When a new sheet is added to the hierarchy, a clear annotation can be needed
+        if( aItem->Type() == SCH_SHEET_PIN_T )
-        // for all new sheet paths added by the new sheet (if this sheet is loaded from
-        // and existing sheet or a existing file, it can also contain subsheets)
-        bool doClearAnnotation = false;
-        SCH_SHEET_LIST initial_sheetpathList( g_RootSheet );
-
-        if( aItem->Type() == SCH_SHEET_T )
-        {
-            if( !EditSheet( (SCH_SHEET*)aItem, g_CurrentSheet, &doClearAnnotation ) )
-            {
-                delete aItem;
-                return;
-            }
-
-            SetSheetNumberAndCount();
-
-            if( !screen->CheckIfOnDrawList( aItem ) )  // don't want a loop!
-                AddToScreen( aItem );
-
-            SetRepeatItem( aItem );
-            SaveCopyInUndoList( undoItem, UR_NEW, aUndoAppend );
-        }
-        else if( aItem->Type() == SCH_SHEET_PIN_T )
         {
             // Sheet pins are owned by their parent sheet.
             SaveCopyInUndoList( undoItem, UR_CHANGED, aUndoAppend );     // save the parent sheet
@@ -1247,14 +1225,6 @@ void SCH_EDIT_FRAME::AddItemToScreenAndUndoList( SCH_ITEM* aItem, bool aUndoAppe
             SaveCopyInUndoList( undoItem, UR_NEW, aUndoAppend );
         }
 
-        if( doClearAnnotation )
-        {
-            // Clear annotation of new sheet paths: the new sheet and its sub-sheets
-            // If needed the missing alternate references of components will be created
-            SCH_SCREENS screensList( g_RootSheet );
-            screensList.ClearAnnotationOfNewSheetPaths( initial_sheetpathList );
-        }
-
         // Update connectivity info for new item
         if( !aItem->IsMoving() )
             RecalculateConnections();

eeschema/sch_edit_frame.h

@@ -818,11 +818,6 @@ private:
     void OnEditComponentSymbolsId( wxCommandEvent& aEvent );
     void OnPreferencesOptions( wxCommandEvent& event );
 
-    /**
-     * Command event handler for duplicating the item at the current location.
-     */
-    void OnDuplicateItem( wxCommandEvent& event );
-
     /* User interface update event handlers. */
     void OnUpdatePaste( wxUpdateUIEvent& event );
     void OnUpdateHiddenPins( wxUpdateUIEvent& event );
@@ -924,6 +919,8 @@ public:
      */
     bool EditSheet( SCH_SHEET* aSheet, SCH_SHEET_PATH* aHierarchy, bool* aClearAnnotationNewItems );
 
+    void InitSheet( SCH_SHEET* aSheet, const wxString& aFilename );
+
     wxPoint GetLastSheetPinPosition() const { return m_lastSheetPinPosition; }
 
 private:

eeschema/sheet.cpp

@@ -37,6 +37,15 @@
 #include <dialogs/dialog_sch_edit_sheet_pin.h>
 
 
+void SCH_EDIT_FRAME::InitSheet( SCH_SHEET* aSheet, const wxString& aFilename )
+{
+    aSheet->SetScreen( new SCH_SCREEN( &Kiway() ) );
+    aSheet->GetScreen()->SetModify();
+    aSheet->GetScreen()->SetMaxUndoItems( m_UndoRedoCountMax );
+    aSheet->GetScreen()->SetFileName( aFilename );
+}
+
+
 bool SCH_EDIT_FRAME::EditSheet( SCH_SHEET* aSheet, SCH_SHEET_PATH* aHierarchy,
                                 bool* aClearAnnotationNewItems )
 {
@@ -108,10 +117,7 @@ bool SCH_EDIT_FRAME::EditSheet( SCH_SHEET* aSheet, SCH_SHEET_PATH* aHierarchy,
         }
         else                                                   // New file.
         {
-            aSheet->SetScreen( new SCH_SCREEN( &Kiway() ) );
+            InitSheet( aSheet, newFilename );
-            aSheet->GetScreen()->SetModify();
-            aSheet->GetScreen()->SetMaxUndoItems( m_UndoRedoCountMax );
-            aSheet->GetScreen()->SetFileName( newFilename );
         }
     }
     else                                                       // Existing sheet.

eeschema/tools/sch_edit_tool.cpp

@@ -682,6 +682,8 @@ int SCH_EDIT_TOOL::Main( const TOOL_EVENT& aEvent )
 
         else if( TOOL_EVT_UTILS::IsCancelInteractive( evt.get() ) )
         {
+            m_toolMgr->RunAction( SCH_ACTIONS::clearSelection, true );
+
             if( m_moveInProgress )
                 restore_state = true;
 
@@ -704,6 +706,13 @@ int SCH_EDIT_TOOL::Main( const TOOL_EVENT& aEvent )
             }
             else if( evt->IsAction( &SCH_ACTIONS::duplicate ) )
             {
+                if( selection.Front()->IsNew() )
+                {
+                    // This doesn't really make sense; we'll just end up dragging a stack of
+                    // objects so Duplicate() is going to ignore this and we'll just carry on.
+                    continue;
+                }
+
                 // Move original back and exit.  The duplicate will run in its own loop.
                 restore_state = true;
                 unselect = false;
@@ -1204,19 +1213,22 @@ int SCH_EDIT_TOOL::Duplicate( const TOOL_EVENT& aEvent )
     if( selection.GetSize() == 0 )
         return 0;
 
+    // This doesn't really make sense; we'll just end up dragging a stack of objects...
+    if( selection.Front()->IsNew() )
+        return 0;
+
     std::vector<SCH_ITEM*> newItems;
 
     // Keep track of existing sheet paths. Duplicating a selection can modify this list
-    bool hasSheetCopied = false;
+    bool copiedSheets = false;
     SCH_SHEET_LIST initial_sheetpathList( g_RootSheet );
 
     for( unsigned ii = 0; ii < selection.GetSize(); ++ii )
     {
         SCH_ITEM* oldItem = static_cast<SCH_ITEM*>( selection.GetItem( ii ) );
         SCH_ITEM* newItem = DuplicateItem( oldItem );
-        newItems.push_back( newItem );
-
         newItem->SetFlags( IS_NEW );
+        newItems.push_back( newItem );
         saveCopyInUndoList( newItem, UR_NEW, ii > 0 );
 
         switch( newItem->Type() )
@@ -1247,7 +1259,7 @@ int SCH_EDIT_TOOL::Duplicate( const TOOL_EVENT& aEvent )
             sheet->SetParent( m_frame->GetScreen() );
             m_frame->AddToScreen( sheet );
 
-            hasSheetCopied = true;
+            copiedSheets = true;
             break;
         }
 
@@ -1268,12 +1280,13 @@ int SCH_EDIT_TOOL::Duplicate( const TOOL_EVENT& aEvent )
         }
     }
 
-    if( hasSheetCopied )
+    if( copiedSheets )
     {
         // We clear annotation of new sheet paths.
         // Annotation of new components added in current sheet is already cleared.
         SCH_SCREENS screensList( g_RootSheet );
         screensList.ClearAnnotationOfNewSheetPaths( initial_sheetpathList );
+        m_frame->SetSheetNumberAndCount();
     }
 
     m_toolMgr->RunAction( SCH_ACTIONS::clearSelection, true );

eeschema/tools/sch_editor_control.cpp

@@ -580,7 +580,7 @@ int SCH_EDITOR_CONTROL::Paste( const TOOL_EVENT& aEvent )
     // Moreover new sheets create new sheetpaths, and component alternate references must
     // be created and cleared
     //
-    bool           hasSheetPasted = false;
+    bool           sheetsPasted = false;
     SCH_SHEET_LIST hierarchy( g_RootSheet );
     SCH_SHEET_LIST initialHierarchy( g_RootSheet );
 
@@ -634,7 +634,7 @@ int SCH_EDITOR_CONTROL::Paste( const TOOL_EVENT& aEvent )
 
                 sheet->SetName( wxString::Format( wxT( "sheet%8.8lX" ), (unsigned long)uid ) );
                 sheet->SetTimeStamp( uid );
-                hasSheetPasted = true;
+                sheetsPasted = true;
             }
         }
     }
@@ -653,16 +653,22 @@ int SCH_EDITOR_CONTROL::Paste( const TOOL_EVENT& aEvent )
             SCH_COMPONENT* component = (SCH_COMPONENT*) item;
             component->Resolve( *symLibTable, partLib );
         }
+        else if( item->Type() == SCH_SHEET_T )
+        {
+            SCH_SHEET* sheet = (SCH_SHEET*) item;
+            m_frame->InitSheet( sheet, sheet->GetFileName() );
+        }
 
         item->SetFlags( IS_NEW | IS_MOVED );
         m_frame->AddItemToScreenAndUndoList( item, i > 0 );
     }
 
-    if( hasSheetPasted )
+    if( sheetsPasted )
     {
         // We clear annotation of new sheet paths.
         SCH_SCREENS screensList( g_RootSheet );
         screensList.ClearAnnotationOfNewSheetPaths( initialHierarchy );
+        m_frame->SetSheetNumberAndCount();
     }
 
     // Now clear the previous selection, select the pasted items, and fire up the "move"