AbstractProcessStep: De-virtualize finish() method

Provide a setDoneHook() setter instead. The hook is
introduced temporarily, as when all the subclasses
are transformed to use the task tree, the done hook
is going to be a part of the subclass' recipe.

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

src/plugins/android/androidbuildapkstep.cpp

@@ -484,6 +484,11 @@ AndroidBuildApkStep::AndroidBuildApkStep(BuildStepList *parent, Utils::Id id)
         if (format == OutputFormat::Stderr)
             stdError(string);
     });
+
+    setDoneHook([this](bool success) {
+        if (m_openPackageLocationForRun && success)
+            QTimer::singleShot(0, this, &AndroidBuildApkStep::showInGraphicalShell);
+    });
 }
 
 bool AndroidBuildApkStep::init()
@@ -632,13 +637,6 @@ QWidget *AndroidBuildApkStep::createConfigWidget()
     return new AndroidBuildApkWidget(this);
 }
 
-void AndroidBuildApkStep::finish(ProcessResult result)
-{
-    if (m_openPackageLocationForRun && isSuccess(result))
-        QTimer::singleShot(0, this, &AndroidBuildApkStep::showInGraphicalShell);
-    AbstractProcessStep::finish(result);
-}
-
 bool AndroidBuildApkStep::verifyKeystorePassword()
 {
     if (!m_keystorePath.exists()) {

src/plugins/android/androidbuildapkstep.h

@@ -63,7 +63,6 @@ private:
     bool init() override;
     void setupOutputFormatter(Utils::OutputFormatter *formatter) override;
     QWidget *createConfigWidget() override;
-    void finish(Utils::ProcessResult result) override;
     bool verifyKeystorePassword();
     bool verifyCertificatePassword();
 

src/plugins/cmakeprojectmanager/cmakebuildstep.cpp

@@ -253,6 +253,11 @@ CMakeBuildStep::CMakeBuildStep(BuildStepList *bsl, Id id) :
 
     connect(target(), &Target::activeRunConfigurationChanged,
             this, &CMakeBuildStep::updateBuildTargetsModel);
+
+    setDoneHook([this](bool) {
+        updateDeploymentData();
+        emit progress(100, {});
+    });
 }
 
 QVariantMap CMakeBuildStep::toMap() const
@@ -795,14 +800,6 @@ void CMakeBuildStep::updateDeploymentData()
     buildSystem()->setDeploymentData(deploymentData);
 }
 
-void CMakeBuildStep::finish(ProcessResult result)
-{
-    updateDeploymentData();
-
-    emit progress(100, {});
-    AbstractProcessStep::finish(result);
-}
-
 // CMakeBuildStepFactory
 
 CMakeBuildStepFactory::CMakeBuildStepFactory()

src/plugins/cmakeprojectmanager/cmakebuildstep.h

@@ -77,7 +77,6 @@ signals:
 private:
     Utils::CommandLine cmakeCommand() const;
 
-    void finish(Utils::ProcessResult result) override;
     bool fromMap(const QVariantMap &map) override;
 
     bool init() override;

src/plugins/cmakeprojectmanager/cmakeinstallstep.cpp

@@ -38,13 +38,12 @@ public:
         cmakeArguments.setDisplayStyle(StringAspect::LineEditDisplay);
 
         setCommandLineProvider([this] { return cmakeCommand(); });
+        setDoneHook([this](bool) { emit progress(100, {}); });
     }
 
 private:
     CommandLine cmakeCommand() const;
 
-    void finish(ProcessResult result) override;
-
     void setupOutputFormatter(OutputFormatter *formatter) override;
     QWidget *createConfigWidget() override;
 
@@ -83,12 +82,6 @@ CommandLine CMakeInstallStep::cmakeCommand() const
     return cmd;
 }
 
