SettingsAccessor: Keep upgraders in a list instead of a map

This also removes the need to store m_firstVersion and m_lastVersion.
Add some convenience functions for that.

The bool return value from addVersionUpgrader is never used in the
unit tests.

Change-Id: I2f372ef7353b72a0c577b429f725277d2c579872
Reviewed-by: Daniel Teske <daniel.teske@digia.com>

src/plugins/projectexplorer/settingsaccessor.cpp

@@ -446,16 +446,24 @@ public:
         Utils::FileName path;
     };
 
+    int firstVersion() const { return m_upgraders.isEmpty() ? -1 : m_upgraders.first()->version(); }
+    int lastVersion() const { return m_upgraders.isEmpty() ? -1 : m_upgraders.last()->version(); }
+    int currentVersion() const { return lastVersion() + 1; }
+    VersionUpgrader *upgrader(const int version) const
+    {
+        int pos = version - firstVersion();
+        if (pos >= 0 && pos < m_upgraders.count())
+            return m_upgraders.at(pos);
+        return 0;
+    }
     Settings bestSettings(const SettingsAccessor *accessor, const QStringList &candidates) const;
 
-    QMap<int, Internal::VersionUpgrader *> m_upgraders;
+    QList<VersionUpgrader *> m_upgraders;
     Utils::PersistentSettingsWriter *m_writer;
 };
 } // end namespace
 
 SettingsAccessor::SettingsAccessor(Project *project) :
-    m_firstVersion(-1),
-    m_lastVersion(-1),
     m_project(project),
     d(new SettingsAccessorPrivate)
 {
@@ -600,12 +608,13 @@ QVariantMap SettingsAccessor::upgradeSettings(const QVariantMap &data, int toVer
         result = data;
 
     if (version >= toVersion
-            || version < m_firstVersion
+            || version < d->firstVersion()
-            || toVersion > currentVersion())
+            || toVersion > d->currentVersion())
         return result;
 
     for (int i = version; i < toVersion; ++i) {
-        VersionUpgrader *upgrader = d->m_upgraders.value(i);
+        VersionUpgrader *upgrader = d->upgrader(i);
+        QTC_CHECK(upgrader && upgrader->version() == i);
         result = upgrader->upgrade(result);
         result = setVersionInMap(result, i + 1);
     }
@@ -676,7 +685,7 @@ QByteArray SettingsAccessor::environmentIdFromMap(const QVariantMap &data)
 
 QVariantMap SettingsAccessor::restoreSettings(QWidget *parent) const
 {
-    if (m_lastVersion < 0)
+    if (d->lastVersion() < 0)
         return QVariantMap();
 
     QVariantMap userSettings = readUserSettings(parent);
@@ -711,41 +720,27 @@ bool SettingsAccessor::saveSettings(const QVariantMap &map, QWidget *parent) con
         data.insert(i.key(), i.value());
     }
 
-    data.insert(QLatin1String(VERSION_KEY), m_lastVersion + 1);
+    data.insert(QLatin1String(VERSION_KEY), d->currentVersion());
     // for compatibility with QtC 3.1 and older:
-    data.insert(QLatin1String(OBSOLETE_VERSION_KEY), m_lastVersion + 1); // TODO: Move into UserfileAccessor!
+    data.insert(QLatin1String(OBSOLETE_VERSION_KEY), d->currentVersion()); // TODO: Move into UserfileAccessor!
     data.insert(QLatin1String(ENVIRONMENT_ID_KEY), SettingsAccessor::creatorId());
     return d->m_writer->save(data, parent);
 }
 
