extragear/multimedia/amarok/src

[Mark Kretschmann]

SVN commit 929114 by markey:

Beginnings of SmartPointerList class:
A QList for storing pointers to QObjects, that automatically removes
the pointers when objects are deleted.

The evil scheme behind this idea is to fix many of our crashes that
result from storing (caching) QLists of pointers to QActions in various
places in Amarok. The problem is that sometimes these QActions get
deleted, and we end up with dangling pointers in these lists. That's
causing crashes like in BR 184630, when we dereference these dangling
pointers.

Now my class does have a few drawbacks still, which may be fixable with
more tweaking, or maybe not:

* Does not work with Qt's foreach(), as it subclasses QObject. You need
 to iterate over the list in traditional ways.
* Casting of pointers is sometimes needed

For testing, I did start to port some of our QList<QAction*> to
SmartPointerList, but not all of them.

You be the judge if this class is a good idea and a viable path.
I'm requesting code review and discussion.

CCMAIL: amarok-devel at kde.org
CCBUG: 184630

 M  +1 -0      CMakeLists.txt  
 M  +3 -11     GlobalCurrentTrackActions.cpp  
 M  +2 -3      GlobalCurrentTrackActions.h  
 A             SmartPointerList.cpp   GlobalCurrentTrackActions.h#929087 [License: GPL (v2+)]
 A             SmartPointerList.h   GlobalCurrentTrackActions.h#929087 [License: GPL (v2+)]
 M  +9 -12     Systray.cpp  
 M  +2 -1      Systray.h  
 M  +16 -9     widgets/MainToolbar.cpp  
 M  +2 -1      widgets/MainToolbar.h  


--- trunk/extragear/multimedia/amarok/src/CMakeLists.txt #929113:929114
@@ -515,6 +515,7 @@
     MountPointManager.cpp
     PaletteHandler.cpp
     PopupDropperFactory.cpp
+    SmartPointerList.cpp
     Systray.cpp
     widgets/hintlineedit.cpp
     widgets/kdatecombo.cpp
--- trunk/extragear/multimedia/amarok/src/GlobalCurrentTrackActions.cpp #929113:929114
@@ -43,7 +43,7 @@
 
 void GlobalCurrentTrackActions::addAction( QAction * action )
 {
-    m_actions.append( action );
+    m_actions.addPointer( action );
 }
 
 QList< QAction * > GlobalCurrentTrackActions::actions()
@@ -52,17 +52,9 @@
 
     QList<QAction*> validActions;
 
-    foreach( QAction* action, m_actions )
-    {
-        if( action )
-            validActions.append( action );
-    }
+    for( int i = 0; i < m_actions.size(); i++ )
+        validActions.append( (QAction*) m_actions[i] );
 
-    m_actions.clear();
-
-    foreach( QAction* action, validActions )
-        m_actions.append( action );
-
     return validActions;
 }
 
--- trunk/extragear/multimedia/amarok/src/GlobalCurrentTrackActions.h #929113:929114
@@ -22,9 +22,9 @@
 
 #include "amarok_export.h"
 #include "Meta.h"
+#include "SmartPointerList.h"
 
 #include <QAction>
-#include <QPointer>
 
 
 class GlobalCurrentTrackActions;
@@ -50,8 +50,7 @@
     GlobalCurrentTrackActions();
     ~GlobalCurrentTrackActions();
     
-    typedef QPointer<QAction> QActionPtr;
-    QList<QActionPtr> m_actions;
+    SmartPointerList m_actions;
 };
 
 #endif
