kicad-developers team mailing list archive

[Simon Richter <Simon.Richter@xxxxxxxxxx>]

SCREEN objects are typically owned by the DRAW_FRAME they are attached to.

SCH_SCREEN objects may additionally be shared between one DRAW_FRAME, and
multiple SCH_SHEET objects, so they are reference counted.

To make this distinction more obvious, and avoid potential errors, move the
pointer into the derived DRAW_FRAME, with an appropriate smart pointer
type. This also avoids a few C-style casts and replaces them by compile
time checks.
---
 common/draw_frame.cpp               |  6 +----
 eeschema/class_sch_screen.h         | 32 ++++++++++++++++++++------
 eeschema/sch_base_frame.cpp         |  5 ----
 eeschema/sch_base_frame.h           |  6 ++++-
 eeschema/sch_screen.cpp             |  2 ++
 eeschema/sch_sheet.cpp              | 46 ++++++++-----------------------------
 eeschema/sch_sheet.h                | 11 ++++-----
 eeschema/sheet.cpp                  |  4 ++--
 gerbview/gerbview_frame.h           |  5 ++++
 include/draw_frame.h                |  6 +----
 include/wxBasePcbFrame.h            |  6 ++++-
 pagelayout_editor/pl_editor_frame.h |  8 +++----
 12 files changed, 64 insertions(+), 73 deletions(-)

