diff --git a/.ci/Debian12/Dockerfile b/.ci/Debian12/Dockerfile index 14c6f018c..fc756aac2 100644 --- a/.ci/Debian12/Dockerfile +++ b/.ci/Debian12/Dockerfile @@ -4,6 +4,7 @@ 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 b597307c5..bdecb56df 100644 --- a/.ci/Debian13/Dockerfile +++ b/.ci/Debian13/Dockerfile @@ -5,6 +5,7 @@ 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 f1a16843a..321aa7c0f 100644 --- a/.ci/Servatrice_Debian12/Dockerfile +++ b/.ci/Servatrice_Debian12/Dockerfile @@ -4,6 +4,7 @@ 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 b9fb2b701..715997474 100644 --- a/.ci/Ubuntu24.04/Dockerfile +++ b/.ci/Ubuntu24.04/Dockerfile @@ -4,6 +4,7 @@ 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 cc6528858..96dd10763 100644 --- a/.ci/Ubuntu26.04/Dockerfile +++ b/.ci/Ubuntu26.04/Dockerfile @@ -5,10 +5,10 @@ RUN apt-get update && \ build-essential \ ca-certificates \ ccache \ + clang-format \ cmake \ file \ g++ \ - gcovr \ git \ libgl-dev \ liblzma-dev \ diff --git a/.ci/compile.sh b/.ci/compile.sh index 6789b9817..823a31898 100755 --- a/.ci/compile.sh +++ b/.ci/compile.sh @@ -8,18 +8,14 @@ # --suffix renames package with this suffix, requires arg # --server compiles servatrice # --test runs tests -# --sanitize builds with AddressSanitizer and UndefinedBehaviorSanitizer, gcc/clang only -# --coverage instruments the test run for line coverage and prints a gcovr summary, gcc only - # --debug or --release sets the build type ie CMAKE_BUILD_TYPE # --ccache [] uses ccache and shows stats, optionally provide size # --evict-ccache runs ccache eviction based on given age after build # --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 -# --profile writes an MSBuild performance summary and binary log, Windows only -# 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 BUILD_PARALLEL_LEVEL BUILD_PROFILE -# (correspond to args: --debug/--release --install --package --suffix --server --test --sanitize --coverage --ccache --dir --profile) +# 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 +# (correspond to args: --debug/--release --install --package --suffix --server --test --ccache --dir ) # exitcode: 1 for failure, 3 for invalid arguments # Read arguments @@ -61,14 +57,6 @@ while [[ $# != 0 ]]; do MAKE_TEST=1 shift ;; - '--sanitize') - MAKE_SANITIZE=1 - shift - ;; - '--coverage') - MAKE_COVERAGE=1 - shift - ;; '--debug') BUILDTYPE="Debug" shift @@ -116,10 +104,6 @@ while [[ $# != 0 ]]; do export CMAKE_GENERATOR=$1 shift ;; - '--profile') - BUILD_PROFILE=1 - shift - ;; '--target-macos-version') shift if [[ $# == 0 ]]; then @@ -147,15 +131,6 @@ if [[ ! $BUILD_DIR ]]; then BUILD_DIR="build" fi mkdir -p "$BUILD_DIR" - -if [[ $MAKE_COVERAGE ]]; then - # Resolved while the working directory is still the source root. $0 is relative to the - # directory the script was invoked from, so after the cd below it no longer resolves, and - # under `set -e` the failed subshell would abort the build rather than just yielding an empty - # variable. Only the coverage report needs this, so only coverage pays for it. - SRC_ROOT="$(cd "$(dirname "$0")/.." && pwd)" -fi - cd "$BUILD_DIR" # Set minimum CMake Version @@ -172,17 +147,6 @@ fi if [[ $MAKE_TEST ]]; then flags+=("-DTEST=1") fi -if [[ $MAKE_SANITIZE ]]; then - flags+=("-DENABLE_SANITIZERS=ON") -else - # The release-package step re-runs cmake in the same build dir, so an - # ENABLE_SANITIZERS=ON left over from the debug step would otherwise leak - # into the shipped binaries (libasan/libubsan Depends in the DEB). - flags+=("-DENABLE_SANITIZERS=OFF") -fi -if [[ $MAKE_COVERAGE ]]; then - flags+=("-DENABLE_COVERAGE=ON") -fi if [[ $USE_CCACHE ]]; then flags+=("-DUSE_CCACHE=1") # PCH-aware caching is required or ccache refuses to cache any TU that @@ -214,22 +178,6 @@ function ccachestatsverbose() { fi } -function cpuinfo() { - # The number of cores decides how much build parallelism is useful; the hosted - # runners are not all sized alike, so read it off the machine instead of assuming. - case "$RUNNER_OS" in - Windows) - echo "cores: ${NUMBER_OF_PROCESSORS:-unknown}" - ;; - macOS) - echo "cores: $(sysctl -n hw.ncpu)" - ;; - *) - echo "cores: $(nproc 2>/dev/null || echo unknown)" - ;; - esac -} - # Compile if [[ $RUNNER_OS == macOS ]]; then # QTDIR is needed for macOS since we actually only use the cached thin Qt binaries instead of the install-qt-action, @@ -264,14 +212,8 @@ if [[ $RUNNER_OS == macOS ]]; then triplet_source="../vcpkg/triplets/community/$arch-osx.cmake" fi cp "$triplet_source" "$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" + echo "set(VCPKG_CMAKE_SYSTEM_VERSION $TARGET_MACOS_VERSION)" >>"$triplet_file" + echo "set(VCPKG_OSX_DEPLOYMENT_TARGET $TARGET_MACOS_VERSION)" >>"$triplet_file" flags+=("-DVCPKG_OVERLAY_TRIPLETS=$triplets_dir") flags+=("-DVCPKG_HOST_TRIPLET=$triplet_version") flags+=("-DVCPKG_TARGET_TRIPLET=$triplet_version") @@ -314,34 +256,9 @@ if [[ $RUNNER_OS == macOS ]]; then fi elif [[ $RUNNER_OS == Windows ]]; then - if [[ $CMAKE_GENERATOR == "Visual Studio"* ]]; then - # cmake only hands MSBuild a job count when one is asked for, and without it - # MSBuild builds a single node: one project at a time, whatever MTT does inside - # it. Ask for a node per core. - if [[ ! $BUILD_PARALLEL_LEVEL ]]; then - BUILD_PARALLEL_LEVEL="${NUMBER_OF_PROCESSORS:-1}" - fi - buildflags+=(--parallel "$BUILD_PARALLEL_LEVEL") - - # Enable MTT, see https://devblogs.microsoft.com/cppblog/improved-parallelism-in-msbuild/ - # and https://devblogs.microsoft.com/cppblog/cpp-build-throughput-investigation-and-tune-up/#multitooltask-mtt - # EnforceProcessCountAcrossBuilds makes the CL_MPCount ceiling apply across all - # the projects the nodes are building, instead of each node running its own - # cl.exe processes and /m and MTT multiplying into an oversubscribed machine. - buildflags+=(-- -p:UseMultiToolTask=true -p:EnableClServerMode=true -p:EnforceProcessCountAcrossBuilds=true -p:CL_MPCount="$BUILD_PARALLEL_LEVEL") - if [[ $BUILD_PROFILE ]]; then - # A performance summary says which projects and tasks cost what, the binary log - # keeps the full timeline; both are far too noisy to always produce. The files - # land next to the solution, i.e. in the build dir the workflow uploads. These - # join the MTT options above, which already carried the "--" separator. - buildflags+=("-flp:PerformanceSummary;v=q;LogFile=msbuild-perf.log" -bl:msbuild.binlog) - fi - else - # Ninja runs the compiler itself, so it neither takes the MSBuild switches - # (it rejects -p: options outright) nor a job count: it sizes its own pool - # for the cores it finds. - echo "ninja $(ninja --version)" - fi + # Enable MTT, see https://devblogs.microsoft.com/cppblog/improved-parallelism-in-msbuild/ + # and https://devblogs.microsoft.com/cppblog/cpp-build-throughput-investigation-and-tune-up/#multitooltask-mtt + buildflags+=(-- -p:UseMultiToolTask=true -p:EnableClServerMode=true) fi if [[ $USE_CCACHE ]]; then @@ -357,19 +274,10 @@ cmake .. "${flags[@]}" echo "::endgroup::" echo "::group::Build project" -cpuinfo echo "Running cmake --build with flags: ${buildflags[*]}" cmake --build . "${buildflags[@]}" echo "::endgroup::" -if [[ $BUILD_PROFILE && $RUNNER_OS == Windows && -f msbuild-perf.log ]]; then - # Repeat the summary in the job log so the numbers are readable without - # downloading the binary log, which is hundreds of megabytes. - echo "::group::MSBuild performance summary" - cat msbuild-perf.log - echo "::endgroup::" -fi - if [[ $USE_CCACHE ]]; then if [[ $CCACHE_EVICTION_AGE ]]; then echo "::group::evict ccache files older than $CCACHE_EVICTION_AGE" @@ -398,50 +306,7 @@ fi if [[ $MAKE_TEST ]]; then echo "::group::Run tests" - if [[ $MAKE_SANITIZE ]]; then - # detect_leaks=0: Qt and the sanitizers' own runtimes both leak at exit by design, and - # LeakSanitizer's default is aggressive enough to bury our real findings under hundreds - # of process-teardown reports. Malloc/free misuse and use-after-free are what ASan is for - # and they stay fully on. - # abort_on_error=1: a bad access leaves the process in an unusable state, so continuing - # only produces a second, more confusing report from the wreckage. - # UBSAN halt_on_error=1: without it UBSan prints "runtime error: ..." but the - # process exits 0 (recover mode), so gtest/ctest report a pass and the job - # stays green. Combined with -fno-sanitize-recover=undefined at compile time, - # the first finding aborts the test process with a non-zero exit. - export ASAN_OPTIONS="detect_leaks=0:abort_on_error=1:print_stacktrace=1:symbolize=1" - export UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:symbolize=1" - fi - # Every TEST() is its own CTest entry, so the suite is process-spawn bound rather than CPU - # bound and -j recovers nearly all of the per-process overhead. The test binaries are safe to - # run concurrently: the only one that claims a fixed resource is single_instance_manager_test, - # which already scopes its local socket name to its own pid so parallel copies cannot collide. - ctest -C "$BUILDTYPE" --output-on-failure -j "${CTEST_JOBS:-4}" - echo "::endgroup::" -fi - -if [[ $MAKE_COVERAGE ]]; then - # gcovr is not a build dependency and is not available everywhere, so a missing one is reported - # and skipped rather than failing an otherwise green build. - echo "::group::Coverage report" - GCOVR="$(command -v gcovr || true)" - if [[ -z $GCOVR ]]; then - echo "gcovr not found; install it to produce a coverage report" - else - "$GCOVR" --version - # This script configures with `cmake ..`, so the build dir it was started from is the object - # directory and the source root is one level up. Both are passed explicitly: gcovr otherwise - # walks the current directory and trips over in-tree build directories left by other tooling, - # whose stale .gcno files cannot be resolved. --config is named too, so the filters apply no - # matter what directory gcovr happens to start from. - GCOVR_ARGS=("." --root "$SRC_ROOT" --object-directory "." --config "$SRC_ROOT/gcovr.cfg") - "$GCOVR" "${GCOVR_ARGS[@]}" --txt --decisions - "$GCOVR" "${GCOVR_ARGS[@]}" --html-details coverage-details.html --html-title "Cockatrice line coverage" - "$GCOVR" "${GCOVR_ARGS[@]}" --lcov -o coverage.lcov - "$GCOVR" "${GCOVR_ARGS[@]}" --cobertura-pretty -o coverage.xml - "$GCOVR" "${GCOVR_ARGS[@]}" --json-summary-pretty -o coverage-summary.json - echo "Coverage artifacts written to coverage-details.html, coverage.lcov, coverage.xml and coverage-summary.json" - fi + ctest -C "$BUILDTYPE" --output-on-failure echo "::endgroup::" fi diff --git a/.ci/docker.sh b/.ci/docker.sh index 412ad6a1d..46112daaa 100644 --- a/.ci/docker.sh +++ b/.ci/docker.sh @@ -13,17 +13,11 @@ # --set-cache sets the location to cache the image or for ccache # # requires: docker -# uses env: NAME IMAGE_SUFFIX CACHE BUILD GET SAVE INTERACTIVE +# uses env: NAME CACHE BUILD GET SAVE INTERACTIVE # (correspond to args: --set-cache --build --get --save --interactive) # sets env: RUN CCACHE_DIR IMAGE_NAME RUN_ARGS RUN_OPTS BUILD_SCRIPT # exitcode: 1 for failure, 2 for missing dockerfile, 3 for invalid arguments # -# IMAGE_SUFFIX is optional and only appended to the image name, never to NAME, so that a matrix -# leg can have its own image and image cache without needing its own directory under .ci. Two legs -# building and tagging the same image concurrently would race on `docker build --tag`. -# It is deliberately not called SUFFIX: compile.sh already uses SUFFIX for output file names, and -# a job-level SUFFIX would be silently overridden by those per-step env blocks. -# # exported RUN function will run the BUILD_SCRIPT inside of the docker container. # note that the docker container will not inherit any environment variables! # @@ -87,7 +81,7 @@ if ! [[ $NAME ]]; then return 3 fi -export IMAGE_NAME="${project_name,,}_${NAME,,}${IMAGE_SUFFIX:+_${IMAGE_SUFFIX,,}}" # lower case +export IMAGE_NAME="${project_name,,}_${NAME,,}" # lower case docker_dir=".ci/$NAME" if ! [[ -r $docker_dir/Dockerfile ]]; then diff --git a/.github/workflows/desktop-build.yml b/.github/workflows/desktop-build.yml index beedaeaf2..17c42b07d 100644 --- a/.github/workflows/desktop-build.yml +++ b/.github/workflows/desktop-build.yml @@ -144,36 +144,17 @@ jobs: package: DEB - # The one leg that runs the suite under AddressSanitizer and - # UndefinedBehaviorSanitizer. Debian 13 and Fedora 44 keep covering the - # uninstrumented suite, so nothing is lost by instrumenting this one. - sanitize: yes - - - distro: Ubuntu - version: 26.04 - variant: cover # separate image and ccache, same distro - - # The one leg that runs instrumented for line coverage and publishes the report as - # a workflow artifact. It is a separate leg rather than a second flag on the - # sanitizer leg because the two instruments answer different questions: the - # sanitizer leg reports what the run did wrong, the coverage leg reports what the - # run never touched at all. Combining them would also slow both down. Packaging is - # skipped because the leg above already publishes the DEB for this distro. - coverage: yes - package: skip - - name: ${{ matrix.distro }} ${{ matrix.version }}${{ matrix.variant && format(' {0}', matrix.variant) || '' }} + name: ${{ matrix.distro }} ${{ matrix.version }} needs: configure runs-on: ubuntu-latest # https://github.com/actions/runner-images continue-on-error: ${{ matrix.allow-failure == 'yes' }} timeout-minutes: 70 env: - CACHE: ${{ github.workspace }}/.cache/${{ matrix.distro }}${{ matrix.version }}${{ matrix.variant }} # directory for caching docker image and ccache + CACHE: ${{ github.workspace }}/.cache/${{ matrix.distro }}${{ matrix.version }} # directory for caching docker image and ccache CCACHE_EVICTION_AGE: 7d CCACHE_SIZE: 600M # space of all repo is 10Gi: https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows#usage-limits-and-eviction-policy CMAKE_GENERATOR: 'Ninja' NAME: ${{ matrix.distro }}${{ matrix.version }} - IMAGE_SUFFIX: ${{ matrix.variant }} steps: - name: "Checkout" @@ -183,8 +164,8 @@ jobs: id: restore_ccache uses: actions/cache/restore@v6 with: - key: ccache-${{ matrix.distro }}${{ matrix.version }}${{ matrix.variant }} - path: ${{ env.CACHE }}/.ccache + key: ccache-${{ matrix.distro }}${{ matrix.version }} + path: ${{ env.CACHE }} - name: "Build ${{ matrix.distro }} ${{ matrix.version }} Docker image" shell: bash @@ -193,9 +174,6 @@ jobs: - name: "Build debug and test" if: matrix.test != 'skip' shell: bash - env: - sanitize: '${{ matrix.sanitize }}' - coverage: '${{ matrix.coverage }}' run: | source .ci/docker.sh args=() @@ -203,33 +181,7 @@ jobs: args+=(--ccache "$CCACHE_SIZE") args+=(--cmake-generator "$CMAKE_GENERATOR") - extra=() - [[ $sanitize == yes ]] && extra+=(--sanitize) - [[ $coverage == yes ]] && extra+=(--coverage) - - RUN --server --debug --test "${extra[@]}" "${args[@]}" - - # RUN bind-mounts the workspace into the container, so the report the build wrote into - # build/ is already on the host and no second container is needed to collect it. - - name: "Collect coverage report" - if: matrix.coverage == 'yes' - shell: bash - run: | - mkdir -p coverage - # --html-details writes one page per source file plus a stylesheet and script, and the - # index links to all of them by relative name. Copying the index alone would publish a - # report where every link 404s, so the whole family has to travel together. - cp build/coverage-details* coverage/ - cp build/coverage.lcov build/coverage.xml build/coverage-summary.json coverage/ - ls -la coverage/ - - - name: "Upload coverage artifact" - if: matrix.coverage == 'yes' - uses: actions/upload-artifact@v4 - with: - name: coverage-report - path: coverage/ - if-no-files-found: warn + RUN --server --debug --test "${args[@]}" - name: "Build release package" id: build @@ -267,7 +219,7 @@ jobs: uses: actions/cache/save@v6 with: key: ${{ steps.restore_ccache.outputs.cache-primary-key }} - path: ${{ env.CACHE }}/.ccache + path: ${{ env.CACHE }} - name: "Upload artifact" id: upload_artifact @@ -325,8 +277,6 @@ 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 @@ -343,9 +293,6 @@ 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 @@ -362,8 +309,6 @@ jobs: soc: Apple type: Release use_ccache: 1 - vcpkg_deployment_target: 14 - vcpkg_scope: 'vcpkg/macos14-arm64-xcode26.6/' xcode: "26.6" - os: macOS @@ -377,30 +322,19 @@ jobs: soc: Apple type: Debug use_ccache: 1 - vcpkg_deployment_target: 14 - vcpkg_scope: 'vcpkg/macos14-arm64-xcode26.6/' xcode: "26.6" - os: Windows target: 10 runner: windows-2025 # https://github.com/actions/runner-images/blob/main/images/windows/Windows2025-VS2026-Readme.md - # Ninja builds the whole dependency graph at once instead of one - # MSBuild node at a time, runs the compiler itself, and matches the - # Linux and macOS legs. Its median build phase measured ~17 min - # against ~21 for the Visual Studio generator (shared 4-core hosts - # swing either by minutes, so read medians, not single runs). - cmake_generator: Ninja + cmake_generator: "Visual Studio 18 2026" + cmake_generator_platform: x64 make_package: 1 package_suffix: "-Win10" qt_version: 6.11.* qt_modules: qtimageformats qtmultimedia qtshadertools qtwebsockets type: Release - # Scope is named after the vs2026 toolchain, not the generator: the - # ABI hash covers cl.exe, which is identical, so this leg keeps - # restoring the warm archives. Renaming would orphan them. - 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 @@ -494,39 +428,12 @@ jobs: shell: bash run: choco install nsis - # Ninja invokes cl.exe directly and needs the MSVC environment on PATH. - - name: "[Windows] Set up MSVC environment" - if: matrix.os == 'Windows' - uses: TheMrMilchmann/setup-msvc-dev@368ef7d1ee4d1171b31d4a7f67f4d954f903f5a9 # v4.1.0 - with: - arch: x64 - - name: "Setup vcpkg cache" 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 @@ -535,6 +442,7 @@ jobs: BUILDTYPE: '${{ matrix.type }}' CCACHE_EVICTION_AGE: ${{ matrix.ccache_eviction_age }} CMAKE_GENERATOR: ${{ matrix.cmake_generator }} + CMAKE_GENERATOR_PLATFORM: ${{ matrix.cmake_generator_platform }} DEVELOPER_DIR: '/Applications/Xcode_${{ matrix.xcode }}.app/Contents/Developer' GITHUB_TOKEN: ${{ github.token }} # needed for vcpkg dependency graph updates, see VCPKG_FEATURE_FLAGS MACOS_CERTIFICATE: ${{ secrets.PROD_MACOS_CERTIFICATE }} @@ -545,19 +453,9 @@ 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" @@ -571,63 +469,6 @@ 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 @@ -703,8 +544,6 @@ jobs: if-no-files-found: error path: ${{ steps.build.outputs.path }} - # Ninja is single-config: binaries and their PDBs land directly in the - # target directories instead of a Release subdirectory. - name: "[Windows] Upload PDBs (Program Databases)" if: matrix.os == 'Windows' && github.ref_type != 'tag' uses: actions/upload-artifact@v7 @@ -712,9 +551,9 @@ jobs: if-no-files-found: error name: ${{ steps.build.outputs.name }}-PDBs path: | - build/cockatrice/*.pdb - build/oracle/*.pdb - build/servatrice/*.pdb + build/cockatrice/Release/*.pdb + build/oracle/Release/*.pdb + build/servatrice/Release/*.pdb - name: "Upload to release" if: needs.configure.outputs.tag != null && matrix.make_package == '1' @@ -742,67 +581,3 @@ 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 deleted file mode 100644 index 3cc7b2203..000000000 --- a/.github/workflows/vcpkg-cache-import.yml +++ /dev/null @@ -1,121 +0,0 @@ -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 bee47972c..f51c636eb 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -22,10 +22,6 @@ option(WITH_ORACLE "Build Cockatrice card database tool (Oracle)" ON) option(WITH_SERVER "Build Cockatrice server (Servatrice)" OFF) # Compile tests option(TEST "Build tests" OFF) -# Build with AddressSanitizer and UndefinedBehaviorSanitizer -option(ENABLE_SANITIZERS "Build with AddressSanitizer and UndefinedBehaviorSanitizer" OFF) -# Build instrumented for GCC line coverage, reported with gcovr -option(ENABLE_COVERAGE "Build instrumented for GCC line coverage" OFF) # Use vcpkg regardless of OS option(USE_VCPKG "Use vcpkg regardless of OS" OFF) @@ -90,7 +86,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.2) +project("Cockatrice" VERSION 3.1.1) # Set release name if not provided via env/cmake var if(NOT DEFINED GIT_TAG_RELEASENAME) @@ -215,12 +211,6 @@ else() set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -pipe") endif() -# Always included, because they define cockatrice_disable_sanitizers_for() and -# cockatrice_disable_coverage_for() for the vendored trees. They only touch the global flags -# when ENABLE_SANITIZERS or ENABLE_COVERAGE is on. -include(Sanitizers) -include(Coverage) - # GNU systems need to define the Mersenne exponent for the RNG to compile w/o warning if(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang") add_definitions("-DSFMT_MEXP=19937") diff --git a/cmake/Coverage.cmake b/cmake/Coverage.cmake deleted file mode 100644 index 0345a2d36..000000000 --- a/cmake/Coverage.cmake +++ /dev/null @@ -1,75 +0,0 @@ -# Line-coverage instrumentation opt-in, plus per-source opt-out -# Input: ENABLE_COVERAGE -# Defines: cockatrice_disable_coverage_for -# -# Included once from the root CMakeLists.txt, immediately after the per-compiler flag -# block, so add_compile_options() reaches every library and executable defined below. -# -# Note the name deliberately shadows CMake's own built-in Modules/Coverage.cmake. It resolves -# to this file only because the root CMakeLists.txt puts cmake/ at the front of -# CMAKE_MODULE_PATH; if that ever moves, include(Coverage) would silently pick up the built-in -# module, which only defines a `COVERAGE` custom target and touches no global flags, so -# instrumentation would quietly stop happening rather than erroring. - -include(CheckCXXSourceCompiles) - -# Opt a set of vendored sources out of instrumentation. -# -# cockatrice_disable_coverage_for( ...) -# -# Third-party code drags the total down with lines no test can ever reach, and a total that -# cannot move is not a useful signal. This is the same carve-out the sanitizer build uses. -# -# The sources must be named relative to the directory that compiles them. CMake resolves a -# source-file property against the directory scope the target was declared in, so calling -# this from the root with absolute paths silently does nothing. That is why the call sites -# live next to the add_library/add_executable that pull the vendored code in. -# -# SKIP_PRECOMPILE_HEADERS mirrors the sanitizer helper. The precompiled header carries the -# instrumentation flags, and mixing an instrumented PCH with an uninstrumented translation -# unit makes GCC reject it. -function(cockatrice_disable_coverage_for) - if(NOT ENABLE_COVERAGE) - return() - endif() - # There is no -fno-coverage: --coverage is shorthand for -fprofile-arcs -ftest-coverage, so both - # halves have to be negated. CC rejects "-fno-coverage" outright, which is how the vendored C - # files below hit it first. - set_source_files_properties( - ${ARGN} PROPERTIES COMPILE_OPTIONS "-fno-profile-arcs;-fno-test-coverage" SKIP_PRECOMPILE_HEADERS ON - ) -endfunction() - -if(ENABLE_COVERAGE) - # Line coverage needs the compiler's own toolchain, so only GCC is supported. Clang's - # equivalent is llvm-cov, which reads a different intermediate format and would need a - # separate report path; rejecting it is better than emitting .gcno files that gcovr cannot - # read. - if(NOT CMAKE_CXX_COMPILER_ID STREQUAL "GNU") - message(FATAL_ERROR "ENABLE_COVERAGE requires GCC, but the compiler is ${CMAKE_CXX_COMPILER_ID}. " - "Clang coverage needs llvm-cov and is not wired up." - ) - endif() - - # Probe the way the flag will actually be used: compile AND link. A .gcno file comes from the - # compile step, but the runtime that writes .gcda files comes from the link step, so probing - # only the compile step would pass on a toolchain that cannot actually produce coverage. - set(_cockatrice_saved_required_flags "${CMAKE_REQUIRED_FLAGS}") - set(_cockatrice_saved_required_link_options "${CMAKE_REQUIRED_LINK_OPTIONS}") - set(CMAKE_REQUIRED_FLAGS "-O0 -g") - set(CMAKE_REQUIRED_LINK_OPTIONS -fprofile-arcs -ftest-coverage) - check_cxx_source_compiles("int main() { return 0; }" COCKATRICE_HAVE_COVERAGE_FLAGS) - set(CMAKE_REQUIRED_FLAGS "${_cockatrice_saved_required_flags}") - set(CMAKE_REQUIRED_LINK_OPTIONS "${_cockatrice_saved_required_link_options}") - - if(NOT COCKATRICE_HAVE_COVERAGE_FLAGS) - message(FATAL_ERROR "ENABLE_COVERAGE is on, but this toolchain rejected the coverage flags.") - endif() - - # -O0 keeps the line mapping honest: at higher optimisation levels the compiler merges and - # relocates lines, and the report describes code that no longer exists in that form. - add_compile_options(--coverage -O0 -g) - add_link_options(--coverage) - - message(STATUS "Coverage instrumentation enabled (--coverage). Report with gcovr over the build dir.") -endif() diff --git a/cmake/FindQtRuntime.cmake b/cmake/FindQtRuntime.cmake index 2d74aca4d..ad52352de 100644 --- a/cmake/FindQtRuntime.cmake +++ b/cmake/FindQtRuntime.cmake @@ -40,9 +40,7 @@ endif() if(TEST) # Union of Qt modules required across all test targets (independent of application targets). # When adding a new test that needs additional Qt modules, add them here rather than in the test's CMakeLists.txt. - # Test is QtTest: it brings QAbstractItemModelTester and QSignalSpy, which are how the Qt model - # and signal contracts get verified without standing up a GUI. - set(_TEST_NEEDED Concurrent Network Svg Widgets Test) + set(_TEST_NEEDED Concurrent Network Svg Widgets) endif() set(REQUIRED_QT_COMPONENTS ${REQUIRED_QT_COMPONENTS} ${_SERVATRICE_NEEDED} ${_COCKATRICE_NEEDED} ${_ORACLE_NEEDED} diff --git a/cmake/NSIS.template.in b/cmake/NSIS.template.in index b6125238f..4c55c00b3 100644 --- a/cmake/NSIS.template.in +++ b/cmake/NSIS.template.in @@ -304,11 +304,10 @@ SetShellVarContext all ; _?=$INSTDIR keeps the old uninstaller running in place. An NSIS uninstaller ; normally copies itself to %TEMP% and runs from there so that it can delete its ; own directory, and the process we started exits at once. ExecWait then returns -; before the uninstall has run, and the temp copy keeps deleting files - from -; 3.1.0 and 3.1.1 on disk a blanket "RMDir /r $INSTDIR" - while the section -; below is still writing the new payload. _?= disables the copy and makes -; ExecWait block until the uninstall finishes. It must be the last argument -; and must not be quoted, even when the path contains spaces. +; before the uninstall has run, and the temp copy's "RMDir /r $INSTDIR" executes +; while the section below is still writing the new payload. _?= disables the copy +; and makes ExecWait block until the uninstall finishes. It must be the last +; argument and must not be quoted, even when the path contains spaces. ; ; --- 32-bit uninstall --- SetRegView 32 @@ -1108,14 +1107,9 @@ ${AndIf} ${FileExists} "$INSTDIR\portable.dat" ; upgrade portable mode !insertmacro CockLog "section: portable upgrade purge" !insertmacro CockLogListing "before-portable-purge" "*.dll" - ; plugins and translations are payload and are rewritten by the payload - ; below, so removing them here is safe. sounds and themes are payload - ; too - the default packs are rewritten below as well - but a portable - ; user's custom sounds and themes live in exactly those folders, so a - ; recursive purge here would delete user content along with the defaults. - ; Stale default files from an older release are overwritten or replaced - ; by the payload, never by a purge. RMDir /r "$INSTDIR\plugins" + RMDir /r "$INSTDIR\sounds" + RMDir /r "$INSTDIR\themes" RMDir /r "$INSTDIR\translations" Delete "$INSTDIR\uninstall.exe" Delete "$INSTDIR\cockatrice.exe" @@ -1269,32 +1263,16 @@ Section "un.Application" UnSecApplication SetShellVarContext all StrCpy $R1 0 - ; Record whether this was a portable install before portable.dat is - ; removed below. + ; Record whether this was a portable install before the directory (and the + ; portable.dat marker inside it) is removed below. IfFileExists "$INSTDIR\portable.dat" 0 +2 StrCpy $R1 1 - ; Remove only what the installer wrote. The two lists below are generated - ; by CPack from the staged install tree: one Delete per installed file and - ; one non-recursive RMDir per installed directory, children first. - ; Anything the user put in $INSTDIR - decks, pictures, portable data - ; folders - is not on those lists and survives, and $INSTDIR itself is - ; only removed when nothing else is left in it. An earlier version of this - ; section ran a blanket "RMDir /r $INSTDIR" to keep build-tree artifacts - ; out of a fresh reinstall; that also deleted user decks whenever a user - ; had pointed their data folders into the install directory, so payload - ; purity may never be bought at that price again. -@CPACK_NSIS_DELETE_FILES@ -@CPACK_NSIS_DELETE_DIRECTORIES@ - - ; Written by the installer itself, so absent from the staged payload - ; above; qdebug.txt is app-generated log output next to the executables. - Delete "$INSTDIR\uninstall.exe" - Delete "$INSTDIR\portable.dat" - Delete "$INSTDIR\qdebug.txt" - - ; Succeeds only when no foreign files remain. - RMDir "$INSTDIR" + ; Remove the entire application directory so any file that is not part of + ; the installed payload (e.g. build-tree artifacts such as *.dir folders, + ; *_autogen and *.tlog files from a build) cannot survive between an + ; uninstall and a fresh reinstall. + RMDir /r "$INSTDIR" RMDir /r "$SMPROGRAMS\Cockatrice" diff --git a/cmake/Sanitizers.cmake b/cmake/Sanitizers.cmake deleted file mode 100644 index 2e5d794de..000000000 --- a/cmake/Sanitizers.cmake +++ /dev/null @@ -1,109 +0,0 @@ -# AddressSanitizer + UndefinedBehaviorSanitizer opt-in, plus per-source opt-out -# Input: ENABLE_SANITIZERS -# Defines: cockatrice_disable_sanitizers_for -# -# Included once from the root CMakeLists.txt, immediately after the per-compiler flag -# block, so add_compile_options() reaches every library and executable defined below. - -include(CheckCXXSourceCompiles) - -# Opt a set of vendored sources out of instrumentation. -# -# cockatrice_disable_sanitizers_for( ...) -# -# Sanitizer findings inside vendored code are not actionable for us, and letting them -# through buries our own bugs in noise until everyone learns to skip the job. -# -# The sources must be named relative to the directory that compiles them. CMake resolves a -# source-file property against the directory scope the target was declared in, so calling -# this from the root with absolute paths silently does nothing. That is why the call sites -# live next to the add_library/add_executable that pull the vendored code in. -# -# SKIP_PRECOMPILE_HEADERS is not optional. The precompiled header is built with the -# sanitizer flags, so it defines __SANITIZE_ADDRESS__; a translation unit compiled with -# -fno-sanitize=all does not, and GCC then refuses the PCH with -# "not used because __SANITIZE_ADDRESS__ not defined", which is a fatal -Werror=invalid-pch -# in the servatrice and oracle builds. -function(cockatrice_disable_sanitizers_for) - if(NOT ENABLE_SANITIZERS) - return() - endif() - set_source_files_properties(${ARGN} PROPERTIES COMPILE_OPTIONS "-fno-sanitize=all" SKIP_PRECOMPILE_HEADERS ON) -endfunction() - -if(ENABLE_SANITIZERS) - # Probe a flag the way it will actually be used: on the compile step AND the link step. - # check_cxx_compiler_flag() only probes the compile step, which makes every sanitizer - # flag look unsupported because linking an ASan object without -fsanitize=address leaves - # __asan_init undefined. That is a silent misconfiguration rather than an error, so the - # probe has to link. - function(cockatrice_probe_flag FLAG VARNAME) - set(CMAKE_REQUIRED_FLAGS "${FLAG}") - set(CMAKE_REQUIRED_LINK_OPTIONS "${FLAG}") - check_cxx_source_compiles("int main() { return 0; }" ${VARNAME}) - unset(CMAKE_REQUIRED_FLAGS) - unset(CMAKE_REQUIRED_LINK_OPTIONS) - endfunction() - - set(SANITIZER_FLAGS - -fsanitize=address,undefined - -fno-omit-frame-pointer - # UBSan defaults to recover mode: it prints "runtime error: ..." but the - # process exits 0, so gtest/ctest report a pass and CI stays green. - # -fno-sanitize-recover makes the first finding abort instead. This is - # probed like the other cosmetic flags, but CI also sets - # UBSAN_OPTIONS=halt_on_error=1 as the runtime-side guarantee. - -fno-sanitize-recover=undefined - # Qt builds QObject subclasses into raw storage and writes the vptr afterwards, which - # the vptr check reports on essentially every QObject instantiation. The check has no - # value for us because nothing outside moc-generated code inherits QObject, so it is - # off outright rather than suppressed piecemeal. - -fno-sanitize=vptr - ) - - # MSVC has no ASan/UBSan equivalent, and clang-cl would need /fsanitize=address plus a - # matching runtime link, which is a separate piece of work. - if(MSVC) - message(FATAL_ERROR "ENABLE_SANITIZERS is not supported with MSVC") - endif() - - # Probe rather than assume, so an older toolchain gets the sanitizers it can support - # instead of failing the build on a flag it does not recognize. - set(SANITIZER_SUPPORTED_FLAGS) - foreach(FLAG ${SANITIZER_FLAGS}) - string(MAKE_C_IDENTIFIER "COCKATRICE_HAS${FLAG}" _FLAG_VAR) - cockatrice_probe_flag("${FLAG}" ${_FLAG_VAR}) - if(${_FLAG_VAR}) - list(APPEND SANITIZER_SUPPORTED_FLAGS ${FLAG}) - else() - message(STATUS "Sanitizers: ${FLAG} unsupported by ${CMAKE_CXX_COMPILER_ID}, skipping") - endif() - endforeach() - - # The whole point of the option is the sanitizers, so an unsupported pair is fatal even - # though the cosmetic flags above are allowed to degrade. - if(NOT "-fsanitize=address,undefined" IN_LIST SANITIZER_SUPPORTED_FLAGS) - message(FATAL_ERROR "ENABLE_SANITIZERS requires -fsanitize=address,undefined, which " - "${CMAKE_CXX_COMPILER_ID} does not accept" - ) - endif() - - add_compile_options(${SANITIZER_SUPPORTED_FLAGS}) - add_link_options(-fsanitize=address,undefined) - - # Recorded so the test tree can label its entries and a plain `make` at the same time - # cannot quietly disagree with what CI ran. - set(COCKATRICE_SANITIZERS - ON - CACHE INTERNAL "AddressSanitizer and UBSan are enabled" - ) -else() - # ENABLE_SANITIZERS is OFF, but COCKATRICE_SANITIZERS is CACHE INTERNAL, so a - # reused build dir (e.g. CI's release-package step reconfiguring the same dir - # after the sanitized debug step) would otherwise keep skipping NO_SANITIZE - # tests and keep the 300s discovery timeout. Reset it explicitly. - set(COCKATRICE_SANITIZERS - OFF - CACHE INTERNAL "AddressSanitizer and UBSan are enabled" FORCE - ) -endif() diff --git a/cmake/pch/qtcore_pch.h b/cmake/pch/qtcore_pch.h index 176f2939c..cc3dd12ee 100644 --- a/cmake/pch/qtcore_pch.h +++ b/cmake/pch/qtcore_pch.h @@ -3,53 +3,22 @@ * * Safe for every target that links Qt Core, including the headless * Servatrice binary. Keep this header free of any widget/gui types. - * - * Candidates are chosen from IWYU include statistics: a header earns a - * spot when it is not already reachable from the rest of this list and - * enough translation units re-parse it. Qt headers are stable across - * builds, so the precompiled header rarely invalidates. */ #include #include -#include -#include -#include #include #include -#include -#include #include -#include #include -#include -#include -#include -#include -#include #include #include #include -#include #include -#include -#include #include -#include -#include #include -#include -#include -#include -#include #include -#include #include #include -#include -#include #include -#include #include -#include -#include diff --git a/cmake/pch/qtwidgets_pch.h b/cmake/pch/qtwidgets_pch.h index 030bec69c..2c63f450e 100644 --- a/cmake/pch/qtwidgets_pch.h +++ b/cmake/pch/qtwidgets_pch.h @@ -4,56 +4,27 @@ * Includes the Qt Core precompiled header plus the heavy Gui, Widgets and * Network layers that virtually every client translation unit re-parses. * Do not use on Servatrice (headless, QT_DONT_USE_QTGUI). - * - * Candidates are chosen from IWYU include statistics: a header earns a - * spot when it is not already reachable from the rest of this list and - * enough translation units re-parse it. Qt headers are stable across - * builds, so the precompiled header rarely invalidates. */ #include "qtcore_pch.h" #include #include -#include -#include -#include -#include -#include -#include -#include #include #include #include -#include #include -#include -#include -#include -#include #include -#include -#include #include #include -#include #include #include -#include #include #include #include -#include #include #include -#include -#include -#include #include -#include -#include #include -#include #include -#include #include diff --git a/cockatrice/CMakeLists.txt b/cockatrice/CMakeLists.txt index 282f5a3c6..396ef7c34 100644 --- a/cockatrice/CMakeLists.txt +++ b/cockatrice/CMakeLists.txt @@ -59,7 +59,6 @@ set(cockatrice_SOURCES src/interface/widgets/dialogs/dlg_update.cpp src/interface/widgets/dialogs/dlg_view_log.cpp src/interface/widgets/dialogs/override_printing_warning.cpp - src/interface/widgets/dialogs/path_safety_warning.cpp src/interface/widgets/dialogs/settings_view.cpp src/interface/widgets/dialogs/tip_of_the_day.cpp src/interface/widgets/deck_share/deck_share_utils.cpp diff --git a/cockatrice/src/client/network/connection_controller/remote_connection_controller.cpp b/cockatrice/src/client/network/connection_controller/remote_connection_controller.cpp index 9b985073d..7480da27e 100644 --- a/cockatrice/src/client/network/connection_controller/remote_connection_controller.cpp +++ b/cockatrice/src/client/network/connection_controller/remote_connection_controller.cpp @@ -239,17 +239,6 @@ void ConnectionController::onLoginError(int r, break; } - case Response::RespTooManyRequests: { - QString bannedStr = endTime ? tr("Too many login attempts. Try again after %1.") - .arg(QDateTime::fromSecsSinceEpoch(endTime).toString()) - : tr("Too many login attempts. Please try again later."); - if (!reasonStr.isEmpty()) { - bannedStr.append("\n\n" + reasonStr); - } - QMessageBox::critical(dialogParent, tr("Error"), bannedStr); - break; - } - case Response::RespWrongPassword: { QMessageBox::critical(dialogParent, tr("Error"), tr("Incorrect username or password. " @@ -376,15 +365,12 @@ void ConnectionController::onRegisterError(int r, QString reasonStr, quint32 end break; } case Response::RespTooManyRequests: { - QString deniedStr = - endTime ? tr("Too many registration attempts. Try again after %1.") - .arg(QDateTime::fromSecsSinceEpoch(endTime).toString()) - : tr("It appears you are attempting to register a new account on this server yet you " - "already have an account registered with the email provided. This server " - "restricts the number of accounts a user can register per address. Please " - "contact the server operator for further assistance or to obtain your " - "credential information."); - QMessageBox::critical(dialogParent, tr("Registration denied"), deniedStr); + QMessageBox::critical(dialogParent, tr("Registration denied"), + tr("It appears you are attempting to register a new account on this server yet you " + "already have an account registered with the email provided. This server " + "restricts the number of accounts a user can register per address. Please " + "contact the server operator for further assistance or to obtain your " + "credential information.")); break; } case Response::RespPasswordTooShort: { 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 192029b23..ae5e92d6c 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,7 +10,6 @@ #include #include #include -#include #include #include #include @@ -178,15 +177,7 @@ bool SpoilerBackgroundUpdater::saveDownloadedFile(QByteArray data) // Data written, so reload the card database qCInfo(SpoilerBackgroundUpdaterLog) << "Spoiler Service Data Written"; - // 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(); })); + const auto reloadOk = 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 08eccf1e4..15a41e7e1 100644 --- a/cockatrice/src/client/settings/cache_settings.cpp +++ b/cockatrice/src/client/settings/cache_settings.cpp @@ -133,10 +133,6 @@ 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/game/game_event_handler.cpp b/cockatrice/src/game/game_event_handler.cpp index 7b08af47c..b86f661bb 100644 --- a/cockatrice/src/game/game_event_handler.cpp +++ b/cockatrice/src/game/game_event_handler.cpp @@ -277,6 +277,8 @@ void GameEventHandler::eventSpectatorLeave(const Event_Leave &event, emit spectatorLeft(eventPlayerId); game->getPlayerManager()->removeSpectator(eventPlayerId); + + emitUserEvent(); } void GameEventHandler::eventGameStateChanged(const Event_GameStateChanged &event, @@ -463,9 +465,9 @@ void GameEventHandler::eventJoin(const Event_Join &event, int /*eventPlayerId*/, PlayerLogic *newPlayer = game->getPlayerManager()->addPlayer(playerId, playerInfo.user_info()); emit logJoinPlayer(newPlayer); emit playerJoined(playerInfo); - - emitUserEvent(); } + + emitUserEvent(); } QString GameEventHandler::getLeaveReason(Event_Leave::LeaveReason reason) diff --git a/cockatrice/src/game_graphics/board/abstract_card_item.cpp b/cockatrice/src/game_graphics/board/abstract_card_item.cpp index cc121cfb2..a79c5873e 100644 --- a/cockatrice/src/game_graphics/board/abstract_card_item.cpp +++ b/cockatrice/src/game_graphics/board/abstract_card_item.cpp @@ -12,7 +12,6 @@ #include "libcockatrice/card/printing/exact_card.h" #include "libcockatrice/utility/card_ref.h" -#include #include #include #include @@ -29,7 +28,6 @@ #include #include #include -#include #include #include #include @@ -44,7 +42,7 @@ class QSizeF; AbstractCardItem::AbstractCardItem(QGraphicsItem *parent, const CardRef &cardRef, PlayerLogic *_owner, int _id) : ArrowTarget(_owner, parent), id(_id), cardRef(cardRef), tapped(false), facedown(false), tapAngle(0), - bgColor(Qt::transparent), isHovered(false), realZValue(0), pendingSelectionCollapse(false) + bgColor(Qt::transparent), isHovered(false), realZValue(0) { setCursor(Qt::OpenHandCursor); setFlag(ItemIsSelectable); @@ -346,16 +344,6 @@ void AbstractCardItem::setFaceDown(bool _facedown) void AbstractCardItem::mousePressEvent(QGraphicsSceneMouseEvent *event) { - cancelPendingSelectionCollapse(); - if (scene() != nullptr) { - // A new press starts a new gesture: no card may collapse the selection out from under it afterwards. - for (QGraphicsItem *selectedItem : scene()->selectedItems()) { - auto *selectedCard = qgraphicsitem_cast(selectedItem); - if (selectedCard != nullptr && selectedCard != this) { - selectedCard->cancelPendingSelectionCollapse(); - } - } - } if ((event->modifiers() & Qt::AltModifier) && event->button() == Qt::LeftButton) { emit cardShiftClicked(cardRef.name); } else if ((event->modifiers() & Qt::ControlModifier)) { @@ -363,10 +351,6 @@ void AbstractCardItem::mousePressEvent(QGraphicsSceneMouseEvent *event) } else if (!isSelected()) { scene()->clearSelection(); setSelected(true); - } else if (event->button() == Qt::LeftButton && scene()->selectedItems().count() > 1) { - // Part of a multi-selection; collapse only if this turns out to be a click rather than a drag, - // so that dragging the whole selection keeps it intact. - pendingSelectionCollapse = true; } if (event->button() == Qt::LeftButton) { setCursor(Qt::ClosedHandCursor); @@ -378,32 +362,6 @@ void AbstractCardItem::mousePressEvent(QGraphicsSceneMouseEvent *event) void AbstractCardItem::mouseReleaseEvent(QGraphicsSceneMouseEvent *event) { - if (pendingSelectionCollapse) { - pendingSelectionCollapse = false; - bool dragged = (event->screenPos() - event->buttonDownScreenPos(event->button())).manhattanLength() >= - 2 * QApplication::startDragDistance(); - if (!dragged) { - if (doubleClickActsOnSelection()) { - // A double-click may still arrive and must act on the whole multi-selection, so collapse only - // once the double-click window has passed without one. The generation guard (and the receiver - // context) drops the collapse if the item is gone, deselected, or a double-click cancelled it; - // the count guard drops it if the selection changed meanwhile (rubber band, ctrl-toggle). - const int generation = ++selectionCollapseGeneration; - const int armedSelectionCount = scene()->selectedItems().count(); - QTimer::singleShot(QApplication::doubleClickInterval(), this, [this, generation, armedSelectionCount] { - if (generation == selectionCollapseGeneration && scene() != nullptr && isSelected() && - scene()->selectedItems().count() == armedSelectionCount && armedSelectionCount > 1) { - scene()->clearSelection(); - setSelected(true); - } - }); - } else { - scene()->clearSelection(); - setSelected(true); - } - } - } - if (event->button() == Qt::MiddleButton) { emit deleteCardInfoPopup(cardRef.name); } @@ -412,17 +370,6 @@ void AbstractCardItem::mouseReleaseEvent(QGraphicsSceneMouseEvent *event) event->accept(); } -void AbstractCardItem::cancelPendingSelectionCollapse() -{ - pendingSelectionCollapse = false; - ++selectionCollapseGeneration; -} - -bool AbstractCardItem::doubleClickActsOnSelection() const -{ - return false; -} - void AbstractCardItem::processHoverEvent() { emit hovered(this); diff --git a/cockatrice/src/game_graphics/board/abstract_card_item.h b/cockatrice/src/game_graphics/board/abstract_card_item.h index e2b150ce8..385f9c32f 100644 --- a/cockatrice/src/game_graphics/board/abstract_card_item.h +++ b/cockatrice/src/game_graphics/board/abstract_card_item.h @@ -41,8 +41,6 @@ protected: private: bool isHovered; qreal realZValue; - bool pendingSelectionCollapse; - int selectionCollapseGeneration = 0; ///< Guards the deferred collapse timer against stale firings private slots: void pixmapUpdated(); @@ -132,8 +130,6 @@ public: } void setFaceDown(bool _facedown); void processHoverEvent(); - /** @brief Drop a pending collapse and disarm any deferred collapse timer. */ - void cancelPendingSelectionCollapse(); void deleteCardInfoPopup() { emit deleteCardInfoPopup(cardRef.name); @@ -148,16 +144,6 @@ protected: void mouseReleaseEvent(QGraphicsSceneMouseEvent *event) override; QVariant itemChange(QGraphicsItem::GraphicsItemChange change, const QVariant &value) override; void cacheBgColor(); - /** - * @brief Whether a double-click on this item acts on the whole selection. - * - * When true, a release without drag defers the multi-selection collapse - * until the double-click window passes, so a following double-click still - * sees every selected card. Subclasses that handle double-clicks on the - * selection must return true here and call - * cancelPendingSelectionCollapse() from their double-click handler. - */ - virtual bool doubleClickActsOnSelection() const; }; #endif diff --git a/cockatrice/src/game_graphics/board/card_item.cpp b/cockatrice/src/game_graphics/board/card_item.cpp index 1901a634f..a04d86c15 100644 --- a/cockatrice/src/game_graphics/board/card_item.cpp +++ b/cockatrice/src/game_graphics/board/card_item.cpp @@ -518,9 +518,6 @@ void CardItem::mouseReleaseEvent(QGraphicsSceneMouseEvent *event) void CardItem::mouseDoubleClickEvent(QGraphicsSceneMouseEvent *event) { - // The preceding release deferred its multi-selection collapse for exactly this: the double-click plays - // every selected card, so the collapse must not run. - cancelPendingSelectionCollapse(); if ((event->modifiers() != Qt::AltModifier) && (event->buttons() == Qt::LeftButton) && (SettingsCache::instance().userInterface().getDoubleClickToPlay())) { handleClickedToPlay(event->modifiers().testFlag(Qt::ShiftModifier)); @@ -528,11 +525,6 @@ void CardItem::mouseDoubleClickEvent(QGraphicsSceneMouseEvent *event) event->accept(); } -bool CardItem::doubleClickActsOnSelection() const -{ - return SettingsCache::instance().userInterface().getDoubleClickToPlay(); -} - bool CardItem::animationEvent() { int rotation = ROTATION_DEGREES_PER_FRAME; diff --git a/cockatrice/src/game_graphics/board/card_item.h b/cockatrice/src/game_graphics/board/card_item.h index 0737d7f9c..df79ef378 100644 --- a/cockatrice/src/game_graphics/board/card_item.h +++ b/cockatrice/src/game_graphics/board/card_item.h @@ -175,7 +175,6 @@ protected: void mouseReleaseEvent(QGraphicsSceneMouseEvent *event) override; void mouseDoubleClickEvent(QGraphicsSceneMouseEvent *event) override; QVariant itemChange(GraphicsItemChange change, const QVariant &value) override; - bool doubleClickActsOnSelection() const override; }; #endif diff --git a/cockatrice/src/game_graphics/deckview/deck_view.cpp b/cockatrice/src/game_graphics/deckview/deck_view.cpp index 630530792..c86bc9925 100644 --- a/cockatrice/src/game_graphics/deckview/deck_view.cpp +++ b/cockatrice/src/game_graphics/deckview/deck_view.cpp @@ -182,15 +182,6 @@ void DeckView::mouseDoubleClickEvent(QMouseEvent *event) QList result; QList sel = scene()->selectedItems(); - // The double-click goes to the view, not the card, so cancel any pending - // selection collapse on the cards before reading the selection. - for (auto *item : sel) { - if (auto *c = dynamic_cast(item)) { - c->cancelPendingSelectionCollapse(); - } - } - sel = scene()->selectedItems(); - for (int i = 0; i < sel.size(); i++) { auto *c = static_cast(sel.at(i)); auto *zone = static_cast(c->parentItem()); diff --git a/cockatrice/src/game_graphics/deckview/deck_view.h b/cockatrice/src/game_graphics/deckview/deck_view.h index 31cb0f87f..3b5b4b6f4 100644 --- a/cockatrice/src/game_graphics/deckview/deck_view.h +++ b/cockatrice/src/game_graphics/deckview/deck_view.h @@ -49,10 +49,6 @@ public: { return originZone; } - bool doubleClickActsOnSelection() const override - { - return true; - } protected: void mouseMoveEvent(QGraphicsSceneMouseEvent *event) override; diff --git a/cockatrice/src/game_graphics/game_view.cpp b/cockatrice/src/game_graphics/game_view.cpp index cb59b3d7d..1c92d74ae 100644 --- a/cockatrice/src/game_graphics/game_view.cpp +++ b/cockatrice/src/game_graphics/game_view.cpp @@ -127,7 +127,6 @@ void GameView::startRubberBand(const QPointF &_selectionOrigin) selectionOrigin = _selectionOrigin; previousBandRect = QRect(); rubberBand->setGeometry(QRect(mapFromScene(selectionOrigin), QSize(0, 0))); - rubberBand->raise(); rubberBand->show(); } diff --git a/cockatrice/src/interface/widgets/dialogs/dlg_settings.cpp b/cockatrice/src/interface/widgets/dialogs/dlg_settings.cpp index 62b8a4909..444c624df 100644 --- a/cockatrice/src/interface/widgets/dialogs/dlg_settings.cpp +++ b/cockatrice/src/interface/widgets/dialogs/dlg_settings.cpp @@ -49,9 +49,6 @@ DlgSettings::DlgSettings(QWidget *parent) : QDialog(parent) QSize target = !hostSize.isEmpty() ? QSize(qRound(hostSize.width() * 0.9), qRound(hostSize.height() * 0.9)) : QSize(qRound(available.width() * 0.9), qRound(available.height() * 0.9)); - // Evenly-spaced columns leave unnatural dead space on very wide windows; clamp the width per review. - static constexpr int MAX_DIALOG_WIDTH = 1280; - target.setWidth(qMin(target.width(), MAX_DIALOG_WIDTH)); target = target.expandedTo(minimum).boundedTo(available.size()); resize(target); setSizeGripEnabled(true); diff --git a/cockatrice/src/interface/widgets/dialogs/path_safety_warning.cpp b/cockatrice/src/interface/widgets/dialogs/path_safety_warning.cpp deleted file mode 100644 index d8eabd72d..000000000 --- a/cockatrice/src/interface/widgets/dialogs/path_safety_warning.cpp +++ /dev/null @@ -1,34 +0,0 @@ -#include "path_safety_warning.h" - -#include "../../client/settings/cache_settings.h" - -#include -#include -#include -#include -#include - -class QWidget; - -bool PathSafetyWarning::confirmOutsideProgramDir(QWidget *parent, const QString &path) -{ - if (SettingsCache::instance().getIsPortableBuild()) { - return true; - } - - if (!PathsSettings::isInsideDir(path, QCoreApplication::applicationDirPath())) { - return true; - } - - QMessageBox box(QMessageBox::Warning, QObject::tr("Path inside the program directory"), - QObject::tr("'%1' is inside the Cockatrice program directory.\n\n" - "Files stored there can be deleted by an update or by uninstalling " - "Cockatrice, so this change was not applied. Choose a location outside " - "the program directory.") - .arg(path), - QMessageBox::Ok, parent); - box.setTextFormat(Qt::PlainText); - box.exec(); - - return false; -} diff --git a/cockatrice/src/interface/widgets/dialogs/path_safety_warning.h b/cockatrice/src/interface/widgets/dialogs/path_safety_warning.h deleted file mode 100644 index 058df3e4b..000000000 --- a/cockatrice/src/interface/widgets/dialogs/path_safety_warning.h +++ /dev/null @@ -1,25 +0,0 @@ -#ifndef COCKATRICE_PATH_SAFETY_WARNING_H -#define COCKATRICE_PATH_SAFETY_WARNING_H - -class QString; -class QWidget; - -namespace PathSafetyWarning -{ - -/** - * @brief Warns when the user tries to configure a data path inside the program directory. - * - * Shows a message box explaining that files kept there can be deleted by an - * update or by uninstalling Cockatrice. Portable builds are always exempt: - * their data lives beside the executables by design. - * - * @param parent The parent widget - * @param path The path the user is trying to configure - * @return true when the path is outside the program directory and safe to apply - */ -bool confirmOutsideProgramDir(QWidget *parent, const QString &path); - -} // namespace PathSafetyWarning - -#endif // COCKATRICE_PATH_SAFETY_WARNING_H diff --git a/cockatrice/src/interface/widgets/server/game_filter_configs.h b/cockatrice/src/interface/widgets/server/game_filter_configs.h index 93ca010d8..1b7322c55 100644 --- a/cockatrice/src/interface/widgets/server/game_filter_configs.h +++ b/cockatrice/src/interface/widgets/server/game_filter_configs.h @@ -17,9 +17,7 @@ struct GameFilterConfigs static constexpr int DEFAULT_MAX_PLAYERS_MAX = 99; bool hideBuddiesOnlyGames = false; - // Defaults to true to match GameFiltersSettings::isHideIgnoredUserGames(), so freshly loaded - // filters compare equal to a default-constructed config. - bool hideIgnoredUserGames = true; + bool hideIgnoredUserGames = false; 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 6922bfe64..27464e14e 100644 --- a/cockatrice/src/interface/widgets/server/game_selector.cpp +++ b/cockatrice/src/interface/widgets/server/game_selector.cpp @@ -113,17 +113,6 @@ 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; } @@ -198,18 +187,6 @@ 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 e3407abea..01e093b06 100644 --- a/cockatrice/src/interface/widgets/server/game_selector.h +++ b/cockatrice/src/interface/widgets/server/game_selector.h @@ -4,7 +4,6 @@ #include "game_type_map.h" #include -#include #include // IWYU pragma: keep // ServerInfo_User appears by value and inside QList in slot signatures, so the @@ -149,9 +148,6 @@ 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. * @@ -215,7 +211,6 @@ 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 315d51d9e..289dbee44 100644 --- a/cockatrice/src/interface/widgets/server/games_model.cpp +++ b/cockatrice/src/interface/widgets/server/games_model.cpp @@ -523,9 +523,6 @@ 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/widgets/settings_page/appearance_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.cpp index be2d5735e..4c38459c6 100644 --- a/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.cpp @@ -23,7 +23,6 @@ #include #include #include -#include #include #include #include @@ -85,9 +84,10 @@ AppearanceSettingsPage::AppearanceSettingsPage() const QString currentStyle = cfg.styleName; const int styleSeedIdx = currentStyle.isEmpty() ? 0 : styleCombo.findData(currentStyle); styleCombo.setCurrentIndex(styleSeedIdx >= 0 ? styleSeedIdx : 0); - previousStyle = styleCombo.currentData().toString(); - connect(&styleCombo, &QComboBox::currentIndexChanged, this, &AppearanceSettingsPage::styleComboChanged); + connect(&styleCombo, &QComboBox::currentIndexChanged, this, + [this] { themeManager->setStyleName(styleCombo.currentData().toString()); }); + connect(&styleCombo, &QComboBox::currentIndexChanged, this, &AppearanceSettingsPage::updateStyleWarning); connect(themeManager, &ThemeManager::themeChanged, this, [this, dirPath] { const QString newDir = themeManager->getAvailableThemes().value(SettingsCache::instance().getThemeName()); @@ -104,7 +104,6 @@ AppearanceSettingsPage::AppearanceSettingsPage() const int styleIdx = currentStyle.isEmpty() ? 0 : styleCombo.findData(currentStyle); styleCombo.setCurrentIndex(styleIdx >= 0 ? styleIdx : 0); styleCombo.blockSignals(false); - previousStyle = styleCombo.currentData().toString(); updateStyleWarning(); }); @@ -468,88 +467,12 @@ void AppearanceSettingsPage::updateHomeTabSettingsVisibility() homeTabDisplayCardNameCheckBox.setVisible(visible); } -static bool isBrokenStyle(const QString &style) -{ - return style.compare("windows11", Qt::CaseInsensitive) == 0 || - style.compare("windowsvista", Qt::CaseInsensitive) == 0; -} - -static int findFusionIndex(const QComboBox &combo) -{ - int fusionIdx = combo.findData(QStringLiteral("Fusion")); - if (fusionIdx >= 0) { - return fusionIdx; - } - for (int i = 0; i < combo.count(); ++i) { - if (combo.itemData(i).toString().compare(QStringLiteral("Fusion"), Qt::CaseInsensitive) == 0) { - return i; - } - } - return -1; -} - void AppearanceSettingsPage::updateStyleWarning() { - styleWarningLabel.setVisible(isBrokenStyle(styleCombo.currentData().toString())); -} - -void AppearanceSettingsPage::styleComboChanged(int index) -{ - if (index < 0) { - return; - } - - const QString newStyle = styleCombo.itemData(index).toString(); - if (!isBrokenStyle(newStyle)) { - themeManager->setStyleName(newStyle); - previousStyle = newStyle; - updateStyleWarning(); - return; - } - - QMessageBox msgBox(this); - msgBox.setWindowTitle(tr("Broken style")); - msgBox.setIcon(QMessageBox::Warning); - msgBox.setText(tr("The \"%1\" style is broken and can break the game view: dragging cards across zones can " - "shrink the board to a tiny grey window that can only be fixed by rejoining the game.") - .arg(newStyle)); - msgBox.setInformativeText( - tr("It can also render controls such as combo boxes unreadable, especially in dark mode.\n\n" - "Fusion is the recommended style on every platform. Switch to Fusion instead?")); - - int fusionIdx = findFusionIndex(styleCombo); - QPushButton *fusionButton = nullptr; - if (fusionIdx >= 0) { - fusionButton = msgBox.addButton(tr("Use Fusion instead"), QMessageBox::AcceptRole); - } - QPushButton *keepButton = msgBox.addButton(tr("Use %1 anyway").arg(newStyle), QMessageBox::DestructiveRole); - msgBox.addButton(QMessageBox::Cancel); - if (fusionButton != nullptr) { - msgBox.setDefaultButton(fusionButton); - } else { - msgBox.setDefaultButton(keepButton); - } - - msgBox.exec(); - - const QAbstractButton *clicked = msgBox.clickedButton(); - if (fusionButton != nullptr && clicked == fusionButton) { - styleCombo.setCurrentIndex(fusionIdx); - return; - } - if (clicked == keepButton) { - themeManager->setStyleName(newStyle); - previousStyle = newStyle; - updateStyleWarning(); - return; - } - - // Cancel: revert to the previously applied style without re-prompting - styleCombo.blockSignals(true); - const int previousIdx = styleCombo.findData(previousStyle); - styleCombo.setCurrentIndex(previousIdx >= 0 ? previousIdx : 0); - styleCombo.blockSignals(false); - updateStyleWarning(); + const QString style = styleCombo.currentData().toString(); + const bool broken = + style.compare("windows11", Qt::CaseInsensitive) == 0 || style.compare("windowsvista", Qt::CaseInsensitive) == 0; + styleWarningLabel.setVisible(broken); } void AppearanceSettingsPage::showShortcutsChanged(QT_STATE_CHANGED_T value) @@ -615,10 +538,9 @@ void AppearanceSettingsPage::retranslateUi() styleComboLabel.setText(tr("Active theme style:")); styleCombo.setToolTip(tr("Qt widget style saved to this theme (\"System\" lets the application decide)")); styleWarningLabel.setText( - tr("Warning: the Windows 11 and Windows Vista styles are broken and can break the game view (dragging " - "cards can shrink the board to a tiny grey window that requires rejoining to fix), and can render " - "controls such as combo boxes unreadable, especially in dark mode. Fusion is the recommended style on " - "every platform: it is Qt's cross-platform style and the one Cockatrice is developed against.")); + tr("Warning: the Windows 11 and Windows Vista styles are buggy and can render controls such as combo boxes " + "unreadable, especially in dark mode. Fusion is the recommended style on every platform: it is Qt's " + "cross-platform style and the one Cockatrice is developed against.")); updateStyleWarning(); editPaletteButton.setText(tr("Edit theme palette")); diff --git a/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.h b/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.h index f919a2a2f..8afd326a3 100644 --- a/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.h +++ b/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.h @@ -23,7 +23,6 @@ private slots: void editPalette(); void updateHomeTabSettingsVisibility(); void updateStyleWarning(); - void styleComboChanged(int index); void showShortcutsChanged(QT_STATE_CHANGED_T enabled); void overrideAllCardArtWithPersonalPreferenceToggled(QT_STATE_CHANGED_T enabled); @@ -41,7 +40,6 @@ private: QComboBox styleCombo; QLabel styleWarningLabel; QPushButton editPaletteButton; - QString previousStyle; QLabel homeTabBackgroundSourceLabel; QComboBox homeTabBackgroundSourceBox; diff --git a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp index 31178ba4c..2020b2031 100644 --- a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp @@ -2,7 +2,6 @@ #include "../../../client/settings/cache_settings.h" #include "../../pixel_map_generator.h" -#include "../dialogs/path_safety_warning.h" #include "update/card_spoiler/spoiler_background_updater.h" #include @@ -364,9 +363,6 @@ void DeckEditorSettingsPage::spoilerPathButtonClicked() if (lsPath.isEmpty()) { return; } - if (!PathSafetyWarning::confirmOutsideProgramDir(this, lsPath + "/spoiler.xml")) { - return; - } mpSpoilerSavePathLineEdit->setText(lsPath + "/spoiler.xml"); SettingsCache::instance().paths().setSpoilerDatabasePath(lsPath + "/spoiler.xml"); diff --git a/cockatrice/src/interface/widgets/settings_page/general_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/general_settings_page.cpp index fa8938b32..73a0f7be2 100644 --- a/cockatrice/src/interface/widgets/settings_page/general_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/general_settings_page.cpp @@ -1,7 +1,6 @@ #include "general_settings_page.h" #include "../../../client/settings/cache_settings.h" -#include "../dialogs/path_safety_warning.h" #include "../interface/card_picture_loader/card_picture_loader.h" #include "../main.h" #include "../server/user/user_info_connection.h" @@ -379,9 +378,6 @@ void GeneralSettingsPage::deckPathButtonClicked() if (path.isEmpty()) { return; } - if (!PathSafetyWarning::confirmOutsideProgramDir(this, path)) { - return; - } deckPathEdit->setText(path); SettingsCache::instance().paths().setDeckPath(path); @@ -393,9 +389,6 @@ void GeneralSettingsPage::filtersPathButtonClicked() if (path.isEmpty()) { return; } - if (!PathSafetyWarning::confirmOutsideProgramDir(this, path)) { - return; - } filtersPathEdit->setText(path); SettingsCache::instance().paths().setFiltersPath(path); @@ -407,9 +400,6 @@ void GeneralSettingsPage::replaysPathButtonClicked() if (path.isEmpty()) { return; } - if (!PathSafetyWarning::confirmOutsideProgramDir(this, path)) { - return; - } replaysPathEdit->setText(path); SettingsCache::instance().paths().setReplaysPath(path); @@ -421,9 +411,6 @@ void GeneralSettingsPage::picsPathButtonClicked() if (path.isEmpty()) { return; } - if (!PathSafetyWarning::confirmOutsideProgramDir(this, path)) { - return; - } picsPathEdit->setText(path); SettingsCache::instance().paths().setPicsPath(path); @@ -435,9 +422,6 @@ void GeneralSettingsPage::cardDatabasePathButtonClicked() if (path.isEmpty()) { return; } - if (!PathSafetyWarning::confirmOutsideProgramDir(this, path)) { - return; - } cardDatabasePathEdit->setText(path); SettingsCache::instance().paths().setCardDatabasePath(path); @@ -449,9 +433,6 @@ void GeneralSettingsPage::customCardDatabaseButtonClicked() if (path.isEmpty()) { return; } - if (!PathSafetyWarning::confirmOutsideProgramDir(this, path)) { - return; - } customCardDatabasePathEdit->setText(path); SettingsCache::instance().paths().setCustomCardDatabasePath(path); @@ -463,9 +444,6 @@ void GeneralSettingsPage::tokenDatabasePathButtonClicked() if (path.isEmpty()) { return; } - if (!PathSafetyWarning::confirmOutsideProgramDir(this, path)) { - return; - } tokenDatabasePathEdit->setText(path); SettingsCache::instance().paths().setTokenDatabasePath(path); diff --git a/cockatrice/src/interface/widgets/tabs/tab_game.cpp b/cockatrice/src/interface/widgets/tabs/tab_game.cpp index 4cf5919ba..9a6862b2c 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_game.cpp +++ b/cockatrice/src/interface/widgets/tabs/tab_game.cpp @@ -1060,12 +1060,6 @@ void TabGame::setCardMenu(CardMenu *menu) return; } - if (aCardMenu->menu() && aCardMenu->menu()->isVisible()) { - aCardMenu->menu()->hide(); - if (gameView && gameView->viewport()) { - gameView->viewport()->update(); - } - } if (menu) { aCardMenu->setMenu(menu); } else { diff --git a/cockatrice/src/interface/widgets/tabs/tab_report.cpp b/cockatrice/src/interface/widgets/tabs/tab_report.cpp index 9018e2477..5f158e9b1 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_report.cpp +++ b/cockatrice/src/interface/widgets/tabs/tab_report.cpp @@ -1,6 +1,5 @@ #include "tab_report.h" -#include "../server/user/user_context_menu.h" #include "../utility/report_utils.h" #include "abstract_client.h" #include "libcockatrice/protocol/pb/response.pb.h" @@ -34,13 +33,11 @@ #include #include #include -#include #include #include #include #include #include -#include #include #include #include @@ -123,10 +120,6 @@ TabReport::TabReport(TabSupervisor *_tabSupervisor, AbstractClient *_client) : T table->horizontalHeader()->setSectionResizeMode(COL_REPORTED, QHeaderView::Stretch); table->horizontalHeader()->setSectionResizeMode(COL_REPORTER, QHeaderView::ResizeToContents); connect(table, &QTableWidget::itemSelectionChanged, this, &TabReport::onSelectionChanged); - table->setContextMenuPolicy(Qt::CustomContextMenu); - connect(table, &QTableWidget::customContextMenuRequested, this, &TabReport::showTableContextMenu); - userContextMenu = new UserContextMenu(tabSupervisor, this); - connect(userContextMenu, &UserContextMenu::openMessageDialog, this, &TabReport::openMessageDialog); descGroup = new QGroupBox; descriptionEdit = new QTextEdit; @@ -238,14 +231,14 @@ TabReport::TabReport(TabSupervisor *_tabSupervisor, AbstractClient *_client) : T assignButton = new QPushButton; resolveButton = new QPushButton; + resolveWithNoteButton = new QPushButton; dismissButton = new QPushButton; - reopenButton = new QPushButton; viewReplayButton = new QPushButton; joinGameButton = new QPushButton; connect(assignButton, &QPushButton::clicked, this, &TabReport::assignReport); - connect(resolveButton, &QPushButton::clicked, this, [this]() { resolveReport(false); }); - connect(dismissButton, &QPushButton::clicked, this, [this]() { resolveReport(true); }); - connect(reopenButton, &QPushButton::clicked, this, &TabReport::reopenReport); + connect(resolveButton, &QPushButton::clicked, this, [this]() { resolveReport(false, false); }); + connect(resolveWithNoteButton, &QPushButton::clicked, this, [this]() { resolveReport(false, true); }); + connect(dismissButton, &QPushButton::clicked, this, [this]() { resolveReport(true, true); }); connect(viewReplayButton, &QPushButton::clicked, this, &TabReport::viewReplay); connect(joinGameButton, &QPushButton::clicked, this, &TabReport::joinGame); @@ -254,8 +247,8 @@ TabReport::TabReport(TabSupervisor *_tabSupervisor, AbstractClient *_client) : T auto *actionBar = new QHBoxLayout; actionBar->addWidget(assignButton); actionBar->addWidget(resolveButton); + actionBar->addWidget(resolveWithNoteButton); actionBar->addWidget(dismissButton); - actionBar->addWidget(reopenButton); actionBar->addSpacing(20); actionBar->addWidget(viewReplayButton); actionBar->addWidget(joinGameButton); @@ -279,7 +272,6 @@ TabReport::TabReport(TabSupervisor *_tabSupervisor, AbstractClient *_client) : T void TabReport::retranslateUi() { - userContextMenu->retranslateUi(); searchEdit->setPlaceholderText(tr("Search by username, category...")); statusFilter->clear(); statusFilter->addItem(tr("All Statuses"), ""); @@ -320,10 +312,9 @@ void TabReport::retranslateUi() statsGroup->setTitle(tr("Report Statistics")); assignButton->setText(tr("Assign to Me")); - resolveButton->setText(tr("Resolve...")); + resolveButton->setText(tr("Resolve")); + resolveWithNoteButton->setText(tr("Resolve with note...")); dismissButton->setText(tr("Dismiss...")); - reopenButton->setText(tr("Reopen...")); - reopenButton->setToolTip(tr("Available for resolved or dismissed reports.")); viewReplayButton->setText(tr("View Replay")); joinGameButton->setText(tr("Join Game")); } @@ -355,7 +346,6 @@ void TabReport::reportListResponse(const Response &response) refreshButton->setEnabled(true); if (response.response_code() != Response::RespOk) { - pendingStatusMessage.clear(); statusLabel->setText(tr("Failed to load reports.")); return; } @@ -369,11 +359,6 @@ void TabReport::reportListResponse(const Response &response) applyFilters(); updateStats(); - if (!pendingStatusMessage.isEmpty()) { - statusLabel->setText(pendingStatusMessage); - pendingStatusMessage.clear(); - } - if (selectedReportIdBeforeRefresh >= 0) { bool found = false; for (int row = 0; row < table->rowCount(); ++row) { @@ -509,36 +494,6 @@ void TabReport::onSelectionChanged() updateActionStates(); } -void TabReport::showTableContextMenu(const QPoint &pos) -{ - QTableWidgetItem *clickedItem = table->itemAt(pos); - if (!clickedItem || clickedItem->row() < 0) { - return; - } - - const int row = clickedItem->row(); - const QTableWidgetItem *reportedItem = table->item(row, COL_REPORTED); - const QTableWidgetItem *idItem = table->item(row, COL_ID); - if (!reportedItem || !idItem || reportedItem->text().isEmpty()) { - return; - } - - int userLevel = ServerInfo_User::IsUser; - const int reportId = idItem->data(Qt::UserRole).toInt(); - for (const ServerInfo_Report &r : filteredReports) { - if (r.report_id() == reportId) { - if (r.reported_user_level() != 0) { - userLevel = r.reported_user_level(); - } - break; - } - } - - table->selectRow(row); - userContextMenu->showContextMenu(table->viewport()->mapToGlobal(pos), reportedItem->text(), - UserLevelFlags(userLevel)); -} - void TabReport::loadReportDetails(int reportId) { detailsRequestedReportId = reportId; @@ -622,8 +577,8 @@ void TabReport::updateActionStates() const QString status = QString::fromStdString(report.status()); assignButton->setEnabled(status == "open"); resolveButton->setEnabled(status == "open" || status == "assigned"); + resolveWithNoteButton->setEnabled(status == "open" || status == "assigned"); dismissButton->setEnabled(status == "open" || status == "assigned"); - reopenButton->setEnabled(status == "resolved" || status == "dismissed"); const bool hasGameId = report.game_id() > 0; const bool hasReplay = hasGameId && report.has_replay_id() && report.replay_id() > 0; @@ -635,8 +590,8 @@ void TabReport::setActionsEnabled(bool enabled) { assignButton->setEnabled(enabled); resolveButton->setEnabled(enabled); + resolveWithNoteButton->setEnabled(enabled); dismissButton->setEnabled(enabled); - reopenButton->setEnabled(enabled); viewReplayButton->setEnabled(false); joinGameButton->setEnabled(false); commentButton->setEnabled(enabled); @@ -690,7 +645,7 @@ void TabReport::assignReport() void TabReport::assignResponse(const Response &response) { if (response.response_code() == Response::RespOk) { - pendingStatusMessage = tr("Assigned."); + statusLabel->setText(tr("Assigned.")); refreshList(); } else { statusLabel->setText(tr("Assignment failed.")); @@ -698,19 +653,22 @@ void TabReport::assignResponse(const Response &response) } } -void TabReport::resolveReport(bool dismissed) +void TabReport::resolveReport(bool dismissed, bool promptNote) { const int reportId = selectedReportId(); if (reportId < 0) { return; } - bool ok; - QString note = QInputDialog::getText(this, dismissed ? tr("Dismiss Report") : tr("Resolve Report"), - dismissed ? tr("Optional note:") : tr("Resolution note (optional):"), - QLineEdit::Normal, QString(), &ok); - if (!ok) { - return; + QString note; + if (promptNote) { + bool ok; + note = QInputDialog::getText(this, dismissed ? tr("Dismiss Report") : tr("Resolve Report"), + dismissed ? tr("Optional note:") : tr("Resolution note (optional):"), + QLineEdit::Normal, QString(), &ok); + if (!ok) { + return; + } } setActionsEnabled(false); @@ -731,46 +689,7 @@ void TabReport::resolveReport(bool dismissed) void TabReport::resolveResponse(const Response &response) { if (response.response_code() == Response::RespOk) { - pendingStatusMessage = tr("Done."); - refreshList(); - } else { - statusLabel->setText(tr("Action failed.")); - updateActionStates(); - } -} - -void TabReport::reopenReport() -{ - const int reportId = selectedReportId(); - if (reportId < 0) { - return; - } - - bool ok; - QString note = QInputDialog::getText(this, tr("Reopen Report"), tr("Reopening note (optional):"), QLineEdit::Normal, - QString(), &ok); - if (!ok) { - return; - } - - setActionsEnabled(false); - statusLabel->setText(tr("Reopening...")); - - Command_ReportReopen cmd; - cmd.set_report_id(reportId); - if (!note.isEmpty()) { - cmd.set_reopen_note(note.toStdString()); - } - - PendingCommand *pend = client->prepareModeratorCommand(cmd); - connect(pend, &PendingCommand::finished, this, &TabReport::reopenResponse); - client->sendCommand(pend); -} - -void TabReport::reopenResponse(const Response &response) -{ - if (response.response_code() == Response::RespOk) { - pendingStatusMessage = tr("Reopened."); + statusLabel->setText(tr("Done.")); refreshList(); } else { statusLabel->setText(tr("Action failed.")); diff --git a/cockatrice/src/interface/widgets/tabs/tab_report.h b/cockatrice/src/interface/widgets/tabs/tab_report.h index 32ebe1a3f..645f949de 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_report.h +++ b/cockatrice/src/interface/widgets/tabs/tab_report.h @@ -4,7 +4,6 @@ #include "tab.h" #include -#include #include #include #include @@ -23,7 +22,6 @@ class QTimer; class GameReplay; class Response; class TabSupervisor; -class UserContextMenu; class TabReport : public Tab { @@ -39,17 +37,15 @@ public: signals: void openReplay(GameReplay *replay); void requestJoinGame(int gameId, int roomId); - void openMessageDialog(const QString &userName, bool focus); private slots: void refreshList(); void reportListResponse(const Response &response); void assignReport(); void assignResponse(const Response &response); - void resolveReport(bool dismissed); + void resolveReport(bool dismissed, bool promptNote); void resolveResponse(const Response &response); void onSelectionChanged(); - void showTableContextMenu(const QPoint &pos); void viewReplay(); void viewReplayResponse(const Response &response); void joinGame(); @@ -60,8 +56,6 @@ private slots: void reportDetailsResponse(const Response &response); void requestStats(); void statsResponse(const Response &response); - void reopenReport(); - void reopenResponse(const Response &response); private: int selectedReportId() const; @@ -91,13 +85,12 @@ private: QPushButton *commentButton; QPushButton *assignButton; QPushButton *resolveButton; + QPushButton *resolveWithNoteButton; QPushButton *dismissButton; - QPushButton *reopenButton; QPushButton *viewReplayButton; QPushButton *joinGameButton; QLabel *statusLabel; QTimer *refreshTimer; - UserContextMenu *userContextMenu; QGroupBox *userContextGroup; QLabel *userContextName; @@ -115,7 +108,6 @@ private: QLabel *userContextNotesLabel; QLabel *userContextRecentReportsLabel; QString lastRequestedUser; - QString pendingStatusMessage; QGroupBox *statsGroup; QLabel *statsTotalLabel; QLabel *statsTrendLabel; diff --git a/cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp b/cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp index 2352d7d7b..70014ec47 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp +++ b/cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp @@ -919,7 +919,6 @@ void TabSupervisor::openTabReport() myAddTab(tabReport, aTabReport); connect(tabReport, &TabReport::openReplay, this, &TabSupervisor::openReplay); connect(tabReport, &TabReport::requestJoinGame, this, &TabSupervisor::joinReportGame); - connect(tabReport, &TabReport::openMessageDialog, this, &TabSupervisor::addMessageTab); connect(tabReport, &QObject::destroyed, this, [this] { tabReport = nullptr; aTabReport->setChecked(false); diff --git a/cockatrice/src/interface/widgets/utility/card_completer_delegate.cpp b/cockatrice/src/interface/widgets/utility/card_completer_delegate.cpp index af07590a8..8042fb9fc 100644 --- a/cockatrice/src/interface/widgets/utility/card_completer_delegate.cpp +++ b/cockatrice/src/interface/widgets/utility/card_completer_delegate.cpp @@ -46,22 +46,6 @@ template class QSharedPointer; namespace { -// Below this row width the delegate switches to a compact layout so the card -// name stays readable when the popup cannot be as wide as PopupRowWidth. -constexpr int CompactRowWidth = 380; -constexpr int CompactSymbolRadius = 6; -constexpr int CompactSymbolSpacing = 1; -constexpr int CompactRightPad = 8; -constexpr int ManaZoneWidth = 110; -constexpr int CompactManaZoneWidth = 70; -constexpr int RightPad = 14; -constexpr int TextLeftPad = 12; -constexpr int TextRightGap = 8; - -// Below this text width the type-line band is dropped so the name can use the -// whole row height instead of being elided to a single readable character. -constexpr int MinNameWidth = 120; - struct ManaColor { QColor fill; @@ -141,7 +125,7 @@ QSize CardCompleterDelegate::sizeHint(const QStyleOptionViewItem &option, const } // Fixed wide rows so the popup has room for name, type line, set and mana - return {PopupRowWidth, CardRowHeight}; + return {480, CardRowHeight}; } // --------------------------------------------------------------------------- @@ -192,12 +176,7 @@ void CardCompleterDelegate::drawManaSymbol(QPainter *p, QPoint centre, const QSt // --------------------------------------------------------------------------- -int CardCompleterDelegate::drawManaCost(QPainter *p, - const QRect &row, - const QString &manaCost, - int radius, - int spacing, - int rightPad) const +int CardCompleterDelegate::drawManaCost(QPainter *p, const QRect &row, const QString &manaCost, int radius) const { if (manaCost.isEmpty()) { return row.right(); @@ -226,9 +205,11 @@ int CardCompleterDelegate::drawManaCost(QPainter *p, totalW += PartGap; } - totalW += parts.at(i).size() * diam + qMax(0, parts.at(i).size() - 1) * spacing; + totalW += parts.at(i).size() * diam + qMax(0, parts.at(i).size() - 1) * SymbolSpacing; } + const int rightPad = 14; + int x = row.right() - rightPad - totalW + radius; const int cy = row.center().y(); @@ -238,11 +219,11 @@ int CardCompleterDelegate::drawManaCost(QPainter *p, for (const QString &sym : symbols) { drawManaSymbol(p, {x, cy}, sym, radius); - x += diam + spacing; + x += diam + SymbolSpacing; } if (i < parts.size() - 1) { - x += PartGap - spacing; + x += PartGap - SymbolSpacing; } } @@ -309,14 +290,6 @@ void CardCompleterDelegate::paint(QPainter *painter, const QStyleOptionViewItem const QString typeLine = card->getCardType(); const QString setCode = setCodeForCard(card); - // When the row is too narrow for the full layout, use smaller mana pips and - // a slimmer mana zone so the card name keeps as much room as possible. - const bool compact = rect.width() < CompactRowWidth; - const int symbolRadius = compact ? CompactSymbolRadius : SymbolRadius; - const int symbolSpacing = compact ? CompactSymbolSpacing : SymbolSpacing; - const int rightPad = compact ? CompactRightPad : RightPad; - const int manaZoneWidth = compact ? CompactManaZoneWidth : ManaZoneWidth; - const QColor accent = accentForColors(card->getColors()); const QColor base = pal.color(QPalette::Base); @@ -365,7 +338,7 @@ void CardCompleterDelegate::paint(QPainter *painter, const QStyleOptionViewItem // Right mana zone // ----------------------------------------------------------------------- - const QRectF manaZone(cardRect.right() - manaZoneWidth, cardRect.top(), manaZoneWidth, cardRect.height()); + const QRectF manaZone(cardRect.right() - 110, cardRect.top(), 110, cardRect.height()); QLinearGradient manaGrad(manaZone.topLeft(), manaZone.bottomLeft()); @@ -407,27 +380,22 @@ void CardCompleterDelegate::paint(QPainter *painter, const QStyleOptionViewItem // Mana cost // ----------------------------------------------------------------------- - const int costLeft = drawManaCost(painter, cardRect.toRect(), manaCost, symbolRadius, symbolSpacing, rightPad); + const int costLeft = drawManaCost(painter, cardRect.toRect(), manaCost, SymbolRadius); // ----------------------------------------------------------------------- // Card name + type line + set code // ----------------------------------------------------------------------- - const int textLeft = cardRect.left() + AccentBarWidth + TextLeftPad; - const int textRight = costLeft - TextRightGap; + const int textLeft = cardRect.left() + AccentBarWidth + 12; + const int textRight = costLeft - 8; const int textWidth = qMax(0, textRight - textLeft); - // If even the compact layout leaves too little room for a readable name, - // drop the type-line band and let the name use the full row height. - const bool showInfoBand = textWidth >= MinNameWidth; - // ----------------------------------------------------------------------- // Card name (top band) // ----------------------------------------------------------------------- { - const QRect nameRect = showInfoBand ? QRect(textLeft, rect.top() + 2, textWidth, 20) - : QRect(textLeft, rect.top() + 2, textWidth, rect.height() - 4); + const QRect nameRect(textLeft, rect.top() + 2, textWidth, 20); QFont f = option.font; f.setPixelSize(13); @@ -448,7 +416,7 @@ void CardCompleterDelegate::paint(QPainter *painter, const QStyleOptionViewItem // Type line + set code (bottom band) // ----------------------------------------------------------------------- - if (showInfoBand) { + { const QRect infoRect(textLeft, rect.top() + 22, textWidth, rect.height() - 24); QFont f = option.font; diff --git a/cockatrice/src/interface/widgets/utility/card_completer_delegate.h b/cockatrice/src/interface/widgets/utility/card_completer_delegate.h index cef25b783..ad81d86e4 100644 --- a/cockatrice/src/interface/widgets/utility/card_completer_delegate.h +++ b/cockatrice/src/interface/widgets/utility/card_completer_delegate.h @@ -36,9 +36,6 @@ public: void paint(QPainter *painter, const QStyleOptionViewItem &option, const QModelIndex &index) const override; QSize sizeHint(const QStyleOptionViewItem &option, const QModelIndex &index) const override; - // Content width of one popup row; the completer popup is sized to fit it. - static constexpr int PopupRowWidth = 480; - private: // Set short codes, resolved once per card name and cached mutable QCache setCodeCache; @@ -51,8 +48,7 @@ private: // Draw all mana pips for a cost string like "2RG" or "{2}{R}{G}"; split and // adventure costs ("1W // W") are drawn as separate groups. Returns the left-most x used - int - drawManaCost(QPainter *p, const QRect &row, const QString &manaCost, int radius, int spacing, int rightPad) const; + int drawManaCost(QPainter *p, const QRect &row, const QString &manaCost, int radius) const; // Resolve the preferred printing's set short code for a card QString setCodeForCard(const QSharedPointer &card) const; diff --git a/cockatrice/src/interface/widgets/utility/card_completer_styler.cpp b/cockatrice/src/interface/widgets/utility/card_completer_styler.cpp index 8aa32556f..1ddcc4200 100644 --- a/cockatrice/src/interface/widgets/utility/card_completer_styler.cpp +++ b/cockatrice/src/interface/widgets/utility/card_completer_styler.cpp @@ -20,7 +20,6 @@ #include #include #include -#include #include #include #include @@ -104,7 +103,6 @@ bool CardCompleterStyler::eventFilter(QObject *obj, QEvent *ev) case QEvent::Show: case QEvent::Move: case QEvent::Resize: - applyPopupWidth(); updateOrientation(); reposition(); break; @@ -161,42 +159,6 @@ bool CardCompleterStyler::handlePopupKeyPress(QKeyEvent *event) // --------------------------------------------------------------------------- -void CardCompleterStyler::applyPopupWidth() -{ - auto *popup = completer->popup(); - - // QCompleter sizes its popup to the line edit, so a narrow search field - // would crush the rows. Ask for the width the rows are designed for (plus - // the frame and scrollbar) and let QWidget's geometry clamping keep the - // popup that wide, clamped to the screen it is shown on. - int contentWidth = CardCompleterDelegate::PopupRowWidth + popup->frameWidth() * 2; - if (auto *vbar = popup->verticalScrollBar(); vbar != nullptr) { - contentWidth += vbar->sizeHint().width(); - } - - const QScreen *screen = popup->screen(); - const QRect available = screen ? screen->availableGeometry() : QRect(); - - const int minWidth = available.isEmpty() ? contentWidth : qMin(contentWidth, available.width()); - popup->setMinimumWidth(minWidth); - - // setMinimumWidth() already widened the popup if it was narrower, but - // QCompleter positioned it for the line-edit width, so the widened popup - // can now extend past the right screen edge. The popup is a top-level - // Qt::Popup window, so geometry()/move() are already in screen - // coordinates: clamp the x from geometry() and move directly. - if (!available.isEmpty()) { - const QRect geom = popup->geometry(); - const int clampedX = - qBound(available.left(), geom.x(), qMax(available.left(), available.right() - geom.width() + 1)); - if (clampedX != geom.x()) { - popup->move(QPoint(clampedX, popup->y())); - } - } -} - -// --------------------------------------------------------------------------- - void CardCompleterStyler::updateOrientation() { above = isPopupAboveWidget(); diff --git a/cockatrice/src/interface/widgets/utility/card_completer_styler.h b/cockatrice/src/interface/widgets/utility/card_completer_styler.h index 2912f7312..9245c9095 100644 --- a/cockatrice/src/interface/widgets/utility/card_completer_styler.h +++ b/cockatrice/src/interface/widgets/utility/card_completer_styler.h @@ -52,7 +52,6 @@ private: void stopPreviewFade(); void reposition(); - void applyPopupWidth(); void updateOrientation(); void ensureClosestSelected(); diff --git a/cockatrice/src/interface/widgets/utility/line_edit_completer.cpp b/cockatrice/src/interface/widgets/utility/line_edit_completer.cpp index d692fb41b..056887f1a 100644 --- a/cockatrice/src/interface/widgets/utility/line_edit_completer.cpp +++ b/cockatrice/src/interface/widgets/utility/line_edit_completer.cpp @@ -130,19 +130,10 @@ void LineEditCompleter::keyPressEvent(QKeyEvent *event) CompleterInfo *active = nullptr; QString prefix; - // An open "[[" card link takes precedence over a mention: while the cursor is - // inside it, an earlier "@" must not shadow the card completer. - const int openCardPos = textValue.lastIndexOf("[[", cursorPos - 1); - const int closeCardPos = openCardPos == -1 ? -1 : textValue.indexOf("]]", openCardPos + 2); - const bool insideCardLink = openCardPos != -1 && (closeCardPos == -1 || closeCardPos >= cursorPos); - for (auto &info : completers) { bool triggered = false; switch (info.trigger) { case CompleterTrigger::Mention: { - if (insideCardLink) { - break; - } int triggerPos = textValue.lastIndexOf("@", cursorPos - 1); if (triggerPos != -1 && (triggerPos == 0 || textValue[triggerPos - 1].isSpace())) { triggered = true; diff --git a/cockatrice/src/interface/widgets/utility/report_utils.cpp b/cockatrice/src/interface/widgets/utility/report_utils.cpp index d7fc083c6..a581c9837 100644 --- a/cockatrice/src/interface/widgets/utility/report_utils.cpp +++ b/cockatrice/src/interface/widgets/utility/report_utils.cpp @@ -100,27 +100,10 @@ void renderReportDetails(QTextEdit *chatLogEdit, const QString &moderatorPrefix, const QString &nonModeratorPrefix) { - QString details; - if (report.has_resolution_note() && !report.resolution_note().empty()) { - details = QObject::tr("Resolution note: %1").arg(QString::fromStdString(report.resolution_note())); - } - if (report.has_resolution_time()) { - QString resolvedBy = QObject::tr("unknown"); - if (report.has_resolved_by_name() && !report.resolved_by_name().empty()) { - resolvedBy = QString::fromStdString(report.resolved_by_name()); - } - if (!details.isEmpty()) { - details += "\n"; - } - details += QObject::tr("Resolved at %1 by %2").arg(formatReportTime(report.resolution_time()), resolvedBy); - } - if (report.has_chat_log() && !report.chat_log().empty()) { - const QString chatLog = QString::fromStdString(report.chat_log()); - chatLogEdit->setPlainText( - details.isEmpty() ? chatLog : details + "\n\n" + QObject::tr("=== Chat Log ===") + "\n" + chatLog); + chatLogEdit->setPlainText(QString::fromStdString(report.chat_log())); } else { - chatLogEdit->setPlainText(details); + chatLogEdit->clear(); } commentsEdit->clear(); diff --git a/cockatrice/src/interface/window_main.cpp b/cockatrice/src/interface/window_main.cpp index 57d6e8b15..189585d84 100644 --- a/cockatrice/src/interface/window_main.cpp +++ b/cockatrice/src/interface/window_main.cpp @@ -58,9 +58,7 @@ #include #include #include -#include #include -#include #include #include #include @@ -70,7 +68,6 @@ #include #include #include -#include #include #include #include @@ -606,45 +603,14 @@ MainWindow::MainWindow(QWidget *parent) QTimer::singleShot(0, this, &MainWindow::applyStartupDestination); } -void MainWindow::warnAboutDataPathsInProgramDir() -{ - auto &paths = SettingsCache::instance().paths(); - if (SettingsCache::instance().getIsPortableBuild() || paths.getAppDirWarningAcknowledged()) { - return; - } - - const QStringList offenders = paths.pathsInsideDir(QCoreApplication::applicationDirPath()); - if (offenders.isEmpty()) { - return; - } - - QMessageBox box(QMessageBox::Warning, tr("Data paths inside the program directory"), - tr("These paths are configured inside the Cockatrice program directory:\n\n%1\n\n" - "Cockatrice keeps its own data outside the program directory by default. Files " - "stored inside it can be deleted by an update or by uninstalling Cockatrice. " - "Change them in the Settings dialog - most are on the General page - to keep " - "them safe.") - .arg(offenders.join(QLatin1Char('\n'))), - QMessageBox::Ok, this); - box.setTextFormat(Qt::PlainText); - auto *dontShowAgain = new QCheckBox(tr("Don't show this again")); - box.setCheckBox(dontShowAgain); - box.exec(); - if (dontShowAgain->isChecked()) { - paths.setAppDirWarningAcknowledged(true); - } -} - void MainWindow::startupConfigCheck() { - warnAboutDataPathsInProgramDir(); - const bool isCleanInstall = SettingsCache::instance().network().getClientVersion() == CLIENT_INFO_NOT_SET; - // 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. + // 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. // 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. @@ -1138,35 +1104,18 @@ void MainWindow::cardDatabaseNewSetsFound(int numUnknownSets, QStringList unknow void MainWindow::cardDatabaseAllNewSetsEnabled() { - auto *cardDatabase = CardDatabaseManager::getInstance(); - - // With no card data there is nothing to have enabled and nothing to reparse. - if (cardDatabase->getCardList().isEmpty()) { + 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. return; } - // 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(); })); + 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(); } /* CARD UPDATER */ @@ -1253,7 +1202,7 @@ void MainWindow::createCardUpdateProcess(bool background) << dir.absoluteFilePath(binaryName); QMessageBox::warning(this, tr("Error"), tr("Unable to run the card database updater: ") + dir.absoluteFilePath(binaryName)); - exitCardDatabaseUpdate(false); + exitCardDatabaseUpdate(); emit cardDatabaseUpdateFinished(false); return; } @@ -1330,7 +1279,7 @@ void MainWindow::cardUpdateStandardError() } } -void MainWindow::exitCardDatabaseUpdate(bool doReload) +void MainWindow::exitCardDatabaseUpdate() { if (!cardUpdateProcess) { return; @@ -1339,21 +1288,7 @@ void MainWindow::exitCardDatabaseUpdate(bool doReload) cardUpdateProcess = nullptr; statusBar()->clearMessage(); - if (doReload) { - // 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(); })); + const auto reloadOk1 = QtConcurrent::run([] { CardDatabaseManager::getInstance()->loadCardDatabases(); }); } void MainWindow::cardUpdateError(QProcess::ProcessError err) @@ -1389,7 +1324,7 @@ void MainWindow::cardUpdateError(QProcess::ProcessError err) } qCWarning(CardDatabaseUpdateLog) << "The card database updater exited with an error:" << error; - exitCardDatabaseUpdate(false); + exitCardDatabaseUpdate(); QMessageBox::warning(this, tr("Error"), tr("The card database updater exited with an error:\n%1").arg(error)); emit cardDatabaseUpdateFinished(false); } @@ -1402,14 +1337,12 @@ void MainWindow::cardUpdateFinished(int exitCode, QProcess::ExitStatus exitStatu cardUpdateProgressOutput(); // drain any progress lines not yet parsed const bool success = (exitStatus == QProcess::NormalExit) && (exitCode == 0); - if (success) { + if (exitStatus == QProcess::NormalExit) { SettingsCache::instance().updates().setLastCardUpdateCheck(QDateTime::currentDateTime().date()); - } else if (exitStatus == QProcess::NormalExit) { - statusBar()->showMessage(tr("Card database update cancelled."), 10000); } qCInfo(CardDatabaseUpdateLog) << "The card database updater finished with exit code" << exitCode << "exit status" << static_cast(exitStatus) << "success:" << success; - exitCardDatabaseUpdate(success); + exitCardDatabaseUpdate(); notifyCardUpdateFinishedFromTray(success); emit cardDatabaseUpdateFinished(success); } @@ -1557,8 +1490,7 @@ 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.")); - // Imported files can introduce sets the config has never seen. - reloadCardDatabaseAndCheckSets(); + const auto reloadOk1 = QtConcurrent::run([] { CardDatabaseManager::getInstance()->loadCardDatabases(); }); } 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 2b6ed5633..bf68ed72a 100644 --- a/cockatrice/src/interface/window_main.h +++ b/cockatrice/src/interface/window_main.h @@ -149,23 +149,12 @@ private: void runFirstRunWizard(bool firstRun = false); - /** @brief Warns once when a non-portable install keeps its data paths inside the program directory. */ - void warnAboutDataPathsInProgramDir(); - inline QString getCardUpdaterBinaryName() { return "oracle"; } void createCardUpdateProcess(bool background = false); - void exitCardDatabaseUpdate(bool doReload); - /** - * @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(); + void exitCardDatabaseUpdate(); /** * @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 3f63efc3b..b9cc8dabc 100644 --- a/cockatrice/src/main.cpp +++ b/cockatrice/src/main.cpp @@ -343,12 +343,11 @@ 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 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. + // 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. 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 4aab7ad2c..e9626d57f 100644 --- a/doc/doxygen/pages/user_documentation/deck_management/importing_decks.md +++ b/doc/doxygen/pages/user_documentation/deck_management/importing_decks.md @@ -38,18 +38,7 @@ 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: -`[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`. +TODO 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/gcovr.cfg b/gcovr.cfg deleted file mode 100644 index ac893b6a4..000000000 --- a/gcovr.cfg +++ /dev/null @@ -1,63 +0,0 @@ -# gcovr configuration for Cockatrice line coverage. -# -# Used by .ci/compile.sh --coverage and by hand: -# -# cmake -S . -B build/cover -DCMAKE_BUILD_TYPE=Debug -DWITH_SERVER=1 -DTEST=1 -DENABLE_COVERAGE=ON -# cmake --build build/cover -j 24 -# ctest --test-dir build/cover -LE bench -j 24 -# gcovr build/cover --root . --object-directory build/cover --txt --decisions -# -# The point of most of this file is subtraction. A raw report is dominated by code no test can -# reach: vendored third-party trees, build directories left over from earlier experiments, and -# the client's main() and window classes, which have no headless test. Those all dilute the one -# number worth watching, so they are filtered out here rather than in every invocation. - -# root is deliberately not set here. This file is normally invoked from the build directory -# (compile.sh runs there, because it configures with `cmake ..`), so a `root` of `.` would -# resolve to the build tree and every filter below would exclude everything. compile.sh passes -# --root explicitly instead. - -# Our own code only. This keeps generated Qt MOC output, protobuf output and anything CMake -# built under the source tree out of the report. -filter = ^cockatrice/ -filter = ^libcockatrice_[^/]+/ -filter = ^servatrice/ -filter = ^oracle/ - -# The tests are the vehicle, not the subject. They are compiled with the same instrumentation, -# so leaving them in would add their near-perfect coverage to the numerator and quietly report a -# healthier project than we have: test code is only ever executed by the run that is measuring -# it. Helpers under tests/ are excluded on the same grounds. -exclude = ^tests/ - -# Vendored third-party code is not ours to cover and is never going to move the number. -exclude = ^vcpkg/ -exclude = ^doc/doxygen/theme/ -exclude = ^libcockatrice_rng/libcockatrice/rng/sfmt/ -exclude = ^libcockatrice_utility/libcockatrice/utility/peglib\.h$ -exclude = ^oracle/src/lzma/ -exclude = ^oracle/src/qt-json/ -exclude = ^oracle/src/zip/ -exclude = ^servatrice/src/smtp/ - -# Qt's own moc/pch/uic output is generated into the build tree, not the source tree, so the -# filters above already keep it out. These cover CLion's directories, which sit in-tree and are -# picked up by the object-directory walk. -exclude = .*cmake-build-.* -exclude = .*\.cache/ - -# Process entry points and the client's window chrome. Each of these needs a display, an -# interactive terminal, or a live database before it does anything a test could observe, so no -# headless run will ever move them. Left in, they contribute thousands of lines at 0% and drag -# the total down by a fixed amount that hides movement in the code we can actually test. The -# window header is excluded with its .cpp because gcovr attributes its inline template code -# there, which is where most of its line count lives. -exclude = ^cockatrice/src/main\.cpp$ -exclude = ^cockatrice/src/interface/window_main\. -exclude = ^oracle/src/main\.cpp$ -exclude = ^servatrice/src/main\.cpp$ - -exclude-throw-branches = yes -exclude-function-lines = yes - -print-summary = yes diff --git a/libcockatrice_card/CMakeLists.txt b/libcockatrice_card/CMakeLists.txt index b3583522c..15388ecfc 100644 --- a/libcockatrice_card/CMakeLists.txt +++ b/libcockatrice_card/CMakeLists.txt @@ -60,5 +60,3 @@ target_link_libraries( PUBLIC libcockatrice_utility PUBLIC ${QT_CORE_MODULE} ) - -target_precompile_headers(libcockatrice_card PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_card/libcockatrice/card/database/card_database.cpp b/libcockatrice_card/libcockatrice/card/database/card_database.cpp index 7b0d5b2f9..469c8d02c 100644 --- a/libcockatrice_card/libcockatrice/card/database/card_database.cpp +++ b/libcockatrice_card/libcockatrice/card/database/card_database.cpp @@ -13,7 +13,6 @@ #include #include #include -#include #include #include #include @@ -81,17 +80,15 @@ 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) { - QMetaObject::invokeMethod(this, &CardDatabase::checkUnknownSets, Qt::AutoConnection); + 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); } } @@ -210,19 +207,15 @@ void CardDatabase::checkUnknownSets() if (numUnknownSets > 0) { emit cardDatabaseNewSetsFound(numUnknownSets, unknownSetNames); } else { - _sets.markAllAsKnownBatched(setPriorityController); + _sets.markAllAsKnown(); } } else { // No set enabled. Probably this is the first time running trice - _sets.enableAllBatched(setPriorityController); + _sets.guessSortKeys(); _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(); } } @@ -230,13 +223,13 @@ void CardDatabase::checkUnknownSets() void CardDatabase::enableAllUnknownSets() { auto _sets = getSetList(); - _sets.enableAllUnknownBatched(setPriorityController); + _sets.enableAllUnknown(); } void CardDatabase::markAllSetsAsKnown() { auto _sets = getSetList(); - _sets.markAllAsKnownBatched(setPriorityController); + _sets.markAllAsKnown(); } 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 00b21f88e..194083615 100644 --- a/libcockatrice_card/libcockatrice/card/database/card_database_loader.cpp +++ b/libcockatrice_card/libcockatrice/card/database/card_database_loader.cpp @@ -44,13 +44,7 @@ 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(); - // 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(); - }); + connect(pathProvider, &ICardDatabasePathProvider::cardDatabasePathChanged, this, [this] { loadCardDatabases(); }); } CardDatabaseLoader::~CardDatabaseLoader() @@ -142,23 +136,7 @@ LoadStatus CardDatabaseLoader::doLoadCardDatabases(bool ignoreCache) loadCardDatabase(p, data); } - // 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)) { + if (!saveToCache(data, sourceHash)) { qCWarning(CardDatabaseLoadingLog) << "Failed to write binary cache to" << cachePath(); } } @@ -170,15 +148,18 @@ 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 bfbaa0f51..fffd31d2f 100644 --- a/libcockatrice_card/libcockatrice/card/database/card_database_loader.h +++ b/libcockatrice_card/libcockatrice/card/database/card_database_loader.h @@ -179,6 +179,7 @@ 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 b9ac74e2d..538d78852 100644 --- a/libcockatrice_card/libcockatrice/card/set/card_set.h +++ b/libcockatrice_card/libcockatrice/card/set/card_set.h @@ -227,10 +227,6 @@ 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 c4faf1c4c..2e030adab 100644 --- a/libcockatrice_card/libcockatrice/card/set/card_set_list.cpp +++ b/libcockatrice_card/libcockatrice/card/set/card_set_list.cpp @@ -1,11 +1,9 @@ #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 @@ -64,87 +62,60 @@ QStringList CardSetList::getUnknownSetsNames() return sets; } -void CardSetList::enableAllUnknownBatched(ICardSetPriorityController *controller) +void CardSetList::enableAllUnknown() { - QVector saveData; for (int i = 0; i < size(); ++i) { CardSetPtr set = at(i); - if (set.isNull()) { - continue; + if (set && !set->getIsKnown() && !set->getIsKnownIgnored()) { + set->setIsKnown(true); + set->setEnabled(true); + } else if (set && set->getIsKnownIgnored() && !set->getEnabled()) { + set->setEnabled(true); } - - 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) +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; - saveData.reserve(size()); for (int i = 0; i < size(); ++i) { CardSetPtr set = at(i); if (set.isNull()) { + // qCWarning(CardInfoLog) << "guessSortKeys set is null"; 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); + set->setSortKey(i); } } diff --git a/libcockatrice_card/libcockatrice/card/set/card_set_list.h b/libcockatrice_card/libcockatrice/card/set/card_set_list.h index f31710b33..b9c7b2a7a 100644 --- a/libcockatrice_card/libcockatrice/card/set/card_set_list.h +++ b/libcockatrice_card/libcockatrice/card/set/card_set_list.h @@ -40,37 +40,36 @@ public: void sortByKey(); /** - * @brief Enables every set with fresh sort keys, persisting in one flush. + * @brief Reassigns sort keys based on the current order. * - * 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. + * 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. */ - void enableAllBatched(ICardSetPriorityController *controller); + void guessSortKeys(); /** - * @brief Enables all sets that are unknown or ignored, persisting in one flush. + * @brief Enables all sets that are unknown or ignored. * * 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 enableAllUnknownBatched(ICardSetPriorityController *controller); + void enableAllUnknown(); /** - * @brief Marks all sets as known and adjusts their enabled state, persisting in one flush. + * @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. * * 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 markAllAsKnownBatched(ICardSetPriorityController *controller); + void markAllAsKnown(); /** * @brief Counts the number of sets that are currently enabled. diff --git a/libcockatrice_deck_list/CMakeLists.txt b/libcockatrice_deck_list/CMakeLists.txt index e61a5702c..0c487466c 100644 --- a/libcockatrice_deck_list/CMakeLists.txt +++ b/libcockatrice_deck_list/CMakeLists.txt @@ -41,5 +41,3 @@ target_link_libraries( libcockatrice_deck_list PUBLIC libcockatrice_interfaces libcockatrice_protocol libcockatrice_utility ${QT_CORE_MODULE} ) - -target_precompile_headers(libcockatrice_deck_list PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") 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 fb6a61769..61af36540 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,7 +33,6 @@ 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]*))"); @@ -111,34 +110,8 @@ bool parse(QTextStream &in, continue; } - const QString rawLine = match.captured(); - QString cardName = rawLine.simplified(); + QString cardName = match.captured().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) { @@ -183,12 +156,11 @@ bool parse(QTextStream &in, } // check if a specific amount is mentioned - if (!hasTabQuantity) { - match = reMultiplier.match(cardName); - if (match.hasMatch()) { - amount = match.captured(1).toInt(); - cardName = match.captured(2); - } + int amount = 1; + match = reMultiplier.match(cardName); + if (match.hasMatch()) { + amount = match.captured(1).toInt(); + cardName = match.captured(2); } // Normalize the card name diff --git a/libcockatrice_filters/CMakeLists.txt b/libcockatrice_filters/CMakeLists.txt index 2d45b8e5b..905533df8 100644 --- a/libcockatrice_filters/CMakeLists.txt +++ b/libcockatrice_filters/CMakeLists.txt @@ -18,5 +18,3 @@ add_dependencies(libcockatrice_filters libcockatrice_card) target_include_directories(libcockatrice_filters PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}) target_link_libraries(libcockatrice_filters PUBLIC libcockatrice_card ${QT_CORE_MODULE}) - -target_precompile_headers(libcockatrice_filters PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_interfaces/CMakeLists.txt b/libcockatrice_interfaces/CMakeLists.txt index 3b17569b1..f53c41807 100644 --- a/libcockatrice_interfaces/CMakeLists.txt +++ b/libcockatrice_interfaces/CMakeLists.txt @@ -31,5 +31,3 @@ add_library(libcockatrice_interfaces STATIC ${MOC_SOURCES}) target_include_directories(libcockatrice_interfaces PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}) target_link_libraries(libcockatrice_interfaces PUBLIC libcockatrice_utility ${QT_CORE_MODULE}) - -target_precompile_headers(libcockatrice_interfaces PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") 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 746ac9490..6b3e684d7 100644 --- a/libcockatrice_interfaces/libcockatrice/interfaces/interface_card_set_priority_controller.h +++ b/libcockatrice_interfaces/libcockatrice/interfaces/interface_card_set_priority_controller.h @@ -12,7 +12,6 @@ public: QString shortName; unsigned int sortKey; bool enabled; - bool isKnown = false; }; /** @@ -58,14 +57,6 @@ 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/CMakeLists.txt b/libcockatrice_models/libcockatrice/models/database/CMakeLists.txt index a73e36dbb..840f14b08 100644 --- a/libcockatrice_models/libcockatrice/models/database/CMakeLists.txt +++ b/libcockatrice_models/libcockatrice/models/database/CMakeLists.txt @@ -29,5 +29,3 @@ add_library( target_include_directories(libcockatrice_models_database PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}) target_link_libraries(libcockatrice_models_database PUBLIC libcockatrice_card libcockatrice_filters ${QT_CORE_MODULE}) - -target_precompile_headers(libcockatrice_models_database PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") 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 a0b9c1061..1bc1b9e38 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, sets[i]->getIsKnown()}); + saveData.append({sets[i]->getShortName(), sortKey, enabled}); } db->getPriorityController()->saveSets(saveData); diff --git a/libcockatrice_models/libcockatrice/models/deck_list/CMakeLists.txt b/libcockatrice_models/libcockatrice/models/deck_list/CMakeLists.txt index 324bed102..a6ab2a204 100644 --- a/libcockatrice_models/libcockatrice/models/deck_list/CMakeLists.txt +++ b/libcockatrice_models/libcockatrice/models/deck_list/CMakeLists.txt @@ -16,5 +16,3 @@ target_include_directories(libcockatrice_models_deck_list PUBLIC ${CMAKE_CURRENT target_link_libraries( libcockatrice_models_deck_list PUBLIC libcockatrice_card libcockatrice_deck_list ${QT_CORE_MODULE} ) - -target_precompile_headers(libcockatrice_models_deck_list PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_network/libcockatrice/network/client/abstract/CMakeLists.txt b/libcockatrice_network/libcockatrice/network/client/abstract/CMakeLists.txt index 53ad524b4..6fba8d629 100644 --- a/libcockatrice_network/libcockatrice/network/client/abstract/CMakeLists.txt +++ b/libcockatrice_network/libcockatrice/network/client/abstract/CMakeLists.txt @@ -19,5 +19,3 @@ target_link_libraries( PUBLIC ${COCKATRICE_QT_VERSION_NAME}::Network ${COCKATRICE_QT_VERSION_NAME}::WebSockets libcockatrice_protocol libcockatrice_network_server_remote ) - -target_precompile_headers(libcockatrice_network_client_abstract PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_network/libcockatrice/network/client/local/CMakeLists.txt b/libcockatrice_network/libcockatrice/network/client/local/CMakeLists.txt index 08c12b7f8..2ac12e1fe 100644 --- a/libcockatrice_network/libcockatrice/network/client/local/CMakeLists.txt +++ b/libcockatrice_network/libcockatrice/network/client/local/CMakeLists.txt @@ -19,5 +19,3 @@ target_link_libraries( PUBLIC ${COCKATRICE_QT_VERSION_NAME}::Network ${COCKATRICE_QT_VERSION_NAME}::WebSockets libcockatrice_network_client_abstract ) - -target_precompile_headers(libcockatrice_network_client_local PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_network/libcockatrice/network/client/remote/CMakeLists.txt b/libcockatrice_network/libcockatrice/network/client/remote/CMakeLists.txt index c32ca0aa8..cb68d0c37 100644 --- a/libcockatrice_network/libcockatrice/network/client/remote/CMakeLists.txt +++ b/libcockatrice_network/libcockatrice/network/client/remote/CMakeLists.txt @@ -19,5 +19,3 @@ target_link_libraries( PUBLIC ${COCKATRICE_QT_VERSION_NAME}::Network ${COCKATRICE_QT_VERSION_NAME}::WebSockets libcockatrice_network_client_abstract libcockatrice_interfaces libcockatrice_utility libcockatrice_protocol ) - -target_precompile_headers(libcockatrice_network_client_remote PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_network/libcockatrice/network/server/local/CMakeLists.txt b/libcockatrice_network/libcockatrice/network/server/local/CMakeLists.txt index 020753d7f..494ae1294 100644 --- a/libcockatrice_network/libcockatrice/network/server/local/CMakeLists.txt +++ b/libcockatrice_network/libcockatrice/network/server/local/CMakeLists.txt @@ -15,5 +15,3 @@ add_dependencies(libcockatrice_network_server_local libcockatrice_protocol) target_include_directories(libcockatrice_network_server_local PUBLIC .) target_link_libraries(libcockatrice_network_server_local PUBLIC ${COCKATRICE_QT_MODULES} libcockatrice_protocol) - -target_precompile_headers(libcockatrice_network_server_local PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_network/libcockatrice/network/server/remote/CMakeLists.txt b/libcockatrice_network/libcockatrice/network/server/remote/CMakeLists.txt index 2127487d2..e11a962d1 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/CMakeLists.txt +++ b/libcockatrice_network/libcockatrice/network/server/remote/CMakeLists.txt @@ -3,7 +3,6 @@ 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 @@ -34,7 +33,6 @@ 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 @@ -64,5 +62,3 @@ target_link_libraries( libcockatrice_network_server_remote PUBLIC libcockatrice_protocol libcockatrice_utility libcockatrice_rng libcockatrice_deck_list ${COCKATRICE_QT_MODULES} ) - -target_precompile_headers(libcockatrice_network_server_remote PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") 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 deleted file mode 100644 index 4ecca95cb..000000000 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/dump_zone_range.cpp +++ /dev/null @@ -1,17 +0,0 @@ -#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 deleted file mode 100644 index a688b09d0..000000000 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/dump_zone_range.h +++ /dev/null @@ -1,29 +0,0 @@ -#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 c03f1e966..47c810d0d 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,6 +1,5 @@ #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" @@ -1436,23 +1435,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(cards.size()); + zoneInfo->set_card_count(numberCards < cards.size() ? cards.size() : numberCards); - for (qsizetype findId = cardRange.first; findId < cardRange.second; ++findId) { - Server_Card *card = cards.at(findId); + 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]; 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(static_cast(findId)); + cardInfo->set_id(findId); } else { cardInfo->set_id(card->getId()); cardInfo->set_x(card->getX()); diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.h b/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.h index ae20955bf..eff5034b4 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.h @@ -56,10 +56,6 @@ public: { return deck; } - bool getSideboardLocked() const - { - return sideboardLocked; - } bool getReadyStart() const { return readyStart; diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_game.cpp b/libcockatrice_network/libcockatrice/network/server/remote/game/server_game.cpp index b1863c91e..330d473b2 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_game.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_game.cpp @@ -61,8 +61,6 @@ #include #include #include -#include -#include #include #include #include @@ -827,48 +825,6 @@ void Server_Game::createGameJoinedEvent(Server_AbstractParticipant *joiningParti } rc.enqueuePostResponseItem(ServerMessage::GAME_EVENT_CONTAINER, prepareGameEvent(event2, -1)); - - // A newly joined player never saw the "Player X has loaded a deck" messages - // (they are only broadcast when a deck is selected), so replay a deck-select - // context for every player that already has a deck loaded. It is delivered - // privately to the joining participant, whose client renders it as a log - // entry in the same way as a fresh deck selection. Skipped on reconnect - // (resuming) — the player already saw these — and once the game has started, - // when the message is no longer useful and would only spam the log. - if (!resuming && !gameStarted) { - for (auto *participant : participants.values()) { - if (participant == joiningParticipant) { - continue; - } - auto *deckOwner = qobject_cast(participant); - if (deckOwner == nullptr) { - continue; - } - const DeckList *deckList = deckOwner->getDeckList(); - if (deckList == nullptr) { - continue; - } - - Event_PlayerPropertiesChanged event; - event.mutable_player_properties()->set_sideboard_locked(deckOwner->getSideboardLocked()); - event.mutable_player_properties()->set_deck_hash(deckList->getDeckHash().toStdString()); - - Context_DeckSelect deckSelect; - deckSelect.set_deck_hash(deckList->getDeckHash().toStdString()); - deckSelect.set_sideboard_size(deckList->getSideboardSize()); - if (getShareDecklistsOnLoad()) { - deckSelect.set_deck_list(deckList->writeToString_Native().toStdString()); - } - - GameEventContext context; - context.GetReflection() - ->MutableMessage(&context, deckSelect.GetDescriptor()->FindExtensionByName("ext")) - ->CopyFrom(deckSelect); - - rc.enqueuePostResponseItem(ServerMessage::GAME_EVENT_CONTAINER, - prepareGameEvent(event, deckOwner->getPlayerId(), &context)); - } - } } void Server_Game::sendGameEventContainer(GameEventContainer *cont, diff --git a/libcockatrice_network/libcockatrice/network/server/remote/server.h b/libcockatrice_network/libcockatrice/network/server/remote/server.h index 932a136ff..99247bb67 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/server.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/server.h @@ -187,39 +187,6 @@ public: virtual void observeGameStartDurationMs(qint64 /* elapsedMs */) { } - /** - * @brief Record a failed login attempt for the (address, username) pair; returns true if now locked out. - * - * Keyed by pair so a successful login as one user cannot wipe out another - * user's failure history from the same address (e.g. a guest login or the - * attacker's own account clearing the way for further guessing). - */ - virtual bool recordFailedLogin(const QString & /*ipAddress*/, const QString & /*userName*/) - { - return false; - } - /** - * @brief True if the (address, username) pair is already locked out of logging in. - * - * Consulted before any authentication work (database round trip, password - * verification) so a blocked pair cannot burn server CPU per attempt. - */ - virtual bool isLoginRateLimited(const QString & /*ipAddress*/, const QString & /*userName*/) - { - return false; - } - /** - * @brief Seconds remaining before the login rate limit for the (address, username) pair lifts. - * Returns 0 if not limited. - */ - virtual int loginRateLimitRemainingSeconds(const QString & /*ipAddress*/, const QString & /*userName*/) - { - return 0; - } - /** @brief Clear any failed-login lockout for the (address, username) pair, e.g. after a successful login. */ - virtual void clearFailedLogins(const QString & /*ipAddress*/, const QString & /*userName*/) - { - } Server_DatabaseInterface *getDatabaseInterface() const; int getNextLocalGameId() diff --git a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp index fbca9589a..fc5cfa148 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp @@ -592,17 +592,6 @@ Response::ResponseCode Server_ProtocolHandler::cmdLogin(const Command_Login &cmd QString reasonStr; int banSecondsLeft = 0; QString connectionType = getConnectionType(); - // Throttle before doing any work: a locked-out address must not get a full - // database round trip and password verification on every attempt. - if (server->isLoginRateLimited(getAddress(), userName)) { - int remaining = server->loginRateLimitRemainingSeconds(getAddress(), userName); - if (remaining > 0) { - auto *re = new Response_Login; - re->set_denied_end_time(QDateTime::currentDateTime().addSecs(remaining).toSecsSinceEpoch()); - rc.setResponseExtension(re); - } - return Response::RespTooManyRequests; - } AuthenticationResult res = server->loginUser(this, userName, password, needsHash, reasonStr, banSecondsLeft, clientId, clientVersion, connectionType); switch (res) { @@ -616,20 +605,10 @@ Response::ResponseCode Server_ProtocolHandler::cmdLogin(const Command_Login &cmd return Response::RespUserIsBanned; } case NotLoggedIn: - if (server->recordFailedLogin(getAddress(), userName)) { - int remaining = server->loginRateLimitRemainingSeconds(getAddress(), userName); - if (remaining > 0) { - auto *re = new Response_Login; - re->set_denied_end_time(QDateTime::currentDateTime().addSecs(remaining).toSecsSinceEpoch()); - rc.setResponseExtension(re); - } - return Response::RespTooManyRequests; - } return Response::RespWrongPassword; case WouldOverwriteOldSession: return Response::RespWouldOverwriteOldSession; case UsernameInvalid: { - server->recordFailedLogin(getAddress(), userName); auto *re = new Response_Login; re->set_denied_reason_str(reasonStr.toStdString()); rc.setResponseExtension(re); @@ -640,12 +619,10 @@ Response::ResponseCode Server_ProtocolHandler::cmdLogin(const Command_Login &cmd case ClientIdRequired: return Response::RespClientIdRequired; case UserIsInactive: - server->recordFailedLogin(getAddress(), userName); return Response::RespAccountNotActivated; case PasswordChangeRequired: return Response::RespPasswordChangeRequired; default: - server->clearFailedLogins(getAddress(), userName); authState = res; usingRealPassword = needsHash; } diff --git a/libcockatrice_protocol/CMakeLists.txt b/libcockatrice_protocol/CMakeLists.txt index 55184045a..7dc0a0360 100644 --- a/libcockatrice_protocol/CMakeLists.txt +++ b/libcockatrice_protocol/CMakeLists.txt @@ -24,5 +24,3 @@ target_include_directories( libcockatrice_protocol PUBLIC ${CMAKE_CURRENT_SOURCE_DIR} ${CMAKE_CURRENT_BINARY_DIR} # points to the generated headers ) - -target_precompile_headers(libcockatrice_protocol PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_protocol/libcockatrice/protocol/pb/CMakeLists.txt b/libcockatrice_protocol/libcockatrice/protocol/pb/CMakeLists.txt index 4e4f0e190..bc4814d5e 100644 --- a/libcockatrice_protocol/libcockatrice/protocol/pb/CMakeLists.txt +++ b/libcockatrice_protocol/libcockatrice/protocol/pb/CMakeLists.txt @@ -55,7 +55,6 @@ set(PROTO_FILES command_report_details.proto command_report_list.proto command_report_my_list.proto - command_report_reopen.proto command_report_resolve.proto command_report_stats.proto command_report_user_info.proto diff --git a/libcockatrice_protocol/libcockatrice/protocol/pb/command_report_reopen.proto b/libcockatrice_protocol/libcockatrice/protocol/pb/command_report_reopen.proto deleted file mode 100644 index 1f9c707e8..000000000 --- a/libcockatrice_protocol/libcockatrice/protocol/pb/command_report_reopen.proto +++ /dev/null @@ -1,10 +0,0 @@ -syntax = "proto2"; -import "moderator_commands.proto"; - -message Command_ReportReopen { - extend ModeratorCommand { - optional Command_ReportReopen ext = 1206; - } - required int32 report_id = 1; - optional string reopen_note = 2; -} diff --git a/libcockatrice_protocol/libcockatrice/protocol/pb/moderator_commands.proto b/libcockatrice_protocol/libcockatrice/protocol/pb/moderator_commands.proto index 5298d3dad..4f1e80c27 100644 --- a/libcockatrice_protocol/libcockatrice/protocol/pb/moderator_commands.proto +++ b/libcockatrice_protocol/libcockatrice/protocol/pb/moderator_commands.proto @@ -26,7 +26,6 @@ message ModeratorCommand { REPLAY_DOWNLOAD_BY_GAME_ID = 1203; REPORT_USER_INFO = 1204; REPORT_STATS = 1205; - REPORT_REOPEN = 1206; } extensions 100 to max; } diff --git a/libcockatrice_protocol/libcockatrice/protocol/pb/serverinfo_report.proto b/libcockatrice_protocol/libcockatrice/protocol/pb/serverinfo_report.proto index 3b624cc6d..a145fd855 100644 --- a/libcockatrice_protocol/libcockatrice/protocol/pb/serverinfo_report.proto +++ b/libcockatrice_protocol/libcockatrice/protocol/pb/serverinfo_report.proto @@ -33,7 +33,4 @@ message ServerInfo_Report { optional string chat_log = 14; optional string resolution_note = 15; - optional string resolved_by_name = 16; - - optional int32 reported_user_level = 17; } diff --git a/libcockatrice_rng/CMakeLists.txt b/libcockatrice_rng/CMakeLists.txt index 6128e3f6e..b988c9d2f 100644 --- a/libcockatrice_rng/CMakeLists.txt +++ b/libcockatrice_rng/CMakeLists.txt @@ -11,10 +11,6 @@ add_library( libcockatrice/rng/sfmt/SFMT.c ) -# SFMT is vendored and its own headers trip UBSan's alignment and vptr checks. -cockatrice_disable_sanitizers_for(libcockatrice/rng/sfmt/SFMT.c) -cockatrice_disable_coverage_for(libcockatrice/rng/sfmt/SFMT.c) - target_include_directories(libcockatrice_rng PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}) target_link_libraries(libcockatrice_rng PUBLIC ${QT_CORE_MODULE}) diff --git a/libcockatrice_settings/CMakeLists.txt b/libcockatrice_settings/CMakeLists.txt index 5a2fa42a9..80a465b6d 100644 --- a/libcockatrice_settings/CMakeLists.txt +++ b/libcockatrice_settings/CMakeLists.txt @@ -71,5 +71,3 @@ target_include_directories( ) target_link_libraries(libcockatrice_settings PUBLIC libcockatrice_card libcockatrice_utility ${QT_CORE_MODULE}) - -target_precompile_headers(libcockatrice_settings PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_settings/libcockatrice/settings/card_database_settings.cpp b/libcockatrice_settings/libcockatrice/settings/card_database_settings.cpp index 49bcb042b..acac1125f 100644 --- a/libcockatrice_settings/libcockatrice/settings/card_database_settings.cpp +++ b/libcockatrice_settings/libcockatrice/settings/card_database_settings.cpp @@ -110,7 +110,6 @@ void CardDatabaseSettings::saveSets(const QVector -#include #include class QObject; @@ -123,52 +121,3 @@ void PathsSettings::setSpoilerDatabasePath(const QString &_spoilerDatabasePath) setValue(_spoilerDatabasePath, "spoilerDatabase"); emit cardDatabasePathChanged(); } - -QStringList PathsSettings::pathsInsideDir(const QString &baseDir) const -{ - QStringList offenders; - const auto check = [&offenders, &baseDir](const QString &path) { - if (!path.isEmpty() && isInsideDir(path, baseDir)) { - offenders.append(QDir::cleanPath(path)); - } - }; - - check(getDeckPath()); - check(getFiltersPath()); - check(getReplaysPath()); - check(getPicsPath()); - check(getCustomPicsPath()); - check(getThemesPath()); - check(getCardDatabasePath()); - check(getCustomCardDatabasePath()); - check(getTokenDatabasePath()); - check(getSpoilerCardDatabasePath()); - check(getRedirectCachePath()); - - offenders.removeDuplicates(); - return offenders; -} - -bool PathsSettings::isInsideDir(const QString &path, const QString &baseDir) -{ -#if defined(Q_OS_WIN) || defined(Q_OS_MACOS) - // Windows and default macOS volumes are case-insensitive filesystems. - constexpr Qt::CaseSensitivity caseSensitivity = Qt::CaseInsensitive; -#else - constexpr Qt::CaseSensitivity caseSensitivity = Qt::CaseSensitive; -#endif - const QString base = QDir::cleanPath(baseDir); - const QString cleaned = QDir::cleanPath(path); - const QString prefix = base.endsWith(QLatin1Char('/')) ? base : base + QLatin1Char('/'); - return cleaned == base || cleaned.startsWith(prefix, caseSensitivity); -} - -bool PathsSettings::getAppDirWarningAcknowledged() const -{ - return getValue("appDirWarningAcknowledged").toBool(); -} - -void PathsSettings::setAppDirWarningAcknowledged(bool _appDirWarningAcknowledged) -{ - setValue(_appDirWarningAcknowledged, "appDirWarningAcknowledged"); -} diff --git a/libcockatrice_settings/libcockatrice/settings/paths_settings.h b/libcockatrice_settings/libcockatrice/settings/paths_settings.h index fa00428e3..399337bbb 100644 --- a/libcockatrice_settings/libcockatrice/settings/paths_settings.h +++ b/libcockatrice_settings/libcockatrice/settings/paths_settings.h @@ -4,7 +4,6 @@ #include "settings_manager.h" #include -#include #include #include @@ -28,24 +27,6 @@ public: [[nodiscard]] QString getSpoilerCardDatabasePath() const override; [[nodiscard]] QString getRedirectCachePath() const override; - /** - * @brief Returns the configured paths that resolve inside baseDir. - * - * Comparison is lexical (QDir::cleanPath); symlinks are not resolved, so a - * path that only reaches baseDir through a symlink is not reported. - */ - [[nodiscard]] QStringList pathsInsideDir(const QString &baseDir) const; - - /** - * @brief Whether path resolves inside baseDir, as a single-path check. - * - * A path counts as inside when it equals baseDir or is nested below it; - * a sibling sharing the name prefix (baseDir + "2") does not. - */ - [[nodiscard]] static bool isInsideDir(const QString &path, const QString &baseDir); - - [[nodiscard]] bool getAppDirWarningAcknowledged() const; - void setDeckPath(const QString &_deckPath); void setFiltersPath(const QString &_filtersPath); void setReplaysPath(const QString &_replaysPath); @@ -56,17 +37,14 @@ public: void setCustomCardDatabasePath(const QString &_customCardDatabasePath); void setTokenDatabasePath(const QString &_tokenDatabasePath); void setSpoilerDatabasePath(const QString &_spoilerDatabasePath); - void setAppDirWarningAcknowledged(bool _appDirWarningAcknowledged); signals: void cardDatabasePathChanged(); void picsPathChanged(); void themeChanged(); -public: - explicit PathsSettings(const QString &settingPath, QObject *parent = nullptr); - private: + explicit PathsSettings(const QString &settingPath, QObject *parent = nullptr); PathsSettings(const PathsSettings & /*other*/); }; diff --git a/libcockatrice_settings/libcockatrice/settings/settings_migration.cpp b/libcockatrice_settings/libcockatrice/settings/settings_migration.cpp index d9652cde3..c3f299f40 100644 --- a/libcockatrice_settings/libcockatrice/settings/settings_migration.cpp +++ b/libcockatrice_settings/libcockatrice/settings/settings_migration.cpp @@ -642,34 +642,6 @@ static void migrateLegacyMessages(const QString &settingsPath) legacySetting.endGroup(); } -/** - * gamefilters.ini keys renamed to camelCase by #7084. Profiles that migrated from the legacy - * native store before that rename still hold the old snake_case keys, which nothing reads - * anymore, so their saved filters silently stopped loading. - */ -static const QMap &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; @@ -680,7 +652,24 @@ static void migrateLegacyGameFilters(const QString &settingsPath) return; } - const QMap &filterKeyMap = renamedGameFilterKeys(); + 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"}, + }; QSettings filtersIni(settingsPath + "gamefilters.ini", QSettings::IniFormat); for (auto it = filterKeyMap.constBegin(); it != filterKeyMap.constEnd(); ++it) { @@ -697,68 +686,6 @@ 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); @@ -817,10 +744,6 @@ 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 0a706af98..78f9c8a3f 100644 --- a/libcockatrice_settings/libcockatrice/settings/settings_migration.h +++ b/libcockatrice_settings/libcockatrice/settings/settings_migration.h @@ -9,15 +9,6 @@ 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/libcockatrice_utility/CMakeLists.txt b/libcockatrice_utility/CMakeLists.txt index 5e6fa986d..d986214d7 100644 --- a/libcockatrice_utility/CMakeLists.txt +++ b/libcockatrice_utility/CMakeLists.txt @@ -49,5 +49,3 @@ target_link_libraries(libcockatrice_utility PUBLIC libcockatrice_rng OpenSSL::Cr set(ORACLE_LIBS) include_directories(${${COCKATRICE_QT_VERSION_NAME}Core_INCLUDE_DIRS}) - -target_precompile_headers(libcockatrice_utility PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/oracle/CMakeLists.txt b/oracle/CMakeLists.txt index 0df345812..0003b77ac 100644 --- a/oracle/CMakeLists.txt +++ b/oracle/CMakeLists.txt @@ -79,9 +79,6 @@ if(ZLIB_FOUND) include_directories(${ZLIB_INCLUDE_DIRS}) add_definitions("-DHAS_ZLIB") list(APPEND oracle_SOURCES src/zip/unzip.cpp src/zip/zipglobal.cpp) - # minizip and zlib are vendored. - cockatrice_disable_sanitizers_for(src/zip/unzip.cpp src/zip/zipglobal.cpp) - cockatrice_disable_coverage_for(src/zip/unzip.cpp src/zip/zipglobal.cpp) else() message(STATUS "Oracle: zlib not found; ZIP support disabled") endif() @@ -92,9 +89,6 @@ if(LIBLZMA_FOUND) include_directories(${LIBLZMA_INCLUDE_DIRS}) add_definitions("-DHAS_LZMA") list(APPEND oracle_SOURCES src/lzma/decompress.cpp) - # The LZMA SDK is vendored. - cockatrice_disable_sanitizers_for(src/lzma/decompress.cpp) - cockatrice_disable_coverage_for(src/lzma/decompress.cpp) else() message(STATUS "Oracle: LibLZMA not found; xz support disabled") endif() diff --git a/oracle/src/main.cpp b/oracle/src/main.cpp index bf56df5c9..cae2ee37d 100644 --- a/oracle/src/main.cpp +++ b/oracle/src/main.cpp @@ -85,8 +85,5 @@ int main(int argc, char *argv[]) QTimer::singleShot(0, &wizard, [&wizard]() { wizard.runInBackground(); }); } - const int execResult = app.exec(); - // A cancel only counts when nothing was written yet: closing the wizard - // after the card database was already saved on disk is still an update. - return (wizard.wasCancelled() && !wizard.hasSavedDatabase()) ? 1 : execResult; + return app.exec(); } diff --git a/oracle/src/oraclewizard.cpp b/oracle/src/oraclewizard.cpp index d2b3a71a4..05f08ce64 100644 --- a/oracle/src/oraclewizard.cpp +++ b/oracle/src/oraclewizard.cpp @@ -128,20 +128,12 @@ void OracleWizard::accept() void OracleWizard::reject() { - cancelled = true; - // The wizard is being closed while a page may still run a worker on the // importer. Ask it to stop before the wizard (and the importer child) is // destroyed, so the worker thread never touches freed memory. if (auto *active = dynamic_cast(currentPage())) { active->cancelWork(); } - - const auto replies = nam->findChildren(); - for (QNetworkReply *reply : replies) { - reply->abort(); - } - QWizard::reject(); } diff --git a/oracle/src/oraclewizard.h b/oracle/src/oraclewizard.h index b1dc5eab2..ea652bdc5 100644 --- a/oracle/src/oraclewizard.h +++ b/oracle/src/oraclewizard.h @@ -20,19 +20,6 @@ public: explicit OracleWizard(QWidget *parent = nullptr); void accept() override; void reject() override; - bool wasCancelled() - { - return cancelled; - } - /** @brief Records that the card database was written to disk, so a later cancel still counts as an update. */ - void markDatabaseSaved() - { - databaseSaved = true; - } - bool hasSavedDatabase() - { - return databaseSaved; - } void enableButtons(); void disableButtons(); void retranslateUi(); @@ -80,8 +67,6 @@ private: QByteArray tokensData; QString cardSourceUrl; QString cardSourceVersion; - bool cancelled = false; - bool databaseSaved = false; void migrateOracleSettings(); /** diff --git a/oracle/src/pages.cpp b/oracle/src/pages.cpp index ed8e04d1a..8c286fb46 100644 --- a/oracle/src/pages.cpp +++ b/oracle/src/pages.cpp @@ -484,10 +484,6 @@ void LoadSetsPage::actDownloadFinishedSetsFile() { // check for a reply auto *reply = dynamic_cast(sender()); - if (wizard()->wasCancelled()) { - reply->deleteLater(); - return; - } auto errorCode = reply->error(); if (errorCode != QNetworkReply::NoError) { QMessageBox::critical(this, tr("Error"), tr("Network error: %1.").arg(reply->errorString())); @@ -899,7 +895,6 @@ bool SaveSetsPage::validatePage() return false; } - wizard()->markDatabaseSaved(); return true; } diff --git a/oracle/src/pagetemplates.cpp b/oracle/src/pagetemplates.cpp index dcc8e070c..d37818dcb 100644 --- a/oracle/src/pagetemplates.cpp +++ b/oracle/src/pagetemplates.cpp @@ -178,10 +178,6 @@ void SimpleDownloadFilePage::actDownloadFinished() { // check for a reply auto *reply = dynamic_cast(sender()); - if (wizard()->wasCancelled()) { - reply->deleteLater(); - return; - } QNetworkReply::NetworkError errorCode = reply->error(); if (errorCode != QNetworkReply::NoError) { QMessageBox::critical(this, tr("Error"), tr("Network error: %1.").arg(reply->errorString())); @@ -246,7 +242,6 @@ bool SimpleDownloadFilePage::saveToFile() // clean saved downloadData downloadData = QByteArray(); - wizard()->markDatabaseSaved(); return true; } diff --git a/servatrice/CMakeLists.txt b/servatrice/CMakeLists.txt index f6d28d6c1..21f71a908 100644 --- a/servatrice/CMakeLists.txt +++ b/servatrice/CMakeLists.txt @@ -9,7 +9,6 @@ set(servatrice_SOURCES src/event_loop_watchdog.cpp src/main.cpp src/metrics_registry.cpp - src/ratelimiter.cpp src/servatrice.cpp src/servatrice_connection_pool.cpp src/servatrice_database_interface.cpp @@ -28,14 +27,6 @@ set(servatrice_SOURCES set(servatrice_RESOURCES servatrice.qrc) -# Qxt SMTP is vendored. -cockatrice_disable_sanitizers_for( - src/smtp/qxthmac.cpp src/smtp/qxtmailattachment.cpp src/smtp/qxtmailmessage.cpp src/smtp/qxtsmtp.cpp -) -cockatrice_disable_coverage_for( - src/smtp/qxthmac.cpp src/smtp/qxtmailattachment.cpp src/smtp/qxtmailmessage.cpp src/smtp/qxtsmtp.cpp -) - if(WIN32) set(servatrice_SOURCES ${servatrice_SOURCES} servatrice.rc) endif(WIN32) diff --git a/servatrice/servatrice.ini.example b/servatrice/servatrice.ini.example index f802f0244..c625705da 100644 --- a/servatrice/servatrice.ini.example +++ b/servatrice/servatrice.ini.example @@ -250,16 +250,6 @@ 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" @@ -366,27 +356,6 @@ max_users_websocket=500 ; Maximum number of users that can connect from the same IP address; useful to avoid bots, default is 4 max_users_per_address=4 -; Maximum number of failed login attempts from a single IP address before that -; address is temporarily locked out. Default is 5; set to 0 to disable. -max_login_attempts_per_ip=5 - -; Length in seconds of the sliding window used for the login lockout. Default is 900 (15 minutes). -login_attempt_window_seconds=900 - -; Maximum number of account registrations from a single IP address within the window below. -; Default is 2; set to 0 to disable. -max_registrations_per_ip=2 - -; Length in seconds of the registration window. Default is 3600 (1 hour). -registration_window_seconds=3600 - -; Maximum number of forgot-password requests from a single IP address within the window below. -; Default is 3; set to 0 to disable. -max_forgot_password_requests_per_ip=3 - -; Length in seconds of the forgot-password window. Default is 3600 (1 hour). -forgot_password_window_seconds=3600 - ; You may want to allow an unlimited number of users from a trusted source. This setting can contain a ; comma-separed list of IP addresses which will allow an unlimited number of connections from each of the ; IP addresses listed (ignoring the max_users_per_address). Default is "127.0.0.1,::1"; example: "192.73.233.244,81.4.100.74" diff --git a/servatrice/src/ratelimiter.cpp b/servatrice/src/ratelimiter.cpp deleted file mode 100644 index e7f209e38..000000000 --- a/servatrice/src/ratelimiter.cpp +++ /dev/null @@ -1,125 +0,0 @@ -#include "ratelimiter.h" - -#include - -bool RateLimiter::recordAttempt(const QString &key, int maxAttempts, int windowSeconds) -{ - return recordAttemptAt(key, maxAttempts, windowSeconds, QDateTime::currentSecsSinceEpoch()); -} - -bool RateLimiter::recordAttemptAt(const QString &key, int maxAttempts, int windowSeconds, qint64 now) -{ - if (maxAttempts <= 0 || windowSeconds <= 0) { - return false; // rate limiting disabled for this endpoint - } - - QMutexLocker locker(&mutex); - pruneLocked(now); - - AttemptHistory &history = attempts[key]; - history.windowSeconds = windowSeconds; - QList ×tamps = history.timestamps; - while (!timestamps.isEmpty() && timestamps.first() <= now - windowSeconds) { - timestamps.removeFirst(); - } - if (timestamps.size() >= maxAttempts) { - // Already at the limit: reject without appending so repeated attempts - // at the attacker's own pace cannot keep sliding the window forward - // and hold the lockout open forever. - return true; - } - - timestamps.append(now); - return false; -} - -bool RateLimiter::isBlocked(const QString &key, int maxAttempts, int windowSeconds) const -{ - return isBlockedAt(key, maxAttempts, windowSeconds, QDateTime::currentSecsSinceEpoch()); -} - -bool RateLimiter::isBlockedAt(const QString &key, int maxAttempts, int windowSeconds, qint64 now) const -{ - if (maxAttempts <= 0 || windowSeconds <= 0) { - return false; - } - - QMutexLocker locker(&mutex); - - const auto it = attempts.constFind(key); - if (it == attempts.constEnd()) { - return false; - } - - int count = 0; - for (const qint64 ×tamp : it.value().timestamps) { - if (timestamp > now - windowSeconds) { - ++count; - } - } - return count >= maxAttempts; -} - -void RateLimiter::clearAttempts(const QString &key) -{ - QMutexLocker locker(&mutex); - attempts.remove(key); -} - -int RateLimiter::retryAfterSeconds(const QString &key, int maxAttempts, int windowSeconds) const -{ - return retryAfterSecondsAt(key, maxAttempts, windowSeconds, QDateTime::currentSecsSinceEpoch()); -} - -int RateLimiter::retryAfterSecondsAt(const QString &key, int maxAttempts, int windowSeconds, qint64 now) const -{ - if (maxAttempts <= 0 || windowSeconds <= 0) { - return 0; - } - - QMutexLocker locker(&mutex); - - const auto it = attempts.constFind(key); - if (it == attempts.constEnd()) { - return 0; - } - - QList timestamps = it.value().timestamps; - while (!timestamps.isEmpty() && timestamps.first() <= now - windowSeconds) { - timestamps.removeFirst(); - } - if (timestamps.size() < maxAttempts) { - return 0; - } - if (timestamps.isEmpty()) { - return 0; - } - - qint64 oldest = timestamps.first(); - qint64 retryAt = oldest + windowSeconds; - if (retryAt <= now) { - return 0; - } - return static_cast(retryAt - now); -} - -void RateLimiter::pruneLocked(qint64 now) -{ - if (lastPruneSecs + PruneIntervalSeconds > now) { - return; - } - lastPruneSecs = now; - - auto it = attempts.begin(); - while (it != attempts.end()) { - QList ×tamps = it.value().timestamps; - while (!timestamps.isEmpty() && timestamps.first() <= now - it.value().windowSeconds) { - timestamps.removeFirst(); - } - if (timestamps.isEmpty()) { - it = attempts.erase(it); - } else { - ++it; - } - } -} diff --git a/servatrice/src/ratelimiter.h b/servatrice/src/ratelimiter.h deleted file mode 100644 index e2bc8fab6..000000000 --- a/servatrice/src/ratelimiter.h +++ /dev/null @@ -1,93 +0,0 @@ -#ifndef RATELIMITER_H -#define RATELIMITER_H - -#include -#include -#include -#include - -/** @brief Thread-safe per-key attempt limiter used to throttle abusive requests. */ -class RateLimiter -{ -public: - /** - * @brief Record an attempt for @p key and report whether the key is now blocked. - * - * A key that is already at its limit is not extended: the attempt is - * rejected without appending, so the lockout lifts once the recorded - * attempts age out of the window. Returns false immediately when - * @p maxAttempts <= 0 or @p windowSeconds <= 0 (rate limiting disabled), - * recording nothing. - * - * Delegates to recordAttemptAt() with the current wall-clock time. - */ - bool recordAttempt(const QString &key, int maxAttempts, int windowSeconds); - - /** - * @brief Record an attempt as seen at epoch second @p now. - * - * Exposed for deterministic tests; production callers should use - * recordAttempt(). - */ - bool recordAttemptAt(const QString &key, int maxAttempts, int windowSeconds, qint64 now); - - /** - * @brief True if @p key already has at least @p maxAttempts attempts within @p windowSeconds. - * - * Reads only; records nothing. Returns false when @p maxAttempts <= 0 or - * @p windowSeconds <= 0. Delegates to isBlockedAt(). - */ - bool isBlocked(const QString &key, int maxAttempts, int windowSeconds) const; - - /** - * @brief Whether @p key is blocked as measured at epoch second @p now. - * - * Exposed for deterministic tests; production callers should use - * isBlocked(). - */ - bool isBlockedAt(const QString &key, int maxAttempts, int windowSeconds, qint64 now) const; - - /** @brief Drop all recorded attempts for the key, e.g. after a successful login. */ - void clearAttempts(const QString &key); - - /** - * @brief Get the retry-after window in seconds for @p key. - * - * Returns the number of seconds from @p now until the oldest attempt - * exits @p windowSeconds and the key is no longer blocked. If the key is - * not blocked, returns 0. Returns 0 if @p maxAttempts <= 0 or - * @p windowSeconds <= 0. - * - * Delegates to retryAfterSecondsAt(). - */ - int retryAfterSeconds(const QString &key, int maxAttempts, int windowSeconds) const; - - /** - * @brief Get the retry-after window in seconds for @p key as measured at @p now. - * - * Exposed for deterministic tests. - */ - int retryAfterSecondsAt(const QString &key, int maxAttempts, int windowSeconds, qint64 now) const; - -private: - struct AttemptHistory - { - QList timestamps; // epoch seconds, oldest first - int windowSeconds; - }; - - /** - * @brief Drops keys whose last attempts have fully aged out of their window. - * - * Runs at most every PruneIntervalSeconds to stop the map from growing - * without bound when attackers rotate source addresses. - */ - void pruneLocked(qint64 now); - - mutable QMutex mutex; - QMap attempts; // key -> attempt history - qint64 lastPruneSecs = 0; - static constexpr int PruneIntervalSeconds = 60; -}; - -#endif diff --git a/servatrice/src/servatrice.cpp b/servatrice/src/servatrice.cpp index a832c1652..0329a31fd 100644 --- a/servatrice/src/servatrice.cpp +++ b/servatrice/src/servatrice.cpp @@ -38,7 +38,6 @@ #include #include #include -#include #include #include #include @@ -51,7 +50,6 @@ #include #include #include -#include #include #include #include @@ -71,8 +69,6 @@ #include #include -inline Q_LOGGING_CATEGORY(ServatriceLog, "servatrice"); - Servatrice_GameServer::Servatrice_GameServer(Servatrice *_server, int _numberPools, const QSqlDatabase &_sqlDatabase, @@ -339,11 +335,6 @@ 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 " @@ -746,36 +737,41 @@ void Servatrice::statusUpdate() return; } - // 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. + auto *queryDelete = + servatriceDatabaseInterface->prepareQuery("delete from {prefix}_activation_emails where name = :name"); + 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(); - smtpClient->enqueueActivationTokenMail(userName, emailAddress, token); + if (smtpClient->enqueueActivationTokenMail(userName, emailAddress, token)) { + queryDelete->bindValue(":name", userName); + servatriceDatabaseInterface->execSqlQuery(queryDelete); + } } } if (getEnableForgotPassword()) { auto *forgotPwQuery = servatriceDatabaseInterface->prepareQuery( - "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"); + "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"); if (!servatriceDatabaseInterface->execSqlQuery(forgotPwQuery)) { return; } - // 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 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(); + QSqlQuery *queryDelete = servatriceDatabaseInterface->prepareQuery( + "update {prefix}_forgot_password set emailed = 1 where name = :name"); - smtpClient->enqueueForgotPasswordTokenMail(userName, emailAddress, token, rowId); + 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(); + + if (smtpClient->enqueueForgotPasswordTokenMail(userName, emailAddress, token)) { + queryDelete->bindValue(":name", userName); + servatriceDatabaseInterface->execSqlQuery(queryDelete); + } } } @@ -783,69 +779,6 @@ 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; @@ -1287,105 +1220,6 @@ int Servatrice::getForgotPasswordTokenLife() const return settingsCache->value("forgotpassword/tokenlife", 60).toInt(); } -/** - * @brief Rate limiter key for one login target. - * - * Keyed by (address, username) so a success only clears that pair's history: - * a guest login or a login as the attacker's own account from the same - * address must not wipe out another user's failure count. Lowercased so - * case variations of the same name share one bucket. - */ -static QString loginLimitKey(const QString &ipAddress, const QString &userName) -{ - return QStringLiteral("login:") + ipAddress + QStringLiteral(":") + userName.toLower(); -} - -bool Servatrice::recordFailedLogin(const QString &ipAddress, const QString &userName) -{ - if (isRateLimitExempt(ipAddress)) { - return false; - } - return rateLimiter.recordAttempt(loginLimitKey(ipAddress, userName), getMaxLoginAttemptsPerIp(), - getLoginAttemptWindowSeconds()); -} - -bool Servatrice::isLoginRateLimited(const QString &ipAddress, const QString &userName) -{ - if (isRateLimitExempt(ipAddress)) { - return false; - } - return rateLimiter.isBlocked(loginLimitKey(ipAddress, userName), getMaxLoginAttemptsPerIp(), - getLoginAttemptWindowSeconds()); -} - -int Servatrice::loginRateLimitRemainingSeconds(const QString &ipAddress, const QString &userName) -{ - return rateLimiter.retryAfterSeconds(loginLimitKey(ipAddress, userName), getMaxLoginAttemptsPerIp(), - getLoginAttemptWindowSeconds()); -} - -bool Servatrice::recordRegistrationAttempt(const QString &ipAddress) -{ - if (isRateLimitExempt(ipAddress)) { - return false; - } - return rateLimiter.recordAttempt("register:" + ipAddress, getMaxRegistrationsPerIp(), - getRegistrationWindowSeconds()); -} - -bool Servatrice::recordForgotPasswordAttempt(const QString &ipAddress) -{ - if (isRateLimitExempt(ipAddress)) { - return false; - } - return rateLimiter.recordAttempt("forgot:" + ipAddress, getMaxForgotPasswordRequestsPerIp(), - getForgotPasswordWindowSeconds()); -} - -bool Servatrice::isRateLimitExempt(const QString &ipAddress) const -{ - // Mirrors the max_users_per_address bypass in initSession: trusted sources - // (e.g. 127.0.0.1 for Webatrice behind local nginx, where every websocket - // client shares one address) are not subject to per-address limits. - const QString trustedSources = settingsCache->value("security/trusted_sources", "127.0.0.1,::1").toString(); - return trustedSources.contains(ipAddress, Qt::CaseInsensitive); -} - -void Servatrice::clearFailedLogins(const QString &ipAddress, const QString &userName) -{ - rateLimiter.clearAttempts(loginLimitKey(ipAddress, userName)); -} - -int Servatrice::getMaxLoginAttemptsPerIp() const -{ - return settingsCache->value("security/max_login_attempts_per_ip", 5).toInt(); -} - -int Servatrice::getLoginAttemptWindowSeconds() const -{ - return settingsCache->value("security/login_attempt_window_seconds", 900).toInt(); -} - -int Servatrice::getMaxRegistrationsPerIp() const -{ - return settingsCache->value("security/max_registrations_per_ip", 2).toInt(); -} - -int Servatrice::getRegistrationWindowSeconds() const -{ - return settingsCache->value("security/registration_window_seconds", 3600).toInt(); -} - -int Servatrice::getMaxForgotPasswordRequestsPerIp() const -{ - return settingsCache->value("security/max_forgot_password_requests_per_ip", 3).toInt(); -} - -int Servatrice::getForgotPasswordWindowSeconds() const -{ - return settingsCache->value("security/forgot_password_window_seconds", 3600).toInt(); -} - bool Servatrice::getEnableForgotPasswordChallenge() const { return settingsCache->value("forgotpassword/enablechallenge", false).toBool(); diff --git a/servatrice/src/servatrice.h b/servatrice/src/servatrice.h index d5414f810..242d445ad 100644 --- a/servatrice/src/servatrice.h +++ b/servatrice/src/servatrice.h @@ -21,8 +21,6 @@ #define SERVATRICE_H #include "metrics_registry.h" -#include "ratelimiter.h" -#include "smtpclient.h" #include #include @@ -183,7 +181,6 @@ 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; @@ -205,8 +202,6 @@ private: std::shared_ptr reportStatsCache; static constexpr int reportStatsCacheTtlSeconds = 60; - RateLimiter rateLimiter; - mutable QMutex serverListMutex; QList serverList; void updateServerList(); @@ -236,12 +231,6 @@ 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); @@ -365,25 +354,6 @@ public: */ void watchWorkerThread(QThread *thread); - RateLimiter *getRateLimiter() - { - return &rateLimiter; - } - bool recordFailedLogin(const QString &ipAddress, const QString &userName) override; - bool isLoginRateLimited(const QString &ipAddress, const QString &userName) override; - int loginRateLimitRemainingSeconds(const QString &ipAddress, const QString &userName) override; - void clearFailedLogins(const QString &ipAddress, const QString &userName) override; - bool recordRegistrationAttempt(const QString &ipAddress); - bool recordForgotPasswordAttempt(const QString &ipAddress); - /// True when the address is in security/trusted_sources and therefore exempt from per-address limits. - bool isRateLimitExempt(const QString &ipAddress) const; - int getMaxLoginAttemptsPerIp() const; - int getLoginAttemptWindowSeconds() const; - int getMaxRegistrationsPerIp() const; - int getRegistrationWindowSeconds() const; - int getMaxForgotPasswordRequestsPerIp() const; - int getForgotPasswordWindowSeconds() const; - bool islConnectionExists(int _serverId) const; void addIslInterface(int _serverId, IslInterface *interface); void removeIslInterface(int _serverId); diff --git a/servatrice/src/serversocketinterface.cpp b/servatrice/src/serversocketinterface.cpp index bf8d02fc7..d46a9b0ee 100644 --- a/servatrice/src/serversocketinterface.cpp +++ b/servatrice/src/serversocketinterface.cpp @@ -110,7 +110,6 @@ #include #include #include -#include #include #include #include @@ -435,8 +434,6 @@ Response::ResponseCode AbstractServerSocketInterface::processExtendedModeratorCo return cmdReportAssign(cmd.GetExtension(Command_ReportAssign::ext), rc); case ModeratorCommand::REPORT_RESOLVE: return cmdReportResolve(cmd.GetExtension(Command_ReportResolve::ext), rc); - case ModeratorCommand::REPORT_REOPEN: - return cmdReportReopen(cmd.GetExtension(Command_ReportReopen::ext), rc); case ModeratorCommand::VIEWLOG_HISTORY: return cmdGetLogHistory(cmd.GetExtension(Command_ViewLogHistory::ext), rc, true); case ModeratorCommand::GRANT_REPLAY_ACCESS: @@ -1986,29 +1983,6 @@ Response::ResponseCode AbstractServerSocketInterface::cmdBanFromServer(const Com return Response::RespOk; } -namespace -{ -int reportUserLevel(bool userExists, int adminFlags) -{ - int userLevel = ServerInfo_User::IsUser; - if (userExists) { - userLevel |= ServerInfo_User::IsRegistered; - if (adminFlags & 1) { - userLevel |= ServerInfo_User::IsAdmin | ServerInfo_User::IsModerator; - } else if (adminFlags & 2) { - userLevel |= ServerInfo_User::IsModerator; - } - if (adminFlags & 4) { - userLevel |= ServerInfo_User::IsJudge; - } - if (adminFlags & 8) { - userLevel |= ServerInfo_User::IsDeveloper; - } - } - return userLevel; -} -} // namespace - Response::ResponseCode AbstractServerSocketInterface::cmdReportList(const Command_ReportList &cmd, ResponseContainer &rc) { @@ -2022,9 +1996,8 @@ Response::ResponseCode AbstractServerSocketInterface::cmdReportList(const Comman // Columns: 0=id, 1=reporter_name, 2=reported_user_name, 3=game_id, // 4=category, 5=description, 6=created_at, 7=status, - // 8=resolution_note, 9=assigned_mod_name, 10=resolved_by_name, - // 11=room_id, 12=replay_id, - // 13=reported_user_id, 14=reported_user_admin + // 8=resolution_note, 9=assigned_mod_name, + // 10=room_id, 11=replay_id QString whereClause; if (unresolvedOnly) { whereClause = "WHERE r.status = 'open' OR r.status = 'assigned' "; @@ -2039,13 +2012,11 @@ Response::ResponseCode AbstractServerSocketInterface::cmdReportList(const Comman QString queryStr = "SELECT r.id, r.reporter_name, r.reported_user_name, r.game_id, " "r.category, r.description, r.created_at, r.status, " - "r.resolution_note, u.name AS assigned_mod_name, ru.name AS resolved_by_name, r.room_id, " + "r.resolution_note, u.name AS assigned_mod_name, r.room_id, " "(SELECT id FROM {prefix}_replays WHERE id_game = r.game_id ORDER BY id DESC LIMIT 1) " - "AS replay_id, u2.id AS reported_user_id, u2.admin AS reported_user_admin " + "AS replay_id " "FROM {prefix}_reports r " - "LEFT JOIN {prefix}_users u ON r.assigned_to = u.id " - "LEFT JOIN {prefix}_users ru ON r.resolved_by = ru.id " - "LEFT JOIN {prefix}_users u2 ON u2.name = r.reported_user_name "; + "LEFT JOIN {prefix}_users u ON r.assigned_to = u.id "; if (unresolvedOnly) { queryStr += "WHERE r.status = 'open' OR r.status = 'assigned' "; @@ -2086,19 +2057,14 @@ Response::ResponseCode AbstractServerSocketInterface::cmdReportList(const Comman if (!query->value(9).isNull()) { info->set_assigned_mod_name(query->value(9).toString().toStdString()); } + if (!query->value(10).isNull()) { - info->set_resolved_by_name(query->value(10).toString().toStdString()); + info->set_room_id(query->value(10).toInt()); } if (!query->value(11).isNull()) { - info->set_room_id(query->value(11).toInt()); + info->set_replay_id(query->value(11).toInt()); } - - if (!query->value(12).isNull()) { - info->set_replay_id(query->value(12).toInt()); - } - - info->set_reported_user_level(reportUserLevel(!query->value(13).isNull(), query->value(14).toInt())); } rc.setResponseExtension(re); @@ -2771,13 +2737,11 @@ Response::ResponseCode AbstractServerSocketInterface::cmdReportMyList(const Comm QString queryStr = "SELECT r.id, r.reporter_name, r.reported_user_name, r.game_id, " "r.category, r.description, r.created_at, r.status, " - "r.resolution_note, u.name AS assigned_mod_name, ru.name AS resolved_by_name, r.room_id, " + "r.resolution_note, u.name AS assigned_mod_name, r.room_id, " "(SELECT id FROM {prefix}_replays WHERE id_game = r.game_id ORDER BY id DESC LIMIT 1) " - "AS replay_id, u2.id AS reported_user_id, u2.admin AS reported_user_admin " + "AS replay_id " "FROM {prefix}_reports r " "LEFT JOIN {prefix}_users u ON r.assigned_to = u.id " - "LEFT JOIN {prefix}_users ru ON r.resolved_by = ru.id " - "LEFT JOIN {prefix}_users u2 ON u2.name = r.reported_user_name " "WHERE r.reporter_id = :reporter_id " "ORDER BY r.created_at DESC LIMIT 200"; @@ -2813,61 +2777,20 @@ Response::ResponseCode AbstractServerSocketInterface::cmdReportMyList(const Comm if (!query->value(9).isNull()) { info->set_assigned_mod_name(query->value(9).toString().toStdString()); } + if (!query->value(10).isNull()) { - info->set_resolved_by_name(query->value(10).toString().toStdString()); + info->set_room_id(query->value(10).toInt()); } if (!query->value(11).isNull()) { - info->set_room_id(query->value(11).toInt()); + info->set_replay_id(query->value(11).toInt()); } - - if (!query->value(12).isNull()) { - info->set_replay_id(query->value(12).toInt()); - } - - info->set_reported_user_level(reportUserLevel(!query->value(13).isNull(), query->value(14).toInt())); } rc.setResponseExtension(re); return Response::RespOk; } -Response::ResponseCode AbstractServerSocketInterface::cmdReportReopen(const Command_ReportReopen &cmd, - ResponseContainer & /*rc*/) -{ - if (!sqlInterface->checkSql()) { - return Response::RespInternalError; - } - - int reportId = cmd.report_id(); - QString note = textFromStdString(cmd.reopen_note()); - - QSqlQuery *query = sqlInterface->prepareQuery("UPDATE {prefix}_reports " - "SET status = 'open', resolution_note = NULL, " - "resolution_time = NULL, resolved_by = NULL, assigned_to = NULL, " - "notified = 0 " - "WHERE id = :id AND status IN ('resolved', 'dismissed')"); - - query->bindValue(":id", reportId); - - if (!sqlInterface->execSqlQuery(query)) { - return Response::RespInternalError; - } - - if (query->numRowsAffected() == 0) { - return Response::RespInvalidData; - } - - QString auditDetails = QString("Report #%1 reopened").arg(reportId); - if (!note.isEmpty()) { - auditDetails += QString(". Reason: %1").arg(note.left(200)); - } - sqlInterface->addAuditRecord(QString::number(reportId), this->getAddress(), - QString::fromStdString(userInfo->clientid()), "REPORT_REOPENED", auditDetails, true); - - return Response::RespOk; -} - Response::ResponseCode AbstractServerSocketInterface::cmdReportDetails(const Command_ReportDetails &cmd, ResponseContainer &rc) { @@ -2883,19 +2806,17 @@ Response::ResponseCode AbstractServerSocketInterface::cmdReportDetails(const Com // Columns: 0=id, 1=reporter_id, 2=reporter_name, 3=reported_user_name, 4=game_id, // 5=category, 6=description, 7=created_at, 8=status, - // 9=resolution_note, 10=assigned_mod_name, 11=resolved_by_name, - // 12=chat_log, 13=room_id, 14=resolution_time, 15=replay_id - QString queryStr = - "SELECT r.id, r.reporter_id, r.reporter_name, r.reported_user_name, r.game_id, " - "r.category, r.description, r.created_at, r.status, " - "r.resolution_note, u.name AS assigned_mod_name, ru.name AS resolved_by_name, r.chat_log, r.room_id, " - "r.resolution_time, " - "(SELECT id FROM {prefix}_replays WHERE id_game = r.game_id ORDER BY id DESC LIMIT 1) " - "AS replay_id " - "FROM {prefix}_reports r " - "LEFT JOIN {prefix}_users u ON r.assigned_to = u.id " - "LEFT JOIN {prefix}_users ru ON r.resolved_by = ru.id " - "WHERE r.id = :id"; + // 9=resolution_note, 10=assigned_mod_name, 11=chat_log, + // 12=room_id, 13=resolution_time, 14=replay_id + QString queryStr = "SELECT r.id, r.reporter_id, r.reporter_name, r.reported_user_name, r.game_id, " + "r.category, r.description, r.created_at, r.status, " + "r.resolution_note, u.name AS assigned_mod_name, r.chat_log, r.room_id, " + "r.resolution_time, " + "(SELECT id FROM {prefix}_replays WHERE id_game = r.game_id ORDER BY id DESC LIMIT 1) " + "AS replay_id " + "FROM {prefix}_reports r " + "LEFT JOIN {prefix}_users u ON r.assigned_to = u.id " + "WHERE r.id = :id"; QSqlQuery *query = sqlInterface->prepareQuery(queryStr); query->bindValue(":id", reportId); @@ -2934,24 +2855,21 @@ Response::ResponseCode AbstractServerSocketInterface::cmdReportDetails(const Com if (!query->value(10).isNull()) { info->set_assigned_mod_name(query->value(10).toString().toStdString()); } + if (!query->value(11).isNull()) { - info->set_resolved_by_name(query->value(11).toString().toStdString()); + info->set_chat_log(query->value(11).toString().toStdString()); } if (!query->value(12).isNull()) { - info->set_chat_log(query->value(12).toString().toStdString()); + info->set_room_id(query->value(12).toInt()); } if (!query->value(13).isNull()) { - info->set_room_id(query->value(13).toInt()); + info->set_resolution_time(query->value(13).toDateTime().toSecsSinceEpoch()); } if (!query->value(14).isNull()) { - info->set_resolution_time(query->value(14).toDateTime().toSecsSinceEpoch()); - } - - if (!query->value(15).isNull()) { - info->set_replay_id(query->value(15).toInt()); + info->set_replay_id(query->value(14).toInt()); } QSqlQuery *commentQuery = @@ -3263,15 +3181,6 @@ Response::ResponseCode AbstractServerSocketInterface::cmdRegisterAccount(const C false); } - Response_Register *re = new Response_Register; - re->set_denied_reason_str("Too many registration attempts"); - int remaining = servatrice->getRateLimiter()->retryAfterSeconds("register:" + this->getAddress(), - servatrice->getMaxRegistrationsPerIp(), - servatrice->getRegistrationWindowSeconds()); - if (remaining > 0) { - re->set_denied_end_time(QDateTime::currentDateTime().addSecs(remaining).toSecsSinceEpoch()); - } - rc.setResponseExtension(re); return Response::RespTooManyRequests; } @@ -3339,7 +3248,9 @@ Response::ResponseCode AbstractServerSocketInterface::cmdRegisterAccount(const C bool AbstractServerSocketInterface::tooManyRegistrationAttempts(const QString &ipAddress) { - return servatrice->recordRegistrationAttempt(ipAddress); + //! \todo Implement registration attempt limiting. + Q_UNUSED(ipAddress); + return false; } Response::ResponseCode AbstractServerSocketInterface::cmdActivateAccount(const Command_Activate &cmd, @@ -3694,15 +3605,6 @@ Response::ResponseCode AbstractServerSocketInterface::cmdForgotPasswordRequest(c qCDebug(AbstractServerSocketInterfaceLog) << "Received reset password request from user:" << userName; - if (servatrice->recordForgotPasswordAttempt(this->getAddress())) { - if (servatrice->getEnableForgotPasswordAudit()) { - sqlInterface->addAuditRecord(userName.simplified(), this->getAddress(), clientId.simplified(), - "PASSWORD_RESET_REQUEST", "Too many requests from this ip address", false); - } - - return Response::RespTooManyRequests; - } - if (!servatrice->getEnableForgotPassword()) { if (servatrice->getEnableForgotPasswordAudit()) { sqlInterface->addAuditRecord(userName.simplified(), this->getAddress(), clientId.simplified(), @@ -3845,14 +3747,6 @@ AbstractServerSocketInterface::cmdForgotPasswordChallenge(const Command_ForgotPa qCDebug(AbstractServerSocketInterfaceLog) << "Received reset password challenge from user:" << userName; - if (servatrice->recordForgotPasswordAttempt(this->getAddress())) { - if (servatrice->getEnableForgotPasswordAudit()) { - sqlInterface->addAuditRecord(userName.simplified(), this->getAddress(), clientId.simplified(), - "PASSWORD_RESET_CHALLENGE", "Too many requests from this ip address", false); - } - return Response::RespTooManyRequests; - } - if (!servatrice->getEnableForgotPasswordChallenge()) { if (servatrice->getEnableForgotPasswordAudit()) { sqlInterface->addAuditRecord(userName.simplified(), this->getAddress(), clientId.simplified(), diff --git a/servatrice/src/serversocketinterface.h b/servatrice/src/serversocketinterface.h index 7de742c66..27708ed27 100644 --- a/servatrice/src/serversocketinterface.h +++ b/servatrice/src/serversocketinterface.h @@ -96,7 +96,6 @@ class Command_ReportAssign; class Command_ReportDetails; class Command_ReportList; class Command_ReportMyList; -class Command_ReportReopen; class Command_ReportResolve; class Command_ReportStats; class Command_ReportUserInfo; @@ -182,7 +181,6 @@ private: Response::ResponseCode cmdUpdateServerMessage(const Command_UpdateServerMessage &cmd, ResponseContainer &rc); Response::ResponseCode cmdReportAssign(const Command_ReportAssign &cmd, ResponseContainer &); Response::ResponseCode cmdReportResolve(const Command_ReportResolve &cmd, ResponseContainer &); - Response::ResponseCode cmdReportReopen(const Command_ReportReopen &cmd, ResponseContainer &); Response::ResponseCode cmdReportUserInfo(const Command_ReportUserInfo &cmd, ResponseContainer &rc); Response::ResponseCode cmdReportStats(const Command_ReportStats &cmd, ResponseContainer &rc); Response::ResponseCode cmdRegisterAccount(const Command_Register &cmd, ResponseContainer &rc); diff --git a/servatrice/src/smtp/qxtsmtp.cpp b/servatrice/src/smtp/qxtsmtp.cpp index a4da649f7..c81e0955c 100644 --- a/servatrice/src/smtp/qxtsmtp.cpp +++ b/servatrice/src/smtp/qxtsmtp.cpp @@ -94,38 +94,6 @@ 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 d264d1047..747e22df9 100644 --- a/servatrice/src/smtp/qxtsmtp.h +++ b/servatrice/src/smtp/qxtsmtp.h @@ -66,8 +66,6 @@ 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 82824e496..a45588b8b 100644 --- a/servatrice/src/smtpclient.cpp +++ b/servatrice/src/smtpclient.cpp @@ -7,56 +7,17 @@ #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 &))); @@ -79,151 +40,104 @@ SmtpClient::~SmtpClient() { if (smtp) { delete smtp; - smtp = nullptr; + smtp = 0; } } -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) { - if (pendingEmails.contains(pendingKey(nickname, EmailType::Activation, token))) { - qCDebug(SmtpClientLog) << "Activation email already pending for" << nickname << "- skipping"; - return false; - } - + QString email = settingsCache->value("smtp/email", "").toString(); + QString name = settingsCache->value("smtp/name", "").toString(); QString subject = settingsCache->value("smtp/subject", "").toString(); QString body = settingsCache->value("smtp/body", "").toString(); - QString email = settingsCache->value("smtp/email", "").toString(); - if (!validateEmailFields(email, subject, body, recipient, token, nickname)) { + if (email.isEmpty()) { + qDebug() << "[MAIL] Missing sender email in configuration"; return false; } - return enqueue(buildEmail(nickname, EmailType::Activation, subject, body, recipient, token, email)); + 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; } -bool SmtpClient::enqueueForgotPasswordTokenMail(const QString &nickname, - const QString &recipient, - const QString &token, - qint64 rowId) +bool SmtpClient::enqueueForgotPasswordTokenMail(const QString &nickname, const QString &recipient, const QString &token) { - if (pendingEmails.contains(pendingKey(nickname, EmailType::ForgotPassword, token))) { - qCDebug(SmtpClientLog) << "Password reset email already pending for" << nickname << "- skipping"; - return false; - } - + QString email = settingsCache->value("smtp/email", "").toString(); + QString name = settingsCache->value("smtp/name", "").toString(); QString subject = settingsCache->value("forgotpassword/subject", "").toString(); QString body = settingsCache->value("forgotpassword/body", "").toString(); - QString email = settingsCache->value("smtp/email", "").toString(); - if (!validateEmailFields(email, subject, body, recipient, token, nickname)) { + if (email.isEmpty()) { + qDebug() << "[MAIL] Missing sender email in configuration"; return false; } - return enqueue(buildEmail(nickname, EmailType::ForgotPassword, subject, body, recipient, token, email, rowId)); + 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; } void SmtpClient::sendAllEmails() { // still connected from the previous round - if (smtp->socket()->state() == QAbstractSocket::ConnectedState || - smtp->socket()->state() == QAbstractSocket::ConnectingState || - smtp->socket()->state() == QAbstractSocket::HostLookupState || - smtp->socket()->state() == QAbstractSocket::ClosingState) { + if (smtp->socket()->state() == QAbstractSocket::ConnectedState) { return; } - if (pendingEmails.isEmpty()) { - return; - } - - // Respect connection-level backoff after auth/conn/encryption failures. - if (connectionAttempts > 0 && QDateTime::currentDateTime() < connectionBackoffUntil) { + if (smtp->pendingMessages() == 0) { return; } @@ -237,16 +151,6 @@ 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) { @@ -258,286 +162,64 @@ void SmtpClient::sendAllEmails() } } -void SmtpClient::relayoutForSending() -{ - // 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(); - - 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() -{ - 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); + qDebug() << "[MAIL] authenticated"; } void SmtpClient::authenticationFailed(const QByteArray &msg) { - qCWarning(SmtpClientLog) << "authenticationFailed" << QString::fromUtf8(msg); - idleTimer->stop(); - recordConnectionFailure(); - mailIdToUser.clear(); - smtp->disconnectFromHost(); + qDebug() << "[MAIL] authenticationFailed" << QString(msg); } 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. + qDebug() << "[MAIL] connected"; } void SmtpClient::connectionFailed(const QByteArray &msg) { - qCWarning(SmtpClientLog) << "connectionFailed" << QString::fromUtf8(msg); - idleTimer->stop(); - recordConnectionFailure(); - mailIdToUser.clear(); - smtp->disconnectFromHost(); + qDebug() << "[MAIL] connectionFailed" << QString(msg); } 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(); + qDebug() << "[MAIL] disconnected"; } void SmtpClient::encrypted() { - qCDebug(SmtpClientLog) << "encrypted"; + qDebug() << "[MAIL] 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(); + qDebug() << "[MAIL] encryptionFailed" << QString(msg); + qDebug() << "[MAIL] Try enabling the \"acceptallcerts\" option in servatrice.ini"; +} + +void SmtpClient::finished() +{ + qDebug() << "[MAIL] finished"; 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) { - 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); + qDebug() << "[MAIL] mailFailed id=" << mailID << " errorCode=" << errorCode << "msg=" << QString(msg); } void SmtpClient::mailSent(int 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); + qDebug() << "[MAIL] mailSent" << mailID; } void SmtpClient::recipientRejected(int mailID, const QString &address, const QByteArray &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); - } + qDebug() << "[MAIL] recipientRejected id=" << mailID << " address=" << address << "msg=" << QString(msg); } void SmtpClient::senderRejected(int mailID, const QString &address, const QByteArray &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); - } + qDebug() << "[MAIL] senderRejected id=" << mailID << " address=" << address << "msg=" << QString(msg); } diff --git a/servatrice/src/smtpclient.h b/servatrice/src/smtpclient.h index 9fc11bf36..8fc4e35cb 100644 --- a/servatrice/src/smtpclient.h +++ b/servatrice/src/smtpclient.h @@ -1,67 +1,26 @@ #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 = 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); + SmtpClient(QObject *parent = 0); + ~SmtpClient(); 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); @@ -71,55 +30,10 @@ 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 +#endif \ No newline at end of file diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 784aac3be..2c288c6ce 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -2,10 +2,109 @@ # If a new test needs additional Qt modules, add them there — not in individual test CMakeLists.txt files. enable_testing() -include(GoogleTest) +add_test(NAME dummy_test COMMAND dummy_test) +add_test(NAME expression_test COMMAND expression_test) +add_test(NAME clamped_arithmetic_test COMMAND clamped_arithmetic_test) +add_test(NAME test_age_formatting COMMAND test_age_formatting) +add_test(NAME password_hash_test COMMAND password_hash_test) +add_test(NAME playmat_resolver_test COMMAND playmat_resolver_test) +add_test(NAME server_card_counter_test COMMAND server_card_counter_test) +add_test(NAME server_counter_test COMMAND server_counter_test) +add_test(NAME server_rate_limiter_test COMMAND server_rate_limiter_test) +add_test(NAME server_developer_role_test COMMAND server_developer_role_test) +add_test(NAME server_game_join_test COMMAND server_game_join_test) +add_test(NAME warning_categories_test COMMAND warning_categories_test) +# The translation loader test compiles a .qm fixture, which needs lrelease from Qt Linguist. +if(TARGET Qt6::lrelease) + add_test(NAME translation_loader_test COMMAND translation_loader_test) +endif() +add_test(NAME report_categories_test COMMAND report_categories_test) +add_test(NAME lag_monitor_test COMMAND lag_monitor_test) +add_test(NAME latency_tracker_test COMMAND latency_tracker_test) +add_test(NAME metrics_registry_test COMMAND metrics_registry_test) +add_test(NAME card_update_progress_test COMMAND card_update_progress_test) +add_test(NAME loader_local_matching_test COMMAND loader_local_matching_test) + +add_test(NAME deck_hash_performance_test COMMAND deck_hash_performance_test) +set_tests_properties(deck_hash_performance_test PROPERTIES TIMEOUT 15) # Find GTest +add_executable(dummy_test dummy_test.cpp) +add_executable(expression_test expression_test.cpp) +add_executable(clamped_arithmetic_test clamped_arithmetic_test.cpp) +add_executable(test_age_formatting test_age_formatting.cpp) +add_executable(password_hash_test password_hash_test.cpp) +add_executable(playmat_resolver_test playmat_resolver_test.cpp) +add_executable(deck_hash_performance_test deck_hash_performance_test.cpp) +add_executable(server_card_counter_test server_card_counter_test.cpp) +add_executable(server_counter_test server_counter_test.cpp) +add_executable(server_rate_limiter_test server_rate_limiter_test.cpp) +add_executable(server_developer_role_test server_developer_role_test.cpp) +add_executable(server_game_join_test server_game_join_test.cpp) +add_executable(warning_categories_test warning_categories_test.cpp) +if(TARGET Qt6::lrelease) + add_executable(translation_loader_test translation_loader_test.cpp) + + # QTranslator only accepts real .qm files, so the translation loader test gets one compiled from its + # own .ts fixture rather than a stub. + set(TRANSLATION_LOADER_TEST_QM "${CMAKE_CURRENT_BINARY_DIR}/translation_loader_test_de.qm") + add_custom_command( + OUTPUT "${TRANSLATION_LOADER_TEST_QM}" + COMMAND Qt6::lrelease "${CMAKE_CURRENT_SOURCE_DIR}/translation_loader_test.ts" -qm "${TRANSLATION_LOADER_TEST_QM}" + DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/translation_loader_test.ts" + COMMENT "Compiling translation_loader_test.ts" + ) + add_custom_target(translation_loader_test_qm DEPENDS "${TRANSLATION_LOADER_TEST_QM}") + add_dependencies(translation_loader_test translation_loader_test_qm) + target_compile_definitions(translation_loader_test PRIVATE TRANSLATION_LOADER_TEST_DIR="${CMAKE_CURRENT_BINARY_DIR}") + + # A multi-config generator (MSBuild, Xcode) puts the executable in a per-config subdirectory while + # the .qm stays in the current binary directory, so copy the fixture next to the binary: that is the + # layout ABuildTreeFindsTheTranslationsNextToTheExecutable is about, and the only one it finds on + # those generators. + add_custom_command( + TARGET translation_loader_test + POST_BUILD + COMMAND ${CMAKE_COMMAND} -E copy_if_different "${TRANSLATION_LOADER_TEST_QM}" + "$/" + VERBATIM + ) +endif() +add_executable(report_categories_test report_categories_test.cpp) +add_executable(lag_monitor_test ${CMAKE_SOURCE_DIR}/cockatrice/src/client/lag_monitor.cpp lag_monitor_test.cpp) +target_include_directories(lag_monitor_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src) +add_executable(latency_tracker_test latency_tracker_test.cpp) +add_executable(metrics_registry_test ../servatrice/src/metrics_registry.cpp metrics_registry_test.cpp) +add_executable( + card_update_progress_test ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/card_database_update/card_update_progress.cpp + card_update_progress_test.cpp +) +target_include_directories(card_update_progress_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src) +add_executable( + loader_local_matching_test + ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp + ${CMAKE_SOURCE_DIR}/cockatrice/src/client/settings/cache_settings.cpp + ${CMAKE_SOURCE_DIR}/cockatrice/src/client/settings/card_counter_settings.cpp + ${CMAKE_SOURCE_DIR}/cockatrice/src/client/settings/shortcuts_settings.cpp + ${CMAKE_SOURCE_DIR}/cockatrice/src/client/network/update/client/release_channel.cpp + ${VERSION_STRING_CPP} + loader_local_matching_test.cpp +) +target_include_directories(loader_local_matching_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src) + +# Guard must match the condition for src/single_instance_manager.cpp in cockatrice/CMakeLists.txt. +if(WITH_CLIENT) + add_executable( + single_instance_manager_test ${CMAKE_SOURCE_DIR}/cockatrice/src/single_instance_manager.cpp + single_instance_manager_test.cpp + ) + target_include_directories(single_instance_manager_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src) + target_include_directories(single_instance_manager_test PRIVATE ${CMAKE_SOURCE_DIR}/libcockatrice_utility) + + add_test(NAME single_instance_manager_test COMMAND single_instance_manager_test) +endif() + find_package(GTest) if(NOT GTEST_FOUND) @@ -30,364 +129,93 @@ if(NOT GTEST_FOUND) target_include_directories(gtest INTERFACE "$") set(GTEST_INCLUDE_DIRS "${CMAKE_BINARY_DIR}/gtest-src/include") - # gtest_main is not optional here. FindGTest sets GTEST_BOTH_LIBRARIES to both halves, so this - # has to match it or the two paths disagree about what provides main(). A test source that - # defines its own main() gets away with the omission; one that does not fails at link with - # `undefined reference to main` pointing at crt1.o, which names neither gtest nor this line. - set(GTEST_BOTH_LIBRARIES gtest gtest_main) + set(GTEST_BOTH_LIBRARIES gtest) + add_dependencies(dummy_test gtest) + add_dependencies(expression_test gtest) + add_dependencies(clamped_arithmetic_test gtest) + add_dependencies(test_age_formatting gtest) + add_dependencies(password_hash_test gtest) + add_dependencies(playmat_resolver_test gtest) + add_dependencies(deck_hash_performance_test gtest) + add_dependencies(server_card_counter_test gtest) + add_dependencies(server_counter_test gtest) + add_dependencies(server_rate_limiter_test gtest) + add_dependencies(server_developer_role_test gtest) + add_dependencies(server_game_join_test gtest) + add_dependencies(warning_categories_test gtest) + if(TARGET translation_loader_test) + add_dependencies(translation_loader_test gtest) + endif() + add_dependencies(report_categories_test gtest) + add_dependencies(lag_monitor_test gtest) + add_dependencies(latency_tracker_test gtest) + add_dependencies(metrics_registry_test gtest) + add_dependencies(card_update_progress_test gtest) + add_dependencies(loader_local_matching_test gtest) + add_dependencies(single_instance_manager_test gtest) endif() -# Directory-scoped on purpose: the test subdirectories below add targets that include the -# fetched gtest headers and do not repeat this path themselves. include_directories(${GTEST_INCLUDE_DIRS}) - -# --- Test registration ---------------------------------------------------------- -# -# Every test target needs the same four steps: build the executable, depend on gtest when it -# was fetched from source, link the shared test libraries, and hand the cases to CTest. The -# `if(NOT GTEST_FOUND)` branch is the easy one to forget — omit it and the link fails with -# undefined gtest symbols — so it lives here once instead of being repeated per test. -# -# This also replaces `add_test(NAME x COMMAND x)`. Naming the binary as a single CTest entry -# hides which case failed and makes `ctest -R Suite.Case` impossible; discovery exposes each -# TEST() as its own CTest entry instead. -# -# Labels: exactly one per test, because gtest_discover_tests() cannot carry more than one. -# GoogleTestAddTests.cmake accumulates the generated script with `foreach(arg ${ARGN})`, so a -# label list serialises as bare `LABELS ci server` in the CTestTestfile, and CTest reads only -# the first value of a property and silently discards the rest. A label therefore classifies a -# test into exactly one group, and the three groups cover every selection worth making: -# -# ci the default hermetic set, so `ctest -LE bench` runs everything but benchmarks -# server servatrice-facing tests, so `ctest -L server` works -# bench the slow suites, kept out of the default run -# -# A second label is a configure-time FATAL_ERROR rather than a line CTest quietly reads as one. -# Anything orthogonal to a test's kind belongs in a helper flag beside the label, so it -# composes with the group instead of competing for the single slot. NO_SANITIZE is such a flag. -# -# TIMEOUT is bounded per test because CTest's implicit default is 1500s: one hung socket wait -# would otherwise stall CI for 25 minutes. -# -# EXTRA_LIBS is appended after the shared libraries so a caller adding a static archive (zlib, -# lzma) keeps it last, which is where the linker needs it. - -function(cockatrice_add_test) - cmake_parse_arguments( - PARSE_ARGV 0 ARG "MANUAL;NO_SANITIZE" "NAME" "SOURCES;LIBS;LABELS;DEFINITIONS;INCLUDE_DIRS;EXTRA_LIBS" - ) - - if(NOT ARG_NAME OR NOT ARG_SOURCES) - message(FATAL_ERROR "cockatrice_add_test requires at least NAME and SOURCES") - endif() - - add_executable(${ARG_NAME} ${ARG_SOURCES}) - - if(ARG_INCLUDE_DIRS) - target_include_directories(${ARG_NAME} PRIVATE ${ARG_INCLUDE_DIRS}) - endif() - - if(ARG_DEFINITIONS) - target_compile_definitions(${ARG_NAME} PRIVATE ${ARG_DEFINITIONS}) - endif() - +target_link_libraries(dummy_test Threads::Threads ${GTEST_BOTH_LIBRARIES}) +target_link_libraries(expression_test libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES}) +target_link_libraries( + clamped_arithmetic_test libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} +) +target_link_libraries( + test_age_formatting libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} +) +target_link_libraries( + password_hash_test libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} +) +target_link_libraries( + playmat_resolver_test libcockatrice_deck_list libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} + ${TEST_QT_MODULES} +) +target_link_libraries( + deck_hash_performance_test libcockatrice_deck_list libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} + ${TEST_QT_MODULES} +) +target_link_libraries( + server_card_counter_test libcockatrice_network Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} +) +target_link_libraries( + server_counter_test libcockatrice_network Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} +) +target_link_libraries( + server_rate_limiter_test libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} +) +target_link_libraries( + server_developer_role_test libcockatrice_network libcockatrice_rng Threads::Threads ${GTEST_BOTH_LIBRARIES} + ${TEST_QT_MODULES} +) +target_link_libraries( + server_game_join_test libcockatrice_network_server_remote libcockatrice_rng Threads::Threads ${GTEST_BOTH_LIBRARIES} + ${TEST_QT_MODULES} +) +target_link_libraries( + warning_categories_test libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} +) +if(TARGET translation_loader_test) target_link_libraries( - ${ARG_NAME} ${ARG_LIBS} Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} ${ARG_EXTRA_LIBS} - ) - - # Only needed when gtest was downloaded and add_subdirectory'd above. An imported gtest - # target already carries its own dependency, so this must stay conditional. - if(NOT GTEST_FOUND) - add_dependencies(${ARG_NAME} gtest gtest_main) - endif() - - # MANUAL targets are built but deliberately kept out of ctest: benchmarks that need a card - # database, hit the network, or measure RAM. Leaving them unregistered keeps CI hermetic - # and fast. They still link gtest so they can use TEST() without a second code path. - if(ARG_MANUAL) - return() - endif() - - # NO_SANITIZE marks a target the sanitizer leg cannot run. Excluded by not handing its cases - # to CTest at all, rather than by a label, so the exclusion cannot rot into a line CTest - # quietly reads as something else. The target still builds, and still runs on the other legs. - if(ARG_NO_SANITIZE AND COCKATRICE_SANITIZERS) - return() - endif() - - # One label only, per the note above. Rejecting a second loudly beats emitting a - # CTestTestfile line that CTest will quietly read as one label. - list(LENGTH ARG_LABELS _label_count) - if(_label_count GREATER 1) - message(FATAL_ERROR "${ARG_NAME}: pass at most one label, got '${ARG_LABELS}'") - endif() - if(NOT ARG_LABELS) - set(ARG_LABELS ci) - endif() - - # GoogleTestAddTests defaults DISCOVERY_TIMEOUT to 5s, which is ample for a plain debug - # binary (--gtest_list_tests costs 0.04s) but not for an instrumented one (~2.8s cold, and - # discovery runs several binaries concurrently behind a -j build). Left at the default, a - # sanitizer build fails at the *link* step with a bare "Process terminated due to timeout" - # and no hint that discovery, not the test, is what is slow. - if(COCKATRICE_SANITIZERS) - set(_discovery_timeout 300) - else() - set(_discovery_timeout 60) - endif() - - gtest_discover_tests( - ${ARG_NAME} DISCOVERY_MODE POST_BUILD DISCOVERY_TIMEOUT ${_discovery_timeout} - PROPERTIES LABELS "${ARG_LABELS}" - TIMEOUT 60 - ) -endfunction() - -# Same wiring for targets that are built but deliberately kept out of ctest: benchmarks that -# need a card database, hit the network, or measure RAM. A separate name rather than a MANUAL -# flag spelled out at each call site, so "this is not in the CI suite" is visible in the CMake -# file. Note the unquoted expansions: an unset multi-value keyword expands to zero arguments, -# which is what lets this forward a caller's keywords verbatim without tripping the FATAL_ERROR -# guard above on an empty list. -function(cockatrice_add_manual_test) - cmake_parse_arguments(PARSE_ARGV 0 ARG "" "NAME" "SOURCES;LIBS;DEFINITIONS;INCLUDE_DIRS;EXTRA_LIBS") - - cockatrice_add_test( - NAME - ${ARG_NAME} - SOURCES - ${ARG_SOURCES} - LIBS - ${ARG_LIBS} - DEFINITIONS - ${ARG_DEFINITIONS} - INCLUDE_DIRS - ${ARG_INCLUDE_DIRS} - EXTRA_LIBS - ${ARG_EXTRA_LIBS} - MANUAL - ) -endfunction() - -# --- Unit tests ------------------------------------------------------------------ - -cockatrice_add_test(NAME expression_test SOURCES expression_test.cpp LIBS libcockatrice_utility) -cockatrice_add_test(NAME clamped_arithmetic_test SOURCES clamped_arithmetic_test.cpp LIBS libcockatrice_utility) -cockatrice_add_test(NAME test_age_formatting SOURCES test_age_formatting.cpp LIBS libcockatrice_utility) -cockatrice_add_test(NAME password_hash_test SOURCES password_hash_test.cpp LIBS libcockatrice_utility) -cockatrice_add_test(NAME report_categories_test SOURCES report_categories_test.cpp LIBS libcockatrice_utility) -cockatrice_add_test(NAME warning_categories_test SOURCES warning_categories_test.cpp LIBS libcockatrice_utility) -cockatrice_add_test( - NAME - playmat_resolver_test - SOURCES - playmat_resolver_test.cpp - LIBS - libcockatrice_deck_list - libcockatrice_utility -) -cockatrice_add_test( - NAME - server_rate_limiter_test - SOURCES - server_rate_limiter_test.cpp - LIBS - libcockatrice_utility - LABELS - server -) -cockatrice_add_test( - NAME - rate_limiter_test - SOURCES - ${CMAKE_SOURCE_DIR}/servatrice/src/ratelimiter.cpp - rate_limiter_test.cpp - INCLUDE_DIRS - ${CMAKE_SOURCE_DIR}/servatrice/src - LABELS - server -) -cockatrice_add_test( - NAME - server_card_counter_test - SOURCES - server_card_counter_test.cpp - LIBS - libcockatrice_network - 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 - SOURCES - server_counter_test.cpp - LIBS - libcockatrice_network - LABELS - server -) -cockatrice_add_test( - NAME - server_developer_role_test - NO_SANITIZE - SOURCES - server_developer_role_test.cpp - LIBS - libcockatrice_network - libcockatrice_rng - LABELS - server -) -cockatrice_add_test( - NAME - server_moderator_role_test - NO_SANITIZE - SOURCES - server_moderator_role_test.cpp - LIBS - libcockatrice_network - libcockatrice_rng - LABELS - server -) -cockatrice_add_test( - NAME - server_game_join_test - SOURCES - server_game_join_test.cpp - LIBS - libcockatrice_network_server_remote - libcockatrice_rng - LABELS - server -) -cockatrice_add_test( - NAME - latency_tracker_test - SOURCES - latency_tracker_test.cpp - LIBS - libcockatrice_network - LABELS - server -) -cockatrice_add_test( - NAME - lag_monitor_test - SOURCES - ${CMAKE_SOURCE_DIR}/cockatrice/src/client/lag_monitor.cpp - lag_monitor_test.cpp - INCLUDE_DIRS - ${CMAKE_SOURCE_DIR}/cockatrice/src -) -cockatrice_add_test( - NAME - metrics_registry_test - SOURCES - ${CMAKE_SOURCE_DIR}/servatrice/src/metrics_registry.cpp - metrics_registry_test.cpp - INCLUDE_DIRS - ${CMAKE_SOURCE_DIR}/servatrice/src - LABELS - server -) -cockatrice_add_test( - NAME - card_update_progress_test - SOURCES - ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/card_database_update/card_update_progress.cpp - card_update_progress_test.cpp - INCLUDE_DIRS - ${CMAKE_SOURCE_DIR}/cockatrice/src -) -cockatrice_add_test( - NAME - loader_local_matching_test - SOURCES - ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp - ${CMAKE_SOURCE_DIR}/cockatrice/src/client/settings/cache_settings.cpp - ${CMAKE_SOURCE_DIR}/cockatrice/src/client/settings/card_counter_settings.cpp - ${CMAKE_SOURCE_DIR}/cockatrice/src/client/settings/shortcuts_settings.cpp - ${CMAKE_SOURCE_DIR}/cockatrice/src/client/network/update/client/release_channel.cpp - ${VERSION_STRING_CPP} - loader_local_matching_test.cpp - LIBS - libcockatrice_settings - INCLUDE_DIRS - ${CMAKE_SOURCE_DIR}/cockatrice/src -) -cockatrice_add_test( - NAME - deck_hash_performance_test - SOURCES - deck_hash_performance_test.cpp - LIBS - libcockatrice_deck_list - libcockatrice_utility - LABELS - bench -) - -# The translation loader test compiles a .qm fixture, which needs lrelease from Qt Linguist. -if(TARGET Qt6::lrelease) - set(TRANSLATION_LOADER_TEST_QM "${CMAKE_CURRENT_BINARY_DIR}/translation_loader_test_de.qm") - add_custom_command( - OUTPUT "${TRANSLATION_LOADER_TEST_QM}" - COMMAND Qt6::lrelease "${CMAKE_CURRENT_SOURCE_DIR}/translation_loader_test.ts" -qm "${TRANSLATION_LOADER_TEST_QM}" - DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/translation_loader_test.ts" - COMMENT "Compiling translation_loader_test.ts" - ) - add_custom_target(translation_loader_test_qm DEPENDS "${TRANSLATION_LOADER_TEST_QM}") - - cockatrice_add_test( - NAME - translation_loader_test - SOURCES - translation_loader_test.cpp - LIBS - libcockatrice_utility - DEFINITIONS - TRANSLATION_LOADER_TEST_DIR="${CMAKE_CURRENT_BINARY_DIR}" - ) - add_dependencies(translation_loader_test translation_loader_test_qm) - - # A multi-config generator (MSBuild, Xcode) puts the executable in a per-config subdirectory while - # the .qm stays in the current binary directory, so copy the fixture next to the binary: that is the - # layout ABuildTreeFindsTheTranslationsNextToTheExecutable is about, and the only one it finds on - # those generators. - add_custom_command( - TARGET translation_loader_test - POST_BUILD - COMMAND ${CMAKE_COMMAND} -E copy_if_different "${TRANSLATION_LOADER_TEST_QM}" - "$/" - VERBATIM + translation_loader_test libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} ) endif() +target_link_libraries( + report_categories_test libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} +) +target_link_libraries(lag_monitor_test Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES}) +target_link_libraries( + latency_tracker_test libcockatrice_network Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} +) +target_include_directories(metrics_registry_test PRIVATE ${CMAKE_SOURCE_DIR}/servatrice/src) +target_link_libraries(metrics_registry_test ${TEST_QT_MODULES} Threads::Threads ${GTEST_BOTH_LIBRARIES}) +target_link_libraries(card_update_progress_test Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES}) +target_link_libraries( + loader_local_matching_test libcockatrice_settings Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} +) -# Guard must match the condition for src/single_instance_manager.cpp in cockatrice/CMakeLists.txt. if(WITH_CLIENT) - cockatrice_add_test( - NAME - single_instance_manager_test - SOURCES - ${CMAKE_SOURCE_DIR}/cockatrice/src/single_instance_manager.cpp - single_instance_manager_test.cpp - INCLUDE_DIRS - ${CMAKE_SOURCE_DIR}/cockatrice/src - ${CMAKE_SOURCE_DIR}/libcockatrice_utility - ) + target_link_libraries(single_instance_manager_test Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES}) endif() add_subdirectory(card_zone_algorithms) diff --git a/tests/card_zone_algorithms/CMakeLists.txt b/tests/card_zone_algorithms/CMakeLists.txt index 945f8a0ed..e03d8f77d 100644 --- a/tests/card_zone_algorithms/CMakeLists.txt +++ b/tests/card_zone_algorithms/CMakeLists.txt @@ -1,4 +1,15 @@ -cockatrice_add_test( - NAME card_zone_algorithms_test SOURCES card_zone_algorithms_test.cpp INCLUDE_DIRS - ${CMAKE_SOURCE_DIR}/cockatrice/src/game/zones +add_executable(card_zone_algorithms_test card_zone_algorithms_test.cpp) + +target_include_directories(card_zone_algorithms_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src/game/zones) + +target_link_libraries( + card_zone_algorithms_test + PRIVATE Threads::Threads + PRIVATE ${GTEST_BOTH_LIBRARIES} ) + +add_test(NAME card_zone_algorithms_test COMMAND card_zone_algorithms_test) + +if(NOT GTEST_FOUND) + add_dependencies(card_zone_algorithms_test gtest) +endif() diff --git a/tests/carddatabase/CMakeLists.txt b/tests/carddatabase/CMakeLists.txt index 22171a9e8..811cebc1d 100644 --- a/tests/carddatabase/CMakeLists.txt +++ b/tests/carddatabase/CMakeLists.txt @@ -9,46 +9,61 @@ add_definitions("-DCARDDB_DATADIR=\"${CMAKE_CURRENT_SOURCE_DIR}/data/\"") # ------------------------ # Card Database Test # ------------------------ -cockatrice_add_test( - NAME +add_executable(carddatabase_test ${MOCKS_SOURCES} ${VERSION_STRING_CPP} carddatabase_test.cpp mocks.cpp) + +target_link_libraries( carddatabase_test - SOURCES - ${VERSION_STRING_CPP} - carddatabase_test.cpp - mocks.cpp - LIBS - libcockatrice_card + PRIVATE libcockatrice_card + PRIVATE Threads::Threads + PRIVATE ${GTEST_BOTH_LIBRARIES} + PRIVATE ${TEST_QT_MODULES} ) +add_test(NAME carddatabase_test COMMAND carddatabase_test) + +# ------------------------ +# Load Benchmark (manual, not run in CI) +# ------------------------ +add_executable(load_benchmark ${MOCKS_SOURCES} ${VERSION_STRING_CPP} load_benchmark.cpp mocks.cpp) + +target_link_libraries( + load_benchmark + PRIVATE libcockatrice_card + PRIVATE libcockatrice_models + PRIVATE Threads::Threads + PRIVATE ${GTEST_BOTH_LIBRARIES} + PRIVATE ${TEST_QT_MODULES} +) + +if(NOT GTEST_FOUND) + add_dependencies(load_benchmark gtest) +endif() + # ------------------------ # Filter String Test # (guard must match the condition for libcockatrice_filters in the root CMakeLists.txt) # ------------------------ if(WITH_ORACLE OR WITH_CLIENT) - cockatrice_add_test( - NAME + add_executable(filter_string_test ${MOCKS_SOURCES} ${VERSION_STRING_CPP} filter_string_test.cpp mocks.cpp) + + target_link_libraries( filter_string_test - SOURCES - ${VERSION_STRING_CPP} - filter_string_test.cpp - mocks.cpp - LIBS - libcockatrice_filters + PRIVATE libcockatrice_filters + PRIVATE Threads::Threads + PRIVATE ${GTEST_BOTH_LIBRARIES} + PRIVATE ${TEST_QT_MODULES} ) + + add_test(NAME filter_string_test COMMAND filter_string_test) + + if(NOT GTEST_FOUND) + add_dependencies(filter_string_test gtest) + endif() endif() # ------------------------ -# Load Benchmark (manual, not run in CI) +# Dependencies on gtest # ------------------------ -cockatrice_add_manual_test( - NAME - load_benchmark - SOURCES - ${VERSION_STRING_CPP} - load_benchmark.cpp - mocks.cpp - LIBS - libcockatrice_card - libcockatrice_models - MANUAL -) +if(NOT GTEST_FOUND) + add_dependencies(carddatabase_test gtest) +endif() diff --git a/tests/carddatabase/carddatabase_test.cpp b/tests/carddatabase/carddatabase_test.cpp index 1b48d934b..1ac259d9b 100644 --- a/tests/carddatabase/carddatabase_test.cpp +++ b/tests/carddatabase/carddatabase_test.cpp @@ -159,19 +159,16 @@ 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; } @@ -209,17 +206,12 @@ 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; }; @@ -383,131 +375,6 @@ 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/deck_list_model/CMakeLists.txt b/tests/deck_list_model/CMakeLists.txt index cd7d693f3..e3096c559 100644 --- a/tests/deck_list_model/CMakeLists.txt +++ b/tests/deck_list_model/CMakeLists.txt @@ -1,23 +1,33 @@ -cockatrice_add_test( - NAME - deck_list_model_custom_zones_test - SOURCES - ${VERSION_STRING_CPP} - deck_list_model_custom_zones_test.cpp - LIBS - libcockatrice_models - libcockatrice_card - libcockatrice_deck_list -) +add_executable(deck_list_model_custom_zones_test ${VERSION_STRING_CPP} deck_list_model_custom_zones_test.cpp) -cockatrice_add_test( - NAME - deck_list_model_zone_integration_test - SOURCES - ${VERSION_STRING_CPP} - deck_list_model_zone_integration_test.cpp - LIBS +if(NOT GTEST_FOUND) + add_dependencies(deck_list_model_custom_zones_test gtest) +endif() + +target_link_libraries( + deck_list_model_custom_zones_test libcockatrice_models libcockatrice_card libcockatrice_deck_list + Threads::Threads + ${GTEST_BOTH_LIBRARIES} + ${TEST_QT_MODULES} ) +add_test(NAME deck_list_model_custom_zones_test COMMAND deck_list_model_custom_zones_test) + +add_executable(deck_list_model_zone_integration_test ${VERSION_STRING_CPP} deck_list_model_zone_integration_test.cpp) + +if(NOT GTEST_FOUND) + add_dependencies(deck_list_model_zone_integration_test gtest) +endif() + +target_link_libraries( + deck_list_model_zone_integration_test + libcockatrice_models + libcockatrice_card + libcockatrice_deck_list + Threads::Threads + ${GTEST_BOTH_LIBRARIES} + ${TEST_QT_MODULES} +) +add_test(NAME deck_list_model_zone_integration_test COMMAND deck_list_model_zone_integration_test) diff --git a/tests/deck_list_zones/CMakeLists.txt b/tests/deck_list_zones/CMakeLists.txt index 7af8fd187..0710be94d 100644 --- a/tests/deck_list_zones/CMakeLists.txt +++ b/tests/deck_list_zones/CMakeLists.txt @@ -1 +1,10 @@ -cockatrice_add_test(NAME deck_list_zones_test SOURCES deck_list_zones_test.cpp LIBS libcockatrice_deck_list) +add_executable(deck_list_zones_test deck_list_zones_test.cpp) + +if(NOT GTEST_FOUND) + add_dependencies(deck_list_zones_test gtest) +endif() + +target_link_libraries( + deck_list_zones_test libcockatrice_deck_list Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} +) +add_test(NAME deck_list_zones_test COMMAND deck_list_zones_test) diff --git a/tests/dummy_test.cpp b/tests/dummy_test.cpp new file mode 100644 index 000000000..8fd9c78dd --- /dev/null +++ b/tests/dummy_test.cpp @@ -0,0 +1,20 @@ +#include "gtest/gtest.h" +#include + +namespace +{ +class FooTest : public ::testing::Test +{ +}; + +TEST(DummyTest, Works) +{ + ASSERT_EQ(1, 1) << "One is not equal to one"; +} +} // namespace + +int main(int argc, char **argv) +{ + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} \ No newline at end of file diff --git a/tests/loader_local_matching_test.cpp b/tests/loader_local_matching_test.cpp index 7632e60d4..13d3e5bb3 100644 --- a/tests/loader_local_matching_test.cpp +++ b/tests/loader_local_matching_test.cpp @@ -13,7 +13,6 @@ #include #include #include -#include #include #include #include @@ -224,43 +223,8 @@ int main(int argc, char **argv) } QCoreApplication app(argc, argv); - - // Every TEST() runs as its own process and CI runs them in parallel (ctest -j4), - // while the picture paths round-trip through the process-global SettingsCache - // file. Parallel cases must therefore never share one settings directory: on - // platforms where QStandardPaths ignores the $HOME override above (macOS - // resolves the home directory via system APIs), every case would otherwise read - // and write the same paths.ini, and a sibling's setPicsPath can land between our - // own set and the loader's get — so the loader searches a foreign (or empty) - // directory and flakily returns a null image. Scope the settings to this process - // with a pid-unique application name; PIDs are never reused by live processes. - QCoreApplication::setOrganizationName(QStringLiteral("CockatriceTest")); - QCoreApplication::setApplicationName( - QStringLiteral("loader-local-matching-test-%1").arg(QCoreApplication::applicationPid())); - QLoggingCategory::setFilterRules("card_picture_loader.*=false\nsettings_cache.*=false"); ::testing::InitGoogleTest(&argc, argv); - - // CMake's gtest discovery runs this binary with --gtest_list_tests, which lists - // without running any test body. Arming the cleanup guard here would make it the - // sole SettingsCache::instance() call in that mode, instantiating the - // Q_GLOBAL_STATIC singleton (and its QNetworkAccessManagers) just to tear down at - // exit — which LeakSanitizer reports as leaks and fails discovery on the sanitizer - // leg. Skip the sandbox cleanup entirely when only listing. - if (::testing::GTEST_FLAG(list_tests)) { - return RUN_ALL_TESTS(); - } - - // Best-effort cleanup of this process's settings sandbox. QStandardPaths test - // mode is never auto-cleaned, and on macOS the $HOME override above is ignored, - // so without this every run litters ~/.qttest with one pid-unique directory. - // Safe under parallel runs: the pid-unique application name above guarantees - // this directory belongs to this process alone. - const auto cleanupSandbox = qScopeGuard([] { - QDir(SettingsCache::instance().getDataPath()).removeRecursively(); - QDir(SettingsCache::instance().getCachePath()).removeRecursively(); - }); - return RUN_ALL_TESTS(); -} +} \ No newline at end of file diff --git a/tests/loading_from_clipboard/CMakeLists.txt b/tests/loading_from_clipboard/CMakeLists.txt index 88711e457..719d62f45 100644 --- a/tests/loading_from_clipboard/CMakeLists.txt +++ b/tests/loading_from_clipboard/CMakeLists.txt @@ -1,13 +1,12 @@ add_definitions("-DCARDDB_DATADIR=\"${CMAKE_CURRENT_SOURCE_DIR}/data/\"") +add_executable(loading_from_clipboard_test ${VERSION_STRING_CPP} clipboard_testing.cpp loading_from_clipboard_test.cpp) -cockatrice_add_test( - NAME - loading_from_clipboard_test - SOURCES - ${VERSION_STRING_CPP} - clipboard_testing.cpp - loading_from_clipboard_test.cpp - LIBS - libcockatrice_deck_list - libcockatrice_card +if(NOT GTEST_FOUND) + add_dependencies(loading_from_clipboard_test gtest) +endif() + +target_link_libraries( + loading_from_clipboard_test libcockatrice_deck_list libcockatrice_card Threads::Threads ${GTEST_BOTH_LIBRARIES} + ${TEST_QT_MODULES} ) +add_test(NAME loading_from_clipboard_test COMMAND loading_from_clipboard_test) diff --git a/tests/loading_from_clipboard/loading_from_clipboard_test.cpp b/tests/loading_from_clipboard/loading_from_clipboard_test.cpp index e1797818e..840d926ff 100644 --- a/tests/loading_from_clipboard/loading_from_clipboard_test.cpp +++ b/tests/loading_from_clipboard/loading_from_clipboard_test.cpp @@ -80,20 +80,6 @@ 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/movecard_tests/CMakeLists.txt b/tests/movecard_tests/CMakeLists.txt index 68d8fc84c..769047148 100755 --- a/tests/movecard_tests/CMakeLists.txt +++ b/tests/movecard_tests/CMakeLists.txt @@ -1,11 +1,16 @@ -cockatrice_add_test( - NAME +add_executable(reverse_card_move_test reverse_card_move_test.cpp) + +if(NOT GTEST_FOUND) + add_dependencies(reverse_card_move_test gtest) +endif() + +target_link_libraries( reverse_card_move_test - SOURCES - reverse_card_move_test.cpp - LIBS - libcockatrice_network_server_remote - libcockatrice_rng - LABELS - server + PRIVATE libcockatrice_network_server_remote + PRIVATE libcockatrice_rng + PRIVATE Threads::Threads + PRIVATE ${GTEST_BOTH_LIBRARIES} + PRIVATE ${TEST_QT_MODULES} ) + +add_test(NAME reverse_card_move_test COMMAND reverse_card_move_test) diff --git a/tests/oracle/CMakeLists.txt b/tests/oracle/CMakeLists.txt index 6958673f7..9bc5ee5be 100644 --- a/tests/oracle/CMakeLists.txt +++ b/tests/oracle/CMakeLists.txt @@ -1,32 +1,37 @@ -cockatrice_add_test(NAME parse_cipt_test SOURCES ../../oracle/src/parsehelpers.cpp parse_cipt_test.cpp) +add_executable(parse_cipt_test ../../oracle/src/parsehelpers.cpp parse_cipt_test.cpp) + +if(NOT GTEST_FOUND) + add_dependencies(parse_cipt_test gtest) +endif() + +target_link_libraries(parse_cipt_test Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES}) + +add_test(NAME parse_cipt_test COMMAND parse_cipt_test) # Oracle importer unit tests -cockatrice_add_test( - NAME - oracle_importer_test - SOURCES - ${VERSION_STRING_CPP} - ../../oracle/src/oracleimporter.cpp - ../../oracle/src/parsehelpers.cpp - ../../oracle/src/raw_json_scanner.cpp - oracle_importer_test.cpp - LIBS - libcockatrice_card - libcockatrice_interfaces +add_executable( + oracle_importer_test ${VERSION_STRING_CPP} ../../oracle/src/oracleimporter.cpp ../../oracle/src/parsehelpers.cpp + ../../oracle/src/raw_json_scanner.cpp oracle_importer_test.cpp ) +if(NOT GTEST_FOUND) + add_dependencies(oracle_importer_test gtest) +endif() + +target_link_libraries( + oracle_importer_test libcockatrice_card libcockatrice_interfaces Threads::Threads ${GTEST_BOTH_LIBRARIES} + ${TEST_QT_MODULES} +) + +add_test(NAME oracle_importer_test COMMAND oracle_importer_test) + # Oracle importer benchmark tests (manual, not run in CI, incl. RAM benchmark) # Optional compression libs, mirrored from oracle/CMakeLists.txt, so the benchmark # can download and decompress whatever AllPrintings format the default URL selects. -# add_definitions() is directory-scoped and order-independent, so HAS_ZLIB/HAS_LZMA also -# reach the two unit tests above. That is harmless: those macros gate code only in -# oracle/src/pages.cpp and oracle/src/pagetemplates.cpp, which no test target compiles. find_package(ZLIB) if(ZLIB_FOUND) add_definitions("-DHAS_ZLIB") set(_ORACLE_BENCH_EXTRA_SOURCES ../../oracle/src/zip/unzip.cpp ../../oracle/src/zip/zipglobal.cpp) - cockatrice_disable_sanitizers_for(${_ORACLE_BENCH_EXTRA_SOURCES}) - cockatrice_disable_coverage_for(${_ORACLE_BENCH_EXTRA_SOURCES}) set(_ORACLE_BENCH_EXTRA_LIBRARIES ${ZLIB_LIBRARIES}) include_directories(${ZLIB_INCLUDE_DIRS}) else() @@ -37,28 +42,28 @@ find_package(LibLZMA) if(LIBLZMA_FOUND) add_definitions("-DHAS_LZMA") list(APPEND _ORACLE_BENCH_EXTRA_SOURCES ../../oracle/src/lzma/decompress.cpp) - cockatrice_disable_sanitizers_for(../../oracle/src/lzma/decompress.cpp) - cockatrice_disable_coverage_for(../../oracle/src/lzma/decompress.cpp) list(APPEND _ORACLE_BENCH_EXTRA_LIBRARIES ${LIBLZMA_LIBRARIES}) include_directories(${LIBLZMA_INCLUDE_DIRS}) else() message(STATUS "Oracle tests: LibLZMA not found; xz download benchmark disabled") endif() -cockatrice_add_manual_test( - NAME +add_executable( + oracle_importer_benchmark_test + ${VERSION_STRING_CPP} ../../oracle/src/oracleimporter.cpp ../../oracle/src/parsehelpers.cpp + ../../oracle/src/raw_json_scanner.cpp oracle_importer_benchmark_test.cpp ${_ORACLE_BENCH_EXTRA_SOURCES} +) + +if(NOT GTEST_FOUND) + add_dependencies(oracle_importer_benchmark_test gtest) +endif() + +target_link_libraries( oracle_importer_benchmark_test - SOURCES - ${VERSION_STRING_CPP} - ../../oracle/src/oracleimporter.cpp - ../../oracle/src/parsehelpers.cpp - ../../oracle/src/raw_json_scanner.cpp - oracle_importer_benchmark_test.cpp - ${_ORACLE_BENCH_EXTRA_SOURCES} - LIBS libcockatrice_card libcockatrice_interfaces - EXTRA_LIBS + Threads::Threads + ${GTEST_BOTH_LIBRARIES} + ${TEST_QT_MODULES} ${_ORACLE_BENCH_EXTRA_LIBRARIES} - MANUAL ) diff --git a/tests/picture_loader_benchmark/CMakeLists.txt b/tests/picture_loader_benchmark/CMakeLists.txt index 0684f58f4..a3b75fe88 100644 --- a/tests/picture_loader_benchmark/CMakeLists.txt +++ b/tests/picture_loader_benchmark/CMakeLists.txt @@ -1,10 +1,8 @@ -# Manual benchmark hitting the real card image hosts. Not registered with CTest: it requires a -# cards.xml, touches the network for minutes at a time, and needs network access. Build it -# explicitly and run by hand. -cockatrice_add_manual_test( - NAME +# Manual benchmark hitting the real card image hosts. Not registered with +# add_test(): it requires a cards.xml, touches the network for minutes at a +# time, and needs network access. Build it explicitly and run by hand. +add_executable( picture_loader_benchmark_test - SOURCES ${VERSION_STRING_CPP} ../../cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp ../../cockatrice/src/interface/card_picture_loader/card_picture_to_load.cpp @@ -13,12 +11,16 @@ cockatrice_add_manual_test( ../../cockatrice/src/client/settings/cache_settings.h picture_loader_benchmark.cpp settings_cache_mock.cpp - LIBS +) + +target_include_directories(picture_loader_benchmark_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src) + +target_link_libraries( + picture_loader_benchmark_test libcockatrice_card libcockatrice_settings libcockatrice_interfaces libcockatrice_utility - INCLUDE_DIRS - ${CMAKE_SOURCE_DIR}/cockatrice/src - MANUAL + Threads::Threads + ${TEST_QT_MODULES} ) diff --git a/tests/rate_limiter_test.cpp b/tests/rate_limiter_test.cpp deleted file mode 100644 index b1fb995f5..000000000 --- a/tests/rate_limiter_test.cpp +++ /dev/null @@ -1,124 +0,0 @@ -#include "ratelimiter.h" - -#include "gtest/gtest.h" - -namespace -{ - -TEST(RateLimiterTest, AllowsAttemptsWithinLimit) -{ - RateLimiter limiter; - ASSERT_FALSE(limiter.recordAttemptAt("ip", 3, 60, 1000)); - ASSERT_FALSE(limiter.recordAttemptAt("ip", 3, 60, 1001)); - // Still under the limit after two attempts, so the key is not blocked... - ASSERT_FALSE(limiter.isBlockedAt("ip", 3, 60, 1002)); - // ...and the third (the limit) is allowed through. - ASSERT_FALSE(limiter.recordAttemptAt("ip", 3, 60, 1002)); - // The next attempt is blocked once three are on the books. - ASSERT_TRUE(limiter.isBlockedAt("ip", 3, 60, 1003)); - ASSERT_TRUE(limiter.recordAttemptAt("ip", 3, 60, 1003)); -} - -TEST(RateLimiterTest, BlocksAttemptsAtTheLimit) -{ - RateLimiter limiter; - ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, 60, 1000)); - ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, 60, 1001)); - ASSERT_TRUE(limiter.recordAttemptAt("ip", 2, 60, 1002)); - ASSERT_TRUE(limiter.isBlockedAt("ip", 2, 60, 1002)); -} - -TEST(RateLimiterTest, NonPositiveConfigDisablesLimiting) -{ - RateLimiter limiter; - // maxAttempts = 0 is the documented "set to 0 to disable"; negative or - // zero windows are config mistakes and must not lock everyone out either. - ASSERT_FALSE(limiter.recordAttempt("ip", 0, 60)); - ASSERT_FALSE(limiter.isBlocked("ip", 0, 60)); - ASSERT_FALSE(limiter.recordAttempt("ip", -1, 60)); - ASSERT_FALSE(limiter.isBlocked("ip", -1, 60)); - ASSERT_FALSE(limiter.recordAttempt("ip", 5, 0)); - ASSERT_FALSE(limiter.isBlocked("ip", 5, 0)); -} - -TEST(RateLimiterTest, ClearAttempts) -{ - RateLimiter limiter; - ASSERT_FALSE(limiter.recordAttemptAt("ip", 1, 60, 1000)); - ASSERT_TRUE(limiter.recordAttemptAt("ip", 1, 60, 1001)); - ASSERT_TRUE(limiter.isBlockedAt("ip", 1, 60, 1001)); - limiter.clearAttempts("ip"); - ASSERT_FALSE(limiter.isBlockedAt("ip", 1, 60, 1002)); - // The cleared key starts fresh and is allowed again. - ASSERT_FALSE(limiter.recordAttemptAt("ip", 1, 60, 1002)); -} - -TEST(RateLimiterTest, KeysAreIndependent) -{ - RateLimiter limiter; - ASSERT_FALSE(limiter.recordAttemptAt("a", 1, 60, 1000)); - ASSERT_TRUE(limiter.recordAttemptAt("a", 1, 60, 1001)); - ASSERT_FALSE(limiter.isBlockedAt("b", 1, 60, 1001)); - ASSERT_TRUE(limiter.isBlockedAt("a", 1, 60, 1001)); -} - -TEST(RateLimiterTest, AttemptsAgeOutOfTheWindow) -{ - RateLimiter limiter; - const qint64 t = 1000; - const int window = 60; - - ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, window, t)); - ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, window, t + 1)); - ASSERT_TRUE(limiter.recordAttemptAt("ip", 2, window, t + 2)); - ASSERT_TRUE(limiter.isBlockedAt("ip", 2, window, t + 2)); - - // A blocked attempt is rejected without recording, so it cannot extend - // the lockout either. - ASSERT_TRUE(limiter.recordAttemptAt("ip", 2, window, t + 3)); - - // Once the recorded attempts fall outside the window the key clears by - // itself, with no manual unlock. - ASSERT_FALSE(limiter.isBlockedAt("ip", 2, window, t + 61)); - ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, window, t + 62)); -} - -TEST(RateLimiterTest, BlockedKeyIsUnlockedAfterSuccessfulClear) -{ - RateLimiter limiter; - const qint64 t = 5000; - - ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, 60, t)); - ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, 60, t + 1)); - ASSERT_TRUE(limiter.recordAttemptAt("ip", 2, 60, t + 2)); - ASSERT_TRUE(limiter.isBlockedAt("ip", 2, 60, t + 2)); - - limiter.clearAttempts("ip"); - ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, 60, t + 3)); -} - -TEST(RateLimiterTest, RetryAfterSecondsReportsTimeUntilUnlock) -{ - RateLimiter limiter; - const qint64 t = 1000; - const int window = 60; - - ASSERT_EQ(0, limiter.retryAfterSecondsAt("ip", 2, window, t)); - ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, window, t)); - ASSERT_EQ(0, limiter.retryAfterSecondsAt("ip", 2, window, t)); - ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, window, t + 1)); - ASSERT_TRUE(limiter.isBlockedAt("ip", 2, window, t + 2)); - // Oldest attempt was at t, window 60, so retry at t+60; now is t+2 => 58. - ASSERT_EQ(58, limiter.retryAfterSecondsAt("ip", 2, window, t + 2)); - // At t+60 the window just elapsed; retry should return 0. - ASSERT_EQ(0, limiter.retryAfterSecondsAt("ip", 2, window, t + 60)); - ASSERT_EQ(0, limiter.retryAfterSecondsAt("ip", 2, window, t + 61)); -} - -} // namespace - -int main(int argc, char **argv) -{ - ::testing::InitGoogleTest(&argc, argv); - return RUN_ALL_TESTS(); -} \ No newline at end of file diff --git a/tests/server_dump_zone_test.cpp b/tests/server_dump_zone_test.cpp deleted file mode 100644 index 71c410655..000000000 --- a/tests/server_dump_zone_test.cpp +++ /dev/null @@ -1,125 +0,0 @@ -/** @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/server_moderator_role_test.cpp b/tests/server_moderator_role_test.cpp deleted file mode 100644 index 2517a2d86..000000000 --- a/tests/server_moderator_role_test.cpp +++ /dev/null @@ -1,156 +0,0 @@ -/** @file server_moderator_role_test.cpp - * @brief Tests for the moderator staff role authorization and dispatch. - * @ingroup Tests - */ - -#include "libcockatrice/protocol/pb/response.pb.h" -#include "libcockatrice/protocol/pb/server_message.pb.h" - -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include - -// The server_remote library references the global RNG, which is normally -// defined by the servatrice/client executable main(). Provide a stub so the -// unit test can link against it. -RNG_Abstract *rng = nullptr; - -namespace -{ - -class TestModeratorHandler : public Server_ProtocolHandler -{ -public: - explicit TestModeratorHandler(Server *_server) : Server_ProtocolHandler(_server, nullptr) - { - } - - QString getAddress() const override - { - return {}; - } - QString getConnectionType() const override - { - return {}; - } - - // Buffer the last response code sent to the client so tests can assert on - // the outcome of processCommandContainer(). - Response::ResponseCode lastResponseCode = Response::RespNothing; - int dispatchCount = 0; - -protected: - void transmitProtocolItem(const ServerMessage &item) override - { - if (item.message_type() == ServerMessage::RESPONSE) { - lastResponseCode = item.response().response_code(); - } - } - - Response::ResponseCode - processExtendedModeratorCommand(int cmdType, const ModeratorCommand &, ResponseContainer &) override - { - ++dispatchCount; - // Fail closed for anything not explicitly handled. - if (cmdType != ModeratorCommand::REPORT_REOPEN) { - return Response::RespFunctionNotAllowed; - } - return Response::RespOk; - } -}; - -class ModeratorRoleTest : public ::testing::Test -{ -protected: - Server server; - TestModeratorHandler handler{&server}; - - void setUserLevel(uint32_t level) - { - ServerInfo_User user; - user.set_user_level(level); - handler.setUserInfo(user); - } -}; - -TEST_F(ModeratorRoleTest, RejectsWhenNotLoggedIn) -{ - CommandContainer cont; - cont.add_moderator_command(); - handler.processCommandContainer(cont); - EXPECT_EQ(handler.lastResponseCode, Response::RespLoginNeeded); - EXPECT_EQ(handler.dispatchCount, 0); -} - -TEST_F(ModeratorRoleTest, RejectsPlainUser) -{ - setUserLevel(ServerInfo_User::IsUser | ServerInfo_User::IsRegistered); - - CommandContainer cont; - cont.add_moderator_command(); - handler.processCommandContainer(cont); - EXPECT_EQ(handler.lastResponseCode, Response::RespLoginNeeded); - EXPECT_EQ(handler.dispatchCount, 0); -} - -TEST_F(ModeratorRoleTest, DispatchesToModeratorCommandForModerator) -{ - setUserLevel(ServerInfo_User::IsModerator); - - CommandContainer cont; - ModeratorCommand *cmd = cont.add_moderator_command(); - cmd->MutableExtension(Command_ReportReopen::ext); - handler.processCommandContainer(cont); - EXPECT_EQ(handler.lastResponseCode, Response::RespOk); - EXPECT_EQ(handler.dispatchCount, 1); -} - -TEST_F(ModeratorRoleTest, DispatchesToModeratorCommandForAdmin) -{ - setUserLevel(ServerInfo_User::IsModerator | ServerInfo_User::IsAdmin); - - CommandContainer cont; - ModeratorCommand *cmd = cont.add_moderator_command(); - cmd->MutableExtension(Command_ReportReopen::ext); - handler.processCommandContainer(cont); - EXPECT_EQ(handler.lastResponseCode, Response::RespOk); - EXPECT_EQ(handler.dispatchCount, 1); -} - -TEST_F(ModeratorRoleTest, RejectsDeveloperThatIsNotModerator) -{ - setUserLevel(ServerInfo_User::IsDeveloper); - - CommandContainer cont; - cont.add_moderator_command(); - handler.processCommandContainer(cont); - EXPECT_EQ(handler.lastResponseCode, Response::RespLoginNeeded); - EXPECT_EQ(handler.dispatchCount, 0); -} - -TEST_F(ModeratorRoleTest, FailClosedForUnknownModeratorCommand) -{ - setUserLevel(ServerInfo_User::IsModerator); - - CommandContainer cont; - cont.add_moderator_command(); // no extension set -> getPbExtension() returns -1 - handler.processCommandContainer(cont); - EXPECT_EQ(handler.lastResponseCode, Response::RespFunctionNotAllowed); - EXPECT_EQ(handler.dispatchCount, 1); -} - -} // namespace - -int main(int argc, char **argv) -{ - ::testing::InitGoogleTest(&argc, argv); - return RUN_ALL_TESTS(); -} diff --git a/tests/settings/CMakeLists.txt b/tests/settings/CMakeLists.txt index 98bed1f4b..7ee32a645 100644 --- a/tests/settings/CMakeLists.txt +++ b/tests/settings/CMakeLists.txt @@ -1,14 +1,26 @@ -cockatrice_add_test(NAME settings_manager_test SOURCES settings_manager_test.cpp LIBS libcockatrice_settings) +add_executable(settings_manager_test settings_manager_test.cpp) +add_executable(settings_defaults_test settings_defaults_test.cpp) +add_executable(settings_migration_test settings_migration_test.cpp) -cockatrice_add_test( - NAME - settings_defaults_test - SOURCES - settings_defaults_test.cpp - LIBS - libcockatrice_settings - DEFINITIONS - SETTINGS_UNIT_TEST +target_link_libraries( + settings_manager_test libcockatrice_settings Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} ) -cockatrice_add_test(NAME settings_migration_test SOURCES settings_migration_test.cpp LIBS libcockatrice_settings) +target_link_libraries( + settings_defaults_test libcockatrice_settings Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} +) +target_compile_definitions(settings_defaults_test PRIVATE SETTINGS_UNIT_TEST) + +target_link_libraries( + settings_migration_test libcockatrice_settings Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} +) + +add_test(NAME settings_manager_test COMMAND settings_manager_test) +add_test(NAME settings_defaults_test COMMAND settings_defaults_test) +add_test(NAME settings_migration_test COMMAND settings_migration_test) + +if(NOT GTEST_FOUND) + add_dependencies(settings_manager_test gtest) + add_dependencies(settings_defaults_test gtest) + add_dependencies(settings_migration_test gtest) +endif() diff --git a/tests/settings/settings_defaults_test.cpp b/tests/settings/settings_defaults_test.cpp index 2ba9f3fbb..65e41af76 100644 --- a/tests/settings/settings_defaults_test.cpp +++ b/tests/settings/settings_defaults_test.cpp @@ -1,7 +1,6 @@ #include "libcockatrice/card/card_localization.h" #include "gtest/gtest.h" -#include #include #include #include @@ -16,7 +15,6 @@ #include #include #include -#include #include #include #include @@ -697,55 +695,6 @@ TEST_F(SettingsDefaultsTest, VisualDeckStorage_ShowUploadTime_SetAndGet) ASSERT_EQ(s.getVisualDeckStorageShowUploadTime(), false); } -TEST_F(SettingsDefaultsTest, Paths_AppDirWarningAcknowledged_Default) -{ - PathsSettings s(settingsPath, nullptr); - ASSERT_EQ(s.getAppDirWarningAcknowledged(), false); -} - -TEST_F(SettingsDefaultsTest, Paths_AppDirWarningAcknowledged_SetAndGet) -{ - PathsSettings s(settingsPath, nullptr); - s.setAppDirWarningAcknowledged(true); - ASSERT_EQ(s.getAppDirWarningAcknowledged(), true); -} - -TEST_F(SettingsDefaultsTest, Paths_PathsInsideDir_DetectsNestedAndIgnoresSiblings) -{ - PathsSettings s(settingsPath, nullptr); - const QString base = QDir::cleanPath(settingsPath); - - s.setDeckPath(base + "/decks/"); - s.setPicsPath(base + "/pics"); - s.setReplaysPath(base + "2"); // sibling sharing the name prefix, not nested - - const QStringList offenders = s.pathsInsideDir(base); - ASSERT_EQ(offenders.size(), 2); - ASSERT_TRUE(offenders.contains(base + "/decks")); - ASSERT_TRUE(offenders.contains(base + "/pics")); -} - -TEST_F(SettingsDefaultsTest, Paths_PathsInsideDir_EmptyWhenNothingMatches) -{ - PathsSettings s(settingsPath, nullptr); - s.setDeckPath("/somewhere/else/decks"); - ASSERT_TRUE(s.pathsInsideDir(QDir::cleanPath(settingsPath)).isEmpty()); -} - -TEST_F(SettingsDefaultsTest, Paths_PathsInsideDir_MatchesExactBaseAndNormalizesDots) -{ - PathsSettings s(settingsPath, nullptr); - const QString base = QDir::cleanPath(settingsPath); - - s.setCustomPicsPath(base); // exactly the base directory - s.setThemesPath(base + "/pics/../decks"); // needs cleanPath normalization - - const QStringList offenders = s.pathsInsideDir(base); - ASSERT_EQ(offenders.size(), 2); - ASSERT_TRUE(offenders.contains(base)); - ASSERT_TRUE(offenders.contains(base + "/decks")); -} - } // namespace int main(int argc, char **argv) diff --git a/tests/settings/settings_migration_test.cpp b/tests/settings/settings_migration_test.cpp index 2c4dcf7a9..440e49b74 100644 --- a/tests/settings/settings_migration_test.cpp +++ b/tests/settings/settings_migration_test.cpp @@ -705,71 +705,6 @@ 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(); diff --git a/tests/theme_migration/CMakeLists.txt b/tests/theme_migration/CMakeLists.txt index 93493ee5f..ac202210e 100644 --- a/tests/theme_migration/CMakeLists.txt +++ b/tests/theme_migration/CMakeLists.txt @@ -1,7 +1,5 @@ -cockatrice_add_test( - NAME +add_executable( theme_migration_test - SOURCES ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/theme_manager.cpp ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/theme_config.cpp ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/pixel_map_generator.cpp @@ -11,11 +9,22 @@ cockatrice_add_test( ${CMAKE_SOURCE_DIR}/cockatrice/src/client/network/update/client/release_channel.cpp ${VERSION_STRING_CPP} theme_migration_test.cpp - LIBS +) +target_include_directories(theme_migration_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src) + +target_link_libraries( + theme_migration_test libcockatrice_settings libcockatrice_network libcockatrice_network_server_remote libcockatrice_protocol - INCLUDE_DIRS - ${CMAKE_SOURCE_DIR}/cockatrice/src + Threads::Threads + ${GTEST_BOTH_LIBRARIES} + ${TEST_QT_MODULES} ) + +add_test(NAME theme_migration_test COMMAND theme_migration_test) + +if(NOT GTEST_FOUND) + add_dependencies(theme_migration_test gtest) +endif()