--- trunk/extragear/multimedia/amarok/src/Systray.cpp #929113:929114
@@ -398,45 +398,42 @@
 void
 Amarok::TrayIcon::setupMenu()
 {
-    foreach( QAction * action, m_extraActions )
-    {
-        if( action )
-            contextMenu()->removeAction( action );
-    }
+    for( int i = 0; i < m_extraActions.size(); i ++ )
+        contextMenu()->removeAction( (QAction*) m_extraActions[i] );
 
     if( !m_track )
         return;
 
-    m_extraActions = The::globalCurrentTrackActions()->actions();
+    m_extraActions.clear();
+    foreach( QAction *action, The::globalCurrentTrackActions()->actions() )
+        m_extraActions.addPointer( action );
 
     if ( m_track->hasCapabilityInterface( Meta::Capability::CurrentTrackActions ) )
     {
         Meta::CurrentTrackActionsCapability *cac = m_track->as<Meta::CurrentTrackActionsCapability>();
         if( cac )
         {
-
             QList<PopupDropperAction *> currentTrackActions = cac->customActions();
             foreach( PopupDropperAction *action, currentTrackActions )
-                m_extraActions.append( action );
+                m_extraActions.addPointer( action );
         }
     }
 
-
     if ( m_extraActions.count() > 0 )
     {
         // remove the two bottom items, so we can push them to the button again
         contextMenu()->removeAction( actionCollection()->action( "file_quit" ) );
         contextMenu()->removeAction( actionCollection()->action( "minimizeRestore" ) );
 
-        foreach( QAction *action, m_extraActions )
-            contextMenu()->addAction( action );
+        for( int i = 0; i < m_extraActions.size(); i ++ )
+            contextMenu()->addAction( (QAction*) m_extraActions[i] );
+
         contextMenu()->addSeparator();
 
         // readd
             contextMenu()->addAction( actionCollection()->action( "minimizeRestore" ) );
         contextMenu()->addAction( actionCollection()->action( "file_quit" ) );
     }
-
 }
 
 void
--- trunk/extragear/multimedia/amarok/src/Systray.h #929113:929114
@@ -21,6 +21,7 @@
 
 #include "EngineObserver.h" //baseclass
 #include "meta/Meta.h"
+#include "SmartPointerList.h"
 
 #include <KAction>
 #include <KSystemTrayIcon> //baseclass
@@ -67,7 +68,7 @@
 
     QPixmap m_baseIcon, m_grayedIcon, m_icon;
     QPixmap m_playOverlay, m_pauseOverlay;
-    QList<QAction *> m_extraActions;
+    SmartPointerList m_extraActions;
 };
 
 }
--- trunk/extragear/multimedia/amarok/src/widgets/MainToolbar.cpp #929113:929114
@@ -125,26 +125,30 @@
 
 void MainToolbar::handleAddActions()
 {
-    foreach( QAction * action, m_additionalActions )
-        m_addControlsToolbar->removeAction( action );
+#if 0
+    foreach( QObject* action, m_additionalActions )
+        m_addControlsToolbar->removeAction( (QAction*) action );
+#endif
+    for( int i = 0; i < m_additionalActions.size(); i++ )
+        m_addControlsToolbar->removeAction( (QAction*) m_additionalActions[i] );
 
     m_additionalActions.clear();
 
     Meta::TrackPtr track = The::engineController()->currentTrack();
 
-    m_additionalActions = The::globalCurrentTrackActions()->actions();
+    m_additionalActions.clear();
+    foreach( QAction* action, The::globalCurrentTrackActions()->actions() )
+        m_addControlsToolbar->addAction( action );
     
     if ( track && track->hasCapabilityInterface( Meta::Capability::CurrentTrackActions ) )
     {
         Meta::CurrentTrackActionsCapability *cac = track->as<Meta::CurrentTrackActionsCapability>();
         if( cac )
         {
-
             QList<PopupDropperAction *> currentTrackActions = cac->customActions();
             foreach( PopupDropperAction *action, currentTrackActions )
-                m_additionalActions.append( action );
+                m_additionalActions.addPointer( action );
 
-
             m_addControlsToolbar->adjustSize();
 
             //centerAddActions();
@@ -152,10 +156,13 @@
         }
     }
 
+    for( int i = 0; i < m_additionalActions.size(); i++ )
+        m_addControlsToolbar->addAction( (QAction*) m_additionalActions[i] );
+#if 0
+    foreach( QObject* action, m_additionalActions )
+        m_addControlsToolbar->addAction( (QAction*) action );
+#endif
 
-    foreach( QAction *action, m_additionalActions )
-        m_addControlsToolbar->addAction( action );
-
     repaint ( 0, 0, -1, -1 ); // make sure that the add info area is shown or hidden at once.
 }
 
--- trunk/extragear/multimedia/amarok/src/widgets/MainToolbar.h #929113:929114
@@ -21,6 +21,7 @@
 #define MAINTOOLBAR_H
 
 #include "EngineObserver.h" //baseclass
+#include "SmartPointerList.h"
 
 #include <KHBox>
 
@@ -63,7 +64,7 @@
     int m_addActionsOffsetX;
     bool m_ignoreCache;
 
-    QList<QAction *> m_additionalActions;
+    SmartPointerList m_additionalActions;
 };
 
 #endif