From c0832a034262a1667b4c116bfb2d3f39b61c4c29 Mon Sep 17 00:00:00 2001 From: Dick Hollenbeck Date: Mon, 5 Aug 2013 16:02:41 -0500 Subject: [PATCH] BUG FIX: eeschema as segfaulting on the 'Insert' key because the m_itemToRepeat was simply a pointer to an object on the display list. At times this object would disappear from the display list, in the test case because of a concatonation of two wires, and if you then tried to clone the non-existent object you'd get a crash. This was not merely a bug, but a naive design choice. IMO. Now the item to repeat is cloned, so will never also be on the display list. --- common/drawpanel.cpp | 4 +- eeschema/bus-wire-junction.cpp | 76 ++++++++++++++++++++-------------- eeschema/edit_bitmap.cpp | 2 +- eeschema/edit_label.cpp | 4 +- eeschema/getpart.cpp | 2 +- eeschema/hierarch.cpp | 2 +- eeschema/hotkeys.cpp | 2 +- eeschema/onleftclick.cpp | 15 +++---- eeschema/schedit.cpp | 16 +++---- eeschema/schframe.cpp | 30 ++++++++++++-- eeschema/sheet.cpp | 11 ++--- include/class_drawpanel.h | 2 +- include/wxEeschemaStruct.h | 26 ++++++++---- 13 files changed, 121 insertions(+), 71 deletions(-) diff --git a/common/drawpanel.cpp b/common/drawpanel.cpp index 521c73e771..4731dbde49 100644 --- a/common/drawpanel.cpp +++ b/common/drawpanel.cpp @@ -145,9 +145,11 @@ EDA_DRAW_PANEL::~EDA_DRAW_PANEL() wxGetApp().GetSettings()->Write( ENBL_AUTO_PAN_KEY, m_enableAutoPan ); } + EDA_DRAW_FRAME* EDA_DRAW_PANEL::GetParent() { - return ( EDA_DRAW_FRAME* ) wxWindow::GetParent(); + wxWindow* mom = wxScrolledWindow::GetParent(); + return (EDA_DRAW_FRAME*) mom; } diff --git a/eeschema/bus-wire-junction.cpp b/eeschema/bus-wire-junction.cpp index 72890bee3e..95af16a3d8 100644 --- a/eeschema/bus-wire-junction.cpp +++ b/eeschema/bus-wire-junction.cpp @@ -162,7 +162,7 @@ void SCH_EDIT_FRAME::BeginSegment( wxDC* DC, int type ) } m_canvas->SetMouseCapture( DrawSegment, AbortCreateNewLine ); - m_itemToRepeat = NULL; + SetRepeatItem( NULL ); } else // A segment is in progress: terminates the current segment and add a new segment. { @@ -239,7 +239,8 @@ void SCH_EDIT_FRAME::EndSegment( wxDC* DC ) return; // Get the last non-null wire (this is the last created segment). - m_itemToRepeat = segment = (SCH_LINE*) s_wires.GetLast(); + SetRepeatItem( segment = (SCH_LINE*) s_wires.GetLast() ); + screen->SetCurItem( NULL ); m_canvas->EndMouseCapture( -1, -1, wxEmptyString, false ); @@ -341,7 +342,7 @@ void SCH_EDIT_FRAME::DeleteCurrentSegment( wxDC* DC ) { SCH_SCREEN* screen = GetScreen(); - m_itemToRepeat = NULL; + SetRepeatItem( NULL ); if( ( screen->GetCurItem() == NULL ) || !screen->GetCurItem()->IsNew() ) return; @@ -359,7 +360,7 @@ SCH_JUNCTION* SCH_EDIT_FRAME::AddJunction( wxDC* aDC, const wxPoint& aPosition, { SCH_JUNCTION* junction = new SCH_JUNCTION( aPosition ); - m_itemToRepeat = junction; + SetRepeatItem( junction ); m_canvas->CrossHairOff( aDC ); // Erase schematic cursor junction->Draw( m_canvas, aDC, wxPoint( 0, 0 ), GR_DEFAULT_DRAWMODE ); @@ -378,19 +379,19 @@ SCH_JUNCTION* SCH_EDIT_FRAME::AddJunction( wxDC* aDC, const wxPoint& aPosition, SCH_NO_CONNECT* SCH_EDIT_FRAME::AddNoConnect( wxDC* aDC, const wxPoint& aPosition ) { - SCH_NO_CONNECT* NewNoConnect; + SCH_NO_CONNECT* no_connect = new SCH_NO_CONNECT( aPosition ); - NewNoConnect = new SCH_NO_CONNECT( aPosition ); - m_itemToRepeat = NewNoConnect; + SetRepeatItem( no_connect ); m_canvas->CrossHairOff( aDC ); // Erase schematic cursor - NewNoConnect->Draw( m_canvas, aDC, wxPoint( 0, 0 ), GR_DEFAULT_DRAWMODE ); + no_connect->Draw( m_canvas, aDC, wxPoint( 0, 0 ), GR_DEFAULT_DRAWMODE ); + m_canvas->CrossHairOn( aDC ); // Display schematic cursor - GetScreen()->Append( NewNoConnect ); + GetScreen()->Append( no_connect ); OnModify(); - SaveCopyInUndoList( NewNoConnect, UR_NEW ); - return NewNoConnect; + SaveCopyInUndoList( no_connect, UR_NEW ); + return no_connect; } @@ -420,35 +421,46 @@ static void AbortCreateNewLine( EDA_DRAW_PANEL* Panel, wxDC* DC ) void SCH_EDIT_FRAME::RepeatDrawItem( wxDC* DC ) { - if( m_itemToRepeat == NULL ) + SCH_ITEM* repeater = GetRepeatItem(); + + if( !repeater ) return; - m_itemToRepeat = (SCH_ITEM*) m_itemToRepeat->Clone(); + //D( repeater>Show( 0, std::cout ); ) - if( m_itemToRepeat->Type() == SCH_COMPONENT_T ) // If repeat component then put in move mode + // clone the repeater, move it, insert into display list, then save a copy + // via SetRepeatItem(); + + SCH_ITEM* my_clone = (SCH_ITEM*) repeater->Clone(); + + // If cloning a component then put into 'move' mode. + /* please tell me where components are snapshotted via SetRepeatItem() + if( my_clone->Type() == SCH_COMPONENT_T ) { wxPoint pos = GetCrossHairPosition() - - ( (SCH_COMPONENT*) m_itemToRepeat )->GetPosition(); + ( (SCH_COMPONENT*) my_clone )->GetPosition(); - m_itemToRepeat->SetFlags( IS_NEW ); - ( (SCH_COMPONENT*) m_itemToRepeat )->SetTimeStamp( GetNewTimeStamp() ); - m_itemToRepeat->Move( pos ); - m_itemToRepeat->Draw( m_canvas, DC, wxPoint( 0, 0 ), g_XorMode ); - MoveItem( m_itemToRepeat, DC ); - return; + my_clone->SetFlags( IS_NEW ); + ( (SCH_COMPONENT*) my_clone )->SetTimeStamp( GetNewTimeStamp() ); + my_clone->Move( pos ); + my_clone->Draw( m_canvas, DC, wxPoint( 0, 0 ), g_XorMode ); + MoveItem( my_clone, DC ); } - - m_itemToRepeat->Move( wxPoint( g_RepeatStep.GetWidth(), g_RepeatStep.GetHeight() ) ); - - if( m_itemToRepeat->CanIncrementLabel() ) - ( (SCH_TEXT*) m_itemToRepeat )->IncrementLabel(); - - if( m_itemToRepeat ) + else + */ { - GetScreen()->Append( m_itemToRepeat ); + my_clone->Move( wxPoint( g_RepeatStep.GetWidth(), g_RepeatStep.GetHeight() ) ); + + if( my_clone->CanIncrementLabel() ) + ( (SCH_TEXT*) my_clone )->IncrementLabel(); + + GetScreen()->Append( my_clone ); GetScreen()->TestDanglingEnds(); - m_itemToRepeat->Draw( m_canvas, DC, wxPoint( 0, 0 ), GR_DEFAULT_DRAWMODE ); - SaveCopyInUndoList( m_itemToRepeat, UR_NEW ); - m_itemToRepeat->ClearFlags(); + my_clone->Draw( m_canvas, DC, wxPoint( 0, 0 ), GR_DEFAULT_DRAWMODE ); + SaveCopyInUndoList( my_clone, UR_NEW ); + my_clone->ClearFlags(); } + + // clone my_clone, now that it has been moved, thus saving new position. + SetRepeatItem( my_clone ); } diff --git a/eeschema/edit_bitmap.cpp b/eeschema/edit_bitmap.cpp index e3fdd42c99..2a83d10a11 100644 --- a/eeschema/edit_bitmap.cpp +++ b/eeschema/edit_bitmap.cpp @@ -144,7 +144,7 @@ void SCH_EDIT_FRAME::MoveImage( SCH_BITMAP* aImageItem, wxDC* aDC ) m_canvas->SetMouseCapture( moveBitmap, abortMoveBitmap ); GetScreen()->SetCurItem( aImageItem ); - m_itemToRepeat = NULL; + SetRepeatItem( NULL ); SetUndoItem( aImageItem ); diff --git a/eeschema/edit_label.cpp b/eeschema/edit_label.cpp index 855a313bca..07f466af74 100644 --- a/eeschema/edit_label.cpp +++ b/eeschema/edit_label.cpp @@ -72,7 +72,7 @@ SCH_TEXT* SCH_EDIT_FRAME::CreateNewText( wxDC* aDC, int aType ) { SCH_TEXT* textItem = NULL; - m_itemToRepeat = NULL; + SetRepeatItem( NULL ); switch( aType ) { @@ -235,7 +235,7 @@ void SCH_EDIT_FRAME::OnConvertTextType( wxCommandEvent& aEvent ) } } - m_itemToRepeat = NULL; + SetRepeatItem( NULL ); OnModify(); newtext->Draw( m_canvas, &dc, wxPoint( 0, 0 ), GR_DEFAULT_DRAWMODE ); m_canvas->CrossHairOn( &dc ); // redraw schematic cursor diff --git a/eeschema/getpart.cpp b/eeschema/getpart.cpp index 6fbf348769..f456831881 100644 --- a/eeschema/getpart.cpp +++ b/eeschema/getpart.cpp @@ -199,7 +199,7 @@ SCH_COMPONENT* SCH_EDIT_FRAME::Load_Component( wxDC* aDC, { int unit = 1; int convert = 1; - m_itemToRepeat = NULL; + SetRepeatItem( NULL ); m_canvas->SetIgnoreMouseEvents( true ); wxString Name = SelectComponentFromLibrary( aLibname, aHistoryList, aUseLibBrowser, diff --git a/eeschema/hierarch.cpp b/eeschema/hierarch.cpp index 7ac4adbca1..b74ff81b56 100644 --- a/eeschema/hierarch.cpp +++ b/eeschema/hierarch.cpp @@ -268,7 +268,7 @@ void HIERARCHY_NAVIG_DLG::OnSelect( wxTreeEvent& event ) void SCH_EDIT_FRAME::DisplayCurrentSheet() { - m_itemToRepeat = NULL; + SetRepeatItem( NULL ); ClearMsgPanel(); SCH_SCREEN* screen = m_CurrentSheet->LastScreen(); diff --git a/eeschema/hotkeys.cpp b/eeschema/hotkeys.cpp index 4afdebe397..474e664235 100644 --- a/eeschema/hotkeys.cpp +++ b/eeschema/hotkeys.cpp @@ -385,7 +385,7 @@ void SCH_EDIT_FRAME::OnHotKey( wxDC* aDC, int aHotKey, const wxPoint& aPosition, break; case HK_REPEAT_LAST: - if( notBusy && m_itemToRepeat && ( m_itemToRepeat->GetFlags() == 0 ) ) + if( notBusy && GetRepeatItem() && ( GetRepeatItem()->GetFlags() == 0 ) ) RepeatDrawItem( aDC ); break; diff --git a/eeschema/onleftclick.cpp b/eeschema/onleftclick.cpp index 067db61625..7fa3a3f509 100644 --- a/eeschema/onleftclick.cpp +++ b/eeschema/onleftclick.cpp @@ -58,7 +58,7 @@ void SCH_EDIT_FRAME::OnLeftClick( wxDC* aDC, const wxPoint& aPosition ) if( ( GetToolId() == ID_NO_TOOL_SELECTED ) || ( item && item->GetFlags() ) ) { m_canvas->SetAutoPanRequest( false ); - m_itemToRepeat = NULL; + SetRepeatItem( NULL ); if( item && item->GetFlags() ) { @@ -128,8 +128,9 @@ void SCH_EDIT_FRAME::OnLeftClick( wxDC* aDC, const wxPoint& aPosition ) { if( false == GetScreen()->GetItem( gridPosition, 0, SCH_NO_CONNECT_T ) ) { - m_itemToRepeat = AddNoConnect( aDC, gridPosition ); - GetScreen()->SetCurItem( m_itemToRepeat ); + SCH_NO_CONNECT* no_connect = AddNoConnect( aDC, gridPosition ); + SetRepeatItem( no_connect ); + GetScreen()->SetCurItem( no_connect ); m_canvas->SetAutoPanRequest( true ); } } @@ -137,7 +138,6 @@ void SCH_EDIT_FRAME::OnLeftClick( wxDC* aDC, const wxPoint& aPosition ) { addCurrentItemToList( aDC ); } - break; case ID_JUNCTION_BUTT: @@ -145,8 +145,9 @@ void SCH_EDIT_FRAME::OnLeftClick( wxDC* aDC, const wxPoint& aPosition ) { if( false == GetScreen()->GetItem( gridPosition, 0, SCH_JUNCTION_T ) ) { - m_itemToRepeat = AddJunction( aDC, gridPosition, true ); - GetScreen()->SetCurItem( m_itemToRepeat ); + SCH_JUNCTION* junction = AddJunction( aDC, gridPosition, true ); + SetRepeatItem( junction ); + GetScreen()->SetCurItem( junction ); m_canvas->SetAutoPanRequest( true ); } } @@ -154,7 +155,6 @@ void SCH_EDIT_FRAME::OnLeftClick( wxDC* aDC, const wxPoint& aPosition ) { addCurrentItemToList( aDC ); } - break; case ID_WIRETOBUS_ENTRY_BUTT: @@ -168,6 +168,7 @@ void SCH_EDIT_FRAME::OnLeftClick( wxDC* aDC, const wxPoint& aPosition ) addCurrentItemToList( aDC ); } break; + case ID_BUSTOBUS_ENTRY_BUTT: if( ( item == NULL ) || ( item->GetFlags() == 0 ) ) { diff --git a/eeschema/schedit.cpp b/eeschema/schedit.cpp index 7eeaac888d..3f6f9efce6 100644 --- a/eeschema/schedit.cpp +++ b/eeschema/schedit.cpp @@ -119,7 +119,7 @@ void SCH_EDIT_FRAME::Process_Special_Functions( wxCommandEvent& event ) { case ID_HIERARCHY: InstallHierarchyFrame( &dc, pos ); - m_itemToRepeat = NULL; + SetRepeatItem( NULL ); break; case wxID_CUT: @@ -127,7 +127,7 @@ void SCH_EDIT_FRAME::Process_Special_Functions( wxCommandEvent& event ) break; HandleBlockEndByPopUp( BLOCK_DELETE, &dc ); - m_itemToRepeat = NULL; + SetRepeatItem( NULL ); SetSheetNumberAndCount(); break; @@ -182,7 +182,7 @@ void SCH_EDIT_FRAME::Process_Special_Functions( wxCommandEvent& event ) m_canvas->MoveCursorToCrossHair(); DeleteConnection( id == ID_POPUP_SCH_DELETE_CONNECTION ); screen->SetCurItem( NULL ); - m_itemToRepeat = NULL; + SetRepeatItem( NULL ); screen->TestDanglingEnds( m_canvas, &dc ); m_canvas->Refresh(); break; @@ -222,7 +222,7 @@ void SCH_EDIT_FRAME::Process_Special_Functions( wxCommandEvent& event ) DeleteItem( item ); screen->SetCurItem( NULL ); - m_itemToRepeat = NULL; + SetRepeatItem( NULL ); screen->TestDanglingEnds( m_canvas, &dc ); SetSheetNumberAndCount(); OnModify(); @@ -375,7 +375,7 @@ void SCH_EDIT_FRAME::Process_Special_Functions( wxCommandEvent& event ) // End switch ( id ) (Command execution) if( GetToolId() == ID_NO_TOOL_SELECTED ) - m_itemToRepeat = NULL; + SetRepeatItem( NULL ); } @@ -445,7 +445,7 @@ void SCH_EDIT_FRAME::OnMoveItem( wxCommandEvent& aEvent ) } if( GetToolId() == ID_NO_TOOL_SELECTED ) - m_itemToRepeat = NULL; + SetRepeatItem( NULL ); } @@ -561,7 +561,7 @@ void SCH_EDIT_FRAME::OnSelectTool( wxCommandEvent& aEvent ) break; default: - m_itemToRepeat = NULL; + SetRepeatItem( NULL ); } // Simulate left click event if we got here from a hot key. @@ -695,7 +695,7 @@ void SCH_EDIT_FRAME::MoveItem( SCH_ITEM* aItem, wxDC* aDC ) { wxCHECK_RET( aItem != NULL, wxT( "Cannot move invalid schematic item" ) ); - m_itemToRepeat = NULL; + SetRepeatItem( NULL ); if( !aItem->IsNew() ) { diff --git a/eeschema/schframe.cpp b/eeschema/schframe.cpp index d6432c0fdb..dadb7a8663 100644 --- a/eeschema/schframe.cpp +++ b/eeschema/schframe.cpp @@ -177,7 +177,8 @@ SCH_EDIT_FRAME::SCH_EDIT_FRAME( wxWindow* aParent, const wxString& aTitle, const wxPoint& aPosition, const wxSize& aSize, long aStyle ) : SCH_BASE_FRAME( aParent, SCHEMATIC_FRAME_TYPE, aTitle, aPosition, aSize, - aStyle, SCH_EDIT_FRAME_NAME ) + aStyle, SCH_EDIT_FRAME_NAME ), + m_item_to_repeat( 0 ) { m_FrameName = SCH_EDIT_FRAME_NAME; m_showAxis = false; // true to show axis @@ -207,8 +208,6 @@ SCH_EDIT_FRAME::SCH_EDIT_FRAME( wxWindow* aParent, const wxString& aTitle, icon.CopyFromBitmap( KiBitmap( icon_eeschema_xpm ) ); SetIcon( icon ); - m_itemToRepeat = NULL; - /* Get config */ LoadSettings(); @@ -268,8 +267,10 @@ SCH_EDIT_FRAME::SCH_EDIT_FRAME( wxWindow* aParent, const wxString& aTitle, SCH_EDIT_FRAME::~SCH_EDIT_FRAME() { + delete m_item_to_repeat; // we own the cloned object, see this->SetRepeatItem() + SetScreen( NULL ); - delete m_CurrentSheet; // a SCH_SHEET_PATH, on the heap. + delete m_CurrentSheet; // a SCH_SHEET_PATH, on the heap. delete m_undoItem; delete g_RootSheet; delete m_findReplaceData; @@ -281,6 +282,27 @@ SCH_EDIT_FRAME::~SCH_EDIT_FRAME() } +void SCH_EDIT_FRAME::SetRepeatItem( SCH_ITEM* aItem ) +{ + // we cannot store a pointer to an item in the display list here since + // that item may be deleted, such as part of a line concatonation or other. + // So simply always keep a copy of the object which is to be repeated. + + SCH_ITEM* old = m_item_to_repeat; + SCH_ITEM* cur = aItem; + + if( cur != old ) + { + if( cur ) + aItem = (SCH_ITEM*) cur->Clone(); + + m_item_to_repeat = aItem; + + delete old; + } +} + + void SCH_EDIT_FRAME::SetSheetNumberAndCount() { SCH_SCREEN* screen = GetScreen(); diff --git a/eeschema/sheet.cpp b/eeschema/sheet.cpp index 3d59b19197..4b907f2693 100644 --- a/eeschema/sheet.cpp +++ b/eeschema/sheet.cpp @@ -45,7 +45,7 @@ bool SCH_EDIT_FRAME::EditSheet( SCH_SHEET* aSheet, wxDC* aDC ) if( aSheet == NULL ) return false; - /* Get the new texts */ + // Get the new texts DIALOG_SCH_SHEET_PROPS dlg( this ); wxString units = GetUnitsLabel( g_UserUnit ); @@ -277,11 +277,12 @@ static void MoveOrResizeSheet( EDA_DRAW_PANEL* aPanel, wxDC* aDC, const wxPoint& } -/* Complete sheet move. */ +// Complete sheet move. static void ExitSheet( EDA_DRAW_PANEL* aPanel, wxDC* aDC ) { SCH_SCREEN* screen = (SCH_SCREEN*) aPanel->GetScreen(); - SCH_ITEM* item = screen->GetCurItem(); + SCH_ITEM* item = screen->GetCurItem(); + SCH_EDIT_FRAME* parent = ( SCH_EDIT_FRAME* ) aPanel->GetParent(); if( (item == NULL) || (item->Type() != SCH_SHEET_T) || (parent == NULL) ) @@ -320,10 +321,10 @@ static void ExitSheet( EDA_DRAW_PANEL* aPanel, wxDC* aDC ) } -/* Create hierarchy sheet. */ +// Create hierarchy sheet. SCH_SHEET* SCH_EDIT_FRAME::CreateSheet( wxDC* aDC ) { - m_itemToRepeat = NULL; + SetRepeatItem( NULL ); SCH_SHEET* sheet = new SCH_SHEET( GetCrossHairPosition() ); diff --git a/include/class_drawpanel.h b/include/class_drawpanel.h index 46439257c9..270999ac7e 100644 --- a/include/class_drawpanel.h +++ b/include/class_drawpanel.h @@ -123,7 +123,7 @@ public: BASE_SCREEN* GetScreen(); - virtual EDA_DRAW_FRAME* GetParent(); + EDA_DRAW_FRAME* GetParent(); void OnPaint( wxPaintEvent& event ); diff --git a/include/wxEeschemaStruct.h b/include/wxEeschemaStruct.h index 8607c835f7..2f050ba60d 100644 --- a/include/wxEeschemaStruct.h +++ b/include/wxEeschemaStruct.h @@ -134,7 +134,7 @@ private: wxArrayString m_findStringHistoryList; wxArrayString m_replaceStringHistoryList; BLOCK_SELECTOR m_blockItems; ///< List of selected items. - SCH_ITEM* m_itemToRepeat; ///< Last item to insert by the repeat command. + SCH_ITEM* m_item_to_repeat; ///< Last item to insert by the repeat command. int m_repeatLabelDelta; ///< Repeat label number increment step. SCH_COLLECTOR m_collectedItems; ///< List of collected items. SCH_FIND_COLLECTOR m_foundItems; ///< List of find/replace items. @@ -899,11 +899,12 @@ private: void EditImage( SCH_BITMAP* aItem ); // Hierarchical Sheet & PinSheet - void InstallHierarchyFrame( wxDC* DC, wxPoint& pos ); - SCH_SHEET* CreateSheet( wxDC* DC ); - void ReSizeSheet( SCH_SHEET* Sheet, wxDC* DC ); - // Loads the cache library associated to the aFileName - bool LoadCacheLibrary( const wxString& aFileName ); + void InstallHierarchyFrame( wxDC* DC, wxPoint& pos ); + SCH_SHEET* CreateSheet( wxDC* DC ); + void ReSizeSheet( SCH_SHEET* Sheet, wxDC* DC ); + + /// Loads the cache library associated to the aFileName + bool LoadCacheLibrary( const wxString& aFileName ); public: /** @@ -1176,7 +1177,18 @@ public: */ void RepeatDrawItem( wxDC* DC ); - void SetRepeatItem( SCH_ITEM* aItem ) { m_itemToRepeat = aItem; } + /** + * Function SetRepeatItem + * clones aItem and owns that clone in this container. + */ + void SetRepeatItem( SCH_ITEM* aItem ); + + /** + * Function GetRepeatItem + * returns the item which is to be repeated with the insert key. Such object + * is owned by this container, and must be cloned. + */ + SCH_ITEM* GetRepeatItem() const { return m_item_to_repeat; } /** * Function SetUndoItem