From 4ae682283c36f5b486499911f0df4db9c9a816a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sat, 12 Sep 2026 11:54:11 -0600 Subject: [PATCH] Path: make WithReplacedExtension(old, new) report a mismatch instead of hiding it It used to return the path unchanged when the path didn't end in oldExtension, so a caller that guessed wrong silently went on using the original file - and "the screenshot next to this savestate" quietly becomes "this savestate". Every caller had to know to check the extension first, and most didn't. Now it's [[nodiscard]] bool with an out-param, in the style of ComputePathTo next door, so the mismatch has to be handled. Changing the signature rather than the behaviour means no call site can keep the old assumption by accident. All four callers wanted "skip it" or "fall back", which they now say out loud. Co-Authored-By: Claude Opus 5 (1M context) --- Common/File/AndroidContentURI.cpp | 12 +++++++----- Common/File/AndroidContentURI.h | 3 ++- Common/File/Path.cpp | 27 ++++++++++++++++----------- Common/File/Path.h | 8 +++++++- UI/GameInfoCache.cpp | 14 ++++++++------ UI/SavedataScreen.cpp | 4 ++-- headless/Compare.cpp | 7 ++++++- unittest/UnitTest.cpp | 10 +++++++++- 8 files changed, 57 insertions(+), 28 deletions(-) diff --git a/Common/File/AndroidContentURI.cpp b/Common/File/AndroidContentURI.cpp index db5808ea58..41137018a5 100644 --- a/Common/File/AndroidContentURI.cpp +++ b/Common/File/AndroidContentURI.cpp @@ -82,14 +82,16 @@ AndroidContentURI AndroidContentURI::WithExtraExtension(std::string_view extensi return uri; } -AndroidContentURI AndroidContentURI::WithReplacedExtension(const std::string &oldExtension, const std::string &newExtension) const { +bool AndroidContentURI::WithReplacedExtension(const std::string &oldExtension, const std::string &newExtension, AndroidContentURI *out) const { _dbg_assert_(!oldExtension.empty() && oldExtension[0] == '.'); _dbg_assert_(!newExtension.empty() && newExtension[0] == '.'); - AndroidContentURI uri = *this; - if (endsWithNoCase(file, oldExtension)) { - uri.file = file.substr(0, file.size() - oldExtension.size()) + newExtension; + if (!endsWithNoCase(file, oldExtension)) { + return false; } - return uri; + AndroidContentURI uri = *this; + uri.file = file.substr(0, file.size() - oldExtension.size()) + newExtension; + *out = uri; + return true; } AndroidContentURI AndroidContentURI::WithReplacedExtension(const std::string &newExtension) const { diff --git a/Common/File/AndroidContentURI.h b/Common/File/AndroidContentURI.h index bc6bc65fe8..fec0089e5b 100644 --- a/Common/File/AndroidContentURI.h +++ b/Common/File/AndroidContentURI.h @@ -32,7 +32,8 @@ public: AndroidContentURI WithRootFilePath(const std::string &filePath); AndroidContentURI WithComponent(std::string_view filePath); AndroidContentURI WithExtraExtension(std::string_view extension); // The ext string contains the dot. - AndroidContentURI WithReplacedExtension(const std::string &oldExtension, const std::string &newExtension) const; + // False, leaving *out alone, if the file doesn't end in oldExtension. See Path's version. + [[nodiscard]] bool WithReplacedExtension(const std::string &oldExtension, const std::string &newExtension, AndroidContentURI *out) const; AndroidContentURI WithReplacedExtension(const std::string &newExtension) const; bool CanNavigateUp() const; diff --git a/Common/File/Path.cpp b/Common/File/Path.cpp index 452c85703d..77ed8de62c 100644 --- a/Common/File/Path.cpp +++ b/Common/File/Path.cpp @@ -119,20 +119,25 @@ Path Path::WithExtraExtension(std::string_view ext) const { return Path(path_ + std::string(ext)); } -Path Path::WithReplacedExtension(const std::string &oldExtension, const std::string &newExtension) const { - if (type_ == PathType::CONTENT_URI) { - AndroidContentURI uri(path_); - return Path(uri.WithReplacedExtension(oldExtension, newExtension).ToString()); - } - +bool Path::WithReplacedExtension(const std::string &oldExtension, const std::string &newExtension, Path *out) const { _dbg_assert_(!oldExtension.empty() && oldExtension[0] == '.'); _dbg_assert_(!newExtension.empty() && newExtension[0] == '.'); - if (endsWithNoCase(path_, oldExtension)) { - std::string newPath = path_.substr(0, path_.size() - oldExtension.size()); - return Path(newPath + newExtension); - } else { - return Path(*this); + + if (type_ == PathType::CONTENT_URI) { + AndroidContentURI uri(path_); + AndroidContentURI replaced; + if (!uri.WithReplacedExtension(oldExtension, newExtension, &replaced)) { + return false; + } + *out = Path(replaced.ToString()); + return true; } + + if (!endsWithNoCase(path_, oldExtension)) { + return false; + } + *out = Path(path_.substr(0, path_.size() - oldExtension.size()) + newExtension); + return true; } Path Path::WithReplacedExtension(const std::string &newExtension) const { diff --git a/Common/File/Path.h b/Common/File/Path.h index 2b3cf3c535..a648d23672 100644 --- a/Common/File/Path.h +++ b/Common/File/Path.h @@ -86,7 +86,13 @@ public: // File extension manipulation. Path WithExtraExtension(std::string_view ext) const; - Path WithReplacedExtension(const std::string &oldExtension, const std::string &newExtension) const; + // Swaps one known extension for another, e.g. ".ppst" -> ".jpg". Returns false and leaves *out + // alone if the path doesn't actually end in oldExtension. That case has to be handled: this + // used to return the path unchanged, so a caller that guessed the extension wrong quietly + // carried on with the original file - and "delete the screenshot next to this savestate" then + // means "delete the savestate". + [[nodiscard]] bool WithReplacedExtension(const std::string &oldExtension, const std::string &newExtension, Path *out) const; + // Replaces whatever extension is there, so there's nothing to fail on. Path WithReplacedExtension(const std::string &newExtension) const; std::string GetFilename() const; // Really, GetLastComponent. Could be a file or directory. Includes the extension. diff --git a/UI/GameInfoCache.cpp b/UI/GameInfoCache.cpp index de57734e6c..5d353f2eb2 100644 --- a/UI/GameInfoCache.cpp +++ b/UI/GameInfoCache.cpp @@ -170,8 +170,8 @@ bool GameInfo::Delete() { const Path &ppstPath = filePath_; INFO_LOG(Log::System, "Deleting file %s", ppstPath.c_str()); MoveFileToTrashOrDelete(ppstPath); - const Path screenshotPath = filePath_.WithReplacedExtension(".ppst", ".jpg"); - if (File::Exists(screenshotPath)) { + Path screenshotPath; + if (filePath_.WithReplacedExtension(".ppst", ".jpg", &screenshotPath) && File::Exists(screenshotPath)) { MoveFileToTrashOrDelete(screenshotPath); } return true; @@ -766,8 +766,9 @@ handleELF: // Let's use the screenshot as an icon, too. if (flags_ & GameInfoFlags::ICON) { - Path screenshotPath = gamePath_.WithReplacedExtension(".ppst", ".jpg"); - if (ReadLocalFileToString(screenshotPath, &info_->icon.data, &info_->lock)) { + Path screenshotPath; + if (gamePath_.WithReplacedExtension(".ppst", ".jpg", &screenshotPath) && + ReadLocalFileToString(screenshotPath, &info_->icon.data, &info_->lock)) { info_->icon.dataLoaded = true; } } @@ -778,9 +779,10 @@ handleELF: { info_->SetTitle(info_->GetFilePath().GetFilename()); if (flags_ & GameInfoFlags::ICON) { - Path screenshotPath = gamePath_.WithReplacedExtension(".ppdmp", ".png"); // Let's use the comparison screenshot as an icon, if it exists. - if (screenshotPath.IsLocalType() && ReadLocalFileToString(screenshotPath, &info_->icon.data, &info_->lock)) { + Path screenshotPath; + if (gamePath_.WithReplacedExtension(".ppdmp", ".png", &screenshotPath) && + screenshotPath.IsLocalType() && ReadLocalFileToString(screenshotPath, &info_->icon.data, &info_->lock)) { info_->icon.dataLoaded = true; } } diff --git a/UI/SavedataScreen.cpp b/UI/SavedataScreen.cpp index 29aa0be2b9..755da9361c 100644 --- a/UI/SavedataScreen.cpp +++ b/UI/SavedataScreen.cpp @@ -86,8 +86,8 @@ SavedataView::SavedataView(UIContext &dc, const Path &savePath, IdentifiedFileTy Add(new Spacer(3.0)); } else { _dbg_assert_(type == IdentifiedFileType::PPSSPP_SAVESTATE); - Path image_path = savePath.WithReplacedExtension(".ppst", ".jpg"); - if (File::Exists(image_path)) { + Path image_path; + if (savePath.WithReplacedExtension(".ppst", ".jpg", &image_path) && File::Exists(image_path)) { toprow->Add(new AsyncImageFileView(image_path, IS_KEEP_ASPECT, new LinearLayoutParams(480, 272, Margins(10, 0)))); } else { auto sa = GetI18NCategory(I18NCat::SAVEDATA); diff --git a/headless/Compare.cpp b/headless/Compare.cpp index 359c766b95..16e7897dad 100644 --- a/headless/Compare.cpp +++ b/headless/Compare.cpp @@ -190,7 +190,12 @@ std::string GetTestName(const Path &bootFilename) } bool CompareOutput(const Path &bootFilename, const std::string &output, bool verbose, bool printEqualLines) { - Path expect_filename = bootFilename.GetFileExtension() == ".prx" ? bootFilename.WithReplacedExtension(".prx", ".expected") : bootFilename.WithExtraExtension(".expected"); + // A .prx test has its expectations next to it as ".expected"; anything else just gets the + // extension tacked on. + Path expect_filename; + if (!bootFilename.WithReplacedExtension(".prx", ".expected", &expect_filename)) { + expect_filename = bootFilename.WithExtraExtension(".expected"); + } std::unique_ptr expect_loader(ConstructFileLoader(expect_filename)); if (expect_loader->Exists()) { diff --git a/unittest/UnitTest.cpp b/unittest/UnitTest.cpp index fc4657c9f7..bc531e02d7 100644 --- a/unittest/UnitTest.cpp +++ b/unittest/UnitTest.cpp @@ -2236,7 +2236,15 @@ static bool TestPath() { EXPECT_EQ_INT((Path("") / "/etc/passwd").empty(), false); EXPECT_EQ_STR(Path("foo.bar/hello").GetFileExtension(), std::string()); - EXPECT_EQ_STR(Path("foo.bar/hello.txt").WithReplacedExtension(".txt", ".html").ToString(), std::string("foo.bar/hello.html")); + Path replaced("unset"); + EXPECT_EQ_INT(Path("foo.bar/hello.txt").WithReplacedExtension(".txt", ".html", &replaced), true); + EXPECT_EQ_STR(replaced.ToString(), std::string("foo.bar/hello.html")); + // The extension has to actually be there. This used to hand back "foo.bar/hello.txt", so a + // caller asking for the .html next to it would have been pointed at the .txt itself. + EXPECT_EQ_INT(Path("foo.bar/hello.txt").WithReplacedExtension(".png", ".html", &replaced), false); + EXPECT_EQ_STR(replaced.ToString(), std::string("foo.bar/hello.html")); // Untouched by the failure. + // Only the trailing extension counts - a dot earlier in the name isn't one. + EXPECT_EQ_INT(Path("foo.txt/hello").WithReplacedExtension(".txt", ".html", &replaced), false); EXPECT_EQ_STR(Path("C:\\Yo").NavigateUp().ToString(), std::string("C:")); #if PPSSPP_PLATFORM(WINDOWS)