ProjectExplorer: Consolidate RunConfiguration identifications

The previously per-Project/RunConfiguration changing meanings of
BuildTargetInfo::buildTarget have by now been split
into separate values in BuildTargetInfo:
  - buildKey     a handle to one item in Target::applicationTargetList
  - displayName  a user-visible string in the run settings page

The buildKey was tweaked to coincide with the previous 'extraId',
i.e. the non-RunConfiguration-type part of the project configuration
id that (still) use id mangling.

This allows replacing the cases of locally stored seven different
versions of buildKey(-ish) data by one RunConfiguration::m_buildKey,
and do all remaining extraId handling in RC::{from,to}Map only,
i.e. remove the base ProjectConfiguration::extraId() virtual and
remove the "re-try fromMap with mangled id" hack entirely.

The id mangling is still used to temporarily maintain .user file
compatibility in some cases for now, but should be replaced by
storing the build key and the RunConfiguration type soon. Qbs
already changes in here to only use the uniqueProductName as
buildKey, without the previously added display name which is
stored as part of the ProjectConfiguration already.

It turns out that RunConfiguration::buildSystemTarget was intended
and used to retrieve an item from the Target::applicationTargetList
for some configurations, coinciding with what buildKey does always.
So use that insteand and drop RunConfiguration::buildSystemTarget.

There is clearly is further consolidation potential left.

handling of (default)displayNames is still a per-runconfiguration
mess and there is further consolidation potential left.

Change-Id: I448ed30f1b562fb91b970e328a42fa5f6fb2e43e
Reviewed-by: Tobias Hunger <tobias.hunger@qt.io>

6 files changed, 38 insertions, 55 deletions

