ProjectExplorer: Rework RunConfiguration::isConfigured()

The old code had a number of problems:
    - There was one function isConfigured() to report whether the
      run config has issues, and a second one, ensureConfigured(),
      needed to be called to retrieve the details. At least one subclass
      implementor forgot to re-implement the first one, so the second
      one was never called.
    - The ensureConfigured() function could show a dialog and thereby
      delay execution of the run configuration, leading to additional
      state and a more complicated execution logic. Also, the dialog
      duplicated the run configuration UI.

We now have only one function returning a list of Task objects. If the
list is not empty, we present them to the user in a non-blocking way and
abort the execution.

Change-Id: I5f2a8126a2c1bd2ca51345b9e37b979bfc0c0b98
Reviewed-by: hjk <hjk@qt.io>

src/plugins/baremetal/baremetalcustomrunconfiguration.cpp

@@ -57,22 +57,14 @@ BareMetalCustomRunConfiguration::BareMetalCustomRunConfiguration(Target *target,
 
 const char *BareMetalCustomRunConfiguration::Id = "BareMetal";
 
-bool BareMetalCustomRunConfiguration::isConfigured() const
+Tasks BareMetalCustomRunConfiguration::checkForIssues() const
 {
-    return !aspect<ExecutableAspect>()->executable().isEmpty();
-}
-
-RunConfiguration::ConfigurationState
-BareMetalCustomRunConfiguration::ensureConfigured(QString *errorMessage)
-{
-    if (!isConfigured()) {
-        if (errorMessage) {
-            *errorMessage = tr("The remote executable must be set "
-                               "in order to run a custom remote run configuration.");
-        }
-        return UnConfigured;
+    Tasks tasks;
+    if (aspect<ExecutableAspect>()->executable().isEmpty()) {
+        tasks << createConfigurationIssue(tr("The remote executable must be set in order to run "
+                                             "a custom remote run configuration."));
     }
-    return Configured;
+    return tasks;
 }
 
 // BareMetalCustomRunConfigurationFactory

src/plugins/baremetal/baremetalcustomrunconfiguration.h

@@ -42,8 +42,7 @@ public:
 
 public:
     static const char *Id;
-    bool isConfigured() const final;
-    ConfigurationState ensureConfigured(QString *errorMessage) final;
+    ProjectExplorer::Tasks checkForIssues() const final;
 };
 
 // BareMetalCustomRunConfigurationFactory

src/plugins/boot2qt/qdbrunconfiguration.cpp

@@ -100,17 +100,14 @@ QdbRunConfiguration::QdbRunConfiguration(Target *target, Core::Id id)
     setDefaultDisplayName(tr("Run on Boot2Qt Device"));
 }
 
-ProjectExplorer::RunConfiguration::ConfigurationState QdbRunConfiguration::ensureConfigured(QString *errorMessage)
+Tasks QdbRunConfiguration::checkForIssues() const
 {
-    QString remoteExecutable = aspect<ExecutableAspect>()->executable().toString();
-    if (remoteExecutable.isEmpty()) {
-        if (errorMessage) {
-            *errorMessage = tr("The remote executable must be set "
-                               "in order to run on a Boot2Qt device.");
-        }
-        return UnConfigured;
+    Tasks tasks;
+    if (aspect<ExecutableAspect>()->executable().toString().isEmpty()) {
+        tasks << createConfigurationIssue(tr("The remote executable must be set "
+                                             "in order to run on a Boot2Qt device."));
     }
-    return Configured;
+    return tasks;
 }
 
 QString QdbRunConfiguration::defaultDisplayName() const

src/plugins/boot2qt/qdbrunconfiguration.h

@@ -45,9 +45,9 @@ class QdbRunConfiguration : public ProjectExplorer::RunConfiguration
 public:
     QdbRunConfiguration(ProjectExplorer::Target *target, Core::Id id);
 
-    ConfigurationState ensureConfigured(QString *errorMessage) override;
-
 private:
+    ProjectExplorer::Tasks checkForIssues() const override;
+
     QString defaultDisplayName() const;
 };
 

src/plugins/projectexplorer/customexecutablerunconfiguration.cpp

