Merge pull request #5234 from nyalldawson/action_lifetime

Fix browser action item lifetime
qgis · Sep 22, 2017 · db738f6 · db738f6
2 parents cf636dc + 387771f
commit db738f6
Show file tree

Hide file tree

Showing 35 changed files with 185 additions and 173 deletions.
diff --git a/doc/api_break.dox b/doc/api_break.dox
@@ -961,6 +961,8 @@ QgsDataItem        {#qgis_api_break_3_0_QgsDataItem}
 - capabilities() has been removed. Use capabilities2() instead (TODO: rename back to capabilities()).
 - emitBeginInsertItems(), emitEndInsertItems(), emitBeginRemoveItems(), emitEndRemoveItems(), emitDataChanged(), emitStateChanged() have been removed.
 - Favourites was renamed to Favorites
+- actions() now requires a new QWidget parent argument. Subclasses should ensure that returned items have been
+correctly parented to this widget.
 
 QgsDataItemProviderRegistry        {#qgis_api_break_3_0_QgsDataItemProviderRegistry}
 ---------------------------

diff --git a/python/core/qgsdataitem.sip b/python/core/qgsdataitem.sip
@@ -131,10 +131,13 @@ Create new data item.
  :rtype: QWidget
 %End
 
-    virtual QList<QAction *> actions();
+    virtual QList<QAction *> actions( QWidget *parent );
 %Docstring
  Returns the list of actions available for this item. This is usually used for the popup menu on right-clicking
  the item. Subclasses should override this to provide actions.
+
+ Subclasses should ensure that ownership of created actions is correctly handled by parenting them
+ to the specified parent widget.
  :rtype: list of QAction
 %End
 
@@ -543,7 +546,7 @@ Check if the given path is hidden from the browser model
  :rtype: bool
 %End
 
-    virtual QList<QAction *> actions();
+    virtual QList<QAction *> actions( QWidget *parent );
 
 
 

diff --git a/src/core/qgsdataitem.cpp b/src/core/qgsdataitem.cpp
@@ -511,6 +511,12 @@ bool QgsDataItem::equal( const QgsDataItem *other )
            mPath == other->path() );
 }
 
+QList<QAction *> QgsDataItem::actions( QWidget *parent )
+{
+  Q_UNUSED( parent );
+  return QList<QAction *>();
+}
+
 bool QgsDataItem::handleDoubleClick()
 {
   return false;
@@ -859,10 +865,10 @@ bool QgsDirectoryItem::hiddenPath( const QString &path )
   return ( idx > -1 );
 }
 
