From 286716a1ff71d799c8cd66e9ece81aaba7195866 Mon Sep 17 00:00:00 2001 From: Jeff Young Date: Mon, 12 Jun 2023 19:33:52 +0100 Subject: [PATCH] Retire AddItemToCommitAndScreen() It duplicates a bunch of stuff in SCH_COMMIT and isn't really compatible with it. Fixes https://gitlab.com/kicad/code/kicad/-/issues/14933 --- eeschema/sch_edit_frame.cpp | 97 +-------------------------- eeschema/sch_edit_frame.h | 5 -- eeschema/sch_item.cpp | 7 +- eeschema/tools/backannotate.cpp | 2 +- eeschema/tools/sch_drawing_tools.cpp | 62 ++++++++++------- eeschema/tools/sch_editor_control.cpp | 6 +- 6 files changed, 48 insertions(+), 131 deletions(-) diff --git a/eeschema/sch_edit_frame.cpp b/eeschema/sch_edit_frame.cpp index cec3906bd1..ccd7e4f0d8 100644 --- a/eeschema/sch_edit_frame.cpp +++ b/eeschema/sch_edit_frame.cpp @@ -1602,100 +1602,6 @@ void SCH_EDIT_FRAME::AutoRotateItem( SCH_SCREEN* aScreen, SCH_ITEM* aItem ) } -void SCH_EDIT_FRAME::AddItemToCommitAndScreen( SCH_COMMIT* aCommit, SCH_SCREEN* aScreen, - SCH_ITEM* aItem ) -{ - wxCHECK_RET( aItem != nullptr, wxT( "Cannot add null item to list." ) ); - - SCH_SHEET* parentSheet = nullptr; - SCH_SYMBOL* parentSymbol = nullptr; - SCH_ITEM* undoItem = aItem; - - if( aItem->Type() == SCH_SHEET_PIN_T ) - { - parentSheet = (SCH_SHEET*) aItem->GetParent(); - - wxCHECK_RET( parentSheet && parentSheet->Type() == SCH_SHEET_T, - wxT( "Cannot place sheet pin in invalid schematic sheet." ) ); - - undoItem = parentSheet; - } - - else if( aItem->Type() == SCH_FIELD_T ) - { - parentSymbol = (SCH_SYMBOL*) aItem->GetParent(); - - wxCHECK_RET( parentSymbol && parentSymbol->Type() == SCH_SYMBOL_T, - wxT( "Cannot place field in invalid schematic symbol." ) ); - - undoItem = parentSymbol; - } - - if( aItem->IsNew() ) - { - if( aItem->Type() == SCH_SHEET_PIN_T ) - { - // Sheet pins are owned by their parent sheet. - aCommit->Modify( undoItem, aScreen ); - parentSheet->AddPin( (SCH_SHEET_PIN*) aItem ); - } - else if( aItem->Type() == SCH_FIELD_T ) - { - // Symbol fields are also owned by their parent, but new symbol fields are - // handled elsewhere. - wxLogMessage( wxT( "addCurrentItemToScreen: unexpected new SCH_FIELD" ) ); - } - else - { - if( !aScreen->CheckIfOnDrawList( aItem ) ) // don't want a loop! - AddToScreen( aItem, aScreen ); - - SaveCopyForRepeatItem( aItem ); - aCommit->Modify( undoItem, aScreen ); - } - - // Update connectivity info for new item - if( !aItem->IsMoving() && aItem->IsConnectable() ) - RecalculateConnections( aCommit, LOCAL_CLEANUP ); - } - - aItem->ClearFlags( IS_NEW ); - - aScreen->SetContentModified(); - UpdateItem( aItem ); - - if( !aItem->IsMoving() && aItem->IsConnectable() ) - { - std::vector pts = aItem->GetConnectionPoints(); - - bool connected = true; - - for( auto i = pts.begin(); i != pts.end(); i++ ) - { - for( auto j = i + 1; j != pts.end(); j++ ) - TrimWire( aCommit, *i, *j ); - - if( aScreen->IsExplicitJunctionNeeded( *i ) ) - { - AddJunction( aCommit, aScreen, *i ); - connected = true; - } - } - - if( connected ) - AutoRotateItem( aScreen, aItem ); - - TestDanglingEnds(); - - for( SCH_ITEM* item : aItem->ConnectedItems( GetCurrentSheet() ) ) - UpdateItem( item ); - } - - aItem->ClearEditFlags(); - GetCanvas()->Refresh(); -} - - void SCH_EDIT_FRAME::updateTitle() { SCH_SCREEN* screen = GetScreen(); @@ -1847,8 +1753,7 @@ void SCH_EDIT_FRAME::RecalculateConnections( SCH_COMMIT* aCommit, SCH_CLEANUP_FL } std::set> all_items = - Schematic().ConnectionGraph()->ExtractAffectedItems( - changed_items ); + Schematic().ConnectionGraph()->ExtractAffectedItems( changed_items ); CONNECTION_GRAPH new_graph( &Schematic() ); diff --git a/eeschema/sch_edit_frame.h b/eeschema/sch_edit_frame.h index c580d0f5c4..9bc6412978 100644 --- a/eeschema/sch_edit_frame.h +++ b/eeschema/sch_edit_frame.h @@ -247,11 +247,6 @@ public: */ void AutoRotateItem( SCH_SCREEN* aScreen, SCH_ITEM* aItem ); - /** - * Add an item to the schematic and adds the changes to the commit. - */ - void AddItemToCommitAndScreen( SCH_COMMIT* aCommit, SCH_SCREEN* aScreen, SCH_ITEM* aItem ); - /** * Run the Find or Find & Replace dialog. */ diff --git a/eeschema/sch_item.cpp b/eeschema/sch_item.cpp index bb8aea1262..21ea837622 100644 --- a/eeschema/sch_item.cpp +++ b/eeschema/sch_item.cpp @@ -149,8 +149,11 @@ SCH_CONNECTION* SCH_ITEM::Connection( const SCH_SHEET_PATH* aSheet ) const if( !IsConnectable() ) return nullptr; - wxCHECK_MSG( !IsConnectivityDirty(), nullptr, - wxT( "Shouldn't be asking for connection if connectivity is dirty!" ) ); + if( IsConnectivityDirty() ) + { + wxFAIL_MSG( wxT( "Shouldn't be asking for connection if connectivity is dirty!" ) ); + return nullptr; + } if( !aSheet ) aSheet = &Schematic()->CurrentSheet(); diff --git a/eeschema/tools/backannotate.cpp b/eeschema/tools/backannotate.cpp index 0fcf0ea7d4..de40f1bd78 100644 --- a/eeschema/tools/backannotate.cpp +++ b/eeschema/tools/backannotate.cpp @@ -638,7 +638,7 @@ void BACK_ANNOTATE::processNetNameChange( SCH_COMMIT* aCommit, const wxString& a label->SetFlags( IS_NEW ); SCH_SCREEN* screen = aConnection->Sheet().LastScreen(); - m_frame->AddItemToCommitAndScreen( aCommit, screen, label ); + aCommit->Add( label, screen ); } m_reporter.ReportHead( msg, RPT_SEVERITY_ACTION ); diff --git a/eeschema/tools/sch_drawing_tools.cpp b/eeschema/tools/sch_drawing_tools.cpp index 51dc3cbd33..6dcfcb3a85 100644 --- a/eeschema/tools/sch_drawing_tools.cpp +++ b/eeschema/tools/sch_drawing_tools.cpp @@ -111,8 +111,8 @@ int SCH_DRAWING_TOOLS::PlaceSymbol( const TOOL_EVENT& aEvent ) std::vector* historyList = nullptr; bool ignorePrimePosition = false; COMMON_SETTINGS* common_settings = Pgm().GetCommonSettings(); - SCH_COMMIT commit( m_toolMgr ); EE_GRID_HELPER grid( m_toolMgr ); + SCH_SCREEN* screen = m_frame->GetScreen(); if( m_inPlaceSymbol ) return 0; @@ -149,16 +149,17 @@ int SCH_DRAWING_TOOLS::PlaceSymbol( const TOOL_EVENT& aEvent ) m_frame->PushTool( aEvent ); auto addSymbol = - [this]( SCH_COMMIT* aCommit, SCH_SYMBOL* aSymbol ) + [this]( SCH_SYMBOL* aSymbol ) { m_frame->SaveCopyForRepeatItem( aSymbol ); m_toolMgr->RunAction( EE_ACTIONS::clearSelection, true ); m_selectionTool->AddItemToSel( aSymbol ); - aSymbol->SetParent( m_frame->GetScreen() ); aSymbol->SetFlags( IS_NEW | IS_MOVING ); - m_frame->AddItemToCommitAndScreen( aCommit, m_frame->GetScreen(), aSymbol ); + + m_view->ClearPreview(); + m_view->AddToPreview( aSymbol, false ); // Add, but not give ownership // Set IS_MOVING again, as AddItemToCommitAndScreen() will have cleared it. aSymbol->SetFlags( IS_MOVING ); @@ -176,11 +177,13 @@ int SCH_DRAWING_TOOLS::PlaceSymbol( const TOOL_EVENT& aEvent ) [&]() { m_toolMgr->RunAction( EE_ACTIONS::clearSelection, true ); - m_frame->RollbackSchematicFromUndo(); + m_view->ClearPreview(); + delete symbol; + symbol = nullptr; + existingRefs.Clear(); hierarchy.GetSymbols( existingRefs ); existingRefs.SortByReferenceOnly(); - symbol = nullptr; }; auto annotate = @@ -224,7 +227,7 @@ int SCH_DRAWING_TOOLS::PlaceSymbol( const TOOL_EVENT& aEvent ) // Prime the pump if( symbol ) { - addSymbol( &commit, symbol ); + addSymbol( symbol ); annotate(); getViewControls()->WarpMouseCursor( getViewControls()->GetMousePosition( false ) ); } @@ -325,7 +328,7 @@ int SCH_DRAWING_TOOLS::PlaceSymbol( const TOOL_EVENT& aEvent ) symbol = new SCH_SYMBOL( *libSymbol, &m_frame->GetCurrentSheet(), sel, cursorPos, &m_frame->Schematic() ); - addSymbol( &commit, symbol ); + addSymbol( symbol ); annotate(); // Update the list of references for the next symbol placement. @@ -342,12 +345,14 @@ int SCH_DRAWING_TOOLS::PlaceSymbol( const TOOL_EVENT& aEvent ) } else { - if( m_frame->eeconfig()->m_AutoplaceFields.enable ) - symbol->AutoplaceFields( /* aScreen */ nullptr, /* aManual */ false ); + m_view->ClearPreview(); + m_frame->AddToScreen( symbol, screen ); - symbol->ClearEditFlags(); - m_view->Update( symbol ); - m_frame->GetScreen()->Update( symbol ); + if( m_frame->eeconfig()->m_AutoplaceFields.enable ) + symbol->AutoplaceFields( screen, false /* aManual */ ); + + SCH_COMMIT commit( m_toolMgr ); + commit.Added( symbol, screen ); SCH_LINE_WIRE_BUS_TOOL* lwbTool = m_toolMgr->GetTool(); lwbTool->TrimOverLappingWires( &commit, &m_selectionTool->GetSelection() ); @@ -383,7 +388,7 @@ int SCH_DRAWING_TOOLS::PlaceSymbol( const TOOL_EVENT& aEvent ) if( new_unit == 1 ) nextSymbol->ClearAnnotation( nullptr, false ); - addSymbol( &commit, nextSymbol ); + addSymbol( nextSymbol ); symbol = nextSymbol; annotate(); @@ -438,7 +443,8 @@ int SCH_DRAWING_TOOLS::PlaceSymbol( const TOOL_EVENT& aEvent ) else if( symbol && ( evt->IsAction( &ACTIONS::refreshPreview ) || evt->IsMotion() ) ) { symbol->SetPosition( cursorPos ); - m_view->Update( symbol ); + m_view->ClearPreview(); + m_view->AddToPreview( symbol, false ); // Add, but not give ownership m_frame->SetMsgPanel( symbol ); } else if( symbol && evt->IsAction( &ACTIONS::doDelete ) ) @@ -634,7 +640,7 @@ int SCH_DRAWING_TOOLS::PlaceImage( const TOOL_EVENT& aEvent ) m_view->ClearPreview(); m_view->AddToPreview( image, false ); // Add, but not give ownership - m_view->RecacheAllItems(); // Bitmaps are cached in Opengl + m_view->RecacheAllItems(); // Bitmaps are cached in Opengl m_selectionTool->AddItemToSel( image ); @@ -644,7 +650,7 @@ int SCH_DRAWING_TOOLS::PlaceImage( const TOOL_EVENT& aEvent ) else { SCH_COMMIT commit( m_toolMgr ); - m_frame->AddItemToCommitAndScreen( &commit, m_frame->GetScreen(), image ); + commit.Add( image, m_frame->GetScreen() ); commit.Push( _( "Add Image" ) ); image = nullptr; @@ -672,7 +678,7 @@ int SCH_DRAWING_TOOLS::PlaceImage( const TOOL_EVENT& aEvent ) image->SetPosition( cursorPos ); m_view->ClearPreview(); m_view->AddToPreview( image, false ); // Add, but not give ownership - m_view->RecacheAllItems(); // Bitmaps are cached in Opengl + m_view->RecacheAllItems(); // Bitmaps are cached in Opengl m_frame->SetMsgPanel( image ); } else if( image && evt->IsAction( &ACTIONS::doDelete ) ) @@ -706,6 +712,7 @@ int SCH_DRAWING_TOOLS::SingleClickPlace( const TOOL_EVENT& aEvent ) SCH_ITEM* previewItem; bool loggedInfoBarError = false; wxString description; + SCH_SCREEN* screen = m_frame->GetScreen(); if( m_inSingleClickPlace ) return 0; @@ -730,19 +737,19 @@ int SCH_DRAWING_TOOLS::SingleClickPlace( const TOOL_EVENT& aEvent ) { case SCH_NO_CONNECT_T: previewItem = new SCH_NO_CONNECT( cursorPos ); - previewItem->SetParent( m_frame->GetScreen() ); + previewItem->SetParent( screen ); description = _( "Add No Connect Flag" ); break; case SCH_JUNCTION_T: previewItem = new SCH_JUNCTION( cursorPos ); - previewItem->SetParent( m_frame->GetScreen() ); + previewItem->SetParent( screen ); description = _( "Add Junction" ); break; case SCH_BUS_WIRE_ENTRY_T: previewItem = new SCH_BUS_WIRE_ENTRY( cursorPos ); - previewItem->SetParent( m_frame->GetScreen() ); + previewItem->SetParent( screen ); description = _( "Add Wire to Bus Entry" ); break; @@ -811,11 +818,11 @@ int SCH_DRAWING_TOOLS::SingleClickPlace( const TOOL_EVENT& aEvent ) } else if( evt->IsClick( BUT_LEFT ) || evt->IsDblClick( BUT_LEFT ) ) { - if( !m_frame->GetScreen()->GetItem( cursorPos, 0, type ) ) + if( !screen->GetItem( cursorPos, 0, type ) ) { if( type == SCH_JUNCTION_T ) { - if( !m_frame->GetScreen()->IsExplicitJunctionAllowed( cursorPos ) ) + if( !screen->IsExplicitJunctionAllowed( cursorPos ) ) { m_frame->ShowInfoBarError( _( "Junction location contains no joinable " "wires and/or pins." ) ); @@ -834,7 +841,7 @@ int SCH_DRAWING_TOOLS::SingleClickPlace( const TOOL_EVENT& aEvent ) newItem->SetFlags( IS_NEW ); SCH_COMMIT commit( m_toolMgr ); - m_frame->AddItemToCommitAndScreen( &commit, m_frame->GetScreen(), newItem ); + commit.Add( newItem, screen ); if( type == SCH_JUNCTION_T ) m_frame->TestDanglingEnds(); @@ -1439,8 +1446,11 @@ int SCH_DRAWING_TOOLS::TwoClickPlace( const TOOL_EVENT& aEvent ) { item->ClearFlags( IS_MOVING ); + if( item->IsConnectable() ) + m_frame->AutoRotateItem( m_frame->GetScreen(), item ); + SCH_COMMIT commit( m_toolMgr ); - m_frame->AddItemToCommitAndScreen( &commit, m_frame->GetScreen(), item ); + commit.Add( item, m_frame->GetScreen() ); commit.Push( description ); item = nullptr; @@ -1881,7 +1891,7 @@ int SCH_DRAWING_TOOLS::DrawSheet( const TOOL_EVENT& aEvent ) sheet->AutoplaceFields( /* aScreen */ nullptr, /* aManual */ false ); SCH_COMMIT commit( m_toolMgr ); - m_frame->AddItemToCommitAndScreen( &commit, m_frame->GetScreen(), sheet ); + commit.Add( sheet, m_frame->GetScreen() ); commit.Push( "Draw Sheet" ); m_frame->UpdateHierarchyNavigator(); diff --git a/eeschema/tools/sch_editor_control.cpp b/eeschema/tools/sch_editor_control.cpp index 96ae5b55b7..a385d0c428 100644 --- a/eeschema/tools/sch_editor_control.cpp +++ b/eeschema/tools/sch_editor_control.cpp @@ -1752,7 +1752,11 @@ int SCH_EDITOR_CONTROL::Paste( const TOOL_EVENT& aEvent ) item->SetFlags( STARTPOINT | ENDPOINT ); item->SetFlags( IS_NEW | IS_PASTED | IS_MOVING ); - m_frame->AddItemToCommitAndScreen( &commit, m_frame->GetScreen(), (SCH_ITEM*) item ); + + if( !m_frame->GetScreen()->CheckIfOnDrawList( (SCH_ITEM*) item ) ) // don't want a loop! + m_frame->AddToScreen( item, m_frame->GetScreen() ); + + commit.Added( (SCH_ITEM*) item, m_frame->GetScreen() ); // Reset flags for subsequent move operation item->SetFlags( IS_NEW | IS_PASTED | IS_MOVING );