From 106dbba0869fdd07da0223fdbf3178b1c3bee633 Mon Sep 17 00:00:00 2001 From: AnyOldName3 Date: Fri, 7 Jul 2023 13:05:48 +0000 Subject: [PATCH] Restore and clarify comments damaged by !2971 --- components/config/gamesettings.cpp | 16 ++++++++++++++-- components/files/configurationmanager.cpp | 16 +++++++++++++--- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/components/config/gamesettings.cpp b/components/config/gamesettings.cpp index 8f633f1e1e..f82ea95266 100644 --- a/components/config/gamesettings.cpp +++ b/components/config/gamesettings.cpp @@ -179,11 +179,18 @@ bool Config::GameSettings::writeFile(QTextStream& stream) { i--; + // path lines (e.g. 'data=...') need quotes and ampersands escaping to match how boost::filesystem::path uses + // boost::io::quoted + // We don't actually use boost::filesystem::path anymore, but use a custom class MaybeQuotedPath which uses + // Boost-like quoting rules but internally stores as a std::filesystem::path. + // Caution: This is intentional behaviour to duplicate how Boost and what we replaced it with worked, and we + // rely on that. if (i.key() == QLatin1String("data") || i.key() == QLatin1String("data-local") || i.key() == QLatin1String("resources") || i.key() == QLatin1String("load-savegame")) { stream << i.key() << "="; + // Equivalent to stream << std::quoted(i.value(), '"', '&'), which won't work on QStrings. QChar delim = '\"'; QChar escape = '&'; QString string = i.value(); @@ -396,13 +403,18 @@ bool Config::GameSettings::writeFileWithComments(QFile& file) { it--; + // path lines (e.g. 'data=...') need quotes and ampersands escaping to match how boost::filesystem::path uses + // boost::io::quoted + // We don't actually use boost::filesystem::path anymore, but use a custom class MaybeQuotedPath which uses + // Boost-like quoting rules but internally stores as a std::filesystem::path. + // Caution: This is intentional behaviour to duplicate how Boost and what we replaced it with worked, and we + // rely on that. if (it.key() == QLatin1String("data") || it.key() == QLatin1String("data-local") || it.key() == QLatin1String("resources") || it.key() == QLatin1String("load-savegame")) { settingLine = it.key() + "="; - // The following is based on boost::io::detail::quoted_manip.hpp, but calling those functions did not work - // as there are too may QStrings involved + // Equivalent to settingLine += std::quoted(it.value(), '"', '&'), which won't work on QStrings. QChar delim = '\"'; QChar escape = '&'; QString string = it.value(); diff --git a/components/files/configurationmanager.cpp b/components/files/configurationmanager.cpp index 7206126599..ece30e5b3f 100644 --- a/components/files/configurationmanager.cpp +++ b/components/files/configurationmanager.cpp @@ -440,12 +440,22 @@ namespace Files std::istream& operator>>(std::istream& istream, MaybeQuotedPath& MaybeQuotedPath) { - // If the stream starts with a double quote, read from stream using std::filesystem::path rules, then discard - // anything remaining. This prevents boost::program_options getting upset that we've not consumed the whole - // stream. If it doesn't start with a double quote, read the whole thing verbatim + // If the stream starts with a double quote, read from stream using boost::filesystem::path rules (which are not + // the same as std::filesystem::path rules), then discard anything remaining. This prevents + // boost::program_options getting upset that we've not consumed the whole stream. If it doesn't start with a + // double quote, read the whole thing verbatim. + + // This function's implementation and comments have a history of being changed by well-meaning but incorrect + // OpenMW developers. Please be very careful you aren't joining them. If you're insufficiently apprehensive, + // here's a 737-word GitLab comment to scare you: + // https://gitlab.com/OpenMW/openmw/-/merge_requests/2979#note_1371082428. if (istream.peek() == '"') { std::string intermediate; + // std::filesystem::path would use '"', '\' here. + // We use & because it's easier when copying and pasting Windows paths, which have many backslashes and few + // ampersands, and because it's backwards-compatible with the previous format, which used + // boost::filesystem::path's operator>>. istream >> std::quoted(intermediate, '"', '&'); static_cast(MaybeQuotedPath) = Misc::StringUtils::stringToU8String(intermediate); if (istream && !istream.eof() && istream.peek() != EOF)