simplify netwotk content fetcher registry

3nids · 3nids · commit 8e20996ddc04 · 2018-05-09T11:04:29.000-04:00
kudos @wonder-sk
diff --git a/python/core/qgsnetworkcontentfetcherregistry.sip.in b/python/core/qgsnetworkcontentfetcherregistry.sip.in
@@ -35,7 +35,7 @@ FetchedContent holds useful information about a network content being fetched
       Failed
     };
 
-    explicit QgsFetchedContent( QTemporaryFile *file = 0, ContentStatus status = NotStarted );
+    explicit QgsFetchedContent( const QString &url, QTemporaryFile *file = 0, ContentStatus status = NotStarted );
 %Docstring
 Constructs a FetchedContent with pointer to the downloaded file and status of the download
 %End
@@ -76,21 +76,6 @@ Return the potential error of the download
     void fetched();
 %Docstring
 Emitted when the file is fetched and accessible
-%End
-
-    void downloadStarted( const bool redownload );
-%Docstring
-Emitted when the download actually starts
-%End
-
-    void cancelTriggered();
-%Docstring
-Emitted when download is canceled.
-%End
-
-    void taskCompleted();
-%Docstring
-Emitted when the download is finished (although file not accessible yet)
 %End
 
 };
@@ -125,7 +110,7 @@ Create the registry for temporary downloaded files
 
     ~QgsNetworkContentFetcherRegistry();
 
-    const QgsFetchedContent *fetch( const QUrl &url, const FetchingMode fetchingMode = DownloadLater );
+    const QgsFetchedContent *fetch( const QString &url, const FetchingMode fetchingMode = DownloadLater );
 %Docstring
  Initialize a download for the given URL
 
diff --git a/src/core/qgsnetworkcontentfetcherregistry.cpp b/src/core/qgsnetworkcontentfetcherregistry.cpp
@@ -27,104 +27,30 @@ QgsNetworkContentFetcherRegistry::QgsNetworkContentFetcherRegistry()
 
 QgsNetworkContentFetcherRegistry::~QgsNetworkContentFetcherRegistry()
 {
-  QMap<QUrl, QgsFetchedContent *>::const_iterator it = mFileRegistry.constBegin();
+  QMap<QString, QgsFetchedContent *>::const_iterator it = mFileRegistry.constBegin();
   for ( ; it != mFileRegistry.constEnd(); ++it )
   {
     delete it.value();
   }
   mFileRegistry.clear();
 }
 
