AbstractRemoteLinuxDeployStep: Refactor tree error handling

Make it behave like AbstractProcessStep.
Move success / failure logging into the recipe.
Make starting the task tree look the same in both classes.
That's a preparation step before moving running task tree
into the base BuildStep class.

Task-number: QTCREATORBUG-29168
Change-Id: I2bf3e2476d3942a01efc3b06778410dea40eef5e
Reviewed-by: hjk <hjk@qt.io>

src/plugins/projectexplorer/abstractprocessstep.cpp

@@ -170,17 +170,19 @@ void AbstractProcessStep::setupOutputFormatter(OutputFormatter *formatter)
 
 void AbstractProcessStep::doRun()
 {
+    QTC_ASSERT(!d->m_taskTree, return);
+
     d->m_taskTree.reset(new TaskTree({runRecipe()}));
     connect(d->m_taskTree.get(), &TaskTree::progressValueChanged, this, [this](int value) {
         emit progress(qRound(double(value) * 100 / std::max(d->m_taskTree->progressMaximum(), 1)), {});
     });
     connect(d->m_taskTree.get(), &TaskTree::done, this, [this] {
-        emit finished(true);
         d->m_taskTree.release()->deleteLater();
+        emit finished(true);
     });
     connect(d->m_taskTree.get(), &TaskTree::errorOccurred, this, [this] {
-        emit finished(false);
         d->m_taskTree.release()->deleteLater();
+        emit finished(false);
     });
     d->m_taskTree->start();
 }
@@ -269,13 +271,13 @@ void AbstractProcessStep::setLowPriority()
 
 void AbstractProcessStep::doCancel()
 {
-    const QString message = Tr::tr("The build step was ended forcefully.");
+    if (!d->m_taskTree)
-    if (d->m_taskTree) {
+        return;
+
     d->m_taskTree.reset();
-        emit addOutput(message, OutputFormat::ErrorMessage);
+    emit addOutput(Tr::tr("The build step was ended forcefully."), OutputFormat::ErrorMessage);
     emit finished(false);
 }
-}
 
 ProcessParameters *AbstractProcessStep::processParameters()
 {

src/plugins/remotelinux/abstractremotelinuxdeploystep.cpp

@@ -26,7 +26,6 @@ namespace Internal {
 class AbstractRemoteLinuxDeployStepPrivate
 {
 public:
-    bool hasError;
     std::function<expected_str<void>()> internalInit;
 
     DeploymentTimeInfo deployTimes;
@@ -110,33 +109,31 @@ bool AbstractRemoteLinuxDeployStep::init()
 
 void AbstractRemoteLinuxDeployStep::doRun()
 {
-    d->hasError = false;
-
     QTC_ASSERT(!d->m_taskTree, return);
 
     d->m_taskTree.reset(new TaskTree({runRecipe()}));
-    const auto endHandler = [this] {
+    const auto onDone = [this] {
         d->m_taskTree.release()->deleteLater();
-        handleFinished();
+        emit finished(true);
     };
-    connect(d->m_taskTree.get(), &TaskTree::done, this, endHandler);
+    const auto onError = [this] {
-    connect(d->m_taskTree.get(), &TaskTree::errorOccurred, this, endHandler);
+        d->m_taskTree.release()->deleteLater();
+        emit finished(false);
+    };
+    connect(d->m_taskTree.get(), &TaskTree::done, this, onDone);
+    connect(d->m_taskTree.get(), &TaskTree::errorOccurred, this, onError);
     d->m_taskTree->start();
 }
 
 void AbstractRemoteLinuxDeployStep::doCancel()
 {
-    if (d->hasError)
-        return;
-
-    emit addOutput(Tr::tr("User requests deployment to stop; cleaning up."),
-                   OutputFormat::NormalMessage);
-    d->hasError = true;
-
     if (!d->m_taskTree)
         return;
+
     d->m_taskTree.reset();
-    handleFinished();
+    emit addOutput(Tr::tr("User requests deployment to stop; cleaning up."),
+                   OutputFormat::NormalMessage);
+    emit finished(false);
 }
 
 void AbstractRemoteLinuxDeployStep::addProgressMessage(const QString &message)
@@ -148,7 +145,6 @@ void AbstractRemoteLinuxDeployStep::addErrorMessage(const QString &message)
 {
     emit addOutput(message, OutputFormat::ErrorMessage);
     emit addTask(DeploymentTask(Task::Error, message), 1); // TODO correct?
-    d->hasError = true;
 }
 
 void AbstractRemoteLinuxDeployStep::addWarningMessage(const QString &message)
@@ -157,16 +153,6 @@ void AbstractRemoteLinuxDeployStep::addWarningMessage(const QString &message)
     emit addTask(DeploymentTask(Task::Warning, message), 1); // TODO correct?
 }
 
-void AbstractRemoteLinuxDeployStep::handleFinished()
-{
-    if (d->hasError)
-        emit addOutput(Tr::tr("Deploy step failed."), OutputFormat::ErrorMessage);
-    else
-        emit addOutput(Tr::tr("Deploy step finished."), OutputFormat::NormalMessage);
-
-    emit finished(!d->hasError);
-}
-
 void AbstractRemoteLinuxDeployStep::handleStdOutData(const QString &data)
 {
     emit addOutput(data, OutputFormat::Stdout, DontAppendNewline);
@@ -188,17 +174,26 @@ GroupItem AbstractRemoteLinuxDeployStep::runRecipe()
         const auto canDeploy = isDeploymentPossible();
         if (!canDeploy) {
             addErrorMessage(canDeploy.error());
-            handleFinished();
             return SetupResult::StopWithError;
         }
         if (!isDeploymentNecessary()) {
             addProgressMessage(Tr::tr("No deployment action necessary. Skipping."));
-            handleFinished();
             return SetupResult::StopWithDone;
         }
         return SetupResult::Continue;
     };
-    return Group { onGroupSetup(onSetup), deployRecipe() };
+    const auto onDone = [this] {
+        emit addOutput(Tr::tr("Deploy step finished."), OutputFormat::NormalMessage);
+    };
+    const auto onError = [this] {
+        emit addOutput(Tr::tr("Deploy step failed."), OutputFormat::ErrorMessage);
+    };
+    return Group {
+        onGroupSetup(onSetup),
+        deployRecipe(),
+        onGroupDone(onDone),
+        onGroupError(onError)
+    };
 }
 
 } // namespace RemoteLinux