Fixed a memory leak in VIEW_ITEM (proper way of doing 9bc2bb2)

The problem with simple deleting VIEW_ITEM_DATA upon VIEW_ITEM removal was
caused by the default copy constructors that copied pointers.
Once a copy of an item was destroyed, the VIEW_ITEM_DATA has been
destroyed, effectively invalidating m_viewPrivData for the other item.

common/view/view.cpp

@@ -55,7 +55,7 @@ public:
 
     ~VIEW_ITEM_DATA()
     {
-        delete[] m_groups;
+        deleteGroups();
     }
 
     int getFlags() const
@@ -174,7 +174,7 @@ private:
     void deleteGroups()
     {
         delete[] m_groups;
-        m_groups = NULL;
+        m_groups = nullptr;
         m_groupsSize = 0;
     }
 
@@ -246,7 +246,7 @@ void VIEW::OnDestroy( VIEW_ITEM* aItem )
 {
     auto data = aItem->viewPrivData();
 
-    if(!data)
+    if( !data )
         return;
 
     data->m_view->Remove( aItem );
@@ -311,7 +311,7 @@ void VIEW::Add( VIEW_ITEM* aItem )
     aItem->ViewGetLayers( layers, layers_count );
     aItem->viewPrivData()->saveLayers( layers, layers_count );
 
-    m_allItems.push_back(aItem);
+    m_allItems.push_back( aItem );
 
     for( int i = 0; i < layers_count; ++i )
     {
@@ -360,7 +360,6 @@ void VIEW::Remove( VIEW_ITEM* aItem )
     }
 
     viewData->deleteGroups();
-    aItem->m_viewPrivData = nullptr;
 }
 
 
@@ -906,22 +905,13 @@ void VIEW::draw( VIEW_ITEM* aItem, bool aImmediate )
     }
 }
 
+
 void VIEW::draw( VIEW_GROUP* aGroup, bool aImmediate )
 {
     for( unsigned int i = 0; i < aGroup->GetSize(); i++)
         draw( aGroup->GetItem(i), aImmediate );
 }
 
-struct VIEW::unlinkItem
-{
-    bool operator()( VIEW_ITEM* aItem )
-    {
-        delete aItem->m_viewPrivData;
-        aItem->m_viewPrivData = nullptr;
-        return true;
-    }
-};
-
 
 struct VIEW::recacheItem
 {
@@ -960,19 +950,10 @@ void VIEW::Clear()
     BOX2I r;
 
     r.SetMaximum();
-
     m_allItems.clear();
 
     for( LAYER_MAP_ITER i = m_layers.begin(); i != m_layers.end(); ++i )
-    {
+        i->second.items->RemoveAll();
-        VIEW_LAYER* l = &( ( *i ).second );
-        unlinkItem v;
-
-        if( m_dynamic )
-            l->items->Query( r, v );
-
-        l->items->RemoveAll();
-    }
 
     m_gal->ClearCache();
 }
@@ -1265,9 +1246,10 @@ void VIEW::UpdateItems()
             continue;
 
         if( viewData->m_requiredUpdate != NONE )
+        {
             invalidateItem( item, viewData->m_requiredUpdate );
-
+            viewData->m_requiredUpdate = NONE;
-        viewData->m_requiredUpdate = NONE;
+        }
     }
 
     m_gal->EndUpdate();

common/view/view_group.cpp

@@ -50,8 +50,7 @@ VIEW_GROUP::VIEW_GROUP( VIEW* aView ) :
 
 VIEW_GROUP::~VIEW_GROUP()
 {
-    if( m_view && viewPrivData() )
+    // VIEW_ITEM destructor removes the object from its parent view
-        m_view->Remove( this );
 }
 
 

include/view/view_item.h

@@ -84,11 +84,13 @@ class VIEW_ITEM
 public:
     VIEW_ITEM() : m_viewPrivData( nullptr )
     {
-
     }
 
     virtual ~VIEW_ITEM();
 
+    VIEW_ITEM( const VIEW_ITEM& aOther ) = delete;
+    VIEW_ITEM& operator=( const VIEW_ITEM& aOther ) = delete;
+
     /**
      * Function ViewBBox()
      * returns the bounding box of the item covering all its layers.

pcbnew/tools/edit_tool.cpp

@@ -449,7 +449,7 @@ int EDIT_TOOL::Remove( const TOOL_EVENT& aEvent )
         return 0;
 
     // Get a copy instead of a reference, as we are going to clear current selection
-    SELECTION selection = m_selectionTool->GetSelection();
+    auto selection = m_selectionTool->GetSelection().GetItems();
 
     // As we are about to remove items, they have to be removed from the selection first
     m_toolMgr->RunAction( COMMON_ACTIONS::selectionClear, true );