safely delete browser items if createChildren() is running in thread

qgis · Dec 15, 2014 · 138d836 · 138d836
1 parent d28619d
commit 138d836
Show file tree

Hide file tree

Showing 3 changed files with 115 additions and 31 deletions.
diff --git a/src/core/qgsdataitem.cpp b/src/core/qgsdataitem.cpp
@@ -160,18 +160,58 @@ QgsDataItem::QgsDataItem( QgsDataItem::Type type, QgsDataItem* parent, QString n
     : QObject()
     , mType( type )
     , mCapabilities( NoCapabilities )
-    , mParent( parent )
+    , mParent( 0 ) // items are created without parent, parent is set later by setParent()
     , mState( NotPopulated )
     , mPopulated( false )
     , mName( name )
     , mPath( path )
+    , mDeferredDelete( false )
     , mWatcher( 0 )
 {
+  Q_UNUSED( parent );
 }
 
 QgsDataItem::~QgsDataItem()
 {
-  QgsDebugMsgLevel( "mName = " + mName + " mPath = " + mPath, 2 );
+  QgsDebugMsgLevel( QString( "mName = %1 mPath = %2 mChildren.size() = %3" ).arg( mName ).arg( mPath ).arg( mChildren.size() ) , 2 );
+  foreach ( QgsDataItem *child, mChildren )
+  {
+    if ( !child ) // should not happen
+      continue;
+    child->deleteLater();
+  }
+  mChildren.clear();
+
+  if ( mWatcher && !mWatcher->isFinished() )
+  {
+    // this should not usually happen (until the item was deleted directly when createChildren was running)
+    QgsDebugMsg( "mWatcher not finished (should not happen) -> waitForFinished()" );
+    mDeferredDelete = true;
+    mWatcher->waitForFinished();
+  }
+}
+
+void QgsDataItem::deleteLater()
+{
+  QgsDebugMsg( "path = " + path() );
+  setParent( 0 ); // also disconnects parent
+  foreach ( QgsDataItem *child, mChildren )
+  {
+    if ( !child ) // should not happen
+      continue;
+    child->deleteLater();
+  }
+  mChildren.clear();
+
+  if ( mWatcher && !mWatcher->isFinished() )
+  {
+    QgsDebugMsg( "mWatcher not finished -> schedule to delete later" );
+    mDeferredDelete = true;
+  }
+  else
+  {
+    QObject::deleteLater();
+  }
 }
 
 QIcon QgsDataItem::icon()
@@ -278,6 +318,14 @@ QVector<QgsDataItem*> QgsDataItem::runCreateChildren( QgsDataItem* item )
 void QgsDataItem::childrenCreated()
 {
   QgsDebugMsg( QString( "path = %1 children.size() = %2" ).arg( path() ).arg( mWatcher->result().size() ) );
+
+  if ( deferredDelete() )
+  {
+    QgsDebugMsg( "Item was scheduled to be deleted later" );
+    QObject::deleteLater();
+    return;
+  }
+
   if ( mChildren.size() == 0 ) // usually populating but may also be refresh if originaly there were no children
   {
     populate( mWatcher->result() );
@@ -376,7 +424,7 @@ void QgsDataItem::refresh( QVector<QgsDataItem*> children )
         mChildren.value( index )->refresh( child->children() );
       }
 
-      delete child;
+      child->deleteLater();
       continue;
     }
     addChildItem( child, true );
@@ -393,6 +441,41 @@ bool QgsDataItem::hasChildren()
   return ( state() == Populated ? mChildren.count() > 0 : true );
 }
 
