diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp index 65b995de9..e1f760d9d 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp @@ -41,7 +41,7 @@ CardPictureLoader::CardPictureLoader() : QObject(nullptr) connect(&SettingsCache::instance().cardsDisplay(), &CardsDisplaySettings::cardLangChanged, this, &CardPictureLoader::cardLangChanged); - qRegisterMetaType(); + qRegisterMetaType("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,124 @@ void CardPictureLoader::saveCardImageToLocalStorage(const ExactCard &card, const } } +void CardPictureLoader::overridePrintingConnectLocalSaveAndEnqueue(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::overridePrintingEnsurePixmapExistsAndSaveLocally(const ExactCard &originalCard, + const ExactCard &overrideCard) +{ + // Same guard as overridePrintingConnectLocalSaveAndEnqueue: 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. + overridePrintingConnectLocalSaveAndEnqueue(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(); diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader.h index 0d3a9d474..6e7d19df1 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader.h @@ -102,6 +102,13 @@ public: */ static bool hasCustomArt(); + /** + * @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 hasLocalOverrides(const ExactCard &card); + /** * @brief Clears the in-memory QPixmap cache for all cards. */ @@ -120,7 +127,10 @@ public slots: * @param image Loaded QImage. */ void imageLoaded(const ExactCard &card, const QImage &image); - void saveCardImageToLocalStorage(const ExactCard &card, const QPixmap &pixmap); + void deleteAllLocalOverrides(const ExactCard &card); + void saveCardImageToLocalStorage(const ExactCard &card, const QPixmap &pixmap, bool allowOverwrite = false); + void overridePrintingConnectLocalSaveAndEnqueue(const ExactCard &originalCard, const ExactCard &overrideCard); + void overridePrintingEnsurePixmapExistsAndSaveLocally(const ExactCard &originalCard, const ExactCard &overrideCard); private slots: /** diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp index 39621839a..c82fca403 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp +++ b/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; } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 4f5dc88eb..dfbdabb5e 100644 --- a/tests/CMakeLists.txt +++ b/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) diff --git a/tests/loader_local_matching_test.cpp b/tests/loader_local_matching_test.cpp new file mode 100644 index 000000000..877728014 --- /dev/null +++ b/tests/loader_local_matching_test.cpp @@ -0,0 +1,177 @@ +#include "client/settings/cache_settings.h" +#include "interface/card_picture_loader/card_picture_loader_local.h" + +#include "gtest/gtest.h" +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +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); + + QCoreApplication app(argc, argv); + QLoggingCategory::setFilterRules("card_picture_loader.*=false\nsettings_cache.*=false"); + + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} \ No newline at end of file