[PictureLoader] Add local override storage and resolution with matcher tests (#7311)

* [PictureLoader] Add local override storage and resolution with matcher tests

* [PictureLoader] Address review comments

- Make deleteAllLocalOverrides static; it does not touch instance state
- Drop the now-unused hasCustomArt dead code
- Rename the override install methods to installPrintingOverride /
  installPrintingOverrideOnLoad

* [Tests] Give loader matcher tests a writable HOME in CI

Under GitHub's docker runner the process uid has no passwd entry, so HOME
resolves to '/' and the test-mode qttest data dir cannot be created.
SettingsCache's QSettings then drops every write, getPicsPath() comes back
empty, and the loader searches a blank path. Point HOME at a QTemporaryDir
for the duration of the run.

---------

Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>

cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp

@@ -8,7 +8,7 @@
 #include <QApplication>
 #include <QBuffer>
 #include <QDebug>
-#include <QDirIterator>
+#include <QDir>
 #include <QFileInfo>
 #include <QMainWindow>
 #include <QMovie>
@@ -41,7 +41,7 @@ CardPictureLoader::CardPictureLoader() : QObject(nullptr)
     connect(&SettingsCache::instance().cardsDisplay(), &CardsDisplaySettings::cardLangChanged, this,
             &CardPictureLoader::cardLangChanged);
 
-    qRegisterMetaType<ExactCard>();
+    qRegisterMetaType<ExactCard>("ExactCard");
     connect(worker, &CardPictureLoaderWorker::imageLoaded, this, &CardPictureLoader::imageLoaded);
 
     statusBar = new CardPictureLoaderStatusBar(nullptr);
@@ -209,7 +209,49 @@ void CardPictureLoader::imageLoaded(const ExactCard &card, const QImage &image)
     card.emitPixmapUpdated();
 }
 