-const QgsFetchedContent *QgsNetworkContentFetcherRegistry::fetch( const QUrl &url, const FetchingMode fetchingMode )
+const QgsFetchedContent *QgsNetworkContentFetcherRegistry::fetch( const QString &url, const FetchingMode fetchingMode )
 {
-  QMutexLocker locker( &mMutex );
+
   if ( mFileRegistry.contains( url ) )
   {
     return mFileRegistry.value( url );
   }
 
-  QgsFetchedContent *content = new QgsFetchedContent( nullptr, QgsFetchedContent::NotStarted );
-
-  // start
-  QObject::connect( content, &QgsFetchedContent::downloadStarted, this, [ = ]( const bool redownload )
-  {
-    if ( redownload && content->status() == QgsFetchedContent::Downloading )
-    {
-      {
-        QMutexLocker locker( &mMutex );
-        if ( content->mFetchingTask )
-          disconnect( content->mFetchingTask, &QgsNetworkContentFetcherTask::fetched, content, &QgsFetchedContent::taskCompleted );
-      }
-      // no locker when calling cancel!
-      content->cancel();
-    }
-    QMutexLocker locker( &mMutex );
-    if ( redownload ||
-         content->status() == QgsFetchedContent::NotStarted ||
-         content->status() == QgsFetchedContent::Failed )
-    {
-      content->mFetchingTask = new QgsNetworkContentFetcherTask( url );
-      connect( content->mFetchingTask, &QgsNetworkContentFetcherTask::fetched, content, &QgsFetchedContent::taskCompleted );
-      QgsApplication::instance()->taskManager()->addTask( content->mFetchingTask );
-      content->mStatus = QgsFetchedContent::Downloading;
-    }
-  } );
-
-  // cancel
-  QObject::connect( content, &QgsFetchedContent::cancelTriggered, this, [ = ]()
-  {
-    QMutexLocker locker( &mMutex );
-    if ( content->mFetchingTask && content->mFetchingTask->canCancel() )
-    {
-      content->mFetchingTask->cancel();
-    }
-    if ( content->mFile )
-    {
-      content->mFile->deleteLater();
-      content->mFilePath = QString();
-    }
-  } );
-
-  // finished
-  connect( content, &QgsFetchedContent::taskCompleted, this, [ = ]()
-  {
-    QMutexLocker locker( &mMutex );
-    if ( !content->mFetchingTask || !content->mFetchingTask->reply() )
-    {
-      // if no reply, it has been canceled
-      content->mStatus = QgsFetchedContent::Failed;
-      content->mError = QNetworkReply::OperationCanceledError;
-      content->mFilePath = QString();
-    }
-    else
-    {
-      QNetworkReply *reply = content->mFetchingTask->reply();
-      if ( reply->error() == QNetworkReply::NoError )
-      {
-        QTemporaryFile *tf = new QTemporaryFile( QStringLiteral( "XXXXXX" ) );
-        content->mFile = tf;
-        tf->open();
-        content->mFile->write( reply->readAll() );
-        // Qt docs notes that on some system if fileName is not called before close, file might get deleted
-        content->mFilePath = tf->fileName();
-        tf->close();
-        content->mStatus = QgsFetchedContent::Finished;
-      }
-      else
-      {
-        content->mStatus = QgsFetchedContent::Failed;
-        content->mError = reply->error();
-        content->mFilePath = QString();
-      }
-    }
-    content->emitFetched();
-  } );
+  QgsFetchedContent *content = new QgsFetchedContent( url, nullptr, QgsFetchedContent::NotStarted );
 
   mFileRegistry.insert( url, content );
 
   if ( fetchingMode == DownloadImmediately )
     content->download();
 
+
   return content;
 }
 