+void QgsDataItem::setParent( QgsDataItem* parent )
+{
+  if ( mParent )
+  {
+    disconnect( this, SIGNAL( beginInsertItems( QgsDataItem*, int, int ) ),
+                mParent, SLOT( emitBeginInsertItems( QgsDataItem*, int, int ) ) );
+    disconnect( this, SIGNAL( endInsertItems() ),
+                mParent, SLOT( emitEndInsertItems() ) );
+    disconnect( this, SIGNAL( beginRemoveItems( QgsDataItem*, int, int ) ),
+                mParent, SLOT( emitBeginRemoveItems( QgsDataItem*, int, int ) ) );
+    disconnect( this, SIGNAL( endRemoveItems() ),
+                mParent, SLOT( emitEndRemoveItems() ) );
+    disconnect( this, SIGNAL( dataChanged( QgsDataItem* ) ),
+                mParent, SLOT( emitDataChanged( QgsDataItem* ) ) );
+    disconnect( this, SIGNAL( stateChanged( QgsDataItem*, QgsDataItem::State ) ),
+                mParent, SLOT( emitStateChanged( QgsDataItem*, QgsDataItem::State ) ) );
+  }
+  if ( parent )
+  {
+    connect( this, SIGNAL( beginInsertItems( QgsDataItem*, int, int ) ),
+             parent, SLOT( emitBeginInsertItems( QgsDataItem*, int, int ) ) );
+    connect( this, SIGNAL( endInsertItems() ),
+             parent, SLOT( emitEndInsertItems() ) );
+    connect( this, SIGNAL( beginRemoveItems( QgsDataItem*, int, int ) ),
+             parent, SLOT( emitBeginRemoveItems( QgsDataItem*, int, int ) ) );
+    connect( this, SIGNAL( endRemoveItems() ),
+             parent, SLOT( emitEndRemoveItems() ) );
+    connect( this, SIGNAL( dataChanged( QgsDataItem* ) ),
+             parent, SLOT( emitDataChanged( QgsDataItem* ) ) );
+    connect( this, SIGNAL( stateChanged( QgsDataItem*, QgsDataItem::State ) ),
+             parent, SLOT( emitStateChanged( QgsDataItem*, QgsDataItem::State ) ) );
+  }
+  mParent = parent;
+}
+
 void QgsDataItem::addChildItem( QgsDataItem * child, bool refresh )
 {
   QgsDebugMsg( QString( "path = %1 add child #%2 - %3 - %4" ).arg( mPath ).arg( mChildren.size() ).arg( child->mName ).arg( child->mType ) );
@@ -423,19 +506,6 @@ void QgsDataItem::addChildItem( QgsDataItem * child, bool refresh )
   mChildren.insert( i, child );
   child->setParent( this );
 
-  connect( child, SIGNAL( beginInsertItems( QgsDataItem*, int, int ) ),
-           this, SLOT( emitBeginInsertItems( QgsDataItem*, int, int ) ) );
-  connect( child, SIGNAL( endInsertItems() ),
-           this, SLOT( emitEndInsertItems() ) );
-  connect( child, SIGNAL( beginRemoveItems( QgsDataItem*, int, int ) ),
-           this, SLOT( emitBeginRemoveItems( QgsDataItem*, int, int ) ) );
-  connect( child, SIGNAL( endRemoveItems() ),
-           this, SLOT( emitEndRemoveItems() ) );
-  connect( child, SIGNAL( dataChanged( QgsDataItem* ) ),
-           this, SLOT( emitDataChanged( QgsDataItem* ) ) );
-  connect( child, SIGNAL( stateChanged( QgsDataItem*, QgsDataItem::State ) ),
-           this, SLOT( emitStateChanged( QgsDataItem*, QgsDataItem::State ) ) );
-
   if ( refresh )
     emit endInsertItems();
 }
@@ -446,7 +516,7 @@ void QgsDataItem::deleteChildItem( QgsDataItem * child )
   Q_ASSERT( i >= 0 );
   emit beginRemoveItems( this, i, i );
   mChildren.remove( i );
-  delete child; // deleting QObject child removes it from QObject parent
+  child->deleteLater();
   emit endRemoveItems();
 }
 
@@ -458,18 +528,6 @@ QgsDataItem * QgsDataItem::removeChildItem( QgsDataItem * child )
   emit beginRemoveItems( this, i, i );
   mChildren.remove( i );
   emit endRemoveItems();
-  disconnect( child, SIGNAL( beginInsertItems( QgsDataItem*, int, int ) ),
-              this, SLOT( emitBeginInsertItems( QgsDataItem*, int, int ) ) );
-  disconnect( child, SIGNAL( endInsertItems() ),
-              this, SLOT( emitEndInsertItems() ) );
-  disconnect( child, SIGNAL( beginRemoveItems( QgsDataItem*, int, int ) ),
-              this, SLOT( emitBeginRemoveItems( QgsDataItem*, int, int ) ) );
-  disconnect( child, SIGNAL( endRemoveItems() ),
-              this, SLOT( emitEndRemoveItems() ) );
-  disconnect( child, SIGNAL( dataChanged( QgsDataItem* ) ),
-              this, SLOT( emitDataChanged( QgsDataItem* ) ) );
-  disconnect( child, SIGNAL( stateChanged( QgsDataItem*, QgsDataItem::State ) ),
-              this, SLOT( emitStateChanged( QgsDataItem*, QgsDataItem::State ) ) );
   child->setParent( 0 );
   return child;
 }

