diff --git a/.ci/Debian12/Dockerfile b/.ci/Debian12/Dockerfile index fc756aac2..14c6f018c 100644 --- a/.ci/Debian12/Dockerfile +++ b/.ci/Debian12/Dockerfile @@ -4,7 +4,6 @@ RUN apt-get update && \ DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ build-essential \ ccache \ - clang-format \ cmake \ file \ g++ \ diff --git a/.ci/Debian13/Dockerfile b/.ci/Debian13/Dockerfile index bdecb56df..b597307c5 100644 --- a/.ci/Debian13/Dockerfile +++ b/.ci/Debian13/Dockerfile @@ -5,7 +5,6 @@ RUN apt-get update && \ build-essential \ ca-certificates \ ccache \ - clang-format \ cmake \ file \ g++ \ diff --git a/.ci/Servatrice_Debian12/Dockerfile b/.ci/Servatrice_Debian12/Dockerfile index 321aa7c0f..f1a16843a 100644 --- a/.ci/Servatrice_Debian12/Dockerfile +++ b/.ci/Servatrice_Debian12/Dockerfile @@ -4,7 +4,6 @@ RUN apt-get update && \ DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ build-essential \ ccache \ - clang-format \ cmake \ file \ g++ \ diff --git a/.ci/Ubuntu24.04/Dockerfile b/.ci/Ubuntu24.04/Dockerfile index 715997474..b9fb2b701 100644 --- a/.ci/Ubuntu24.04/Dockerfile +++ b/.ci/Ubuntu24.04/Dockerfile @@ -4,7 +4,6 @@ RUN apt-get update && \ DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ build-essential \ ccache \ - clang-format \ cmake \ file \ g++ \ diff --git a/.ci/Ubuntu26.04/Dockerfile b/.ci/Ubuntu26.04/Dockerfile index 3166b7eb4..cc6528858 100644 --- a/.ci/Ubuntu26.04/Dockerfile +++ b/.ci/Ubuntu26.04/Dockerfile @@ -5,7 +5,6 @@ RUN apt-get update && \ build-essential \ ca-certificates \ ccache \ - clang-format \ cmake \ file \ g++ \ diff --git a/.ci/compile.sh b/.ci/compile.sh index 606b9d201..302972f30 100755 --- a/.ci/compile.sh +++ b/.ci/compile.sh @@ -17,7 +17,7 @@ # --dir sets the name of the build dir, default is "build" # --cmake-generator sets CMAKE_GENERATOR as used by cmake # --target-macos-version sets the min os version - only used for macOS builds -# uses env: BUILDTYPE MAKE_INSTALL MAKE_PACKAGE PACKAGE_TYPE PACKAGE_SUFFIX MAKE_SERVER MAKE_NO_CLIENT MAKE_TEST USE_CCACHE CCACHE_SIZE CCACHE_EVICTION_AGE BUILD_DIR CMAKE_GENERATOR TARGET_MACOS_VERSION MAKE_SANITIZE MAKE_COVERAGE +# uses env: BUILDTYPE MAKE_INSTALL MAKE_PACKAGE PACKAGE_TYPE PACKAGE_SUFFIX MAKE_SERVER MAKE_NO_CLIENT MAKE_TEST USE_CCACHE CCACHE_SIZE CCACHE_EVICTION_AGE BUILD_DIR CMAKE_GENERATOR TARGET_MACOS_VERSION MAKE_SANITIZE MAKE_COVERAGE VCPKG_TARGET_MACOS_VERSION # (correspond to args: --debug/--release --install --package --suffix --server --test --sanitize --coverage --ccache --dir ) # exitcode: 1 for failure, 3 for invalid arguments @@ -243,8 +243,14 @@ if [[ $RUNNER_OS == macOS ]]; then triplet_source="../vcpkg/triplets/community/$arch-osx.cmake" fi cp "$triplet_source" "$triplet_file" - echo "set(VCPKG_CMAKE_SYSTEM_VERSION $TARGET_MACOS_VERSION)" >>"$triplet_file" - echo "set(VCPKG_OSX_DEPLOYMENT_TARGET $TARGET_MACOS_VERSION)" >>"$triplet_file" + # The triplet is what vcpkg hashes into the ABI hash of every port, so two legs that + # agree here reuse each other's binary cache. Targets 14, 15 and 26 on the same Xcode + # produce identical binaries when they share the lowest deployment target, so they all + # build against 14 to keep one vcpkg cache scope instead of three. + # Keep in sync with "vcpkg_scope" and "vcpkg_deployment_target" in desktop-build.yml. + vcpkg_target="${VCPKG_TARGET_MACOS_VERSION:-$TARGET_MACOS_VERSION}" + echo "set(VCPKG_CMAKE_SYSTEM_VERSION $vcpkg_target)" >>"$triplet_file" + echo "set(VCPKG_OSX_DEPLOYMENT_TARGET $vcpkg_target)" >>"$triplet_file" flags+=("-DVCPKG_OVERLAY_TRIPLETS=$triplets_dir") flags+=("-DVCPKG_HOST_TRIPLET=$triplet_version") flags+=("-DVCPKG_TARGET_TRIPLET=$triplet_version") diff --git a/.github/workflows/desktop-build.yml b/.github/workflows/desktop-build.yml index c532bc615..2bc0ac11d 100644 --- a/.github/workflows/desktop-build.yml +++ b/.github/workflows/desktop-build.yml @@ -338,6 +338,8 @@ jobs: soc: Intel type: Release use_ccache: 1 + vcpkg_scope: 'vcpkg/macos13-x64-xcode26.3/' + vcpkg_upload: 1 # only one leg per vcpkg_scope hands its archives over xcode: "26.3" - os: macOS @@ -354,6 +356,9 @@ jobs: soc: Apple type: Release use_ccache: 1 + vcpkg_deployment_target: 14 + vcpkg_scope: 'vcpkg/macos14-arm64-xcode26.6/' + vcpkg_upload: 1 # only one leg per vcpkg_scope hands its archives over xcode: "26.6" - os: macOS @@ -370,6 +375,8 @@ jobs: soc: Apple type: Release use_ccache: 1 + vcpkg_deployment_target: 14 + vcpkg_scope: 'vcpkg/macos14-arm64-xcode26.6/' xcode: "26.6" - os: macOS @@ -383,6 +390,8 @@ jobs: soc: Apple type: Debug use_ccache: 1 + vcpkg_deployment_target: 14 + vcpkg_scope: 'vcpkg/macos14-arm64-xcode26.6/' xcode: "26.6" - os: Windows @@ -396,6 +405,8 @@ jobs: qt_version: 6.11.* qt_modules: qtimageformats qtmultimedia qtshadertools qtwebsockets type: Release + vcpkg_scope: 'vcpkg/windows10-x64-vs2026/' + vcpkg_upload: 1 # only one leg per vcpkg_scope hands its archives over name: ${{ matrix.os }} ${{ matrix.target }}${{ matrix.soc == 'Intel' && ' Intel' || '' }}${{ matrix.type == 'Debug' && ' Debug' || '' }} needs: configure @@ -493,8 +504,28 @@ jobs: id: vcpkg-cache uses: TAServers/vcpkg-cache@v3 with: + # Every entry is keyed by vcpkg ABI hash, and one hash set exists per platform, + # target and toolchain. The action restores every entry it finds under the + # prefix, not just the hashes vcpkg asks for, so with one shared vcpkg/ + # namespace every leg downloads all 66 archives of all 5 legs on every run. + # Each of those restores also refreshes the entry's last access date, which + # is why a dead ABI generation is never evicted by age. Naming the scope is + # what makes those generations identifiable, and prune-caches below able to + # delete them. + prefix: ${{ matrix.vcpkg_scope }} token: ${{ secrets.GITHUB_TOKEN }} + # The action only creates its directory when the prefix matched at least one + # entry, and it reads that directory unguarded when it saves at the end of the + # job. A scope nobody has written to yet therefore leaves the directory missing + # and fails the job, which is exactly the state every scope starts in. Make the + # directory up front so an empty scope is a cache miss instead of an error. + - name: "Create vcpkg cache directory" + shell: bash + env: + CACHE_DIR: '${{ steps.vcpkg-cache.outputs.path }}' + run: mkdir -p "$CACHE_DIR" + # Uses environment variables, see compile.sh for more details - name: "Build Cockatrice" id: build @@ -514,9 +545,19 @@ jobs: PACKAGE_SUFFIX: '${{ matrix.package_suffix }}' TARGET_MACOS_VERSION: ${{ matrix.override_target }} USE_CCACHE: ${{ matrix.use_ccache }} + # Only pushes to the default branch may write the shared binary cache: the + # archives a run compiles are only as trustworthy as the code they were + # built from, and only merged code may populate the scope that master and + # release builds restore and link. Pull requests (same-repository ones + # included) restore the shared cache but never write it; with the source + # left at read the collect step below finds nothing on PRs, so no + # pull-request-scoped entry is ever saved that no later run could restore. + # Forks pushing to their own default branch resolve github.ref against + # their own repository, so they keep a working cache of their own there. VCPKG_BINARY_SOURCES: "clear;files,${{ steps.vcpkg-cache.outputs.path }},${{ case(github.ref == 'refs/heads/master', 'readwrite', 'read') }}" VCPKG_DISABLE_METRICS: 1 VCPKG_FEATURE_FLAGS: dependencygraph + VCPKG_TARGET_MACOS_VERSION: ${{ matrix.vcpkg_deployment_target }} run: .ci/compile.sh --server --test --vcpkg # CPack reports a failed makensis run as "Problem running NSIS command" @@ -530,6 +571,63 @@ jobs: shell: bash run: find . -name NSISOutput.log -exec cat {} + + # A master run hands over the archives it had to build itself so that + # vcpkg-cache-import.yml, which runs in the default branch scope, can save them + # where later runs can restore them. Pull requests hand over nothing: their cache + # entries would be scoped to refs/pull//merge and stay unreadable for every + # other run, so writing them only burns the repository's quota. + # vcpkg_upload keeps this to one leg per scope: macOS 14, 15 and 26 share a scope + # by design, and all three rebuild the same archives, so uploading from each of them + # would publish three identical 269 MB artifacts under one artifact name and make + # the importer spawn a save job per copy. + - name: "Collect new vcpkg entries" + id: vcpkg_new + if: github.ref == 'refs/heads/master' && matrix.vcpkg_upload == 1 + shell: bash + env: + CACHE_DIR: '${{ steps.vcpkg-cache.outputs.path }}' + GH_TOKEN: ${{ github.token }} + VCPKG_SCOPE: '${{ matrix.vcpkg_scope }}' + run: | + # The archive name is the ABI hash, so a missing key is exactly what vcpkg had + # to compile from source in this job. + gh api --paginate \ + "repos/$GITHUB_REPOSITORY/actions/caches?ref=refs/heads/master&key=$VCPKG_SCOPE&per_page=100" \ + --jq '.actions_caches[].key' | sort -u >"$RUNNER_TEMP/master-keys.txt" + find "$CACHE_DIR" -type f -name '*.zip' -exec basename {} .zip \; | + sed "s|^|$VCPKG_SCOPE|" | sort -u >"$RUNNER_TEMP/local-keys.txt" + comm -23 "$RUNNER_TEMP/local-keys.txt" "$RUNNER_TEMP/master-keys.txt" >"$RUNNER_TEMP/new-keys.txt" + + mkdir -p "$RUNNER_TEMP/vcpkg-new" + : >"$RUNNER_TEMP/vcpkg-new/keys.txt" + while read -r key; do + hash="${key##*/}" + archive="$(find "$CACHE_DIR" -type f -name "$hash.zip" -print -quit)" + dir="$RUNNER_TEMP/vcpkg-new/${hash:0:2}" # vcpkg's own two character fanout + mkdir -p "$dir" + cp "$archive" "$dir/" + echo "$key ${hash:0:2}/$hash.zip" >>"$RUNNER_TEMP/vcpkg-new/keys.txt" + done <"$RUNNER_TEMP/new-keys.txt" + + count="$(wc -l <"$RUNNER_TEMP/new-keys.txt")" + echo "vcpkg scope $VCPKG_SCOPE: $count entries to import" + echo "count=$count" >>"$GITHUB_OUTPUT" + echo "slug=$(sed -e 's|^vcpkg/||' -e 's|/$||' -e 's|/|-|g' <<<"$VCPKG_SCOPE")" >>"$GITHUB_OUTPUT" + + # archive: false accepts a single file and nothing else, and a scope hands over one + # archive per ABI hash vcpkg had to rebuild, which is 23 files on a cold runner. + # Zipping the directory keeps the fanout at the artifact root, which is where + # vcpkg-cache-import.yml looks for keys.txt once download-artifact unpacks it. + - name: "Upload new vcpkg entries" + # An empty count means the step above did not run at all, on master and on forks + if: steps.vcpkg_new.outputs.count != '' && steps.vcpkg_new.outputs.count != '0' + uses: actions/upload-artifact@v7 + with: + if-no-files-found: error # a count above zero with nothing staged is a bug + name: vcpkg-new-${{ steps.vcpkg_new.outputs.slug }} + path: ${{ runner.temp }}/vcpkg-new + retention-days: 1 + # Delete used cache to emulate a ccache update. See https://github.com/actions/cache/issues/342 - name: "[macOS] Delete remote compiler cache (ccache)" if: matrix.os == 'macOS' && matrix.use_ccache == 1 && github.ref == 'refs/heads/master' && steps.restore_ccache.outputs.cache-hit @@ -642,3 +740,67 @@ jobs: BUILD_PATH: ${{ steps.build.outputs.path }} GH_TOKEN: ${{ github.token }} run: gh attestation verify "$BUILD_PATH" --repo Cockatrice/Cockatrice + + prune-caches: + name: Prune caches + # Only master writes caches at all, so only master may clean them up too + if: github.ref == 'refs/heads/master' + runs-on: ubuntu-slim # https://github.com/actions/runner-images/blob/main/images/ubuntu-slim/ubuntu-slim-Readme.md + continue-on-error: true # freeing space must never fail a build + + steps: + # The repository holds 10Gi of cache against entries that want more, and GitHub + # evicts the least recently used entry whenever that is exceeded. The vcpkg + # entries are the reason that never happens: the restore above touches every + # entry under the prefix on every run, so a dead ABI generation keeps its access + # date fresh forever and age based eviction can never reach it. Deleting by age + # alone cannot fix that, only knowing which scope an entry belongs to can. + - name: "Delete unusable cache entries" + shell: bash + env: + GH_TOKEN: ${{ github.token }} + # Must list every vcpkg_scope of the build-vcpkg matrix above, or the entries of + # a scope that is still in use get deleted on the next run. + LIVE_VCPKG_SCOPES: 'vcpkg/macos13-x64-xcode26.3/ vcpkg/macos14-arm64-xcode26.6/ vcpkg/windows10-x64-vs2026/' + run: | + gh api --paginate "repos/$GITHUB_REPOSITORY/actions/caches?per_page=100" \ + --jq '.actions_caches[] | [.id, .ref, .key, .last_accessed_at] | @tsv' >"$RUNNER_TEMP/caches.tsv" + + cutoff="$(( $(date -u +%s) - 7 * 24 * 3600 ))" + live=" ${LIVE_VCPKG_SCOPES} " + deleted=0 + while IFS=$'\t' read -r id ref key accessed; do + [[ -n $id ]] || continue + # Only ever touch the key shapes this workflow creates, so that an entry of + # some other workflow can never be deleted by accident + case "$key" in + vcpkg/* | ccache-* | Qt*) ;; + *) continue ;; + esac + + reason="" + if [[ $ref != refs/heads/master ]]; then + reason="scoped to $ref" + elif [[ $key == vcpkg/* ]]; then + # "/", cut the hash off the end to get the scope + scope="vcpkg/${key#vcpkg/}" + scope="${scope%/*}/" + if [[ $live != *" $scope "* ]]; then + reason="no matrix leg uses $scope" + fi + fi + if [[ -z $reason ]]; then + stamp="$(date -u -d "$accessed" +%s 2>/dev/null || echo 0)" + if [[ $stamp -ne 0 && $stamp -lt $cutoff ]]; then + reason="unused since $accessed" + fi + fi + + if [[ -n $reason ]]; then + echo "::notice::deleting $key, $reason" + gh api -X DELETE "repos/$GITHUB_REPOSITORY/actions/caches/$id" + deleted=$((deleted + 1)) + fi + done <"$RUNNER_TEMP/caches.tsv" + + echo "::notice::deleted $deleted of $(wc -l <"$RUNNER_TEMP/caches.tsv") cache entries" diff --git a/.github/workflows/vcpkg-cache-import.yml b/.github/workflows/vcpkg-cache-import.yml new file mode 100644 index 000000000..3cc7b2203 --- /dev/null +++ b/.github/workflows/vcpkg-cache-import.yml @@ -0,0 +1,121 @@ +name: Import vcpkg cache + +# After a runner image update changes every port's ABI hash, the first master run +# rebuilds protobuf, openssl and abseil from source and saves them into the shared +# cache, where every later run can restore them. +# +# Only master pushes are imported here: the collect step, keys.txt and the zip +# contents all come from the workflow run being imported, so importing from an +# unmerged pull request would let anyone who can push a branch — or a bot actor +# such as Dependabot, whose PRs are same-repo — promote arbitrary binaries into +# the scope that master and tag builds restore and link into signed releases. +# Forks build untrusted code and must never reach that scope either. +# +# repository_dispatch is not used on purpose: it would also resolve to the default branch, +# but it counts as a low-trust trigger and may only write there when it declares a +# write-capable cache-mode, which puts it in the same position as workflow_run. + +permissions: + actions: write # needed to save entries into the default branch's cache scope + contents: read + +on: + workflow_run: + workflows: ["Build Desktop"] + types: [completed] + +# Every import writes the same scopes, so let them queue instead of racing each other +concurrency: + group: vcpkg-cache-import + cancel-in-progress: false + +jobs: + list: + name: List new vcpkg entries + # Only a successful push to the default branch has archives worth importing. + # Only code that landed on master may populate the scope master runs restore. + # Bot actors are excluded on top: their runs must never promote binaries into + # the shared scope even if they somehow target the default branch. + if: >- + github.event.workflow_run.conclusion == 'success' && + github.event.workflow_run.event == 'push' && + github.event.workflow_run.head_branch == 'master' && + github.event.workflow_run.head_repository.full_name == github.repository && + !endsWith(github.event.workflow_run.actor.login, '[bot]') && + !endsWith(github.event.workflow_run.triggering_actor.login, '[bot]') + runs-on: ubuntu-slim # https://github.com/actions/runner-images/blob/main/images/ubuntu-slim/ubuntu-slim-Readme.md + # workflow_run is a low-trust trigger, so cache access is read-only by default even + # though this run resolves to the default branch. Assert below that this took effect, + # because a save under read-only access only warns and would import nothing silently. + cache-mode: write + outputs: + entries: ${{ steps.scan.outputs.entries }} + + steps: + - name: "Download new vcpkg entries" + uses: actions/download-artifact@v8 + with: + pattern: vcpkg-new-* + path: incoming + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ secrets.GITHUB_TOKEN }} + + - name: "Assert write access" + shell: bash + run: | + if [[ $ACTIONS_CACHE_MODE != *write* ]]; then + echo "::error::expected a write-capable cache, got '$ACTIONS_CACHE_MODE'" + exit 1 + fi + + - name: "List cache entries" + id: scan + shell: bash + working-directory: incoming + run: | + entries="$(find . -name keys.txt -type f -exec sh -c ' + for manifest; do + artifact="$(basename "$(dirname "$manifest")")" + while read -r key path; do + printf "{\"artifact\": \"%s\", \"key\": \"%s\", \"path\": \"%s\"}\n" \ + "$artifact" "$key" "$path" + done <"$manifest" + done + ' _ {} +)" + [[ -n $entries ]] || exit 0 + # Compact, because a multi line value cannot be written to GITHUB_OUTPUT + entries="$(jq -cs '.' <<<"$entries")" + echo "entries=$entries" >>"$GITHUB_OUTPUT" + echo "::notice::importing $(jq length <<<"$entries") vcpkg entries" + + save: + name: "Save ${{ matrix.entry.key }}" + needs: list + if: needs.list.outputs.entries + runs-on: ubuntu-slim + cache-mode: write + strategy: + # One job per archive, they run in parallel and each only uploads a single file + fail-fast: false + max-parallel: 20 + matrix: + entry: ${{ fromJSON(needs.list.outputs.entries) }} + + steps: + - name: "Download new vcpkg entries" + uses: actions/download-artifact@v8 + with: + pattern: ${{ matrix.entry.artifact }} + path: incoming + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ secrets.GITHUB_TOKEN }} + + - name: "Save vcpkg entry" + uses: actions/cache/save@v6 + with: + # The payload is a Windows or macOS archive while the upload runs on Linux, and + # without this the entry would be stamped as a Linux cache that neither of them + # is allowed to restore. + enableCrossOsArchive: true + key: ${{ matrix.entry.key }} + path: incoming/${{ matrix.entry.path }} \ No newline at end of file diff --git a/CMakeLists.txt b/CMakeLists.txt index b272d4ad8..bee47972c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -90,7 +90,7 @@ endif() # A project name is needed for CPack # Version can be overriden by git tags, see cmake/getversion.cmake -project("Cockatrice" VERSION 3.1.1) +project("Cockatrice" VERSION 3.1.2) # Set release name if not provided via env/cmake var if(NOT DEFINED GIT_TAG_RELEASENAME) diff --git a/cockatrice/src/client/network/update/card_spoiler/spoiler_background_updater.cpp b/cockatrice/src/client/network/update/card_spoiler/spoiler_background_updater.cpp index ae5e92d6c..192029b23 100644 --- a/cockatrice/src/client/network/update/card_spoiler/spoiler_background_updater.cpp +++ b/cockatrice/src/client/network/update/card_spoiler/spoiler_background_updater.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -177,7 +178,15 @@ bool SpoilerBackgroundUpdater::saveDownloadedFile(QByteArray data) // Data written, so reload the card database qCInfo(SpoilerBackgroundUpdaterLog) << "Spoiler Service Data Written"; - const auto reloadOk = QtConcurrent::run([] { CardDatabaseManager::getInstance()->loadCardDatabases(); }); + // Spoilers can introduce sets the config has never seen; the loader never + // runs checkUnknownSets() itself, so run it once the finished snapshot has + // been swapped in. + auto *watcher = new QFutureWatcher(this); + connect(watcher, &QFutureWatcher::finished, this, [watcher] { + watcher->deleteLater(); + CardDatabaseManager::getInstance()->checkUnknownSets(); + }); + watcher->setFuture(QtConcurrent::run([] { CardDatabaseManager::getInstance()->loadCardDatabases(); })); // If the user has notifications enabled, let them know // when the database was last updated diff --git a/cockatrice/src/client/settings/cache_settings.cpp b/cockatrice/src/client/settings/cache_settings.cpp index 15a41e7e1..08eccf1e4 100644 --- a/cockatrice/src/client/settings/cache_settings.cpp +++ b/cockatrice/src/client/settings/cache_settings.cpp @@ -133,6 +133,10 @@ SettingsCache::SettingsCache() // Migrate settings from global.ini to per-class INI files (runs at most once) SettingsMigration::migrateSettingsFromGlobalIni(settingsPath); + // Repair gamefilters.ini keys renamed to camelCase by #7084 for profiles that migrated + // before that rename (idempotent, safe to run on every startup, portable builds included). + SettingsMigration::migrateGameFiltersToCamelCase(settingsPath); + shortcutsSettings = new ShortcutsSettings(settingsPath, this); cardDatabaseSettings = new CardDatabaseSettings(settingsPath, this); serversSettings = new ServersSettings(settingsPath, this); diff --git a/cockatrice/src/interface/widgets/server/game_filter_configs.h b/cockatrice/src/interface/widgets/server/game_filter_configs.h index 1b7322c55..93ca010d8 100644 --- a/cockatrice/src/interface/widgets/server/game_filter_configs.h +++ b/cockatrice/src/interface/widgets/server/game_filter_configs.h @@ -17,7 +17,9 @@ struct GameFilterConfigs static constexpr int DEFAULT_MAX_PLAYERS_MAX = 99; bool hideBuddiesOnlyGames = false; - bool hideIgnoredUserGames = false; + // Defaults to true to match GameFiltersSettings::isHideIgnoredUserGames(), so freshly loaded + // filters compare equal to a default-constructed config. + bool hideIgnoredUserGames = true; bool hideFullGames = false; bool hideGamesThatStarted = false; bool hidePasswordProtectedGames = false; diff --git a/cockatrice/src/interface/widgets/server/game_selector.cpp b/cockatrice/src/interface/widgets/server/game_selector.cpp index 27464e14e..6922bfe64 100644 --- a/cockatrice/src/interface/widgets/server/game_selector.cpp +++ b/cockatrice/src/interface/widgets/server/game_selector.cpp @@ -113,6 +113,17 @@ GameSelector::GameSelector(AbstractClient *_client, quickFilterToolBar->setVisible( SettingsCache::instance().userInterface().getShowGameSelectorFilterToolbar()); }); + // The quick-filter toolbar only updates the in-memory model, so persist every change. + // Otherwise each new GameSelector (room switch, reconnect, restart) would revert to stale + // saved values and e.g. hide full/started games the user had unchecked. The save is + // debounced so typing in the search field does not rewrite the settings file on every + // keystroke; a pending save is flushed in the destructor. + filterSaveDebounceTimer.setSingleShot(true); + filterSaveDebounceTimer.setInterval(500); + connect(&filterSaveDebounceTimer, &QTimer::timeout, this, + [this] { gameListProxyModel->saveFilterParameters(gameTypeMap); }); + connect(gameListProxyModel, &GamesProxyModel::filtersChanged, this, + [this] { filterSaveDebounceTimer.start(); }); } else { quickFilterToolBar = nullptr; } @@ -187,6 +198,18 @@ GameSelector::GameSelector(AbstractClient *_client, connect(client, &AbstractClient::removeFromListEventReceived, this, &GameSelector::processRemoveFromListEvent); } +GameSelector::~GameSelector() +{ + // Nothing else persists quick-filter toolbar edits: the toolbar only updates the in-memory model, + // and the debounced timer above is the sole writer for that path (actSetFilter/actClearFilter save + // synchronously, but the toolbar does not). Flush a pending debounced save so edits made within + // the debounce interval before destruction are not lost on room switch, reconnect, or close. + if (filterSaveDebounceTimer.isActive()) { + filterSaveDebounceTimer.stop(); + gameListProxyModel->saveFilterParameters(gameTypeMap); + } +} + void GameSelector::ignoreListReceived(const QList &) { gameListProxyModel->refresh(); diff --git a/cockatrice/src/interface/widgets/server/game_selector.h b/cockatrice/src/interface/widgets/server/game_selector.h index 01e093b06..e3407abea 100644 --- a/cockatrice/src/interface/widgets/server/game_selector.h +++ b/cockatrice/src/interface/widgets/server/game_selector.h @@ -4,6 +4,7 @@ #include "game_type_map.h" #include +#include #include // IWYU pragma: keep // ServerInfo_User appears by value and inside QList in slot signatures, so the @@ -148,6 +149,9 @@ private: const bool showFilters; /**< Determines whether filter buttons are displayed. */ GameTypeMap gameTypeMap; /**< Mapping of game types for the current room. */ + QTimer filterSaveDebounceTimer; /**< Delays persisting quick-filter edits so typing does not rewrite the settings + file on every keystroke. */ + /** * @brief Updates the widget title to reflect the current number of displayed games. * @@ -211,6 +215,7 @@ public: const bool restoresettings, const bool _showfilters, QWidget *parent = nullptr); + ~GameSelector() override; /** * @brief Updates UI text for translation/localization. diff --git a/cockatrice/src/interface/widgets/server/games_model.cpp b/cockatrice/src/interface/widgets/server/games_model.cpp index 289dbee44..315d51d9e 100644 --- a/cockatrice/src/interface/widgets/server/games_model.cpp +++ b/cockatrice/src/interface/widgets/server/games_model.cpp @@ -523,6 +523,9 @@ bool GamesProxyModel::filterAcceptsRow(int sourceRow) const void GamesProxyModel::refresh() { +#if (QT_VERSION >= QT_VERSION_CHECK(6, 9, 0)) + beginFilterChange(); +#endif #if (QT_VERSION >= QT_VERSION_CHECK(6, 10, 0)) endFilterChange(QSortFilterProxyModel::Direction::Rows); #else diff --git a/cockatrice/src/interface/window_main.cpp b/cockatrice/src/interface/window_main.cpp index 4f363181e..815cec1d2 100644 --- a/cockatrice/src/interface/window_main.cpp +++ b/cockatrice/src/interface/window_main.cpp @@ -70,6 +70,7 @@ #include #include #include +#include #include #include #include @@ -640,10 +641,10 @@ void MainWindow::startupConfigCheck() const bool isCleanInstall = SettingsCache::instance().network().getClientVersion() == CLIENT_INFO_NOT_SET; - // checkUnknownSets() is intentionally deferred from the card database load - // (which runs in main() before MainWindow exists) so that - // cardDatabaseNewSetsFound / cardDatabaseAllNewSetsEnabled have live - // receivers when emitted. + // The loader never runs checkUnknownSets() itself (see main.cpp): the + // front-loaded parse runs before MainWindow exists, so + // cardDatabaseNewSetsFound / cardDatabaseAllNewSetsEnabled would have no + // receivers. MainWindow owns the check after startup. // On a clean install the onboarding wizard owns the first-run experience; // wait until it closes so the legacy "all sets enabled" welcome and the // Manage Sets dialog don't appear first. @@ -1137,18 +1138,35 @@ void MainWindow::cardDatabaseNewSetsFound(int numUnknownSets, QStringList unknow void MainWindow::cardDatabaseAllNewSetsEnabled() { - if (firstRunWizardActive || CardDatabaseManager::getInstance()->getCardList().isEmpty()) { - // The onboarding wizard owns the first-run messaging on a clean install, - // and with no card data there are no sets to have enabled. + auto *cardDatabase = CardDatabaseManager::getInstance(); + + // With no card data there is nothing to have enabled and nothing to reparse. + if (cardDatabase->getCardList().isEmpty()) { return; } - QMessageBox::information( - this, tr("Welcome"), - tr("Hi! It seems like you're running this version of Cockatrice for the first time.\nAll the sets in the card " - "database have been enabled.\nRead more about changing the set order or disabling specific sets and " - "consequent effects in the \"Manage Sets\" dialog.")); - actManageSets(); + // checkUnknownSets() enabled every set after the database had already been + // parsed under the (empty) first-run enablement, so the parser dropped every + // printing of every set. Reparse now that the sets are enabled, otherwise the + // deck editor's card database display stays empty until the next launch. + auto *watcher = new QFutureWatcher(this); + connect(watcher, &QFutureWatcher::finished, this, [this, watcher] { + watcher->deleteLater(); + if (firstRunWizardActive) { + // The onboarding wizard owns the first-run messaging on a clean install. + return; + } + QMessageBox::information( + this, tr("Welcome"), + tr("Hi! It seems like you're running this version of Cockatrice for the first time.\nAll the sets in the " + "card " + "database have been enabled.\nRead more about changing the set order or disabling specific sets and " + "consequent effects in the \"Manage Sets\" dialog.")); + // The reload above replaces the set instances, so Manage Sets must open + // only after the finished snapshot has been swapped in. + actManageSets(); + }); + watcher->setFuture(QtConcurrent::run([cardDatabase] { cardDatabase->reloadCardDatabasesAndNotify(); })); } /* CARD UPDATER */ @@ -1321,7 +1339,19 @@ void MainWindow::exitCardDatabaseUpdate() cardUpdateProcess = nullptr; statusBar()->clearMessage(); - const auto reloadOk1 = QtConcurrent::run([] { CardDatabaseManager::getInstance()->loadCardDatabases(); }); + // This can be the first successful load of the session (e.g. the onboarding + // download), so it must own the set check for that load. + reloadCardDatabaseAndCheckSets(); +} + +void MainWindow::reloadCardDatabaseAndCheckSets() +{ + auto *watcher = new QFutureWatcher(this); + connect(watcher, &QFutureWatcher::finished, this, [watcher] { + watcher->deleteLater(); + CardDatabaseManager::getInstance()->checkUnknownSets(); + }); + watcher->setFuture(QtConcurrent::run([] { CardDatabaseManager::getInstance()->loadCardDatabases(); })); } void MainWindow::cardUpdateError(QProcess::ProcessError err) @@ -1523,7 +1553,8 @@ void MainWindow::actAddCustomSet() QMessageBox::information( this, tr("Load sets/cards"), tr("The new sets/cards have been added successfully.\nCockatrice will now reload the card database.")); - const auto reloadOk1 = QtConcurrent::run([] { CardDatabaseManager::getInstance()->loadCardDatabases(); }); + // Imported files can introduce sets the config has never seen. + reloadCardDatabaseAndCheckSets(); } else { QMessageBox::warning(this, tr("Load sets/cards"), tr("Sets/cards failed to import.")); } diff --git a/cockatrice/src/interface/window_main.h b/cockatrice/src/interface/window_main.h index 6b1186d3c..ee8873000 100644 --- a/cockatrice/src/interface/window_main.h +++ b/cockatrice/src/interface/window_main.h @@ -158,6 +158,14 @@ private: } void createCardUpdateProcess(bool background = false); void exitCardDatabaseUpdate(); + /** + * @brief Reloads the card database on a worker thread and runs checkUnknownSets() once the finished + * snapshot has been swapped in. + * + * The loader never runs checkUnknownSets() itself (see main.cpp); post-update loads that can introduce + * new sets must own the check, and it has to run after the queued snapshot swap, not on the worker. + */ + void reloadCardDatabaseAndCheckSets(); /** * @brief Tries to bring the open card database updater window to the front. * @return true when the updater acknowledged the request diff --git a/cockatrice/src/main.cpp b/cockatrice/src/main.cpp index b9cc8dabc..3f63efc3b 100644 --- a/cockatrice/src/main.cpp +++ b/cockatrice/src/main.cpp @@ -343,11 +343,12 @@ int main(int argc, char *argv[]) // contention that happens when the load runs alongside window construction. // The CardDatabaseModel populates from the already-loaded data in its // constructor, so the window appears fully populated with no startup lag. - // Note: checkUnknownSets() is deferred from the initial load to - // MainWindow::startupConfigCheck() so that - // cardDatabaseNewSetsFound / cardDatabaseAllNewSetsEnabled have live - // receivers when emitted. Subsequent reloads (e.g. path changes) call - // checkUnknownSets() directly from the loader after the first load. + // Note: checkUnknownSets() is deliberately not run for this front-loaded + // parse. It would run before MainWindow exists, so + // cardDatabaseNewSetsFound / cardDatabaseAllNewSetsEnabled would have no + // receivers, and enabling the sets up-front would suppress the first-run + // experience MainWindow::startupConfigCheck() and the onboarding wizard + // provide. Callers that load after startup own the check. CardDatabaseManager::getInstance()->loadCardDatabases(); MainWindow ui; diff --git a/doc/doxygen/pages/user_documentation/deck_management/importing_decks.md b/doc/doxygen/pages/user_documentation/deck_management/importing_decks.md index e9626d57f..4aab7ad2c 100644 --- a/doc/doxygen/pages/user_documentation/deck_management/importing_decks.md +++ b/doc/doxygen/pages/user_documentation/deck_management/importing_decks.md @@ -38,7 +38,18 @@ Selecting this action will open a new text editor dialog with the contents of yo The import dialog expects each line to be a card with the following format: -TODO +`[quantity] Card Name [options]` + +- `quantity` is the number of copies and can be written as `2`, `2x` or `2X`. If omitted, a single copy is imported. +- `options` are appended after the card name and are understood on a best-effort basis: + - a set code and collector number, either as `Card Name (SET) 123` or `Card Name (SET) 123-1` + - a foil marker, `Card Name *F*` +- Lines can also carry a `SB:` prefix or a set code prefix (`[SET]`) — Cockatrice tries to stay compatible with the most + common deck formats. + +Tab-separated exports from third-party apps (such as Delver Lens) are also accepted: any line whose first column is a +plain quantity treats the following columns as the card name and printer/set metadata (which is discarded), for example +`2\tCard Name\tSet Name`. Each card should be on a separate line and there should be no empty lines between cards. The first empty line between two blocks of cards will be considered as the divider between mainboard and sideboard. diff --git a/libcockatrice_card/libcockatrice/card/database/card_database.cpp b/libcockatrice_card/libcockatrice/card/database/card_database.cpp index 469c8d02c..7b0d5b2f9 100644 --- a/libcockatrice_card/libcockatrice/card/database/card_database.cpp +++ b/libcockatrice_card/libcockatrice/card/database/card_database.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -80,15 +81,17 @@ void CardDatabase::reloadCardDatabasesAndNotify() { // An explicit reload bypasses the binary cache so the files on disk are // always re-read, even when their size/mtime hash still matches the cache. + // Models refresh from the swap's cardDatabaseReset in a single batch, so + // no cardDatabaseEnabledSetsChanged is emitted here: that rebuilt the + // 36k-card models a second time on the GUI thread, which froze startup + // after enabling new/custom sets. The follow-up unknown-sets check is + // queued to the GUI thread so it sees the swapped-in data (this may run + // on a worker via QtConcurrent::run); it is silent unless new sets + // appeared mid-reload. loadStatus = loader->loadCardDatabases(/* ignoreCache */ true); if (loadStatus == Ok) { - checkUnknownSets(); - // A reload reconstructs the exact same enabled-set state, so the cached - // set-name / alt-name data computed during construction is still valid. - // Skipping the per-card refresh avoids a full 36k-card recompute that - // would otherwise duplicate the work already done while building cards. - notifyEnabledSetsChanged(false); + QMetaObject::invokeMethod(this, &CardDatabase::checkUnknownSets, Qt::AutoConnection); } } @@ -207,15 +210,19 @@ void CardDatabase::checkUnknownSets() if (numUnknownSets > 0) { emit cardDatabaseNewSetsFound(numUnknownSets, unknownSetNames); } else { - _sets.markAllAsKnown(); + _sets.markAllAsKnownBatched(setPriorityController); } } else { // No set enabled. Probably this is the first time running trice - _sets.guessSortKeys(); + _sets.enableAllBatched(setPriorityController); _sets.sortByKey(); - _sets.enableAll(); - notifyEnabledSetsChanged(); + // No cardDatabaseEnabledSetsChanged here: the reload chained off + // cardDatabaseAllNewSetsEnabled re-parses with the sets enabled and its + // swap's cardDatabaseReset rebuilds the models in one batch. Emitting + // both back-to-back rebuilt the 36k-card models twice on the GUI thread, + // stalling startup. (When the card list is empty the slot returns early + // without reloading, but there is nothing to display either.) emit cardDatabaseAllNewSetsEnabled(); } } @@ -223,13 +230,13 @@ void CardDatabase::checkUnknownSets() void CardDatabase::enableAllUnknownSets() { auto _sets = getSetList(); - _sets.enableAllUnknown(); + _sets.enableAllUnknownBatched(setPriorityController); } void CardDatabase::markAllSetsAsKnown() { auto _sets = getSetList(); - _sets.markAllAsKnown(); + _sets.markAllAsKnownBatched(setPriorityController); } void CardDatabase::notifyEnabledSetsChanged(bool recomputeCachedSets) diff --git a/libcockatrice_card/libcockatrice/card/database/card_database_loader.cpp b/libcockatrice_card/libcockatrice/card/database/card_database_loader.cpp index 194083615..00b21f88e 100644 --- a/libcockatrice_card/libcockatrice/card/database/card_database_loader.cpp +++ b/libcockatrice_card/libcockatrice/card/database/card_database_loader.cpp @@ -44,7 +44,13 @@ CardDatabaseLoader::CardDatabaseLoader(QObject *parent, // the finished snapshot is swapped into the live database on the GUI thread. // when SettingsCache's path changes, trigger reloads - connect(pathProvider, &ICardDatabasePathProvider::cardDatabasePathChanged, this, [this] { loadCardDatabases(); }); + connect(pathProvider, &ICardDatabasePathProvider::cardDatabasePathChanged, this, [this] { + loadCardDatabases(); + // This reload runs synchronously on the emitting (GUI) thread, so the + // finished snapshot is already swapped in when it returns: enable any + // sets the newly pointed-at database introduced. + database->checkUnknownSets(); + }); } CardDatabaseLoader::~CardDatabaseLoader() @@ -136,7 +142,23 @@ LoadStatus CardDatabaseLoader::doLoadCardDatabases(bool ignoreCache) loadCardDatabase(p, data); } - if (!saveToCache(data, sourceHash)) { + // A snapshot parsed while no set is enabled has had every printing + // stripped by the parsers, and the cache reader re-derives enablement + // from the priority controller, so such an entry can never be reused + // meaningfully. Skip writing it so a first-run (blank config) load + // cannot poison the cache with an empty snapshot. The check mirrors + // what the parsers saw: any set still reporting enabled means the + // snapshot is complete for the current enablement. + bool anySetEnabled = false; + for (const CardSetPtr &set : data.sets) { + if (set && set->getEnabled()) { + anySetEnabled = true; + break; + } + } + if (!data.sets.isEmpty() && !anySetEnabled) { + qCInfo(CardDatabaseLoadingLog) << "Skipping binary cache: no set is enabled yet, snapshot was stripped"; + } else if (!saveToCache(data, sourceHash)) { qCWarning(CardDatabaseLoadingLog) << "Failed to write binary cache to" << cachePath(); } } @@ -148,18 +170,15 @@ LoadStatus CardDatabaseLoader::doLoadCardDatabases(bool ignoreCache) database->refreshCachedReverseRelatedCards(data.cards); qCInfo(CardDatabaseLoadingSuccessOrFailureLog) << "Card Database Loading Success"; + // NOTE: checkUnknownSets() is intentionally NOT called here. During the + // front-loaded parse in main() MainWindow does not exist yet, so + // cardDatabaseNewSetsFound / cardDatabaseAllNewSetsEnabled would be + // emitted with no receivers, and enabling the sets up-front would + // suppress the first-run experience MainWindow::startupConfigCheck() + // and the onboarding wizard are designed around. Callers that load + // after startup own the check (and any reload it triggers). emit databaseDataReady(std::move(data)); emit loadingFinished(); - // During the front-loaded parse in main() this runs before MainWindow - // exists, so cardDatabaseNewSetsFound / cardDatabaseAllNewSetsEnabled - // would be emitted with no receivers. Skip the check on the first load; - // MainWindow::startupConfigCheck() calls checkUnknownSets() once its - // signal connections are live. On subsequent reloads (e.g. path change) - // MainWindow exists and signals have live receivers. - if (initialLoadComplete) { - database->checkUnknownSets(); - } - initialLoadComplete = true; } else { qCInfo(CardDatabaseLoadingSuccessOrFailureLog) << "Card Database Loading Failed"; emit loadingFailed(); // bring up the settings dialog diff --git a/libcockatrice_card/libcockatrice/card/database/card_database_loader.h b/libcockatrice_card/libcockatrice/card/database/card_database_loader.h index fffd31d2f..bfbaa0f51 100644 --- a/libcockatrice_card/libcockatrice/card/database/card_database_loader.h +++ b/libcockatrice_card/libcockatrice/card/database/card_database_loader.h @@ -179,7 +179,6 @@ private: QBasicMutex *loadFromFileMutex = new QBasicMutex(); /**< Mutex for single-file loading. */ QBasicMutex *reloadDatabaseMutex = new QBasicMutex(); /**< Mutex for reloading entire database. */ - bool initialLoadComplete = false; /**< Set after the first successful load. */ }; #endif // COCKATRICE_CARD_DATABASE_LOADER_H diff --git a/libcockatrice_card/libcockatrice/card/set/card_set.h b/libcockatrice_card/libcockatrice/card/set/card_set.h index 538d78852..b9ac74e2d 100644 --- a/libcockatrice_card/libcockatrice/card/set/card_set.h +++ b/libcockatrice_card/libcockatrice/card/set/card_set.h @@ -227,6 +227,10 @@ public: * @param _isknown True if known, false if unknown. */ void setIsKnown(bool _isknown); + void setIsKnownInMemory(bool _isknown) + { + isknown = _isknown; + } /** * @brief Determines whether the set has incomplete metadata and should be ignored. diff --git a/libcockatrice_card/libcockatrice/card/set/card_set_list.cpp b/libcockatrice_card/libcockatrice/card/set/card_set_list.cpp index 2e030adab..c4faf1c4c 100644 --- a/libcockatrice_card/libcockatrice/card/set/card_set_list.cpp +++ b/libcockatrice_card/libcockatrice/card/set/card_set_list.cpp @@ -1,9 +1,11 @@ #include "card_set_list.h" #include "libcockatrice/card/set/card_set.h" +#include "libcockatrice/interfaces/interface_card_set_priority_controller.h" #include #include +#include #include #include @@ -62,60 +64,87 @@ QStringList CardSetList::getUnknownSetsNames() return sets; } -void CardSetList::enableAllUnknown() +void CardSetList::enableAllUnknownBatched(ICardSetPriorityController *controller) { - for (int i = 0; i < size(); ++i) { - CardSetPtr set = at(i); - if (set && !set->getIsKnown() && !set->getIsKnownIgnored()) { - set->setIsKnown(true); - set->setEnabled(true); - } else if (set && set->getIsKnownIgnored() && !set->getEnabled()) { - set->setEnabled(true); - } - } -} - -void CardSetList::enableAll() -{ - for (int i = 0; i < size(); ++i) { - CardSetPtr set = at(i); - - if (set == nullptr) { - // qCWarning(CardInfoLog) << "enabledAll has null"; - continue; - } - - if (!set->getIsKnownIgnored()) { - set->setIsKnown(true); - } - - set->setEnabled(true); - } -} - -void CardSetList::markAllAsKnown() -{ - for (int i = 0; i < size(); ++i) { - CardSetPtr set = at(i); - if (set && !set->getIsKnown() && !set->getIsKnownIgnored()) { - set->setIsKnown(true); - set->setEnabled(false); - } else if (set && set->getIsKnownIgnored() && !set->getEnabled()) { - set->setEnabled(true); - } - } -} - -void CardSetList::guessSortKeys() -{ - defaultSort(); + QVector saveData; for (int i = 0; i < size(); ++i) { CardSetPtr set = at(i); if (set.isNull()) { - // qCWarning(CardInfoLog) << "guessSortKeys set is null"; continue; } - set->setSortKey(i); + + bool changed = false; + if (!set->getIsKnown() && !set->getIsKnownIgnored()) { + set->setIsKnownInMemory(true); + set->setEnabledInMemory(true); + changed = true; + } else if (set->getIsKnownIgnored() && !set->getEnabled()) { + set->setEnabledInMemory(true); + changed = true; + } + if (changed) { + saveData.append( + {set->getShortName(), static_cast(set->getSortKey()), true, set->getIsKnown()}); + } + } + + if (!saveData.isEmpty()) { + controller->saveSets(saveData); + } +} + +void CardSetList::enableAllBatched(ICardSetPriorityController *controller) +{ + defaultSort(); + + QVector saveData; + saveData.reserve(size()); + for (int i = 0; i < size(); ++i) { + CardSetPtr set = at(i); + if (set.isNull()) { + continue; + } + + const auto sortKey = static_cast(i); + set->setSortKeyInMemory(sortKey); + if (!set->getIsKnownIgnored()) { + set->setIsKnownInMemory(true); + } + set->setEnabledInMemory(true); + saveData.append({set->getShortName(), sortKey, true, set->getIsKnown()}); + } + + if (!saveData.isEmpty()) { + controller->saveSets(saveData); + } +} + +void CardSetList::markAllAsKnownBatched(ICardSetPriorityController *controller) +{ + QVector saveData; + for (int i = 0; i < size(); ++i) { + CardSetPtr set = at(i); + if (set.isNull()) { + continue; + } + + bool changed = false; + if (!set->getIsKnown() && !set->getIsKnownIgnored()) { + set->setIsKnownInMemory(true); + set->setEnabledInMemory(false); + changed = true; + } else if (set->getIsKnownIgnored() && !set->getEnabled()) { + set->setEnabledInMemory(true); + changed = true; + } + if (changed) { + saveData.append({set->getShortName(), static_cast(set->getSortKey()), set->getEnabled(), + set->getIsKnown()}); + } + } + + if (!saveData.isEmpty()) { + controller->saveSets(saveData); } } diff --git a/libcockatrice_card/libcockatrice/card/set/card_set_list.h b/libcockatrice_card/libcockatrice/card/set/card_set_list.h index b9c7b2a7a..f31710b33 100644 --- a/libcockatrice_card/libcockatrice/card/set/card_set_list.h +++ b/libcockatrice_card/libcockatrice/card/set/card_set_list.h @@ -40,36 +40,37 @@ public: void sortByKey(); /** - * @brief Reassigns sort keys based on the current order. + * @brief Enables every set with fresh sort keys, persisting in one flush. * - * Calls defaultSort() and then assigns sequential sort keys - * to all sets according to their resulting positions, replacing - * any existing sort keys to ensure consistent ordering. + * Assigns sequential sort keys in default-sorted order and marks every set + * known and enabled, writing all records through the controller's batched + * saveSets() in a single settings flush instead of ~3 QSettings rewrite + * cycles per set, which froze the GUI thread for seconds on a first run + * with a full database. + * @param controller The set priority controller to persist the records with. */ - void guessSortKeys(); + void enableAllBatched(ICardSetPriorityController *controller); /** - * @brief Enables all sets that are unknown or ignored. + * @brief Enables all sets that are unknown or ignored, persisting in one flush. * * Sets that are not marked as known and not ignored are marked as known * and enabled. Ignored-known sets are also enabled, but remain ignored. + * Only changed sets are written, so the common case of a few new sets costs + * a single settings flush no matter how large the database is. + * @param controller The set priority controller to persist the records with. */ - void enableAllUnknown(); + void enableAllUnknownBatched(ICardSetPriorityController *controller); /** - * @brief Enables all sets in the list. - * - * Equivalent to calling `setEnabled(true)` on each entry. - */ - void enableAll(); - - /** - * @brief Marks all sets as known and adjusts their enabled state. + * @brief Marks all sets as known and adjusts their enabled state, persisting in one flush. * * Unknown, non-ignored sets become known and disabled. * Ignored-known sets are enabled if they were previously disabled. + * Only changed sets are written; when nothing is unknown no flush happens at all. + * @param controller The set priority controller to persist the records with. */ - void markAllAsKnown(); + void markAllAsKnownBatched(ICardSetPriorityController *controller); /** * @brief Counts the number of sets that are currently enabled. diff --git a/libcockatrice_deck_list/libcockatrice/deck_list/deck_list_plain_text_parser.cpp b/libcockatrice_deck_list/libcockatrice/deck_list/deck_list_plain_text_parser.cpp index 61af36540..fb6a61769 100644 --- a/libcockatrice_deck_list/libcockatrice/deck_list/deck_list_plain_text_parser.cpp +++ b/libcockatrice_deck_list/libcockatrice/deck_list/deck_list_plain_text_parser.cpp @@ -33,6 +33,7 @@ bool parse(QTextStream &in, // Regex for advanced card parsing static const QRegularExpression reMultiplier(R"(^[xX\(\[]*(\d+)[xX\*\)\]]* ?(.+))"); + static const QRegularExpression reTabQuantity(R"(^\d+[xX]?$)"); // Regex for extracting set code and collector number with attached symbols static const QRegularExpression reHyphenFormat(R"(\((\w{3,})\)\s+(\w{3,})-(\d+[^\w\s]*))"); @@ -110,8 +111,34 @@ bool parse(QTextStream &in, continue; } - QString cardName = match.captured().simplified(); + const QString rawLine = match.captured(); + QString cardName = rawLine.simplified(); bool sideboard = false; + int amount = 1; + + // Card names never contain tab characters, so a leading numeric field + // marks a tabular record emitted by third-party deck tools (e.g. Delver + // Lens "2\tCard Name\tSet Name"). The first two columns are the quantity + // and the card name; any further columns are printer/set metadata and + // are discarded. + bool hasTabQuantity = false; + const auto tabFields = rawLine.split('\t'); + int firstField = 0; + while (firstField < tabFields.size() && tabFields.at(firstField).trimmed().isEmpty()) { + ++firstField; + } + if (firstField + 1 < tabFields.size()) { + const QString quantity = tabFields.at(firstField).trimmed(); + if (reTabQuantity.match(quantity).hasMatch()) { + hasTabQuantity = true; + QString digits = quantity; + if (digits.endsWith('x') || digits.endsWith('X')) { + digits.chop(1); + } + amount = digits.toInt(); + cardName = tabFields.at(firstField + 1).simplified(); + } + } // Sideboard detection if (sBStart < 0) { @@ -156,11 +183,12 @@ bool parse(QTextStream &in, } // check if a specific amount is mentioned - int amount = 1; - match = reMultiplier.match(cardName); - if (match.hasMatch()) { - amount = match.captured(1).toInt(); - cardName = match.captured(2); + if (!hasTabQuantity) { + match = reMultiplier.match(cardName); + if (match.hasMatch()) { + amount = match.captured(1).toInt(); + cardName = match.captured(2); + } } // Normalize the card name diff --git a/libcockatrice_interfaces/libcockatrice/interfaces/interface_card_set_priority_controller.h b/libcockatrice_interfaces/libcockatrice/interfaces/interface_card_set_priority_controller.h index 6b3e684d7..746ac9490 100644 --- a/libcockatrice_interfaces/libcockatrice/interfaces/interface_card_set_priority_controller.h +++ b/libcockatrice_interfaces/libcockatrice/interfaces/interface_card_set_priority_controller.h @@ -12,6 +12,7 @@ public: QString shortName; unsigned int sortKey; bool enabled; + bool isKnown = false; }; /** @@ -57,6 +58,14 @@ public: */ virtual QStringList getEnabledSetNames() const = 0; + /** + * @brief Persists per-set options for every entry in a single settings flush. + * + * Each individual setSortKey/setEnabled/setIsKnown call re-opens and re-syncs + * the whole .ini when backed by QSettings, so persisting hundreds of sets + * one option at a time freezes the GUI thread for seconds. Callers that touch + * more than a handful of sets must bundle the records and call this once. + */ virtual void saveSets(const QVector &data) = 0; }; diff --git a/libcockatrice_models/libcockatrice/models/database/card_set/card_sets_model.cpp b/libcockatrice_models/libcockatrice/models/database/card_set/card_sets_model.cpp index 1bc1b9e38..a0b9c1061 100644 --- a/libcockatrice_models/libcockatrice/models/database/card_set/card_sets_model.cpp +++ b/libcockatrice_models/libcockatrice/models/database/card_set/card_sets_model.cpp @@ -266,7 +266,7 @@ void SetsModel::save(CardDatabase *db) const bool enabled = enabledSets.contains(sets[i]); sets[i]->setSortKeyInMemory(sortKey); sets[i]->setEnabledInMemory(enabled); - saveData.append({sets[i]->getShortName(), sortKey, enabled}); + saveData.append({sets[i]->getShortName(), sortKey, enabled, sets[i]->getIsKnown()}); } db->getPriorityController()->saveSets(saveData); diff --git a/libcockatrice_network/libcockatrice/network/server/remote/CMakeLists.txt b/libcockatrice_network/libcockatrice/network/server/remote/CMakeLists.txt index 5840f80c0..2127487d2 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/CMakeLists.txt +++ b/libcockatrice_network/libcockatrice/network/server/remote/CMakeLists.txt @@ -3,6 +3,7 @@ set(CMAKE_AUTOUIC ON) set(CMAKE_AUTORCC ON) set(HEADERS + game/dump_zone_range.h game/server_abstract_participant.h game/server_abstract_player.h game/server_arrow.h @@ -33,6 +34,7 @@ qt6_wrap_cpp(MOC_SOURCES ${HEADERS}) add_library( libcockatrice_network_server_remote STATIC ${MOC_SOURCES} + game/dump_zone_range.cpp game/server_abstract_participant.cpp game/server_abstract_player.cpp game/server_arrow.cpp diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/dump_zone_range.cpp b/libcockatrice_network/libcockatrice/network/server/remote/game/dump_zone_range.cpp new file mode 100644 index 000000000..4ecca95cb --- /dev/null +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/dump_zone_range.cpp @@ -0,0 +1,17 @@ +#include "dump_zone_range.h" + +#include + +std::pair dumpZoneCardRange(qsizetype cardCount, int numberCards, bool isReversed) +{ + qsizetype count = 0; + if (numberCards == -1) { + count = cardCount; + } else if (numberCards > 0) { + count = std::min(numberCards, cardCount); + } + count = std::max(count, 0); + + const qsizetype begin = isReversed ? cardCount - count : 0; + return {begin, begin + count}; +} diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/dump_zone_range.h b/libcockatrice_network/libcockatrice/network/server/remote/game/dump_zone_range.h new file mode 100644 index 000000000..a688b09d0 --- /dev/null +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/dump_zone_range.h @@ -0,0 +1,29 @@ +#ifndef DUMP_ZONE_RANGE_H +#define DUMP_ZONE_RANGE_H + +#include +#include + +/** + * @brief Resolve a client's zone dump request against a zone's real card count. + * + * @c number_cards arrives from the client unvalidated. -1 means "the whole zone", which + * is the convention the client uses when sizing its own view of one. Every other negative + * value, and every value larger than the zone, has to be brought into range rather than + * used as an index: with a reversed request the zone offset is + * @c cardCount - @c numberCards, which runs off the front of the list when the request + * exceeds the zone and off the back when @c numberCards is -1. + * + * Kept in its own translation unit so it can be exercised without the rest of + * server_abstract_player.cpp, which refers to the RNG global that only the servatrice + * binary defines. + * + * @param cardCount number of cards the zone actually holds + * @param numberCards the requested count, -1 for the whole zone + * @param isReversed take the trailing cards instead of the leading ones + * @return the half-open index range [begin, end) into the zone's card list, always + * contained in [0, cardCount) + */ +std::pair dumpZoneCardRange(qsizetype cardCount, int numberCards, bool isReversed); + +#endif // DUMP_ZONE_RANGE_H diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.cpp b/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.cpp index 47c810d0d..c03f1e966 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.cpp @@ -1,5 +1,6 @@ #include "server_abstract_player.h" +#include "dump_zone_range.h" #include "game/server_abstract_participant.h" #include "game/server_arrowtarget.h" #include "libcockatrice/protocol/pb/card_attributes.pb.h" @@ -1435,23 +1436,23 @@ Server_AbstractPlayer::cmdDumpZone(const Command_DumpZone &cmd, ResponseContaine int numberCards = cmd.number_cards(); const QList &cards = zone->getCards(); + const auto cardRange = dumpZoneCardRange(cards.size(), numberCards, cmd.is_reversed()); auto *re = new Response_DumpZone; ServerInfo_Zone *zoneInfo = re->mutable_zone_info(); zoneInfo->set_name(zone->getName().toStdString()); zoneInfo->set_type(zone->getType()); zoneInfo->set_with_coords(zone->hasCoords()); - zoneInfo->set_card_count(numberCards < cards.size() ? cards.size() : numberCards); + zoneInfo->set_card_count(cards.size()); - for (int i = 0; (i < cards.size()) && (i < numberCards || numberCards == -1); ++i) { - const auto &findId = cmd.is_reversed() ? cards.size() - numberCards + i : i; - Server_Card *card = cards[findId]; + for (qsizetype findId = cardRange.first; findId < cardRange.second; ++findId) { + Server_Card *card = cards.at(findId); QString displayedName = card->getFaceDown() ? QString() : card->getName(); ServerInfo_Card *cardInfo = zoneInfo->add_card_list(); cardInfo->set_provider_id(card->getProviderId().toStdString()); cardInfo->set_name(displayedName.toStdString()); if (zone->getType() == ServerInfo_Zone::HiddenZone) { - cardInfo->set_id(findId); + cardInfo->set_id(static_cast(findId)); } else { cardInfo->set_id(card->getId()); cardInfo->set_x(card->getX()); diff --git a/libcockatrice_settings/libcockatrice/settings/card_database_settings.cpp b/libcockatrice_settings/libcockatrice/settings/card_database_settings.cpp index acac1125f..49bcb042b 100644 --- a/libcockatrice_settings/libcockatrice/settings/card_database_settings.cpp +++ b/libcockatrice_settings/libcockatrice/settings/card_database_settings.cpp @@ -110,6 +110,7 @@ void CardDatabaseSettings::saveSets(const QVector &renamedGameFilterKeys() +{ + static const QMap keyMap = { + {"hide_buddies_only_games", "hideBuddiesOnlyGames"}, + {"hide_full_games", "hideFullGames"}, + {"hide_games_that_started", "hideGamesThatStarted"}, + {"hide_password_protected_games", "hidePasswordProtectedGames"}, + {"hide_ignored_user_games", "hideIgnoredUserGames"}, + {"hide_not_buddy_created_games", "hideNotBuddyCreatedGames"}, + {"hide_open_decklist_games", "hideOpenDecklistGames"}, + {"game_name_filter", "gameNameFilter"}, + {"creator_name_filter", "hostNameFilter"}, + {"min_players", "minPlayers"}, + {"max_players", "maxPlayers"}, + {"max_game_age_time", "maxGameAgeTime"}, + {"show_only_if_spectators_can_watch", "showOnlyIfSpectatorsCanWatch"}, + {"show_spectator_password_protected", "showSpectatorPasswordProtected"}, + {"show_only_if_spectators_can_chat", "showOnlyIfSpectatorsCanChat"}, + {"show_only_if_spectators_can_see_hands", "showOnlyIfSpectatorsCanSeeHands"}, + }; + return keyMap; +} + static void migrateLegacyGameFilters(const QString &settingsPath) { QSettings legacySetting; @@ -652,24 +680,7 @@ static void migrateLegacyGameFilters(const QString &settingsPath) return; } - const QMap filterKeyMap = { - {"hide_buddies_only_games", "hideBuddiesOnlyGames"}, - {"hide_full_games", "hideFullGames"}, - {"hide_games_that_started", "hideGamesThatStarted"}, - {"hide_password_protected_games", "hidePasswordProtectedGames"}, - {"hide_ignored_user_games", "hideIgnoredUserGames"}, - {"hide_not_buddy_created_games", "hideNotBuddyCreatedGames"}, - {"hide_open_decklist_games", "hideOpenDecklistGames"}, - {"game_name_filter", "gameNameFilter"}, - {"creator_name_filter", "creatorNameFilter"}, - {"min_players", "minPlayers"}, - {"max_players", "maxPlayers"}, - {"max_game_age_time", "maxGameAgeTime"}, - {"show_only_if_spectators_can_watch", "showOnlyIfSpectatorsCanWatch"}, - {"show_spectator_password_protected", "showSpectatorPasswordProtected"}, - {"show_only_if_spectators_can_chat", "showOnlyIfSpectatorsCanChat"}, - {"show_only_if_spectators_can_see_hands", "showOnlyIfSpectatorsCanSeeHands"}, - }; + const QMap &filterKeyMap = renamedGameFilterKeys(); QSettings filtersIni(settingsPath + "gamefilters.ini", QSettings::IniFormat); for (auto it = filterKeyMap.constBegin(); it != filterKeyMap.constEnd(); ++it) { @@ -686,6 +697,68 @@ static void migrateLegacyGameFilters(const QString &settingsPath) legacySetting.endGroup(); } +static const QString LEGACY_GAME_FILTERS_GAME_TYPE_PREFIX = QStringLiteral("filter_games/game_type/"); +static const QString GAME_FILTERS_GAME_TYPE_PREFIX = QStringLiteral("filter_games/gameType/"); + +static bool gameFiltersCamelCaseMigrationPending(const QString &settingsPath) +{ + if (!QFile::exists(settingsPath + "gamefilters.ini")) { + return false; + } + + QSettings filtersIni(settingsPath + "gamefilters.ini", QSettings::IniFormat); + const QMap &keyMap = renamedGameFilterKeys(); + for (auto it = keyMap.constBegin(); it != keyMap.constEnd(); ++it) { + if (filtersIni.contains("filter_games/" + it.key()) && !filtersIni.contains("filter_games/" + it.value())) { + return true; + } + } + // Orphaned by #7084, which migrated the legacy host-name filter to a key nothing reads. + if (filtersIni.contains("filter_games/creatorNameFilter") && !filtersIni.contains("filter_games/hostNameFilter")) { + return true; + } + for (const auto &key : filtersIni.allKeys()) { + if (key.startsWith(LEGACY_GAME_FILTERS_GAME_TYPE_PREFIX) && + !filtersIni.contains(GAME_FILTERS_GAME_TYPE_PREFIX + + key.mid(LEGACY_GAME_FILTERS_GAME_TYPE_PREFIX.size()))) { + return true; + } + } + return false; +} + +bool SettingsMigration::migrateGameFiltersToCamelCase(const QString &settingsPath) +{ + if (!gameFiltersCamelCaseMigrationPending(settingsPath)) { + return false; + } + + QSettings filtersIni(settingsPath + "gamefilters.ini", QSettings::IniFormat); + const QMap &keyMap = renamedGameFilterKeys(); + for (auto it = keyMap.constBegin(); it != keyMap.constEnd(); ++it) { + const QString oldKey = "filter_games/" + it.key(); + const QString newKey = "filter_games/" + it.value(); + if (filtersIni.contains(oldKey) && !filtersIni.contains(newKey)) { + filtersIni.setValue(newKey, filtersIni.value(oldKey)); + } + } + if (filtersIni.contains("filter_games/creatorNameFilter") && !filtersIni.contains("filter_games/hostNameFilter")) { + filtersIni.setValue("filter_games/hostNameFilter", filtersIni.value("filter_games/creatorNameFilter")); + } + for (const auto &key : filtersIni.allKeys()) { + if (key.startsWith(LEGACY_GAME_FILTERS_GAME_TYPE_PREFIX)) { + const QString newKey = GAME_FILTERS_GAME_TYPE_PREFIX + key.mid(LEGACY_GAME_FILTERS_GAME_TYPE_PREFIX.size()); + if (!filtersIni.contains(newKey)) { + filtersIni.setValue(newKey, filtersIni.value(key)); + } + } + } + // The old keys are deliberately kept: pre-#7084 builds still read them, so removing them + // would break downgrades, and nothing in the current code reads them anymore. + filtersIni.sync(); + return true; +} + bool SettingsMigration::migrateLegacySettings(const QString &settingsPath) { QSettings personalIni(settingsPath + "personal.ini", QSettings::IniFormat); @@ -744,6 +817,10 @@ bool SettingsMigration::isMigrationPending(const QString &settingsPath) return true; } + if (gameFiltersCamelCaseMigrationPending(settingsPath)) { + return true; + } + QSettings personalIni(settingsPath + "personal.ini", QSettings::IniFormat); if (personalIni.value("migration/legacy_complete", false).toBool()) { return false; diff --git a/libcockatrice_settings/libcockatrice/settings/settings_migration.h b/libcockatrice_settings/libcockatrice/settings/settings_migration.h index 78f9c8a3f..0a706af98 100644 --- a/libcockatrice_settings/libcockatrice/settings/settings_migration.h +++ b/libcockatrice_settings/libcockatrice/settings/settings_migration.h @@ -9,6 +9,15 @@ public: static bool migrateSettingsFromGlobalIni(const QString &settingsPath); static bool migrateLegacySettings(const QString &settingsPath); + /** + * Moves pre-camelCase gamefilters.ini keys (snake_case, game_type/) to the camelCase + * keys used since #7084, for profiles that already migrated before that rename and therefore + * never pass through migrateLegacySettings again. Copies values only where the new key is + * absent, never deletes the old keys, and returns true when anything was copied. Idempotent, + * so it runs on every startup. + */ + static bool migrateGameFiltersToCamelCase(const QString &settingsPath); + /** * Returns true when at least one of the two migrations would still do work, i.e. when a * backup of the settings directory is worth taking before they run. diff --git a/servatrice/servatrice.ini.example b/servatrice/servatrice.ini.example index c625705da..3ee52dd21 100644 --- a/servatrice/servatrice.ini.example +++ b/servatrice/servatrice.ini.example @@ -250,6 +250,16 @@ email=root@localhost ; Sender email name name="Cockatrice server" +; Maximum number of attempts to deliver a given e-mail before giving up. +; If the smtp server is unreachable or repeatedly rejects a message, the server +; will retry with an exponentially growing delay up to this many times, then drop +; the message from the queue and log the failure. Default: 5 +maxretries=5 + +; Initial retry delay in seconds between delivery attempts. Doubles with each +; failed attempt (capped at 30 minutes). Default: 60 +retrydelay=60 + ; Email subject subject="Cockatrice server account activation token" diff --git a/servatrice/src/servatrice.cpp b/servatrice/src/servatrice.cpp index 0329a31fd..9ec9ef5bc 100644 --- a/servatrice/src/servatrice.cpp +++ b/servatrice/src/servatrice.cpp @@ -38,6 +38,7 @@ #include #include #include +#include #include #include #include @@ -69,6 +70,8 @@ #include #include +inline Q_LOGGING_CATEGORY(ServatriceLog, "servatrice"); + Servatrice_GameServer::Servatrice_GameServer(Servatrice *_server, int _numberPools, const QSqlDatabase &_sqlDatabase, @@ -335,6 +338,11 @@ bool Servatrice::initServer() servatriceDatabaseInterface->clearSessionTables(); } + // Connect the SMTP client's delivery-confirmation signals to the database cleanup handlers. + // Emails are removed from the queue tables only after delivery is confirmed (see statusUpdate()). + connect(smtpClient, &SmtpClient::mailDelivered, this, &Servatrice::onEmailDelivered); + connect(smtpClient, &SmtpClient::mailPermanentlyFailed, this, &Servatrice::onEmailPermanentlyFailed); + if (getRoomsMethodString() == "sql") { QSqlQuery *query = servatriceDatabaseInterface->prepareQuery( "select id, name, descr, permissionlevel, privlevel, auto_join, join_message, chat_history_size from " @@ -737,41 +745,36 @@ void Servatrice::statusUpdate() return; } - auto *queryDelete = - servatriceDatabaseInterface->prepareQuery("delete from {prefix}_activation_emails where name = :name"); - + // Rows are intentionally NOT deleted here: the durable source of truth for a pending + // activation e-mail is the database row, which is only removed once delivery has been + // confirmed by the SMTP client (see onEmailDelivered). This makes sending resilient to + // transient SMTP failures and server restarts. while (servDbSelQuery->next()) { const QString userName = servDbSelQuery->value(0).toString(); const auto emailAddress = EmailParser::getParsedEmailAddress(servDbSelQuery->value(1).toString()); const QString token = servDbSelQuery->value(2).toString(); - if (smtpClient->enqueueActivationTokenMail(userName, emailAddress, token)) { - queryDelete->bindValue(":name", userName); - servatriceDatabaseInterface->execSqlQuery(queryDelete); - } + smtpClient->enqueueActivationTokenMail(userName, emailAddress, token); } } if (getEnableForgotPassword()) { auto *forgotPwQuery = servatriceDatabaseInterface->prepareQuery( - "select a.name, b.email, b.token from {prefix}_forgot_password a left join {prefix}_users b on a.name " - "= b.name where a.emailed = 0"); + "select a.id, a.name, b.email, b.token from {prefix}_forgot_password a left join {prefix}_users b on " + "a.name = b.name where a.emailed = 0"); if (!servatriceDatabaseInterface->execSqlQuery(forgotPwQuery)) { return; } - QSqlQuery *queryDelete = servatriceDatabaseInterface->prepareQuery( - "update {prefix}_forgot_password set emailed = 1 where name = :name"); - + // Rows are intentionally left with emailed = 0 here; they are marked emailed only once + // delivery is confirmed by the SMTP client (see onEmailDelivered). while (forgotPwQuery->next()) { - const QString userName = forgotPwQuery->value(0).toString(); - const auto emailAddress = EmailParser::getParsedEmailAddress(forgotPwQuery->value(1).toString()); - const QString token = forgotPwQuery->value(2).toString(); + const qint64 rowId = forgotPwQuery->value(0).toLongLong(); + const QString userName = forgotPwQuery->value(1).toString(); + const auto emailAddress = EmailParser::getParsedEmailAddress(forgotPwQuery->value(2).toString()); + const QString token = forgotPwQuery->value(3).toString(); - if (smtpClient->enqueueForgotPasswordTokenMail(userName, emailAddress, token)) { - queryDelete->bindValue(":name", userName); - servatriceDatabaseInterface->execSqlQuery(queryDelete); - } + smtpClient->enqueueForgotPasswordTokenMail(userName, emailAddress, token, rowId); } } @@ -779,6 +782,69 @@ void Servatrice::statusUpdate() } } +void Servatrice::onEmailDelivered(const QString &userName, EmailType type, const QString &token, qint64 rowId) +{ + if (cleanupDatabaseForEmail(userName, type, token, rowId)) { + qCDebug(ServatriceLog) << "E-mail delivered to" << userName; + } else { + qCWarning(ServatriceLog) << "Failed to clean up database row for delivered e-mail to" << userName + << "- row may be re-enqueued on next status update"; + } +} + +void Servatrice::onEmailPermanentlyFailed(const QString &userName, + EmailType type, + FailureReason reason, + const QString &token, + qint64 rowId) +{ + switch (reason) { + case FailureReason::RetryExhausted: + qCWarning(ServatriceLog) << "E-mail to" << userName << "permanently failed after max retries"; + break; + case FailureReason::RecipientRejected: + qCWarning(ServatriceLog) << "E-mail to" << userName << "permanently failed: recipient rejected"; + break; + case FailureReason::SenderRejected: + qCWarning(ServatriceLog) << "E-mail to" << userName << "permanently failed: sender rejected"; + break; + } + + if (!cleanupDatabaseForEmail(userName, type, token, rowId)) { + qCWarning(ServatriceLog) << "Failed to clean up database row for permanently failed e-mail to" << userName + << "- row may be re-enqueued on next status update"; + } +} + +bool Servatrice::cleanupDatabaseForEmail(const QString &userName, EmailType type, const QString &token, qint64 rowId) +{ + Q_UNUSED(token); + switch (type) { + case EmailType::Activation: { + QSqlQuery *query = + servatriceDatabaseInterface->prepareQuery("delete from {prefix}_activation_emails where name = :name"); + query->bindValue(":name", userName); + return servatriceDatabaseInterface->execSqlQuery(query); + } + case EmailType::ForgotPassword: { + // Scope the update to the row that was actually sent: the table + // holds one row per request, and marking by name would also mark + // rows for newer tokens that were never delivered. + if (rowId >= 0) { + QSqlQuery *query = servatriceDatabaseInterface->prepareQuery( + "update {prefix}_forgot_password set emailed = 1 where id = :id"); + query->bindValue(":id", rowId); + return servatriceDatabaseInterface->execSqlQuery(query); + } + QSqlQuery *query = servatriceDatabaseInterface->prepareQuery( + "update {prefix}_forgot_password set emailed = 1 where name = :name"); + query->bindValue(":name", userName); + return servatriceDatabaseInterface->execSqlQuery(query); + } + } + return false; +} + SessionEvent *Servatrice::makeShutdownEvent() const { Event_ServerShutdown event; diff --git a/servatrice/src/servatrice.h b/servatrice/src/servatrice.h index 242d445ad..205a4f04d 100644 --- a/servatrice/src/servatrice.h +++ b/servatrice/src/servatrice.h @@ -21,6 +21,7 @@ #define SERVATRICE_H #include "metrics_registry.h" +#include "smtpclient.h" #include #include @@ -181,6 +182,7 @@ private: QMap serverRequiredFeatureList; QString officialWarnings; Servatrice_DatabaseInterface *servatriceDatabaseInterface; + bool cleanupDatabaseForEmail(const QString &userName, EmailType type, const QString &token, qint64 rowId); int serverId; int uptime; QMutex txBytesMutex, rxBytesMutex; @@ -231,6 +233,12 @@ public slots: void scheduleShutdown(const QString &reason, int minutes); void updateLoginMessage(); void setRequiredFeatures(const QString &featureList); + void onEmailDelivered(const QString &userName, EmailType type, const QString &token, qint64 rowId); + void onEmailPermanentlyFailed(const QString &userName, + EmailType type, + FailureReason reason, + const QString &token, + qint64 rowId); public: explicit Servatrice(QObject *parent = nullptr); diff --git a/servatrice/src/smtp/qxtsmtp.cpp b/servatrice/src/smtp/qxtsmtp.cpp index c81e0955c..a4da649f7 100644 --- a/servatrice/src/smtp/qxtsmtp.cpp +++ b/servatrice/src/smtp/qxtsmtp.cpp @@ -94,6 +94,38 @@ int QxtSmtp::pendingMessages() const return qxt_d().pending.count(); } +/*! + * \brief Drops all queued (not yet transmitted) messages without sending them. + * + * Any message still awaiting delivery is discarded. This is used by callers that + * keep their own authoritative queue and wish to rebuild the SMTP buffer from scratch + * (e.g. after a reconnect) without risking duplicates. + */ +void QxtSmtp::clearPending() +{ + qxt_d().pending.clear(); +} + +/*! + * \brief Clears the outgoing queue and resets the SMTP state machine. + * + * Unlike clearPending(), this also resets the internal state to Disconnected so + * that subsequent send() calls will not fire sendNext() prematurely on a + * not-yet-connected socket. Use this when rebuilding the queue from scratch + * before a fresh connectToHost() / connectToSecureHost() cycle. + */ +void QxtSmtp::reset() +{ + qxt_d().pending.clear(); + qxt_d().extensions.clear(); + qxt_d().buffer.clear(); + qxt_d().recipients.clear(); + qxt_d().rcptNumber = 0; + qxt_d().rcptAck = 0; + qxt_d().mailAck = false; + qxt_d().state = QxtSmtpPrivate::Disconnected; +} + QTcpSocket *QxtSmtp::socket() const { return qxt_d().socket; diff --git a/servatrice/src/smtp/qxtsmtp.h b/servatrice/src/smtp/qxtsmtp.h index 747e22df9..d264d1047 100644 --- a/servatrice/src/smtp/qxtsmtp.h +++ b/servatrice/src/smtp/qxtsmtp.h @@ -66,6 +66,8 @@ public: int send(const QxtMailMessage& message); int pendingMessages() const; + void clearPending(); + void reset(); QTcpSocket* socket() const; void connectToHost(const QString& hostName, quint16 port = 25); diff --git a/servatrice/src/smtpclient.cpp b/servatrice/src/smtpclient.cpp index a45588b8b..82824e496 100644 --- a/servatrice/src/smtpclient.cpp +++ b/servatrice/src/smtpclient.cpp @@ -7,17 +7,56 @@ #include #include #include +#include #include #include +#include #include #include #include #include +static const qint64 DEFAULT_MAX_RETRIES = 5; +static const qint64 DEFAULT_RETRY_DELAY = 60; +static const qint64 MAX_RETRY_DELAY = 1800; // 30 minutes +static const qint64 IDLE_TIMEOUT_MS = 60000; // 60 seconds +// Maximum bit-shift exponent used in the exponential back-off formula. +// With a base delay of 60 s, shift 10 gives 60 * 1024 ≈ 17 h before the +// MAX_RETRY_DELAY cap is applied, preventing unbounded intermediate values. +static const int MAX_BACKOFF_SHIFT = 10; + +inline Q_LOGGING_CATEGORY(SmtpClientLog, "smtp_client") + +static QString pendingKey(const QString &userName, EmailType type, const QString &token) +{ + const char *typeName = (type == EmailType::Activation) ? "activation" : "forgotpassword"; + // The token is part of the key: a regenerated token (forgot-password issues a + // fresh one per request) must not be suppressed by a stale pending entry. + return userName + '/' + typeName + '/' + token; +} + +// True only for permanent 5xx replies. Anything else (4xx greylisting/rate +// limiting, or an unparsable reply) must be retried, never treated as final: +// destroying the durable queue row on a transient failure loses the mail. +static bool isPermanentSmtpReply(const QByteArray &msg) +{ + for (char c : msg) { + if (c == ' ' || c == '\t' || c == '\r' || c == '\n' || c == '-') { + continue; + } + return c == '5'; + } + return false; +} + SmtpClient::SmtpClient(QObject *parent) : QObject(parent) { smtp = new QxtSmtp(this); + idleTimer = new QTimer(this); + idleTimer->setSingleShot(true); + connect(idleTimer, SIGNAL(timeout()), this, SLOT(idleTimeout())); + connect(smtp, SIGNAL(authenticated()), this, SLOT(authenticated())); connect(smtp, SIGNAL(authenticationFailed(const QByteArray &)), this, SLOT(authenticationFailed(const QByteArray &))); @@ -40,104 +79,151 @@ SmtpClient::~SmtpClient() { if (smtp) { delete smtp; - smtp = 0; + smtp = nullptr; } } +int SmtpClient::maxRetries() const +{ + // Bounded like retryDelaySeconds(): an unset/blank value must not turn the + // first transient failure into a permanent one (attempts >= 0 is immediate). + return qBound(1, settingsCache->value("smtp/maxretries", DEFAULT_MAX_RETRIES).toInt(), 100); +} + +int SmtpClient::retryDelaySeconds() const +{ + return qBound(1, settingsCache->value("smtp/retrydelay", DEFAULT_RETRY_DELAY).toInt(), + static_cast(MAX_RETRY_DELAY)); +} + +QDateTime SmtpClient::nextRetryTime(int attempts, int retryDelay) const +{ + // Exponential backoff, capped to avoid hammering an unreachable server. + const int shift = qMax(0, qMin(attempts - 1, MAX_BACKOFF_SHIFT)); + const qint64 delay = qMin(retryDelay * qint64(1) << shift, MAX_RETRY_DELAY); + return QDateTime::currentDateTime().addSecs(delay); +} + +SmtpClient::PendingEmail SmtpClient::buildEmail(const QString &nickname, + EmailType type, + const QString &subject, + const QString &body, + const QString &recipient, + const QString &token, + const QString &email, + qint64 rowId) +{ + QString name = settingsCache->value("smtp/name", "").toString(); + + PendingEmail pending; + pending.userName = nickname; + pending.type = type; + pending.token = token; + pending.rowId = rowId; + pending.maxAttempts = maxRetries(); + pending.retryDelay = retryDelaySeconds(); + pending.nextAttempt = QDateTime::currentDateTime(); + + QString emailBody = body; + emailBody.replace("%username", nickname).replace("%token", token); + + pending.message.setSender(name + " <" + email + ">"); + pending.message.addRecipient(recipient); + pending.message.setSubject(subject); + pending.message.setBody(emailBody); + + return pending; +} + +bool SmtpClient::enqueue(const PendingEmail &email) +{ + const QString key = pendingKey(email.userName, email.type, email.token); + if (pendingEmails.contains(key)) { + qCDebug(SmtpClientLog) << "Email already pending for" << email.userName << "- not duplicating"; + return false; + } + + // A fresher database row for the same user/type supersedes a stale pending + // entry: drop the stale one so only the current token is ever delivered. + // (Any in-flight mapping for the dropped key is removed as well; the + // mailSent/recordFailed lookups tolerate missing entries.) + for (auto it = pendingEmails.begin(); it != pendingEmails.end();) { + if (it.value().userName == email.userName && it.value().type == email.type) { + for (auto mapIt = mailIdToUser.begin(); mapIt != mailIdToUser.end();) { + if (mapIt.value() == it.key()) { + mapIt = mailIdToUser.erase(mapIt); + } else { + ++mapIt; + } + } + qCDebug(SmtpClientLog) << "Replacing stale pending email for" << email.userName; + it = pendingEmails.erase(it); + } else { + ++it; + } + } + + pendingEmails.insert(key, email); + qCDebug(SmtpClientLog) << "Enqueued" << (email.type == EmailType::Activation ? "activation" : "password reset") + << "mail to" << email.userName; + return true; +} + bool SmtpClient::enqueueActivationTokenMail(const QString &nickname, const QString &recipient, const QString &token) { - QString email = settingsCache->value("smtp/email", "").toString(); - QString name = settingsCache->value("smtp/name", "").toString(); + if (pendingEmails.contains(pendingKey(nickname, EmailType::Activation, token))) { + qCDebug(SmtpClientLog) << "Activation email already pending for" << nickname << "- skipping"; + return false; + } + QString subject = settingsCache->value("smtp/subject", "").toString(); QString body = settingsCache->value("smtp/body", "").toString(); + QString email = settingsCache->value("smtp/email", "").toString(); - if (email.isEmpty()) { - qDebug() << "[MAIL] Missing sender email in configuration"; + if (!validateEmailFields(email, subject, body, recipient, token, nickname)) { return false; } - if (subject.isEmpty()) { - qDebug() << "[MAIL] Missing subject field in configuration"; - return false; - } - - if (body.isEmpty()) { - qDebug() << "[MAIL] Missing body field in configuration"; - return false; - } - - if (recipient.isEmpty()) { - qDebug() << "[MAIL] Missing recipient field for user " << nickname; - return false; - } - - if (token.isEmpty()) { - qDebug() << "[MAIL] Missing token field for user " << nickname; - return false; - } - - QxtMailMessage message; - message.setSender(name + " <" + email + ">"); - message.addRecipient(recipient); - message.setSubject(subject); - message.setBody(body.replace("%username", nickname).replace("%token", token)); - - int id = smtp->send(message); - qDebug() << "[MAIL] Enqueued mail to" << recipient << "as" << id; - return true; + return enqueue(buildEmail(nickname, EmailType::Activation, subject, body, recipient, token, email)); } -bool SmtpClient::enqueueForgotPasswordTokenMail(const QString &nickname, const QString &recipient, const QString &token) +bool SmtpClient::enqueueForgotPasswordTokenMail(const QString &nickname, + const QString &recipient, + const QString &token, + qint64 rowId) { - QString email = settingsCache->value("smtp/email", "").toString(); - QString name = settingsCache->value("smtp/name", "").toString(); + if (pendingEmails.contains(pendingKey(nickname, EmailType::ForgotPassword, token))) { + qCDebug(SmtpClientLog) << "Password reset email already pending for" << nickname << "- skipping"; + return false; + } + QString subject = settingsCache->value("forgotpassword/subject", "").toString(); QString body = settingsCache->value("forgotpassword/body", "").toString(); + QString email = settingsCache->value("smtp/email", "").toString(); - if (email.isEmpty()) { - qDebug() << "[MAIL] Missing sender email in configuration"; + if (!validateEmailFields(email, subject, body, recipient, token, nickname)) { return false; } - if (subject.isEmpty()) { - qDebug() << "[MAIL] Missing subject field in configuration"; - return false; - } - - if (body.isEmpty()) { - qDebug() << "[MAIL] Missing body field in configuration"; - return false; - } - - if (recipient.isEmpty()) { - qDebug() << "[MAIL] Missing recipient field for user " << nickname; - return false; - } - - if (token.isEmpty()) { - qDebug() << "[MAIL] Missing token field for user " << nickname; - return false; - } - - QxtMailMessage message; - message.setSender(name + " <" + email + ">"); - message.addRecipient(recipient); - message.setSubject(subject); - message.setBody(body.replace("%username", nickname).replace("%token", token)); - - int id = smtp->send(message); - qDebug() << "[MAIL] Enqueued mail to" << recipient << "as" << id; - return true; + return enqueue(buildEmail(nickname, EmailType::ForgotPassword, subject, body, recipient, token, email, rowId)); } void SmtpClient::sendAllEmails() { // still connected from the previous round - if (smtp->socket()->state() == QAbstractSocket::ConnectedState) { + if (smtp->socket()->state() == QAbstractSocket::ConnectedState || + smtp->socket()->state() == QAbstractSocket::ConnectingState || + smtp->socket()->state() == QAbstractSocket::HostLookupState || + smtp->socket()->state() == QAbstractSocket::ClosingState) { return; } - if (smtp->pendingMessages() == 0) { + if (pendingEmails.isEmpty()) { + return; + } + + // Respect connection-level backoff after auth/conn/encryption failures. + if (connectionAttempts > 0 && QDateTime::currentDateTime() < connectionBackoffUntil) { return; } @@ -151,6 +237,16 @@ void SmtpClient::sendAllEmails() smtp->setUsername(username); smtp->setPassword(password); + // Push due emails into QxtSmtp *before* connecting so they are already + // queued when the authenticated() signal fires and QxtSmtp's internal + // sendNext() runs. + relayoutForSending(); + + // Nothing due right now — skip the SMTP round-trip. + if (mailIdToUser.isEmpty()) { + return; + } + // Connect if (connectionType == "ssl") { if (acceptAllCerts) { @@ -162,64 +258,286 @@ void SmtpClient::sendAllEmails() } } -void SmtpClient::authenticated() +void SmtpClient::relayoutForSending() { - qDebug() << "[MAIL] authenticated"; -} + // Rebuild the SMTP buffer exclusively from our authoritative set, so a reconnect + // after a partial transmission never results in duplicate deliveries. + // reset() also clears the internal state machine so that send() does not + // fire sendNext() prematurely on a not-yet-connected socket. + smtp->reset(); + mailIdToUser.clear(); -void SmtpClient::authenticationFailed(const QByteArray &msg) -{ - qDebug() << "[MAIL] authenticationFailed" << QString(msg); -} - -void SmtpClient::connected() -{ - qDebug() << "[MAIL] connected"; -} - -void SmtpClient::connectionFailed(const QByteArray &msg) -{ - qDebug() << "[MAIL] connectionFailed" << QString(msg); -} - -void SmtpClient::disconnected() -{ - qDebug() << "[MAIL] disconnected"; -} - -void SmtpClient::encrypted() -{ - qDebug() << "[MAIL] encrypted"; -} - -void SmtpClient::encryptionFailed(const QByteArray &msg) -{ - qDebug() << "[MAIL] encryptionFailed" << QString(msg); - qDebug() << "[MAIL] Try enabling the \"acceptallcerts\" option in servatrice.ini"; + const QDateTime now = QDateTime::currentDateTime(); + for (auto it = pendingEmails.begin(); it != pendingEmails.end(); ++it) { + PendingEmail &pending = it.value(); + if (pending.nextAttempt > now) { + continue; // not yet due; honours the backoff schedule + } + int id = smtp->send(pending.message); + mailIdToUser.insert(id, pendingKey(pending.userName, pending.type, pending.token)); + } } void SmtpClient::finished() { - qDebug() << "[MAIL] finished"; + qCDebug(SmtpClientLog) << "finished"; smtp->disconnectFromHost(); } +void SmtpClient::authenticated() +{ + qCDebug(SmtpClientLog) << "authenticated"; + // The session has actually succeeded (TCP connect alone proves nothing: + // TLS/auth may still fail), so the connection-level backoff resets here. + connectionAttempts = 0; + // Start the idle timer here, once the connection is fully ready to send, + // rather than on TCP connect: SSL/STARTTLS handshake and auth may take a + // while and must not be cut short by the idle timeout. + idleTimer->start(IDLE_TIMEOUT_MS); +} + +void SmtpClient::authenticationFailed(const QByteArray &msg) +{ + qCWarning(SmtpClientLog) << "authenticationFailed" << QString::fromUtf8(msg); + idleTimer->stop(); + recordConnectionFailure(); + mailIdToUser.clear(); + smtp->disconnectFromHost(); +} + +void SmtpClient::connected() +{ + qCDebug(SmtpClientLog) << "connected"; + // NOTE: connectionAttempts is deliberately not reset here. This fires on + // TCP connect, before the TLS handshake and AUTH; resetting here would pin + // the connection-level backoff at its base delay for exactly the failures + // (bad password/cert/STARTTLS) it is meant to cover. It resets in + // authenticated() and on the first successful mailSent instead. +} + +void SmtpClient::connectionFailed(const QByteArray &msg) +{ + qCWarning(SmtpClientLog) << "connectionFailed" << QString::fromUtf8(msg); + idleTimer->stop(); + recordConnectionFailure(); + mailIdToUser.clear(); + smtp->disconnectFromHost(); +} + +void SmtpClient::disconnected() +{ + qCDebug(SmtpClientLog) << "disconnected - emails still pending:" << pendingEmails.count(); + idleTimer->stop(); + // A mid-session drop (peer close/RST past the connect phase, or the idle + // timeout firing mid-batch) emits no mailFailed/connectionFailed signal, so + // route every in-flight mail through recordFailed(): otherwise attempts and + // backoff are bypassed and the mails requeue with stale state forever. + const QList inFlight = mailIdToUser.keys(); + for (int mailID : inFlight) { + recordFailed(mailID); + } + mailIdToUser.clear(); +} + +void SmtpClient::encrypted() +{ + qCDebug(SmtpClientLog) << "encrypted"; +} + +void SmtpClient::encryptionFailed(const QByteArray &msg) +{ + qCWarning(SmtpClientLog) << "encryptionFailed" << QString::fromUtf8(msg); + qCWarning(SmtpClientLog) << "Try enabling the \"acceptallcerts\" option in servatrice.ini"; + idleTimer->stop(); + recordConnectionFailure(); + mailIdToUser.clear(); + smtp->disconnectFromHost(); +} + +void SmtpClient::idleTimeout() +{ + qCDebug(SmtpClientLog) << "idle timeout - disconnecting"; + smtp->disconnectFromHost(); +} + +void SmtpClient::recordFailed(int mailID) +{ + auto it = mailIdToUser.find(mailID); + if (it == mailIdToUser.end()) { + return; + } + const QString key = it.value(); + mailIdToUser.erase(it); + + auto pendingIt = pendingEmails.find(key); + if (pendingIt == pendingEmails.end()) { + return; + } + + PendingEmail &pending = pendingIt.value(); + pending.attempts++; + + if (pending.attempts >= pending.maxAttempts) { + const QString userName = pending.userName; + const EmailType type = pending.type; + const QString token = pending.token; + const qint64 rowId = pending.rowId; + qCWarning(SmtpClientLog) << "Email to" << userName << "permanently failed after" << pending.attempts + << "attempts - dropping from pending set"; + pendingEmails.erase(pendingIt); + emit mailPermanentlyFailed(userName, type, FailureReason::RetryExhausted, token, rowId); + return; + } + + pending.nextAttempt = nextRetryTime(pending.attempts, pending.retryDelay); + qCWarning(SmtpClientLog) << "Email to" << pending.userName << "failed (attempt" << pending.attempts << "of" + << pending.maxAttempts << "), retrying" << pending.nextAttempt.toString(Qt::ISODate); +} + +void SmtpClient::recordConnectionFailure() +{ + ++connectionAttempts; + connectionBackoffUntil = nextRetryTime(connectionAttempts, retryDelaySeconds()); + qCWarning(SmtpClientLog) << "Connection-layer failure #" << connectionAttempts << "- next attempt after" + << connectionBackoffUntil.toString(Qt::ISODate); + + // Connection failures count toward the per-email retry budget so that + // undeliverable mail eventually gives up rather than retrying forever. + QList keysToRemove; + for (auto it = pendingEmails.begin(); it != pendingEmails.end(); ++it) { + PendingEmail &pending = it.value(); + pending.attempts++; + if (pending.attempts >= pending.maxAttempts) { + qCWarning(SmtpClientLog) << "Email to" << pending.userName << "permanently failed after" << pending.attempts + << "connection failures - dropping from pending set"; + keysToRemove.append(it.key()); + emit mailPermanentlyFailed(pending.userName, pending.type, FailureReason::RetryExhausted, pending.token, + pending.rowId); + } + } + for (const QString &key : keysToRemove) { + pendingEmails.remove(key); + } +} + +bool SmtpClient::validateEmailFields(const QString &email, + const QString &subject, + const QString &body, + const QString &recipient, + const QString &token, + const QString &nickname) +{ + if (email.isEmpty()) { + qCCritical(SmtpClientLog) << "Missing sender email in configuration"; + return false; + } + if (subject.isEmpty()) { + qCCritical(SmtpClientLog) << "Missing subject field in configuration"; + return false; + } + if (body.isEmpty()) { + qCCritical(SmtpClientLog) << "Missing body field in configuration"; + return false; + } + if (recipient.isEmpty()) { + qCCritical(SmtpClientLog) << "Missing recipient field for user" << nickname; + return false; + } + if (token.isEmpty()) { + qCCritical(SmtpClientLog) << "Missing token field for user" << nickname; + return false; + } + return true; +} + void SmtpClient::mailFailed(int mailID, int errorCode, const QByteArray &msg) { - qDebug() << "[MAIL] mailFailed id=" << mailID << " errorCode=" << errorCode << "msg=" << QString(msg); + qCWarning(SmtpClientLog) << "mailFailed id=" << mailID << " errorCode=" << errorCode + << "msg=" << QString::fromUtf8(msg); + // Progress was made (the server answered), so the idle timer measures from + // here rather than acting as a cap on the whole session. + idleTimer->start(IDLE_TIMEOUT_MS); + recordFailed(mailID); } void SmtpClient::mailSent(int mailID) { - qDebug() << "[MAIL] mailSent" << mailID; + qCDebug(SmtpClientLog) << "mailSent" << mailID; + // A delivered mail proves the session works: reset the connection-level + // backoff and restart the idle timer from this point of progress. + connectionAttempts = 0; + idleTimer->start(IDLE_TIMEOUT_MS); + + auto it = mailIdToUser.find(mailID); + if (it == mailIdToUser.end()) { + return; + } + const QString key = it.value(); + mailIdToUser.erase(it); + + auto pendingIt = pendingEmails.find(key); + if (pendingIt == pendingEmails.end()) { + return; + } + + const QString userName = pendingIt.value().userName; + const EmailType type = pendingIt.value().type; + const QString token = pendingIt.value().token; + const qint64 rowId = pendingIt.value().rowId; + pendingEmails.erase(pendingIt); + qCDebug(SmtpClientLog) << "Delivered email to" << userName; + emit mailDelivered(userName, type, token, rowId); } void SmtpClient::recipientRejected(int mailID, const QString &address, const QByteArray &msg) { - qDebug() << "[MAIL] recipientRejected id=" << mailID << " address=" << address << "msg=" << QString(msg); + qCWarning(SmtpClientLog) << "recipientRejected id=" << mailID << " address=" << address + << "msg=" << QString::fromUtf8(msg); + if (!isPermanentSmtpReply(msg)) { + // Transient 4xx (e.g. greylisting): consume one attempt and reschedule + // with backoff instead of destroying the durable queue row. + recordFailed(mailID); + return; + } + // A rejected recipient is a permanent condition; consider the mail undeliverable. + auto it = mailIdToUser.find(mailID); + if (it != mailIdToUser.end()) { + const QString key = it.value(); + if (auto pendingIt = pendingEmails.find(key); pendingIt != pendingEmails.end()) { + const QString userName = pendingIt.value().userName; + const EmailType type = pendingIt.value().type; + const QString token = pendingIt.value().token; + const qint64 rowId = pendingIt.value().rowId; + pendingEmails.erase(pendingIt); + qCWarning(SmtpClientLog) << "Email to" << userName << "rejected - dropping from pending set"; + emit mailPermanentlyFailed(userName, type, FailureReason::RecipientRejected, token, rowId); + } + mailIdToUser.erase(it); + } } void SmtpClient::senderRejected(int mailID, const QString &address, const QByteArray &msg) { - qDebug() << "[MAIL] senderRejected id=" << mailID << " address=" << address << "msg=" << QString(msg); + qCWarning(SmtpClientLog) << "senderRejected id=" << mailID << " sender=" << address + << "msg=" << QString::fromUtf8(msg); + if (!isPermanentSmtpReply(msg)) { + // Transient 4xx on MAIL FROM (e.g. rate limiting): retry with backoff. + recordFailed(mailID); + return; + } + // A rejected sender is a permanent condition; consider the mail undeliverable. + auto it = mailIdToUser.find(mailID); + if (it != mailIdToUser.end()) { + const QString key = it.value(); + if (auto pendingIt = pendingEmails.find(key); pendingIt != pendingEmails.end()) { + const QString userName = pendingIt.value().userName; + const EmailType type = pendingIt.value().type; + const QString token = pendingIt.value().token; + const qint64 rowId = pendingIt.value().rowId; + pendingEmails.erase(pendingIt); + qCWarning(SmtpClientLog) << "Email to" << userName << "sender rejected - dropping from pending set"; + emit mailPermanentlyFailed(userName, type, FailureReason::SenderRejected, token, rowId); + } + mailIdToUser.erase(it); + } } diff --git a/servatrice/src/smtpclient.h b/servatrice/src/smtpclient.h index 8fc4e35cb..9fc11bf36 100644 --- a/servatrice/src/smtpclient.h +++ b/servatrice/src/smtpclient.h @@ -1,26 +1,67 @@ #ifndef SMTPCLIENT_H #define SMTPCLIENT_H +#include "smtp/qxtmailmessage.h" + +#include +#include #include #include +#include #include class QxtSmtp; class QByteArray; +/** + * @brief Types of e-mail the SMTP client is able to deliver. + */ +enum class EmailType +{ + Activation, + ForgotPassword +}; + +Q_DECLARE_METATYPE(EmailType) + +/** + * @brief Why a permanent e-mail delivery failure occurred. + */ +enum class FailureReason +{ + RetryExhausted, + RecipientRejected, + SenderRejected +}; + +Q_DECLARE_METATYPE(FailureReason) + class SmtpClient : public QObject { Q_OBJECT public: - SmtpClient(QObject *parent = 0); - ~SmtpClient(); + SmtpClient(QObject *parent = nullptr); + ~SmtpClient() override; + +public slots: + bool enqueueActivationTokenMail(const QString &nickname, const QString &recipient, const QString &token); + bool enqueueForgotPasswordTokenMail(const QString &nickname, + const QString &recipient, + const QString &token, + qint64 rowId = -1); + void sendAllEmails(); + +signals: + void mailDelivered(const QString &userName, EmailType type, const QString &token, qint64 rowId); + void mailPermanentlyFailed(const QString &userName, + EmailType type, + FailureReason reason, + const QString &token, + qint64 rowId); protected: QxtSmtp *smtp; -public slots: - bool enqueueActivationTokenMail(const QString &nickname, const QString &recipient, const QString &token); - bool enqueueForgotPasswordTokenMail(const QString &nickname, const QString &recipient, const QString &token); - void sendAllEmails(); + protected slots: void authenticated(); void authenticationFailed(const QByteArray &msg); @@ -30,10 +71,55 @@ protected slots: void encrypted(); void encryptionFailed(const QByteArray &msg); void finished(); + void idleTimeout(); void mailFailed(int mailID, int errorCode, const QByteArray &msg); void mailSent(int mailID); void recipientRejected(int mailID, const QString &address, const QByteArray &msg); void senderRejected(int mailID, const QString &address, const QByteArray &msg); + +private: + struct PendingEmail + { + QString userName; + EmailType type; + QString token; + // Database row id for tables that allow multiple rows per user + // ({prefix}_forgot_password); -1 when the name alone identifies the row. + qint64 rowId = -1; + QxtMailMessage message; + int attempts = 0; + int maxAttempts = 0; + int retryDelay = 0; + QDateTime nextAttempt; + }; + + PendingEmail buildEmail(const QString &nickname, + EmailType type, + const QString &subject, + const QString &body, + const QString &recipient, + const QString &token, + const QString &email, + qint64 rowId = -1); + bool enqueue(const PendingEmail &email); + void relayoutForSending(); + QDateTime nextRetryTime(int attempts, int retryDelay) const; + void recordFailed(int mailID); + void recordConnectionFailure(); + bool validateEmailFields(const QString &email, + const QString &subject, + const QString &body, + const QString &recipient, + const QString &token, + const QString &nickname); + int maxRetries() const; + int retryDelaySeconds() const; + + QHash pendingEmails; + QHash mailIdToUser; + QTimer *idleTimer; + int connectionAttempts = 0; + QDateTime connectionBackoffUntil; }; -#endif \ No newline at end of file +#endif diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 22dbedb98..9955e1189 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -206,6 +206,17 @@ cockatrice_add_test( LABELS server ) +cockatrice_add_test( + NAME + server_dump_zone_test + SOURCES + server_dump_zone_test.cpp + LIBS + libcockatrice_rng + libcockatrice_network + LABELS + server +) cockatrice_add_test( NAME server_counter_test diff --git a/tests/carddatabase/carddatabase_test.cpp b/tests/carddatabase/carddatabase_test.cpp index 1ac259d9b..1b48d934b 100644 --- a/tests/carddatabase/carddatabase_test.cpp +++ b/tests/carddatabase/carddatabase_test.cpp @@ -159,16 +159,19 @@ public: void setSortKey(QString shortName, unsigned int sortKey) override { + ++individualWrites; setOptions[shortName].sortKey = sortKey; } void setEnabled(QString shortName, bool enabled) override { + ++individualWrites; setOptions[shortName].enabled = enabled; } void setIsKnown(QString shortName, bool isknown) override { + ++individualWrites; setOptions[shortName].isKnown = isknown; } @@ -206,12 +209,17 @@ public: void saveSets(const QVector &data) override { + ++batchedWrites; for (const SetSaveData &entry : data) { setOptions[entry.shortName].sortKey = entry.sortKey; setOptions[entry.shortName].enabled = entry.enabled; + setOptions[entry.shortName].isKnown = entry.isKnown; } } + int individualWrites = 0; ///< Per-set/per-option controller writes (the slow path). + int batchedWrites = 0; ///< Batched saveSets() flushes (the fast path). + private: QHash setOptions; }; @@ -375,6 +383,131 @@ TEST_F(CardDatabaseCacheInvalidationTest, ExplicitReloadBypassesCache) EXPECT_FALSE(db.getCardList().value("Cat")->getSets().contains("CAT")); EXPECT_TRUE(db.getCardList().value("Cat")->getSets().contains("DOG")); } + +TEST_F(CardDatabaseCacheInvalidationTest, FirstRunWithNoSetsEnabledSkipsCacheWrite) +{ + // A blank config: the parser discovers every set, but none of them has an + // ini entry, so all of them default to disabled (first run). + SeededSetPriorityController blankController; + NoopCardPreferenceProvider prefs; + SandboxPathProvider pathProvider(root); + CardDatabase db(nullptr, &prefs, &pathProvider, &blankController); + + db.loadCardDatabases(); + ASSERT_FALSE(db.getCardList().value("Cat").isNull()) << "cards are parsed even while their sets are disabled"; + EXPECT_TRUE(db.getCardList().value("Cat")->getSets().isEmpty()) + << "no printings survive an all-sets-disabled parse"; + EXPECT_TRUE(cacheContents().isEmpty()) << "a stripped first-run snapshot must not be cached"; +} + +TEST_F(CardDatabaseCacheInvalidationTest, EnablingAllSetsAfterFirstRunRestoresPrintings) +{ + SeededSetPriorityController blankController; + NoopCardPreferenceProvider prefs; + SandboxPathProvider pathProvider(root); + CardDatabase db(nullptr, &prefs, &pathProvider, &blankController); + + db.loadCardDatabases(); + ASSERT_TRUE(cacheContents().isEmpty()); + + // This mirrors what checkUnknownSets() does on a first run: enable every + // set, then reload so the parser keeps the printings and the complete + // snapshot gets cached under the enabled-set hash. + for (const CardSetPtr &set : db.getSetList()) { + blankController.setEnabled(set->getShortName(), true); + } + db.reloadCardDatabasesAndNotify(); + + ASSERT_FALSE(db.getCardList().value("Cat").isNull()); + EXPECT_TRUE(db.getCardList().value("Cat")->getSets().contains("CAT")); + EXPECT_FALSE(cacheContents().isEmpty()) << "the enabled-set snapshot must be cached"; +} + +TEST_F(CardDatabaseCacheInvalidationTest, CheckUnknownSetsBatchesFirstRunEnableAll) +{ + // Same first-run starting point as above, but driven through the real + // checkUnknownSets() path, which must persist the enable-all with a single + // batched saveSets() instead of per-set/per-option controller writes. + SeededSetPriorityController blankController; + NoopCardPreferenceProvider prefs; + SandboxPathProvider pathProvider(root); + CardDatabase db(nullptr, &prefs, &pathProvider, &blankController); + + db.loadCardDatabases(); + ASSERT_TRUE(db.getSetList().getEnabledSetsNum() == 0); + + db.checkUnknownSets(); + + EXPECT_EQ(blankController.individualWrites, 0) << "first-run enable-all must not write per set"; + EXPECT_EQ(blankController.batchedWrites, 1) << "first-run enable-all must persist with a single flush"; + for (const CardSetPtr &set : db.getSetList()) { + EXPECT_TRUE(set->getEnabled()) << set->getShortName().toStdString(); + EXPECT_TRUE(blankController.isEnabled(set->getShortName())) << set->getShortName().toStdString(); + if (!set->getIsKnownIgnored()) { + EXPECT_TRUE(set->getIsKnown()) << set->getShortName().toStdString(); + EXPECT_TRUE(blankController.isKnown(set->getShortName())) << set->getShortName().toStdString(); + } + } + + // The follow-up reload re-parses with the sets enabled, restoring printings. + db.reloadCardDatabasesAndNotify(); + ASSERT_FALSE(db.getCardList().value("Cat").isNull()); + EXPECT_TRUE(db.getCardList().value("Cat")->getSets().contains("CAT")); +} + +TEST_F(CardDatabaseCacheInvalidationTest, EnableAllUnknownSetsIsBatched) +{ + // The "new sets found" dialog path: enabling the unknown delta must persist + // with a single flush no matter how many sets the database holds, so users + // with hundreds of custom sets don't stall the GUI thread. Built directly + // on CardSet objects because the fixture sets carry no metadata (and would + // all count as ignored). + SeededSetPriorityController controller; + CardSetList sets; + sets.append( + CardSet::newInstance(&controller, "AAA", "Alpha Set", "core", QDate(2020, 1, 1), CardSet::PriorityPrimary)); + sets.append( + CardSet::newInstance(&controller, "BBB", "Beta Set", "expansion", QDate(2021, 6, 1), CardSet::PriorityPrimary)); + sets.append(CardSet::newInstance(&controller, "XXX")); // no metadata: ignored + + sets.enableAllUnknownBatched(&controller); + + EXPECT_EQ(controller.individualWrites, 0) << "unknown-set enable must not write per set"; + EXPECT_EQ(controller.batchedWrites, 1) << "unknown-set enable must persist with a single flush"; + for (const CardSetPtr &set : sets) { + EXPECT_TRUE(set->getEnabled()) << set->getShortName().toStdString(); + EXPECT_TRUE(controller.isEnabled(set->getShortName())) << set->getShortName().toStdString(); + if (set->getIsKnownIgnored()) { + EXPECT_FALSE(set->getIsKnown()) << set->getShortName().toStdString(); + } else { + EXPECT_TRUE(set->getIsKnown()) << set->getShortName().toStdString(); + EXPECT_TRUE(controller.isKnown(set->getShortName())) << set->getShortName().toStdString(); + } + } +} + +TEST_F(CardDatabaseCacheInvalidationTest, MarkAllSetsAsKnownIsBatched) +{ + // The dialog's "No" path: same single-flush requirement as enabling. + SeededSetPriorityController controller; + CardSetList sets; + sets.append( + CardSet::newInstance(&controller, "AAA", "Alpha Set", "core", QDate(2020, 1, 1), CardSet::PriorityPrimary)); + sets.append( + CardSet::newInstance(&controller, "BBB", "Beta Set", "expansion", QDate(2021, 6, 1), CardSet::PriorityPrimary)); + sets.append(CardSet::newInstance(&controller, "XXX")); // no metadata: ignored + + sets.markAllAsKnownBatched(&controller); + + EXPECT_EQ(controller.individualWrites, 0) << "mark-known must not write per set"; + EXPECT_EQ(controller.batchedWrites, 1) << "mark-known must persist with a single flush"; + EXPECT_TRUE(controller.isKnown("AAA")); + EXPECT_TRUE(controller.isKnown("BBB")); + EXPECT_FALSE(controller.isEnabled("AAA")); + EXPECT_FALSE(controller.isEnabled("BBB")); + // Ignored sets keep their known state but are enabled. + EXPECT_TRUE(controller.isEnabled("XXX")); +} } // namespace int main(int argc, char **argv) diff --git a/tests/loading_from_clipboard/loading_from_clipboard_test.cpp b/tests/loading_from_clipboard/loading_from_clipboard_test.cpp index 840d926ff..e1797818e 100644 --- a/tests/loading_from_clipboard/loading_from_clipboard_test.cpp +++ b/tests/loading_from_clipboard/loading_from_clipboard_test.cpp @@ -80,6 +80,20 @@ TEST(LoadingFromClipboardTest, WeirdWhitespaceIsIgnored) testDeck(clipboard, result); } +TEST(LoadingFromClipboardTest, TabSeparatedImport) +{ + QString clipboard("2\tMystic Snake\tStreets of New Capenna\n" + "3x\tCounterspell\tNPH\n" + "\t2\tSol Ring\tFMB\n" + "1\tForest\n" + "\n" + "2x\tDoom Blade\tM11\n"); + + Result result("", "", {{"Mystic Snake", 2}, {"Counterspell", 3}, {"Sol Ring", 2}, {"Forest", 1}}, + {{"Doom Blade", 2}}); + testDeck(clipboard, result); +} + TEST(LoadingFromClipboardTest, RemoveBlankEntriesFromBeginningAndEnd) { QString clipboard("\n" diff --git a/tests/server_dump_zone_test.cpp b/tests/server_dump_zone_test.cpp new file mode 100644 index 000000000..71c410655 --- /dev/null +++ b/tests/server_dump_zone_test.cpp @@ -0,0 +1,125 @@ +/** @file server_dump_zone_test.cpp + * @brief Tests for resolving a client's zone dump request against a zone's card count. + * @ingroup Tests + */ + +#include +#include +#include +#include +#include + +namespace +{ +/// Indices a dump request resolves to, as the handler would walk them. +std::vector walked(qsizetype cardCount, int numberCards, bool isReversed) +{ + const auto [begin, end] = dumpZoneCardRange(cardCount, numberCards, isReversed); + std::vector out; + for (qsizetype i = begin; i < end; ++i) { + out.push_back(i); + } + return out; +} +} // namespace + +TEST(ServerDumpZone, ForwardTakesLeadingCards) +{ + EXPECT_EQ(walked(5, 2, false), (std::vector{0, 1})); +} + +TEST(ServerDumpZone, ForwardTakingAllCardsCoversTheZone) +{ + EXPECT_EQ(walked(3, 3, false), (std::vector{0, 1, 2})); +} + +TEST(ServerDumpZone, ForwardAskingForOneDumpsOne) +{ + EXPECT_EQ(walked(3, 1, false), (std::vector{0})); +} + +TEST(ServerDumpZone, MinusOneMeansTheWholeZone) +{ + EXPECT_EQ(walked(4, -1, false), (std::vector{0, 1, 2, 3})); +} + +TEST(ServerDumpZone, ReversedTakesTrailingCards) +{ + EXPECT_EQ(walked(5, 2, true), (std::vector{3, 4})); +} + +TEST(ServerDumpZone, ReversedAskingForOneTakesTheLast) +{ + EXPECT_EQ(walked(5, 1, true), (std::vector{4})); +} + +TEST(ServerDumpZone, ZeroDumpsNothing) +{ + EXPECT_TRUE(walked(5, 0, false).empty()); + EXPECT_TRUE(walked(5, 0, true).empty()); +} + +// A request for more cards than the zone holds is clamped rather than walked off the end. +// Forward, the old loop stopped at the zone's size on its own. +TEST(ServerDumpZone, ForwardOversizedRequestIsClamped) +{ + EXPECT_EQ(walked(3, 1000, false), (std::vector{0, 1, 2})); +} + +// Reversed, an oversized request put the offset at cardCount - numberCards, which is +// negative, so the walk started before the first card. +TEST(ServerDumpZone, ReversedOversizedRequestIsClamped) +{ + EXPECT_EQ(walked(3, 1000, true), (std::vector{0, 1, 2})); +} + +// Reversed, -1 put the offset at cardCount + 1, so the walk started one past the last card. +TEST(ServerDumpZone, ReversedWholeZoneRequestStaysInRange) +{ + EXPECT_EQ(walked(3, -1, true), (std::vector{0, 1, 2})); +} + +// Only -1 means "the whole zone". Any other negative count is not a request for anything, +// so it resolves to an empty range rather than to the whole zone. +TEST(ServerDumpZone, OtherNegativeCountsDumpNothing) +{ + EXPECT_TRUE(walked(5, -2, false).empty()); + EXPECT_TRUE(walked(5, -2, true).empty()); + EXPECT_TRUE(walked(5, INT_MIN, true).empty()); +} + +TEST(ServerDumpZone, EmptyZoneYieldsAnEmptyRange) +{ + EXPECT_TRUE(walked(0, -1, false).empty()); + EXPECT_TRUE(walked(0, -1, true).empty()); + EXPECT_TRUE(walked(0, 10, false).empty()); + EXPECT_TRUE(walked(0, 10, true).empty()); +} + +TEST(ServerDumpZone, NonPositiveCardCountYieldsAnEmptyRange) +{ + EXPECT_TRUE(walked(-1, -1, false).empty()); + EXPECT_TRUE(walked(-1, -1, true).empty()); +} + +// The invariant the handler depends on: whatever a client asks for, the indices it walks +// are a subset of the zone's cards and never step outside the list. number_cards is an +// unbounded client-supplied sint32, so this sweeps the edges rather than a chosen few. +TEST(ServerDumpZone, ResolvedRangeIsAlwaysWithinTheZone) +{ + const QList cardCounts = {0, 1, 2, 3, 7, 60, 1000000}; + const QList requests = {INT_MIN, -1000, -2, -1, 0, 1, 2, 3, 59, 60, 61, 999999, INT_MAX}; + + for (int cardCount : cardCounts) { + for (int request : requests) { + for (bool isReversed : {false, true}) { + const auto [begin, end] = dumpZoneCardRange(cardCount, request, isReversed); + SCOPED_TRACE(testing::Message() + << "cardCount=" << cardCount << " request=" << request << " isReversed=" << isReversed); + ASSERT_GE(begin, 0); + ASSERT_GE(end, begin); + ASSERT_LE(end, cardCount); + } + } + } +} diff --git a/tests/settings/settings_migration_test.cpp b/tests/settings/settings_migration_test.cpp index 440e49b74..2c4dcf7a9 100644 --- a/tests/settings/settings_migration_test.cpp +++ b/tests/settings/settings_migration_test.cpp @@ -705,6 +705,71 @@ TEST_F(SettingsMigrationTest, LegacyMigrationCamelCasesKeys) ASSERT_EQ(readFromIni("gamefilters.ini", "filter_games/gameType/deadbeef"), QVariant(true)); } +TEST_F(SettingsMigrationTest, LegacyMigrationMapsCreatorNameFilterToHostNameFilter) +{ + { + QSettings nativeSettings; + nativeSettings.setValue("filter_games/creator_name_filter", QStringList({"Alice", "Bob"})); + nativeSettings.sync(); + } + + ASSERT_TRUE(SettingsMigration::migrateLegacySettings(settingsPath)); + + ASSERT_TRUE(fileExists("gamefilters.ini")); + ASSERT_EQ(readFromIni("gamefilters.ini", "filter_games/hostNameFilter"), QVariant(QStringList({"Alice", "Bob"}))); +} + +TEST_F(SettingsMigrationTest, GameFiltersIniSnakeCaseMigratesToCamelCase) +{ + { + QSettings filtersIni(settingsPath + "gamefilters.ini", QSettings::IniFormat); + filtersIni.setValue("filter_games/hide_full_games", true); + filtersIni.setValue("filter_games/hide_games_that_started", true); + filtersIni.setValue("filter_games/game_name_filter", "test game"); + filtersIni.setValue("filter_games/creator_name_filter", QStringList({"Alice"})); + filtersIni.setValue("filter_games/game_type/deadbeef", true); + filtersIni.sync(); + } + + ASSERT_TRUE(SettingsMigration::migrateGameFiltersToCamelCase(settingsPath)); + + ASSERT_EQ(readFromIni("gamefilters.ini", "filter_games/hideFullGames"), QVariant(true)); + ASSERT_EQ(readFromIni("gamefilters.ini", "filter_games/hideGamesThatStarted"), QVariant(true)); + ASSERT_EQ(readFromIni("gamefilters.ini", "filter_games/gameNameFilter"), QVariant("test game")); + ASSERT_EQ(readFromIni("gamefilters.ini", "filter_games/hostNameFilter"), QVariant(QStringList({"Alice"}))); + ASSERT_EQ(readFromIni("gamefilters.ini", "filter_games/gameType/deadbeef"), QVariant(true)); + + // Idempotent: a second run has nothing left to do + ASSERT_FALSE(SettingsMigration::migrateGameFiltersToCamelCase(settingsPath)); +} + +TEST_F(SettingsMigrationTest, GameFiltersIniOrphanCreatorNameFilterMigratesToHostName) +{ + { + QSettings filtersIni(settingsPath + "gamefilters.ini", QSettings::IniFormat); + filtersIni.setValue("filter_games/creatorNameFilter", QStringList({"Alice"})); + filtersIni.sync(); + } + + ASSERT_TRUE(SettingsMigration::migrateGameFiltersToCamelCase(settingsPath)); + ASSERT_EQ(readFromIni("gamefilters.ini", "filter_games/hostNameFilter"), QVariant(QStringList({"Alice"}))); + ASSERT_FALSE(SettingsMigration::migrateGameFiltersToCamelCase(settingsPath)); +} + +TEST_F(SettingsMigrationTest, GameFiltersIniKeepsNewerCamelCaseValues) +{ + { + QSettings filtersIni(settingsPath + "gamefilters.ini", QSettings::IniFormat); + filtersIni.setValue("filter_games/hide_full_games", true); + filtersIni.setValue("filter_games/hideFullGames", false); + filtersIni.sync(); + } + + // Nothing to do: the current-format key already exists and must win + ASSERT_FALSE(SettingsMigration::migrateGameFiltersToCamelCase(settingsPath)); + ASSERT_EQ(readFromIni("gamefilters.ini", "filter_games/hideFullGames"), QVariant(false)); +} + TEST_F(SettingsMigrationTest, LegacyMigrationEmptyNativeFormatWritesSentinel) { QSettings().clear();