-void SettingsAccessor::addVersionUpgrader(VersionUpgrader *handler)
+bool SettingsAccessor::addVersionUpgrader(VersionUpgrader *upgrader)
 {
-    const int version(handler->version());
+    QTC_ASSERT(upgrader, return false);
-    QTC_ASSERT(handler, return);
+    int version = upgrader->version();
-    QTC_ASSERT(version >= 0, return);
+    QTC_ASSERT(version >= 0, return false);
-    QTC_ASSERT(!d->m_upgraders.contains(version), return);
-    QTC_ASSERT(d->m_upgraders.isEmpty() ||
-               (version == m_lastVersion + 1 || version == m_firstVersion - 1), return);
 
-    if (d->m_upgraders.isEmpty()) {
+    if (d->m_upgraders.isEmpty() || d->currentVersion() == version)
-        m_firstVersion = version;
+        d->m_upgraders.append(upgrader);
-        m_lastVersion = version;
+    else if (d->firstVersion() - 1 == version)
-    } else {
+        d->m_upgraders.prepend(upgrader);
-        if (version < m_firstVersion)
+    else
-            m_firstVersion = version;
+        QTC_ASSERT(false, return false); // Upgrader was added out of sequence or twice
-        if (version > m_lastVersion)
-            m_lastVersion = version;
-    }
 
-    d->m_upgraders.insert(version, handler);
+    return true;
-
-    // Postconditions:
-    Q_ASSERT(m_lastVersion >= 0);
-    Q_ASSERT(m_firstVersion >= 0);
-    Q_ASSERT(m_lastVersion >= m_firstVersion);
-    Q_ASSERT(d->m_upgraders.count() == m_lastVersion - m_firstVersion + 1);
-    for (int i = m_firstVersion; i < m_lastVersion; ++i)
-        Q_ASSERT(d->m_upgraders.contains(i));
 }
 
 /* Will always return the default name first */
@@ -781,7 +776,12 @@ QString SettingsAccessor::defaultFileName(const QString &suffix) const
 
 int SettingsAccessor::currentVersion() const
 {
-    return m_lastVersion + 1;
+    return d->currentVersion();
+}
+
+int SettingsAccessor::firstSupportedVersion() const
+{
+    return d->firstVersion();
 }
 
 void SettingsAccessor::backupUserFile() const
@@ -800,8 +800,9 @@ void SettingsAccessor::backupUserFile() const
         backupName += QLatin1String(".") + QString::fromLatin1(oldEnvironmentId).mid(1, 7);
     const int oldVersion = versionFromMap(oldSettings.map);
     if (oldVersion != currentVersion()) {
-        if (d->m_upgraders.contains(oldVersion))
+        VersionUpgrader *upgrader = d->upgrader(oldVersion);
-            backupName += QLatin1String(".") + d->m_upgraders.value(oldVersion)->backupExtension();
+        if (upgrader)
+            backupName += QLatin1String(".") + upgrader->backupExtension();
         else
             backupName += QLatin1String(".") + QString::number(oldVersion);
     }
@@ -925,7 +926,7 @@ SettingsAccessorPrivate::Settings SettingsAccessorPrivate::bestSettings(const Se
             qWarning() << "Skipping settings file" << tmp.path.toUserOutput() << "(too new).";
             continue;
         }
-        if (tmpVersion < accessor->m_firstVersion) {
+        if (tmpVersion < accessor->firstSupportedVersion()) {
             qWarning() << "Skipping settings file" << tmp.path.toUserOutput() << "(too old).";
             continue;
         }
@@ -938,7 +939,7 @@ SettingsAccessorPrivate::Settings SettingsAccessorPrivate::bestSettings(const Se
             if (tmpVersion > SettingsAccessor::versionFromMap(newestNonMatching.map))
                 newestNonMatching = tmp;
         }
-        if (SettingsAccessor::versionFromMap(newestMatching.map) == accessor->m_lastVersion + 1)
+        if (SettingsAccessor::versionFromMap(newestMatching.map) == accessor->currentVersion())
             break;
     }
 

src/plugins/projectexplorer/settingsaccessor.h

@@ -60,7 +60,10 @@ public:
     static int originalVersionFromMap(const QVariantMap &data);
     static QVariantMap setOriginalVersionInMap(const QVariantMap &data, int version);
 
-    void addVersionUpgrader(Internal::VersionUpgrader *handler); // Takes ownership of the handler!
+    int currentVersion() const;
+    int firstSupportedVersion() const;
+
+    bool addVersionUpgrader(Internal::VersionUpgrader *upgrader); // takes ownership of upgrader
 
 protected:
     QVariantMap readFile(const Utils::FileName &path) const;
@@ -71,7 +74,6 @@ private:
     QStringList findSettingsFiles(const QString &suffix) const;
     static QByteArray creatorId();
     QString defaultFileName(const QString &suffix) const;
-    int currentVersion() const;
     void backupUserFile() const;
 
     QVariantMap readUserSettings(QWidget *parent) const;
@@ -80,8 +82,6 @@ private:
 
     static QByteArray environmentIdFromMap(const QVariantMap &data);
 
-    int m_firstVersion;
-    int m_lastVersion;
     QString m_userSuffix;
     QString m_sharedSuffix;