Fix and adjustment for Named States #1982
Conversation
…sistentDir()` to save. Adjusted named save states to save into a folder, rather than by prefix.
WalkthroughSave state storage and lookup moved from the current working directory to the system persistent directory via g_system->getPersistentDir(). Prefix usage changed to false, validation now checks only postfix, name extraction strips only the postfix, and directories are created if missing before path construction. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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());
1 participant
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.