-void CMakeInstallStep::finish(ProcessResult result)
-{
-    emit progress(100, {});
-    AbstractProcessStep::finish(result);
-}
-
 QWidget *CMakeInstallStep::createConfigWidget()
 {
     auto updateDetails = [this] {

src/plugins/projectexplorer/abstractprocessstep.cpp

@@ -82,6 +82,7 @@ public:
     std::function<CommandLine()> m_commandLineProvider;
     std::function<FilePath()> m_workingDirectoryProvider;
     std::function<void(Environment &)> m_environmentModifier;
+    std::function<void(bool)> m_doneHook; // TODO: Remove me when all subclasses moved to Tasking
     bool m_ignoreReturnValue = false;
     bool m_lowPriority = false;
     std::unique_ptr<QTextDecoder> stdoutStream;
@@ -131,6 +132,11 @@ void AbstractProcessStep::setUseEnglishOutput()
     d->m_environmentModifier = [](Environment &env) { env.setupEnglishOutput(); };
 }
 
+void AbstractProcessStep::setDoneHook(const std::function<void(bool)> &doneHook)
+{
+    d->m_doneHook = doneHook;
+}
+
 void AbstractProcessStep::setCommandLineProvider(const std::function<CommandLine()> &provider)
 {
     d->m_commandLineProvider = provider;
@@ -350,15 +356,13 @@ void AbstractProcessStep::setDisplayedParameters(ProcessParameters *params)
     d->m_displayedParams = params;
 }
 
-bool AbstractProcessStep::isSuccess(ProcessResult result) const
-{
-    return result == ProcessResult::FinishedWithSuccess
-            || (result == ProcessResult::FinishedWithError && d->m_ignoreReturnValue);
-}
-
 void AbstractProcessStep::finish(ProcessResult result)
 {
-    emit finished(isSuccess(result));
+    const bool success = result == ProcessResult::FinishedWithSuccess
+                         || (result == ProcessResult::FinishedWithError && d->m_ignoreReturnValue);
+    if (d->m_doneHook)
+        d->m_doneHook(success);
+    emit finished(success);
 }
 
 } // namespace ProjectExplorer

src/plugins/projectexplorer/abstractprocessstep.h

@@ -33,6 +33,7 @@ protected:
     bool setupProcessParameters(ProcessParameters *params) const;
     bool ignoreReturnValue() const;
     void setIgnoreReturnValue(bool b);
+    void setDoneHook(const std::function<void(bool)> &doneHook);
     void setCommandLineProvider(const std::function<Utils::CommandLine()> &provider);
     void setWorkingDirectoryProvider(const std::function<Utils::FilePath()> &provider);
     void setEnvironmentModifier(const std::function<void(Utils::Environment &)> &modifier);
@@ -46,9 +47,6 @@ protected:
     void doCancel() override;
     void setLowPriority();
     void setDisplayedParameters(ProcessParameters *params);
-    bool isSuccess(Utils::ProcessResult result) const;
-
-    virtual void finish(Utils::ProcessResult result);
 
     bool checkWorkingDirectory();
     void setupProcess(Utils::Process *process);
@@ -57,6 +55,7 @@ protected:
 
 private:
     void setupStreams();
+    void finish(Utils::ProcessResult result);
 
     class Private;
     Private *d;

src/plugins/qmakeprojectmanager/qmakemakestep.cpp

@@ -40,7 +40,6 @@ public:
     QmakeMakeStep(BuildStepList *bsl, Id id);
 
 private:
-    void finish(ProcessResult result) override;
     bool init() override;
     void setupOutputFormatter(OutputFormatter *formatter) override;
     void doRun() override;
@@ -60,6 +59,15 @@ QmakeMakeStep::QmakeMakeStep(BuildStepList *bsl, Id id)
         setUserArguments("clean");
     }
     supportDisablingForSubdirs();
+
+    setDoneHook([this](bool success) {
+        if (!success && !isCanceled() && m_unalignedBuildDir
+            && settings().warnAgainstUnalignedBuildDir()) {
+            const QString msg = Tr::tr("The build directory is not at the same level as the source "
+                                       "directory, which could be the reason for the build failure.");
+            emit addTask(BuildSystemTask(Task::Warning, msg));
+        }
+    });
 }
 
 bool QmakeMakeStep::init()
@@ -219,17 +227,6 @@ void QmakeMakeStep::doRun()
     AbstractProcessStep::doRun();
 }
 
