Conversation
There was a problem hiding this comment.
Pull request overview
Updates RetroAchievements (“rcheevos”) integration to support multi-set game data and refreshes the UI/repository models accordingly.
Changes:
- Upgrades the
rcheevossubmodule and syncs vendored headers/build integration. - Refactors achievements domain models/repository/service APIs (Game, Sets, Achievements, Leaderboards; new field names).
- Adds new QML-facing
RetroAchievementsGameItemand updates QML/components for navigation and focus highlighting.
Reviewed changes
Copilot reviewed 52 out of 57 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/app/emulation/emulator_instance_test.cpp | Updates commented test code to reflect renamed “create” API. |
| tests/app/achievements/sqlite_achievement_repository_test.cpp | Updates tests for new achievement/user field names and set lookup behavior. |
| src/main.cpp | Registers new QML type, changes DB filename, and updates log text. |
| src/app/rcheevos/user.h | Expands RA user model and adds JSON (de)serialization helpers. |
| src/app/rcheevos/ra_client.cpp | Updates progress/user persistence calls and user score field mapping. |
| src/app/rcheevos/offline/rcheevos_offline_client.hpp | Renames offline request handlers to “achievementsets” and updates signatures/attributes. |
| src/app/rcheevos/offline/rcheevos_offline_client.cpp | Reworks offline request routing and response processing for multi-set/game models. |
| src/app/rcheevos/offline/earned_achievement.hpp | Removes unused offline model. |
| src/app/rcheevos/offline/cached_achievement.hpp | Removes unused offline model. |
| src/app/rcheevos/login2_response.hpp | Updates types/formatting for new user score field types. |
| src/app/rcheevos/award_achievement_response.hpp | Updates types/formatting for score fields. |
| src/app/emulator_item_renderer.cpp | Updates debug/comment strings for renamed “create” API. |
| src/app/achievements/user_achievement_status.hpp | Removes unused model. |
| src/app/achievements/sqlite_achievement_repository.hpp | Updates repository interface/docs for multi-set/game support. |
| src/app/achievements/models/leaderboard.hpp | Adds Leaderboard model with JSON (de)serialization. |
| src/app/achievements/models/game.hpp | Adds Game model to represent multi-set game content. |
| src/app/achievements/models/achievement_set.hpp | Expands AchievementSet model + JSON (de)serialization (sets + leaderboards). |
| src/app/achievements/models/achievement.hpp | Expands Achievement model + JSON (de)serialization (badges/rarity/etc). |
| src/app/achievements/gui/retro_achievements_game_item.hpp | Adds QML-facing Game wrapper exposing sets for a content hash. |
| src/app/achievements/gui/retro_achievements_game_item.cpp | Implements content-hash lookup to game/sets for UI. |
| src/app/achievements/gui/achievement_list_model.cpp | Updates UI binding to use new achievement title/badge fields. |
| src/app/achievements/gui/AchievementSetItem.hpp | Refactors AchievementSetItem API to be constructed from a set model. |
| src/app/achievements/gui/AchievementSetItem.cpp | Refactors set item initialization to avoid content-hash fetch in item. |
| src/app/achievements/achievement_service.hpp | Refactors service API (create methods, game-by-hash, session now keyed by gameId). |
| src/app/achievements/achievement_service.cpp | Implements new service methods and removes patch-response processing path. |
| src/app/achievements/achievement_repository.hpp | Updates repository interface for game/multi-set and leaderboard support. |
| qml/components/navigation/LeftNavigationBar.qml | UI color tweak. |
| qml/components/achievements/AchievementListButton.qml | Adds padding for layout/interaction. |
| qml/components/FLFocusHighlight.qml | Reworks focus highlight to crossfade between targets. |
| qml/Main3.qml | Simplifies page instantiation structure. |
| libs/rcheevos | Bumps rcheevos submodule revision. |
| include/rcheevos/rcheevos.h | Syncs vendored rcheevos header formatting/contents. |
| include/rcheevos/rc_util.h | Syncs vendored rcheevos header formatting/contents. |
| include/rcheevos/rc_runtime_types.h | Syncs vendored rcheevos header to new upstream API/ABI. |
| include/rcheevos/rc_runtime.h | Syncs vendored rcheevos header to new upstream API/ABI. |
| include/rcheevos/rc_libretro.h | Syncs vendored rcheevos header to new upstream API/ABI. |
| include/rcheevos/rc_hash.h | Syncs vendored rcheevos header to new upstream API/ABI. |
| include/rcheevos/rc_export.h | Syncs vendored rcheevos header to new upstream API/ABI. |
| include/rcheevos/rc_error.h | Syncs vendored rcheevos header to new upstream API/ABI. |
| include/rcheevos/rc_consoles.h | Syncs vendored rcheevos header to new upstream API/ABI. |
| include/rcheevos/rc_client_raintegration.h | Syncs vendored rcheevos header to new upstream API/ABI. |
| include/rcheevos/rc_api_user.h | Syncs vendored rcheevos header to new upstream API/ABI. |
| include/rcheevos/rc_api_runtime.h | Syncs vendored rcheevos header to new upstream API/ABI. |
| include/rcheevos/rc_api_request.h | Syncs vendored rcheevos header to new upstream API/ABI. |
| include/rcheevos/rc_api_info.h | Syncs vendored rcheevos header to new upstream API/ABI. |
| include/rcheevos/rc_api_editor.h | Syncs vendored rcheevos header to new upstream API/ABI. |
| include/rcheevos/module.modulemap | Updates module map to include raintegration header/module. |
| cmake/rcheevos.cmake | Adjusts rcheevos build source selection. |
| CMakeLists.txt | Adds new RetroAchievements game item sources to build. |
| .gitmodules | Reorders submodule declarations. |
Comments suppressed due to low confidence (12)
src/app/achievements/gui/retro_achievements_game_item.cpp:1
- The constructor ignores the
parentparameter and does not initialize theQObjectbase with it, which breaks Qt parent/child ownership and can cause leaks and missing object-tree behavior. Initialize the base class with: QObject(parent)(and keep any other required base initialization if applicable).
src/app/achievements/gui/retro_achievements_game_item.cpp:1 m_achievementSets.clear()only clears the container; it does not delete the previously allocatedAchievementSetItemobjects (they remain children ofthis), so repeatedsetContentHashcalls will leak memory and duplicate children. Before clearing, delete the old items (e.g.,qDeleteAll(m_achievementSets)), and also apply the current hardcore state to newly created set items (callsetItem->setHardcore(m_hardcore)after construction) so the UI stays consistent whenm_hardcoreis already enabled.
src/app/rcheevos/offline/rcheevos_offline_client.cpp:1handleAchievementSetsRequestreturns{"Success":true}butprocessAchievementSetsResponselater parses anAchievementSetResponsethat requires additional fields (e.g.,GameId,Title,Sets). This mismatch will cause JSON parsing/deserialization failures at runtime when the offline handler path is used. Either generate a full, schema-correctAchievementSetResponsefrom cached data, or return a failure response (or HTTP error) that preventsprocessAchievementSetsResponsefrom attempting to deserialize.
src/app/rcheevos/offline/rcheevos_offline_client.cpp:1- The constructed
AchievementSetleaves the “calculated fields” (numAchievements,totalPoints) at their default values, then persists the set. If the repository/UI uses these stored values (as the tests suggest), sets fetched from the API will appear as having 0 achievements/points. Compute and populate these fields fromset.achievements(excluding inactive/prototype flags if that’s intended) before callingm_achievementService.create(achievementSet).
src/app/rcheevos/offline/rcheevos_offline_client.cpp:1 - The service/repository API was renamed to accept a
gameId(and events were also renamed togameId), but this call passes anachievementSetId. With multi-set support, set IDs and game IDs are no longer interchangeable, which will produce incorrect locked/remaining counts (and potentially wrong filtering) when a game has multiple sets. Either (a) change the API back to explicitly acceptachievementSetId, or (b) look up the correspondinggameIdfor the set and pass that consistently.
src/app/rcheevos/user.h:1 - These scalar fields are not default-initialized, and
from_jsononly conditionally assigns them. If any of the keys are missing, theUserinstance will contain indeterminate values (undefined behavior when read). Default-initialize the scalar members (e.g.,unsigned score{};) and ensuredisplayNameis either parsed/emitted or removed to keep the model consistent (currently it’s never populated nor serialized).
src/app/achievements/models/achievement.hpp:1 - Most fields are not default-initialized, and
from_jsononly sets them if keys exist; missing keys will leave indeterminate values for scalars likepoints,flags, timestamps, and rarities. Default-initialize all members (especially scalars) and consider explicitly settingachievementSetId/displayOrderin the parsing path (or documenting that they are set externally) to avoid partially-initialized achievements.
src/app/achievements/models/leaderboard.hpp:1 - Fields are not default-initialized, and
from_jsonconditionally assigns several of them. If any key is missing, booleans/integers can remain indeterminate. Default-initialize all scalar members (and consider whetherachievementSetId/displayOrdershould be populated from JSON or always injected by the caller).
src/app/achievements/sqlite_achievement_repository.hpp:1 - The updated docs are internally inconsistent:
gameIdis described as an “achievement set ID”, and the SQL snippet usesachievement_set_id = :gameId. This is confusing (and likely incorrect) now that the code distinguishesgameIdvsachievementSetId. Update parameter names and SQL snippets to match the actual ID being queried and keep the terminology consistent throughout the interface/docs.
src/app/achievements/sqlite_achievement_repository.hpp:1 - The updated docs are internally inconsistent:
gameIdis described as an “achievement set ID”, and the SQL snippet usesachievement_set_id = :gameId. This is confusing (and likely incorrect) now that the code distinguishesgameIdvsachievementSetId. Update parameter names and SQL snippets to match the actual ID being queried and keep the terminology consistent throughout the interface/docs.
src/app/achievements/sqlite_achievement_repository.hpp:1 - The updated docs are internally inconsistent:
gameIdis described as an “achievement set ID”, and the SQL snippet usesachievement_set_id = :gameId. This is confusing (and likely incorrect) now that the code distinguishesgameIdvsachievementSetId. Update parameter names and SQL snippets to match the actual ID being queried and keep the terminology consistent throughout the interface/docs.
src/main.cpp:1 - Changing the on-disk database filename will effectively “reset” achievements data for existing installs unless there’s migration/compat handling elsewhere. If this is intentional (schema break), consider adding an explicit migration/upgrade path (copy/transform old DB) or logging a clear message so users understand why prior data is missing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| file(GLOB RCHEEVOS_SOURCES | ||
| ${CMAKE_SOURCE_DIR}/libs/rcheevos/src/*.c | ||
| ${CMAKE_SOURCE_DIR}/libs/rcheevos/src/rc_client.c | ||
| ${CMAKE_SOURCE_DIR}/libs/rcheevos/src/rc_client_raintegration.c | ||
| ${CMAKE_SOURCE_DIR}/libs/rcheevos/src/rc_compat.c | ||
| ${CMAKE_SOURCE_DIR}/libs/rcheevos/src/rc_libretro.c | ||
| ${CMAKE_SOURCE_DIR}/libs/rcheevos/src/rc_util.c | ||
| ${CMAKE_SOURCE_DIR}/libs/rcheevos/src/rc_version.c | ||
| ${CMAKE_SOURCE_DIR}/libs/rcheevos/src/**/*.c) |
There was a problem hiding this comment.
This mixes explicit source entries with a recursive glob that likely matches the same files again (rc_client.c, rc_util.c, etc.). Depending on generator/tooling, duplicate source paths can cause build issues or at minimum make the build definition harder to reason about. Prefer either a fully explicit source list (recommended for reproducible builds) or a glob without manually listed files (and consider CONFIGURE_DEPENDS if you keep globs).
No description provided.