-void CardPictureLoader::saveCardImageToLocalStorage(const ExactCard &card, const QPixmap &pixmap)
+void CardPictureLoader::deleteAllLocalOverrides(const ExactCard &card)
+{
+    const QString picsRoot = SettingsCache::instance().paths().getPicsPath();
+    if (picsRoot.isEmpty() || !card) {
+        return;
+    }
+
+    QDir baseDir(picsRoot);
+    if (!baseDir.cd("downloadedPics")) {
+        return;
+    }
+
+    const QString name = card.getInfo().getCorrectedName();
+
+    QString set, collector, uuid;
+    auto printing = card.getPrinting();
+    if (printing.getSet()) {
+        set = printing.getSet()->getCorrectedShortName();
+        collector = printing.getProperty("num");
+        uuid = printing.getUuid();
+    }
+
+    for (const auto &scheme : CardPictureLoaderLocalSchemes::exportSchemes()) {
+        QString rel = CardPictureLoaderLocalSchemes::expandPattern(scheme.pattern, name, set, collector, uuid);
+
+        if (rel.isEmpty()) {
+            continue;
+        }
+
+        rel += ".png";
+        rel = QDir::cleanPath(rel);
+
+        QString fullPath = baseDir.filePath(rel);
+
+        if (QFile::exists(fullPath)) {
+            QFile::remove(fullPath);
+        }
+    }
+}
+
+void CardPictureLoader::saveCardImageToLocalStorage(const ExactCard &card,
+                                                    const QPixmap &pixmap,
+                                                    const bool allowOverwrite)
 {
     if (pixmap.isNull() || !card) {
         return;
@@ -269,8 +311,9 @@ void CardPictureLoader::saveCardImageToLocalStorage(const ExactCard &card, const
 
     QFileInfo outInfo(baseDir.filePath(relativePath));
 
-    // Do not overwrite existing files
-    if (outInfo.exists()) {
+    // Automatic cache writes (FILESYSTEM_CACHE) must never clobber an explicit user override.
+    // Only the explicit override paths pass allowOverwrite == true.
+    if (!allowOverwrite && outInfo.exists()) {
         return;
     }
 
@@ -291,6 +334,122 @@ void CardPictureLoader::saveCardImageToLocalStorage(const ExactCard &card, const
     }
 }
 
+void CardPictureLoader::installPrintingOverrideOnLoad(const ExactCard &originalCard, const ExactCard &overrideCard)
+{
+    // Overriding a card with itself is the reset case, not a real override: every code path below
+    // would re-enter itself through emitPixmapUpdated(). Reject it outright.
+    if (originalCard == overrideCard) {
+        return;
+    }
+
+    CardInfoPtr cardPtr = overrideCard.getCardPtr();
+    if (!cardPtr) {
+        return;
+    }
+
+    // Heap-allocate so the lambda can capture it before the connection is made
+    auto *connectionHandle = new QMetaObject::Connection;
+
+    *connectionHandle =
+        connect(cardPtr.data(), &CardInfo::pixmapUpdated, cardPtr.data(),
+                [originalCard, overrideCard, connectionHandle, this](const PrintingInfo &printing) {
+                    // All printings share the same CardInfo, so ignore updates triggered by any
+                    // other printing (e.g., the original card re-loading from disk).
+                    if (printing != overrideCard.getPrinting()) {
+                        return;
+                    }
+
+                    QPixmap pixmap;
+                    if (QPixmapCache::find(overrideCard.getPixmapCacheKey(), &pixmap) && !pixmap.isNull()) {
+                        // The override art has resolved — persist it and reflect it immediately.
+                        // Retire the connection before emitting so the refresh can't re-enter.
+                        saveCardImageToLocalStorage(originalCard, pixmap, /*allowOverwrite=*/true);
+
+                        QObject::disconnect(*connectionHandle);
+                        delete connectionHandle;
+
+                        QPixmapCache::clear();
+                        originalCard.emitPixmapUpdated();
+                        return;
+                    }
+
+                    // The art could not be resolved. Keep the connection armed so a late resolution
+                    // still lands, and surface a visible refusal instead of a silent no-op. An
+                    // override already on disk is left untouched and simply re-displayed.
+                    QPixmapCache::clear();
+                    if (!hasLocalOverrides(originalCard)) {
+                        QPixmap refusedPixmap;
+                        getCardBackLoadingFailedPixmap(refusedPixmap, QSize(480, 672));
+                        QPixmapCache::insert(originalCard.getPixmapCacheKey(), refusedPixmap);
+                    }
+                    originalCard.emitPixmapUpdated();
+                });
+
+    // Now enqueue; if the image is already loading (deduplicated in the worker),
+    // the signal will still fire when it completes
+    CardPictureLoader::getInstance().worker->enqueueImageLoad(overrideCard);
+}
+
+void CardPictureLoader::installPrintingOverride(const ExactCard &originalCard, const ExactCard &overrideCard)
+{
+    // Same guard as installPrintingOverrideOnLoad: self-override is the reset case.
+    if (originalCard == overrideCard) {
+        return;
+    }
+
+    QPixmap pixmap;
+    const QString key = overrideCard.getPixmapCacheKey();
+
+    if (QPixmapCache::find(key, &pixmap) && !pixmap.isNull()) {
+        // Already cached — save immediately; the caller refreshes the card.
+        saveCardImageToLocalStorage(originalCard, pixmap, /*allowOverwrite=*/true);
+        return;
+    }
+
+    // Cache miss or previously failed load — enqueue load and wait for the signal.
+    installPrintingOverrideOnLoad(originalCard, overrideCard);
+}
+
+bool CardPictureLoader::hasLocalOverrides(const ExactCard &card)
+{
+    const QString picsRoot = SettingsCache::instance().paths().getPicsPath();
+    if (picsRoot.isEmpty() || !card) {
+        return false;
+    }
+
+    QDir baseDir(picsRoot);
+    if (!baseDir.cd("downloadedPics")) {
+        return false;
+    }
+
+    const QString name = card.getInfo().getCorrectedName();
+
+    QString set, collector, uuid;
+    const PrintingInfo printing = card.getPrinting();
+    if (printing.getSet()) {
+        set = printing.getSet()->getCorrectedShortName();
+        collector = printing.getProperty("num");
+        uuid = printing.getUuid();
+    }
+
+    for (const auto &scheme : CardPictureLoaderLocalSchemes::exportSchemes()) {
+        QString rel = CardPictureLoaderLocalSchemes::expandPattern(scheme.pattern, name, set, collector, uuid);
+
+        if (rel.isEmpty()) {
+            continue;
+        }
+
+        rel += ".png";
+        rel = QDir::cleanPath(rel);
+
+        if (QFile::exists(baseDir.filePath(rel))) {
+            return true;
+        }
+    }
+
+    return false;
+}
+
 void CardPictureLoader::clearPixmapCache()
 {
     QPixmapCache::clear();
@@ -338,32 +497,3 @@ void CardPictureLoader::cardLangChanged()
     QPixmapCache::clear();
     failedAt.clear();
 }
-
-bool CardPictureLoader::hasCustomArt()
-{
-    auto picsPath = SettingsCache::instance().paths().getPicsPath();
-    QDirIterator it(picsPath, QDir::Dirs | QDir::NoDotAndDotDot);
-
-    // Check if there is at least one non-directory file in the pics path, other
-    // than in the "downloadedPics" subdirectory.
-    while (it.hasNext()) {
-#if (QT_VERSION >= QT_VERSION_CHECK(6, 3, 0))
-        QFileInfo dir(it.nextFileInfo());
-#else
-        // nextFileInfo() is only available in Qt 6.3+, for previous versions, we build
-        // the QFileInfo from a QString which requires more system calls.
-        QFileInfo dir(it.next());
-#endif
-
-        if (it.fileName() == "downloadedPics") {
-            continue;
-        }
-
-        QDirIterator subIt(it.filePath(), QDir::Files, QDirIterator::Subdirectories | QDirIterator::FollowSymlinks);
-        if (subIt.hasNext()) {
-            return true;
-        }
-    }
-
-    return false;
-}

cockatrice/src/interface/card_picture_loader/card_picture_loader.h

@@ -97,10 +97,17 @@ public:
     static void cacheCardPixmaps(const QList<ExactCard> &cards);
 
     /**
-     * @brief Check if the user has custom card art in the picsPath directory.
-     * @return True if any custom art exists.
+     * @brief Check if a local override image already exists for the card.
+     * @param card The card to check.
+     * @return True if the card has at least one locally stored override image.
      */
-    static bool hasCustomArt();
+    static bool hasLocalOverrides(const ExactCard &card);
+
+    /**
+     * @brief Removes all locally stored override images for the card.
+     * @param card The card to remove the override images of.
+     */
+    static void deleteAllLocalOverrides(const ExactCard &card);
 
     /**
      * @brief Clears the in-memory QPixmap cache for all cards.
@@ -120,7 +127,9 @@ public slots:
      * @param image Loaded QImage.
      */
     void imageLoaded(const ExactCard &card, const QImage &image);
-    void saveCardImageToLocalStorage(const ExactCard &card, const QPixmap &pixmap);
+    void saveCardImageToLocalStorage(const ExactCard &card, const QPixmap &pixmap, bool allowOverwrite = false);
+    void installPrintingOverride(const ExactCard &originalCard, const ExactCard &overrideCard);
+    void installPrintingOverrideOnLoad(const ExactCard &originalCard, const ExactCard &overrideCard);
 
 private slots:
     /**

cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp

@@ -94,6 +94,10 @@ QImage CardPictureLoaderLocal::tryLoadCardImageFromDisk(const QString &setName,
             candidatePaths << picsPath + "/downloadedPics/" + setName + "/" + nameVariant;
         }
 
+        // Non-set-folder export schemes (e.g., Name_Set_Collector) write straight into
+        // downloadedPics/; check there as a fallback so local overrides round-trip.
+        candidatePaths << picsPath + "/downloadedPics/" + nameVariant;
+
         for (const QString &path : candidatePaths) {
             QFileInfo fileInfo(path);
             QDir dir = fileInfo.dir();
@@ -105,7 +109,8 @@ QImage CardPictureLoaderLocal::tryLoadCardImageFromDisk(const QString &setName,
 
             QStringList files = dir.entryList(QDir::Files);
             for (const QString &file : files) {
-                if (!file.startsWith(baseName)) {
+                QFileInfo fi(file);
+                if (fi.completeBaseName() != baseName) {
                     continue;
                 }
 

tests/CMakeLists.txt

@@ -16,6 +16,7 @@ add_test(NAME warning_categories_test COMMAND warning_categories_test)
 add_test(NAME lag_monitor_test COMMAND lag_monitor_test)
 add_test(NAME latency_tracker_test COMMAND latency_tracker_test)
 add_test(NAME metrics_registry_test COMMAND metrics_registry_test)
+add_test(NAME loader_local_matching_test COMMAND loader_local_matching_test)
 
 add_test(NAME deck_hash_performance_test COMMAND deck_hash_performance_test)
 set_tests_properties(deck_hash_performance_test PROPERTIES TIMEOUT 15)
@@ -38,6 +39,17 @@ add_executable(lag_monitor_test ${CMAKE_SOURCE_DIR}/cockatrice/src/client/lag_mo
 target_include_directories(lag_monitor_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src)
 add_executable(latency_tracker_test latency_tracker_test.cpp)
 add_executable(metrics_registry_test ../servatrice/src/metrics_registry.cpp metrics_registry_test.cpp)
+add_executable(
+  loader_local_matching_test
+  ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp
+  ${CMAKE_SOURCE_DIR}/cockatrice/src/client/settings/cache_settings.cpp
+  ${CMAKE_SOURCE_DIR}/cockatrice/src/client/settings/card_counter_settings.cpp
+  ${CMAKE_SOURCE_DIR}/cockatrice/src/client/settings/shortcuts_settings.cpp
+  ${CMAKE_SOURCE_DIR}/cockatrice/src/client/network/update/client/release_channel.cpp
+  ${VERSION_STRING_CPP}
+  loader_local_matching_test.cpp
+)
+target_include_directories(loader_local_matching_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src)
 
 find_package(GTest)
 
@@ -79,6 +91,7 @@ if(NOT GTEST_FOUND)
   add_dependencies(lag_monitor_test gtest)
   add_dependencies(latency_tracker_test gtest)
   add_dependencies(metrics_registry_test gtest)
+  add_dependencies(loader_local_matching_test gtest)
 endif()
 
 include_directories(${GTEST_INCLUDE_DIRS})
@@ -123,6 +136,9 @@ target_link_libraries(
 )
 target_include_directories(metrics_registry_test PRIVATE ${CMAKE_SOURCE_DIR}/servatrice/src)
 target_link_libraries(metrics_registry_test ${TEST_QT_MODULES} Threads::Threads ${GTEST_BOTH_LIBRARIES})
+target_link_libraries(
+  loader_local_matching_test libcockatrice_settings Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES}
+)
 
 add_subdirectory(card_zone_algorithms)
 add_subdirectory(carddatabase)

tests/loader_local_matching_test.cpp

@@ -0,0 +1,187 @@
+#include "client/settings/cache_settings.h"
+#include "interface/card_picture_loader/card_picture_loader_local.h"
+
+#include "gtest/gtest.h"
+#include <QColor>
+#include <QCoreApplication>
+#include <QDir>
+#include <QFileInfo>
+#include <QImage>
+#include <QImageWriter>
+#include <QLoggingCategory>
+#include <QStandardPaths>
+#include <QTemporaryDir>
+#include <libcockatrice/card/lazy_properties_hash.h>
+#include <libcockatrice/card/printing/exact_card.h>
+#include <libcockatrice/card/set/card_set.h>
+#include <libcockatrice/interfaces/noop_card_set_priority_controller.h>
+#include <libcockatrice/settings/paths_settings.h>
+
+namespace
+{
+
+/**
+ * @brief Builds an ExactCard with the requested identity fields.
+ *
+ * Mirrors how the client constructs cards: the set short name feeds tryLoad()'s
+ * setName, and the "num" printing property feeds the collector number.
+ */
+ExactCard cardFor(const QString &name, const QString &setShortName, const QString &collectorNumber)
+{
+    CardSetPtr set;
+    if (!setShortName.isEmpty()) {
+        set = CardSet::newInstance(new NoopCardSetPriorityController(), setShortName, setShortName);
+    }
+
+    LazyPropertiesHash properties;
+    if (!collectorNumber.isEmpty()) {
+        properties.insert("num", collectorNumber);
+    }
+
+    return ExactCard(CardInfo::newInstance(name), PrintingInfo(set, properties));
+}
+
+class LocalMatcherTest : public ::testing::Test
+{
+protected:
+    QTemporaryDir tempDir;                    ///< Sandboxed "pics" root for every test.
+    CardPictureLoaderLocal *loader = nullptr; ///< Constructed per test against the sandboxed paths.
+
+    QString picsPath() const
+    {
+        return tempDir.path() + "/pics";
+    }
+
+    void SetUp() override
+    {
+        // The loader ctor snapshots the global picture paths once, so point them at the
+        // sandbox before constructing it.
+        SettingsCache::instance().paths().setPicsPath(picsPath());
+        SettingsCache::instance().paths().setCustomPicsPath(picsPath() + "/CUSTOM/");
+
+        loader = new CardPictureLoaderLocal(nullptr);
+    }
+
+    void TearDown() override
+    {
+        delete loader;
+        loader = nullptr;
+    }
+
+    /**
+     * @brief Writes a valid 1x1 PNG under the sandboxed pics path.
+     */
+    void writePngUnderPics(const QString &relativePath, const QColor &color = Qt::red)
+    {
+        const QString fullPath = picsPath() + "/" + relativePath;
+        ASSERT_TRUE(QDir().mkpath(QFileInfo(fullPath).absolutePath()));
+
+        QImage image(1, 1, QImage::Format_RGB32);
+        image.fill(color);
+
+        QImageWriter writer(fullPath, "PNG");
+        ASSERT_TRUE(writer.write(image));
+    }
+};
+
+TEST_F(LocalMatcherTest, ExactMatchBareFileWinsOverSuffixedCompanion)
+{
+    writePngUnderPics("downloadedPics/TestCard.png");
+    writePngUnderPics("downloadedPics/TestCard (1).png");
+
+    const QImage image = loader->tryLoad(cardFor("TestCard", "", ""));
+
+    EXPECT_FALSE(image.isNull()) << "The bare TestCard.png must be picked over its suffixed companion";
+}
+
+TEST_F(LocalMatcherTest, SuffixedFileWithoutExactMatchIsNotLoaded)
+{
+    // The pre-refactor prefix match would have accepted "TestCard (1).png" for "TestCard".
+    writePngUnderPics("downloadedPics/TestCard (1).png");
+
+    const QImage image = loader->tryLoad(cardFor("TestCard", "", ""));
+
+    EXPECT_TRUE(image.isNull()) << "A suffixed file must not satisfy an exact card-name lookup";
+}
+
+TEST_F(LocalMatcherTest, SetFolderLookupIgnoresSuffixedFiles)
+{
+    writePngUnderPics("M10/TestCard (1).png");
+
+    const QImage image = loader->tryLoad(cardFor("TestCard", "M10", ""));
+
+    EXPECT_TRUE(image.isNull()) << "Set-folder lookups must also require an exact name match";
+}
+
+TEST_F(LocalMatcherTest, SetFolderLookupStillResolvesExactFile)
+{
+    writePngUnderPics("M10/TestCard.png");
+    writePngUnderPics("M10/TestCard (1).png");
+
+    const QImage image = loader->tryLoad(cardFor("TestCard", "M10", ""));
+
+    EXPECT_FALSE(image.isNull()) << "The exact file in the set folder must still resolve";
+}
+
+TEST_F(LocalMatcherTest, RootDownloadedPicsFallbackResolvesSchemeFilename)
+{
+    // Non-set-folder export schemes (Name_Set_Collector) write straight into downloadedPics/.
+    writePngUnderPics("downloadedPics/TestCard_M10_1.png");
+
+    const QImage image = loader->tryLoad(cardFor("TestCard", "M10", "1"));
+
+    EXPECT_FALSE(image.isNull()) << "downloadedPics/TestCard_M10_1.png must resolve via the root fallback";
+}
+
+TEST_F(LocalMatcherTest, RootFallbackResolvesDashSeparatedVariant)
+{
+    writePngUnderPics("downloadedPics/TestCard-M10-1.png");
+
+    const QImage image = loader->tryLoad(cardFor("TestCard", "M10", "1"));
+
+    EXPECT_FALSE(image.isNull()) << "The dash-separated import variant must resolve via the root fallback";
+}
+
+TEST_F(LocalMatcherTest, DownloadedPicsSetSubfolderStillResolves)
+{
+    writePngUnderPics("downloadedPics/M10/TestCard_M10_1.png");
+
+    const QImage image = loader->tryLoad(cardFor("TestCard", "M10", "1"));
+
+    EXPECT_FALSE(image.isNull()) << "The set-subfolder export scheme must keep resolving";
+}
+
+TEST_F(LocalMatcherTest, SetFolderCandidateTakesPrecedenceOverRootFallback)
+{
+    writePngUnderPics("M10/TestCard_M10_1.png", Qt::red);
+    writePngUnderPics("downloadedPics/TestCard_M10_1.png", Qt::blue);
+
+    const QImage image = loader->tryLoad(cardFor("TestCard", "M10", "1"));
+
+    ASSERT_FALSE(image.isNull());
+    EXPECT_EQ(image.pixelColor(0, 0), QColor(Qt::red)) << "The set-folder candidate must be preferred";
+}
+
+} // namespace
+
+int main(int argc, char **argv)
+{
+    // Redirect SettingsCache reads/writes (app-data location) away from the real user profile.
+    QStandardPaths::setTestModeEnabled(true);
+
+    // Some CI containers run as a uid without a passwd entry (e.g. GitHub's docker
+    // runner), so HOME resolves to "/" and the test-mode qttest data dir cannot be
+    // created. SettingsCache's QSettings then silently drops every write, reads come
+    // back empty, and the paths the loader searches are "". Give the test a writable
+    // HOME for the duration of the run so settings behave like on a normal machine.
+    QTemporaryDir home;
+    if (home.isValid()) {
+        qputenv("HOME", home.path().toLocal8Bit());
+    }
+
+    QCoreApplication app(argc, argv);
+    QLoggingCategory::setFilterRules("card_picture_loader.*=false\nsettings_cache.*=false");
+
+    ::testing::InitGoogleTest(&argc, argv);
+    return RUN_ALL_TESTS();
+}