QmlDesigner: Improve version handling

Consolidate version handling in imports. It has drawbacks if we copy
code to different places in our code base. Having tests for the version
conversion has advantages too.

The invalid version has not anymore using -1 but the maximum integer as
indicator because an empty version is higher than any other version.

Change-Id: I8608b9509660f4253e8e5ba7397d964defe35e51
Reviewed-by: Thomas Hartmann <thomas.hartmann@qt.io>
Reviewed-by: Miikka Heikkinen <miikka.heikkinen@qt.io>
Reviewed-by: Tim Jenssen <tim.jenssen@qt.io>

src/plugins/qmldesigner/components/itemlibrary/itemlibrarymodel.cpp

@@ -304,28 +304,6 @@ Import ItemLibraryModel::entryToImport(const ItemLibraryEntry &entry)
 
 }
 
-// Returns true if first import version is higher or equal to second import version
-static bool compareVersions(const QString &version1, const QString &version2)
-{
-    if (version2.isEmpty() || version1 == version2)
-        return true;
-    const QStringList version1List = version1.split(QLatin1Char('.'));
-    const QStringList version2List = version2.split(QLatin1Char('.'));
-    if (version1List.count() == 2 && version2List.count() == 2) {
-        int major1 = version1List.constFirst().toInt();
-        int major2 = version2List.constFirst().toInt();
-        if (major1 > major2) {
-            return true;
-        } else if (major1 == major2) {
-            int minor1 = version1List.constLast().toInt();
-            int minor2 = version2List.constLast().toInt();
-            if (minor1 >= minor2)
-                return true;
-        }
-    }
-    return false;
-}
-
 void ItemLibraryModel::update(ItemLibraryInfo *itemLibraryInfo, Model *model)
 {
     if (!model)
@@ -363,7 +341,7 @@ void ItemLibraryModel::update(ItemLibraryInfo *itemLibraryInfo, Model *model)
                 addNew = false; // add only 1 Quick3DAssets import section
             } else if (oldImport && oldImport->importEntry().url() == import.url()) {
                 // Retain the higher version if multiples exist
-                if (compareVersions(oldImport->importEntry().version(), import.version()))
+                if (oldImport->importEntry().toVersion() >= import.toVersion() || import.hasVersion())
                     addNew = false;
                 else
                     delete oldImport;

src/plugins/qmldesigner/designercore/include/import.h

@@ -11,8 +11,37 @@
 
 #include "qmldesignercorelib_global.h"
 
+#include <limits>
+
 namespace QmlDesigner {
 
+class Version
+{
+public:
+    friend bool operator==(Version first, Version second)
+    {
+        return first.major == second.major && first.minor == second.minor;
+    }
+
+    friend bool operator<(Version first, Version second)
+    {
+        return std::tie(first.major, first.minor) < std::tie(second.major, second.minor);
+    }
+
+    friend bool operator>(Version first, Version second) { return second < first; }
+    friend bool operator<=(Version first, Version second) { return !(second < first); }
+    friend bool operator>=(Version first, Version second) { return !(first < second); }
+
+    bool isEmpty() const
+    {
+        return major == std::numeric_limits<int>::max() || minor == std::numeric_limits<int>::max();
+    }
+
+public:
+    int major = std::numeric_limits<int>::max();
+    int minor = std::numeric_limits<int>::max();
+};
+
 class QMLDESIGNERCORE_EXPORT Import
 {
 public:
@@ -41,6 +70,7 @@ public:
 
     int majorVersion() const;
     int minorVersion() const;
+    Version toVersion() const;
     static int majorFromVersion(const QString &version);
     static int minorFromVersion(const QString &version);
 

src/plugins/qmldesigner/designercore/model/abstractview.cpp

@@ -893,32 +893,28 @@ QmlTimeline AbstractView::currentTimeline() const
 
 static int getMinorVersionFromImport(const Model *model)
 {
-    const Imports imports = model->imports();
-    for (const Import &import : imports) {
-        if (import.isLibraryImport() && import.url() == "QtQuick") {
-            const QString versionString = import.version();
-            if (versionString.contains(".")) {
-                const QString minorVersionString = versionString.split(".").constLast();
-                return minorVersionString.toInt();
-            }
-        }
-    }
+    const Imports &imports = model->imports();
+
+    auto found = std::find_if(imports.begin(), imports.end(), [](const auto &import) {
+        return import.url() == "QtQuick";
+    });
+
+    if (found != imports.end())
+        return found->minorVersion();
 
     return -1;
 }
 
 static int getMajorVersionFromImport(const Model *model)
 {
-    const Imports imports = model->imports();
-    for (const Import &import : imports) {
-        if (import.isLibraryImport() && import.url() == QStringLiteral("QtQuick")) {
-            const QString versionString = import.version();
-            if (versionString.contains(QStringLiteral("."))) {
-                const QString majorVersionString = versionString.split(QStringLiteral(".")).constFirst();
-                return majorVersionString.toInt();
-            }
-        }
-    }
+    const Imports &imports = model->imports();
+
+    auto found = std::find_if(imports.begin(), imports.end(), [](const auto &import) {
+        return import.url() == "QtQuick";
+    });
+
+    if (found != imports.end())
+        return found->majorVersion();
 
     return -1;
 }

src/plugins/qmldesigner/designercore/model/import.cpp

@@ -82,6 +82,26 @@ int Import::minorVersion() const
     return minorFromVersion(m_version);
 }
 
+Version Import::toVersion() const
+{
+    auto found = std::find(m_version.begin(), m_version.end(), u'.');
+    if (found == m_version.end())
+        return {};
+
+    QStringView majorVersionToken{m_version.begin(), found};
+    bool canConvertMajor = false;
+    int majorVersion = majorVersionToken.toInt(&canConvertMajor);
+
+    QStringView minorVersionToken{std::next(found), m_version.end()};
+    bool canConvertMinor = false;
+    int minorVersion = minorVersionToken.toInt(&canConvertMinor);
+
+    if (canConvertMajor && canConvertMinor)
+        return {majorVersion, minorVersion};
+
+    return {};
+}
+
 int Import::majorFromVersion(const QString &version)
 {
     auto found = std::find(version.begin(), version.end(), u'.');

src/plugins/qmldesigner/designercore/model/model.cpp

@@ -1462,37 +1462,19 @@ void Model::setUsedImports(Imports usedImports)
     }
 }
 
-static bool compareVersions(const QString &version1, const QString &version2, bool allowHigherVersion)
+static bool compareVersions(const Import &import1, const Import &import2, bool allowHigherVersion)
 {
+    auto version1 = import1.toVersion();
+    auto version2 = import2.toVersion();
+
     if (version2.isEmpty())
         return true;
     if (version1 == version2)
         return true;
     if (!allowHigherVersion)
         return false;
-    QStringList version1List = version1.split(QLatin1Char('.'));
-    QStringList version2List = version2.split(QLatin1Char('.'));
-    if (version1List.count() == 2 && version2List.count() == 2) {
-        bool ok;
-        int major1 = version1List.constFirst().toInt(&ok);
-        if (!ok)
-            return false;
-        int major2 = version2List.constFirst().toInt(&ok);
-        if (!ok)
-            return false;
-        if (major1 >= major2) {
-            int minor1 = version1List.constLast().toInt(&ok);
-            if (!ok)
-                return false;
-            int minor2 = version2List.constLast().toInt(&ok);
-            if (!ok)
-                return false;
-            if (minor1 >= minor2)
-                return true;
-        }
-    }
 
-    return false;
+    return version1 >= version2;
 }
 
 bool Model::hasImport(const Import &import, bool ignoreAlias, bool allowHigherVersion) const
@@ -1510,7 +1492,7 @@ bool Model::hasImport(const Import &import, bool ignoreAlias, bool allowHigherVe
         }
         if (existingImport.isLibraryImport() && import.isLibraryImport()) {
             if (existingImport.url() == import.url()
-                && compareVersions(existingImport.version(), import.version(), allowHigherVersion)) {
+                && compareVersions(existingImport, import, allowHigherVersion)) {
                 return true;
             }
         }
@@ -1684,7 +1666,7 @@ bool Model::isImportPossible(const Import &import, bool ignoreAlias, bool allowH
 
         if (possibleImport.isLibraryImport() && import.isLibraryImport()) {
             if (possibleImport.url() == import.url()
-                && compareVersions(possibleImport.version(), import.version(), allowHigherVersion)) {
+                && compareVersions(possibleImport, import, allowHigherVersion)) {
                 return true;
             }
         }
@@ -1719,7 +1701,7 @@ Import Model::highestPossibleImport(const QString &importPath)
 
     for (const Import &import : possibleImports()) {
         if (import.url() == importPath) {
-            if (candidate.isEmpty() || compareVersions(import.version(), candidate.version(), true))
+            if (candidate.isEmpty() || compareVersions(import, candidate, true))
                 candidate = import;
         }
     }

src/plugins/qmldesigner/designercore/model/texttomodelmerger.cpp

@@ -19,6 +19,7 @@
 #include "signalhandlerproperty.h"
 #include "variantproperty.h"
 #include <externaldependenciesinterface.h>
+#include <import.h>
 #include <projectstorage/modulescanner.h>
 #include <rewritingexception.h>
 
@@ -61,12 +62,15 @@ bool isSupportedAttachedProperties(const QString &propertyName)
            || propertyName.startsWith(QLatin1String("InsightCategory."));
 }
 
-QStringList supportedVersionsList()
+bool isSupportedVersion(QmlDesigner::Version version)
 {
-    static const QStringList list = {"2.0", "2.1", "2.2",  "2.3",  "2.4",  "2.5",  "2.6",  "2.7",
-                                     "2.8", "2.9", "2.10", "2.11", "2.12", "2.13", "2.14", "2.15",
-                                     "6.0", "6.1", "6.2",  "6.3",  "6.4",  "6.5"};
-    return list;
+    if (version.major == 2)
+        return version.minor <= 15;
+
+    if (version.major == 6)
+        return version.minor <= 5;
+
+    return false;
 }
 
 QStringList globalQtEnums()
@@ -106,9 +110,10 @@ QStringList knownEnumScopes()
     return list;
 }
 
