Improved memory managment of algorithm dialogs

Ensure that dialogs are always correctly deleted when appropriate. Also, if an algorithm is running in a background task and hits an error, we automatically re-show the algorithm dialog and the associated log for debugging. Fixes #16858
qgis · Jan 16, 2018 · 87c1986 · 87c1986
1 parent bc9d739
commit 87c1986
Show file tree

Hide file tree

Showing 9 changed files with 96 additions and 33 deletions.
diff --git a/python/gui/processing/qgsprocessingalgorithmdialogbase.sip b/python/gui/processing/qgsprocessingalgorithmdialogbase.sip
@@ -32,6 +32,7 @@ class QgsProcessingAlgorithmDialogBase : QDialog
 %Docstring
 Constructor for QgsProcessingAlgorithmDialogBase.
 %End
+    ~QgsProcessingAlgorithmDialogBase();
 
     void setAlgorithm( QgsProcessingAlgorithm *algorithm );
 %Docstring
@@ -143,6 +144,9 @@ from this dialog.
 
   protected:
 
+    virtual void closeEvent( QCloseEvent *e );
+
+
     QPushButton *runButton();
 %Docstring
 Returns the dialog's run button.
@@ -204,6 +208,12 @@ Returns the dialog's message bar.
     void hideShortHelp();
 %Docstring
 Hides the short help panel.
+%End
+
+    void setCurrentTask( QgsProcessingAlgRunnerTask *task /Transfer/ );
+%Docstring
+Sets the current ``task`` running in the dialog. The task will automatically be started
+by the dialog. Ownership of ``task`` is transferred to the dialog.
 %End
 
   protected slots:

diff --git a/python/plugins/processing/ProcessingPlugin.py b/python/plugins/processing/ProcessingPlugin.py
@@ -87,7 +87,6 @@ def runAlg(file):
 
         alg.setProvider(QgsApplication.processingRegistry().providerById('model'))
         dlg = AlgorithmDialog(alg)
-        dlg.setAttribute(Qt.WA_DeleteOnClose)
         dlg.show()
         return True
 

diff --git a/python/plugins/processing/gui/AlgorithmDialog.py b/python/plugins/processing/gui/AlgorithmDialog.py
@@ -64,15 +64,13 @@ class AlgorithmDialog(QgsProcessingAlgorithmDialogBase):
     def __init__(self, alg):
         super().__init__()
         self.feedback_dialog = None
-        self.task = None
 
         self.setAlgorithm(alg)
         self.setMainWidget(self.getParametersPanel(alg, self))
 
         self.runAsBatchButton = QPushButton(QCoreApplication.translate("AlgorithmDialog", "Run as Batch Process…"))
         self.runAsBatchButton.clicked.connect(self.runAsBatch)
         self.buttonBox().addButton(self.runAsBatchButton, QDialogButtonBox.ResetRole) # reset role to ensure left alignment
-        QgsApplication.taskManager().taskTriggered.connect(self.taskTriggered)
 
     def getParametersPanel(self, alg, parent):
         return ParametersPanel(parent, alg)
@@ -235,7 +233,6 @@ def accept(self):
                 self.cancelButton().setEnabled(self.algorithm().flags() & QgsProcessingAlgorithm.FlagCanCancel)
 
                 def on_complete(ok, results):
-                    self.task = None
                     if ok:
                         feedback.pushInfo(self.tr('Execution completed in {0:0.2f} seconds'.format(time.time() - start_time)))
                         feedback.pushInfo(self.tr('Results:'))
@@ -258,9 +255,9 @@ def on_complete(ok, results):
                     # Make sure the Log tab is visible before executing the algorithm
                     self.showLog()
 
-                    self.task = QgsProcessingAlgRunnerTask(self.algorithm(), parameters, context, feedback)
-                    self.task.executed.connect(on_complete)
-                    QgsApplication.taskManager().addTask(self.task)
+                    task = QgsProcessingAlgRunnerTask(self.algorithm(), parameters, context, feedback)
+                    task.executed.connect(on_complete)
+                    self.setCurrentTask(task)
                 else:
                     self.feedback_dialog = self.createProgressDialog()
                     self.feedback_dialog.show()
@@ -280,13 +277,6 @@ def on_complete(ok, results):
             self.messageBar().pushMessage("", self.tr("Wrong or missing parameter value: {0}").format(e.parameter.description()),
                                           level=QgsMessageBar.WARNING, duration=5)
 
-    def taskTriggered(self, task):
-        if task == self.task:
-            self.show()
-            self.raise_()
-            self.setWindowState(self.windowState() & ~Qt.WindowMinimized | Qt.WindowActive)
-            self.activateWindow()
-
     def finish(self, successful, result, context, feedback):
         keepOpen = not successful or ProcessingConfig.getSetting(ProcessingConfig.KEEP_DIALOG_OPEN)
 

diff --git a/python/plugins/processing/gui/ProcessingToolbox.py b/python/plugins/processing/gui/ProcessingToolbox.py
@@ -257,9 +257,6 @@ def executeAlgorithmAsBatchProcess(self):
                 dlg = BatchAlgorithmDialog(alg)
                 dlg.show()
                 dlg.exec_()