@@ -135,11 +61,10 @@ QFile *QgsNetworkContentFetcherRegistry::localFile( const QString &filePathOrUrl
 
   if ( !QUrl::fromUserInput( filePathOrUrl ).isLocalFile() )
   {
-    QMutexLocker locker( &mMutex );
-    if ( mFileRegistry.contains( QUrl( path ) ) )
+    if ( mFileRegistry.contains( path ) )
     {
-      const QgsFetchedContent *content = mFileRegistry.value( QUrl( path ) );
-      if ( content->status() == QgsFetchedContent::Finished && !content->file() )
+      const QgsFetchedContent *content = mFileRegistry.value( path );
+      if ( content && content->status() == QgsFetchedContent::Finished && content->file() )
       {
         file = content->file();
       }
@@ -166,10 +91,9 @@ QString QgsNetworkContentFetcherRegistry::localPath( const QString &filePathOrUr
 
   if ( !QUrl::fromUserInput( filePathOrUrl ).isLocalFile() )
   {
-    QMutexLocker locker( &mMutex );
-    if ( mFileRegistry.contains( QUrl( path ) ) )
+    if ( mFileRegistry.contains( path ) )
     {
-      const QgsFetchedContent *content = mFileRegistry.value( QUrl( path ) );
+      const QgsFetchedContent *content = mFileRegistry.value( path );
       if ( content->status() == QgsFetchedContent::Finished && !content->filePath().isEmpty() )
       {
         path = content->filePath();
@@ -190,3 +114,75 @@ QString QgsNetworkContentFetcherRegistry::localPath( const QString &filePathOrUr
 
 
 
+
+void QgsFetchedContent::download( bool redownload )
+{
+
+  if ( redownload && status() == QgsFetchedContent::Downloading )
+  {
+    {
+      if ( mFetchingTask )
+        disconnect( mFetchingTask, &QgsNetworkContentFetcherTask::taskCompleted, this, &QgsFetchedContent::taskCompleted );
+    }
+    cancel();
+  }
+  if ( redownload ||
+       status() == QgsFetchedContent::NotStarted ||
+       status() == QgsFetchedContent::Failed )
+  {
+    mFetchingTask = new QgsNetworkContentFetcherTask( mUrl );
+    // use taskCompleted which is main thread rather than fetched signal in worker thread
+    connect( mFetchingTask, &QgsNetworkContentFetcherTask::taskCompleted, this, &QgsFetchedContent::taskCompleted );
+    QgsApplication::instance()->taskManager()->addTask( mFetchingTask );
+    mStatus = QgsFetchedContent::Downloading;
+  }
+
+}
+
+void QgsFetchedContent::cancel()
+{
+  if ( mFetchingTask && mFetchingTask->canCancel() )
+  {
+    mFetchingTask->cancel();
+  }
+  if ( mFile )
+  {
+    mFile->deleteLater();
+    mFilePath = QString();
+  }
+}
+
+
+void QgsFetchedContent::taskCompleted()
+{
+  if ( !mFetchingTask || !mFetchingTask->reply() )
+  {
+    // if no reply, it has been canceled
+    mStatus = QgsFetchedContent::Failed;
+    mError = QNetworkReply::OperationCanceledError;
+    mFilePath = QString();
+  }
+  else
+  {
+    QNetworkReply *reply = mFetchingTask->reply();
+    if ( reply->error() == QNetworkReply::NoError )
+    {
+      QTemporaryFile *tf = new QTemporaryFile( QStringLiteral( "XXXXXX" ) );
+      mFile = tf;
+      tf->open();
+      mFile->write( reply->readAll() );
+      // Qt docs notes that on some system if fileName is not called before close, file might get deleted
+      mFilePath = tf->fileName();
+      tf->close();
+      mStatus = QgsFetchedContent::Finished;
+    }
+    else
+    {
+      mStatus = QgsFetchedContent::Failed;
+      mError = reply->error();
+      mFilePath = QString();
+    }
+  }
+
+  emit fetched();
+}
diff --git a/src/core/qgsnetworkcontentfetcherregistry.h b/src/core/qgsnetworkcontentfetcherregistry.h
@@ -54,8 +54,8 @@ class CORE_EXPORT QgsFetchedContent : public QObject
     };
 
     //! Constructs a FetchedContent with pointer to the downloaded file and status of the download
-    explicit QgsFetchedContent( QTemporaryFile *file = nullptr, ContentStatus status = NotStarted )
-      : QObject(), mFile( file ), mStatus( status ) {}
+    explicit QgsFetchedContent( const QString &url, QTemporaryFile *file = nullptr, ContentStatus status = NotStarted )
+      : QObject(), mUrl( url ), mFile( file ), mStatus( status ) {}
 
     ~QgsFetchedContent()
     {
@@ -84,36 +84,27 @@ class CORE_EXPORT QgsFetchedContent : public QObject
      * \brief Start the download
      * \param redownload if set to true, it will restart any achieved or pending download.
      */
-    void download( bool redownload = false ) {emit downloadStarted( redownload );}
+    void download( bool redownload = false );
 
     /**
      * @brief Cancel the download operation
      */
-    void cancel() {emit cancelTriggered();}
+    void cancel();
 
   signals:
     //! Emitted when the file is fetched and accessible
     void fetched();
 
-    //! Emitted when the download actually starts
-    void downloadStarted( const bool redownload );
-
-    //! Emitted when download is canceled.
-    void cancelTriggered();
-
-    //! Emitted when the download is finished (although file not accessible yet)
+  private slots:
     void taskCompleted();
 
   private:
-    void emitFetched() {emit fetched();}
+    QString mUrl;
     QTemporaryFile *mFile = nullptr;
     QString mFilePath;
     QgsNetworkContentFetcherTask *mFetchingTask = nullptr;
     ContentStatus mStatus = NotStarted;
     QNetworkReply::NetworkError mError = QNetworkReply::NoError;
-
-    // allow modification of task and file from main class
-    friend class QgsNetworkContentFetcherRegistry;
 };
 
 /**
@@ -151,7 +142,7 @@ class CORE_EXPORT QgsNetworkContentFetcherRegistry : public QObject
      * \param fetchingMode defines if the download will start immediately or shall be manually triggered
      * \note If the download starts immediately, it will not redownload any already fetched or currently fetching file.
      */
-    const QgsFetchedContent *fetch( const QUrl &url, const FetchingMode fetchingMode = DownloadLater );
+    const QgsFetchedContent *fetch( const QString &url, const FetchingMode fetchingMode = DownloadLater );
 
 #ifndef SIP_RUN
 
@@ -169,10 +160,7 @@ class CORE_EXPORT QgsNetworkContentFetcherRegistry : public QObject
     QString localPath( const QString &filePathOrUrl );
 
   private:
-    QMap<QUrl, QgsFetchedContent *> mFileRegistry;
-
-    //! Mutex to prevent concurrent access to the class from multiple threads at once (may corrupt the entries otherwise).
-    mutable QMutex mMutex;
+    QMap<QString, QgsFetchedContent *> mFileRegistry;
 
 };
 
diff --git a/src/gui/qgsattributeform.cpp b/src/gui/qgsattributeform.cpp
@@ -1121,7 +1121,7 @@ void QgsAttributeForm::init()
     QgsDebugMsg( QString( "loading form: %1" ).arg( mLayer->editFormConfig().uiForm() ) );
     const QString path = mLayer->editFormConfig().uiForm();
     QFile *file = QgsApplication::instance()->networkContentFetcherRegistry()->localFile( path );
-    if ( file->isReadable() && file->open( QFile::ReadOnly ) )
+    if ( file && file->open( QFile::ReadOnly ) )
     {
       QUiLoader loader;
 
diff --git a/tests/src/python/test_qgsnetworkcontentfetcherregistry.py b/tests/src/python/test_qgsnetworkcontentfetcherregistry.py
@@ -22,7 +22,6 @@
 from qgis.testing import unittest, start_app
 from qgis.core import QgsNetworkContentFetcherRegistry, QgsFetchedContent, QgsApplication
 from utilities import unitTestDataPath
-from qgis.PyQt.QtCore import QUrl
 from qgis.PyQt.QtNetwork import QNetworkReply, QNetworkRequest
 import socketserver
 import threading
@@ -55,7 +54,7 @@ def __init__(self, methodName):
 
     def testFetchBadUrl(self):
         registry = QgsApplication.networkContentFetcherRegistry()
-        content = registry.fetch(QUrl('http://x'))
+        content = registry.fetch('http://x')
         self.loaded = False
 
         def check_reply():
@@ -72,7 +71,7 @@ def check_reply():
     def testFetchGoodUrl(self):
         url = 'http://localhost:' + str(self.port) + '/qgis_local_server/index.html'
         registry = QgsApplication.networkContentFetcherRegistry()
-        content = registry.fetch(QUrl(url))
+        content = registry.fetch(url)
         self.loaded = False
 
         def check_reply():
@@ -89,7 +88,7 @@ def check_reply():
         self.assertEqual(registry.localPath(url), content.filePath())
 
         # create new content with same URL
-        contentV2 = registry.fetch(QUrl(url))
+        contentV2 = registry.fetch(url)
         self.assertEqual(contentV2.status(), QgsFetchedContent.Finished)
 
     def testFetchReloadUrl(self):
@@ -99,7 +98,7 @@ def writeSimpleFile(content):
             self.file_content = content
 
         registry = QgsApplication.networkContentFetcherRegistry()
-        content = registry.fetch(QUrl('http://localhost:' + str(self.port) + '/qgis_local_server/simple_content.txt'))
+        content = registry.fetch('http://localhost:' + str(self.port) + '/qgis_local_server/simple_content.txt')
         self.loaded = False
         writeSimpleFile('my initial content')
 
@@ -136,7 +135,7 @@ def testLocalPath(self):
         self.assertEqual(registry.localPath('xxxx'), 'xxxx')
 
         # an existent but unfinished download should return an empty path
-        content = registry.fetch(QUrl('xxxx'))
+        content = registry.fetch('xxxx')
         self.assertEqual(registry.localPath('xxxx'), '')
 
 

Original file line number	Diff line number	Diff line change
`@@ -1121,7 +1121,7 @@ void QgsAttributeForm::init()`
`1121`	`1121`	`QgsDebugMsg( QString( "loading form: %1" ).arg( mLayer->editFormConfig().uiForm() ) );`
`1122`	`1122`	`const QString path = mLayer->editFormConfig().uiForm();`
`1123`	`1123`	`QFile *file = QgsApplication::instance()->networkContentFetcherRegistry()->localFile( path );`
`1124`		`- if ( file->isReadable() && file->open( QFile::ReadOnly ) )`
	`1124`	`+ if ( file && file->open( QFile::ReadOnly ) )`
`1125`	`1125`	`{`
`1126`	`1126`	`QUiLoader loader;`
`1127`	`1127`