@@ -57,120 +57,6 @@ namespace ProjectExplorer {
 
 const char CUSTOM_EXECUTABLE_ID[] = "ProjectExplorer.CustomExecutableRunConfiguration";
 
-// Dialog prompting the user to complete the configuration.
-
-static void copyAspect(ProjectConfigurationAspect *source, ProjectConfigurationAspect *target)
-{
-    QVariantMap data;
-    source->toMap(data);
-    target->fromMap(data);
-}
-
-class CustomExecutableDialog : public QDialog
-{
-    Q_DECLARE_TR_FUNCTIONS(CustomExecutableDialog)
-public:
-    explicit CustomExecutableDialog(RunConfiguration *rc);
-
-    void accept() override;
-    bool event(QEvent *event) override;
-
-private:
-    void changed() {
-        bool isValid = !m_executableChooser->rawPath().isEmpty();
-        m_dialogButtonBox->button(QDialogButtonBox::Ok)->setEnabled(isValid);
-    }
-
-private:
-    void environmentWasChanged();
-
-    QDialogButtonBox *m_dialogButtonBox = nullptr;
-    RunConfiguration *m_rc = nullptr;
-    ArgumentsAspect m_arguments;
-    WorkingDirectoryAspect m_workingDirectory;
-    TerminalAspect m_terminal;
-    PathChooser *m_executableChooser = nullptr;
-};
-
-CustomExecutableDialog::CustomExecutableDialog(RunConfiguration *rc)
-    : QDialog(Core::ICore::dialogParent()),
-      m_rc(rc)
-{
-    auto vbox = new QVBoxLayout(this);
-    vbox->addWidget(new QLabel(tr("Could not find the executable, please specify one.")));
-
-    auto detailsContainer = new DetailsWidget(this);
-    detailsContainer->setState(DetailsWidget::NoSummary);
-    vbox->addWidget(detailsContainer);
-
-    m_dialogButtonBox = new QDialogButtonBox(QDialogButtonBox::Ok | QDialogButtonBox::Cancel);
-    m_dialogButtonBox->button(QDialogButtonBox::Ok)->setEnabled(false);
-    connect(m_dialogButtonBox, &QDialogButtonBox::accepted, this, &CustomExecutableDialog::accept);
-    connect(m_dialogButtonBox, &QDialogButtonBox::rejected, this, &QDialog::reject);
-    vbox->addWidget(m_dialogButtonBox);
-    vbox->setSizeConstraint(QLayout::SetMinAndMaxSize);
-
-    auto detailsWidget = new QWidget(detailsContainer);
-    detailsContainer->setWidget(detailsWidget);
-
-    m_executableChooser = new PathChooser(this);
-    m_executableChooser->setHistoryCompleter("Qt.CustomExecutable.History");
-    m_executableChooser->setExpectedKind(PathChooser::ExistingCommand);
-    m_executableChooser->setPath(rc->aspect<ExecutableAspect>()->executable().toString());
-    connect(m_executableChooser, &PathChooser::rawPathChanged,
-            this, &CustomExecutableDialog::changed);
-
-    copyAspect(rc->aspect<ArgumentsAspect>(), &m_arguments);
-    copyAspect(rc->aspect<WorkingDirectoryAspect>(), &m_workingDirectory);
-    copyAspect(rc->aspect<TerminalAspect>(), &m_terminal);
-
-    {
-        LayoutBuilder builder(detailsWidget);
-        builder.addItems(tr("Executable:"), m_executableChooser);
-        m_arguments.addToLayout(builder.startNewRow());
-        m_workingDirectory.addToLayout(builder.startNewRow());
-        m_terminal.addToLayout(builder.startNewRow());
-    }
-
-    auto enviromentAspect = rc->aspect<EnvironmentAspect>();
-    connect(enviromentAspect, &EnvironmentAspect::environmentChanged,
-            this, &CustomExecutableDialog::environmentWasChanged);
-    environmentWasChanged();
-
-    Core::VariableChooser::addSupportForChildWidgets(this, m_rc->macroExpander());
-}
-
-void CustomExecutableDialog::accept()
-{
-    auto executable = FilePath::fromString(m_executableChooser->path());
-    m_rc->aspect<ExecutableAspect>()->setExecutable(executable);
-    copyAspect(&m_arguments, m_rc->aspect<ArgumentsAspect>());
-    copyAspect(&m_workingDirectory, m_rc->aspect<WorkingDirectoryAspect>());
-    copyAspect(&m_terminal, m_rc->aspect<TerminalAspect>());
-
-    QDialog::accept();
-}
-
-bool CustomExecutableDialog::event(QEvent *event)
-{
-    if (event->type() == QEvent::ShortcutOverride) {
-        auto *ke = static_cast<QKeyEvent *>(event);
-        if (ke->key() == Qt::Key_Escape && !ke->modifiers()) {
-            ke->accept();
-            return true;
-        }
-    }
-    return QDialog::event(event);
-}
-
-void CustomExecutableDialog::environmentWasChanged()
-{
-    auto aspect = m_rc->aspect<EnvironmentAspect>();
-    QTC_ASSERT(aspect, return);
-    m_executableChooser->setEnvironment(aspect->environment());
-}
-
-
 // CustomExecutableRunConfiguration
 
 CustomExecutableRunConfiguration::CustomExecutableRunConfiguration(Target *target)
@@ -199,53 +85,11 @@ CustomExecutableRunConfiguration::CustomExecutableRunConfiguration(Target *targe
     setDefaultDisplayName(defaultDisplayName());
 }
 
-// Note: Qt4Project deletes all empty customexecrunconfigs for which isConfigured() == false.
-CustomExecutableRunConfiguration::~CustomExecutableRunConfiguration()
-{
-    if (m_dialog) {
-        emit configurationFinished();
-        disconnect(m_dialog, &QDialog::finished,
-                   this, &CustomExecutableRunConfiguration::configurationDialogFinished);
-    }
-    delete m_dialog;
-}
-
-RunConfiguration::ConfigurationState CustomExecutableRunConfiguration::ensureConfigured(QString *errorMessage)
-{
-    if (m_dialog) {// uhm already shown
-        errorMessage->clear(); // no error dialog
-        m_dialog->activateWindow();
-        m_dialog->raise();
-        return UnConfigured;
-    }
-
-    m_dialog = new CustomExecutableDialog(this);
-    connect(m_dialog, &QDialog::finished,
-            this, &CustomExecutableRunConfiguration::configurationDialogFinished);
-    m_dialog->setWindowTitle(displayName()); // pretty pointless
-    m_dialog->show();
-    return Waiting;
-}
-
-void CustomExecutableRunConfiguration::configurationDialogFinished()
-{
-    disconnect(m_dialog, &QDialog::finished,
-               this, &CustomExecutableRunConfiguration::configurationDialogFinished);
-    m_dialog->deleteLater();
-    m_dialog = nullptr;
-    emit configurationFinished();
-}
-
 QString CustomExecutableRunConfiguration::rawExecutable() const
 {
     return aspect<ExecutableAspect>()->executable().toString();
 }
 
-bool CustomExecutableRunConfiguration::isConfigured() const
-{
-    return !rawExecutable().isEmpty();
-}
-
 bool CustomExecutableRunConfiguration::isEnabled() const
 {
     return true;
@@ -278,6 +122,16 @@ QString CustomExecutableRunConfiguration::defaultDisplayName() const
         return tr("Run %1").arg(QDir::toNativeSeparators(rawExecutable()));
 }
 
+Tasks CustomExecutableRunConfiguration::checkForIssues() const
+{
+    Tasks tasks;
+    if (rawExecutable().isEmpty()) {
+        tasks << createConfigurationIssue(tr("You need to set an executable in the custom run "
+                                             "configuration."));
+    }
+    return tasks;
+}
+
 // Factory
 
 CustomExecutableRunConfigurationFactory::CustomExecutableRunConfigurationFactory() :

src/plugins/projectexplorer/customexecutablerunconfiguration.h

@@ -38,23 +38,16 @@ class PROJECTEXPLORER_EXPORT CustomExecutableRunConfiguration : public RunConfig
 public:
     CustomExecutableRunConfiguration(Target *target, Core::Id id);
     explicit CustomExecutableRunConfiguration(Target *target);
-    ~CustomExecutableRunConfiguration() override;
-
-    Runnable runnable() const override;
-
-    /** Returns whether this runconfiguration ever was configured with an executable
-     */
-    bool isConfigured() const override;
-    bool isEnabled() const override;
-    ConfigurationState ensureConfigured(QString *errorMessage) override;
 
     QString defaultDisplayName() const;
 
 private:
+    Runnable runnable() const override;
+    bool isEnabled() const override;
+    Tasks checkForIssues() const override;
+
     void configurationDialogFinished();
     QString rawExecutable() const;
-
-    class CustomExecutableDialog *m_dialog = nullptr;
 };
 
 class CustomExecutableRunConfigurationFactory : public FixedRunConfigurationFactory

