diff --git a/cockatrice/src/interface/theme_icon.cpp b/cockatrice/src/interface/theme_icon.cpp index a355b5174..038a2fba0 100644 --- a/cockatrice/src/interface/theme_icon.cpp +++ b/cockatrice/src/interface/theme_icon.cpp @@ -10,6 +10,8 @@ #include #include #include +#include +#include #include #include #include @@ -78,6 +80,11 @@ QPixmap renderSvg(const QByteArray &data, const QSize &size, qreal devicePixelRa return pixmap; } +bool hasSentinel(const QByteArray &svg) +{ + return !svg.isEmpty() && QString::fromUtf8(svg).contains(QStringLiteral("currentColor"), Qt::CaseInsensitive); +} + QString cacheKey(const QString &path, const QByteArray &color, const QSize &size, qreal devicePixelRatio) { return path + QLatin1Char('|') + QString::fromLatin1(color) + QLatin1Char('|') + QString::number(size.width()) + @@ -88,15 +95,25 @@ QString cacheKey(const QString &path, const QByteArray &color, const QSize &size * @brief QIcon engine rendering a `theme:` SVG with `currentColor` resolved on demand. * * Qt's SVG renderer has no ambient color, so the sentinel is substituted from - * the current palette on every pixmap request. The render cache is keyed by - * resolved path, palette color, pixel size and DPR, so a theme change produces - * new keys instead of reusing art built under the previous palette. + * the current palette on every pixmap request. Asset resolution, the file bytes + * and the intrinsic size are cached until the theme changes: a repaint then + * does no file I/O at all. The render cache is keyed by resolved path, palette + * color, pixel size and DPR, so a palette change produces new keys instead of + * reusing art built under the previous palette. */ -class ThemeSvgIconEngine final : public QIconEngine +class ThemeSvgIconEngine final : public QObject, public QIconEngine { public: ThemeSvgIconEngine(QString _prefix, QPalette::ColorRole _role) : prefix(std::move(_prefix)), role(_role) { + if (themeManager) { + // A theme switch changes the resolved asset (scheme-qualified + // variants, raster overrides) and a palette change invalidates + // every cached colour, so both drop the caches instead of + // re-reading theme.cfg and the assets on every repaint. + connect(themeManager, &ThemeManager::themeChanged, this, &ThemeSvgIconEngine::invalidate); + connect(themeManager, &ThemeManager::paletteChanged, this, &ThemeSvgIconEngine::invalidate); + } } QIconEngine *clone() const override @@ -113,12 +130,14 @@ public: } const QSize pixelSize(qRound(logicalSize.width() * devicePixelRatio), qRound(logicalSize.height() * devicePixelRatio)); - QPixmap pixmap = render(pixelSize, devicePixelRatio, mode); + const QPixmap pixmap = render(pixelSize, devicePixelRatio, mode); if (!pixmap.isNull()) { - pixmap.setDevicePixelRatio(devicePixelRatio); + // QRect::center() would floor and drop the last row/column of a + // 16x16 glyph; the center has to be fractional so glyphs stay + // unshifted and unsclipped in item views. const QSizeF contentSize = pixmap.deviceIndependentSize(); const QRectF targetRect( - QPointF(rect.center()) - QPointF(contentSize.width() / 2.0, contentSize.height() / 2.0), contentSize); + QRectF(rect).center() - QPointF(contentSize.width() / 2.0, contentSize.height() / 2.0), contentSize); painter->drawPixmap(targetRect, pixmap, QRectF(pixmap.rect())); } } @@ -137,11 +156,7 @@ public: // Qt < 6.8 passes the device-dependent size to the engine. const QSize pixelSize = logicalSize; #endif - QPixmap pixmap = render(pixelSize, scale, mode); - if (!pixmap.isNull()) { - pixmap.setDevicePixelRatio(scale); - } - return pixmap; + return render(pixelSize, scale, mode); } QSize actualSize(const QSize &size, QIcon::Mode, QIcon::State) override @@ -150,26 +165,41 @@ public: } private: + void invalidate() + { + hasResolvedAsset = false; + hasIntrinsicSize = false; + cache.clear(); + } + void ensureResolved() const { - const QString themePath = themeManager->getCurrentThemePath(); - const bool darkMode = themeManager->isDarkModeActive(); - if (!hasResolvedPath || themePath != resolvedThemePath || darkMode != resolvedDarkMode) { - resolvedPath = themeManager->assetPath(prefix); - resolvedThemePath = themePath; - resolvedDarkMode = darkMode; - hasResolvedPath = true; - hasIntrinsicSize = false; + if (hasResolvedAsset && (!themeManager || themeManager->getCurrentThemePath() == resolvedThemePath)) { + return; } + resolvedThemePath = themeManager ? themeManager->getCurrentThemePath() : QString(); + resolvedPath = themeManager ? themeManager->assetPath(prefix) : QString(); + resolvedAssetIsSvg = resolvedPath.endsWith(QStringLiteral(".svg"), Qt::CaseInsensitive); + resolvedSvg = resolvedAssetIsSvg ? readSvg(resolvedPath) : QByteArray(); + resolvedHasSentinel = hasSentinel(resolvedSvg); + hasResolvedAsset = true; + hasIntrinsicSize = false; } QSize intrinsicSize() const { ensureResolved(); if (!hasIntrinsicSize) { - const QByteArray svg = readSvg(resolvedPath); - QSvgRenderer renderer(svg); - intrinsicSizeCache = renderer.isValid() ? renderer.defaultSize() : QSize(24, 24); + if (resolvedAssetIsSvg) { + QSvgRenderer renderer(resolvedSvg); + intrinsicSizeCache = renderer.isValid() ? renderer.defaultSize() : QSize(); + } else { + // A theme may override a glyph with a raster; take its real + // aspect ratio instead of assuming the 24x24 SVG default, so + // actualSize() and the rendered pixmap agree. + QImageReader reader(QStringLiteral("theme:") + resolvedPath); + intrinsicSizeCache = reader.size(); + } if (intrinsicSizeCache.isEmpty()) { intrinsicSizeCache = QSize(24, 24); } @@ -186,35 +216,44 @@ private: return intrinsicSize().scaled(size, Qt::KeepAspectRatio); } + QPixmap fromAsset(const QSize &pixelSize, QIcon::Mode mode) const + { + QPixmap pixmap(QStringLiteral("theme:") + resolvedPath); + if (!pixelSize.isEmpty() && pixmap.size() != pixelSize) { + pixmap = pixmap.scaled(pixelSize, Qt::KeepAspectRatio, Qt::SmoothTransformation); + } + if (mode == QIcon::Disabled && !pixmap.isNull()) { + pixmap = QApplication::style()->generatedIconPixmap(QIcon::Disabled, pixmap, nullptr); + } + return pixmap; + } + QPixmap render(const QSize &pixelSize, qreal devicePixelRatio, QIcon::Mode mode) const { ensureResolved(); - const QString themePath = resolvedThemePath; - const bool darkMode = resolvedDarkMode; - - const QByteArray svg = readSvg(resolvedPath); - if (svg.isEmpty() || !QString::fromUtf8(svg).contains(QStringLiteral("currentColor"), Qt::CaseInsensitive)) { + if (!resolvedHasSentinel) { // Authored art carries its own colors: use Qt's normal loader so // raster overrides and non-SVG assets keep working. - QPixmap pixmap(QStringLiteral("theme:") + resolvedPath); - if (!pixelSize.isEmpty() && pixmap.size() != pixelSize) { - pixmap = pixmap.scaled(pixelSize, Qt::KeepAspectRatio, Qt::SmoothTransformation); - } - if (mode == QIcon::Disabled && !pixmap.isNull()) { - pixmap = QApplication::style()->generatedIconPixmap(QIcon::Disabled, pixmap, nullptr); + QPixmap pixmap = fromAsset(pixelSize, mode); + if (!pixmap.isNull()) { + pixmap.setDevicePixelRatio(devicePixelRatio); } return pixmap; } const QByteArray color = sentinelColor(mode, role); - const QString key = cacheKey(resolvedPath, color, pixelSize, devicePixelRatio) + QLatin1Char('|') + themePath + - (darkMode ? QLatin1Char('d') : QLatin1Char('l')); + const QString key = cacheKey(resolvedPath, color, pixelSize, devicePixelRatio); const auto cached = cache.constFind(key); if (cached != cache.constEnd()) { return cached.value(); } - QPixmap pixmap = renderSvg(resolveSvgColorSentinel(svg, color), pixelSize, devicePixelRatio); + QPixmap pixmap = renderSvg(resolveSvgColorSentinel(resolvedSvg, color), pixelSize, devicePixelRatio); + if (!pixmap.isNull()) { + // Set the ratio before caching: assigning it to a cached pixmap + // afterwards detaches (deep-copies) it on every paint. + pixmap.setDevicePixelRatio(devicePixelRatio); + } if (cache.size() >= 64) { cache.clear(); } @@ -227,9 +266,11 @@ private: mutable QHash cache; mutable QString resolvedPath; mutable QString resolvedThemePath; + mutable QByteArray resolvedSvg; mutable QSize intrinsicSizeCache; - mutable bool resolvedDarkMode = false; - mutable bool hasResolvedPath = false; + mutable bool resolvedAssetIsSvg = false; + mutable bool resolvedHasSentinel = false; + mutable bool hasResolvedAsset = false; mutable bool hasIntrinsicSize = false; }; } // namespace @@ -246,7 +287,7 @@ QPixmap loadSentinelSvgPixmap(const QString &resolvedPath, QPalette::ColorRole r } const QByteArray svg = readSvg(resolvedPath); - if (svg.isEmpty() || !QString::fromUtf8(svg).contains(QStringLiteral("currentColor"), Qt::CaseInsensitive)) { + if (!hasSentinel(svg)) { return {}; } diff --git a/libcockatrice_utility/libcockatrice/utility/svg_recolor.cpp b/libcockatrice_utility/libcockatrice/utility/svg_recolor.cpp index 7072c1586..e25a31ba5 100644 --- a/libcockatrice_utility/libcockatrice/utility/svg_recolor.cpp +++ b/libcockatrice_utility/libcockatrice/utility/svg_recolor.cpp @@ -1,15 +1,41 @@ #include "svg_recolor.h" -#include +#include + +namespace +{ +constexpr QByteArrayView SENTINEL = "currentColor"; + +/// Case-insensitive byte search for the sentinel, starting at @p from. +/// QByteArray::indexOf() only matches case-sensitively for QByteArrayView, and +/// its char overload cannot match a word. +qsizetype findSentinel(const QByteArray &bytes, qsizetype from) +{ + for (qsizetype i = from; i + SENTINEL.size() <= bytes.size(); ++i) { + if (bytes.sliced(i, SENTINEL.size()).compare(SENTINEL, Qt::CaseInsensitive) == 0) { + return i; + } + } + return -1; +} +} // namespace QByteArray resolveSvgColorSentinel(const QByteArray &svg, const QByteArray &color) { - const QString source = QString::fromUtf8(svg); - if (!source.contains(QStringLiteral("currentColor"), Qt::CaseInsensitive)) { + // The sentinel and the replacement are both ASCII, so stay in bytes: a + // QString round trip would decode the whole document (and mangle bytes that + // are not valid UTF-8) even on the no-op path. + qsizetype pos = findSentinel(svg, 0); + if (pos < 0) { return svg; } - QString resolved = source; - resolved.replace(QStringLiteral("currentColor"), QString::fromLatin1(color), Qt::CaseInsensitive); - return resolved.toUtf8(); + QByteArray resolved = svg; + while (pos >= 0) { + resolved.replace(pos, SENTINEL.size(), color); + // Skip past the inserted color: a caller's color never contains the + // sentinel, and this keeps the scan linear. + pos = findSentinel(resolved, pos + color.size()); + } + return resolved; } diff --git a/libcockatrice_utility/libcockatrice/utility/svg_recolor.h b/libcockatrice_utility/libcockatrice/utility/svg_recolor.h index 767d46120..56d609b87 100644 --- a/libcockatrice_utility/libcockatrice/utility/svg_recolor.h +++ b/libcockatrice_utility/libcockatrice/utility/svg_recolor.h @@ -16,8 +16,13 @@ * (for example a QPalette role serialized to #RRGGBB) and receive SVG bytes * that are safe to hand to QSvgRenderer. * - * @param svg UTF-8 SVG source. Every `currentColor` occurrence, in any case, - * is replaced. + * Matching is a plain, case-insensitive substring search, not a CSS parse: an + * identifier that merely starts with the sentinel (for example + * `id="currentColorStop"`) is rewritten too. That is acceptable for the assets + * this project authors, and keeps the resolver dependency-free. + * + * @param svg SVG source bytes, expected to be UTF-8. Invalid UTF-8 is preserved + * byte-for-byte, since the resolver never decodes the document. * @param color Concrete SVG color (for example "#ff00ff"), as ASCII bytes. * @return The recolored SVG source; the input is left untouched. */ diff --git a/tests/svg_recolor_test.cpp b/tests/svg_recolor_test.cpp index 62990c322..abe1e8d1a 100644 --- a/tests/svg_recolor_test.cpp +++ b/tests/svg_recolor_test.cpp @@ -43,6 +43,19 @@ TEST(SvgRecolor, DoesNotModifyTheInput) EXPECT_EQ(svg, original); } +TEST(SvgRecolor, PreservesBytesThatAreNotValidUtf8) +{ + QByteArray svg = ""; + svg.append("\xff\xfe"); + svg.append(R"()"); + + const QByteArray result = resolveSvgColorSentinel(svg, "#ff8800"); + + EXPECT_TRUE(result.contains("\xff\xfe")); + EXPECT_FALSE(result.contains("currentColor")); + EXPECT_EQ(result.count("#ff8800"), 1); +} + TEST(SvgRecolor, NoSentinelIsANoOp) { const QByteArray svg = R"()";