Fix review comments. Use QImage for QML and QPixmap for rest of code. Do not cache images for QML. Fix tests. Use signal in QML.

Signed-off-by: alex-z <blackslayer4@gmail.com>

src/gui/iconutils.cpp

@@ -25,125 +25,157 @@
 namespace {
 QString findSvgFilePath(const QString &fileName, const QStringList &possibleColors)
 {
-    const QString baseSvgNoColor{QString{OCC::Theme::themePrefix} + fileName};
-    if (QFile::exists(baseSvgNoColor)) {
-        return baseSvgNoColor;
-    }
-
-    for (const auto &color : possibleColors) {
-        const QString baseSVG{QString{OCC::Theme::themePrefix} + color + QLatin1Char('/') + fileName};
-
-        if (QFile::exists(baseSVG)) {
-            return baseSVG;
+    QString result;
+    result = QString{OCC::Theme::themePrefix} + fileName;
+    if (QFile::exists(result)) {
+        return result;
+    } else {
+        for (const auto &color : possibleColors) {
+            result = QString{OCC::Theme::themePrefix} + color + QStringLiteral("/") + fileName;
+
+            if (QFile::exists(result)) {
+                return result;
+            }
         }
+        result.clear();
     }
 
-    return {};
+    return result;
 }
 }
 
 namespace OCC {
 namespace Ui {
-    Q_LOGGING_CATEGORY(lcIconUtils, "nextcloud.gui.iconutils", QtInfoMsg)
-    namespace IconUtils {
-        QPixmap pixmapForBackground(const QString &fileName, const QColor &backgroundColor)
-        {
-            Q_ASSERT(!fileName.isEmpty());
-
-            const auto pixmapColor = backgroundColor.isValid()
-                && !Theme::isDarkColor(backgroundColor)
-                ? QColorConstants::Svg::black
-                : QColorConstants::Svg::white;
-
-            return createSvgPixmapWithCustomColor(fileName, pixmapColor);
-        }
-
-        QPixmap createSvgPixmapWithCustomColor(const QString &fileName, const QColor &customColor, const QSize &size)
-        {
-            Q_ASSERT(!fileName.isEmpty());
-            Q_ASSERT(customColor.isValid());
-
-            if (fileName.isEmpty()) {
-                qWarning(lcIconUtils) << "fileName is empty";
-            }
+namespace IconUtils {
+Q_LOGGING_CATEGORY(lcIconUtils, "nextcloud.gui.iconutils", QtInfoMsg)
+QPixmap pixmapForBackground(const QString &fileName, const QColor &backgroundColor)
+{
+    Q_ASSERT(!fileName.isEmpty());
 
-            if (!customColor.isValid()) {
-                qWarning(lcIconUtils) << "customColor is invalid";
-            }
+    const auto pixmapColor = backgroundColor.isValid() && !Theme::isDarkColor(backgroundColor)
+        ? QColorConstants::Svg::black
+        : QColorConstants::Svg::white;
+    ;
+    return createSvgPixmapWithCustomColorCached(fileName, pixmapColor);
+}
 
-            const auto customColorName = customColor.name();
+QImage createSvgImageWithCustomColor(const QString &fileName, const QColor &customColor, QSize *originalSize, const QSize &requestedSize)
+{
+    Q_ASSERT(!fileName.isEmpty());
+    Q_ASSERT(customColor.isValid());
 
-            const QString cacheKey = fileName + QLatin1Char(',') + customColorName;
+    QImage result{};
 
-            QPixmap cachedPixmap;
+    if (fileName.isEmpty() || !customColor.isValid()) {
+        qWarning(lcIconUtils) << "invalid fileName or customColor";
+        return result;
+    }
 
-            // check for existing QPixmap in cache
-            if (QPixmapCache::find(cacheKey, &cachedPixmap)) {
-                return cachedPixmap;
+    // some icons are present in white or black only, so, we need to check both when needed
+    const auto iconBaseColors = QStringList{QStringLiteral("black"), QStringLiteral("white")};
+
+    // check if there is an existing image matching the custom color
+    {
+        const auto customColorName = [&customColor]() {
+            auto result = customColor.name();
+            if (result.startsWith(QStringLiteral("#"))) {
+                if (result == QStringLiteral("#000000")) {
+                    result = QStringLiteral("black");
+                }
+                if (result == QStringLiteral("#ffffff")) {
+                    result = QStringLiteral("white");
+                }
             }
+            return result;
+        }();
 
-            // some icons are present in white or black only, so, we need to check both when needed
-            const auto iconBaseColors = QStringList{QStringLiteral("black"), QStringLiteral("white")};
-
-            // check if there is an existing pixmap matching the custom color
-            if (iconBaseColors.contains(customColorName)) {
-                cachedPixmap = QPixmap::fromImage(QImage{QString{OCC::Theme::themePrefix} + customColorName + QLatin1Char('/') + fileName});
-                QPixmapCache::insert(cacheKey, cachedPixmap);
-                return cachedPixmap;
+        if (iconBaseColors.contains(customColorName)) {
+            result = QImage{QString{OCC::Theme::themePrefix} + customColorName + QStringLiteral("/") + fileName};
+            if (!result.isNull()) {
+                return result;
             }
+        }
+    }
+    
+    // find the first matching svg file
+    const auto sourceSvg = findSvgFilePath(fileName, iconBaseColors);
+
+    Q_ASSERT(!sourceSvg.isEmpty());
+    if (sourceSvg.isEmpty()) {
+        qWarning(lcIconUtils) << "Failed to find base SVG file for" << fileName;
+        return result;
+    }
 
-            // find the first matching svg file
-            const auto sourceSvg = findSvgFilePath(fileName, iconBaseColors);
+    result = drawSvgWithCustomFillColor(sourceSvg, customColor, originalSize, requestedSize);
 
-            Q_ASSERT(!sourceSvg.isEmpty());
-            if (sourceSvg.isEmpty()) {
-                qWarning(lcIconUtils) << "Failed to find base SVG file for" << cacheKey;
-                return {};
-            }
+    Q_ASSERT(!result.isNull());
+    if (result.isNull()) {
+        qWarning(lcIconUtils) << "Failed to load pixmap for" << fileName;
+    }
+
+    return result;
+}
 
-            cachedPixmap = drawSvgWithCustomFillColor(sourceSvg, customColor, size);
+QPixmap createSvgPixmapWithCustomColorCached(const QString &fileName, const QColor &customColor, QSize *originalSize, const QSize &requestedSize)
+{
+    QPixmap cachedPixmap;
 
-            Q_ASSERT(!cachedPixmap.isNull());
-            if (cachedPixmap.isNull()) {
-                qWarning(lcIconUtils) << "Failed to load pixmap for" << cacheKey;
-                return {};
-            }
+    const auto customColorName = customColor.name();
 
-            QPixmapCache::insert(cacheKey, cachedPixmap);
+    const QString cacheKey = fileName + QStringLiteral(",") + customColorName;
 
-            return cachedPixmap;
+    // check for existing QPixmap in cache
+    if (QPixmapCache::find(cacheKey, &cachedPixmap)) {
+        if (originalSize) {
+            *originalSize = {};
         }
+        return cachedPixmap;
+    }
 
-        QPixmap drawSvgWithCustomFillColor(const QString &sourceSvgPath, const QColor &fillColor, const QSize &size)
-        {
-            QSvgRenderer svgRenderer;
+    cachedPixmap = QPixmap::fromImage(createSvgImageWithCustomColor(fileName, customColor, originalSize, requestedSize));
 
-            if (!svgRenderer.load(sourceSvgPath)) {
-                qCWarning(lcIconUtils) << "Could no load initial SVG image";
-                return {};
-            }
+    if (!cachedPixmap.isNull()) {
+        QPixmapCache::insert(cacheKey, cachedPixmap);
+    }
 
-            const auto requestedSize = size.isValid() ? size : svgRenderer.defaultSize();
+    return cachedPixmap;
+}
 
-            // render source image
-            QImage svgImage(requestedSize, QImage::Format_ARGB32);
-            {
-                QPainter svgImagePainter(&svgImage);
-                svgImage.fill(Qt::GlobalColor::transparent);
-                svgRenderer.render(&svgImagePainter);
-            }
+QImage drawSvgWithCustomFillColor(
+    const QString &sourceSvgPath, const QColor &fillColor, QSize *originalSize, const QSize &requestedSize)
+{
+    QSvgRenderer svgRenderer;
 
-            // draw target image with custom fillColor
-            QImage image(requestedSize, QImage::Format_ARGB32);
-            image.fill(QColor(fillColor));
-            {
-                QPainter imagePainter(&image);
-                imagePainter.setCompositionMode(QPainter::CompositionMode_DestinationIn);
-                imagePainter.drawImage(0, 0, svgImage);
-            }
+    if (!svgRenderer.load(sourceSvgPath)) {
+        qCWarning(lcIconUtils) << "Could no load initial SVG image";
+        return {};
+    }
 
-            return QPixmap::fromImage(image);
-        }
+    const auto reqSize = requestedSize.isValid() ? requestedSize : svgRenderer.defaultSize();
+
+    if (originalSize) {
+        *originalSize = svgRenderer.defaultSize();
+    }
+
+    // render source image
+    QImage svgImage(reqSize, QImage::Format_ARGB32);
+    {
+        QPainter svgImagePainter(&svgImage);
+        svgImage.fill(Qt::GlobalColor::transparent);
+        svgRenderer.render(&svgImagePainter);
     }
+
+    // draw target image with custom fillColor
+    QImage image(reqSize, QImage::Format_ARGB32);
+    image.fill(QColor(fillColor));
+    {
+        QPainter imagePainter(&image);
+        imagePainter.setCompositionMode(QPainter::CompositionMode_DestinationIn);
+        imagePainter.drawImage(0, 0, svgImage);
+    }
+
+    return image;
+}
+}
 }
 }

src/gui/iconutils.h

@@ -22,8 +22,9 @@ namespace OCC {
 namespace Ui {
 namespace IconUtils {
 QPixmap pixmapForBackground(const QString &fileName, const QColor &backgroundColor);
-QPixmap createSvgPixmapWithCustomColor(const QString &fileName, const QColor &customColor, const QSize &size = {});
-QPixmap drawSvgWithCustomFillColor(const QString &sourceSvgPath, const QColor &fillColor, const QSize &size = {});
+QImage createSvgImageWithCustomColor(const QString &fileName, const QColor &customColor, QSize *originalSize = nullptr, const QSize &requestedSize = {});
+QPixmap createSvgPixmapWithCustomColorCached(const QString &fileName, const QColor &customColor, QSize *originalSize = nullptr, const QSize &requestedSize = {});
+QImage drawSvgWithCustomFillColor(const QString &sourceSvgPath, const QColor &fillColor, QSize *originalSize = nullptr, const QSize &requestedSize = {});
 }
 }
 }

src/gui/tray/UnifiedSearchInputContainer.qml

@@ -17,7 +17,7 @@ TextField {
 
     readonly property int textFieldHorizontalPaddingOffset: 14
 
-    property var onClearText: function(){}
+    signal clearText()
 
     leftPadding: trayWindowUnifiedSearchTextFieldSearchIcon.width + trayWindowUnifiedSearchTextFieldSearchIcon.anchors.leftMargin + textFieldHorizontalPaddingOffset
     rightPadding: trayWindowUnifiedSearchTextFieldClearTextButton.width + trayWindowUnifiedSearchTextFieldClearTextButton.anchors.rightMargin + textFieldHorizontalPaddingOffset
@@ -87,7 +87,7 @@ TextField {
 
             anchors.fill: parent
 
-            onClicked: trayWindowUnifiedSearchTextField.onClearText()
+            onClicked: trayWindowUnifiedSearchTextField.clearText()
         }
     }
 }

src/gui/tray/Window.qml

@@ -588,7 +588,7 @@ Window {
             readOnly: !UserModel.currentUser.isConnected || UserModel.currentUser.unifiedSearchResultsListModel.currentFetchMoreInProgressProviderId
             isSearchInProgress: UserModel.currentUser.unifiedSearchResultsListModel.isSearchInProgress
             onTextEdited: { UserModel.currentUser.unifiedSearchResultsListModel.searchTerm = trayWindowUnifiedSearchInputContainer.text }
-            onClearText: function () { UserModel.currentUser.unifiedSearchResultsListModel.searchTerm = "" }
+            onClearText: { UserModel.currentUser.unifiedSearchResultsListModel.searchTerm = "" }
         }
 
         ErrorBox {

src/gui/tray/svgimageprovider.cpp

@@ -28,11 +28,9 @@ namespace Ui {
 
     QImage SvgImageProvider::requestImage(const QString &id, QSize *size, const QSize &requestedSize)
     {
-        Q_UNUSED(size)
-
         Q_ASSERT(!id.isEmpty());
 
-        const auto idSplit = id.split(QLatin1Char('/'), Qt::SkipEmptyParts);
+        const auto idSplit = id.split(QStringLiteral("/"), Qt::SkipEmptyParts);
 
         if (idSplit.isEmpty()) {
             qCWarning(lcSvgImageProvider) << "Image id is incorrect!";
@@ -47,7 +45,7 @@ namespace Ui {
             return {};
         }
 
-        return IconUtils::createSvgPixmapWithCustomColor(pixmapName, pixmapColor, requestedSize).toImage();
+        return IconUtils::createSvgImageWithCustomColor(pixmapName, pixmapColor, size, requestedSize);
     }
 }
 }

test/testiconutils.cpp

@@ -35,21 +35,19 @@ private slots:
         const QDir blackSvgDir(blackSvgDirPath);
         const QStringList blackImages = blackSvgDir.entryList(QStringList("*.svg"));
 
-        if (!blackImages.isEmpty()) {
-            QVERIFY(!OCC::Ui::IconUtils::drawSvgWithCustomFillColor(blackSvgDirPath + QLatin1Char('/') + blackImages.at(0), QColorConstants::Svg::red).isNull());
-        }
+        Q_ASSERT(!blackImages.isEmpty());
 
-        if (!blackImages.isEmpty()) {
-            QVERIFY(!OCC::Ui::IconUtils::drawSvgWithCustomFillColor(blackSvgDirPath + QLatin1Char('/') + blackImages.at(0), QColorConstants::Svg::green).isNull());
-        }
+        QVERIFY(!OCC::Ui::IconUtils::drawSvgWithCustomFillColor(blackSvgDirPath + QStringLiteral("/") + blackImages.at(0), QColorConstants::Svg::red).isNull());
+
+        QVERIFY(!OCC::Ui::IconUtils::drawSvgWithCustomFillColor(blackSvgDirPath + QStringLiteral("/") + blackImages.at(0), QColorConstants::Svg::green).isNull());
 
         const QString whiteSvgDirPath{QString{OCC::Theme::themePrefix} + QStringLiteral("white")};
         const QDir whiteSvgDir(whiteSvgDirPath);
         const QStringList whiteImages = whiteSvgDir.entryList(QStringList("*.svg"));
 
-        if (!whiteImages.isEmpty()) {
-            QVERIFY(!OCC::Ui::IconUtils::drawSvgWithCustomFillColor(whiteSvgDirPath + QLatin1Char('/') + whiteImages.at(0), QColorConstants::Svg::blue).isNull());
-        }
+        Q_ASSERT(!whiteImages.isEmpty());
+
+        QVERIFY(!OCC::Ui::IconUtils::drawSvgWithCustomFillColor(whiteSvgDirPath + QStringLiteral("/") + whiteImages.at(0), QColorConstants::Svg::blue).isNull());
     }
 
     void testCreateSvgPixmapWithCustomColor()
@@ -57,20 +55,18 @@ private slots:
         const QDir blackSvgDir(QString(QString{OCC::Theme::themePrefix}) + QStringLiteral("black"));
         const QStringList blackImages = blackSvgDir.entryList(QStringList("*.svg"));
 
-        if (!blackImages.isEmpty()) {
-            QVERIFY(!OCC::Ui::IconUtils::createSvgPixmapWithCustomColor(blackImages.at(0), QColorConstants::Svg::red).isNull());
-        }
+        QVERIFY(!blackImages.isEmpty());
+
+        QVERIFY(!OCC::Ui::IconUtils::createSvgImageWithCustomColor(blackImages.at(0), QColorConstants::Svg::red).isNull());
 
-        if (!blackImages.isEmpty()) {
-            QVERIFY(!OCC::Ui::IconUtils::createSvgPixmapWithCustomColor(blackImages.at(0), QColorConstants::Svg::green).isNull());
-        }
+        QVERIFY(!OCC::Ui::IconUtils::createSvgImageWithCustomColor(blackImages.at(0), QColorConstants::Svg::green).isNull());
 
         const QDir whiteSvgDir(QString(QString{OCC::Theme::themePrefix}) + QStringLiteral("white"));
         const QStringList whiteImages = whiteSvgDir.entryList(QStringList("*.svg"));
         
-        if (!whiteImages.isEmpty()) {
-            QVERIFY(!OCC::Ui::IconUtils::createSvgPixmapWithCustomColor(whiteImages.at(0), QColorConstants::Svg::blue).isNull());
-        }
+        QVERIFY(!whiteImages.isEmpty());
+
+        QVERIFY(!OCC::Ui::IconUtils::createSvgImageWithCustomColor(whiteImages.at(0), QColorConstants::Svg::blue).isNull());
     }
 
     void testPixmapForBackground()
@@ -81,15 +77,13 @@ private slots:
         const QDir whiteSvgDir(QString(QString{OCC::Theme::themePrefix}) + QStringLiteral("white"));
         const QStringList whiteImages = whiteSvgDir.entryList(QStringList("*.svg"));
 
-        if (blackImages.size() > 0) {
-            // white pixmap for dark background - should not fail
-            QVERIFY(!OCC::Ui::IconUtils::pixmapForBackground(whiteImages.at(0), QColor("blue")).isNull());
-        }
+        QVERIFY(!blackImages.isEmpty());
+
+        QVERIFY(!OCC::Ui::IconUtils::pixmapForBackground(whiteImages.at(0), QColor("blue")).isNull());
+
+        QVERIFY(!whiteImages.isEmpty());
 
-        if (whiteImages.size() > 0) {
-            // black pixmap for bright background - should not fail
-            QVERIFY(!OCC::Ui::IconUtils::pixmapForBackground(blackImages.at(0), QColor("yellow")).isNull());
-        }
+        QVERIFY(!OCC::Ui::IconUtils::pixmapForBackground(blackImages.at(0), QColor("yellow")).isNull());
     }
 };