-bool supportedQtQuickVersion(const QString &version)
+bool supportedQtQuickVersion(const QmlDesigner::Import &import)
 {
-    return version.isEmpty() || supportedVersionsList().contains(version);
+    auto version = import.toVersion();
+    return version.isEmpty() || isSupportedVersion(version);
 }
 
 QString stripQuotes(const QString &str)
@@ -120,7 +125,7 @@ QString stripQuotes(const QString &str)
     return str;
 }
 
-inline QString deEscape(const QString &value)
+QString deEscape(const QString &value)
 {
     QString result = value;
 
@@ -2165,13 +2170,12 @@ void TextToModelMerger::collectImportErrors(QList<DocumentMessage> *errors)
     bool hasQtQuick = false;
     for (const QmlDesigner::Import &import : m_rewriterView->model()->imports()) {
         if (import.isLibraryImport() && import.url() == QStringLiteral("QtQuick")) {
-
-            if (supportedQtQuickVersion(import.version())) {
+            if (supportedQtQuickVersion(import)) {
                 hasQtQuick = true;
 
                 auto &externalDependencies = m_rewriterView->externalDependencies();
                 if (externalDependencies.hasStartupTarget()) {
-                    const bool qt6import = import.version().startsWith("6");
+                    const bool qt6import = !import.hasVersion() || import.majorVersion() == 6;
 
                     if (!externalDependencies.isQt6Import() && (m_hasVersionlessImport || qt6import)) {
                         const QmlJS::DiagnosticMessage diagnosticMessage(

tests/unit/unittest/CMakeLists.txt

@@ -53,6 +53,7 @@ add_qtc_test(unittest GTEST
     gtest-qt-printing.cpp gtest-qt-printing.h
     gtest-std-printing.h
     lastchangedrowid-test.cpp
+    import-test.cpp
     matchingtext-test.cpp
     mockfutureinterface.h
     mockmutex.h

tests/unit/unittest/google-using-declarations.h

@@ -24,6 +24,7 @@ using testing::EndsWith;
 using testing::Eq;
 using testing::Exactly;
 using testing::Field;
+using testing::FieldsAre;
 using testing::Ge;
 using testing::Gt;
 using testing::HasSubstr;

tests/unit/unittest/import-test.cpp

@@ -0,0 +1,340 @@
+// Copyright (C) 2023 The Qt Company Ltd.
+// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only WITH Qt-GPL-exception-1.0
+
+#include "googletest.h"
+
+#include <import.h>
+
+namespace {
+
+TEST(Import, ParseVersion)
+{
+    auto import = QmlDesigner::Import::createLibraryImport("Qml", "6.5");
+
+    auto version = import.toVersion();
+
+    ASSERT_THAT(version, FieldsAre(6, 5));
+}
+
+TEST(Import, ParseMajorVersion)
+{
+    auto import = QmlDesigner::Import::createLibraryImport("Qml", "6.5");
+
+    auto version = import.majorVersion();
+
+    ASSERT_THAT(version, 6);
+}
+
+TEST(Import, MajorVersionIsInvalidForEmptyString)
+{
+    auto import = QmlDesigner::Import::createLibraryImport("Qml");
+
+    auto version = import.majorVersion();
+
+    ASSERT_THAT(version, -1);
+}
+
+TEST(Import, MajorVersionIsInvalidForBrokenString)
+{
+    auto import = QmlDesigner::Import::createLibraryImport("Qml", "6,5");
+
+    auto version = import.majorVersion();
+
+    ASSERT_THAT(version, -1);
+}
+
+TEST(Import, ParseMinorVersion)
+{
+    auto import = QmlDesigner::Import::createLibraryImport("Qml", "6.5");
+
+    auto version = import.minorVersion();
+
+    ASSERT_THAT(version, 5);
+}
+
+TEST(Import, MinorVersionIsInvalidForEmptyString)
+{
+    auto import = QmlDesigner::Import::createLibraryImport("Qml");
+
+    auto version = import.minorVersion();
+
+    ASSERT_THAT(version, -1);
+}
+
+TEST(Import, MinorVersionIsInvalidForBrokenString)
+{
+    auto import = QmlDesigner::Import::createLibraryImport("Qml", "6,5");
+
+    auto version = import.minorVersion();
+
+    ASSERT_THAT(version, -1);
+}
+
+TEST(Import, VersionIsNotEmpty)
+{
+    auto import = QmlDesigner::Import::createLibraryImport("Qml", "6.5");
+
+    auto version = import.toVersion();
+
+    ASSERT_FALSE(version.isEmpty());
+}
+
+TEST(Import, BrokenVersionStringIsEmptyVersion)
+{
+    auto import = QmlDesigner::Import::createLibraryImport("Qml", "6");
+
+    auto version = import.toVersion();
+
+    ASSERT_TRUE(version.isEmpty());
+}
+
+TEST(Import, EmptyVersionStringIsEmptyVersion)
+{
+    auto import = QmlDesigner::Import::createLibraryImport("Qml");
+
+    auto version = import.toVersion();
+
+    ASSERT_TRUE(version.isEmpty());
+}
+
+TEST(Import, SameVersionsAreEqual)
+{
+    QmlDesigner::Version version1{6, 5};
+    QmlDesigner::Version version2{6, 5};
+
+    bool isEqual = version1 == version2;
+
+    ASSERT_TRUE(isEqual);
+}
+
+TEST(Import, InvalidVersionsAreEqual)
+{
+    QmlDesigner::Version version1;
+    QmlDesigner::Version version2;
+
+    bool isEqual = version1 == version2;
+
+    ASSERT_TRUE(isEqual);
+}
+
+TEST(Import, DifferentMinorVersionsAreNotEqual)
+{
+    QmlDesigner::Version version1{6, 4};
+    QmlDesigner::Version version2{6, 5};
+
+    bool isEqual = version1 == version2;
+
+    ASSERT_FALSE(isEqual);
+}
+
+TEST(Import, DifferentMajorVersionsAreNotEqual)
+{
+    QmlDesigner::Version version1{5, 5};
+    QmlDesigner::Version version2{6, 5};
+
+    bool isEqual = version1 == version2;
+
+    ASSERT_FALSE(isEqual);
+}
+
+TEST(Import, LessMinorVersionsAreLess)
+{
+    QmlDesigner::Version version1{6, 4};
+    QmlDesigner::Version version2{6, 5};
+
+    bool isLess = version1 < version2;
+
+    ASSERT_TRUE(isLess);
+}
+
+TEST(Import, LessMajorVersionsAreLess)
+{
+    QmlDesigner::Version version1{5, 15};
+    QmlDesigner::Version version2{6, 5};
+
+    bool isLess = version1 < version2;
+
+    ASSERT_TRUE(isLess);
+}
+
+TEST(Import, SameVersionsAreNotLess)
+{
+    QmlDesigner::Version version1{6, 5};
+    QmlDesigner::Version version2{6, 5};
+
+    bool isLess = version1 < version2;
+
+    ASSERT_FALSE(isLess);
+}
+
+TEST(Import, EmptyVersionIsNotLess)
+{
+    QmlDesigner::Version version1;
+    QmlDesigner::Version version2{6, 5};
+
+    bool isLess = version1 < version2;
+
+    ASSERT_FALSE(isLess);
+}
+
+TEST(Import, NonEmptyVersionIsIsLessThanEmptyVersion)
+{
+    QmlDesigner::Version version1{6, 5};
+    QmlDesigner::Version version2;
+
+    bool isLess = version1 < version2;
+
+    ASSERT_TRUE(isLess);
+}
+
+TEST(Import, GreaterMinorVersionsAreGreater)
+{
+    QmlDesigner::Version version1{6, 6};
+    QmlDesigner::Version version2{6, 5};
+
+    bool isGreater = version1 > version2;
+
+    ASSERT_TRUE(isGreater);
+}
+
+TEST(Import, GreaterMajorVersionsAreGreater)
+{
+    QmlDesigner::Version version1{6, 5};
+    QmlDesigner::Version version2{5, 15};
+
+    bool isGreater = version1 > version2;
+
+    ASSERT_TRUE(isGreater);
+}
+
+TEST(Import, SameVersionsAreNotGreater)
+{
+    QmlDesigner::Version version1{6, 5};
+    QmlDesigner::Version version2{6, 5};
+
+    bool isGreater = version1 > version2;
+
+    ASSERT_FALSE(isGreater);
+}
+
+TEST(Import, EmptyVersionIsGreater)
+{
+    QmlDesigner::Version version1;
+    QmlDesigner::Version version2{6, 5};
+
+    bool isGreater = version1 > version2;
+
+    ASSERT_TRUE(isGreater);
+}
+
+TEST(Import, NonEmptyVersionIsIsNotGreaterThanEmptyVersion)
+{
+    QmlDesigner::Version version1{6, 5};
+    QmlDesigner::Version version2;
+
+    bool isGreater = version1 > version2;
+
+    ASSERT_FALSE(isGreater);
+}
+
+TEST(Import, LessEqualMinorVersionsAreLessEqual)
+{
+    QmlDesigner::Version version1{6, 4};
+    QmlDesigner::Version version2{6, 5};
+
+    bool isLessEqual = version1 <= version2;
+
+    ASSERT_TRUE(isLessEqual);
+}
+
+TEST(Import, LessqualMajorVersionsAreLessqual)
+{
+    QmlDesigner::Version version1{5, 15};
+    QmlDesigner::Version version2{6, 5};
+
+    bool isLessEqual = version1 <= version2;
+
+    ASSERT_TRUE(isLessEqual);
+}
+
+TEST(Import, SameVersionsAreLessqual)
+{
+    QmlDesigner::Version version1{6, 5};
+    QmlDesigner::Version version2{6, 5};
+
+    bool isLessEqual = version1 <= version2;
+
+    ASSERT_TRUE(isLessEqual);
+}
+
+TEST(Import, EmptyVersionIsNotLessqual)
+{
+    QmlDesigner::Version version1;
+    QmlDesigner::Version version2{6, 5};
+
+    bool isLessEqual = version1 <= version2;
+
+    ASSERT_FALSE(isLessEqual);
+}
+
+TEST(Import, NonEmptyVersionIsIsLessqualThanEmptyVersion)
+{
+    QmlDesigner::Version version1{6, 5};
+    QmlDesigner::Version version2;
+
+    bool isLessEqual = version1 <= version2;
+
+    ASSERT_TRUE(isLessEqual);
+}
+
+TEST(Import, GreaterEqualMinorVersionsAreGreaterEqual)
+{
+    QmlDesigner::Version version1{6, 6};
+    QmlDesigner::Version version2{6, 5};
+
+    bool isGreaterEqual = version1 >= version2;
+
+    ASSERT_TRUE(isGreaterEqual);
+}
+
+TEST(Import, GreaterEqualMajorVersionsAreGreaterEqual)
+{
+    QmlDesigner::Version version1{6, 5};
+    QmlDesigner::Version version2{5, 15};
+
+    bool isGreaterEqual = version1 >= version2;
+
+    ASSERT_TRUE(isGreaterEqual);
+}
+
+TEST(Import, SameVersionsAreGreaterEqual)
+{
+    QmlDesigner::Version version1{6, 5};
+    QmlDesigner::Version version2{6, 5};
+
+    bool isGreaterEqual = version1 >= version2;
+
+    ASSERT_TRUE(isGreaterEqual);
+}
+
+TEST(Import, EmptyVersionIsGreaterEqual)
+{
+    QmlDesigner::Version version1;
+    QmlDesigner::Version version2{6, 5};
+
+    bool isGreaterEqual = version1 >= version2;
+
+    ASSERT_TRUE(isGreaterEqual);
+}
+
+TEST(Import, NonEmptyVersionIsIsNotGreaterEqualThanEmptyVersion)
+{
+    QmlDesigner::Version version1{6, 5};
+    QmlDesigner::Version version2;
+
+    bool isGreaterEqual = version1 >= version2;
+
+    ASSERT_FALSE(isGreaterEqual);
+}
+
+} // namespace