From: alex-z Date: Wed, 3 Nov 2021 08:00:33 +0000 (+0200) Subject: Fix review comments. Use QImage for QML and QPixmap for rest of code. Do not cache... X-Git-Tag: archive/raspbian/3.16.7-1_deb13u1+rpi1~1^2~12^2~18^2~120^2 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=2f3c58daaccc4677acc105454719fac3efbf69a8;p=nextcloud-desktop.git 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 --- diff --git a/src/gui/iconutils.cpp b/src/gui/iconutils.cpp index 8ae50e2af..40ad051ce 100644 --- a/src/gui/iconutils.cpp +++ b/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; +} +} } } diff --git a/src/gui/iconutils.h b/src/gui/iconutils.h index 168789db0..af5930cb5 100644 --- a/src/gui/iconutils.h +++ b/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 = {}); } } } diff --git a/src/gui/tray/UnifiedSearchInputContainer.qml b/src/gui/tray/UnifiedSearchInputContainer.qml index 3a021eda0..2edbfbac6 100644 --- a/src/gui/tray/UnifiedSearchInputContainer.qml +++ b/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() } } } diff --git a/src/gui/tray/Window.qml b/src/gui/tray/Window.qml index 637d727d5..eab74c981 100644 --- a/src/gui/tray/Window.qml +++ b/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 { diff --git a/src/gui/tray/svgimageprovider.cpp b/src/gui/tray/svgimageprovider.cpp index 2f8c1f0ba..ab5760700 100644 --- a/src/gui/tray/svgimageprovider.cpp +++ b/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); } } } diff --git a/test/testiconutils.cpp b/test/testiconutils.cpp index cfdbe9478..9f16acd7f 100644 --- a/test/testiconutils.cpp +++ b/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()); } };