-void QmakeMakeStep::finish(ProcessResult result)
-{
-    if (!isSuccess(result) && !isCanceled() && m_unalignedBuildDir
-            && settings().warnAgainstUnalignedBuildDir()) {
-        const QString msg = Tr::tr("The build directory is not at the same level as the source "
-                               "directory, which could be the reason for the build failure.");
-        emit addTask(BuildSystemTask(Task::Warning, msg));
-    }
-    MakeStep::finish(result);
-}
-
 QStringList QmakeMakeStep::displayArguments() const
 {
     const auto bc = static_cast<QmakeBuildConfiguration *>(buildConfiguration());

src/plugins/remotelinux/makeinstallstep.cpp

@@ -41,7 +41,6 @@ private:
     bool fromMap(const QVariantMap &map) override;
     QWidget *createConfigWidget() override;
     bool init() override;
-    void finish(ProcessResult result) override;
     bool isJobCountSupported() const override { return false; }
 
     void updateCommandFromAspect();
@@ -132,6 +131,35 @@ MakeInstallStep::MakeInstallStep(BuildStepList *parent, Id id) : MakeStep(parent
         if (format == OutputFormat::Stderr && string.contains("target 'install'"))
             m_noInstallTarget = true;
     });
+
+    setDoneHook([this](bool success) {
+        if (success) {
+            const FilePath rootDir = makeCommand().withNewPath(m_installRoot().path()); // FIXME: Needed?
+
+            m_deploymentData = DeploymentData();
+            m_deploymentData.setLocalInstallRoot(rootDir);
+
+            const int startPos = rootDir.path().length();
+
+            const auto appFileNames = transform<QSet<QString>>(buildSystem()->applicationTargets(),
+                [](const BuildTargetInfo &appTarget) { return appTarget.targetFilePath.fileName(); });
+
+            auto handleFile = [this, &appFileNames, startPos](const FilePath &filePath) {
+                const DeployableFile::Type type = appFileNames.contains(filePath.fileName())
+                    ? DeployableFile::TypeExecutable : DeployableFile::TypeNormal;
+                const QString targetDir = filePath.parentDir().path().mid(startPos);
+                m_deploymentData.addFile(filePath, targetDir, type);
+                return IterationPolicy::Continue;
+            };
+            rootDir.iterateDirectory(
+                handleFile, {{}, QDir::Files | QDir::Hidden, QDirIterator::Subdirectories});
+
+            buildSystem()->setDeploymentData(m_deploymentData);
+        } else if (m_noInstallTarget && m_isCmakeProject) {
+            emit addTask(DeploymentTask(Task::Warning, Tr::tr("You need to add an install statement "
+                "to your CMakeLists.txt file for deployment to work.")));
+        }
+    });
 }
 
 QWidget *MakeInstallStep::createConfigWidget()
@@ -188,38 +216,6 @@ bool MakeInstallStep::init()
     return true;
 }
 
-void MakeInstallStep::finish(ProcessResult result)
-{
-    if (isSuccess(result)) {
-        const FilePath rootDir = makeCommand().withNewPath(m_installRoot().path()); // FIXME: Needed?
-
-        m_deploymentData = DeploymentData();
-        m_deploymentData.setLocalInstallRoot(rootDir);
-
-        const int startPos = rootDir.path().length();
-
-        const auto appFileNames = transform<QSet<QString>>(buildSystem()->applicationTargets(),
-            [](const BuildTargetInfo &appTarget) { return appTarget.targetFilePath.fileName(); });
-
-        auto handleFile = [this, &appFileNames, startPos](const FilePath &filePath) {
-            const DeployableFile::Type type = appFileNames.contains(filePath.fileName())
-                ? DeployableFile::TypeExecutable
-                : DeployableFile::TypeNormal;
-            const QString targetDir = filePath.parentDir().path().mid(startPos);
-            m_deploymentData.addFile(filePath, targetDir, type);
-            return IterationPolicy::Continue;
-        };
-        rootDir.iterateDirectory(handleFile,
-                                 {{}, QDir::Files | QDir::Hidden, QDirIterator::Subdirectories});
-
-        buildSystem()->setDeploymentData(m_deploymentData);
-    } else if (m_noInstallTarget && m_isCmakeProject) {
-        emit addTask(DeploymentTask(Task::Warning, Tr::tr("You need to add an install statement "
-                   "to your CMakeLists.txt file for deployment to work.")));
-    }
-    MakeStep::finish(result);
-}
-
 void MakeInstallStep::updateCommandFromAspect()
 {
     if (m_customCommand.isChecked())