From 0269503dd890e72cb9e0756b187797e72a365d7e Mon Sep 17 00:00:00 2001 From: Tobias Hunger Date: Mon, 20 Nov 2017 16:19:05 +0100 Subject: [PATCH] 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 --- src/libs/utils/settingsaccessor.cpp | 42 ++++++++++------------ src/libs/utils/settingsaccessor.h | 4 ++- tests/auto/utils/settings/tst_settings.cpp | 27 +++++++++++--- 3 files changed, 44 insertions(+), 29 deletions(-) diff --git a/src/libs/utils/settingsaccessor.cpp b/src/libs/utils/settingsaccessor.cpp index 5493be88b8d..eb9deac61f6 100644 --- a/src/libs/utils/settingsaccessor.cpp +++ b/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) - return false; - if (newEnv == id) - return true; - } - } + if (!newValid) + return false; 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(); diff --git a/src/libs/utils/settingsaccessor.h b/src/libs/utils/settingsaccessor.h index 7b6f190ff10..dc964946661 100644 --- a/src/libs/utils/settingsaccessor.h +++ b/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; diff --git a/tests/auto/utils/settings/tst_settings.cpp b/tests/auto/utils/settings/tst_settings.cpp index 12cec1a44d7..766d5732890 100644 --- a/tests/auto/utils/settings/tst_settings.cpp +++ b/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)); }