diff --git a/cockatrice/src/interface/pixel_map_generator.cpp b/cockatrice/src/interface/pixel_map_generator.cpp index f024ba580..037293af9 100644 --- a/cockatrice/src/interface/pixel_map_generator.cpp +++ b/cockatrice/src/interface/pixel_map_generator.cpp @@ -4,6 +4,7 @@ #include "theme_icon.h" #include "theme_manager.h" +#include #include #include #include @@ -435,39 +436,19 @@ QIcon UserLevelPixmapGenerator::generateIconWithColorOverride(int height, QMap UserLevelPixmapGenerator::iconCache; -QPixmap LockPixmapGenerator::generatePixmap(int height) -{ - - int key = height; - if (pmCache.contains(key)) { - return pmCache.value(key); - } - - const QString resolved = themeManager->assetPath(QStringLiteral("icons/lock")); - QPixmap pixmap = loadSentinelSvgPixmap(resolved, QPalette::WindowText, QSize(height, height)); - if (pixmap.isNull()) { - pixmap = tryLoadImage(QStringLiteral("theme:") + resolved, QSize(height, height), true); - } - pmCache.insert(key, pixmap); - return pixmap; -} - -QMap LockPixmapGenerator::pmCache; - QPixmap DropdownIconPixmapGenerator::generatePixmap(int height, bool expanded) { - QString key = QString::number(expanded) + ":" + QString::number(height); + const QString key = QString::number(expanded) + ":" + QString::number(height); if (pmCache.contains(key)) { return pmCache.value(key); } + // Render through the themed icon engine: it keeps the glyph's aspect ratio, + // follows the palette, and renders at the device pixel ratio instead of + // handing a DPR-1 bitmap to a label that would upscale and blur it on a 2x + // display. const QString prefix = QStringLiteral("icons/") + (expanded ? "dropdown_expanded" : "dropdown_collapsed"); - const QString resolved = themeManager->assetPath(prefix); - QPixmap pixmap = loadSentinelSvgPixmap(resolved, QPalette::WindowText, QSize(height, height)); - if (pixmap.isNull()) { - pixmap = tryLoadImage(QStringLiteral("theme:") + resolved, QSize(height, height), true); - } - + const QPixmap pixmap = loadThemedIcon(prefix).pixmap(QSize(height, height), qApp->devicePixelRatio()); pmCache.insert(key, pixmap); return pixmap; } @@ -539,7 +520,6 @@ void clearPixmapGeneratorCaches() PingPixmapGenerator::clear(); CountryPixmapGenerator::clear(); UserLevelPixmapGenerator::clear(); - LockPixmapGenerator::clear(); DropdownIconPixmapGenerator::clear(); ManaSymbolPixmapGenerator::clear(); } diff --git a/cockatrice/src/interface/pixel_map_generator.h b/cockatrice/src/interface/pixel_map_generator.h index 299f72609..951f4231b 100644 --- a/cockatrice/src/interface/pixel_map_generator.h +++ b/cockatrice/src/interface/pixel_map_generator.h @@ -106,19 +106,6 @@ public: } }; -class LockPixmapGenerator -{ -private: - static QMap pmCache; - -public: - static QPixmap generatePixmap(int height); - static void clear() - { - pmCache.clear(); - } -}; - class DropdownIconPixmapGenerator { private: 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/cockatrice/src/interface/widgets/server/games_model.cpp b/cockatrice/src/interface/widgets/server/games_model.cpp index 315d51d9e..217a62abc 100644 --- a/cockatrice/src/interface/widgets/server/games_model.cpp +++ b/cockatrice/src/interface/widgets/server/games_model.cpp @@ -1,6 +1,7 @@ #include "games_model.h" #include "../../../client/settings/cache_settings.h" +#include "../../theme_icon.h" #include "../interface/pixel_map_generator.h" #include "libcockatrice/protocol/pb/serverinfo_user.pb.h" #include "user/user_list_proxy.h" @@ -188,7 +189,10 @@ QVariant GamesModel::data(const QModelIndex &index, int role) const return result.join(", "); } case Qt::DecorationRole: { - return gameentry.with_password() ? QIcon(LockPixmapGenerator::generatePixmap(13)) : QVariant(); + // The themed icon follows the palette, so the lock also + // switches to HighlightedText on a selected row, and the + // item view paints it at the device pixel ratio. + return gameentry.with_password() ? loadThemedIcon(QStringLiteral("icons/lock")) : QVariant(); case Qt::TextAlignmentRole: return Qt::AlignLeft; default: 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/icon_assets/icon-baseline.txt b/tests/icon_assets/icon-baseline.txt deleted file mode 100644 index 01a3f8f15..000000000 --- a/tests/icon_assets/icon-baseline.txt +++ /dev/null @@ -1,9 +0,0 @@ -# Icon rule baseline (ratchet). -# -# One violation per line: . -# Rules: xml, viewbox, editor-metadata, hardcoded-color, color-filename, no-token, physical-size, raster. -# -# This file may only shrink. When an icon is fixed, delete its line(s) in the -# same change; the test fails while a fixed entry remains. Never add new lines. -# -# The UI glyph set is fully migrated; this list is intentionally empty. diff --git a/tests/icon_assets/icon_assets_test.cpp b/tests/icon_assets/icon_assets_test.cpp index 93681d59c..f0b06d5cc 100644 --- a/tests/icon_assets/icon_assets_test.cpp +++ b/tests/icon_assets/icon_assets_test.cpp @@ -13,18 +13,12 @@ namespace { const QString ICONS_RELATIVE_PATH = QStringLiteral("cockatrice/resources/icons"); -const QString BASELINE_RELATIVE_PATH = QStringLiteral("tests/icon_assets/icon-baseline.txt"); QString iconsDir() { return QDir(QStringLiteral(COCKATRICE_SOURCE_DIR)).absoluteFilePath(ICONS_RELATIVE_PATH); } -QString baselinePath() -{ - return QDir(QStringLiteral(COCKATRICE_SOURCE_DIR)).absoluteFilePath(BASELINE_RELATIVE_PATH); -} - /** @brief True for content art that is allowed to carry full color. */ bool isContentArt(const QString &relativePath) { @@ -220,26 +214,6 @@ QSet collectViolations() return violations; } -/** @brief Parses the ratchet file; returns false with an error when it cannot be read. */ -bool readBaseline(QSet &entries, QString &error) -{ - QFile file(baselinePath()); - if (!file.open(QIODevice::ReadOnly | QIODevice::Text)) { - error = QStringLiteral("Cannot read %1").arg(baselinePath()); - return false; - } - - const QStringList lines = QString::fromUtf8(file.readAll()).split(QLatin1Char('\n')); - for (const QString &line : lines) { - const QString trimmed = line.trimmed(); - if (trimmed.isEmpty() || trimmed.startsWith(QLatin1Char('#'))) { - continue; - } - entries.insert(trimmed); - } - return true; -} - QString formatSet(const QSet &entries) { QStringList lines(entries.begin(), entries.end()); @@ -250,44 +224,18 @@ QString formatSet(const QSet &entries) } // namespace /** - * @brief The icon ratchet: no new violations, and fixed icons must leave the baseline. + * @brief Every UI glyph follows the palette-token rules. * - * Existing UI glyphs are grandfathered in tests/icon_assets/icon-baseline.txt. Fixing an icon - * without deleting its baseline lines fails on purpose, so the list can only shrink. + * The legacy set was paid down icon by icon, so no baseline grandfathers + * violations: a new violation fails right here. Scheme variants + * (`*-dark.svg`/`*-light.svg`) and mana art are exempt inside collectViolations(). */ -TEST(IconAssets, BaselineRatchet) +TEST(IconAssets, AssetsFollowTheRules) { ASSERT_TRUE(QDir::isAbsolutePath(iconsDir())); ASSERT_TRUE(QDir(iconsDir()).exists()) << "Missing icon directory: " << iconsDir().toStdString(); - QSet baseline; - QString error; - ASSERT_TRUE(readBaseline(baseline, error)) << error.toStdString(); - - const QSet actual = collectViolations(); - - const QSet newViolations = actual - baseline; - const QSet staleBaseline = baseline - actual; - - EXPECT_TRUE(newViolations.isEmpty()) << "New icon rule violations. Fix the icon; do not add exceptions:\n" - << formatSet(newViolations).toStdString(); - - EXPECT_TRUE(staleBaseline.isEmpty()) << "These baseline entries are fixed or gone; delete them from " - << BASELINE_RELATIVE_PATH.toStdString() << ":\n" - << formatSet(staleBaseline).toStdString(); -} - -/** @brief Guards against duplicate or malformed baseline entries. */ -TEST(IconAssets, BaselineIsWellFormed) -{ - QSet baseline; - QString error; - ASSERT_TRUE(readBaseline(baseline, error)) << error.toStdString(); - EXPECT_TRUE(baseline.isEmpty()) << "The legacy baseline is fully paid down; fix new violations instead"; - - static const QRegularExpression entryPattern(QStringLiteral( - R"(^(xml|viewbox|editor-metadata|hardcoded-color|color-filename|no-token|physical-size|raster) \S+$)")); - for (const QString &entry : baseline) { - EXPECT_TRUE(entryPattern.match(entry).hasMatch()) << "Malformed baseline entry: " << entry.toStdString(); - } + const QSet violations = collectViolations(); + EXPECT_TRUE(violations.isEmpty()) << "Icon rule violations. Fix the icon; do not add exceptions:\n" + << formatSet(violations).toStdString(); } 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"()";