Tighten up the lifecycle management of VIEW_ITEMs in

general, and the SYMBOL_VIEWER_FRAME's m_previewItem in
particular.

(Attempted fix for Sentry KICAD-G1.)

common/view/view.cpp

@@ -240,16 +240,14 @@ private:
 
 void VIEW::OnDestroy( VIEW_ITEM* aItem )
 {
-    VIEW_ITEM_DATA* data = aItem->viewPrivData();
-
-    if( !data )
-        return;
-
-    if( data->m_view )
-        data->m_view->VIEW::Remove( aItem );
+    if( aItem->m_viewPrivData )
+    {
+        if( aItem->m_viewPrivData->m_view )
+            aItem->m_viewPrivData->m_view->VIEW::Remove( aItem );
 
-    delete data;
-    aItem->ClearViewPrivData();
+        delete aItem->m_viewPrivData;
+        aItem->m_viewPrivData = nullptr;
+    }
 }
 
 
@@ -323,6 +321,8 @@ void VIEW::Add( VIEW_ITEM* aItem, int aDrawPriority )
     if( !aItem->m_viewPrivData )
         aItem->m_viewPrivData = new VIEW_ITEM_DATA;
 
+    wxASSERT_MSG( aItem->m_viewPrivData->m_view == nullptr, wxS( "Already in a different view!" ) );
+
     aItem->m_viewPrivData->m_view = this;
     aItem->m_viewPrivData->m_drawPriority = aDrawPriority;
 
@@ -334,7 +334,7 @@ void VIEW::Add( VIEW_ITEM* aItem, int aDrawPriority )
     for( int i = 0; i < layers_count; ++i )
     {
         wxCHECK2_MSG( layers[i] >= 0 && static_cast<unsigned>( layers[i] ) < m_layers.size(),
-                continue, wxS( "Invalid layer" ) );
+                      continue, wxS( "Invalid layer" ) );
 
         VIEW_LAYER& l = m_layers[layers[i]];
         l.items->Insert( aItem );
@@ -348,41 +348,36 @@ void VIEW::Add( VIEW_ITEM* aItem, int aDrawPriority )
 
 void VIEW::Remove( VIEW_ITEM* aItem )
 {
-    if( !aItem )
-        return;
-
-    auto viewData = aItem->viewPrivData();
-
-    if( !viewData )
-        return;
+    if( aItem && aItem->m_viewPrivData )
+    {
+        wxCHECK( aItem->m_viewPrivData->m_view == this, /*void*/ );
+        auto item = std::find( m_allItems->begin(), m_allItems->end(), aItem );
 
-    wxCHECK( viewData->m_view == this, /*void*/ );
-    auto item = std::find( m_allItems->begin(), m_allItems->end(), aItem );
+        if( item != m_allItems->end() )
+        {
+            m_allItems->erase( item );
+            aItem->m_viewPrivData->clearUpdateFlags();
+        }
 
-    if( item != m_allItems->end() )
-    {
-        m_allItems->erase( item );
-        viewData->clearUpdateFlags();
-    }
+        int layers[VIEW::VIEW_MAX_LAYERS], layers_count;
+        aItem->m_viewPrivData->getLayers( layers, layers_count );
 
-    int layers[VIEW::VIEW_MAX_LAYERS], layers_count;
-    viewData->getLayers( layers, layers_count );
+        for( int i = 0; i < layers_count; ++i )
+        {
+            VIEW_LAYER& l = m_layers[layers[i]];
+            l.items->Remove( aItem );
+            MarkTargetDirty( l.target );
 
-    for( int i = 0; i < layers_count; ++i )
-    {
-        VIEW_LAYER& l = m_layers[layers[i]];
-        l.items->Remove( aItem );
-        MarkTargetDirty( l.target );
+            // Clear the GAL cache
+            int prevGroup = aItem->m_viewPrivData->getGroup( layers[i] );
 
-        // Clear the GAL cache
-        int prevGroup = viewData->getGroup( layers[i] );
+            if( prevGroup >= 0 )
+                m_gal->DeleteGroup( prevGroup );
+        }
 
-        if( prevGroup >= 0 )
-            m_gal->DeleteGroup( prevGroup );
+        aItem->m_viewPrivData->deleteGroups();
+        aItem->m_viewPrivData->m_view = nullptr;
     }
-
-    viewData->deleteGroups();
-    viewData->m_view = nullptr;
 }
 
 

eeschema/symbol_viewer_frame.cpp

@@ -114,8 +114,7 @@ SYMBOL_VIEWER_FRAME::SYMBOL_VIEWER_FRAME( KIWAY* aKiway, wxWindow* aParent, FRAM
                     aFrameType == FRAME_SCH_VIEWER_MODAL ? LIB_VIEW_NAME_MODAL : LIB_VIEW_NAME ),
         m_unitChoice( nullptr ),
         m_libList( nullptr ),
-        m_symbolList( nullptr ),
-        m_previewItem( nullptr )
+        m_symbolList( nullptr )
 {
     wxASSERT( aFrameType == FRAME_SCH_VIEWER || aFrameType == FRAME_SCH_VIEWER_MODAL );
 
@@ -318,7 +317,10 @@ SYMBOL_VIEWER_FRAME::~SYMBOL_VIEWER_FRAME()
         m_toolManager->ShutdownAllTools();
 
     if( m_previewItem )
-        GetCanvas()->GetView()->Remove( m_previewItem );
+    {
+        GetCanvas()->GetView()->Remove( m_previewItem.get() );
+        m_previewItem = nullptr;
+    }
 }
 
 
@@ -497,7 +499,7 @@ void SYMBOL_VIEWER_FRAME::updatePreviewSymbol()
 
     if( m_previewItem )
     {
-        view->Remove( m_previewItem );
+        view->Remove( m_previewItem.get() );
         m_previewItem = nullptr;
     }
 
@@ -508,11 +510,11 @@ void SYMBOL_VIEWER_FRAME::updatePreviewSymbol()
         GetRenderSettings()->m_ShowUnit = m_unit;
         GetRenderSettings()->m_ShowConvert = m_convert;
 
-        m_previewItem = symbol;
-        view->Add( m_previewItem );
+        m_previewItem = symbol->Flatten();
+        view->Add( m_previewItem.get() );
 
         wxString parentName;
-        std::shared_ptr<LIB_SYMBOL> parent  = m_previewItem->GetParent().lock();
+        std::shared_ptr<LIB_SYMBOL> parent  = symbol->GetParent().lock();
 
         if( parent )
             parentName = parent->GetName();

eeschema/symbol_viewer_frame.h

@@ -200,9 +200,9 @@ private:
      * Updated to `true` if a list rewrite on GUI activation resulted in the symbol
      * selection changing, or if the user has changed the selection manually.
      */
-    bool m_selection_changed;
+    bool                m_selection_changed;
 
-    LIB_SYMBOL* m_previewItem;
+    std::unique_ptr<LIB_SYMBOL> m_previewItem;
 
     DECLARE_EVENT_TABLE()
 };

include/view/view_item.h

@@ -142,11 +142,6 @@ public:
         return m_viewPrivData;
     }
 
-    void ClearViewPrivData()
-    {
-        m_viewPrivData = nullptr;
-    }
-
     void SetForcedTransparency( double aForcedTransparency )
     {
         m_forcedTransparency = aForcedTransparency;