diff --git a/src/core/qgsdataitem.h b/src/core/qgsdataitem.h
@@ -53,6 +53,8 @@ class CORE_EXPORT QgsDataItem : public QObject
       Favourites
     };
 
+    /** Create new data item.
+     * @param parent - the param is ignored, children created by createChildren() do not have parent, parent is set later by setParent if the item really becomes a parent child */
     QgsDataItem( QgsDataItem::Type type, QgsDataItem* parent, QString name, QString path );
     virtual ~QgsDataItem();
 
@@ -62,7 +64,8 @@ class CORE_EXPORT QgsDataItem : public QObject
 
     virtual void refresh();
 
-    // Create vector of children
+    /** Create children. Children are not expected to have parent set.
+     * This method MUST BE THREAD SAFE. */
     virtual QVector<QgsDataItem*> createChildren();
 
     // Populate children using children vector created by createChildren()
@@ -139,8 +142,12 @@ class CORE_EXPORT QgsDataItem : public QObject
     // members
 
     Type type() const { return mType; }
+    /** Get item parent. QgsDataItem maintains its own items hierarchy, it does not use
+     *  QObject hierarchy. */
     QgsDataItem* parent() const { return mParent; }
-    void setParent( QgsDataItem* parent ) { mParent = parent; }
+    /** Set item parent and connect / disconnect parent to / from item signals.
+     *  It does not add itself to parents children (mChildren) */
+    void setParent( QgsDataItem* parent );
     QVector<QgsDataItem*> children() const { return mChildren; }
     virtual QIcon icon();
     QString name() const { return mName; }
@@ -160,6 +167,13 @@ class CORE_EXPORT QgsDataItem : public QObject
     virtual void populate( QVector<QgsDataItem*> children );
     virtual void refresh( QVector<QgsDataItem*> children );
     QIcon populatingIcon() { return mPopulatingIcon; }
+    /** The item is scheduled to be deleted. E.g. if deleteLater() is called when
+     * item is in Populating state (createChildren() running in another thread),
+     * the deferredDelete() returns true and item will be deleted once Populating finished.
+     * Items with slow reateChildren() (for example network or database based) may
+     * check during createChildren() if deferredDelete() returns true and return from
+     * createChildren() immediately because result will be useless. */
+    bool deferredDelete() { return mDeferredDelete; }
 
     Type mType;
     Capabilities mCapabilities;
@@ -180,6 +194,14 @@ class CORE_EXPORT QgsDataItem : public QObject
     static QMap<QString, QIcon> mIconMap;
 
   public slots:
+    /** Safely delete the item:
+     *   - disconnects parent
+     *   - unsets parent (but does not remove itself)
+     *   - deletes all its descendants recursively
+     *   - waits until Populating state (createChildren() in thread) finished without blocking main thread
+     *   - calls QObject::deleteLater()
+     */
+    virtual void deleteLater();
     void emitBeginInsertItems( QgsDataItem* parent, int first, int last );
     void emitEndInsertItems();
     void emitBeginRemoveItems( QgsDataItem* parent, int first, int last );
@@ -201,6 +223,8 @@ class CORE_EXPORT QgsDataItem : public QObject
   private:
     static QVector<QgsDataItem*> runCreateChildren( QgsDataItem* item );
 
+    // Set to true if object has to be deleted when possible (nothing running in threads)
+    bool mDeferredDelete;
     QFutureWatcher< QVector <QgsDataItem*> > *mWatcher;
     // number of items currently in loading (populating) state
     static int mPopulatingCount;

diff --git a/src/providers/postgres/qgspostgresdataitems.cpp b/src/providers/postgres/qgspostgresdataitems.cpp
@@ -47,6 +47,7 @@ QVector<QgsDataItem*> QgsPGConnectionItem::createChildren()
   QVector<QgsDataItem*>items;
 
   QgsDataSourceURI uri = QgsPostgresConn::connUri( mName );
+  // TODO: wee need to cancel somehow acquireConnection() if deleteLater() was called on this item to avoid later credential dialog if connection failed
   QgsPostgresConn *conn = QgsPostgresConnPool::instance()->acquireConnection( uri.connectionInfo() );
   if ( !conn )
   {
@@ -302,6 +303,7 @@ QVector<QgsDataItem*> QgsPGSchemaItem::createChildren()
                                    QgsPostgresConn::geometryColumnsOnly( mName ),
                                    QgsPostgresConn::publicSchemaOnly( mName ),
                                    QgsPostgresConn::allowGeometrylessTables( mName ) );
+
   if ( !ok )
   {
     items.append( new QgsErrorItem( this, tr( "Failed to get layers" ), mPath + "/error" ) );