-                # have to manually delete the dialog - otherwise it's owned by the
-                # iface mainWindow and never deleted
-                dlg.deleteLater()
 
     def executeAlgorithm(self):
         item = self.algorithmTree.currentItem()
@@ -298,9 +295,6 @@ def executeAlgorithm(self):
                         ProcessingConfig.SHOW_RECENT_ALGORITHMS)
                     if showRecent:
                         self.addRecentAlgorithms(True)
-                # have to manually delete the dialog - otherwise it's owned by the
-                # iface mainWindow and never deleted
-                # dlg.deleteLater()
             else:
                 feedback = MessageBarProgress()
                 context = dataobjects.createContext(feedback)

diff --git a/python/plugins/processing/gui/ScriptEditorDialog.py b/python/plugins/processing/gui/ScriptEditorDialog.py
@@ -284,11 +284,6 @@ def runAlgorithm(self):
         prevMapTool = canvas.mapTool()
 
         dlg.show()
-        dlg.exec_()
-
-        # have to manually delete the dialog - otherwise it's owned by the
-        # iface mainWindow and never deleted
-        dlg.deleteLater()
 
         if canvas.mapTool() != prevMapTool:
             try:

diff --git a/python/plugins/processing/gui/menus.py b/python/plugins/processing/gui/menus.py
@@ -211,9 +211,6 @@ def _executeAlgorithm(alg):
         prevMapTool = canvas.mapTool()
         dlg.show()
         dlg.exec_()
-        # have to manually delete the dialog - otherwise it's owned by the
-        # iface mainWindow and never deleted
-        del dlg
         if canvas.mapTool() != prevMapTool:
             try:
                 canvas.mapTool().reset()

diff --git a/python/plugins/processing/modeler/ModelerDialog.py b/python/plugins/processing/modeler/ModelerDialog.py
@@ -323,9 +323,6 @@ def runModel(self):
 
         dlg = AlgorithmDialog(self.model)
         dlg.exec_()
-        # have to manually delete the dialog - otherwise it's owned by the
-        # iface mainWindow and never deleted
-        dlg.deleteLater()
 
     def save(self):
         self.saveModel(False)

diff --git a/src/gui/processing/qgsprocessingalgorithmdialogbase.cpp b/src/gui/processing/qgsprocessingalgorithmdialogbase.cpp
@@ -20,6 +20,8 @@
 #include "qgsgui.h"
 #include "processing/qgsprocessingalgorithm.h"
 #include "processing/qgsprocessingprovider.h"
+#include "qgstaskmanager.h"
+#include "processing/qgsprocessingalgrunnertask.h"
 #include <QToolButton>
 #include <QDesktopServices>
 #include <QScrollBar>
@@ -92,7 +94,7 @@ QgsProcessingAlgorithmDialogBase::QgsProcessingAlgorithmDialogBase( QWidget *par
   mSplitterState = splitter->saveState();
   splitterChanged( 0, 0 );
 
-  connect( mButtonBox, &QDialogButtonBox::rejected, this, &QgsProcessingAlgorithmDialogBase::reject );
+  connect( mButtonBox, &QDialogButtonBox::rejected, this, &QgsProcessingAlgorithmDialogBase::closeClicked );
   connect( mButtonBox, &QDialogButtonBox::accepted, this, &QgsProcessingAlgorithmDialogBase::accept );
 
   // Rename OK button to Run
@@ -109,6 +111,8 @@ QgsProcessingAlgorithmDialogBase::QgsProcessingAlgorithmDialogBase( QWidget *par
   mMessageBar = new QgsMessageBar();
   mMessageBar->setSizePolicy( QSizePolicy::Minimum, QSizePolicy::Fixed );
   verticalLayout->insertWidget( 0,  mMessageBar );
+
+  connect( QgsApplication::taskManager(), &QgsTaskManager::taskTriggered, this, &QgsProcessingAlgorithmDialogBase::taskTriggered );
 }
 
 void QgsProcessingAlgorithmDialogBase::setAlgorithm( QgsProcessingAlgorithm *algorithm )
@@ -271,6 +275,47 @@ void QgsProcessingAlgorithmDialogBase::linkClicked( const QUrl &url )
   QDesktopServices::openUrl( url.toString() );
 }
 
+void QgsProcessingAlgorithmDialogBase::algExecuted( bool successful, const QVariantMap & )
+{
+  mAlgorithmTask = nullptr;
+
+  if ( !successful )
+  {
+    // show dialog to display errors
+    show();
+    raise();
+    setWindowState( ( windowState() & ~Qt::WindowMinimized ) | Qt::WindowActive );
+    activateWindow();
+    showLog();
+  }
+  else
+  {
+    // delete dialog if closed
+    if ( !isVisible() )
+    {
+      deleteLater();
+    }
+  }
+}
+
+void QgsProcessingAlgorithmDialogBase::taskTriggered( QgsTask *task )
+{
+  if ( task == mAlgorithmTask )
+  {
+    show();
+    raise();
+    setWindowState( ( windowState() & ~Qt::WindowMinimized ) | Qt::WindowActive );
+    activateWindow();
+    showLog();
+  }
+}
+
+void QgsProcessingAlgorithmDialogBase::closeClicked()
+{
+  reject();
+  close();
+}
+
 void QgsProcessingAlgorithmDialogBase::reportError( const QString &error )
 {
   setInfo( error, true );
@@ -323,6 +368,19 @@ QDialog *QgsProcessingAlgorithmDialogBase::createProgressDialog()
   return dialog;
 }
 
