SettingsAccessor: Improve SettingsAccessor::isBetterMatch

Improve the behavior of isBetterMatch, so that it now handles
invalid versions as well as invalid ids. Simplify code where possible.

Change-Id: I2e65c0cc01d32fa77df8b5ec0d62b765e6c458f7
Reviewed-by: Eike Ziller <eike.ziller@qt.io>

src/libs/utils/settingsaccessor.cpp

@@ -313,8 +313,6 @@ QVariantMap SettingsAccessor::prepareSettings(const QVariantMap &data) const
  * Check which of two sets of data are a better match to load.
  *
  * This method is used to compare data extracted from two XML settings files.
- * It will never be called with a version too old or too new to be read by
- * the current instance of Qt Creator.
  *
  * Compares \a newData against \a origData.
  *
@@ -322,29 +320,31 @@ QVariantMap SettingsAccessor::prepareSettings(const QVariantMap &data) const
  */
 bool SettingsAccessor::isBetterMatch(const QVariantMap &origData, const QVariantMap &newData) const
 {
-    if (origData.isEmpty())
-        return true;
+    const int origVersion = versionFromMap(origData);
+    const bool origValid = isValidVersionAndId(origVersion, settingsIdFromMap(origData));
 
-    const QByteArray id = settingsId();
+    const int newVersion = versionFromMap(newData);
+    const bool newValid = isValidVersionAndId(newVersion, settingsIdFromMap(newData));
 
-    int origVersion = versionFromMap(origData);
-    QByteArray origEnv = settingsIdFromMap(origData);
+    if (!origValid)
+        return newValid;
 
-    int newVersion = versionFromMap(newData);
-    QByteArray newEnv = settingsIdFromMap(newData);
-
-    if (!id.isEmpty()) {
-        if (origEnv != newEnv) {
-            if (origEnv == id)
+    if (!newValid)
         return false;
-            if (newEnv == id)
-                return true;
-        }
-    }
 
     return newVersion > origVersion;
 }
 
+bool SettingsAccessor::isValidVersionAndId(const int version, const QByteArray &id) const
+{
+    const QByteArray requiredId = settingsId();
+    const int firstVersion = firstSupportedVersion();
+    const int lastVersion = currentVersion();
+
+    return (version >= firstVersion && version <= lastVersion)
+            && ( id == requiredId || requiredId.isEmpty());
+}
+
 /*!
  * Upgrade the settings in \a data to the version \a toVersion.
  *
@@ -742,11 +742,6 @@ SettingsAccessorPrivate::Settings SettingsAccessorPrivate::bestSettings(const Se
     Settings bestMatch;
     foreach (const FileName &path, pathList) {
         QVariantMap tmp = accessor->readFile(path);
-
-        int version = SettingsAccessor::versionFromMap(tmp);
-        if (version < firstVersion() || version > currentVersion())
-            continue;
-
         if (accessor->isBetterMatch(bestMatch.map, tmp)) {
             bestMatch.path = path;
             bestMatch.map = tmp;
@@ -780,6 +775,7 @@ QVariantMap SettingsAccessor::mergeSettings(const QVariantMap &userMap,
 // -------------------------------------------------------------------------
 // SettingsData
 // -------------------------------------------------------------------------
+
 bool SettingsAccessorPrivate::Settings::isValid() const
 {
     return SettingsAccessor::versionFromMap(map) > -1 && !path.isEmpty();

src/libs/utils/settingsaccessor.h

@@ -38,7 +38,8 @@ namespace Utils {
 // --------------------------------------------------------------------
 // VersionUpgrader:
 // --------------------------------------------------------------------
-// Handles updating a QVariantMap from version() - 1 to version()
+
+// Handles updating a QVariantMap from version() to version() + 1
 class QTCREATOR_UTILS_EXPORT VersionUpgrader
 {
 public:
@@ -124,6 +125,7 @@ protected:
     virtual QVariantMap prepareToSaveSettings(const QVariantMap &data) const;
 
     virtual bool isBetterMatch(const QVariantMap &origData, const QVariantMap &newData) const;
+    virtual bool isValidVersionAndId(const int version, const QByteArray &id) const;
 
     virtual Utils::FileName backupName(const QVariantMap &data) const;
 

tests/auto/utils/settings/tst_settings.cpp

@@ -87,6 +87,7 @@ public:
     }
 
     // Make methods public for the tests:
+    using Utils::SettingsAccessor::isValidVersionAndId;
     using Utils::SettingsAccessor::isBetterMatch;
 };
 
@@ -106,6 +107,8 @@ private slots:
     void addVersionUpgrader_v3v4v5();
     void addVersionUpgrader_v0v1();
 
+    void isValidVersionAndId();
+
     void isBetterMatch();
     void isBetterMatch_idMismatch();
     void isBetterMatch_noId();
@@ -200,6 +203,25 @@ void tst_SettingsAccessor::addVersionUpgrader_v0v1()
     QCOMPARE(accessor.currentVersion(), 2);
 }
 
+void tst_SettingsAccessor::isValidVersionAndId()
+{
+    const TestSettingsAccessor accessor;
+
+    QVERIFY(!accessor.isValidVersionAndId(4, TESTACCESSOR_DEFAULT_ID));
+    QVERIFY(accessor.isValidVersionAndId(5, TESTACCESSOR_DEFAULT_ID));
+    QVERIFY(accessor.isValidVersionAndId(6, TESTACCESSOR_DEFAULT_ID));
+    QVERIFY(accessor.isValidVersionAndId(7, TESTACCESSOR_DEFAULT_ID));
+    QVERIFY(accessor.isValidVersionAndId(8, TESTACCESSOR_DEFAULT_ID));
+    QVERIFY(!accessor.isValidVersionAndId(9, TESTACCESSOR_DEFAULT_ID));
+
+    QVERIFY(!accessor.isValidVersionAndId(4, "foo"));
+    QVERIFY(!accessor.isValidVersionAndId(5, "foo"));
+    QVERIFY(!accessor.isValidVersionAndId(6, "foo"));
+    QVERIFY(!accessor.isValidVersionAndId(7, "foo"));
+    QVERIFY(!accessor.isValidVersionAndId(8, "foo"));
+    QVERIFY(!accessor.isValidVersionAndId(9, "foo"));
+}
+
 void tst_SettingsAccessor::isBetterMatch()
 {
     const TestSettingsAccessor accessor;
@@ -262,12 +284,7 @@ void tst_SettingsAccessor::isBetterMatch_twoEmptyMaps()
     const QVariantMap a;
     const QVariantMap b;
 
-    // The following two fails are harmless: They claim an empty map is better than another empty
-    // map, so it will trigger a useless copy of one empty map over another.
-    // This copy will be avoided when reworking isBetterMatch later.
-    QEXPECT_FAIL("", "harmless but unexpected behavior", Continue);
     QVERIFY(!accessor.isBetterMatch(a, b));
-    QEXPECT_FAIL("", "harmless but unexpected behavior", Continue);
     QVERIFY(!accessor.isBetterMatch(b, a));
 }