-QList<QAction *> QgsDirectoryItem::actions()
+QList<QAction *> QgsDirectoryItem::actions( QWidget *parent )
 {
   QList<QAction *> result;
-  QAction *openFolder = new QAction( tr( "Open Directory…" ), this );
+  QAction *openFolder = new QAction( tr( "Open Directory…" ), parent );
   connect( openFolder, &QAction::triggered, this, [ = ]
   {
     QDesktopServices::openUrl( QUrl::fromLocalFile( mDirPath ) );

diff --git a/src/core/qgsdataitem.h b/src/core/qgsdataitem.h
@@ -139,10 +139,14 @@ class CORE_EXPORT QgsDataItem : public QObject
 
     virtual QWidget *paramWidget() SIP_FACTORY { return nullptr; }
 
-    /** Returns the list of actions available for this item. This is usually used for the popup menu on right-clicking
+    /**
+     * Returns the list of actions available for this item. This is usually used for the popup menu on right-clicking
      * the item. Subclasses should override this to provide actions.
+     *
+     * Subclasses should ensure that ownership of created actions is correctly handled by parenting them
+     * to the specified parent widget.
      */
-    virtual QList<QAction *> actions() { return QList<QAction *>(); }
+    virtual QList<QAction *> actions( QWidget *parent );
 
     /** Returns whether the item accepts drag and dropped layers - e.g. for importing a dataset to a provider.
      * Subclasses should override this and handleDrop() to accept dropped layers.
@@ -485,7 +489,7 @@ class CORE_EXPORT QgsDirectoryItem : public QgsDataCollectionItem
     //! Check if the given path is hidden from the browser model
     static bool hiddenPath( const QString &path );
 
-    QList<QAction *> actions() override;
+    QList<QAction *> actions( QWidget *parent ) override;
 
 
   public slots:

diff --git a/src/gui/qgsbrowserdockwidget.cpp b/src/gui/qgsbrowserdockwidget.cpp
@@ -209,7 +209,7 @@ void QgsBrowserDockWidget::showContextMenu( QPoint pt )
     menu->addAction( tr( "Add a Directory..." ), this, SLOT( addFavoriteDirectory() ) );
   }
 
-  QList<QAction *> actions = item->actions();
+  QList<QAction *> actions = item->actions( menu );
   if ( !actions.isEmpty() )
   {
     if ( !menu->actions().isEmpty() )

diff --git a/src/providers/arcgisrest/qgsafsdataitems.cpp b/src/providers/arcgisrest/qgsafsdataitems.cpp
@@ -50,9 +50,9 @@ QVector<QgsDataItem *> QgsAfsRootItem::createChildren()
 }
 
 #ifdef HAVE_GUI
-QList<QAction *> QgsAfsRootItem::actions()
+QList<QAction *> QgsAfsRootItem::actions( QWidget *parent )
 {
-  QAction *actionNew = new QAction( tr( "New Connection..." ), this );
+  QAction *actionNew = new QAction( tr( "New Connection..." ), parent );
   connect( actionNew, &QAction::triggered, this, &QgsAfsRootItem::newConnection );
   return QList<QAction *>() << actionNew;
 }
@@ -120,15 +120,15 @@ bool QgsAfsConnectionItem::equal( const QgsDataItem *other )
 }
 
 #ifdef HAVE_GUI
-QList<QAction *> QgsAfsConnectionItem::actions()
+QList<QAction *> QgsAfsConnectionItem::actions( QWidget *parent )
 {
   QList<QAction *> lst;
 
-  QAction *actionEdit = new QAction( tr( "Edit..." ), this );
+  QAction *actionEdit = new QAction( tr( "Edit..." ), parent );
   connect( actionEdit, &QAction::triggered, this, &QgsAfsConnectionItem::editConnection );
   lst.append( actionEdit );
 
-  QAction *actionDelete = new QAction( tr( "Delete" ), this );
+  QAction *actionDelete = new QAction( tr( "Delete" ), parent );
   connect( actionDelete, &QAction::triggered, this, &QgsAfsConnectionItem::deleteConnection );
   lst.append( actionDelete );
 

diff --git a/src/providers/arcgisrest/qgsafsdataitems.h b/src/providers/arcgisrest/qgsafsdataitems.h
@@ -26,7 +26,7 @@ class QgsAfsRootItem : public QgsDataCollectionItem
     QgsAfsRootItem( QgsDataItem *parent, const QString &name, const QString &path );
     QVector<QgsDataItem *> createChildren() override;
 #ifdef HAVE_GUI
-    QList<QAction *> actions() override;
+    QList<QAction *> actions( QWidget *parent ) override;
     QWidget *paramWidget() override;
 #endif
 
@@ -46,7 +46,7 @@ class QgsAfsConnectionItem : public QgsDataCollectionItem
     QVector<QgsDataItem *> createChildren() override;
     bool equal( const QgsDataItem *other ) override;
 #ifdef HAVE_GUI
-    QList<QAction *> actions() override;
+    QList<QAction *> actions( QWidget *parent ) override;
 #endif
 
   public slots:

diff --git a/src/providers/arcgisrest/qgsamsdataitems.cpp b/src/providers/arcgisrest/qgsamsdataitems.cpp
@@ -48,9 +48,9 @@ QVector<QgsDataItem *> QgsAmsRootItem::createChildren()
 }
 
 #ifdef HAVE_GUI
-QList<QAction *> QgsAmsRootItem::actions()
+QList<QAction *> QgsAmsRootItem::actions( QWidget *parent )
 {
-  QAction *actionNew = new QAction( tr( "New Connection..." ), this );
+  QAction *actionNew = new QAction( tr( "New Connection..." ), parent );
   connect( actionNew, &QAction::triggered, this, &QgsAmsRootItem::newConnection );
   return QList<QAction *>() << actionNew;
 }
@@ -136,15 +136,15 @@ bool QgsAmsConnectionItem::equal( const QgsDataItem *other )
 }
 
 #ifdef HAVE_GUI
-QList<QAction *> QgsAmsConnectionItem::actions()
+QList<QAction *> QgsAmsConnectionItem::actions( QWidget *parent )
 {
   QList<QAction *> lst;
 
-  QAction *actionEdit = new QAction( tr( "Edit..." ), this );
+  QAction *actionEdit = new QAction( tr( "Edit..." ), parent );
   connect( actionEdit, &QAction::triggered, this, &QgsAmsConnectionItem::editConnection );
   lst.append( actionEdit );
 
-  QAction *actionDelete = new QAction( tr( "Delete" ), this );
+  QAction *actionDelete = new QAction( tr( "Delete" ), parent );
   connect( actionDelete, &QAction::triggered, this, &QgsAmsConnectionItem::deleteConnection );
   lst.append( actionDelete );
 

diff --git a/src/providers/arcgisrest/qgsamsdataitems.h b/src/providers/arcgisrest/qgsamsdataitems.h
@@ -29,7 +29,7 @@ class QgsAmsRootItem : public QgsDataCollectionItem
 
     QVector<QgsDataItem *> createChildren() override;
 #ifdef HAVE_GUI
-    virtual QList<QAction *> actions() override;
+    virtual QList<QAction *> actions( QWidget *parent ) override;
     virtual QWidget *paramWidget() override;
 #endif
 
@@ -49,7 +49,7 @@ class QgsAmsConnectionItem : public QgsDataCollectionItem
     QVector<QgsDataItem *> createChildren() override;
     bool equal( const QgsDataItem *other ) override;
 #ifdef HAVE_GUI
-    QList<QAction *> actions() override;
+    QList<QAction *> actions( QWidget *parent ) override;
 #endif
 
   public slots:

diff --git a/src/providers/db2/qgsdb2dataitems.cpp b/src/providers/db2/qgsdb2dataitems.cpp
@@ -250,19 +250,19 @@ bool QgsDb2ConnectionItem::equal( const QgsDataItem *other )
 }
 
 #ifdef HAVE_GUI
-QList<QAction *> QgsDb2ConnectionItem::actions()
+QList<QAction *> QgsDb2ConnectionItem::actions( QWidget *parent )
 {
   QList<QAction *> lst;
 
-  QAction *actionRefresh = new QAction( tr( "Refresh connection" ), this );
+  QAction *actionRefresh = new QAction( tr( "Refresh Connection" ), parent );
   connect( actionRefresh, &QAction::triggered, this, &QgsDb2ConnectionItem::refreshConnection );
   lst.append( actionRefresh );
 
-  QAction *actionEdit = new QAction( tr( "Edit connection..." ), this );
+  QAction *actionEdit = new QAction( tr( "Edit Connection..." ), parent );
   connect( actionEdit, &QAction::triggered, this, &QgsDb2ConnectionItem::editConnection );
   lst.append( actionEdit );
 
-  QAction *actionDelete = new QAction( tr( "Delete connection" ), this );
+  QAction *actionDelete = new QAction( tr( "Delete Connection" ), parent );
   connect( actionDelete, &QAction::triggered, this, &QgsDb2ConnectionItem::deleteConnection );
   lst.append( actionDelete );
 
@@ -430,14 +430,13 @@ QVector<QgsDataItem *> QgsDb2RootItem::createChildren()
 }
 
 #ifdef HAVE_GUI
-QList<QAction *> QgsDb2RootItem::actions()
+QList<QAction *> QgsDb2RootItem::actions( QWidget *parent )
 {
   QList<QAction *> actionList;
 
-  QAction *action = new QAction( tr( "New Connection..." ), this );
+  QAction *action = new QAction( tr( "New Connection..." ), parent );
   connect( action, &QAction::triggered, this, &QgsDb2RootItem::newConnection );
   actionList.append( action );
-  QgsDebugMsg( "DB2: Browser Panel; New Connection option added." );
 
   return actionList;
 }

diff --git a/src/providers/db2/qgsdb2dataitems.h b/src/providers/db2/qgsdb2dataitems.h
@@ -45,7 +45,7 @@ class QgsDb2RootItem : public QgsDataCollectionItem
 #ifdef HAVE_GUI
     virtual QWidget *paramWidget() override;
 
-    virtual QList<QAction *> actions() override;
+    QList<QAction *> actions( QWidget *parent ) override;
 #endif
 
   public slots:
@@ -90,10 +90,7 @@ class QgsDb2ConnectionItem : public QgsDataCollectionItem
 
 #ifdef HAVE_GUI
 
-    /**
-     * Add Refresh, Edit, and Delete actions for every QgsDb2ConnectionItem.
-     */
-    virtual QList<QAction *> actions() override;
+    QList<QAction *> actions( QWidget *parent ) override;
 #endif
 
     virtual bool acceptDrop() override { return true; }

diff --git a/src/providers/geonode/qgsgeonodedataitems.cpp b/src/providers/geonode/qgsgeonodedataitems.cpp
@@ -66,10 +66,10 @@ QVector<QgsDataItem *> QgsGeoNodeConnectionItem::createChildren()
   return services;
 }
 
-QList<QAction *> QgsGeoNodeConnectionItem::actions()
+QList<QAction *> QgsGeoNodeConnectionItem::actions( QWidget *parent )
 {
-  QAction *actionEdit = new QAction( tr( "Edit Connection..." ), this );
-  QAction *actionDelete = new QAction( tr( "Delete Connection" ), this );
+  QAction *actionEdit = new QAction( tr( "Edit Connection..." ), parent );
+  QAction *actionDelete = new QAction( tr( "Delete Connection" ), parent );
   connect( actionEdit, &QAction::triggered, this, &QgsGeoNodeConnectionItem::editConnection );
   connect( actionDelete, &QAction::triggered, this, &QgsGeoNodeConnectionItem::deleteConnection );
   return QList<QAction *>() << actionEdit << actionDelete;
@@ -239,9 +239,9 @@ QVector<QgsDataItem *> QgsGeoNodeRootItem::createChildren()
   return connections;
 }
 
-QList<QAction *> QgsGeoNodeRootItem::actions()
+QList<QAction *> QgsGeoNodeRootItem::actions( QWidget *parent )
 {
-  QAction *actionNew = new QAction( tr( "New Connection..." ), this );
+  QAction *actionNew = new QAction( tr( "New Connection..." ), parent );
   connect( actionNew, &QAction::triggered, this, &QgsGeoNodeRootItem::newConnection );
   return QList<QAction *>() << actionNew;
 }

diff --git a/src/providers/geonode/qgsgeonodedataitems.h b/src/providers/geonode/qgsgeonodedataitems.h
@@ -28,7 +28,7 @@ class QgsGeoNodeConnectionItem : public QgsDataCollectionItem
   public:
     QgsGeoNodeConnectionItem( QgsDataItem *parent, QString name, QString path, std::unique_ptr< QgsGeoNodeConnection > conn );
     QVector<QgsDataItem *> createChildren() override;
-    virtual QList<QAction *> actions() override;
+    QList<QAction *> actions( QWidget *parent ) override;
 
     QString mGeoNodeName;
 
@@ -67,7 +67,7 @@ class QgsGeoNodeRootItem : public QgsDataCollectionItem
 
     QVector<QgsDataItem *> createChildren() override;
 
-    virtual QList<QAction *> actions() override;
+    virtual QList<QAction *> actions( QWidget *parent ) override;
 
   private slots:
     void newConnection();

diff --git a/src/providers/grass/qgsgrassprovidermodule.cpp b/src/providers/grass/qgsgrassprovidermodule.cpp
@@ -51,11 +51,11 @@ QgsGrassItemActions::QgsGrassItemActions( const QgsGrassObject &grassObject, boo
 {
 }
 
-QList<QAction *> QgsGrassItemActions::actions()
+QList<QAction *> QgsGrassItemActions::actions( QWidget *parent )
 {
   QList<QAction *> list;
 
-  QAction *optionsAction = new QAction( tr( "GRASS Options" ), this );
+  QAction *optionsAction = new QAction( tr( "GRASS Options" ), parent );
   connect( optionsAction, &QAction::triggered, QgsGrass::instance(), &QgsGrass::openOptions );
   list << optionsAction;
 
@@ -65,14 +65,14 @@ QList<QAction *> QgsGrassItemActions::actions()
   // TODO: check ownership
   if ( mGrassObject.type() == QgsGrassObject::Location )
   {
-    QAction *newMapsetAction = new QAction( QgsApplication::getThemeIcon( QStringLiteral( "grass_new_mapset.png" ) ), tr( "New mapset" ), this );
+    QAction *newMapsetAction = new QAction( QgsApplication::getThemeIcon( QStringLiteral( "grass_new_mapset.png" ) ), tr( "New mapset" ), parent );
     connect( newMapsetAction, &QAction::triggered, this, &QgsGrassItemActions::newMapset );
     list << newMapsetAction;
   }
 
   if ( mGrassObject.type() == QgsGrassObject::Mapset && isMapsetOwner )
   {
-    QAction *openMapsetAction = new QAction( QgsApplication::getThemeIcon( QStringLiteral( "grass_open_mapset.png" ) ), tr( "Open mapset" ), this );
+    QAction *openMapsetAction = new QAction( QgsApplication::getThemeIcon( QStringLiteral( "grass_open_mapset.png" ) ), tr( "Open mapset" ), parent );
     connect( openMapsetAction, &QAction::triggered, this, &QgsGrassItemActions::openMapset );
     list << openMapsetAction;
   }
@@ -82,13 +82,13 @@ QList<QAction *> QgsGrassItemActions::actions()
   {
     if ( !QgsGrass::instance()->isMapsetInSearchPath( mGrassObject.mapset() ) )
     {
-      QAction *openMapsetAction = new QAction( tr( "Add mapset to search path" ), this );
+      QAction *openMapsetAction = new QAction( tr( "Add mapset to search path" ), parent );
       connect( openMapsetAction, &QAction::triggered, this, &QgsGrassItemActions::addMapsetToSearchPath );
       list << openMapsetAction;
     }
     else
     {
-      QAction *openMapsetAction = new QAction( tr( "Remove mapset from search path" ), this );
+      QAction *openMapsetAction = new QAction( tr( "Remove mapset from search path" ), parent );
       connect( openMapsetAction, &QAction::triggered, this, &QgsGrassItemActions::removeMapsetFromSearchPath );
       list << openMapsetAction;
     }
@@ -97,11 +97,11 @@ QList<QAction *> QgsGrassItemActions::actions()
   if ( ( mGrassObject.type() == QgsGrassObject::Raster || mGrassObject.type() == QgsGrassObject::Vector
          ||  mGrassObject.type() == QgsGrassObject::Group ) && isMapsetOwner )
   {
-    QAction *renameAction = new QAction( tr( "Rename" ), this );
+    QAction *renameAction = new QAction( tr( "Rename" ), parent );
     connect( renameAction, &QAction::triggered, this, &QgsGrassItemActions::renameGrassObject );
     list << renameAction;
 
-    QAction *deleteAction = new QAction( tr( "Delete" ), this );
+    QAction *deleteAction = new QAction( tr( "Delete" ), parent );
     connect( deleteAction, &QAction::triggered, this, &QgsGrassItemActions::deleteGrassObject );
     list << deleteAction;
   }
@@ -110,15 +110,15 @@ QList<QAction *> QgsGrassItemActions::actions()
        && mValid && isMapsetOwner )
   {
     // TODO: disable new layer actions on maps currently being edited
-    QAction *newPointAction = new QAction( tr( "New Point Layer" ), this );
+    QAction *newPointAction = new QAction( tr( "New Point Layer" ), parent );
     connect( newPointAction, &QAction::triggered, this, &QgsGrassItemActions::newPointLayer );
     list << newPointAction;
 
-    QAction *newLineAction = new QAction( tr( "New Line Layer" ), this );
+    QAction *newLineAction = new QAction( tr( "New Line Layer" ), parent );
     connect( newLineAction, &QAction::triggered, this, &QgsGrassItemActions::newLineLayer );
     list << newLineAction;
 
-    QAction *newPolygonAction = new QAction( tr( "New Polygon Layer" ), this );
+    QAction *newPolygonAction = new QAction( tr( "New Polygon Layer" ), parent );
     connect( newPolygonAction, &QAction::triggered, this, &QgsGrassItemActions::newPolygonLayer );
     list << newPolygonAction;
   }
@@ -1178,11 +1178,11 @@ QgsGrassImportItem::~QgsGrassImportItem()
 }
 
 #ifdef HAVE_GUI
-QList<QAction *> QgsGrassImportItem::actions()
+QList<QAction *> QgsGrassImportItem::actions( QWidget *parent )
 {
   QList<QAction *> lst;
 
-  QAction *actionRename = new QAction( tr( "Cancel" ), this );
+  QAction *actionRename = new QAction( tr( "Cancel" ), parent );
   connect( actionRename, &QAction::triggered, this, &QgsGrassImportItem::cancel );
   lst.append( actionRename );