diff --git a/common/draw_frame.cpp b/common/draw_frame.cpp
index b28e63d..fc6907e 100644
--- a/common/draw_frame.cpp
+++ b/common/draw_frame.cpp
@@ -141,7 +141,6 @@ EDA_DRAW_FRAME::EDA_DRAW_FRAME( KIWAY* aKiway, wxWindow* aParent,
     m_toolManager         = NULL;
     m_toolDispatcher      = NULL;
     m_messagePanel        = NULL;
-    m_currentScreen       = NULL;
     m_toolId              = ID_NO_TOOL_SELECTED;
     m_lastDrawToolId      = ID_NO_TOOL_SELECTED;
     m_showAxis            = false;      // true to draw axis.
@@ -214,9 +213,6 @@ EDA_DRAW_FRAME::~EDA_DRAW_FRAME()
     delete m_toolDispatcher;
     delete m_galCanvas;
 
-    delete m_currentScreen;
-    m_currentScreen = NULL;
-
     m_auimgr.UnInit();
 
     ReleaseFile();
@@ -553,7 +549,7 @@ wxPoint EDA_DRAW_FRAME::GetGridPosition( const wxPoint& aPosition ) const
 {
     wxPoint pos = aPosition;
 
-    if( m_currentScreen != NULL && m_snapToGrid )
+    if( GetScreen() != NULL && m_snapToGrid )
         pos = GetNearestGridPosition( aPosition );
 
     return pos;
diff --git a/eeschema/class_sch_screen.h b/eeschema/class_sch_screen.h
index 2fb4043..a6715f0 100644
--- a/eeschema/class_sch_screen.h
+++ b/eeschema/class_sch_screen.h
@@ -97,6 +97,15 @@ private:
      */
     void addConnectedItemsToBlock( const wxPoint& aPosition );
 
+    ~SCH_SCREEN();
+
+    void DecRefCount();
+
+    void IncRefCount();
+
+    friend void intrusive_ptr_add_ref( SCH_SCREEN* );
+    friend void intrusive_ptr_release( SCH_SCREEN* );
+
 public:
 
     /**
@@ -104,7 +113,10 @@ public:
      */
     SCH_SCREEN( KIWAY* aKiway );
 
-    ~SCH_SCREEN();
+    bool IsShared() const
+    {
+        return m_refCount > 1;
+    }
 
     virtual wxString GetClass() const
     {
@@ -125,12 +137,6 @@ public:
     //TITLE_BLOCK& GetTitleBlock() const                      { return (TITLE_BLOCK&) m_titles; }
     void SetTitleBlock( const TITLE_BLOCK& aTitleBlock )    { m_titles = aTitleBlock; }
 
-    void DecRefCount();
-
-    void IncRefCount();
-
-    int GetRefCount() const                                 { return m_refCount; }
-
     /**
      * Function GetDrawItems().
      * @return - A pointer to the first item in the linked list of draw items.
@@ -526,6 +532,18 @@ public:
 };
 
 
+inline void intrusive_ptr_add_ref( SCH_SCREEN *a_screen )
+{
+    a_screen->IncRefCount();
+}
+
+
+inline void intrusive_ptr_release( SCH_SCREEN *a_screen )
+{
+    a_screen->DecRefCount();
+}
+
+
 /**
  * Class SCH_SCREENS
  * is a container class that holds multiple SCH_SCREENs in a hierarchy.
diff --git a/eeschema/sch_base_frame.cpp b/eeschema/sch_base_frame.cpp
index 52898e8..fb5ed60 100644
--- a/eeschema/sch_base_frame.cpp
+++ b/eeschema/sch_base_frame.cpp
@@ -75,11 +75,6 @@ void SCH_BASE_FRAME::SetDrawBgColor( EDA_COLOR_T aColor)
 }
 
 
-SCH_SCREEN* SCH_BASE_FRAME::GetScreen() const
-{
-    return (SCH_SCREEN*) EDA_DRAW_FRAME::GetScreen();
-}
-
 const wxString SCH_BASE_FRAME::GetZoomLevelIndicator() const
 {
     return EDA_DRAW_FRAME::GetZoomLevelIndicator();
diff --git a/eeschema/sch_base_frame.h b/eeschema/sch_base_frame.h
index c99e1a6..4ec7509 100644
--- a/eeschema/sch_base_frame.h
+++ b/eeschema/sch_base_frame.h
@@ -27,6 +27,8 @@
 #include <draw_frame.h>
 #include <class_sch_screen.h>
 
+#include <boost/intrusive_ptr.hpp>
+
 class PAGE_INFO;
 class TITLE_BLOCK;
 class LIB_VIEW_FRAME;
@@ -53,6 +55,7 @@ protected:
     int      m_repeatDeltaLabel;    ///< the increment value of labels like bus members
                                     ///< when they are repeated
 
+    boost::intrusive_ptr<SCH_SCREEN>   m_screen;
 
 public:
     SCH_BASE_FRAME( KIWAY* aKiway, wxWindow* aParent,
@@ -61,7 +64,8 @@ public:
                     const wxPoint& aPosition, const wxSize& aSize,
                     long aStyle, const wxString & aFrameName );
 
-    SCH_SCREEN* GetScreen() const;                              // overload EDA_DRAW_FRAME
+    virtual SCH_SCREEN* GetScreen() const override { return m_screen.get(); }
+    void SetScreen( SCH_SCREEN* a_screen ) { m_screen.reset( a_screen ); }
 
     /**
      * @return the increment value of the position of an item
diff --git a/eeschema/sch_screen.cpp b/eeschema/sch_screen.cpp
index 20a18b9..43610ba 100644
--- a/eeschema/sch_screen.cpp
+++ b/eeschema/sch_screen.cpp
@@ -136,6 +136,8 @@ void SCH_SCREEN::DecRefCount()
     wxCHECK_RET( m_refCount != 0,
                  wxT( "Screen reference count already zero.  Bad programmer!" ) );
     m_refCount--;
+    if( m_refCount == 0 )
+        delete this;
 }
 
 
diff --git a/eeschema/sch_sheet.cpp b/eeschema/sch_sheet.cpp
index 33338b7..8f60531 100644
--- a/eeschema/sch_sheet.cpp
+++ b/eeschema/sch_sheet.cpp
@@ -74,25 +74,12 @@ SCH_SHEET::SCH_SHEET( const SCH_SHEET& aSheet ) :
 
     for( size_t i = 0;  i < m_pins.size();  i++ )
         m_pins[i].SetParent( this );
-
-    if( m_screen )
-        m_screen->IncRefCount();
 }
 
 
 SCH_SHEET::~SCH_SHEET()
 {
-//    wxLogDebug( wxT( "Destroying sheet " ) + m_name );
-
-    // also, look at the associated sheet & its reference count
-    // perhaps it should be deleted also.
-    if( m_screen )
-    {
-        m_screen->DecRefCount();
-
-        if( m_screen->GetRefCount() == 0 )
-            delete m_screen;
-    }
+    return;
 }
 
 
@@ -104,33 +91,16 @@ EDA_ITEM* SCH_SHEET::Clone() const
 
 void SCH_SHEET::SetScreen( SCH_SCREEN* aScreen )
 {
-    if( aScreen == m_screen )
-        return;
-
-    if( m_screen != NULL )
-    {
-        m_screen->DecRefCount();
-
-        if( m_screen->GetRefCount() == 0 )
-        {
-            delete m_screen;
-            m_screen = NULL;
-        }
-    }
-
     m_screen = aScreen;
-
-    if( m_screen )
-        m_screen->IncRefCount();
 }
 
 
-int SCH_SHEET::GetScreenCount() const
+bool SCH_SHEET::IsSharedScreen() const
 {
     if( m_screen == NULL )
         return 0;
 
-    return m_screen->GetRefCount();
+    return m_screen->IsShared();
 }
 
 
@@ -710,10 +680,12 @@ bool SCH_SHEET::SearchHierarchy( const wxString& aFilename, SCH_SCREEN** aScreen
             {
                 SCH_SHEET* sheet = (SCH_SHEET*) item;
 
-                if( sheet->m_screen
-                    && sheet->m_screen->GetFileName().CmpNoCase( aFilename ) == 0 )
+                SCH_SCREEN* candidate_screen = sheet->GetScreen();
+
+                if( candidate_screen
+                    && candidate_screen->GetFileName().CmpNoCase( aFilename ) == 0 )
                 {
-                    *aScreen = sheet->m_screen;
+                    *aScreen = candidate_screen;
                     return true;
                 }
 
@@ -779,7 +751,7 @@ bool SCH_SHEET::Load( SCH_EDIT_FRAME* aFrame )
         {
             SetScreen( new SCH_SCREEN( &aFrame->Kiway() ) );
 
-            success = aFrame->LoadOneEEFile( m_screen, m_fileName );
+            success = aFrame->LoadOneEEFile( m_screen.get(), m_fileName );
 
             if( success )
             {
diff --git a/eeschema/sch_sheet.h b/eeschema/sch_sheet.h
index e899f41..85aa284 100644
--- a/eeschema/sch_sheet.h
+++ b/eeschema/sch_sheet.h
@@ -31,6 +31,7 @@
 #define SCH_SHEEET_H
 
 #include <boost/ptr_container/ptr_vector.hpp>
+#include <boost/intrusive_ptr.hpp>
 #include <boost/foreach.hpp>
 #include <sch_text.h>
 
@@ -218,7 +219,7 @@ class SCH_SHEET : public SCH_ITEM
 
     /// Screen that contains the physical data for the sheet.  In complex hierarchies
     /// multiple sheets can share a common screen.
-    SCH_SCREEN* m_screen;
+    boost::intrusive_ptr<SCH_SCREEN> m_screen;
 
     /// The list of sheet connection points.
     SCH_SHEET_PINS m_pins;
@@ -282,7 +283,7 @@ public:
 
     void SetFileNameSize( int aSize ) { m_fileNameSize = aSize; }
 
-    SCH_SCREEN* GetScreen() { return m_screen; }
+    SCH_SCREEN* GetScreen() { return m_screen.get(); }
 
     wxSize GetSize() { return m_size; }
 
@@ -303,11 +304,9 @@ public:
     void SetScreen( SCH_SCREEN* aScreen );
 
     /**
-     * Function GetScreenCount
-     * returns the number of times the associated screen for the sheet is being used.  If
-     * no screen is associated with the sheet, then zero is returned.
+     * Check whether the screen is shared
      */
-    int GetScreenCount() const;
+    bool IsSharedScreen() const;
 
     bool Save( FILE* aFile ) const;
 
diff --git a/eeschema/sheet.cpp b/eeschema/sheet.cpp
index f57157e..f5b0b90 100644
--- a/eeschema/sheet.cpp
+++ b/eeschema/sheet.cpp
@@ -185,7 +185,7 @@ bool SCH_EDIT_FRAME::EditSheet( SCH_SHEET* aSheet, SCH_SHEET_PATH* aHierarchy )
             }
             else                                               // Save to new file name.
             {
-                if( aSheet->GetScreenCount() > 1 )
+                if( aSheet->IsSharedScreen() )
                 {
                     msg += _( "This sheet uses shared data in a complex hierarchy.\n\n" );
                     msg += _( "Do you wish to convert it to a simple hierarchical sheet?" );
@@ -211,7 +211,7 @@ bool SCH_EDIT_FRAME::EditSheet( SCH_SHEET* aSheet, SCH_SHEET_PATH* aHierarchy )
             // If the the associated screen is shared by more than one sheet, remove the
             // screen and reload the file to a new screen.  Failure to do this will trash
             // the screen reference counting in complex hierarchies.
-            if( aSheet->GetScreenCount() > 1 )
+            if( aSheet->IsSharedScreen() )
             {
                 aSheet->SetScreen( NULL );
                 loadFromFile = true;
diff --git a/gerbview/gerbview_frame.h b/gerbview/gerbview_frame.h
index e80b3be..0d2ae64 100644
--- a/gerbview/gerbview_frame.h
+++ b/gerbview/gerbview_frame.h
@@ -168,10 +168,15 @@ private:
     // An array sting to store warning messages when reaging a gerber file.
     wxArrayString   m_Messages;
 
+    std::auto_ptr<GBR_SCREEN>   m_screen;
+
 public:
     GERBVIEW_FRAME( KIWAY* aKiway, wxWindow* aParent );
     ~GERBVIEW_FRAME();
 
+    virtual GBR_SCREEN* GetScreen() const { return m_screen.get(); }
+    void    SetScreen( GBR_SCREEN* a_screen ) { m_screen.reset( a_screen ); }
+
     void    OnCloseWindow( wxCloseEvent& Event );
 
     bool    OpenProjectFiles( const std::vector<wxString>& aFileSet, int aCtl );   // overload KIWAY_PLAYER
diff --git a/include/draw_frame.h b/include/draw_frame.h
index 7765e97..e15f744 100644
--- a/include/draw_frame.h
+++ b/include/draw_frame.h
@@ -50,8 +50,6 @@ class EDA_DRAW_FRAME : public KIWAY_PLAYER
     ///< Id of active button on the vertical toolbar.
     int         m_toolId;
 
-    BASE_SCREEN*    m_currentScreen;            ///< current used SCREEN
-
     bool        m_snapToGrid;                   ///< Indicates if cursor should be snapped to grid.
     bool        m_galCanvasActive;              ///< whether to use new GAL engine
 
@@ -129,8 +127,6 @@ protected:
     /// One-shot to avoid a recursive mouse event during hotkey movement
     bool            m_movingCursorWithKeyboard;
 
-    void SetScreen( BASE_SCREEN* aScreen )  { m_currentScreen = aScreen; }
-
     /**
      * Function unitsChangeRefresh
      * is called when when the units setting has changed to allow for any derived classes
@@ -302,7 +298,7 @@ public:
      * derivatives.  It is overloaded by derived classes to return
      * SCH_SCREEN or PCB_SCREEN.
      */
-    virtual BASE_SCREEN* GetScreen() const  { return m_currentScreen; }
+    virtual BASE_SCREEN* GetScreen() const = 0;
 
     /**
      * Execute a remote command send via a socket to the application,
diff --git a/include/wxBasePcbFrame.h b/include/wxBasePcbFrame.h
index 81f592e..100b6cf 100644
--- a/include/wxBasePcbFrame.h
+++ b/include/wxBasePcbFrame.h
@@ -66,6 +66,9 @@ class FPID;
  */
 class PCB_BASE_FRAME : public EDA_DRAW_FRAME
 {
+private:
+    std::auto_ptr<PCB_SCREEN> m_screen;
+
 public:
     DISPLAY_OPTIONS m_DisplayOptions;
     EDA_UNITS_T m_UserGridUnit;
@@ -197,7 +200,8 @@ public:
     virtual void SetToolID( int aId, int aCursor, const wxString& aToolMsg );
     virtual void UpdateStatusBar();
 
-    PCB_SCREEN* GetScreen() const { return (PCB_SCREEN*) EDA_DRAW_FRAME::GetScreen(); }
+    virtual PCB_SCREEN* GetScreen() const override { return m_screen.get(); }
+    void SetScreen( PCB_SCREEN* a_screen ) { m_screen.reset( a_screen ); }
 
     /**
      * Function BestZoom
diff --git a/pagelayout_editor/pl_editor_frame.h b/pagelayout_editor/pl_editor_frame.h
index 67a5a29..8cb70ff 100644
--- a/pagelayout_editor/pl_editor_frame.h
+++ b/pagelayout_editor/pl_editor_frame.h
@@ -62,6 +62,8 @@ class PL_EDITOR_FRAME : public EDA_DRAW_FRAME
 
     wxPoint     m_grid_origin;
 
+    std::auto_ptr<PL_EDITOR_SCREEN> m_screen;
+
 protected:
     /// The last filename chosen to be proposed to the user
     wxString                m_lastFileName;
@@ -118,10 +120,8 @@ public:
      */
     const wxString GetZoomLevelIndicator() const;
 
-    PL_EDITOR_SCREEN* GetScreen() const                         // overload EDA_DRAW_FRAME
-    {
-        return (PL_EDITOR_SCREEN*) EDA_DRAW_FRAME::GetScreen();
-    }
+    virtual PL_EDITOR_SCREEN* GetScreen() const override { return m_screen.get(); }
+    void SetScreen( PL_EDITOR_SCREEN* a_screen ) { m_screen.reset( a_screen ); }
 
     const wxPoint& GetAuxOrigin() const                         // overload EDA_DRAW_FRAME
     {