QmlDesigner: Remove Model::generateIdFromName()

- Added UniqueName::getId()
- Cleaned up generateNewId() and made it use UniqueName::getId()
- Moved UniqueName to designercore

Also reversed how the predicate of UniqueName::get() works.

Change-Id: I89c50f7d80610243f56be165b1495ef428da457c
Reviewed-by: <github-actions-qt-creator@cristianadam.eu>
Reviewed-by: Qt CI Patch Build Bot <ci_patchbuild_bot@qt.io>
Reviewed-by: Miikka Heikkinen <miikka.heikkinen@qt.io>

src/plugins/qmldesigner/CMakeLists.txt

@@ -46,7 +46,6 @@ add_qtc_library(QmlDesignerUtils STATIC
     hdrimage.cpp hdrimage.h
     ktximage.cpp ktximage.h
     imageutils.cpp imageutils.h
-    uniquename.cpp uniquename.h
     qmldesignerutils_global.h
 )
 
@@ -96,10 +95,9 @@ add_qtc_library(QmlDesignerCore STATIC
     ${CMAKE_CURRENT_LIST_DIR}/designercore/include
   SOURCES_PREFIX ${CMAKE_CURRENT_LIST_DIR}/designercore
   SOURCES
-    rewritertransaction.cpp
+    rewritertransaction.cpp rewritertransaction.h
-    rewritertransaction.h
+    generatedcomponentutils.cpp generatedcomponentutils.h
-    generatedcomponentutils.cpp
+    uniquename.cpp uniquename.h
-    generatedcomponentutils.h
 )
 
 extend_qtc_library(QmlDesignerCore

src/plugins/qmldesigner/components/assetslibrary/assetslibrarymodel.cpp

@@ -5,6 +5,7 @@
 
 #include <modelnodeoperations.h>
 #include <qmldesignerplugin.h>
+#include <uniquename.h>
 
 #include <coreplugin/icore.h>
 
@@ -12,7 +13,6 @@
 #include <utils/asset.h>
 #include <utils/filepath.h>
 #include <utils/filesystemwatcher.h>
-#include <utils/uniquename.h>
 
 #include <QFileInfo>
 #include <QFileSystemModel>

src/plugins/qmldesigner/components/assetslibrary/assetslibrarywidget.cpp

@@ -17,6 +17,7 @@
 #include <qmldesignerplugin.h>
 #include <studioquickwidget.h>
 #include <theme.h>
+#include <uniquename.h>
 #include <utils3d.h>
 
 #include <coreplugin/fileutils.h>
@@ -28,7 +29,6 @@
 #include <utils/environment.h>
 #include <utils/filepath.h>
 #include <utils/qtcassert.h>
-#include <utils/uniquename.h>
 
 #include <QFileDialog>
 #include <QFileInfo>

src/plugins/qmldesigner/components/collectioneditor/collectionview.cpp

@@ -242,7 +242,7 @@ void CollectionView::addResource(const QUrl &url, const QString &name)
         VariantProperty nameProperty = resourceNode.variantProperty("objectName");
         sourceProperty.setValue(sourceAddress);
         nameProperty.setValue(name);
-        resourceNode.setIdWithoutRefactoring(model()->generateIdFromName(name, "model"));
+        resourceNode.setIdWithoutRefactoring(model()->generateNewId(name, "model"));
         rootModelNode().defaultNodeAbstractProperty().reparentHere(resourceNode);
     });
 }

src/plugins/qmldesigner/components/contentlibrary/contentlibraryusermodel.cpp

@@ -13,10 +13,10 @@
 #include <designerpaths.h>
 #include <imageutils.h>
 #include <qmldesignerplugin.h>
+#include <uniquename.h>
 
 #include <utils/algorithm.h>
 #include <utils/qtcassert.h>
-#include <utils/uniquename.h>
 
 #include <QFileInfo>
 #include <QJsonArray>
