From cf9249a905a6aa7bec904fc51b9ee7d9ad39dd06 Mon Sep 17 00:00:00 2001 From: Christian Kandeler Date: Thu, 5 Dec 2019 16:28:52 +0100 Subject: [PATCH] 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 --- .../baremetalcustomrunconfiguration.cpp | 20 +-- .../baremetalcustomrunconfiguration.h | 3 +- src/plugins/boot2qt/qdbrunconfiguration.cpp | 15 +- src/plugins/boot2qt/qdbrunconfiguration.h | 4 +- .../customexecutablerunconfiguration.cpp | 166 ++---------------- .../customexecutablerunconfiguration.h | 15 +- .../projectexplorer/projectexplorer.cpp | 47 +---- src/plugins/projectexplorer/projectexplorer.h | 1 - .../projectexplorer/runconfiguration.cpp | 19 +- .../projectexplorer/runconfiguration.h | 15 +- .../remotelinuxcustomrunconfiguration.cpp | 28 ++- .../remotelinuxcustomrunconfiguration.h | 4 +- 12 files changed, 59 insertions(+), 278 deletions(-) diff --git a/src/plugins/baremetal/baremetalcustomrunconfiguration.cpp b/src/plugins/baremetal/baremetalcustomrunconfiguration.cpp index db4aac76223..362fc3ebb15 100644 --- a/src/plugins/baremetal/baremetalcustomrunconfiguration.cpp +++ b/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()->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()->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 diff --git a/src/plugins/baremetal/baremetalcustomrunconfiguration.h b/src/plugins/baremetal/baremetalcustomrunconfiguration.h index a7ccd48233b..130478b025c 100644 --- a/src/plugins/baremetal/baremetalcustomrunconfiguration.h +++ b/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 diff --git a/src/plugins/boot2qt/qdbrunconfiguration.cpp b/src/plugins/boot2qt/qdbrunconfiguration.cpp index 80fdd072607..ca74afad0de 100644 --- a/src/plugins/boot2qt/qdbrunconfiguration.cpp +++ b/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()->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()->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 diff --git a/src/plugins/boot2qt/qdbrunconfiguration.h b/src/plugins/boot2qt/qdbrunconfiguration.h index 9d31669f654..f1a0bf0443d 100644 --- a/src/plugins/boot2qt/qdbrunconfiguration.h +++ b/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; }; diff --git a/src/plugins/projectexplorer/customexecutablerunconfiguration.cpp b/src/plugins/projectexplorer/customexecutablerunconfiguration.cpp index 62ba2eace55..e0de305e5f3 100644 --- a/src/plugins/projectexplorer/customexecutablerunconfiguration.cpp +++ b/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()->executable().toString()); - connect(m_executableChooser, &PathChooser::rawPathChanged, - this, &CustomExecutableDialog::changed); - - copyAspect(rc->aspect(), &m_arguments); - copyAspect(rc->aspect(), &m_workingDirectory); - copyAspect(rc->aspect(), &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(); - 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()->setExecutable(executable); - copyAspect(&m_arguments, m_rc->aspect()); - copyAspect(&m_workingDirectory, m_rc->aspect()); - copyAspect(&m_terminal, m_rc->aspect()); - - QDialog::accept(); -} - -bool CustomExecutableDialog::event(QEvent *event) -{ - if (event->type() == QEvent::ShortcutOverride) { - auto *ke = static_cast(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(); - 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()->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() : diff --git a/src/plugins/projectexplorer/customexecutablerunconfiguration.h b/src/plugins/projectexplorer/customexecutablerunconfiguration.h index bcd7b3b9bae..351eaad95a1 100644 --- a/src/plugins/projectexplorer/customexecutablerunconfiguration.h +++ b/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 diff --git a/src/plugins/projectexplorer/projectexplorer.cpp b/src/plugins/projectexplorer/projectexplorer.cpp index da51ba1efce..1ceb10c9867 100644 --- a/src/plugins/projectexplorer/projectexplorer.cpp +++ b/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 m_delayedRunConfiguration; - QList> 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(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 > ProjectExplorerPluginPrivate::recentProjects() const { return Utils::filtered(dd->m_recentProjects, [](const QPair &p) { diff --git a/src/plugins/projectexplorer/projectexplorer.h b/src/plugins/projectexplorer/projectexplorer.h index 93d2c141b6c..15f132d7f17 100644 --- a/src/plugins/projectexplorer/projectexplorer.h +++ b/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); diff --git a/src/plugins/projectexplorer/runconfiguration.cpp b/src/plugins/projectexplorer/runconfiguration.cpp index 74959b5938c..21b5bb02b92 100644 --- a/src/plugins/projectexplorer/runconfiguration.cpp +++ b/src/plugins/projectexplorer/runconfiguration.cpp @@ -258,20 +258,6 @@ QMap 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(); diff --git a/src/plugins/projectexplorer/runconfiguration.h b/src/plugins/projectexplorer/runconfiguration.h index b154dae1c2d..22cb774cb8a 100644 --- a/src/plugins/projectexplorer/runconfiguration.h +++ b/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 #include @@ -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); diff --git a/src/plugins/remotelinux/remotelinuxcustomrunconfiguration.cpp b/src/plugins/remotelinux/remotelinuxcustomrunconfiguration.cpp index d43cc7a9f1b..32db508f8a7 100644 --- a/src/plugins/remotelinux/remotelinuxcustomrunconfiguration.cpp +++ b/src/plugins/remotelinux/remotelinuxcustomrunconfiguration.cpp @@ -68,24 +68,6 @@ RemoteLinuxCustomRunConfiguration::RemoteLinuxCustomRunConfiguration(Target *tar setDefaultDisplayName(runConfigDefaultDisplayName()); } -bool RemoteLinuxCustomRunConfiguration::isConfigured() const -{ - return !aspect()->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()->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() diff --git a/src/plugins/remotelinux/remotelinuxcustomrunconfiguration.h b/src/plugins/remotelinux/remotelinuxcustomrunconfiguration.h index aaaf488ba40..186412fe3c2 100644 --- a/src/plugins/remotelinux/remotelinuxcustomrunconfiguration.h +++ b/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