Fix and adjustment for Named States#1982
Conversation
…sistentDir()` to save. Adjusted named save states to save into a folder, rather than by prefix.
📝 WalkthroughWalkthroughSave state lookup and path creation now use g_system->getPersistentDir() with the GUI save-state subfolder (prefix(false)). Listing filters by postfix only; displayed names strip only the postfix. Subfolders are created if missing and full save paths are built by joining persistent dir, subfolder, base name, and postfix. ChangesSave State Path Handling Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/gui/widgets/named_savestates.cc (2)
145-169: Suffix check and filesystem error handling could be more robust/clearThe new logic to list named save states under
g_system->getPersistentDir() / getSaveStatePrefix(false)is consistent with the new storage layout and keeps the UI working on the persistent directory only.Two minor robustness/readability suggestions:
Suffix check
The current condition:if (filename.rfind(postfix) == filename.length() - postfix.length()) {works, but mixes
rfindsemantics and unsigned arithmetic. A more explicit “ends with” style check is easier to read and avoids size_t underflow patterns:if (filename.size() >= postfix.size() && filename.compare(filename.size() - postfix.size(), postfix.size(), postfix) == 0) { std::string niceName = filename.substr(0, filename.size() - postfix.size()); ... }Directory iteration error handling (optional)
You already usestd::error_code ecwithexistsanddirectory_iterator, but never inspectec. If the directory is unreadable or iteration fails, the UI will silently show an empty list. It might be worth at least logging whenecis set after constructing the iterator, so users get a hint if filesystem permissions or disk issues prevent loading named states.These are not correctness blockers, just clarity and diagnostics improvements.
187-195: Usestd::filesystem::pathcomposition instead of manual"//"string concatenationThe new
createSaveStatePathcorrectly targetsg_system->getPersistentDir() / getSaveStatePrefix(false)as the base directory, but the final path is built as a formatted string with a hard‑coded"//"separator:std::string pathStr = fmt::format("{}//{}{}", base_path.string(), saveStateName, gui->getSaveStatePostfix()); return std::filesystem::path(pathStr);This is non‑idiomatic and can be fragile across platforms (mixing native separators with string
"//"). It’s safer and clearer to letstd::filesystem::pathhandle separators and to simplify directory creation at the same time. For example:- std::error_code ec; - std::filesystem::path base_path = g_system->getPersistentDir() / gui->getSaveStatePrefix(false); - if (!std::filesystem::exists(base_path, ec)) { - std::filesystem::create_directory(base_path); - } - - std::string pathStr = fmt::format("{}//{}{}", base_path.string(), saveStateName, gui->getSaveStatePostfix()); - return std::filesystem::path(pathStr); + std::error_code ec; + std::filesystem::path base_path = + g_system->getPersistentDir() / gui->getSaveStatePrefix(false); + + // Create the directory tree if needed; avoid exceptions on failure. + std::filesystem::create_directories(base_path, ec); + if (ec) { + g_system->log(LogClass::UI, + "Failed to create named save directory '%s': %s\n", + base_path.string().c_str(), ec.message().c_str()); + // Optionally: early-return here if you prefer to abort save on error. + } + + std::filesystem::path file_name = + saveStateName + gui->getSaveStatePostfix(); + return base_path / file_name;This avoids manual separator management, uses the platform’s native path rules, and gives you a single, exception‑free path for directory creation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/gui/widgets/named_savestates.cc(3 hunks)
🧰 Additional context used
🪛 Cppcheck (2.18.0)
src/gui/widgets/named_savestates.cc
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
src/gui/widgets/named_savestates.cc (1)
24-24: Include ofcore/system.his appropriate hereUsing
g_system->getPersistentDir()andg_system->log()in this file requires thecore/system.hinclude; the addition looks correct and expected.
Added logging when a folder is created
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/gui/widgets/named_savestates.cc (2)
145-149: ConfirmgetSaveStatePrefix(false)semantics for folder namingYou now use
getSaveStatePrefix(false)both for listing (line 147) and as the sub-folder increateSaveStatePath, while the UI still displaysgetSaveStatePrefix(true)above the input. Please double‑check that:
- the
falsevariant is guaranteed to be a safe directory name (no separators), and- the relationship between the shown prefix (
true) and the on-disk layout (false) is intentional and documented,
so users aren’t confused by seeing a prefix that doesn’t literally match the folder name.
189-203: Add error handling/logging around save-state directory creationThe new
createSaveStatePathcorrectly moves named states under a per‑game subfolder in the persistent dir, but the directory creation path is a bit optimistic:
std::filesystem::create_directory(base_path);is called without anerror_code, and- the log unconditionally claims the folder was created, even if the call fails or throws.
Consider using the non‑throwing overload with
std::error_codeand logging failures explicitly. For example:- std::error_code ec; - std::string sub_folder = gui->getSaveStatePrefix(false); - std::filesystem::path base_path = g_system->getPersistentDir() / sub_folder; - if (!std::filesystem::exists(base_path, ec)) { - std::filesystem::create_directory(base_path); - g_system->log(LogClass::UI, "Created save state folder: %s\n", sub_folder.c_str()); - } + std::error_code ec; + std::string sub_folder = gui->getSaveStatePrefix(false); + std::filesystem::path base_path = g_system->getPersistentDir() / sub_folder; + if (!std::filesystem::exists(base_path, ec)) { + if (!std::filesystem::create_directory(base_path, ec)) { + if (ec) { + g_system->log(LogClass::UI, + "Failed to create save state folder '%s': %s\n", + base_path.string().c_str(), ec.message().c_str()); + } + } else { + g_system->log(LogClass::UI, "Created save state folder: %s\n", + base_path.string().c_str()); + } + }This keeps exceptions out of this path and ensures logs reflect actual outcomes, which is helpful on read‑only or misconfigured persistent directories.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/gui/widgets/named_savestates.cc(3 hunks)
🧰 Additional context used
🪛 Cppcheck (2.18.0)
src/gui/widgets/named_savestates.cc
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
src/gui/widgets/named_savestates.cc (2)
24-24: Include ofcore/system.his appropriateThis include is required for
g_system/getPersistentDir()and resolves the static analysis “missing include” hint for this file; no changes needed.
152-167: Harden filename postfix check to avoid length underflow edge caseThe current postfix filter can misbehave when
filename.length() < postfix.length()because the subtraction wraps onsize_t, making the condition unexpectedly true. Guard the length first and then compare the tail explicitly:- if (filename.rfind(postfix) == filename.length() - postfix.length()) { - std::string niceName = - filename.substr(0, filename.length() - (postfix.length())); + if (filename.size() >= postfix.size() && + filename.compare(filename.size() - postfix.size(), + postfix.size(), + postfix) == 0) { + std::string niceName = + filename.substr(0, filename.size() - postfix.size());
|
I wonder if it's easier to implement moving the numbered save states too |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/gui/widgets/named_savestates.cc`:
- Around line 194-199: Remove the spurious unary plus operators before the
string arguments in the g_system->log call (remove +"Failed..." and
+base_path...) and modify the create-directory failure handling: after calling
std::filesystem::create_directory(base_path, ec), check ec to determine if an
actual error occurred before logging a failure; only call g_system->log with the
error message when ec indicates an error (use ec.message() and
base_path.string().c_str()), otherwise treat a false return with no error as a
non-fatal “already exists” case and log the created/exists message (referencing
std::filesystem::create_directory, ec, base_path, sub_folder, and
g_system->log).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c759db89-18f6-41f1-ad81-4bdd8f21fb8f
📒 Files selected for processing (1)
src/gui/widgets/named_savestates.cc
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/gui/widgets/named_savestates.cc (1)
157-159: 💤 Low valueConsider a safer suffix check to avoid unsigned underflow.
If
filenameis shorter thanpostfix, the subtractionfilename.length() - postfix.length()underflows. While this typically produces a value different fromrfind's result (so files are correctly rejected), it's relying on coincidental behavior.♻️ Proposed fix using explicit bounds check
- if (filename.rfind(postfix) == filename.length() - postfix.length()) { - std::string niceName = - filename.substr(0, filename.length() - (postfix.length())); + if (filename.length() > postfix.length() && + filename.compare(filename.length() - postfix.length(), postfix.length(), postfix) == 0) { + std::string niceName = filename.substr(0, filename.length() - postfix.length());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gui/widgets/named_savestates.cc` around lines 157 - 159, The suffix check using filename.rfind(postfix) == filename.length() - postfix.length() can underflow when filename is shorter than postfix; update the logic in the block that computes niceName so you first ensure filename.size() >= postfix.size() (or otherwise check bounds) before doing the subtraction, then compare the suffix (e.g., with rfind or substr) and only compute niceName = filename.substr(0, filename.length() - postfix.length()) after that guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/gui/widgets/named_savestates.cc`:
- Around line 157-159: The suffix check using filename.rfind(postfix) ==
filename.length() - postfix.length() can underflow when filename is shorter than
postfix; update the logic in the block that computes niceName so you first
ensure filename.size() >= postfix.size() (or otherwise check bounds) before
doing the subtraction, then compare the suffix (e.g., with rfind or substr) and
only compute niceName = filename.substr(0, filename.length() - postfix.length())
after that guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0d88654a-346d-4d25-bef2-b8f091d12030
📒 Files selected for processing (1)
src/gui/widgets/named_savestates.cc
2 participants
Fix for named save states using
current_path()to load, andgetPersistentDir()to save.Adjusted named save states to save into a folder. [#1922]
Breaking change:
Existing named save states will not be located as they are not in the new folder. This can be fixed by moving save states into the appropriate folder.