@@ -318,11 +318,11 @@ QPair<QString, QString> ContentLibraryUserModel::getUniqueLibItemNames(const QSt
     baseQml.prepend("My");
 
     QString uniqueQml = UniqueName::get(baseQml, [&] (const QString &name) {
-        return !itemQmls.contains(name);
+        return itemQmls.contains(name);
     });
 
     QString uniqueIcon = UniqueName::get(defaultName, [&] (const QString &name) {
-        return !itemIcons.contains(name);
+        return itemIcons.contains(name);
     });
 
     return {uniqueQml + ".qml", uniqueIcon + ".png"};

src/plugins/qmldesigner/components/contentlibrary/contentlibraryview.cpp

@@ -779,7 +779,7 @@ ModelNode ContentLibraryView::createMaterial(const TypeName &typeName)
     QString newName = QString::fromUtf8(typeName).replace(rgx, " \\1\\2").trimmed();
     if (newName.endsWith(" Material"))
         newName.chop(9); // remove trailing " Material"
-    QString newId = model()->generateIdFromName(newName, "material");
+    QString newId = model()->generateNewId(newName, "material");
     newMatNode.setIdWithRefactoring(newId);
 
     VariantProperty objNameProp = newMatNode.variantProperty("objectName");
@@ -805,7 +805,7 @@ ModelNode ContentLibraryView::createMaterial(const NodeMetaInfo &metaInfo)
     QString newName = QString::fromLatin1(metaInfo.simplifiedTypeName()).replace(rgx, " \\1\\2").trimmed();
     if (newName.endsWith(" Material"))
         newName.chop(9); // remove trailing " Material"
-    QString newId = model()->generateIdFromName(newName, "material");
+    QString newId = model()->generateNewId(newName, "material");
     newMatNode.setIdWithRefactoring(newId);
 
     VariantProperty objNameProp = newMatNode.variantProperty("objectName");

src/plugins/qmldesigner/components/materialeditor/materialeditorview.cpp

@@ -961,7 +961,7 @@ void MaterialEditorView::renameMaterial(ModelNode &material, const QString &newN
         return;
 
     executeInTransaction(__FUNCTION__, [&] {
-        material.setIdWithRefactoring(model()->generateIdFromName(newName, "material"));
+        material.setIdWithRefactoring(model()->generateNewId(newName, "material"));
 
         VariantProperty objNameProp = material.variantProperty("objectName");
         objNameProp.setValue(newName);
@@ -998,7 +998,7 @@ void MaterialEditorView::duplicateMaterial(const ModelNode &material)
         QString newName = sourceMat.modelNode().variantProperty("objectName").value().toString() + " copy";
         VariantProperty objNameProp = duplicateMatNode.variantProperty("objectName");
         objNameProp.setValue(newName);
-        duplicateMatNode.setIdWithoutRefactoring(model()->generateIdFromName(newName, "material"));
+        duplicateMatNode.setIdWithoutRefactoring(model()->generateNewId(newName, "material"));
 
         // sync properties. Only the base state is duplicated.
         const QList<AbstractProperty> props = material.properties();

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

@@ -255,10 +255,8 @@ public:
     bool hasId(const QString &id) const;
     bool hasImport(const QString &importUrl) const;
 
-    QString generateNewId(const QString &prefixName,
+    QString generateNewId(const QString &prefixName, const QString &fallbackPrefix = "element",
-                          const QString &fallbackPrefix = "element",
                           std::optional<std::function<bool(const QString &)>> isDuplicate = {}) const;
-    QString generateIdFromName(const QString &name, const QString &fallbackId = "element") const;
 
     void startDrag(QMimeData *mimeData, const QPixmap &icon);
     void endDrag();

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

@@ -9,7 +9,6 @@
 #include "../projectstorage/sourcepath.h"
 #include "../projectstorage/sourcepathcache.h"
 #include "abstractview.h"
-#include "auxiliarydataproperties.h"
 #include "internalbindingproperty.h"
 #include "internalnodeabstractproperty.h"
 #include "internalnodelistproperty.h"
@@ -33,6 +32,8 @@
 #include "signalhandlerproperty.h"
 #include "variantproperty.h"
 
+#include <uniquename.h>
+
 #include <projectstorage/projectstorage.h>
 
 #include <qmljs/qmljsmodelmanagerinterface.h>
@@ -46,8 +47,6 @@
 #include <QRegularExpression>
 #include <qcompilerdetection.h>
 
-#include <string>
-
 /*!
 \defgroup CoreModel
 */
@@ -1864,90 +1863,22 @@ bool Model::hasImport(const QString &importUrl) const
     });
 }
 
-static QString firstCharToLower(const QString &string)
+QString Model::generateNewId(const QString &prefixName, const QString &fallbackPrefix,
-{
-    QString resultString = string;
-
-    if (!resultString.isEmpty())
-        resultString[0] = resultString.at(0).toLower();
-
-    return resultString;
-}
-
-QString Model::generateNewId(const QString &prefixName,
-                             const QString &fallbackPrefix,
                              std::optional<std::function<bool(const QString &)>> isDuplicate) const
 {
-    // First try just the prefixName without number as postfix, then continue with 2 and further
+    QString newId = prefixName;
-    // as postfix until id does not already exist.
-    // Properties of the root node are not allowed for ids, because they are available in the
-    // complete context without qualification.
 
-    int counter = 0;
+    if (newId.isEmpty())
+        newId = fallbackPrefix;
 
-    static const QRegularExpression nonWordCharsRegex("\\W");
+    if (!isDuplicate.has_value()) // TODO: to be removed separately to not break build
-    QString newBaseId = firstCharToLower(prefixName);
-    newBaseId.remove(nonWordCharsRegex);
-
-    if (!newBaseId.isEmpty()) {
-        QChar firstChar = newBaseId.at(0);
-        if (firstChar.isDigit())
-            newBaseId.prepend('_');
-    } else {
-        newBaseId = fallbackPrefix;
-    }
-
-    QString newId = newBaseId;
-
-    if (!isDuplicate.has_value())
         isDuplicate = std::bind(&Model::hasId, this, std::placeholders::_1);
 
-    while (!ModelNode::isValidId(newId) || isDuplicate.value()(newId)
+    return UniqueName::getId(prefixName, [&] (const QString &id) {
-           || d->rootNode()->property(newId.toUtf8())) {
+        // Properties of the root node are not allowed for ids, because they are available in the
-        ++counter;
+        // complete context without qualification.
-        newId = QStringView(u"%1%2").arg(firstCharToLower(newBaseId)).arg(counter);
+        return isDuplicate.value()(id) || d->rootNode()->property(id.toUtf8());
-    }
+    });
-
-    return newId;
-}
-
-// Generate a unique camelCase id from a name
-// note: this methods does the same as generateNewId(). The 2 methods should be merged into one
-QString Model::generateIdFromName(const QString &name, const QString &fallbackId) const
-{
-    QString newId;
-    if (name.isEmpty()) {
-        newId = fallbackId;
-    } else {
-        // convert to camel case
-        QStringList nameWords = name.split(" ");
-        nameWords[0] = nameWords[0].at(0).toLower() + nameWords[0].mid(1);
-        for (int i = 1; i < nameWords.size(); ++i)
-            nameWords[i] = nameWords[i].at(0).toUpper() + nameWords[i].mid(1);
-        newId = nameWords.join("");
-
-        // if id starts with a number prepend an underscore
-        if (newId.at(0).isDigit())
-            newId.prepend('_');
-    }
-
-    // If the new id is not valid (e.g. qml keyword match), try fixing it by prepending underscore
-    if (!ModelNode::isValidId(newId))
-        newId.prepend("_");
-
-    QRegularExpression rgx("\\d+$"); // matches a number at the end of a string
-    while (hasId(newId)) { // id exists
-        QRegularExpressionMatch match = rgx.match(newId);
-        if (match.hasMatch()) { // ends with a number, increment it
-            QString numStr = match.captured();
-            int num = numStr.toInt() + 1;
-            newId = newId.mid(0, match.capturedStart()) + QString::number(num);
-        } else {
-            newId.append('1');
-        }
-    }
-
-    return newId;
 }
 
 void Model::startDrag(QMimeData *mimeData, const QPixmap &icon)

src/plugins/qmldesigner/designercore/uniquename.cpp

@@ -0,0 +1,121 @@
+// Copyright (C) 2024 The Qt Company Ltd.
+// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only WITH Qt-GPL-exception-1.0
+
+#include "uniquename.h"
+
+#include <QFileInfo>
+#include <QRegularExpression>
+
+namespace QmlDesigner {
+
+/**
+ * @brief Generates a unique name based on the provided name.
+ *
+ * This method iteratively generates a name by appending suffixes until a unique name is found.
+ * The uniqueness of the generated name is determined by the provided predicate function.
+ *
+ * @param name The original name to be made unique.
+ * @param predicate A function that checks if a name exists. Returns true if the name exists,
+ *        false if name is unique.
+ * @return A unique name derived from the provided name.
+ */
+QString UniqueName::get(const QString &name, std::function<bool(const QString &)> predicate)
+{
+    if (!predicate(name))
+        return name;
+
+    // match prefix and number (including zero padding) parts
+    static QRegularExpression rgx("(\\D*?)(\\d+)$");
+    QRegularExpressionMatch match = rgx.match(name);
+
+    QString prefix;
+    int number = 0;
+    int padding = 0;
+
+    if (match.hasMatch()) {
+        // Split the name into prefix and number
+        prefix = match.captured(1);
+        QString numberStr = match.captured(2);
+        number = numberStr.toInt();
+        padding = numberStr.size();
+    } else {
+        prefix = name;
+    }
+
+    QString nameTemplate = "%1%2";
+    QString newName;
+    do {
+        newName = nameTemplate.arg(prefix).arg(++number, padding, 10, QChar('0'));
+    } while (predicate(newName));
+
+    return newName;
+}
+
+/**
+ * @brief Generates a unique path based on the provided path. If the path belongs to a file, the
+ * filename or if it's a directory, the directory name will be adjusted to ensure uniqueness.
+ *
+ * This method appends a numerical suffix (or increment it if it exists) to the filename or
+ * directory name if necessary to make it unique.
+ *
+ * @param path The original path to be made unique.
+ * @return A unique path derived from the provided path.
+ */
+QString UniqueName::getPath(const QString &path)
+{
+    // Remove the trailing slash if it exists (otherwise QFileInfo::path() returns empty)
+    QString adjustedPath = path;
+    if (adjustedPath.endsWith('/'))
+        adjustedPath.chop(1);
+
+    QFileInfo fileInfo = QFileInfo(adjustedPath);
+    QString baseName = fileInfo.baseName();
+    QString suffix = fileInfo.completeSuffix();
+    if (!suffix.isEmpty())
+        suffix.prepend('.');
+
+    QString parentDir = fileInfo.path();
+    QString pathTemplate = parentDir + "/%1" + suffix;
+
+    QString uniqueBaseName = UniqueName::get(baseName, [&] (const QString &currName) {
+        return !QFileInfo::exists(pathTemplate.arg(currName));
+    });
+
+    return pathTemplate.arg(uniqueBaseName);
+}
+
+/**
+ * @brief Generates a unique ID based on the provided id
+ *
+ *  This works similar to get() with additional restrictions:
+ *  - Removes non-Latin1 characters
+ *  - Removes spaces
+ *  - Ensures the first letter is lowercase
+ *  - Converts spaces to camel case
+ *  - Prepends an underscore if id starts with a number or is a reserved word
+ *
+ * @param id The original id to be made unique.
+ * @return A unique Id (when predicate() returns false)
+ */
+QString UniqueName::getId(const QString &id, std::function<bool(const QString &)> predicate)
+{
+    // remove non word (non A-Z, a-z, 0-9) characters
+    static const QRegularExpression nonWordCharsRgx("\\W");
+    QString newId = id.simplified();
+    newId.remove(nonWordCharsRgx);
+
+    // convert to camel case
+    QStringList idParts = newId.split(" ");
+    idParts[0] = idParts[0].at(0).toLower() + idParts[0].mid(1);
+    for (int i = 1; i < idParts.size(); ++i)
+        idParts[i] = idParts[i].at(0).toUpper() + idParts[i].mid(1);
+    newId = idParts.join("");
+
+    // prepend _ if starts with a digit or invalid id (such as reserved words)
+    if (newId.at(0).isDigit() || std::binary_search(keywords.begin(), keywords.end(), newId))
+        newId.prepend('_');
+
+    return UniqueName::get(newId, predicate);
+}
+
+} // namespace QmlDesigner

src/plugins/qmldesigner/designercore/uniquename.h

@@ -0,0 +1,32 @@
+// Copyright (C) 2024 The Qt Company Ltd.
+// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only WITH Qt-GPL-exception-1.0
+
+#pragma once
+
+#include <qmldesignercorelib_exports.h>
+
+#include <QString>
+#include <QStringList>
+
+namespace QmlDesigner {
+
+class QMLDESIGNERCORE_EXPORT UniqueName
+{
+public:
+    static QString get(const QString &name, std::function<bool(const QString &)> predicate);
+    static QString getPath(const QString &path);
+    static QString getId(const QString &id, std::function<bool(const QString &)> predicate);
+
+private:
+    static inline const QStringList keywords {
+        "anchors", "as", "baseState", "border", "bottom", "break", "case", "catch", "clip", "color",
+        "continue", "data", "debugger", "default", "delete", "do", "else", "enabled", "finally",
+        "flow", "focus", "font", "for", "function", "height", "if", "import", "in", "instanceof",
+        "item", "layer", "left", "margin", "new", "opacity", "padding", "parent", "print", "rect",
+        "return", "right", "scale", "shaderInfo", "source", "sprite", "spriteSequence", "state",
+        "switch", "text", "this", "throw", "top", "try", "typeof", "var", "visible", "void",
+        "while", "with", "x", "y"
+    };
+};
+
+} // namespace QmlDesigner

src/plugins/qmldesigner/utils/uniquename.cpp

@@ -1,93 +0,0 @@
-// Copyright (C) 2024 The Qt Company Ltd.
-// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only WITH Qt-GPL-exception-1.0
-
-#include "uniquename.h"
-
-#include <QFileInfo>
-#include <QRegularExpression>
-
-namespace QmlDesigner {
-
-/**
- * @brief Generates a unique name based on the provided name.
- *
- * This method iteratively generates a name by appending suffixes until a unique name is found.
- * The uniqueness of the generated name is determined by the provided predicate function.
- *
- * @param oldName The original name to be made unique.
- * @param predicate A function that checks if a name is unique. Returns true if the name is unique,
- *        false otherwise.
- * @return A unique name derived from the provided name.
- */
-QString UniqueName::get(const QString &oldName, std::function<bool(const QString &)> predicate)
-{
-    QString newName = oldName;
-    while (!predicate(newName))
-        newName = nextName(newName);
-
-    return newName;
-}
-
-/**
- * @brief Generates a unique path based on the provided path. If the path belongs to a file, the
- * filename or if it's a directory, the directory name will be adjusted to ensure uniqueness.
- *
- * This method appends a numerical suffix (or increment it if it exists) to the filename or
- * directory name if necessary to make it unique.
- *
- * @param path The original path to be made unique.
- * @return A unique path derived from the provided path.
- */
-QString UniqueName::getPath(const QString &path)
-{
-    // Remove the trailing slash if it exists (otherwise QFileInfo::path() returns empty)
-    QString adjustedPath = path;
-    if (adjustedPath.endsWith('/'))
-        adjustedPath.chop(1);
-
-    QFileInfo fileInfo = QFileInfo(adjustedPath);
-    QString baseName = fileInfo.baseName();
-    QString suffix = fileInfo.completeSuffix();
-    if (!suffix.isEmpty())
-        suffix.prepend('.');
-
-    QString parentDir = fileInfo.path();
-    QString pathTemplate = parentDir + "/%1" + suffix;
-
-    QString uniqueBaseName = UniqueName::get(baseName, [&] (const QString &currName) {
-        return !QFileInfo::exists(pathTemplate.arg(currName));
-    });
-
-    return pathTemplate.arg(uniqueBaseName);
-}
-
-QString UniqueName::nextName(const QString &oldName)
-{
-    static QRegularExpression rgx("\\d+$"); // match a number at the end of a string
-
-    QString uniqueName = oldName;
-    // if the name ends with a number, increment it
-    QRegularExpressionMatch match = rgx.match(uniqueName);
-    if (match.hasMatch()) { // ends with a number
-        QString numStr = match.captured(0);
-        int num = numStr.toInt() + 1;
-
-        // get number of padding zeros, ex: for "005" = 2
-        int nPaddingZeros = 0;
-        for (; nPaddingZeros < numStr.size() && numStr[nPaddingZeros] == '0'; ++nPaddingZeros);
-
-        // if the incremented number's digits increased, decrease the padding zeros
-        if (std::fmod(std::log10(num), 1.0) == 0)
-            --nPaddingZeros;
-
-        uniqueName = oldName.left(match.capturedStart())
-                     + QString('0').repeated(nPaddingZeros)
-                     + QString::number(num);
-    } else {
-        uniqueName = oldName + '1';
-    }
-
-    return uniqueName;
-}
-
-} // namespace QmlDesigner

src/plugins/qmldesigner/utils/uniquename.h

@@ -1,20 +0,0 @@
-// Copyright (C) 2024 The Qt Company Ltd.
-// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only WITH Qt-GPL-exception-1.0
-
-#pragma once
-
-#include <QString>
-
-namespace QmlDesigner {
-
-class UniqueName
-{
-public:
-    static QString get(const QString &oldName, std::function<bool(const QString &)> predicate);
-    static QString getPath(const QString &oldName);
-
-private:
-    static QString nextName(const QString &oldName);
-};
-
-} // namespace QmlDesigner

tests/unit/tests/testdesignercore/CMakeLists.txt

@@ -147,6 +147,8 @@ add_qtc_library(TestDesignerCore OBJECT
     tracing/qmldesignertracing.cpp tracing/qmldesignertracing.h
     rewritertransaction.cpp
     rewritertransaction.h
+    uniquename.cpp
+    uniquename.h
 )
 
 extend_qtc_library(TestDesignerCore