From 83409c32c462d60260596731a5ebb452041a1d53 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sat, 23 Nov 2024 04:21:26 +0100 Subject: [PATCH] Cache redirects properly by implementing our own QSettings cache for urls. (#5186) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Cache redirects properly by implementing our own QSettings cache for urls. * Load and store redirects properly. * Set the maximum network cache size from settings value on PictureLoaderWorker instantiation. * Address comments. * Lint. * Adjust debug statements to be in line with existing ones. * Minor Tweaks * Make redirect cache ttl a user-adjustable setting. * Fix Build * Minor Cleanup * Minor Cleanup * Build Fix --------- Co-authored-by: Lukas Brübach Co-authored-by: ZeldaZach --- cockatrice/src/client/ui/picture_loader.cpp | 110 ++++++++++++++++++-- cockatrice/src/client/ui/picture_loader.h | 15 +++ cockatrice/src/dialogs/dlg_settings.cpp | 26 ++++- cockatrice/src/dialogs/dlg_settings.h | 2 + cockatrice/src/settings/cache_settings.cpp | 9 ++ cockatrice/src/settings/cache_settings.h | 26 ++++- dbconverter/src/mocks.cpp | 3 + tests/carddatabase/mocks.cpp | 3 + 8 files changed, 178 insertions(+), 16 deletions(-) diff --git a/cockatrice/src/client/ui/picture_loader.cpp b/cockatrice/src/client/ui/picture_loader.cpp index ad18a9071..a28a98eac 100644 --- a/cockatrice/src/client/ui/picture_loader.cpp +++ b/cockatrice/src/client/ui/picture_loader.cpp @@ -1,17 +1,14 @@ #include "picture_loader.h" -#include "../../game/cards/card_database.h" #include "../../game/cards/card_database_manager.h" #include "../../settings/cache_settings.h" -#include "theme_manager.h" #include #include #include #include -#include #include -#include +#include #include #include #include @@ -23,7 +20,6 @@ #include #include #include -#include #include #include #include @@ -136,6 +132,8 @@ PictureLoaderWorker::PictureLoaderWorker() #endif auto cache = new QNetworkDiskCache(this); cache->setCacheDirectory(SettingsCache::instance().getNetworkCachePath()); + cache->setMaximumCacheSize(1024L * 1024L * + static_cast(SettingsCache::instance().getNetworkCacheSizeInMB())); // Note: the settings is in MB, but QNetworkDiskCache uses bytes connect(&SettingsCache::instance(), &SettingsCache::networkCacheSizeChanged, cache, [cache](int newSizeInMB) { cache->setMaximumCacheSize(1024L * 1024L * static_cast(newSizeInMB)); }); @@ -145,6 +143,13 @@ PictureLoaderWorker::PictureLoaderWorker() networkManager->setRedirectPolicy(QNetworkRequest::ManualRedirectPolicy); connect(networkManager, SIGNAL(finished(QNetworkReply *)), this, SLOT(picDownloadFinished(QNetworkReply *))); + cacheFilePath = SettingsCache::instance().getRedirectCachePath() + REDIRECT_CACHE_FILENAME; + loadRedirectCache(); + cleanStaleEntries(); + + connect(QCoreApplication::instance(), &QCoreApplication::aboutToQuit, this, + &PictureLoaderWorker::saveRedirectCache); + pictureLoaderThread = new QThread; pictureLoaderThread->start(QThread::LowPriority); moveToThread(pictureLoaderThread); @@ -447,11 +452,104 @@ bool PictureLoaderWorker::imageIsBlackListed(const QByteArray &picData) QNetworkReply *PictureLoaderWorker::makeRequest(const QUrl &url) { + // Check if the redirect is cached + QUrl cachedRedirect = getCachedRedirect(url); + if (!cachedRedirect.isEmpty()) { + qDebug().nospace() << "PictureLoader: [card: " << cardBeingDownloaded.getCard()->getCorrectedName() + << " set: " << cardBeingDownloaded.getSetName() << "]: Using cached redirect for " + << url.toDisplayString() << " to " << cachedRedirect.toDisplayString(); + return makeRequest(cachedRedirect); // Use the cached redirect + } + QNetworkRequest req(url); + if (!picDownload) { req.setAttribute(QNetworkRequest::CacheLoadControlAttribute, QNetworkRequest::AlwaysCache); } - return networkManager->get(req); + + QNetworkReply *reply = networkManager->get(req); + + connect(reply, &QNetworkReply::finished, this, [this, reply, url]() { + QVariant redirectTarget = reply->attribute(QNetworkRequest::RedirectionTargetAttribute); + + if (redirectTarget.isValid()) { + QUrl redirectUrl = redirectTarget.toUrl(); + if (redirectUrl.isRelative()) { + redirectUrl = url.resolved(redirectUrl); + } + + cacheRedirect(url, redirectUrl); + qDebug().nospace() << "PictureLoader: [card: " << cardBeingDownloaded.getCard()->getCorrectedName() + << " set: " << cardBeingDownloaded.getSetName() << "]: Caching redirect from " + << url.toDisplayString() << " to " << redirectUrl.toDisplayString(); + } + + reply->deleteLater(); + }); + + return reply; +} + +void PictureLoaderWorker::cacheRedirect(const QUrl &originalUrl, const QUrl &redirectUrl) +{ + redirectCache[originalUrl] = qMakePair(redirectUrl, QDateTime::currentDateTimeUtc()); + saveRedirectCache(); +} + +QUrl PictureLoaderWorker::getCachedRedirect(const QUrl &originalUrl) const +{ + if (redirectCache.contains(originalUrl)) { + return redirectCache[originalUrl].first; + } + return {}; +} + +void PictureLoaderWorker::loadRedirectCache() +{ + QSettings settings(cacheFilePath, QSettings::IniFormat); + + redirectCache.clear(); + int size = settings.beginReadArray(REDIRECT_HEADER_NAME); + for (int i = 0; i < size; ++i) { + settings.setArrayIndex(i); + QUrl originalUrl = settings.value(REDIRECT_ORIGINAL_URL).toUrl(); + QUrl redirectUrl = settings.value(REDIRECT_URL).toUrl(); + QDateTime timestamp = settings.value(REDIRECT_TIMESTAMP).toDateTime(); + + if (originalUrl.isValid() && redirectUrl.isValid()) { + redirectCache[originalUrl] = qMakePair(redirectUrl, timestamp); + } + } + settings.endArray(); +} + +void PictureLoaderWorker::saveRedirectCache() const +{ + QSettings settings(cacheFilePath, QSettings::IniFormat); + + settings.beginWriteArray(REDIRECT_HEADER_NAME, static_cast(redirectCache.size())); + int index = 0; + for (auto it = redirectCache.cbegin(); it != redirectCache.cend(); ++it) { + settings.setArrayIndex(index++); + settings.setValue(REDIRECT_ORIGINAL_URL, it.key()); + settings.setValue(REDIRECT_URL, it.value().first); + settings.setValue(REDIRECT_TIMESTAMP, it.value().second); + } + settings.endArray(); +} + +void PictureLoaderWorker::cleanStaleEntries() +{ + QDateTime now = QDateTime::currentDateTimeUtc(); + + auto it = redirectCache.begin(); + while (it != redirectCache.end()) { + if (it.value().second.addDays(SettingsCache::instance().getRedirectCacheTtl()) < now) { + it = redirectCache.erase(it); // Remove stale entry + } else { + ++it; + } + } } void PictureLoaderWorker::picDownloadFinished(QNetworkReply *reply) diff --git a/cockatrice/src/client/ui/picture_loader.h b/cockatrice/src/client/ui/picture_loader.h index 1c840eb40..245114358 100644 --- a/cockatrice/src/client/ui/picture_loader.h +++ b/cockatrice/src/client/ui/picture_loader.h @@ -11,6 +11,12 @@ class QNetworkAccessManager; class QNetworkReply; class QThread; +#define REDIRECT_HEADER_NAME "redirects" +#define REDIRECT_ORIGINAL_URL "original" +#define REDIRECT_URL "redirect" +#define REDIRECT_TIMESTAMP "timestamp" +#define REDIRECT_CACHE_FILENAME "cache.ini" + class PictureToLoad { private: @@ -83,6 +89,9 @@ private: QList loadQueue; QMutex mutex; QNetworkAccessManager *networkManager; + QHash> redirectCache; // Stores redirect and timestamp + QString cacheFilePath; // Path to persistent storage + static constexpr int CacheTTLInDays = 30; // TODO: Make user configurable QList cardsToDownload; PictureToLoad cardBeingLoaded; PictureToLoad cardBeingDownloaded; @@ -91,6 +100,12 @@ private: bool cardImageExistsOnDisk(QString &setName, QString &correctedCardName); bool imageIsBlackListed(const QByteArray &); QNetworkReply *makeRequest(const QUrl &url); + void cacheRedirect(const QUrl &originalUrl, const QUrl &redirectUrl); + QUrl getCachedRedirect(const QUrl &originalUrl) const; + void loadRedirectCache(); + void saveRedirectCache() const; + void cleanStaleEntries(); + private slots: void picDownloadFinished(QNetworkReply *reply); void picDownloadFailed(); diff --git a/cockatrice/src/dialogs/dlg_settings.cpp b/cockatrice/src/dialogs/dlg_settings.cpp index efdff930a..cef4ceaee 100644 --- a/cockatrice/src/dialogs/dlg_settings.cpp +++ b/cockatrice/src/dialogs/dlg_settings.cpp @@ -629,11 +629,22 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() networkCacheEdit.setValue(SettingsCache::instance().getNetworkCacheSizeInMB()); networkCacheEdit.setSuffix(" MB"); + networkRedirectCacheTtlEdit.setMinimum(NETWORK_REDIRECT_CACHE_TTL_MIN); + networkRedirectCacheTtlEdit.setMaximum(NETWORK_REDIRECT_CACHE_TTL_MAX); + networkRedirectCacheTtlEdit.setSingleStep(1); + networkRedirectCacheTtlEdit.setValue(SettingsCache::instance().getRedirectCacheTtl()); + networkRedirectCacheTtlEdit.setSuffix(" " + tr("Day(s)")); + auto networkCacheLayout = new QHBoxLayout; networkCacheLayout->addStretch(); networkCacheLayout->addWidget(&networkCacheLabel); networkCacheLayout->addWidget(&networkCacheEdit); + auto networkRedirectCacheLayout = new QHBoxLayout; + networkRedirectCacheLayout->addStretch(); + networkRedirectCacheLayout->addWidget(&networkRedirectCacheTtlLabel); + networkRedirectCacheLayout->addWidget(&networkRedirectCacheTtlEdit); + auto pixmapCacheLayout = new QHBoxLayout; pixmapCacheLayout->addStretch(); pixmapCacheLayout->addWidget(&pixmapCacheLabel); @@ -645,8 +656,9 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() lpGeneralGrid->addLayout(messageListLayout, 1, 0, 1, 2); lpGeneralGrid->addLayout(networkCacheLayout, 2, 0, 1, 2); lpGeneralGrid->addLayout(pixmapCacheLayout, 3, 0, 1, 2); - lpGeneralGrid->addWidget(&urlLinkLabel, 4, 0); - lpGeneralGrid->addWidget(&clearDownloadedPicsButton, 4, 1); + lpGeneralGrid->addLayout(networkRedirectCacheLayout, 4, 0, 1, 2); + lpGeneralGrid->addWidget(&urlLinkLabel, 5, 0); + lpGeneralGrid->addWidget(&clearDownloadedPicsButton, 5, 1); // Spoiler Layout lpSpoilerGrid->addWidget(&mcDownloadSpoilersCheckBox, 0, 0); @@ -657,13 +669,15 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() lpSpoilerGrid->addWidget(updateNowButton, 2, 1); lpSpoilerGrid->addWidget(&infoOnSpoilersLabel, 3, 0, 1, 3, Qt::AlignTop); - // On a change to the check box, hide/unhide the other fields + // On a change to the checkbox, hide/un-hide the other fields connect(&mcDownloadSpoilersCheckBox, SIGNAL(toggled(bool)), &SettingsCache::instance(), SLOT(setDownloadSpoilerStatus(bool))); connect(&mcDownloadSpoilersCheckBox, SIGNAL(toggled(bool)), this, SLOT(setSpoilersEnabled(bool))); connect(&pixmapCacheEdit, SIGNAL(valueChanged(int)), &SettingsCache::instance(), SLOT(setPixmapCacheSize(int))); connect(&networkCacheEdit, SIGNAL(valueChanged(int)), &SettingsCache::instance(), SLOT(setNetworkCacheSizeInMB(int))); + connect(&networkRedirectCacheTtlEdit, SIGNAL(valueChanged(int)), &SettingsCache::instance(), + SLOT(setNetworkRedirectCacheTtl(int))); mpGeneralGroupBox = new QGroupBox; mpGeneralGroupBox->setLayout(lpGeneralGrid); @@ -844,9 +858,11 @@ void DeckEditorSettingsPage::retranslateUi() urlLinkLabel.setText(QString("%2").arg(WIKI_CUSTOM_PIC_URL).arg(tr("How to add a custom URL"))); clearDownloadedPicsButton.setText(tr("Delete Downloaded Images")); resetDownloadURLs.setText(tr("Reset Download URLs")); - networkCacheLabel.setText(tr("Downloaded images directory size:")); + networkCacheLabel.setText(tr("Network Cache Size:")); networkCacheEdit.setToolTip(tr("On-disk cache for downloaded pictures")); - pixmapCacheLabel.setText(tr("Picture cache size:")); + networkRedirectCacheTtlLabel.setText(tr("Redirect Cache TTL:")); + networkRedirectCacheTtlEdit.setToolTip(tr("How long cached redirects for urls are valid for.")); + pixmapCacheLabel.setText(tr("Picture Cache Size:")); pixmapCacheEdit.setToolTip(tr("In-memory cache for pictures not currently on screen")); } diff --git a/cockatrice/src/dialogs/dlg_settings.h b/cockatrice/src/dialogs/dlg_settings.h index 70915cc0e..09c5ded13 100644 --- a/cockatrice/src/dialogs/dlg_settings.h +++ b/cockatrice/src/dialogs/dlg_settings.h @@ -174,6 +174,8 @@ private: QPushButton *updateNowButton; QLabel networkCacheLabel; QSpinBox networkCacheEdit; + QLabel networkRedirectCacheTtlLabel; + QSpinBox networkRedirectCacheTtlEdit; QSpinBox pixmapCacheEdit; QLabel pixmapCacheLabel; }; diff --git a/cockatrice/src/settings/cache_settings.cpp b/cockatrice/src/settings/cache_settings.cpp index 05de9fd48..da0e5c0b0 100644 --- a/cockatrice/src/settings/cache_settings.cpp +++ b/cockatrice/src/settings/cache_settings.cpp @@ -221,6 +221,7 @@ SettingsCache::SettingsCache() pixmapCacheSize = PIXMAPCACHE_SIZE_DEFAULT; networkCacheSize = settings->value("personal/networkCacheSize", NETWORK_CACHE_SIZE_DEFAULT).toInt(); + redirectCacheTtl = settings->value("personal/redirectCacheTtl", NETWORK_REDIRECT_CACHE_TTL_DEFAULT).toInt(); picDownload = settings->value("personal/picturedownload", true).toBool(); @@ -657,6 +658,13 @@ void SettingsCache::setNetworkCacheSizeInMB(const int _networkCacheSize) emit networkCacheSizeChanged(networkCacheSize); } +void SettingsCache::setNetworkRedirectCacheTtl(const int _redirectCacheTtl) +{ + redirectCacheTtl = _redirectCacheTtl; + settings->setValue("personal/redirectCacheSize", redirectCacheTtl); + emit redirectCacheTtlChanged(redirectCacheTtl); +} + void SettingsCache::setClientID(const QString &_clientID) { clientID = _clientID; @@ -1030,6 +1038,7 @@ void SettingsCache::loadPaths() replaysPath = getSafeConfigPath("paths/replays", dataPath + "/replays/"); themesPath = getSafeConfigPath("paths/themes", dataPath + "/themes/"); picsPath = getSafeConfigPath("paths/pics", dataPath + "/pics/"); + redirectCachePath = getSafeConfigPath("paths/redirects", getCachePath() + "/redirects/"); // this has never been exposed as an user-configurable setting if (picsPath.endsWith("/")) { customPicsPath = getSafeConfigPath("paths/custompics", picsPath + "CUSTOM/"); diff --git a/cockatrice/src/settings/cache_settings.h b/cockatrice/src/settings/cache_settings.h index e0bf8b9ed..fdfdb6f53 100644 --- a/cockatrice/src/settings/cache_settings.h +++ b/cockatrice/src/settings/cache_settings.h @@ -16,16 +16,21 @@ class ReleaseChannel; -// size should be a multiple of 64 -#define PIXMAPCACHE_SIZE_DEFAULT 2047 +// In MB (Increments of 64) +#define PIXMAPCACHE_SIZE_DEFAULT 2048 #define PIXMAPCACHE_SIZE_MIN 64 -#define PIXMAPCACHE_SIZE_MAX 2047 +#define PIXMAPCACHE_SIZE_MAX 4096 // In MB constexpr int NETWORK_CACHE_SIZE_DEFAULT = 1024 * 4; // 4 GB constexpr int NETWORK_CACHE_SIZE_MIN = 1; // 1 MB constexpr int NETWORK_CACHE_SIZE_MAX = 1024 * 1024; // 1 TB +// In Days +#define NETWORK_REDIRECT_CACHE_TTL_DEFAULT 30 +#define NETWORK_REDIRECT_CACHE_TTL_MIN 1 +#define NETWORK_REDIRECT_CACHE_TTL_MAX 90 + #define DEFAULT_LANG_NAME "English" #define CLIENT_INFO_NOT_SET "notset" @@ -53,6 +58,7 @@ signals: void ignoreUnregisteredUserMessagesChanged(); void pixmapCacheSizeChanged(int newSizeInMBs); void networkCacheSizeChanged(int newSizeInMBs); + void redirectCacheTtlChanged(int newTtl); void masterVolumeChanged(int value); void chatMentionCompleterChanged(); void downloadSpoilerTimeIndexChanged(); @@ -73,8 +79,8 @@ private: QByteArray tokenDialogGeometry; QByteArray setsDialogGeometry; QString lang; - QString deckPath, replaysPath, picsPath, customPicsPath, cardDatabasePath, customCardDatabasePath, themesPath, - spoilerDatabasePath, tokenDatabasePath, themeName; + QString deckPath, replaysPath, picsPath, redirectCachePath, customPicsPath, cardDatabasePath, + customCardDatabasePath, themesPath, spoilerDatabasePath, tokenDatabasePath, themeName; bool notifyAboutUpdates; bool notifyAboutNewVersion; bool showTipsOnStartup; @@ -116,6 +122,7 @@ private: bool useTearOffMenus; int pixmapCacheSize; int networkCacheSize; + int redirectCacheTtl; bool scaleCards; int verticalCardOverlapPercent; bool showMessagePopups; @@ -183,6 +190,10 @@ public: { return picsPath; } + QString getRedirectCachePath() const + { + return redirectCachePath; + } QString getCustomPicsPath() const { return customPicsPath; @@ -356,6 +367,10 @@ public: { return networkCacheSize; } + int getRedirectCacheTtl() const + { + return redirectCacheTtl; + } bool getScaleCards() const { return scaleCards; @@ -557,6 +572,7 @@ public slots: void setIgnoreUnregisteredUserMessages(QT_STATE_CHANGED_T _ignoreUnregisteredUserMessages); void setPixmapCacheSize(const int _pixmapCacheSize); void setNetworkCacheSizeInMB(const int _networkCacheSize); + void setNetworkRedirectCacheTtl(const int _redirectCacheTtl); void setCardScaling(const QT_STATE_CHANGED_T _scaleCards); void setStackCardOverlapPercent(const int _verticalCardOverlapPercent); void setShowMessagePopups(const QT_STATE_CHANGED_T _showMessagePopups); diff --git a/dbconverter/src/mocks.cpp b/dbconverter/src/mocks.cpp index 37c1424dc..1c90612fa 100644 --- a/dbconverter/src/mocks.cpp +++ b/dbconverter/src/mocks.cpp @@ -220,6 +220,9 @@ void SettingsCache::setPixmapCacheSize(const int /* _pixmapCacheSize */) void SettingsCache::setNetworkCacheSizeInMB(const int /* _networkCacheSize */) { } +void SettingsCache::setNetworkRedirectCacheTtl(const int /* _redirectCacheTtl */) +{ +} void SettingsCache::setClientID(const QString & /* _clientID */) { } diff --git a/tests/carddatabase/mocks.cpp b/tests/carddatabase/mocks.cpp index 112fc9416..f311241de 100644 --- a/tests/carddatabase/mocks.cpp +++ b/tests/carddatabase/mocks.cpp @@ -224,6 +224,9 @@ void SettingsCache::setPixmapCacheSize(const int /* _pixmapCacheSize */) void SettingsCache::setNetworkCacheSizeInMB(const int /* _networkCacheSize */) { } +void SettingsCache::setNetworkRedirectCacheTtl(const int /* _redirectCacheTtl */) +{ +} void SettingsCache::setClientID(const QString & /* _clientID */) { }