diff --git a/src/plugins/projectexplorer/buildtargetinfo.h b/src/plugins/projectexplorer/buildtargetinfo.h
index 2ba3225d5d..a6b46aed11 100644
--- a/src/plugins/projectexplorer/buildtargetinfo.h
+++ b/src/plugins/projectexplorer/buildtargetinfo.h
@@ -39,18 +39,8 @@ namespace ProjectExplorer {
 class PROJECTEXPLORER_EXPORT BuildTargetInfo
 {
 public:
-    BuildTargetInfo() = default;
-    BuildTargetInfo(const QString &targetName, const Utils::FileName &targetFilePath,
-                    const Utils::FileName &projectFilePath, bool isQtcRunnable = true) :
-        targetName(targetName),
-        targetFilePath(targetFilePath),
-        projectFilePath(projectFilePath),
-        isQtcRunnable(isQtcRunnable)
-    { }
-
-    QString targetName;
-    QString displayName;
     QString buildKey; // Used to identify this BuildTargetInfo object in its list.
+    QString displayName;
 
     Utils::FileName targetFilePath;
     Utils::FileName projectFilePath;
@@ -63,9 +53,8 @@ public:
 
 inline bool operator==(const BuildTargetInfo &ti1, const BuildTargetInfo &ti2)
 {
-    return ti1.targetName == ti2.targetName
+    return ti1.buildKey == ti2.buildKey
         && ti1.displayName == ti2.displayName
-        && ti1.buildKey == ti2.buildKey
         && ti1.targetFilePath == ti2.targetFilePath
         && ti1.projectFilePath == ti2.projectFilePath;
 }
@@ -77,7 +66,7 @@ inline bool operator!=(const BuildTargetInfo &ti1, const BuildTargetInfo &ti2)
 
 inline uint qHash(const BuildTargetInfo &ti)
 {
-    return qHash(ti.targetName) ^ qHash(ti.displayName) ^ qHash(ti.buildKey);
+    return qHash(ti.displayName) ^ qHash(ti.buildKey);
 }
 
 class PROJECTEXPLORER_EXPORT BuildTargetInfoList
@@ -97,16 +86,10 @@ public:
         return Utils::FileName();
     }
 
-    bool hasTarget(const QString &targetName) {
-        return Utils::anyOf(list, [&targetName](const BuildTargetInfo &ti) {
-            return ti.targetName == targetName;
-        });
-    }
-
-    Utils::FileName targetFilePath(const QString &targetName) {
-        return Utils::findOrDefault(list, [&targetName](const BuildTargetInfo &ti) {
-            return ti.targetName == targetName
-                || ti.projectFilePath.toString() == targetName;
+    Utils::FileName targetFilePath(const QString &buildKey) {
+        return Utils::findOrDefault(list, [&buildKey](const BuildTargetInfo &ti) {
+            return ti.buildKey == buildKey
+                || ti.projectFilePath.toString() == buildKey;
         }).targetFilePath;
     }
 
diff --git a/src/plugins/projectexplorer/projectconfiguration.cpp b/src/plugins/projectexplorer/projectconfiguration.cpp
index 5e49e97ade..a2213f86ae 100644
--- a/src/plugins/projectexplorer/projectconfiguration.cpp
+++ b/src/plugins/projectexplorer/projectconfiguration.cpp
@@ -45,11 +45,6 @@ Core::Id ProjectConfiguration::id() const
     return m_id;
 }
 
-QString ProjectConfiguration::extraId() const
-{
-    return QString();
-}
-
 QString ProjectConfiguration::settingsIdKey()
 {
     return QString(CONFIGURATION_ID_KEY);
@@ -105,7 +100,7 @@ QVariantMap ProjectConfiguration::toMap() const
 {
     QTC_CHECK(m_id.isValid());
     QVariantMap map;
-    map.insert(QLatin1String(CONFIGURATION_ID_KEY), m_id.withSuffix(extraId()).toSetting());
+    map.insert(QLatin1String(CONFIGURATION_ID_KEY), m_id.toSetting());
     map.insert(QLatin1String(DISPLAY_NAME_KEY), m_displayName);
     map.insert(QLatin1String(DEFAULT_DISPLAY_NAME_KEY), m_defaultDisplayName);
     return map;
@@ -114,6 +109,8 @@ QVariantMap ProjectConfiguration::toMap() const
 bool ProjectConfiguration::fromMap(const QVariantMap &map)
 {
     Core::Id id = Core::Id::fromSetting(map.value(QLatin1String(CONFIGURATION_ID_KEY)));
+    // Note: This is only "startsWith", not ==, as RunConfigurations currently still
+    // mangle in their build keys.
     QTC_ASSERT(id.toString().startsWith(m_id.toString()), return false);
 
     m_displayName = map.value(QLatin1String(DISPLAY_NAME_KEY), QString()).toString();
diff --git a/src/plugins/projectexplorer/projectconfiguration.h b/src/plugins/projectexplorer/projectconfiguration.h
index 40ed0a5437..27078d649e 100644
--- a/src/plugins/projectexplorer/projectconfiguration.h
+++ b/src/plugins/projectexplorer/projectconfiguration.h
@@ -72,9 +72,6 @@ public:
 
     virtual bool isActive() const = 0;
 
-    // Used in settings to mangle in build targets in RunConfigurations.
-    virtual QString extraId() const;
-
     static QString settingsIdKey();
 
 signals:
diff --git a/src/plugins/projectexplorer/runconfiguration.cpp b/src/plugins/projectexplorer/runconfiguration.cpp
index 1af6e7f930..c8e40bef8d 100644
--- a/src/plugins/projectexplorer/runconfiguration.cpp
+++ b/src/plugins/projectexplorer/runconfiguration.cpp
@@ -326,6 +326,11 @@ QVariantMap RunConfiguration::toMap() const
 {
     QVariantMap map = ProjectConfiguration::toMap();
 
+    // FIXME: Remove this id mangling, e.g. by using a separate entry for the build key.
+    if (!m_buildKey.isEmpty()) {
+        const Core::Id mangled = id().withSuffix(m_buildKey);
+        map.insert(settingsIdKey(), mangled.toSetting());
+    }
     foreach (IRunConfigurationAspect *aspect, m_aspects)
         aspect->toMap(map);
 
@@ -345,10 +350,17 @@ Abi RunConfiguration::abi() const
 
 bool RunConfiguration::fromMap(const QVariantMap &map)
 {
+    if (!ProjectConfiguration::fromMap(map))
+        return false;
+
+    // FIXME: Remove this id mangling, e.g. by using a separate entry for the build key.
+    const Core::Id mangledId = Core::Id::fromSetting(map.value(settingsIdKey()));
+    m_buildKey = mangledId.suffixAfter(id());
+
     foreach (IRunConfigurationAspect *aspect, m_aspects)
         aspect->fromMap(map);
 
-    return ProjectConfiguration::fromMap(map);
+    return  true;
 }
 
 /*!
@@ -485,10 +497,10 @@ RunConfigurationFactory::availableCreators(Target *parent) const
     return Utils::transform(buildTargets, [&](const BuildTargetInfo &ti) {
         QString displayName = ti.displayName;
         if (displayName.isEmpty())
-            displayName = decoratedTargetName(ti.targetName, parent);
+            displayName = decoratedTargetName(ti.buildKey, parent);
         else if (m_decorateDisplayNames)
             displayName = decoratedTargetName(displayName, parent);
-        RunConfigurationCreationInfo rci(this, m_runConfigBaseId, ti.targetName, displayName);
+        RunConfigurationCreationInfo rci(this, m_runConfigBaseId, ti.buildKey, displayName);
         rci.creationMode = ti.isQtcRunnable || !hasAnyQtcRunnable
                 ? RunConfigurationCreationInfo::AlwaysCreate
                 : RunConfigurationCreationInfo::ManualCreationOnly;
@@ -551,15 +563,9 @@ RunConfiguration *RunConfigurationCreationInfo::create(Target *target) const
     if (!rc)
         return nullptr;
 
-    // "FIX" ids by mangling in the extra data (build system target etc)
-    // for compatibility for the current format used in settings.
-    if (!targetName.isEmpty()) {
-        QVariantMap data = rc->toMap();
-        data[ProjectConfiguration::settingsIdKey()] = id.withSuffix(targetName).toString();
-        rc->fromMap(data);
-    }
-
+    rc->m_buildKey = buildKey;
     rc->doAdditionalSetup(*this);
+    rc->setDefaultDisplayName(displayName);
 
     return rc;
 }
diff --git a/src/plugins/projectexplorer/runconfiguration.h b/src/plugins/projectexplorer/runconfiguration.h
index ae268990db..9dd22d784b 100644
--- a/src/plugins/projectexplorer/runconfiguration.h
+++ b/src/plugins/projectexplorer/runconfiguration.h
@@ -245,9 +245,9 @@ public:
     virtual Runnable runnable() const;
     virtual Abi abi() const;
 
-    // Return the name of the build system target that created this run configuration.
+    // Return a handle to the build system target that created this run configuration.
     // May return an empty string if no target built the executable!
-    virtual QString buildSystemTarget() const { return QString(); }
+    QString buildKey() const { return m_buildKey; }
 
     void addExtraAspect(IRunConfigurationAspect *aspect);
 
@@ -280,6 +280,7 @@ private:
     friend class RunConfigurationCreationInfo;
 
     QList<IRunConfigurationAspect *> m_aspects;
+    QString m_buildKey;
 };
 
 class RunConfigurationCreationInfo
@@ -289,11 +290,11 @@ public:
     RunConfigurationCreationInfo() = default;
     RunConfigurationCreationInfo(const RunConfigurationFactory *factory,
                                  Core::Id id,
-                                 QString extra, QString displayName,
+                                 QString buildKey, QString displayName,
                                  CreationMode creationMode = AlwaysCreate,
                                  bool useTerminal = false)
         : factory(factory), id(id),
-          targetName(extra),
+          buildKey(buildKey),
           displayName(displayName),
           creationMode(creationMode),
           useTerminal(useTerminal)
@@ -303,9 +304,8 @@ public:
 
     const RunConfigurationFactory *factory = nullptr;
     Core::Id id;
-    QString targetName;
-    QString displayName;
     QString buildKey;
+    QString displayName;
     CreationMode creationMode = AlwaysCreate;
     bool useTerminal = false;
 };
diff --git a/src/plugins/projectexplorer/target.cpp b/src/plugins/projectexplorer/target.cpp
index 1dd9561649..de47a67f26 100644
--- a/src/plugins/projectexplorer/target.cpp
+++ b/src/plugins/projectexplorer/target.cpp
@@ -564,8 +564,8 @@ void Target::updateDefaultRunConfigurations()
     foreach (RunConfiguration *rc, existingConfigured) {
         bool present = false;
         for (const RunConfigurationCreationInfo &item : creators) {
-            QString rcExtraId = rc->extraId();
-            if (item.id == rc->id() && (item.targetName == rcExtraId || item.buildKey == rcExtraId)) {
+            QString buildKey = rc->buildKey();
+            if (item.id == rc->id() && item.buildKey == buildKey) {
                 existing.append(item);
                 present = true;
             }
@@ -581,7 +581,7 @@ void Target::updateDefaultRunConfigurations()
             continue;
         bool exists = false;
         for (const RunConfigurationCreationInfo &ex : existing) {
-            if (ex.id == item.id && ex.targetName == item.targetName)
+            if (ex.id == item.id && ex.buildKey == item.buildKey)
                 exists = true;
         }
         if (exists)
@@ -776,7 +776,7 @@ bool Target::fromMap(const QVariantMap &map)
                      id.isValid() ? qPrintable(id.toString()) : "UNKNOWN");
             continue;
         }
-        QTC_CHECK(dc->id().withSuffix(dc->extraId()) == ProjectExplorer::idFromMap(valueMap));
+        QTC_CHECK(dc->id() == ProjectExplorer::idFromMap(valueMap));
         addDeployConfiguration(dc);
         if (i == activeConfiguration)
             setActiveDeployConfiguration(dc);
@@ -801,7 +801,7 @@ bool Target::fromMap(const QVariantMap &map)
         RunConfiguration *rc = RunConfigurationFactory::restore(this, valueMap);
         if (!rc)
             continue;
-        QTC_CHECK(rc->id().withSuffix(rc->extraId()) == ProjectExplorer::idFromMap(valueMap));
+        QTC_CHECK(rc->id().withSuffix(rc->buildKey()) == ProjectExplorer::idFromMap(valueMap));
         addRunConfiguration(rc);
         if (i == activeConfiguration)
             setActiveRunConfiguration(rc);