+void QgsProcessingAlgorithmDialogBase::closeEvent( QCloseEvent *e )
+{
+  QDialog::closeEvent( e );
+
+  if ( !mAlgorithmTask )
+  {
+    // when running a background task, the dialog is kept around and deleted only when the task
+    // completes. But if not running a task, we auto cleanup (later - gotta give callers a chance
+    // to retrieve results and execution status).
+    deleteLater();
+  }
+}
+
 void QgsProcessingAlgorithmDialogBase::setPercentage( double percent )
 {
   // delay setting maximum progress value until we know algorithm reports progress
@@ -398,6 +456,13 @@ void QgsProcessingAlgorithmDialogBase::hideShortHelp()
   textShortHelp->setVisible( false );
 }
 
+void QgsProcessingAlgorithmDialogBase::setCurrentTask( QgsProcessingAlgRunnerTask *task )
+{
+  mAlgorithmTask = task;
+  connect( mAlgorithmTask, &QgsProcessingAlgRunnerTask::executed, this, &QgsProcessingAlgorithmDialogBase::algExecuted );
+  QgsApplication::taskManager()->addTask( mAlgorithmTask );
+}
+
 void QgsProcessingAlgorithmDialogBase::setInfo( const QString &message, bool isError, bool escapeHtml )
 {
   if ( isError )
@@ -417,8 +482,8 @@ void QgsProcessingAlgorithmDialogBase::setInfo( const QString &message, bool isE
 QgsProcessingAlgorithmProgressDialog::QgsProcessingAlgorithmProgressDialog( QWidget *parent )
   : QDialog( parent )
 {
-  setWindowFlags( Qt::Dialog ); // hide close button
   setupUi( this );
+  QgsGui::enableAutoGeometryRestore( this );
 }
 
 QProgressBar *QgsProcessingAlgorithmProgressDialog::progressBar()

diff --git a/src/gui/processing/qgsprocessingalgorithmdialogbase.h b/src/gui/processing/qgsprocessingalgorithmdialogbase.h
@@ -29,6 +29,8 @@ class QgsProcessingAlgorithm;
 class QToolButton;
 class QgsProcessingAlgorithmDialogBase;
 class QgsMessageBar;
+class QgsProcessingAlgRunnerTask;
+class QgsTask;
 
 #ifndef SIP_RUN
 
@@ -87,6 +89,7 @@ class GUI_EXPORT QgsProcessingAlgorithmDialogBase : public QDialog, private Ui::
      * Constructor for QgsProcessingAlgorithmDialogBase.
      */
     QgsProcessingAlgorithmDialogBase( QWidget *parent = nullptr, Qt::WindowFlags flags = nullptr );
+    ~QgsProcessingAlgorithmDialogBase();
 
     /**
      * Sets the \a algorithm to run in the dialog.
@@ -189,6 +192,8 @@ class GUI_EXPORT QgsProcessingAlgorithmDialogBase : public QDialog, private Ui::
 
   protected:
 
+    void closeEvent( QCloseEvent *e ) override;
+
     /**
      * Returns the dialog's run button.
      */
@@ -248,6 +253,12 @@ class GUI_EXPORT QgsProcessingAlgorithmDialogBase : public QDialog, private Ui::
      */
     void hideShortHelp();
 
+    /**
+     * Sets the current \a task running in the dialog. The task will automatically be started
+     * by the dialog. Ownership of \a task is transferred to the dialog.
+     */
+    void setCurrentTask( QgsProcessingAlgRunnerTask *task SIP_TRANSFER );
+
   protected slots:
 
     /**
@@ -262,6 +273,9 @@ class GUI_EXPORT QgsProcessingAlgorithmDialogBase : public QDialog, private Ui::
 
     void splitterChanged( int pos, int index );
     void linkClicked( const QUrl &url );
+    void algExecuted( bool successful, const QVariantMap &results );
+    void taskTriggered( QgsTask *task );
+    void closeClicked();
 
   private:
 
@@ -275,6 +289,8 @@ class GUI_EXPORT QgsProcessingAlgorithmDialogBase : public QDialog, private Ui::
     QVariantMap mResults;
     QWidget *mMainWidget = nullptr;
     QgsProcessingAlgorithm *mAlgorithm = nullptr;
+    QgsProcessingAlgRunnerTask *mAlgorithmTask = nullptr;
+
     bool mHelpCollapsed = false;
 
     QString formatHelp( QgsProcessingAlgorithm *algorithm );