src/plugins/projectexplorer/projectexplorer.cpp

@@ -431,8 +431,6 @@ public:
 
     void updateWelcomePage();
 
-    void runConfigurationConfigurationFinished();
-
     void checkForShutdown();
     void timerEvent(QTimerEvent *) override;
 
@@ -516,7 +514,6 @@ public:
 
     QString m_lastOpenDirectory;
     QPointer<RunConfiguration> m_delayedRunConfiguration;
-    QList<QPair<RunConfiguration *, Core::Id>> m_delayedRunConfigurationForRun;
     QString m_projectFilterString;
     MiniProjectTargetSelector * m_targetSelector;
     ProjectExplorerSettings m_projectExplorerSettings;
@@ -2306,22 +2303,15 @@ void ProjectExplorerPluginPrivate::restoreSession()
 
 void ProjectExplorerPluginPrivate::executeRunConfiguration(RunConfiguration *runConfiguration, Core::Id runMode)
 {
-    if (!runConfiguration->isConfigured()) {
-        QString errorMessage;
-        RunConfiguration::ConfigurationState state = runConfiguration->ensureConfigured(&errorMessage);
-
-        if (state == RunConfiguration::UnConfigured) {
-            m_instance->showRunErrorMessage(errorMessage);
-            return;
-        } else if (state == RunConfiguration::Waiting) {
-            connect(runConfiguration, &RunConfiguration::configurationFinished,
-                    this, &ProjectExplorerPluginPrivate::runConfigurationConfigurationFinished);
-            m_delayedRunConfigurationForRun.append(qMakePair(runConfiguration, runMode));
-            return;
-        }
+    const Tasks runConfigIssues = runConfiguration->checkForIssues();
+    if (!runConfigIssues.isEmpty()) {
+        for (const Task &t : runConfigIssues)
+            TaskHub::addTask(t);
+        // TODO: Insert an extra task with a "link" to the run settings page?
+        TaskHub::requestPopup();
+        return;
     }
 
-
     auto runControl = new RunControl(runMode);
     runControl->setRunConfiguration(runConfiguration);
 
@@ -2335,14 +2325,6 @@ void ProjectExplorerPluginPrivate::executeRunConfiguration(RunConfiguration *run
     startRunControl(runControl);
 }
 
-void ProjectExplorerPlugin::showRunErrorMessage(const QString &errorMessage)
-{
-    // Empty, non-null means 'canceled' (custom executable dialog for libraries), whereas
-    // empty, null means an error occurred, but message was not set
-    if (!errorMessage.isEmpty() || errorMessage.isNull())
-        QMessageBox::critical(ICore::mainWindow(), errorMessage.isNull() ? tr("Unknown error") : tr("Could Not Run"), errorMessage);
-}
-
 void ProjectExplorerPlugin::startRunControl(RunControl *runControl)
 {
     dd->startRunControl(runControl);
@@ -2429,21 +2411,6 @@ void ProjectExplorerPluginPrivate::buildQueueFinished(bool success)
     emit m_instance->updateRunActions();
 }
 
-void ProjectExplorerPluginPrivate::runConfigurationConfigurationFinished()
-{
-    auto rc = qobject_cast<RunConfiguration *>(sender());
-    Core::Id runMode = Constants::NO_RUN_MODE;
-    for (int i = 0; i < m_delayedRunConfigurationForRun.size(); ++i) {
-        if (m_delayedRunConfigurationForRun.at(i).first == rc) {
-            runMode = m_delayedRunConfigurationForRun.at(i).second;
-            m_delayedRunConfigurationForRun.removeAt(i);
-            break;
-        }
-    }
-    if (runMode != Constants::NO_RUN_MODE && rc->isConfigured())
-        executeRunConfiguration(rc, runMode);
-}
-
 QList<QPair<QString, QString> > ProjectExplorerPluginPrivate::recentProjects() const
 {
     return Utils::filtered(dd->m_recentProjects, [](const QPair<QString, QString> &p) {

src/plugins/projectexplorer/projectexplorer.h

@@ -142,7 +142,6 @@ public:
 
     static void startRunControl(RunControl *runControl);
     static void showOutputPaneForRunControl(RunControl *runControl);
-    static void showRunErrorMessage(const QString &errorMessage);
 
     // internal public for FlatModel
     static void renameFile(Node *node, const QString &newFilePath);

src/plugins/projectexplorer/runconfiguration.cpp

@@ -258,20 +258,6 @@ QMap<Core::Id, QVariantMap> RunConfiguration::aspectData() const
     return data;
 }
 
-bool RunConfiguration::isConfigured() const
-{
-    return true;
-}
-
-RunConfiguration::ConfigurationState RunConfiguration::ensureConfigured(QString *errorMessage)
-{
-    if (isConfigured())
-        return Configured;
-    if (errorMessage)
-        *errorMessage = tr("Unknown error.");
-    return UnConfigured;
-}
-
 BuildConfiguration *RunConfiguration::activeBuildConfiguration() const
 {
     return target()->activeBuildConfiguration();
@@ -287,6 +273,11 @@ void RunConfiguration::setUpdater(const Updater &updater)
     m_updater = updater;
 }
 
+Task RunConfiguration::createConfigurationIssue(const QString &description) const
+{
+    return {Task::Error, description, FilePath(), -1, Constants::TASK_CATEGORY_BUILDSYSTEM};
+}
+
 QVariantMap RunConfiguration::toMap() const
 {
     QVariantMap map = ProjectConfiguration::toMap();

src/plugins/projectexplorer/runconfiguration.h

@@ -25,11 +25,12 @@
 
 #pragma once
 
-#include "projectconfiguration.h"
-#include "projectexplorerconstants.h"
 #include "applicationlauncher.h"
 #include "buildtargetinfo.h"
 #include "devicesupport/idevice.h"
+#include "projectconfiguration.h"
+#include "projectexplorerconstants.h"
+#include "task.h"
 
 #include <utils/environment.h>
 #include <utils/port.h>
@@ -133,11 +134,8 @@ public:
 
     virtual QWidget *createConfigurationWidget();
 
-    virtual bool isConfigured() const;
-    // Pop up configuration dialog in case for example the executable is missing.
-    enum ConfigurationState { Configured, UnConfigured, Waiting };
-    // TODO rename function
-    virtual ConfigurationState ensureConfigured(QString *errorMessage = nullptr);
+    bool isConfigured() const { return checkForIssues().isEmpty(); }
+    virtual Tasks checkForIssues() const { return {}; }
 
     Utils::OutputFormatter *createOutputFormatter() const;
 
@@ -176,7 +174,6 @@ public:
     void update();
 
 signals:
-    void configurationFinished();
     void enabledChanged();
 
 protected:
@@ -191,6 +188,8 @@ protected:
 
     virtual void doAdditionalSetup(const RunConfigurationCreationInfo &) {}
 
+    Task createConfigurationIssue(const QString &description) const;
+
 private:
     static void addAspectFactory(const AspectFactory &aspectFactory);
 

src/plugins/remotelinux/remotelinuxcustomrunconfiguration.cpp

@@ -68,24 +68,6 @@ RemoteLinuxCustomRunConfiguration::RemoteLinuxCustomRunConfiguration(Target *tar
     setDefaultDisplayName(runConfigDefaultDisplayName());
 }
 
-bool RemoteLinuxCustomRunConfiguration::isConfigured() const
-{
-    return !aspect<ExecutableAspect>()->executable().isEmpty();
-}
-
-RunConfiguration::ConfigurationState
-RemoteLinuxCustomRunConfiguration::ensureConfigured(QString *errorMessage)
-{
-    if (!isConfigured()) {
-        if (errorMessage) {
-            *errorMessage = tr("The remote executable must be set "
-                               "in order to run a custom remote run configuration.");
-        }
-        return UnConfigured;
-    }
-    return Configured;
-}
-
 Core::Id RemoteLinuxCustomRunConfiguration::runConfigId()
 {
     return "RemoteLinux.CustomRunConfig";
@@ -107,6 +89,16 @@ Runnable RemoteLinuxCustomRunConfiguration::runnable() const
     return r;
 }
 
+Tasks RemoteLinuxCustomRunConfiguration::checkForIssues() const
+{
+    Tasks tasks;
+    if (aspect<ExecutableAspect>()->executable().isEmpty()) {
+        tasks << createConfigurationIssue(tr("The remote executable must be set in order to run "
+                                             "a custom remote run configuration."));
+    }
+    return tasks;
+}
+
 // RemoteLinuxCustomRunConfigurationFactory
 
 RemoteLinuxCustomRunConfigurationFactory::RemoteLinuxCustomRunConfigurationFactory()

src/plugins/remotelinux/remotelinuxcustomrunconfiguration.h

@@ -37,14 +37,12 @@ class RemoteLinuxCustomRunConfiguration : public ProjectExplorer::RunConfigurati
 public:
     RemoteLinuxCustomRunConfiguration(ProjectExplorer::Target *target, Core::Id id);
 
-    bool isConfigured() const override;
-    ConfigurationState ensureConfigured(QString *errorMessage) override;
-
     static Core::Id runConfigId();
     QString runConfigDefaultDisplayName();
 
 private:
     ProjectExplorer::Runnable runnable() const override;
+    ProjectExplorer::Tasks checkForIssues() const override;
 };
 
 class RemoteLinuxCustomRunConfigurationFactory