From f372373e6f1305ac6cbd9ab988a014d3e9a95b1b Mon Sep 17 00:00:00 2001 From: Rubidium Date: Sun, 30 Apr 2023 09:51:37 +0200 Subject: [PATCH 01/25] Codechange: use std::string/vector for language cases over manual management --- src/strgen/strgen.h | 10 ++++------ src/strgen/strgen_base.cpp | 40 +++++++++++--------------------------- 2 files changed, 15 insertions(+), 35 deletions(-) diff --git a/src/strgen/strgen.h b/src/strgen/strgen.h index 4ea9c328d5..d3b89c0d51 100644 --- a/src/strgen/strgen.h +++ b/src/strgen/strgen.h @@ -15,12 +15,10 @@ /** Container for the different cases of a string. */ struct Case { - int caseidx; ///< The index of the case. - char *string; ///< The translation of the case. - Case *next; ///< The next, chained, case. + int caseidx; ///< The index of the case. + std::string string; ///< The translation of the case. - Case(int caseidx, const char *string, Case *next); - ~Case(); + Case(int caseidx, const std::string &string); }; /** Information about a single string. */ @@ -31,7 +29,7 @@ struct LangString { size_t hash_next; ///< Next hash entry. size_t index; ///< The index in the language file. int line; ///< Line of string in source-file. - Case *translated_case; ///< Cases of the translation. + std::vector translated_cases; ///< Cases of the translation. LangString(const char *name, const char *english, size_t index, int line); ~LangString(); diff --git a/src/strgen/strgen_base.cpp b/src/strgen/strgen_base.cpp index 4f190f47e2..8f6e047cf5 100644 --- a/src/strgen/strgen_base.cpp +++ b/src/strgen/strgen_base.cpp @@ -38,20 +38,12 @@ static const CmdStruct *ParseCommandString(const char **str, char *param, int *a * Create a new case. * @param caseidx The index of the case. * @param string The translation of the case. - * @param next The next chained case. */ -Case::Case(int caseidx, const char *string, Case *next) : - caseidx(caseidx), string(stredup(string)), next(next) +Case::Case(int caseidx, const std::string &string) : + caseidx(caseidx), string(string) { } -/** Free everything we allocated. */ -Case::~Case() -{ - free(this->string); - delete this->next; -} - /** * Create a new string. * @param name The name of the string. @@ -61,7 +53,7 @@ Case::~Case() */ LangString::LangString(const char *name, const char *english, size_t index, int line) : name(stredup(name)), english(stredup(english)), translated(nullptr), - hash_next(0), index(index), line(line), translated_case(nullptr) + hash_next(0), index(index), line(line) { } @@ -71,7 +63,6 @@ LangString::~LangString() free(this->name); free(this->english); free(this->translated); - delete this->translated_case; } /** Free all data related to the translation. */ @@ -80,8 +71,7 @@ void LangString::FreeTranslation() free(this->translated); this->translated = nullptr; - delete this->translated_case; - this->translated_case = nullptr; + this->translated_cases.clear(); } /** @@ -776,7 +766,7 @@ void StringReader::HandleString(char *str) if (!CheckCommandsMatch(s, ent->english, str)) return; if (casep != nullptr) { - ent->translated_case = new Case(ResolveCaseName(casep, strlen(casep)), s, ent->translated_case); + ent->translated_cases.emplace_back(ResolveCaseName(casep, strlen(casep)), s); } else { ent->translated = stredup(s); /* If the string was translated, use the line from the @@ -975,7 +965,6 @@ void LanguageWriter::WriteLang(const StringData &data) for (size_t tab = 0; tab < data.tabs; tab++) { for (uint j = 0; j != in_use[tab]; j++) { const LangString *ls = data.strings[(tab * TAB_SIZE) + j]; - const Case *casep; const char *cmdp; /* For undefined strings, just set that it's an empty string */ @@ -1001,38 +990,31 @@ void LanguageWriter::WriteLang(const StringData &data) /* Extract the strings and stuff from the english command string */ ExtractCommandString(&_cur_pcs, ls->english, false); - if (ls->translated_case != nullptr || ls->translated != nullptr) { - casep = ls->translated_case; + if (!ls->translated_cases.empty() || ls->translated != nullptr) { cmdp = ls->translated; } else { - casep = nullptr; cmdp = ls->english; } _translated = cmdp != ls->english; - if (casep != nullptr) { - const Case *c; - uint num; - + if (!ls->translated_cases.empty()) { /* Need to output a case-switch. * It has this format * <0x9E> * Each LEN is printed using 2 bytes in big endian order. */ buffer.AppendUtf8(SCC_SWITCH_CASE); - /* Count the number of cases */ - for (num = 0, c = casep; c; c = c->next) num++; - buffer.AppendByte(num); + buffer.AppendByte((byte)ls->translated_cases.size()); /* Write each case */ - for (c = casep; c != nullptr; c = c->next) { - buffer.AppendByte(c->caseidx); + for (const Case &c : ls->translated_cases) { + buffer.AppendByte(c.caseidx); /* Make some space for the 16-bit length */ uint pos = (uint)buffer.size(); buffer.AppendByte(0); buffer.AppendByte(0); /* Write string */ - PutCommandString(&buffer, c->string); + PutCommandString(&buffer, c.string.c_str()); buffer.AppendByte(0); // terminate with a zero /* Fill in the length */ uint size = (uint)buffer.size() - (pos + 2); From 234a143ee51c72e4880501610a9430df3951853e Mon Sep 17 00:00:00 2001 From: Rubidium Date: Sun, 30 Apr 2023 10:22:14 +0200 Subject: [PATCH 02/25] Codechange: use std::string for LangString over C-style strings --- src/game/game_text.cpp | 4 +-- src/strgen/strgen.cpp | 2 +- src/strgen/strgen.h | 17 ++++++------- src/strgen/strgen_base.cpp | 51 +++++++++++++++----------------------- 4 files changed, 31 insertions(+), 43 deletions(-) diff --git a/src/game/game_text.cpp b/src/game/game_text.cpp index 25ca95cbdd..6168401e7d 100644 --- a/src/game/game_text.cpp +++ b/src/game/game_text.cpp @@ -158,7 +158,7 @@ struct StringNameWriter : HeaderWriter { { } - void WriteStringID(const char *name, int stringid) + void WriteStringID(const std::string &name, int stringid) { if (stringid == (int)this->strings.size()) this->strings.emplace_back(name); } @@ -271,7 +271,7 @@ static void ExtractStringParams(const StringData &data, StringParamsList ¶ms if (ls != nullptr) { StringParams ¶m = params.emplace_back(); ParsedCommandStruct pcs; - ExtractCommandString(&pcs, ls->english, false); + ExtractCommandString(&pcs, ls->english.c_str(), false); for (const CmdStruct *cs : pcs.cmd) { if (cs == nullptr) break; diff --git a/src/strgen/strgen.cpp b/src/strgen/strgen.cpp index 804d091e6b..f264964378 100644 --- a/src/strgen/strgen.cpp +++ b/src/strgen/strgen.cpp @@ -236,7 +236,7 @@ struct HeaderFileWriter : HeaderWriter, FileWriter { this->output_stream << "#define TABLE_STRINGS_H\n"; } - void WriteStringID(const char *name, int stringid) + void WriteStringID(const std::string &name, int stringid) { if (prev + 1 != stringid) this->output_stream << "\n"; fmt::print(this->output_stream, "static const StringID {} = 0x{:X};\n", name, stringid); diff --git a/src/strgen/strgen.h b/src/strgen/strgen.h index d3b89c0d51..544454ed1a 100644 --- a/src/strgen/strgen.h +++ b/src/strgen/strgen.h @@ -23,16 +23,15 @@ struct Case { /** Information about a single string. */ struct LangString { - char *name; ///< Name of the string. - char *english; ///< English text. - char *translated; ///< Translated text. - size_t hash_next; ///< Next hash entry. - size_t index; ///< The index in the language file. - int line; ///< Line of string in source-file. + std::string name; ///< Name of the string. + std::string english; ///< English text. + std::string translated; ///< Translated text. + size_t hash_next; ///< Next hash entry. + size_t index; ///< The index in the language file. + int line; ///< Line of string in source-file. std::vector translated_cases; ///< Cases of the translation. - LangString(const char *name, const char *english, size_t index, int line); - ~LangString(); + LangString(const std::string &name, const std::string &english, size_t index, int line); void FreeTranslation(); }; @@ -93,7 +92,7 @@ struct HeaderWriter { * @param name The name of the string. * @param stringid The ID of the string. */ - virtual void WriteStringID(const char *name, int stringid) = 0; + virtual void WriteStringID(const std::string &name, int stringid) = 0; /** * Finalise writing the file. diff --git a/src/strgen/strgen_base.cpp b/src/strgen/strgen_base.cpp index 8f6e047cf5..3ddeffb5ea 100644 --- a/src/strgen/strgen_base.cpp +++ b/src/strgen/strgen_base.cpp @@ -51,26 +51,15 @@ Case::Case(int caseidx, const std::string &string) : * @param index The index in the string table. * @param line The line this string was found on. */ -LangString::LangString(const char *name, const char *english, size_t index, int line) : - name(stredup(name)), english(stredup(english)), translated(nullptr), - hash_next(0), index(index), line(line) +LangString::LangString(const std::string &name, const std::string &english, size_t index, int line) : + name(name), english(english), hash_next(0), index(index), line(line) { } -/** Free everything we allocated. */ -LangString::~LangString() -{ - free(this->name); - free(this->english); - free(this->translated); -} - /** Free all data related to the translation. */ void LangString::FreeTranslation() { - free(this->translated); - this->translated = nullptr; - + this->translated.clear(); this->translated_cases.clear(); } @@ -140,7 +129,7 @@ LangString *StringData::Find(const char *s) while (idx-- > 0) { LangString *ls = this->strings[idx]; - if (strcmp(ls->name, s) == 0) return ls; + if (ls->name == s) return ls; idx = ls->hash_next; } return nullptr; @@ -179,12 +168,12 @@ uint StringData::Version() const int argno; int casei; - s = ls->name; + s = ls->name.c_str(); hash ^= i * 0x717239; hash = (hash & 1 ? hash >> 1 ^ 0xDEADBEEF : hash >> 1); hash = this->VersionHashStr(hash, s + 1); - s = ls->english; + s = ls->english.c_str(); while ((cs = ParseCommandString(&s, buf, &argno, &casei)) != nullptr) { if (cs->flags & C_DONTCOUNT) continue; @@ -630,7 +619,7 @@ const CmdStruct *TranslateCmdForCompare(const CmdStruct *a) } -static bool CheckCommandsMatch(char *a, char *b, const char *name) +static bool CheckCommandsMatch(const char *a, const char *b, const char *name) { /* If we're not translating, i.e. we're compiling the base language, * it is pointless to do all these checks as it'll always be correct. @@ -757,18 +746,18 @@ void StringReader::HandleString(char *str) return; } - if (ent->translated && casep == nullptr) { + if (!ent->translated.empty() && casep == nullptr) { StrgenError("String name '{}' is used multiple times", str); return; } /* make sure that the commands match */ - if (!CheckCommandsMatch(s, ent->english, str)) return; + if (!CheckCommandsMatch(s, ent->english.c_str(), str)) return; if (casep != nullptr) { ent->translated_cases.emplace_back(ResolveCaseName(casep, strlen(casep)), s); } else { - ent->translated = stredup(s); + ent->translated = s; /* If the string was translated, use the line from the * translated language so errors in the translated file * are properly referenced to. */ @@ -950,7 +939,7 @@ void LanguageWriter::WriteLang(const StringData &data) for (uint j = 0; j != in_use[tab]; j++) { const LangString *ls = data.strings[(tab * TAB_SIZE) + j]; - if (ls != nullptr && ls->translated == nullptr) _lang.missing++; + if (ls != nullptr && ls->translated.empty()) _lang.missing++; } } @@ -965,7 +954,7 @@ void LanguageWriter::WriteLang(const StringData &data) for (size_t tab = 0; tab < data.tabs; tab++) { for (uint j = 0; j != in_use[tab]; j++) { const LangString *ls = data.strings[(tab * TAB_SIZE) + j]; - const char *cmdp; + const std::string *cmdp; /* For undefined strings, just set that it's an empty string */ if (ls == nullptr) { @@ -973,11 +962,11 @@ void LanguageWriter::WriteLang(const StringData &data) continue; } - _cur_ident = ls->name; + _cur_ident = ls->name.c_str(); _cur_line = ls->line; /* Produce a message if a string doesn't have a translation. */ - if (_show_todo > 0 && ls->translated == nullptr) { + if (_show_todo > 0 && ls->translated.empty()) { if ((_show_todo & 2) != 0) { StrgenWarning("'{}' is untranslated", ls->name); } @@ -988,15 +977,15 @@ void LanguageWriter::WriteLang(const StringData &data) } /* Extract the strings and stuff from the english command string */ - ExtractCommandString(&_cur_pcs, ls->english, false); + ExtractCommandString(&_cur_pcs, ls->english.c_str(), false); - if (!ls->translated_cases.empty() || ls->translated != nullptr) { - cmdp = ls->translated; + if (!ls->translated_cases.empty() || !ls->translated.empty()) { + cmdp = &ls->translated; } else { - cmdp = ls->english; + cmdp = &ls->english; } - _translated = cmdp != ls->english; + _translated = cmdp != &ls->english; if (!ls->translated_cases.empty()) { /* Need to output a case-switch. @@ -1023,7 +1012,7 @@ void LanguageWriter::WriteLang(const StringData &data) } } - if (cmdp != nullptr) PutCommandString(&buffer, cmdp); + if (!cmdp->empty()) PutCommandString(&buffer, cmdp->c_str()); this->WriteLength((uint)buffer.size()); this->Write(buffer.data(), buffer.size()); From 4f94655cc23242a60e504c85baecb0fd3dbde99e Mon Sep 17 00:00:00 2001 From: Rubidium Date: Sun, 30 Apr 2023 18:39:53 +0200 Subject: [PATCH 03/25] Codechange: use vector/unorder_map over custom implementation --- src/game/game_text.cpp | 2 +- src/strgen/strgen.h | 20 +++++------ src/strgen/strgen_base.cpp | 69 ++++++++++---------------------------- 3 files changed, 28 insertions(+), 63 deletions(-) diff --git a/src/game/game_text.cpp b/src/game/game_text.cpp index 6168401e7d..694827971f 100644 --- a/src/game/game_text.cpp +++ b/src/game/game_text.cpp @@ -266,7 +266,7 @@ static StringParam::ParamType GetParamType(const CmdStruct *cs) static void ExtractStringParams(const StringData &data, StringParamsList ¶ms) { for (size_t i = 0; i < data.max_strings; i++) { - const LangString *ls = data.strings[i]; + const LangString *ls = data.strings[i].get(); if (ls != nullptr) { StringParams ¶m = params.emplace_back(); diff --git a/src/strgen/strgen.h b/src/strgen/strgen.h index 544454ed1a..6c793a5c0b 100644 --- a/src/strgen/strgen.h +++ b/src/strgen/strgen.h @@ -13,6 +13,9 @@ #include "../language.h" #include "../3rdparty/fmt/format.h" +#include +#include + /** Container for the different cases of a string. */ struct Case { int caseidx; ///< The index of the case. @@ -26,7 +29,6 @@ struct LangString { std::string name; ///< Name of the string. std::string english; ///< English text. std::string translated; ///< Translated text. - size_t hash_next; ///< Next hash entry. size_t index; ///< The index in the language file. int line; ///< Line of string in source-file. std::vector translated_cases; ///< Cases of the translation. @@ -37,18 +39,16 @@ struct LangString { /** Information about the currently known strings. */ struct StringData { - LangString **strings; ///< Array of all known strings. - size_t *hash_heads; ///< Hash table for the strings. + std::vector> strings; ///< List of all known strings. + std::unordered_map name_to_string; ///< Lookup table for the strings. size_t tabs; ///< The number of 'tabs' of strings. size_t max_strings; ///< The maximum number of strings. size_t next_string_id;///< The next string ID to allocate. StringData(size_t tabs); - ~StringData(); void FreeTranslation(); - uint HashStr(const char *s) const; - void Add(const char *s, LangString *ls); - LangString *Find(const char *s); + void Add(std::unique_ptr ls); + LangString *Find(const std::string_view s); uint VersionHashStr(uint hash, const char *s) const; uint Version() const; uint CountInUse(uint tab) const; @@ -57,12 +57,12 @@ struct StringData { /** Helper for reading strings. */ struct StringReader { StringData &data; ///< The data to fill during reading. - const char *file; ///< The file we are reading. + const std::string file; ///< The file we are reading. bool master; ///< Are we reading the master file? bool translation; ///< Are we reading a translation, implies !master. However, the base translation will have this false. - StringReader(StringData &data, const char *file, bool master, bool translation); - virtual ~StringReader(); + StringReader(StringData &data, const std::string &file, bool master, bool translation); + virtual ~StringReader() {} void HandleString(char *str); /** diff --git a/src/strgen/strgen_base.cpp b/src/strgen/strgen_base.cpp index 3ddeffb5ea..5775da5c17 100644 --- a/src/strgen/strgen_base.cpp +++ b/src/strgen/strgen_base.cpp @@ -52,7 +52,7 @@ Case::Case(int caseidx, const std::string &string) : * @param line The line this string was found on. */ LangString::LangString(const std::string &name, const std::string &english, size_t index, int line) : - name(name), english(english), hash_next(0), index(index), line(line) + name(name), english(english), index(index), line(line) { } @@ -69,52 +69,28 @@ void LangString::FreeTranslation() */ StringData::StringData(size_t tabs) : tabs(tabs), max_strings(tabs * TAB_SIZE) { - this->strings = CallocT(max_strings); - this->hash_heads = CallocT(max_strings); + this->strings.resize(max_strings); this->next_string_id = 0; } -/** Free everything we allocated. */ -StringData::~StringData() -{ - for (size_t i = 0; i < this->max_strings; i++) delete this->strings[i]; - free(this->strings); - free(this->hash_heads); -} - /** Free all data related to the translation. */ void StringData::FreeTranslation() { for (size_t i = 0; i < this->max_strings; i++) { - LangString *ls = this->strings[i]; + LangString *ls = this->strings[i].get(); if (ls != nullptr) ls->FreeTranslation(); } } -/** - * Create a hash of the string for finding them back quickly. - * @param s The string to hash. - * @return The hashed string. - */ -uint StringData::HashStr(const char *s) const -{ - uint hash = 0; - for (; *s != '\0'; s++) hash = ROL(hash, 3) ^ *s; - return hash % this->max_strings; -} - /** * Add a newly created LangString. * @param s The name of the string. * @param ls The string to add. */ -void StringData::Add(const char *s, LangString *ls) +void StringData::Add(std::unique_ptr ls) { - uint hash = this->HashStr(s); - ls->hash_next = this->hash_heads[hash]; - /* Off-by-one for hash find. */ - this->hash_heads[hash] = ls->index + 1; - this->strings[ls->index] = ls; + this->name_to_string[ls->name] = ls.get(); + this->strings[ls->index].swap(ls); } /** @@ -122,17 +98,12 @@ void StringData::Add(const char *s, LangString *ls) * @param s The string name to search on. * @return The LangString or nullptr if it is not known. */ -LangString *StringData::Find(const char *s) +LangString *StringData::Find(const std::string_view s) { - size_t idx = this->hash_heads[this->HashStr(s)]; + auto it = this->name_to_string.find(s); + if (it == this->name_to_string.end()) return nullptr; - while (idx-- > 0) { - LangString *ls = this->strings[idx]; - - if (ls->name == s) return ls; - idx = ls->hash_next; - } - return nullptr; + return it->second; } /** @@ -159,7 +130,7 @@ uint StringData::Version() const uint hash = 0; for (size_t i = 0; i < this->max_strings; i++) { - const LangString *ls = this->strings[i]; + const LangString *ls = this->strings[i].get(); if (ls != nullptr) { const CmdStruct *cs; @@ -555,17 +526,11 @@ static const CmdStruct *ParseCommandString(const char **str, char *param, int *a * @param master Are we reading the master file? * @param translation Are we reading a translation? */ -StringReader::StringReader(StringData &data, const char *file, bool master, bool translation) : - data(data), file(stredup(file)), master(master), translation(translation) +StringReader::StringReader(StringData &data, const std::string &file, bool master, bool translation) : + data(data), file(file), master(master), translation(translation) { } -/** Make sure the right reader gets freed. */ -StringReader::~StringReader() -{ - free(file); -} - void ExtractCommandString(ParsedCommandStruct *p, const char *s, bool warnings) { char param[MAX_COMMAND_PARAM_SIZE]; @@ -739,7 +704,7 @@ void StringReader::HandleString(char *str) } /* Allocate a new LangString */ - this->data.Add(str, new LangString(str, s, this->data.next_string_id++, _cur_line)); + this->data.Add(std::make_unique(str, s, this->data.next_string_id++, _cur_line)); } else { if (ent == nullptr) { StrgenWarning("String name '{}' does not exist in master file", str); @@ -791,7 +756,7 @@ void StringReader::ParseFile() _warnings = _errors = 0; _translation = this->translation; - _file = this->file; + _file = this->file.c_str(); /* Abusing _show_todo to replace "warning" with "info" for translations. */ _show_todo &= 3; @@ -938,7 +903,7 @@ void LanguageWriter::WriteLang(const StringData &data) _lang.offsets[tab] = TO_LE16(n); for (uint j = 0; j != in_use[tab]; j++) { - const LangString *ls = data.strings[(tab * TAB_SIZE) + j]; + const LangString *ls = data.strings[(tab * TAB_SIZE) + j].get(); if (ls != nullptr && ls->translated.empty()) _lang.missing++; } } @@ -953,7 +918,7 @@ void LanguageWriter::WriteLang(const StringData &data) for (size_t tab = 0; tab < data.tabs; tab++) { for (uint j = 0; j != in_use[tab]; j++) { - const LangString *ls = data.strings[(tab * TAB_SIZE) + j]; + const LangString *ls = data.strings[(tab * TAB_SIZE) + j].get(); const std::string *cmdp; /* For undefined strings, just set that it's an empty string */ From 802d6cb5093210c214e175f52d2b797b93a09901 Mon Sep 17 00:00:00 2001 From: Rubidium Date: Sun, 30 Apr 2023 19:17:42 +0200 Subject: [PATCH 04/25] Fix: memory leak when parsing (strgen) commands by moving to C++ containers --- src/game/game_text.cpp | 5 ++-- src/strgen/strgen.h | 11 ++++--- src/strgen/strgen_base.cpp | 60 ++++++++++++++++++-------------------- 3 files changed, 35 insertions(+), 41 deletions(-) diff --git a/src/game/game_text.cpp b/src/game/game_text.cpp index 694827971f..adfdcd6ebe 100644 --- a/src/game/game_text.cpp +++ b/src/game/game_text.cpp @@ -270,10 +270,9 @@ static void ExtractStringParams(const StringData &data, StringParamsList ¶ms if (ls != nullptr) { StringParams ¶m = params.emplace_back(); - ParsedCommandStruct pcs; - ExtractCommandString(&pcs, ls->english.c_str(), false); + ParsedCommandStruct pcs = ExtractCommandString(ls->english.c_str(), false); - for (const CmdStruct *cs : pcs.cmd) { + for (const CmdStruct *cs : pcs.consuming_commands) { if (cs == nullptr) break; param.emplace_back(GetParamType(cs), cs->consumes); } diff --git a/src/strgen/strgen.h b/src/strgen/strgen.h index 6c793a5c0b..593060bb15 100644 --- a/src/strgen/strgen.h +++ b/src/strgen/strgen.h @@ -137,18 +137,17 @@ struct LanguageWriter { struct CmdStruct; struct CmdPair { - const CmdStruct *a; - const char *v; + const CmdStruct *cmd; + std::string param; }; struct ParsedCommandStruct { - uint np; - CmdPair pairs[32]; - const CmdStruct *cmd[32]; // ordered by param # + std::vector non_consuming_commands; + std::array consuming_commands{ nullptr }; // ordered by param # }; const CmdStruct *TranslateCmdForCompare(const CmdStruct *a); -void ExtractCommandString(ParsedCommandStruct *p, const char *s, bool warnings); +ParsedCommandStruct ExtractCommandString(const char *s, bool warnings); void StrgenWarningI(const std::string &msg); void StrgenErrorI(const std::string &msg); diff --git a/src/strgen/strgen_base.cpp b/src/strgen/strgen_base.cpp index 5775da5c17..b114dea3d5 100644 --- a/src/strgen/strgen_base.cpp +++ b/src/strgen/strgen_base.cpp @@ -336,7 +336,7 @@ void EmitPlural(Buffer *buffer, char *buf, int value) /* Parse out the number, if one exists. Otherwise default to prev arg. */ if (!ParseRelNum(&buf, &argidx, &offset)) argidx--; - const CmdStruct *cmd = _cur_pcs.cmd[argidx]; + const CmdStruct *cmd = _cur_pcs.consuming_commands[argidx]; if (offset == -1) { /* Use default offset */ if (cmd == nullptr || cmd->default_plural_offset < 0) { @@ -401,7 +401,7 @@ void EmitGender(Buffer *buffer, char *buf, int value) * If no relative number exists, default to +0 */ ParseRelNum(&buf, &argidx, &offset); - const CmdStruct *cmd = _cur_pcs.cmd[argidx]; + const CmdStruct *cmd = _cur_pcs.consuming_commands[argidx]; if (cmd == nullptr || (cmd->flags & C_GENDER) == 0) { StrgenFatal("Command '{}' can't have a gender", cmd == nullptr ? "" : cmd->cmd); } @@ -531,14 +531,14 @@ StringReader::StringReader(StringData &data, const std::string &file, bool maste { } -void ExtractCommandString(ParsedCommandStruct *p, const char *s, bool warnings) +ParsedCommandStruct ExtractCommandString(const char *s, bool warnings) { char param[MAX_COMMAND_PARAM_SIZE]; int argno; int argidx = 0; int casei; - memset(p, 0, sizeof(*p)); + ParsedCommandStruct p; for (;;) { /* read until next command from a. */ @@ -551,17 +551,16 @@ void ExtractCommandString(ParsedCommandStruct *p, const char *s, bool warnings) if (ar->consumes) { if (argno != -1) argidx = argno; - if (argidx < 0 || (uint)argidx >= lengthof(p->cmd)) StrgenFatal("invalid param idx {}", argidx); - if (p->cmd[argidx] != nullptr && p->cmd[argidx] != ar) StrgenFatal("duplicate param idx {}", argidx); + if (argidx < 0 || (uint)argidx >= p.consuming_commands.max_size()) StrgenFatal("invalid param idx {}", argidx); + if (p.consuming_commands[argidx] != nullptr && p.consuming_commands[argidx] != ar) StrgenFatal("duplicate param idx {}", argidx); - p->cmd[argidx++] = ar; + p.consuming_commands[argidx++] = ar; } else if (!(ar->flags & C_DONTCOUNT)) { // Ignore some of them - if (p->np >= lengthof(p->pairs)) StrgenFatal("too many commands in string, max {}", lengthof(p->pairs)); - p->pairs[p->np].a = ar; - p->pairs[p->np].v = param[0] != '\0' ? stredup(param) : ""; - p->np++; + p.non_consuming_commands.emplace_back(CmdPair{ar, param}); } } + + return p; } @@ -592,45 +591,42 @@ static bool CheckCommandsMatch(const char *a, const char *b, const char *name) */ if (!_translation) return true; - ParsedCommandStruct templ; - ParsedCommandStruct lang; bool result = true; - ExtractCommandString(&templ, b, true); - ExtractCommandString(&lang, a, true); + ParsedCommandStruct templ = ExtractCommandString(b, true); + ParsedCommandStruct lang = ExtractCommandString(a, true); /* For each string in templ, see if we find it in lang */ - if (templ.np != lang.np) { + if (templ.non_consuming_commands.max_size() != lang.non_consuming_commands.max_size()) { StrgenWarning("{}: template string and language string have a different # of commands", name); result = false; } - for (uint i = 0; i < templ.np; i++) { + for (auto &templ_nc : templ.non_consuming_commands) { /* see if we find it in lang, and zero it out */ bool found = false; - for (uint j = 0; j < lang.np; j++) { - if (templ.pairs[i].a == lang.pairs[j].a && - strcmp(templ.pairs[i].v, lang.pairs[j].v) == 0) { + for (auto &lang_nc : lang.non_consuming_commands) { + if (templ_nc.cmd == lang_nc.cmd && templ_nc.param == lang_nc.param) { /* it was found in both. zero it out from lang so we don't find it again */ - lang.pairs[j].a = nullptr; + lang_nc.cmd = nullptr; found = true; break; } } if (!found) { - StrgenWarning("{}: command '{}' exists in template file but not in language file", name, templ.pairs[i].a->cmd); + StrgenWarning("{}: command '{}' exists in template file but not in language file", name, templ_nc.cmd->cmd); result = false; } } /* if we reach here, all non consumer commands match up. * Check if the non consumer commands match up also. */ - for (uint i = 0; i < lengthof(templ.cmd); i++) { - if (TranslateCmdForCompare(templ.cmd[i]) != lang.cmd[i]) { + for (uint i = 0; i < templ.consuming_commands.max_size(); i++) { + if (TranslateCmdForCompare(templ.consuming_commands[i]) != lang.consuming_commands[i]) { StrgenWarning("{}: Param idx #{} '{}' doesn't match with template command '{}'", name, i, - lang.cmd[i] == nullptr ? "" : TranslateCmdForCompare(lang.cmd[i])->cmd, - templ.cmd[i] == nullptr ? "" : templ.cmd[i]->cmd); + lang.consuming_commands[i] == nullptr ? "" : TranslateCmdForCompare(lang.consuming_commands[i])->cmd, + templ.consuming_commands[i] == nullptr ? "" : templ.consuming_commands[i]->cmd); result = false; } } @@ -801,20 +797,20 @@ static int TranslateArgumentIdx(int argidx, int offset) { int sum; - if (argidx < 0 || (uint)argidx >= lengthof(_cur_pcs.cmd)) { + if (argidx < 0 || (uint)argidx >= _cur_pcs.consuming_commands.max_size()) { StrgenFatal("invalid argidx {}", argidx); } - const CmdStruct *cs = _cur_pcs.cmd[argidx]; + const CmdStruct *cs = _cur_pcs.consuming_commands[argidx]; if (cs != nullptr && cs->consumes <= offset) { StrgenFatal("invalid argidx offset {}:{}", argidx, offset); } - if (_cur_pcs.cmd[argidx] == nullptr) { + if (_cur_pcs.consuming_commands[argidx] == nullptr) { StrgenFatal("no command for this argidx {}", argidx); } for (int i = sum = 0; i < argidx; i++) { - cs = _cur_pcs.cmd[i]; + cs = _cur_pcs.consuming_commands[i]; sum += (cs != nullptr) ? cs->consumes : 1; } @@ -860,7 +856,7 @@ static void PutCommandString(Buffer *buffer, const char *str) } /* Output the one from the master string... it's always accurate. */ - cs = _cur_pcs.cmd[_cur_argidx++]; + cs = _cur_pcs.consuming_commands[_cur_argidx++]; if (cs == nullptr) { StrgenFatal("{}: No argument exists at position {}", _cur_ident, _cur_argidx - 1); } @@ -942,7 +938,7 @@ void LanguageWriter::WriteLang(const StringData &data) } /* Extract the strings and stuff from the english command string */ - ExtractCommandString(&_cur_pcs, ls->english.c_str(), false); + _cur_pcs = ExtractCommandString(ls->english.c_str(), false); if (!ls->translated_cases.empty() || !ls->translated.empty()) { cmdp = &ls->translated; From 10e12154f5e94e6b4856df9992235398f0139c3a Mon Sep 17 00:00:00 2001 From: Rubidium Date: Mon, 22 May 2023 18:06:05 +0200 Subject: [PATCH 05/25] Fix: false positive warning in fmt library (backport ef55d4f of upstream fmt) --- src/3rdparty/fmt/core.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/3rdparty/fmt/core.h b/src/3rdparty/fmt/core.h index d6333bbeb1..26517a4958 100644 --- a/src/3rdparty/fmt/core.h +++ b/src/3rdparty/fmt/core.h @@ -1666,8 +1666,7 @@ constexpr auto encode_types() -> unsigned long long { template FMT_CONSTEXPR FMT_INLINE auto make_value(T&& val) -> value { - auto&& arg = arg_mapper().map(FMT_FORWARD(val)); - using arg_type = remove_cvref_t; + using arg_type = remove_cvref_t().map(val))>; constexpr bool formattable_char = !std::is_same::value; @@ -1686,7 +1685,7 @@ FMT_CONSTEXPR FMT_INLINE auto make_value(T&& val) -> value { formattable, "Cannot format an argument. To make type T formattable provide a " "formatter specialization: https://fmt.dev/latest/api.html#udt"); - return {arg}; + return {arg_mapper().map(val)}; } template From 53f83c31b01e798299b98aaf5ccaf61aa99b8109 Mon Sep 17 00:00:00 2001 From: Rubidium Date: Fri, 2 Jun 2023 16:08:45 +0200 Subject: [PATCH 06/25] Codechange: use std::string to return the debug level information --- src/debug.cpp | 20 ++++++-------------- src/debug.h | 2 +- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/src/debug.cpp b/src/debug.cpp index 8876fdd021..46d6495324 100644 --- a/src/debug.cpp +++ b/src/debug.cpp @@ -220,22 +220,14 @@ void SetDebugString(const char *s, void (*error_func)(const std::string &)) * Just return a string with the values of all the debug categories. * @return string with debug-levels */ -const char *GetDebugString() +std::string GetDebugString() { - const DebugLevel *i; - static char dbgstr[150]; - char dbgval[20]; - - memset(dbgstr, 0, sizeof(dbgstr)); - i = debug_level; - seprintf(dbgstr, lastof(dbgstr), "%s=%d", i->name, *i->level); - - for (i++; i != endof(debug_level); i++) { - seprintf(dbgval, lastof(dbgval), ", %s=%d", i->name, *i->level); - strecat(dbgstr, dbgval, lastof(dbgstr)); + std::string result; + for (const DebugLevel *i = debug_level; i != endof(debug_level); ++i) { + if (!result.empty()) result += ", "; + fmt::format_to(std::back_inserter(result), "{}={}", i->name, *i->level); } - - return dbgstr; + return result; } /** diff --git a/src/debug.h b/src/debug.h index 2db1d1132b..c2f24e1494 100644 --- a/src/debug.h +++ b/src/debug.h @@ -58,7 +58,7 @@ extern int _debug_random_level; void DumpDebugFacilityNames(std::back_insert_iterator &output_iterator); void SetDebugString(const char *s, void (*error_func)(const std::string &)); -const char *GetDebugString(); +std::string GetDebugString(); /* Shorter form for passing filename and linenumber */ #define FILE_LINE __FILE__, __LINE__ From 37a3fc4df3c1b89a19f6961e0a87822e775234f2 Mon Sep 17 00:00:00 2001 From: Rubidium Date: Fri, 28 Apr 2023 18:25:35 +0200 Subject: [PATCH 07/25] Codechange: replace strstr with more appropriate function --- src/openttd.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/openttd.cpp b/src/openttd.cpp index 19a2bc515b..f8c29ee392 100644 --- a/src/openttd.cpp +++ b/src/openttd.cpp @@ -383,9 +383,9 @@ void MakeNewgameSettingsLive() void OpenBrowser(const char *url) { /* Make sure we only accept urls that are sure to open a browser. */ - if (strstr(url, "http://") != url && strstr(url, "https://") != url) return; - - OSOpenBrowser(url); + if (StrStartsWith(url, "http://") || StrStartsWith(url, "https://")) { + OSOpenBrowser(url); + } } /** Callback structure of statements to be executed after the NewGRF scan. */ From 0ab7bc7a2bac5d0e52212e2572ad98694c22e5d7 Mon Sep 17 00:00:00 2001 From: Rubidium Date: Wed, 31 May 2023 21:38:18 +0200 Subject: [PATCH 08/25] Codechange: use fmt::format to create type prefixed driver names --- src/driver.cpp | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/driver.cpp b/src/driver.cpp index 10a20599b1..ea03604bbe 100644 --- a/src/driver.cpp +++ b/src/driver.cpp @@ -210,13 +210,11 @@ DriverFactoryBase::DriverFactoryBase(Driver::Type type, int priority, const char type(type), priority(priority), name(name), description(description) { /* Prefix the name with driver type to make it unique */ - char buf[32]; - strecpy(buf, GetDriverTypeName(type), lastof(buf)); - strecpy(buf + 5, name, lastof(buf)); + std::string typed_name = fmt::format("{}{}", GetDriverTypeName(type), name); Drivers &drivers = GetDrivers(); - assert(drivers.find(buf) == drivers.end()); - drivers.insert(Drivers::value_type(buf, this)); + assert(drivers.find(typed_name) == drivers.end()); + drivers.insert(Drivers::value_type(typed_name, this)); } /** @@ -225,11 +223,9 @@ DriverFactoryBase::DriverFactoryBase(Driver::Type type, int priority, const char DriverFactoryBase::~DriverFactoryBase() { /* Prefix the name with driver type to make it unique */ - char buf[32]; - strecpy(buf, GetDriverTypeName(type), lastof(buf)); - strecpy(buf + 5, this->name, lastof(buf)); + std::string typed_name = fmt::format("{}{}", GetDriverTypeName(type), name); - Drivers::iterator it = GetDrivers().find(buf); + Drivers::iterator it = GetDrivers().find(typed_name); assert(it != GetDrivers().end()); GetDrivers().erase(it); From 513ede7669b2639e5228d3d7a79e6e8fe36f0d2e Mon Sep 17 00:00:00 2001 From: Rubidium Date: Fri, 2 Jun 2023 18:13:51 +0200 Subject: [PATCH 09/25] Codechange: use C++ strings/paths to resolve links in tars --- src/fileio.cpp | 104 +++++++++++++++++++------------------------------ 1 file changed, 41 insertions(+), 63 deletions(-) diff --git a/src/fileio.cpp b/src/fileio.cpp index 13b0e26f5b..990510d545 100644 --- a/src/fileio.cpp +++ b/src/fileio.cpp @@ -26,6 +26,7 @@ #endif #include #include +#include #include "safeguards.h" @@ -392,15 +393,16 @@ static void TarAddLink(const std::string &srcParam, const std::string &destParam * Replace '/' by #PATHSEPCHAR, and force 'name' to lowercase. * @param name Filename to process. */ -static void SimplifyFileName(char *name) +static void SimplifyFileName(std::string &name) { - /* Force lowercase */ - strtolower(name); - - /* Tar-files always have '/' path-separator, but we want our PATHSEPCHAR */ + for (char &c : name) { + /* Force lowercase */ + c = std::tolower(c); #if (PATHSEPCHAR != '/') - for (char *n = name; *n != '\0'; n++) if (*n == '/') *n = PATHSEPCHAR; + /* Tar-files always have '/' path-separator, but we want our PATHSEPCHAR */ + if (c == '/') c = PATHSEPCHAR; #endif + } } /** @@ -456,6 +458,24 @@ bool TarScanner::AddFile(Subdirectory sd, const std::string &filename) return this->AddFile(filename, 0); } +/** + * Helper to extract a string for the tar header. We must assume that the tar + * header contains garbage and is malicious. So, we cannot rely on the string + * being properly terminated. + * As such, do not use strlen to determine the actual length (explicitly or + * implictly via the std::string constructor), but also do not create a string + * of the buffer length as that makes the string contain essentially garbage. + * @param buffer The buffer to read from. + * @param buffer_length The length of the buffer to read from. + * @return The string data. + */ +static std::string ExtractString(char *buffer, size_t buffer_length) +{ + size_t length = 0; + for (; length < buffer_length && buffer[length] != '\0'; length++) {} + return StrMakeValid(std::string(buffer, length)); +} + bool TarScanner::AddFile(const std::string &filename, size_t basepath_length, const std::string &tar_filename) { /* No tar within tar. */ @@ -467,7 +487,7 @@ bool TarScanner::AddFile(const std::string &filename, size_t basepath_length, co char mode[8]; char uid[8]; char gid[8]; - char size[12]; ///< Size of the file, in ASCII + char size[12]; ///< Size of the file, in ASCII octals char mtime[12]; char chksum[8]; char typeflag; @@ -499,10 +519,6 @@ bool TarScanner::AddFile(const std::string &filename, size_t basepath_length, co TarLinkList links; ///< Temporary list to collect links TarHeader th; - char buf[sizeof(th.name) + 1], *end; - char name[sizeof(th.prefix) + 1 + sizeof(th.name) + 1]; - char link[sizeof(th.linkname) + 1]; - char dest[sizeof(th.prefix) + 1 + sizeof(th.name) + 1 + 1 + sizeof(th.linkname) + 1]; size_t num = 0, pos = 0; /* Make a char of 512 empty bytes */ @@ -524,25 +540,25 @@ bool TarScanner::AddFile(const std::string &filename, size_t basepath_length, co return false; } - name[0] = '\0'; + std::string name; /* The prefix contains the directory-name */ if (th.prefix[0] != '\0') { - strecpy(name, th.prefix, lastof(name)); - strecat(name, PATHSEP, lastof(name)); + name = ExtractString(th.prefix, lengthof(th.prefix)); + name += PATHSEP; } /* Copy the name of the file in a safe way at the end of 'name' */ - strecat(name, th.name, lastof(name)); + name += ExtractString(th.name, lengthof(th.name)); - /* Calculate the size of the file.. for some strange reason this is stored as a string */ - strecpy(buf, th.size, lastof(buf)); - size_t skip = std::strtoul(buf, &end, 8); + /* The size of the file, for some strange reason, this is stored as a string in octals. */ + std::string size = ExtractString(th.size, lengthof(th.size)); + size_t skip = size.empty() ? 0 : std::stoul(size, nullptr, 8); switch (th.typeflag) { case '\0': case '0': { // regular file - if (strlen(name) == 0) break; + if (name.empty()) break; /* Store this entry in the list */ TarFileListEntry entry; @@ -562,9 +578,9 @@ bool TarScanner::AddFile(const std::string &filename, size_t basepath_length, co case '1': // hard links case '2': { // symbolic links /* Copy the destination of the link in a safe way at the end of 'linkname' */ - strecpy(link, th.linkname, lastof(link)); + std::string link = ExtractString(th.linkname, lengthof(th.linkname)); - if (strlen(name) == 0 || strlen(link) == 0) break; + if (name.empty() || link.empty()) break; /* Convert to lowercase and our PATHSEPCHAR */ SimplifyFileName(name); @@ -578,48 +594,10 @@ bool TarScanner::AddFile(const std::string &filename, size_t basepath_length, co /* Process relative path. * Note: The destination of links must not contain any directory-links. */ - strecpy(dest, name, lastof(dest)); - char *destpos = strrchr(dest, PATHSEPCHAR); - if (destpos == nullptr) destpos = dest; - *destpos = '\0'; - - char *pos = link; - while (*pos != '\0') { - char *next = strchr(pos, PATHSEPCHAR); - if (next == nullptr) { - next = pos + strlen(pos); - } else { - /* Terminate the substring up to the path separator character. */ - *next++= '\0'; - } - - if (strcmp(pos, ".") == 0) { - /* Skip '.' (current dir) */ - } else if (strcmp(pos, "..") == 0) { - /* level up */ - if (dest[0] == '\0') { - Debug(misc, 5, "Ignoring link pointing outside of data directory: {} -> {}", name, link); - break; - } - - /* Truncate 'dest' after last PATHSEPCHAR. - * This assumes that the truncated part is a real directory and not a link. */ - destpos = strrchr(dest, PATHSEPCHAR); - if (destpos == nullptr) destpos = dest; - *destpos = '\0'; - } else { - /* Append at end of 'dest' */ - if (destpos != dest) destpos = strecpy(destpos, PATHSEP, lastof(dest)); - destpos = strecpy(destpos, pos, lastof(dest)); - } - - if (destpos >= lastof(dest)) { - Debug(misc, 0, "The length of a link in tar-file '{}' is too large (malformed?)", filename); - fclose(f); - return false; - } - - pos = next; + std::string dest = (std::filesystem::path(name).remove_filename() /= link).lexically_normal().string(); + if (dest[0] == PATHSEPCHAR || StrStartsWith(dest, "..")) { + Debug(misc, 5, "Ignoring link pointing outside of data directory: {} -> {}", name, link); + break; } /* Store links in temporary list */ From a5a3a07005ed430426efe62bb6dbb61c691266f5 Mon Sep 17 00:00:00 2001 From: PeterN Date: Sat, 3 Jun 2023 12:07:59 +0100 Subject: [PATCH 10/25] Fix: GRF Parameters not displayed due to scope issue. (#10911) Move params so it is still in scope when the text is actually drawn. --- src/newgrf_gui.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/newgrf_gui.cpp b/src/newgrf_gui.cpp index cec827f300..d9f0d63787 100644 --- a/src/newgrf_gui.cpp +++ b/src/newgrf_gui.cpp @@ -110,8 +110,9 @@ static void ShowNewGRFInfo(const GRFConfig *c, const Rect &r, bool show_params) /* Show GRF parameter list */ if (show_params) { + std::string params; if (c->num_params > 0) { - std::string params = GRFBuildParamList(c); + params = GRFBuildParamList(c); SetDParam(0, STR_JUST_RAW_STRING); SetDParamStr(1, params); } else { From dec7ff6b0ccd1fdac9d96128422d5179c37b7162 Mon Sep 17 00:00:00 2001 From: Peter Nelson Date: Sat, 3 Jun 2023 13:19:59 +0100 Subject: [PATCH 11/25] Fix: Make dropdowns self-close when losing focus. --- src/widgets/dropdown.cpp | 5 +++++ src/window.cpp | 2 -- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/widgets/dropdown.cpp b/src/widgets/dropdown.cpp index a9eb238453..7fe72bc904 100644 --- a/src/widgets/dropdown.cpp +++ b/src/widgets/dropdown.cpp @@ -199,6 +199,11 @@ struct DropdownWindow : Window { } } + void OnFocusLost() override + { + this->Close(); + } + Point OnInitialPosition(int16 sm_width, int16 sm_height, int window_number) override { return this->position; diff --git a/src/window.cpp b/src/window.cpp index 5c056146be..997491d8aa 100644 --- a/src/window.cpp +++ b/src/window.cpp @@ -2331,7 +2331,6 @@ static void StartWindowDrag(Window *w) _drag_delta.y = w->top - _cursor.pos.y; BringWindowToFront(w); - CloseWindowById(WC_DROPDOWN_MENU, 0); } /** @@ -2349,7 +2348,6 @@ static void StartWindowSizing(Window *w, bool to_left) _drag_delta.y = _cursor.pos.y; BringWindowToFront(w); - CloseWindowById(WC_DROPDOWN_MENU, 0); } /** From 25116499380050f854bb7530f1e2bca4c4003c9d Mon Sep 17 00:00:00 2001 From: Peter Nelson Date: Fri, 2 Jun 2023 14:27:06 +0100 Subject: [PATCH 12/25] Codechange: Use window parent association for dropdowns. This replaces the separate window class and number properties, and allows the window system to close dropdowns automatically. --- src/game/game_gui.cpp | 6 +++--- src/group_gui.cpp | 4 ++-- src/newgrf_gui.cpp | 4 ++-- src/order_gui.cpp | 3 --- src/script/script_gui.cpp | 6 +++--- src/settings_gui.cpp | 2 +- src/vehicle_gui.cpp | 2 +- src/widgets/dropdown.cpp | 34 ++++++++++------------------------ 8 files changed, 22 insertions(+), 39 deletions(-) diff --git a/src/game/game_gui.cpp b/src/game/game_gui.cpp index 8b0c1a3333..02aa36af00 100644 --- a/src/game/game_gui.cpp +++ b/src/game/game_gui.cpp @@ -276,7 +276,7 @@ struct GSConfigWindow : public Window { int num = it - this->visible_settings.begin(); if (this->clicked_row != num) { this->CloseChildWindows(WC_QUERY_STRING); - HideDropDownMenu(this); + this->CloseChildWindows(WC_DROPDOWN_MENU); this->clicked_row = num; this->clicked_dropdown = false; } @@ -292,7 +292,7 @@ struct GSConfigWindow : public Window { if (!bool_item && IsInsideMM(x, 0, SETTING_BUTTON_WIDTH) && config_item.complete_labels) { if (this->clicked_dropdown) { /* unclick the dropdown */ - HideDropDownMenu(this); + this->CloseChildWindows(WC_DROPDOWN_MENU); this->clicked_dropdown = false; this->closing_dropdown = false; } else { @@ -408,7 +408,7 @@ struct GSConfigWindow : public Window { this->SetWidgetDisabledState(WID_GSC_TEXTFILE + tft, !GameConfig::GetConfig()->GetTextfile(tft, (CompanyID)OWNER_DEITY).has_value()); } this->RebuildVisibleSettings(); - HideDropDownMenu(this); + this->CloseChildWindows(WC_DROPDOWN_MENU); this->CloseChildWindows(WC_QUERY_STRING); } private: diff --git a/src/group_gui.cpp b/src/group_gui.cpp index 38c719e7e1..4beee5a318 100644 --- a/src/group_gui.cpp +++ b/src/group_gui.cpp @@ -461,7 +461,7 @@ public: if (!(IsAllGroupID(this->vli.index) || IsDefaultGroupID(this->vli.index) || Group::IsValidID(this->vli.index))) { this->vli.index = ALL_GROUP; - HideDropDownMenu(this); + this->CloseChildWindows(WC_DROPDOWN_MENU); } this->SetDirty(); } @@ -512,7 +512,7 @@ public: /* The drop down menu is out, *but* it may not be used, retract it. */ if (this->vehicles.size() == 0 && this->IsWidgetLowered(WID_GL_MANAGE_VEHICLES_DROPDOWN)) { this->RaiseWidget(WID_GL_MANAGE_VEHICLES_DROPDOWN); - HideDropDownMenu(this); + this->CloseChildWindows(WC_DROPDOWN_MENU); } /* Disable all lists management button when the list is empty */ diff --git a/src/newgrf_gui.cpp b/src/newgrf_gui.cpp index d9f0d63787..a2b8af54ee 100644 --- a/src/newgrf_gui.cpp +++ b/src/newgrf_gui.cpp @@ -363,7 +363,7 @@ struct NewGRFParametersWindow : public Window { uint num = it - this->grf_config->param_info.begin(); if (this->clicked_row != num) { this->CloseChildWindows(WC_QUERY_STRING); - HideDropDownMenu(this); + this->CloseChildWindows(WC_DROPDOWN_MENU); this->clicked_row = num; this->clicked_dropdown = false; } @@ -379,7 +379,7 @@ struct NewGRFParametersWindow : public Window { if (par_info.type != PTYPE_BOOL && IsInsideMM(x, 0, SETTING_BUTTON_WIDTH) && par_info.complete_labels) { if (this->clicked_dropdown) { /* unclick the dropdown */ - HideDropDownMenu(this); + this->CloseChildWindows(WC_DROPDOWN_MENU); this->clicked_dropdown = false; this->closing_dropdown = false; } else { diff --git a/src/order_gui.cpp b/src/order_gui.cpp index 832999c9b2..a99b520fd7 100644 --- a/src/order_gui.cpp +++ b/src/order_gui.cpp @@ -854,7 +854,6 @@ public: if (this->selected_order == -1) break; this->CloseChildWindows(); - HideDropDownMenu(this); this->selected_order = -1; break; @@ -886,7 +885,6 @@ public: if (to == INVALID_VEH_ORDER_ID) { /* Deleting selected order */ this->CloseChildWindows(); - HideDropDownMenu(this); this->selected_order = -1; break; } @@ -1186,7 +1184,6 @@ public: /* This order won't be selected any more, close all child windows and dropdowns */ this->CloseChildWindows(); - HideDropDownMenu(this); if (sel == INVALID_VEH_ORDER_ID || this->vehicle->owner != _local_company) { /* Deselect clicked order */ diff --git a/src/script/script_gui.cpp b/src/script/script_gui.cpp index 09c7a0098b..a179ddcef9 100644 --- a/src/script/script_gui.cpp +++ b/src/script/script_gui.cpp @@ -434,7 +434,7 @@ struct ScriptSettingsWindow : public Window { int num = it - this->visible_settings.begin(); if (this->clicked_row != num) { this->CloseChildWindows(WC_QUERY_STRING); - HideDropDownMenu(this); + this->CloseChildWindows(WC_DROPDOWN_MENU); this->clicked_row = num; this->clicked_dropdown = false; } @@ -450,7 +450,7 @@ struct ScriptSettingsWindow : public Window { if (!bool_item && IsInsideMM(x, 0, SETTING_BUTTON_WIDTH) && config_item.complete_labels) { if (this->clicked_dropdown) { /* unclick the dropdown */ - HideDropDownMenu(this); + this->CloseChildWindows(WC_DROPDOWN_MENU); this->clicked_dropdown = false; this->closing_dropdown = false; } else { @@ -560,7 +560,7 @@ struct ScriptSettingsWindow : public Window { void OnInvalidateData(int data = 0, bool gui_scope = true) override { this->RebuildVisibleSettings(); - HideDropDownMenu(this); + this->CloseChildWindows(WC_DROPDOWN_MENU); this->CloseChildWindows(WC_QUERY_STRING); } diff --git a/src/settings_gui.cpp b/src/settings_gui.cpp index 076b2d088e..d055557b27 100644 --- a/src/settings_gui.cpp +++ b/src/settings_gui.cpp @@ -2349,7 +2349,7 @@ struct GameSettingsWindow : Window { if (this->valuedropdown_entry == pe) { /* unclick the dropdown */ - HideDropDownMenu(this); + this->CloseChildWindows(WC_DROPDOWN_MENU); this->closing_dropdown = false; this->valuedropdown_entry->SetButtons(0); this->valuedropdown_entry = nullptr; diff --git a/src/vehicle_gui.cpp b/src/vehicle_gui.cpp index 7e16099f1d..400306d9d4 100644 --- a/src/vehicle_gui.cpp +++ b/src/vehicle_gui.cpp @@ -1966,7 +1966,7 @@ public: this->SortVehicleList(); if (this->vehicles.size() == 0 && this->IsWidgetLowered(WID_VL_MANAGE_VEHICLES_DROPDOWN)) { - HideDropDownMenu(this); + this->CloseChildWindows(WC_DROPDOWN_MENU); } /* Hide the widgets that we will not use in this window diff --git a/src/widgets/dropdown.cpp b/src/widgets/dropdown.cpp index 7fe72bc904..6d1ed9c27d 100644 --- a/src/widgets/dropdown.cpp +++ b/src/widgets/dropdown.cpp @@ -117,8 +117,6 @@ static WindowDesc _dropdown_desc( /** Drop-down menu window */ struct DropdownWindow : Window { - WindowClass parent_wnd_class; ///< Parent window class. - WindowNumber parent_wnd_num; ///< Parent window number. int parent_button; ///< Parent widget number where the window is dropped from. const DropDownList list; ///< List with dropdown menu items. int selected_index; ///< Index of the selected item in the list. @@ -175,8 +173,7 @@ struct DropdownWindow : Window { this->vscroll->SetCapacity(size.height * this->list.size() / list_height); this->vscroll->SetCount(this->list.size()); - this->parent_wnd_class = parent->window_class; - this->parent_wnd_num = parent->window_number; + this->parent = parent; this->parent_button = button; this->selected_index = selected; this->click_delay = 0; @@ -190,13 +187,10 @@ struct DropdownWindow : Window { * Also mark it dirty in case the callback deals with the screen. (e.g. screenshots). */ this->Window::Close(); - Window *w2 = FindWindowById(this->parent_wnd_class, this->parent_wnd_num); - if (w2 != nullptr) { - Point pt = _cursor.pos; - pt.x -= w2->left; - pt.y -= w2->top; - w2->OnDropdownClose(pt, this->parent_button, this->selected_index, this->instant_close); - } + Point pt = _cursor.pos; + pt.x -= this->parent->left; + pt.y -= this->parent->top; + this->parent->OnDropdownClose(pt, this->parent_button, this->selected_index, this->instant_close); } void OnFocusLost() override @@ -292,17 +286,11 @@ struct DropdownWindow : Window { void OnMouseLoop() override { - Window *w2 = FindWindowById(this->parent_wnd_class, this->parent_wnd_num); - if (w2 == nullptr) { - this->Close(); - return; - } - if (this->click_delay != 0 && --this->click_delay == 0) { /* Close the dropdown, so it doesn't affect new window placement. * Also mark it dirty in case the callback deals with the screen. (e.g. screenshots). */ this->Close(); - w2->OnDropdownSelect(this->parent_button, this->selected_index); + this->parent->OnDropdownSelect(this->parent_button, this->selected_index); return; } @@ -495,16 +483,14 @@ void ShowDropDownMenu(Window *w, const StringID *strings, int selected, int butt int HideDropDownMenu(Window *pw) { for (Window *w : Window::Iterate()) { + if (w->parent != pw) continue; if (w->window_class != WC_DROPDOWN_MENU) continue; DropdownWindow *dw = dynamic_cast(w); assert(dw != nullptr); - if (pw->window_class == dw->parent_wnd_class && - pw->window_number == dw->parent_wnd_num) { - int parent_button = dw->parent_button; - dw->Close(); - return parent_button; - } + int parent_button = dw->parent_button; + dw->Close(); + return parent_button; } return -1; From 2e62682f7328670f29cfcd78cec18490bec24d6f Mon Sep 17 00:00:00 2001 From: Peter Nelson Date: Sat, 3 Jun 2023 13:21:08 +0100 Subject: [PATCH 13/25] Codechange: Close dropdowns by class instead of id. --- src/widgets/dropdown.cpp | 2 +- src/window.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/widgets/dropdown.cpp b/src/widgets/dropdown.cpp index 6d1ed9c27d..f6e50e5ad2 100644 --- a/src/widgets/dropdown.cpp +++ b/src/widgets/dropdown.cpp @@ -339,7 +339,7 @@ struct DropdownWindow : Window { */ void ShowDropDownListAt(Window *w, DropDownList &&list, int selected, int button, Rect wi_rect, Colours wi_colour, bool instant_close) { - CloseWindowById(WC_DROPDOWN_MENU, 0); + CloseWindowByClass(WC_DROPDOWN_MENU); /* The preferred position is just below the dropdown calling widget */ int top = w->top + wi_rect.bottom + 1; diff --git a/src/window.cpp b/src/window.cpp index 997491d8aa..927861d742 100644 --- a/src/window.cpp +++ b/src/window.cpp @@ -3483,7 +3483,7 @@ void ChangeVehicleViewports(VehicleID from_index, VehicleID to_index) */ void RelocateAllWindows(int neww, int newh) { - CloseWindowById(WC_DROPDOWN_MENU, 0); + CloseWindowByClass(WC_DROPDOWN_MENU); for (Window *w : Window::Iterate()) { int left, top; From 35f7f7e8dcaa0edcbe07583c41d801af19249e6a Mon Sep 17 00:00:00 2001 From: Rubidium Date: Wed, 31 May 2023 21:34:10 +0200 Subject: [PATCH 14/25] Codechange: use std::string for executing the console aliases --- src/console.cpp | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/src/console.cpp b/src/console.cpp index 56b0014b50..5c5ae2fb65 100644 --- a/src/console.cpp +++ b/src/console.cpp @@ -213,8 +213,7 @@ static std::string RemoveUnderscores(std::string name) */ static void IConsoleAliasExec(const IConsoleAlias *alias, byte tokencount, char *tokens[ICON_TOKEN_COUNT], const uint recurse_count) { - char alias_buffer[ICON_MAX_STREAMSIZE] = { '\0' }; - char *alias_stream = alias_buffer; + std::string alias_buffer; Debug(console, 6, "Requested command is an alias; parsing..."); @@ -226,14 +225,13 @@ static void IConsoleAliasExec(const IConsoleAlias *alias, byte tokencount, char for (const char *cmdptr = alias->cmdline.c_str(); *cmdptr != '\0'; cmdptr++) { switch (*cmdptr) { case '\'': // ' will double for "" - alias_stream = strecpy(alias_stream, "\"", lastof(alias_buffer)); + alias_buffer += '\"'; break; case ';': // Cmd separator; execute previous and start new command - IConsoleCmdExec(alias_buffer, recurse_count); + IConsoleCmdExec(alias_buffer.c_str(), recurse_count); - alias_stream = alias_buffer; - *alias_stream = '\0'; // Make sure the new command is terminated. + alias_buffer.clear(); cmdptr++; break; @@ -243,21 +241,21 @@ static void IConsoleAliasExec(const IConsoleAlias *alias, byte tokencount, char switch (*cmdptr) { case '+': { // All parameters separated: "[param 1]" "[param 2]" for (uint i = 0; i != tokencount; i++) { - if (i != 0) alias_stream = strecpy(alias_stream, " ", lastof(alias_buffer)); - alias_stream = strecpy(alias_stream, "\"", lastof(alias_buffer)); - alias_stream = strecpy(alias_stream, tokens[i], lastof(alias_buffer)); - alias_stream = strecpy(alias_stream, "\"", lastof(alias_buffer)); + if (i != 0) alias_buffer += ' '; + alias_buffer += '\"'; + alias_buffer += tokens[i]; + alias_buffer += '\"'; } break; } case '!': { // Merge the parameters to one: "[param 1] [param 2] [param 3...]" - alias_stream = strecpy(alias_stream, "\"", lastof(alias_buffer)); + alias_buffer += '\"'; for (uint i = 0; i != tokencount; i++) { - if (i != 0) alias_stream = strecpy(alias_stream, " ", lastof(alias_buffer)); - alias_stream = strecpy(alias_stream, tokens[i], lastof(alias_buffer)); + if (i != 0) alias_buffer += " "; + alias_buffer += tokens[i]; } - alias_stream = strecpy(alias_stream, "\"", lastof(alias_buffer)); + alias_buffer += '\"'; break; } @@ -270,27 +268,26 @@ static void IConsoleAliasExec(const IConsoleAlias *alias, byte tokencount, char return; } - alias_stream = strecpy(alias_stream, "\"", lastof(alias_buffer)); - alias_stream = strecpy(alias_stream, tokens[param], lastof(alias_buffer)); - alias_stream = strecpy(alias_stream, "\"", lastof(alias_buffer)); + alias_buffer += '\"'; + alias_buffer += tokens[param]; + alias_buffer += '\"'; break; } } break; default: - *alias_stream++ = *cmdptr; - *alias_stream = '\0'; + alias_buffer += *cmdptr; break; } - if (alias_stream >= lastof(alias_buffer) - 1) { + if (alias_buffer.size() >= ICON_MAX_STREAMSIZE - 1) { IConsolePrint(CC_ERROR, "Requested alias execution would overflow execution buffer."); return; } } - IConsoleCmdExec(alias_buffer, recurse_count); + IConsoleCmdExec(alias_buffer.c_str(), recurse_count); } /** From d68b5c9162f469edbf5aaf0481de674fbcbcff3d Mon Sep 17 00:00:00 2001 From: Rubidium Date: Wed, 31 May 2023 20:26:49 +0200 Subject: [PATCH 15/25] Codechange: replace buffer + strecpy with std::string for getting clipboard contents --- src/os/macosx/macos.mm | 14 +++++--------- src/os/os2/os2.cpp | 16 +++++++--------- src/os/unix/unix.cpp | 12 +++++------- src/os/windows/win32.cpp | 25 +++++++++---------------- src/textbuf.cpp | 13 +++++-------- 5 files changed, 31 insertions(+), 49 deletions(-) diff --git a/src/os/macosx/macos.mm b/src/os/macosx/macos.mm index 773c77e32f..9c6a7e370b 100644 --- a/src/os/macosx/macos.mm +++ b/src/os/macosx/macos.mm @@ -183,25 +183,21 @@ const char *GetCurrentLocale(const char *) /** * Return the contents of the clipboard (COCOA). * - * @param buffer Clipboard content. - * @param last The pointer to the last element of the destination buffer - * @return Whether clipboard is empty or not. + * @return The (optional) clipboard contents. */ -bool GetClipboardContents(char *buffer, const char *last) +std::optional GetClipboardContents() { NSPasteboard *pb = [ NSPasteboard generalPasteboard ]; NSArray *types = [ NSArray arrayWithObject:NSPasteboardTypeString ]; NSString *bestType = [ pb availableTypeFromArray:types ]; /* Clipboard has no text data available. */ - if (bestType == nil) return false; + if (bestType == nil) return std::nullopt; NSString *string = [ pb stringForType:NSPasteboardTypeString ]; - if (string == nil || [ string length ] == 0) return false; + if (string == nil || [ string length ] == 0) return std::nullopt; - strecpy(buffer, [ string UTF8String ], last); - - return true; + return [ string UTF8String ]; } /** Set the application's bundle directory. diff --git a/src/os/os2/os2.cpp b/src/os/os2/os2.cpp index 3be323e407..ae49d8633a 100644 --- a/src/os/os2/os2.cpp +++ b/src/os/os2/os2.cpp @@ -154,27 +154,25 @@ void ShowOSErrorBox(const char *buf, bool system) WinTerminate(hab); } -bool GetClipboardContents(char *buffer, const char *last) +std::optional GetClipboardContents() { /* XXX -- Currently no clipboard support implemented with GCC */ #ifndef __INNOTEK_LIBC__ HAB hab = 0; - if (WinOpenClipbrd(hab)) - { - const char *text = (const char*)WinQueryClipbrdData(hab, CF_TEXT); + if (WinOpenClipbrd(hab)) { + const char *text = (const char *)WinQueryClipbrdData(hab, CF_TEXT); - if (text != nullptr) - { - strecpy(buffer, text, last); + if (text != nullptr) { + std::string result = text; WinCloseClipbrd(hab); - return true; + return result; } WinCloseClipbrd(hab); } #endif - return false; + return std::nullopt; } diff --git a/src/os/unix/unix.cpp b/src/os/unix/unix.cpp index 2dfce9f21b..9e99cbee57 100644 --- a/src/os/unix/unix.cpp +++ b/src/os/unix/unix.cpp @@ -217,22 +217,20 @@ void ShowOSErrorBox(const char *buf, bool system) #endif #ifndef WITH_COCOA -bool GetClipboardContents(char *buffer, const char *last) +std::optional GetClipboardContents() { #ifdef WITH_SDL2 - if (SDL_HasClipboardText() == SDL_FALSE) { - return false; - } + if (SDL_HasClipboardText() == SDL_FALSE) return std::nullopt; char *clip = SDL_GetClipboardText(); if (clip != nullptr) { - strecpy(buffer, clip, last); + std::string result = clip; SDL_free(clip); - return true; + return result; } #endif - return false; + return std::nullopt; } #endif diff --git a/src/os/windows/win32.cpp b/src/os/windows/win32.cpp index a18f693d53..73cf4dc3f9 100644 --- a/src/os/windows/win32.cpp +++ b/src/os/windows/win32.cpp @@ -433,26 +433,19 @@ void DetermineBasePaths(const char *exe) } -bool GetClipboardContents(char *buffer, const char *last) +std::optional GetClipboardContents() { - HGLOBAL cbuf; - const char *ptr; + if (!IsClipboardFormatAvailable(CF_UNICODETEXT)) return std::nullopt; - if (IsClipboardFormatAvailable(CF_UNICODETEXT)) { - OpenClipboard(nullptr); - cbuf = GetClipboardData(CF_UNICODETEXT); + OpenClipboard(nullptr); + HGLOBAL cbuf = GetClipboardData(CF_UNICODETEXT); - ptr = (const char*)GlobalLock(cbuf); - int out_len = WideCharToMultiByte(CP_UTF8, 0, (LPCWSTR)ptr, -1, buffer, (last - buffer) + 1, nullptr, nullptr); - GlobalUnlock(cbuf); - CloseClipboard(); + std::string result = FS2OTTD(static_cast(GlobalLock(cbuf))); + GlobalUnlock(cbuf); + CloseClipboard(); - if (out_len == 0) return false; - } else { - return false; - } - - return true; + if (result.empty()) return std::nullopt; + return result; } diff --git a/src/textbuf.cpp b/src/textbuf.cpp index 807b004804..151c753d92 100644 --- a/src/textbuf.cpp +++ b/src/textbuf.cpp @@ -23,11 +23,9 @@ * Try to retrieve the current clipboard contents. * * @note OS-specific function. - * @param buffer Clipboard content. - * @param last The pointer to the last element of the destination buffer - * @return True if some text could be retrieved. + * @return The (optional) clipboard contents. */ -bool GetClipboardContents(char *buffer, const char *last); +std::optional GetClipboardContents(); int _caret_timer; @@ -223,11 +221,10 @@ bool Textbuf::InsertString(const char *str, bool marked, const char *caret, cons */ bool Textbuf::InsertClipboard() { - char utf8_buf[512]; + auto contents = GetClipboardContents(); + if (!contents.has_value()) return false; - if (!GetClipboardContents(utf8_buf, lastof(utf8_buf))) return false; - - return this->InsertString(utf8_buf, false); + return this->InsertString(contents.value().c_str(), false); } /** From 7d6aff3a344d05ea3c7d05d619f1207ab56861e5 Mon Sep 17 00:00:00 2001 From: translators Date: Sat, 3 Jun 2023 18:42:31 +0000 Subject: [PATCH 16/25] Update: Translations from eints french: 16 changes by ottdfevr --- src/lang/french.txt | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/lang/french.txt b/src/lang/french.txt index 5b1c028314..c33f53a1dd 100644 --- a/src/lang/french.txt +++ b/src/lang/french.txt @@ -1042,6 +1042,13 @@ STR_GAME_OPTIONS_GUI_SCALE_3X :3x STR_GAME_OPTIONS_GUI_SCALE_4X :4x STR_GAME_OPTIONS_GUI_SCALE_5X :x5 +STR_GAME_OPTIONS_PARTICIPATE_SURVEY_FRAME :{BLACK}Données d'analyse +STR_GAME_OPTIONS_PARTICIPATE_SURVEY :{BLACK}Participer à l'envoi de données d'analyse +STR_GAME_OPTIONS_PARTICIPATE_SURVEY_TOOLTIP :{BLACK}Si activé, OpenTTD transmettra les données d'analyse en quittant une partie +STR_GAME_OPTIONS_PARTICIPATE_SURVEY_LINK :{BLACK}À propos des données et de la confidentialité +STR_GAME_OPTIONS_PARTICIPATE_SURVEY_LINK_TOOLTIP :{BLACK}Ouvre une fenêtre à propos de la confidentialité des données d'analyse envoyées +STR_GAME_OPTIONS_PARTICIPATE_SURVEY_PREVIEW :{BLACK}Prévisualisation des données d'analyse +STR_GAME_OPTIONS_PARTICIPATE_SURVEY_PREVIEW_TOOLTIP :{BLACK}Voir les données d'analyse de la partie en cours STR_GAME_OPTIONS_GRAPHICS :{BLACK} Graphiques @@ -2404,6 +2411,12 @@ STR_NETWORK_ASK_RELAY_NO :{BLACK}Non STR_NETWORK_ASK_RELAY_YES_ONCE :{BLACK}Oui, cette fois uniquement STR_NETWORK_ASK_RELAY_YES_ALWAYS :{BLACK}Oui, ne plus me demander +STR_NETWORK_ASK_SURVEY_CAPTION :Participer à l'envoi automatique de données d'analyse ? +STR_NETWORK_ASK_SURVEY_TEXT :Souhaitez-vous participer à l'envoi automatique de données d'analyse ?{}OpenTTD transmettra les données d'analyse lorsque vous quitterez une partie.{}Vous pouvez changer ceci à tout moment dans les options du jeu. +STR_NETWORK_ASK_SURVEY_PREVIEW :Prévisualiser les données d'analyse +STR_NETWORK_ASK_SURVEY_LINK :Données d'analyse et confidentialité +STR_NETWORK_ASK_SURVEY_NO :Non +STR_NETWORK_ASK_SURVEY_YES :Oui STR_NETWORK_SPECTATORS :Spectateurs @@ -2694,7 +2707,8 @@ STR_STATION_BUILD_DRAG_DROP_TOOLTIP :{BLACK}Construi STR_STATION_BUILD_STATION_CLASS_TOOLTIP :{BLACK}Choisir le type de station à afficher STR_STATION_BUILD_STATION_TYPE_TOOLTIP :{BLACK}Choisir le type de station à construire -STR_STATION_CLASS_DFLT :Station par défaut +STR_STATION_CLASS_DFLT :Par défaut +STR_STATION_CLASS_DFLT_STATION :Station par défaut STR_STATION_CLASS_WAYP :Points de contrôle # Signal window @@ -4658,6 +4672,7 @@ STR_TEXTFILE_VIEW_LICENCE :{BLACK}Licence STR_TEXTFILE_README_CAPTION :{WHITE}Lisez-moi du module {STRING} {STRING} STR_TEXTFILE_CHANGELOG_CAPTION :{WHITE}Journal des modifications pour le module {STRING} {STRING} STR_TEXTFILE_LICENCE_CAPTION :{WHITE}Licence du module {STRING} {STRING} +STR_TEXTFILE_SURVEY_RESULT_CAPTION :{WHITE}Prévisualisation des données d'analyse # Vehicle loading indicators From 21adfa7567c895e477d1a84c4c99de58981b72b5 Mon Sep 17 00:00:00 2001 From: Patric Stout Date: Sat, 3 Jun 2023 21:07:56 +0200 Subject: [PATCH 17/25] Fix: track "memory installed" for surveys less precisely (#10910) It turns out, for Windows and Linux having the exact memory allows for easy tracing of an individual. That is exactly against the idea of the survey. And honestly, we don't need this precision. --- src/network/network_survey.cpp | 29 +++++++++++++++++++++++++++-- src/os/macosx/survey_osx.cpp | 6 +++++- src/os/unix/survey_unix.cpp | 6 +++++- src/os/windows/survey_win.cpp | 6 +++++- 4 files changed, 42 insertions(+), 5 deletions(-) diff --git a/src/network/network_survey.cpp b/src/network/network_survey.cpp index d1c229c7b7..b3f64662a6 100644 --- a/src/network/network_survey.cpp +++ b/src/network/network_survey.cpp @@ -298,6 +298,33 @@ static void SurveyGameScript(nlohmann::json &survey) survey = fmt::format("{}.{}", Game::GetInfo()->GetName(), Game::GetInfo()->GetVersion()); } +/** + * Change the bytes of memory into a textual version rounded up to the biggest unit. + * + * For example, 16751108096 would become 16 GiB. + * + * @param memory The bytes of memory. + * @return std::string A textual representation. + */ +std::string SurveyMemoryToText(uint64_t memory) +{ + memory = memory / 1024; // KiB + memory = CeilDiv(memory, 1024); // MiB + + /* Anything above 512 MiB we represent in GiB. */ + if (memory > 512) { + return fmt::format("{} GiB", CeilDiv(memory, 1024)); + } + + /* Anything above 64 MiB we represent in a multiplier of 128 MiB. */ + if (memory > 64) { + return fmt::format("{} MiB", Ceil(memory, 128)); + } + + /* Anything else in a multiplier of 4 MiB. */ + return fmt::format("{} MiB", Ceil(memory, 4)); +} + #endif /* WITH_NLOHMANN_JSON */ /** @@ -328,8 +355,6 @@ std::string NetworkSurveyHandler::CreatePayload(Reason reason, bool for_preview) { auto &info = survey["info"]; SurveyOS(info["os"]); - info["os"]["hardware_concurrency"] = std::thread::hardware_concurrency(); - SurveyOpenTTD(info["openttd"]); SurveyConfiguration(info["configuration"]); SurveyFont(info["font"]); diff --git a/src/os/macosx/survey_osx.cpp b/src/os/macosx/survey_osx.cpp index 88edebd9f6..14260fbd68 100644 --- a/src/os/macosx/survey_osx.cpp +++ b/src/os/macosx/survey_osx.cpp @@ -16,9 +16,12 @@ #include #include +#include #include "../../safeguards.h" +extern std::string SurveyMemoryToText(uint64_t memory); + void SurveyOS(nlohmann::json &json) { int ver_maj, ver_min, ver_bug; @@ -32,7 +35,8 @@ void SurveyOS(nlohmann::json &json) json["min_ver"] = MAC_OS_X_VERSION_MIN_REQUIRED; json["max_ver"] = MAC_OS_X_VERSION_MAX_ALLOWED; - json["memory"] = MacOSGetPhysicalMemory(); + json["memory"] = SurveyMemoryToText(MacOSGetPhysicalMemory()); + json["hardware_concurrency"] = std::thread::hardware_concurrency(); } #endif /* WITH_NLOHMANN_JSON */ diff --git a/src/os/unix/survey_unix.cpp b/src/os/unix/survey_unix.cpp index d86b58e98c..d831a78390 100644 --- a/src/os/unix/survey_unix.cpp +++ b/src/os/unix/survey_unix.cpp @@ -13,10 +13,13 @@ #include #include +#include #include #include "../../safeguards.h" +extern std::string SurveyMemoryToText(uint64_t memory); + void SurveyOS(nlohmann::json &json) { struct utsname name; @@ -32,7 +35,8 @@ void SurveyOS(nlohmann::json &json) long pages = sysconf(_SC_PHYS_PAGES); long page_size = sysconf(_SC_PAGE_SIZE); - json["memory"] = pages * page_size; + json["memory"] = SurveyMemoryToText(pages * page_size); + json["hardware_concurrency"] = std::thread::hardware_concurrency(); } #endif /* WITH_NLOHMANN_JSON */ diff --git a/src/os/windows/survey_win.cpp b/src/os/windows/survey_win.cpp index 407ddb31d4..1af48a39e7 100644 --- a/src/os/windows/survey_win.cpp +++ b/src/os/windows/survey_win.cpp @@ -14,10 +14,13 @@ #include "../../3rdparty/fmt/format.h" #include +#include #include #include "../../safeguards.h" +extern std::string SurveyMemoryToText(uint64_t memory); + void SurveyOS(nlohmann::json &json) { _OSVERSIONINFOA os; @@ -31,7 +34,8 @@ void SurveyOS(nlohmann::json &json) status.dwLength = sizeof(status); GlobalMemoryStatusEx(&status); - json["memory"] = status.ullTotalPhys; + json["memory"] = SurveyMemoryToText(status.ullTotalPhys); + json["hardware_concurrency"] = std::thread::hardware_concurrency(); } #endif /* WITH_NLOHMANN_JSON */ From cd751a598a169efefc702e11eca21b1f398adb00 Mon Sep 17 00:00:00 2001 From: Patric Stout Date: Sat, 3 Jun 2023 21:09:02 +0200 Subject: [PATCH 18/25] Fix: Wayland crash on startup due to Pango also using FontConfig (#10916) Basically, we haven't been a good neighbour. Turns out you shouldn't actually call FcFini when you are done, as some library might still want to use FontConfig. And they use a shared instance for their administration. The idea is that you call FcInit once, and use FcConfigReference after that to get an instance, you can release. This entry is ref-counted, and things happen automatically based on that. At least, I think. --- src/os/unix/font_unix.cpp | 109 ++++++++++++++++++++------------------ 1 file changed, 58 insertions(+), 51 deletions(-) diff --git a/src/os/unix/font_unix.cpp b/src/os/unix/font_unix.cpp index 976d6774b6..bfc759c0dd 100644 --- a/src/os/unix/font_unix.cpp +++ b/src/os/unix/font_unix.cpp @@ -31,65 +31,69 @@ FT_Error GetFontByFaceName(const char *font_name, FT_Face *face) if (!FcInit()) { ShowInfo("Unable to load font configuration"); - } else { - FcPattern *match; - FcPattern *pat; - FcFontSet *fs; - FcResult result; - char *font_style; - char *font_family; + return err; + } - /* Split & strip the font's style */ - font_family = stredup(font_name); - font_style = strchr(font_family, ','); - if (font_style != nullptr) { - font_style[0] = '\0'; - font_style++; - while (*font_style == ' ' || *font_style == '\t') font_style++; - } + auto fc_instance = FcConfigReference(nullptr); + assert(fc_instance != nullptr); - /* Resolve the name and populate the information structure */ - pat = FcNameParse((FcChar8 *)font_family); - if (font_style != nullptr) FcPatternAddString(pat, FC_STYLE, (FcChar8 *)font_style); - FcConfigSubstitute(nullptr, pat, FcMatchPattern); - FcDefaultSubstitute(pat); - fs = FcFontSetCreate(); - match = FcFontMatch(nullptr, pat, &result); + FcPattern *match; + FcPattern *pat; + FcFontSet *fs; + FcResult result; + char *font_style; + char *font_family; - if (fs != nullptr && match != nullptr) { - int i; - FcChar8 *family; - FcChar8 *style; - FcChar8 *file; - int32_t index; - FcFontSetAdd(fs, match); + /* Split & strip the font's style */ + font_family = stredup(font_name); + font_style = strchr(font_family, ','); + if (font_style != nullptr) { + font_style[0] = '\0'; + font_style++; + while (*font_style == ' ' || *font_style == '\t') font_style++; + } - for (i = 0; err != FT_Err_Ok && i < fs->nfont; i++) { - /* Try the new filename */ - if (FcPatternGetString(fs->fonts[i], FC_FILE, 0, &file) == FcResultMatch && - FcPatternGetString(fs->fonts[i], FC_FAMILY, 0, &family) == FcResultMatch && - FcPatternGetString(fs->fonts[i], FC_STYLE, 0, &style) == FcResultMatch && - FcPatternGetInteger(fs->fonts[i], FC_INDEX, 0, &index) == FcResultMatch) { + /* Resolve the name and populate the information structure */ + pat = FcNameParse((FcChar8 *)font_family); + if (font_style != nullptr) FcPatternAddString(pat, FC_STYLE, (FcChar8 *)font_style); + FcConfigSubstitute(nullptr, pat, FcMatchPattern); + FcDefaultSubstitute(pat); + fs = FcFontSetCreate(); + match = FcFontMatch(nullptr, pat, &result); - /* The correct style? */ - if (font_style != nullptr && !StrEqualsIgnoreCase(font_style, (char *)style)) continue; + if (fs != nullptr && match != nullptr) { + int i; + FcChar8 *family; + FcChar8 *style; + FcChar8 *file; + int32_t index; + FcFontSetAdd(fs, match); - /* Font config takes the best shot, which, if the family name is spelled - * wrongly a 'random' font, so check whether the family name is the - * same as the supplied name */ - if (StrEqualsIgnoreCase(font_family, (char *)family)) { - err = FT_New_Face(_library, (char *)file, index, face); - } + for (i = 0; err != FT_Err_Ok && i < fs->nfont; i++) { + /* Try the new filename */ + if (FcPatternGetString(fs->fonts[i], FC_FILE, 0, &file) == FcResultMatch && + FcPatternGetString(fs->fonts[i], FC_FAMILY, 0, &family) == FcResultMatch && + FcPatternGetString(fs->fonts[i], FC_STYLE, 0, &style) == FcResultMatch && + FcPatternGetInteger(fs->fonts[i], FC_INDEX, 0, &index) == FcResultMatch) { + + /* The correct style? */ + if (font_style != nullptr && !StrEqualsIgnoreCase(font_style, (char *)style)) continue; + + /* Font config takes the best shot, which, if the family name is spelled + * wrongly a 'random' font, so check whether the family name is the + * same as the supplied name */ + if (StrEqualsIgnoreCase(font_family, (char *)family)) { + err = FT_New_Face(_library, (char *)file, index, face); } } } - - free(font_family); - FcPatternDestroy(pat); - FcFontSetDestroy(fs); - FcFini(); } + free(font_family); + FcPatternDestroy(pat); + FcFontSetDestroy(fs); + FcConfigDestroy(fc_instance); + return err; } @@ -98,10 +102,13 @@ FT_Error GetFontByFaceName(const char *font_name, FT_Face *face) bool SetFallbackFont(FontCacheSettings *settings, const char *language_isocode, int winlangid, MissingGlyphSearcher *callback) { - if (!FcInit()) return false; - bool ret = false; + if (!FcInit()) return ret; + + auto fc_instance = FcConfigReference(nullptr); + assert(fc_instance != nullptr); + /* Fontconfig doesn't handle full language isocodes, only the part * before the _ of e.g. en_GB is used, so "remove" everything after * the _. */ @@ -168,6 +175,6 @@ bool SetFallbackFont(FontCacheSettings *settings, const char *language_isocode, FcFontSetDestroy(fs); } - FcFini(); + FcConfigDestroy(fc_instance); return ret; } From a2c0e6aa1863db1a7b1b32b6834f240bd44c5cf7 Mon Sep 17 00:00:00 2001 From: Jonathan G Rennison Date: Sat, 3 Jun 2023 22:04:24 +0100 Subject: [PATCH 19/25] Fix #10831: Level crossing parts left barred after crossing tile removal (#10874) --- src/rail_cmd.cpp | 2 +- src/road_cmd.cpp | 2 +- src/road_func.h | 1 + src/train_cmd.cpp | 41 ++++++++++++++++++++++++++++++++++++++++- 4 files changed, 43 insertions(+), 3 deletions(-) diff --git a/src/rail_cmd.cpp b/src/rail_cmd.cpp index 1f44022211..eee3856a24 100644 --- a/src/rail_cmd.cpp +++ b/src/rail_cmd.cpp @@ -651,7 +651,7 @@ CommandCost CmdRemoveSingleRail(DoCommandFlag flags, TileIndex tile, Track track cost.AddCost(RailClearCost(GetRailType(tile))); if (flags & DC_EXEC) { - MarkDirtyAdjacentLevelCrossingTiles(tile, GetCrossingRoadAxis(tile)); + UpdateAdjacentLevelCrossingTilesOnLevelCrossingRemoval(tile, GetCrossingRoadAxis(tile)); if (HasReservedTracks(tile, trackbit)) { v = GetTrainForReservation(tile, track); diff --git a/src/road_cmd.cpp b/src/road_cmd.cpp index 0f112dc0a7..c29fef09e7 100644 --- a/src/road_cmd.cpp +++ b/src/road_cmd.cpp @@ -504,7 +504,7 @@ static CommandCost RemoveRoad(TileIndex tile, DoCommandFlag flags, RoadBits piec } if (flags & DC_EXEC) { - MarkDirtyAdjacentLevelCrossingTiles(tile, GetCrossingRoadAxis(tile)); + UpdateAdjacentLevelCrossingTilesOnLevelCrossingRemoval(tile, GetCrossingRoadAxis(tile)); /* A full diagonal road tile has two road bits. */ UpdateCompanyRoadInfrastructure(existing_rt, GetRoadOwner(tile, rtt), -2); diff --git a/src/road_func.h b/src/road_func.h index 3c770a9de1..9b66fcc85b 100644 --- a/src/road_func.h +++ b/src/road_func.h @@ -155,6 +155,7 @@ RoadTypes AddDateIntroducedRoadTypes(RoadTypes current, TimerGameCalendar::Date void UpdateLevelCrossing(TileIndex tile, bool sound = true, bool force_bar = false); void MarkDirtyAdjacentLevelCrossingTiles(TileIndex tile, Axis road_axis); +void UpdateAdjacentLevelCrossingTilesOnLevelCrossingRemoval(TileIndex tile, Axis road_axis); void UpdateCompanyRoadInfrastructure(RoadType rt, Owner o, int count); struct TileInfo; diff --git a/src/train_cmd.cpp b/src/train_cmd.cpp index 13c241f4e9..6a0243d26c 100644 --- a/src/train_cmd.cpp +++ b/src/train_cmd.cpp @@ -1790,7 +1790,8 @@ void UpdateLevelCrossing(TileIndex tile, bool sound, bool force_bar) /** * Find adjacent level crossing tiles in this multi-track crossing and mark them dirty. - * @param The tile which causes the update. + * @param tile The tile which causes the update. + * @param road_axis The road axis. */ void MarkDirtyAdjacentLevelCrossingTiles(TileIndex tile, Axis road_axis) { @@ -1804,6 +1805,44 @@ void MarkDirtyAdjacentLevelCrossingTiles(TileIndex tile, Axis road_axis) } } +/** + * Update adjacent level crossing tiles in this multi-track crossing, due to removal of a level crossing tile. + * @param tile The crossing tile which has been or is about to be removed, and which caused the update. + * @param road_axis The road axis. + */ +void UpdateAdjacentLevelCrossingTilesOnLevelCrossingRemoval(TileIndex tile, Axis road_axis) +{ + const DiagDirection dir1 = AxisToDiagDir(road_axis); + const DiagDirection dir2 = ReverseDiagDir(dir1); + for (DiagDirection dir : { dir1, dir2 }) { + const TileIndexDiff diff = TileOffsByDiagDir(dir); + bool occupied = false; + for (TileIndex t = tile + diff; t < Map::Size() && IsLevelCrossingTile(t) && GetCrossingRoadAxis(t) == road_axis; t += diff) { + occupied |= CheckLevelCrossing(t); + } + if (occupied) { + /* Mark the immediately adjacent tile dirty */ + const TileIndex t = tile + diff; + if (t < Map::Size() && IsLevelCrossingTile(t) && GetCrossingRoadAxis(t) == road_axis) { + MarkTileDirtyByTile(t); + } + } else { + /* Unbar the crossing tiles in this direction as necessary */ + for (TileIndex t = tile + diff; t < Map::Size() && IsLevelCrossingTile(t) && GetCrossingRoadAxis(t) == road_axis; t += diff) { + if (IsCrossingBarred(t)) { + /* The crossing tile is barred, unbar it and continue to check the next tile */ + SetCrossingBarred(t, false); + MarkTileDirtyByTile(t); + } else { + /* The crossing tile is already unbarred, mark the tile dirty and stop checking */ + MarkTileDirtyByTile(t); + break; + } + } + } + } +} + /** * Bars crossing and plays ding-ding sound if not barred already * @param tile tile with crossing From 8a2d55090475042cf307f58ab9791e2f1294eda3 Mon Sep 17 00:00:00 2001 From: PeterN Date: Sat, 3 Jun 2023 23:25:01 +0100 Subject: [PATCH 20/25] Codechange: Use std::reverse instead of custom implementation. (#10918) --- src/core/mem_func.hpp | 33 --------------------------------- src/sortlist_type.h | 2 +- 2 files changed, 1 insertion(+), 34 deletions(-) diff --git a/src/core/mem_func.hpp b/src/core/mem_func.hpp index 12acce13fc..81bf903fea 100644 --- a/src/core/mem_func.hpp +++ b/src/core/mem_func.hpp @@ -65,37 +65,4 @@ static inline int MemCmpT(const T *ptr1, const T *ptr2, size_t num = 1) return memcmp(ptr1, ptr2, num * sizeof(T)); } -/** - * Type safe memory reverse operation. - * Reverse a block of memory in steps given by the - * type of the pointers. - * - * @param ptr1 Start-pointer to the block of memory. - * @param ptr2 End-pointer to the block of memory. - */ -template -static inline void MemReverseT(T *ptr1, T *ptr2) -{ - assert(ptr1 != nullptr && ptr2 != nullptr); - assert(ptr1 < ptr2); - - do { - Swap(*ptr1, *ptr2); - } while (++ptr1 < --ptr2); -} - -/** - * Type safe memory reverse operation (overloaded) - * - * @param ptr Pointer to the block of memory. - * @param num The number of items we want to reverse. - */ -template -static inline void MemReverseT(T *ptr, size_t num) -{ - assert(ptr != nullptr); - - MemReverseT(ptr, ptr + (num - 1)); -} - #endif /* MEM_FUNC_HPP */ diff --git a/src/sortlist_type.h b/src/sortlist_type.h index 21cbca16fa..45799cd835 100644 --- a/src/sortlist_type.h +++ b/src/sortlist_type.h @@ -234,7 +234,7 @@ public: { this->flags ^= VL_DESC; - if (this->IsSortable()) MemReverseT(std::vector::data(), std::vector::size()); + if (this->IsSortable()) std::reverse(std::vector::begin(), std::vector::end()); } /** From a969a78f81277f5d27e393469e677a59d4c2d730 Mon Sep 17 00:00:00 2001 From: Patric Stout Date: Sat, 3 Jun 2023 23:08:37 +0200 Subject: [PATCH 21/25] Fix: [SDL] unify the way X11 and Wayland handle mouse events Basically, we drop RelativeMode completely, and use the same trick as used by the Windows driver: read all motion events till the last one, and use that as value. --- src/video/sdl2_v.cpp | 22 +++++++++++++++------- src/video/sdl_v.cpp | 17 +++++++++++++++-- 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/src/video/sdl2_v.cpp b/src/video/sdl2_v.cpp index 6f6cb51561..f455fdd7c7 100644 --- a/src/video/sdl2_v.cpp +++ b/src/video/sdl2_v.cpp @@ -371,12 +371,25 @@ bool VideoDriver_SDL_Base::PollEvent() if (!SDL_PollEvent(&ev)) return false; switch (ev.type) { - case SDL_MOUSEMOTION: - if (_cursor.UpdateCursorPosition(ev.motion.x, ev.motion.y, true)) { + case SDL_MOUSEMOTION: { + int32_t x = ev.motion.x; + int32_t y = ev.motion.y; + + if (_cursor.fix_at) { + /* Get all queued mouse events now in case we have to warp the cursor. In the + * end, we only care about the current mouse position and not bygone events. */ + while (SDL_PeepEvents(&ev, 1, SDL_GETEVENT, SDL_MOUSEMOTION, SDL_MOUSEMOTION)) { + x = ev.motion.x; + y = ev.motion.y; + } + } + + if (_cursor.UpdateCursorPosition(x, y, false)) { SDL_WarpMouseInWindow(this->sdl_window, _cursor.pos.x, _cursor.pos.y); } HandleMouseEvents(); break; + } case SDL_MOUSEWHEEL: if (ev.wheel.y > 0) { @@ -478,10 +491,8 @@ bool VideoDriver_SDL_Base::PollEvent() } else if (ev.window.event == SDL_WINDOWEVENT_ENTER) { // mouse entered the window, enable cursor _cursor.in_window = true; -#ifdef __EMSCRIPTEN__ /* Ensure pointer lock will not occur. */ SDL_SetRelativeMouseMode(SDL_FALSE); -#endif } else if (ev.window.event == SDL_WINDOWEVENT_LEAVE) { // mouse left the window, undraw cursor UndrawMouseCursor(); @@ -500,9 +511,6 @@ static const char *InitializeSDL() * UpdateWindowSurface() to update the window's texture instead of * its surface. */ SDL_SetHint(SDL_HINT_FRAMEBUFFER_ACCELERATION, "0"); -#ifndef __EMSCRIPTEN__ - SDL_SetHint(SDL_HINT_MOUSE_RELATIVE_MODE_WARP, "1"); -#endif /* Check if the video-driver is already initialized. */ if (SDL_WasInit(SDL_INIT_VIDEO) != 0) return nullptr; diff --git a/src/video/sdl_v.cpp b/src/video/sdl_v.cpp index dde67c909b..4f02bf36e3 100644 --- a/src/video/sdl_v.cpp +++ b/src/video/sdl_v.cpp @@ -478,12 +478,25 @@ bool VideoDriver_SDL::PollEvent() if (!SDL_PollEvent(&ev)) return false; switch (ev.type) { - case SDL_MOUSEMOTION: - if (_cursor.UpdateCursorPosition(ev.motion.x, ev.motion.y, true)) { + case SDL_MOUSEMOTION: { + int32_t x = ev.motion.x; + int32_t y = ev.motion.y; + + if (_cursor.fix_at) { + /* Get all queued mouse events now in case we have to warp the cursor. In the + * end, we only care about the current mouse position and not bygone events. */ + while (SDL_PeepEvents(&ev, 1, SDL_GETEVENT, SDL_MOUSEMOTION)) { + x = ev.motion.x; + y = ev.motion.y; + } + } + + if (_cursor.UpdateCursorPosition(x, y, false)) { SDL_WarpMouse(_cursor.pos.x, _cursor.pos.y); } HandleMouseEvents(); break; + } case SDL_MOUSEBUTTONDOWN: if (_rightclick_emulate && SDL_GetModState() & KMOD_CTRL) { From e83f244750a03045893c19cf78b63693a23fc9f7 Mon Sep 17 00:00:00 2001 From: Patric Stout Date: Sat, 3 Jun 2023 23:09:41 +0200 Subject: [PATCH 22/25] Codechange: simplify UpdateCursorPositionRelative The function is only called with fix_at=true, so don't support the other cases. --- src/gfx.cpp | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/src/gfx.cpp b/src/gfx.cpp index 4d268e3dbe..08a2f62779 100644 --- a/src/gfx.cpp +++ b/src/gfx.cpp @@ -1880,27 +1880,17 @@ void SetAnimatedMouseCursor(const AnimCursor *table) } /** - * Update cursor position on mouse movement for relative modes. + * Update cursor position based on a relative change. + * * @param delta_x How much change in the X position. * @param delta_y How much change in the Y position. */ void CursorVars::UpdateCursorPositionRelative(int delta_x, int delta_y) { - if (this->fix_at) { - this->delta.x = delta_x; - this->delta.y = delta_y; - } else { - int last_position_x = this->pos.x; - int last_position_y = this->pos.y; + assert(this->fix_at); - this->pos.x = Clamp(this->pos.x + delta_x, 0, _cur_resolution.width - 1); - this->pos.y = Clamp(this->pos.y + delta_y, 0, _cur_resolution.height - 1); - - this->delta.x = last_position_x - this->pos.x; - this->delta.y = last_position_y - this->pos.y; - - this->dirty = true; - } + this->delta.x = delta_x; + this->delta.y = delta_y; } /** From 0d840b457012b320a4110b537edcafa95108feb5 Mon Sep 17 00:00:00 2001 From: Patric Stout Date: Sat, 3 Jun 2023 23:10:16 +0200 Subject: [PATCH 23/25] Codechange: remove queue_wrap / last_position from mouse movement No backend uses it anymore, so also no longer any need to support it. --- src/gfx.cpp | 40 ++++++------------------------------ src/gfx_type.h | 6 +----- src/video/allegro_v.cpp | 2 +- src/video/cocoa/cocoa_wnd.mm | 2 +- src/video/sdl2_v.cpp | 2 +- src/video/sdl_v.cpp | 2 +- src/video/win32_v.cpp | 2 +- 7 files changed, 12 insertions(+), 44 deletions(-) diff --git a/src/gfx.cpp b/src/gfx.cpp index 08a2f62779..2f9e71c95b 100644 --- a/src/gfx.cpp +++ b/src/gfx.cpp @@ -1897,50 +1897,22 @@ void CursorVars::UpdateCursorPositionRelative(int delta_x, int delta_y) * Update cursor position on mouse movement. * @param x New X position. * @param y New Y position. - * @param queued_warp True, if the OS queues mouse warps after pending mouse movement events. - * False, if the warp applies instantaneous. * @return true, if the OS cursor position should be warped back to this->pos. */ -bool CursorVars::UpdateCursorPosition(int x, int y, bool queued_warp) +bool CursorVars::UpdateCursorPosition(int x, int y) { - /* Detecting relative mouse movement is somewhat tricky. - * - There may be multiple mouse move events in the video driver queue (esp. when OpenTTD lags a bit). - * - When we request warping the mouse position (return true), a mouse move event is appended at the end of the queue. - * - * So, when this->fix_at is active, we use the following strategy: - * - The first movement triggers the warp to reset the mouse position. - * - Subsequent events have to compute movement relative to the previous event. - * - The relative movement is finished, when we receive the event matching the warp. - */ + this->delta.x = x - this->pos.x; + this->delta.y = y - this->pos.y; - if (x == this->pos.x && y == this->pos.y) { - /* Warp finished. */ - this->queued_warp = false; - } - - this->delta.x = x - (this->queued_warp ? this->last_position.x : this->pos.x); - this->delta.y = y - (this->queued_warp ? this->last_position.y : this->pos.y); - - this->last_position.x = x; - this->last_position.y = y; - - bool need_warp = false; if (this->fix_at) { - if (this->delta.x != 0 || this->delta.y != 0) { - /* Trigger warp. - * Note: We also trigger warping again, if there is already a pending warp. - * This makes it more tolerant about the OS or other software in between - * botchering the warp. */ - this->queued_warp = queued_warp; - need_warp = true; - } + return this->delta.x != 0 || this->delta.y != 0; } else if (this->pos.x != x || this->pos.y != y) { - this->queued_warp = false; // Cancel warping, we are no longer confining the position. this->dirty = true; this->pos.x = x; this->pos.y = y; } - return need_warp; + + return false; } bool ChangeResInGame(int width, int height) diff --git a/src/gfx_type.h b/src/gfx_type.h index 50ac4f6892..55f6a361a2 100644 --- a/src/gfx_type.h +++ b/src/gfx_type.h @@ -144,11 +144,7 @@ struct CursorVars { bool vehchain; ///< vehicle chain is dragged void UpdateCursorPositionRelative(int delta_x, int delta_y); - bool UpdateCursorPosition(int x, int y, bool queued_warp); - -private: - bool queued_warp; - Point last_position; + bool UpdateCursorPosition(int x, int y); }; /** Data about how and where to blit pixels. */ diff --git a/src/video/allegro_v.cpp b/src/video/allegro_v.cpp index 21386f3405..5cf0c5852a 100644 --- a/src/video/allegro_v.cpp +++ b/src/video/allegro_v.cpp @@ -391,7 +391,7 @@ bool VideoDriver_Allegro::PollEvent() } /* Mouse movement */ - if (_cursor.UpdateCursorPosition(mouse_x, mouse_y, false)) { + if (_cursor.UpdateCursorPosition(mouse_x, mouse_y)) { position_mouse(_cursor.pos.x, _cursor.pos.y); } if (_cursor.delta.x != 0 || _cursor.delta.y) mouse_action = true; diff --git a/src/video/cocoa/cocoa_wnd.mm b/src/video/cocoa/cocoa_wnd.mm index df08df857f..17d9869731 100644 --- a/src/video/cocoa/cocoa_wnd.mm +++ b/src/video/cocoa/cocoa_wnd.mm @@ -658,7 +658,7 @@ void CocoaDialog(const char *title, const char *message, const char *buttonLabel _cursor.UpdateCursorPositionRelative(event.deltaX * self.getContentsScale, event.deltaY * self.getContentsScale); } else { NSPoint pt = [ self mousePositionFromEvent:event ]; - _cursor.UpdateCursorPosition(pt.x, pt.y, false); + _cursor.UpdateCursorPosition(pt.x, pt.y); } HandleMouseEvents(); diff --git a/src/video/sdl2_v.cpp b/src/video/sdl2_v.cpp index f455fdd7c7..72734e03d4 100644 --- a/src/video/sdl2_v.cpp +++ b/src/video/sdl2_v.cpp @@ -384,7 +384,7 @@ bool VideoDriver_SDL_Base::PollEvent() } } - if (_cursor.UpdateCursorPosition(x, y, false)) { + if (_cursor.UpdateCursorPosition(x, y)) { SDL_WarpMouseInWindow(this->sdl_window, _cursor.pos.x, _cursor.pos.y); } HandleMouseEvents(); diff --git a/src/video/sdl_v.cpp b/src/video/sdl_v.cpp index 4f02bf36e3..eb05f32797 100644 --- a/src/video/sdl_v.cpp +++ b/src/video/sdl_v.cpp @@ -491,7 +491,7 @@ bool VideoDriver_SDL::PollEvent() } } - if (_cursor.UpdateCursorPosition(x, y, false)) { + if (_cursor.UpdateCursorPosition(x, y)) { SDL_WarpMouse(_cursor.pos.x, _cursor.pos.y); } HandleMouseEvents(); diff --git a/src/video/win32_v.cpp b/src/video/win32_v.cpp index b1c8cafae5..7b06809a55 100644 --- a/src/video/win32_v.cpp +++ b/src/video/win32_v.cpp @@ -490,7 +490,7 @@ LRESULT CALLBACK WndProcGdi(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam) } } - if (_cursor.UpdateCursorPosition(x, y, false)) { + if (_cursor.UpdateCursorPosition(x, y)) { POINT pt; pt.x = _cursor.pos.x; pt.y = _cursor.pos.y; From 5821194ad1367258e754e1f11426395ce85804f0 Mon Sep 17 00:00:00 2001 From: Patric Stout Date: Sun, 4 Jun 2023 02:10:21 +0200 Subject: [PATCH 24/25] Add: [Linux] change default scroll mode to non-mouse-lock (#10920) Wayland doesn't support mouse warping, X11 only for native systems (so not for remote desktop, WSLg, etc), and emscripten neither without complications. All these cannot offer a mouse-lock. --- src/lang/english.txt | 2 +- src/table/settings/gui_settings.ini | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/lang/english.txt b/src/lang/english.txt index f5ed779815..0c4e06aff6 100644 --- a/src/lang/english.txt +++ b/src/lang/english.txt @@ -1558,7 +1558,7 @@ STR_CONFIG_SETTING_LINKGRAPH_COLOURS_GREY_TO_RED :Grey to red STR_CONFIG_SETTING_LINKGRAPH_COLOURS_GREYSCALE :Greyscale STR_CONFIG_SETTING_SCROLLMODE :Viewport scroll behaviour: {STRING2} -STR_CONFIG_SETTING_SCROLLMODE_HELPTEXT :Behaviour when scrolling the map +STR_CONFIG_SETTING_SCROLLMODE_HELPTEXT :Behaviour when scrolling the map. The "mouse position locked" options don't work on all systems, like web-based versions, touchscreens, Linux with Wayland, and others ###length 4 STR_CONFIG_SETTING_SCROLLMODE_DEFAULT :Move viewport with RMB, mouse position locked STR_CONFIG_SETTING_SCROLLMODE_RMB_LOCKED :Move map with RMB, mouse position locked diff --git a/src/table/settings/gui_settings.ini b/src/table/settings/gui_settings.ini index 08f43b6661..d2dc032fb6 100644 --- a/src/table/settings/gui_settings.ini +++ b/src/table/settings/gui_settings.ini @@ -98,7 +98,7 @@ strval = STR_CONFIG_SETTING_AUTOSCROLL_DISABLED cat = SC_BASIC [SDTC_VAR] -ifdef = __EMSCRIPTEN__ +ifdef = UNIX var = gui.scroll_mode type = SLE_UINT8 flags = SF_NOT_IN_SAVE | SF_NO_NETWORK_SYNC | SF_GUI_DROPDOWN @@ -111,7 +111,7 @@ strval = STR_CONFIG_SETTING_SCROLLMODE_DEFAULT cat = SC_BASIC [SDTC_VAR] -ifndef = __EMSCRIPTEN__ +ifndef = UNIX var = gui.scroll_mode type = SLE_UINT8 flags = SF_NOT_IN_SAVE | SF_NO_NETWORK_SYNC | SF_GUI_DROPDOWN From 3effb8931c36b8bbacae8fe77ad5b85406767d9f Mon Sep 17 00:00:00 2001 From: Jonathan G Rennison Date: Sun, 4 Jun 2023 10:15:35 +0100 Subject: [PATCH 25/25] Add: [Script] GSAsyncMode to set async mode of gamescript commands (#10913) In asynchronous mode, don't wait for result of executed command, just fire-and-forget, and return estimated cost/result --- src/script/api/CMakeLists.txt | 2 + src/script/api/script_asyncmode.cpp | 61 ++++++++++++++++++++++++++++ src/script/api/script_asyncmode.hpp | 63 +++++++++++++++++++++++++++++ src/script/api/script_object.cpp | 34 +++++++++++++--- src/script/api/script_object.hpp | 30 +++++++++++--- src/script/script_storage.hpp | 9 +++++ 6 files changed, 189 insertions(+), 10 deletions(-) create mode 100644 src/script/api/script_asyncmode.cpp create mode 100644 src/script/api/script_asyncmode.hpp diff --git a/src/script/api/CMakeLists.txt b/src/script/api/CMakeLists.txt index 0cab134930..f41efd068c 100644 --- a/src/script/api/CMakeLists.txt +++ b/src/script/api/CMakeLists.txt @@ -146,6 +146,7 @@ add_files( script_accounting.hpp script_admin.hpp script_airport.hpp + script_asyncmode.hpp script_base.hpp script_basestation.hpp script_bridge.hpp @@ -219,6 +220,7 @@ add_files( script_accounting.cpp script_admin.cpp script_airport.cpp + script_asyncmode.cpp script_base.cpp script_basestation.cpp script_bridge.cpp diff --git a/src/script/api/script_asyncmode.cpp b/src/script/api/script_asyncmode.cpp new file mode 100644 index 0000000000..afad59187d --- /dev/null +++ b/src/script/api/script_asyncmode.cpp @@ -0,0 +1,61 @@ +/* + * This file is part of OpenTTD. + * OpenTTD is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software Foundation, version 2. + * OpenTTD is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the GNU General Public License for more details. You should have received a copy of the GNU General Public License along with OpenTTD. If not, see . + */ + +/** @file script_asyncmode.cpp Implementation of ScriptAsyncMode. */ + +#include "../../stdafx.h" +#include "script_asyncmode.hpp" +#include "../script_instance.hpp" +#include "../script_fatalerror.hpp" + +#include "../../safeguards.h" + +bool ScriptAsyncMode::AsyncModeProc() +{ + /* In async mode we only return 'true', telling the DoCommand it + * should stop run the command in asynchronous/fire-and-forget mode. */ + return true; +} + +bool ScriptAsyncMode::NonAsyncModeProc() +{ + /* In non-async mode we only return 'false', normal operation. */ + return false; +} + +ScriptAsyncMode::ScriptAsyncMode(HSQUIRRELVM vm) +{ + int nparam = sq_gettop(vm) - 1; + if (nparam < 1) { + throw sq_throwerror(vm, "You need to pass a boolean to the constructor"); + } + + SQBool sqasync; + if (SQ_FAILED(sq_getbool(vm, 2, &sqasync))) { + throw sq_throwerror(vm, "Argument must be a boolean"); + } + + this->last_mode = this->GetDoCommandMode(); + this->last_instance = this->GetDoCommandModeInstance(); + + this->SetDoCommandAsyncMode(sqasync ? &ScriptAsyncMode::AsyncModeProc : &ScriptAsyncMode::NonAsyncModeProc, this); +} + +void ScriptAsyncMode::FinalRelease() +{ + if (this->GetDoCommandAsyncModeInstance() != this) { + /* Ignore this error if the script already died. */ + if (!ScriptObject::GetActiveInstance()->IsDead()) { + throw Script_FatalError("Asyncmode object was removed while it was not the latest *Mode object created."); + } + } +} + +ScriptAsyncMode::~ScriptAsyncMode() +{ + this->SetDoCommandAsyncMode(this->last_mode, this->last_instance); +} diff --git a/src/script/api/script_asyncmode.hpp b/src/script/api/script_asyncmode.hpp new file mode 100644 index 0000000000..ba8014b86d --- /dev/null +++ b/src/script/api/script_asyncmode.hpp @@ -0,0 +1,63 @@ +/* + * This file is part of OpenTTD. + * OpenTTD is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software Foundation, version 2. + * OpenTTD is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the GNU General Public License for more details. You should have received a copy of the GNU General Public License along with OpenTTD. If not, see . + */ + +/** @file script_asyncmode.hpp Switch the script instance to Async Mode. */ + +#ifndef SCRIPT_ASYNCMODE_HPP +#define SCRIPT_ASYNCMODE_HPP + +#include "script_object.hpp" + +/** + * Class to switch current mode to Async Mode. + * If you create an instance of this class, the mode will be switched to + * either Asynchronous or Non-Asynchronous mode. + * The original mode is stored and recovered from when ever the instance is destroyed. + * In Asynchronous mode all the commands you execute are queued for later execution. The + * system checks if it would be able to execute your requests, and returns what + * the cost would be. The actual cost and whether the command succeeded when the command + * is eventually executed may differ from what was reported to the script. + * @api game + */ +class ScriptAsyncMode : public ScriptObject { +private: + ScriptAsyncModeProc *last_mode; ///< The previous mode we were in. + ScriptObject *last_instance; ///< The previous instance of the mode. + +protected: + static bool AsyncModeProc(); + static bool NonAsyncModeProc(); + +public: +#ifndef DOXYGEN_API + /** + * The constructor wrapper from Squirrel. + */ + ScriptAsyncMode(HSQUIRRELVM vm); +#else + /** + * Creating instance of this class switches the build mode to Asynchronous or Non-Asynchronous (normal). + * @note When the instance is destroyed, it restores the mode that was + * current when the instance was created! + * @param asynchronous Whether the new mode should be Asynchronous, if true, or Non-Asynchronous, if false. + */ + ScriptAsyncMode(bool asynchronous); +#endif /* DOXYGEN_API */ + + /** + * Destroying this instance resets the asynchronous mode to the mode it was + * in when the instance was created. + */ + ~ScriptAsyncMode(); + + /** + * @api -all + */ + virtual void FinalRelease(); +}; + +#endif /* SCRIPT_ASYNCMODE_HPP */ diff --git a/src/script/api/script_object.cpp b/src/script/api/script_object.cpp index 0f78464432..8e3a684240 100644 --- a/src/script/api/script_object.cpp +++ b/src/script/api/script_object.cpp @@ -82,6 +82,22 @@ ScriptObject::ActiveInstance::~ActiveInstance() return GetStorage()->mode_instance; } +/* static */ void ScriptObject::SetDoCommandAsyncMode(ScriptAsyncModeProc *proc, ScriptObject *instance) +{ + GetStorage()->async_mode = proc; + GetStorage()->async_mode_instance = instance; +} + +/* static */ ScriptAsyncModeProc *ScriptObject::GetDoCommandAsyncMode() +{ + return GetStorage()->async_mode; +} + +/* static */ ScriptObject *ScriptObject::GetDoCommandAsyncModeInstance() +{ + return GetStorage()->async_mode_instance; +} + /* static */ void ScriptObject::SetLastCommand(const CommandDataBuffer &data, Commands cmd) { ScriptStorage *s = GetStorage(); @@ -239,7 +255,7 @@ ScriptObject::ActiveInstance::~ActiveInstance() return ScriptObject::GetActiveInstance()->GetDoCommandCallback(); } -std::tuple ScriptObject::DoCommandPrep() +std::tuple ScriptObject::DoCommandPrep() { if (!ScriptObject::CanSuspend()) { throw Script_FatalError("You are not allowed to execute any DoCommand (even indirect) in your constructor, Save(), Load(), and any valuator."); @@ -248,17 +264,20 @@ std::tuple ScriptObject::DoCommandPrep() /* Are we only interested in the estimate costs? */ bool estimate_only = GetDoCommandMode() != nullptr && !GetDoCommandMode()(); + /* Should the command be executed asynchronously? */ + bool asynchronous = GetDoCommandAsyncMode() != nullptr && GetDoCommandAsyncMode()(); + bool networking = _networking && !_generating_world; if (!ScriptCompanyMode::IsDeity() && !ScriptCompanyMode::IsValid()) { ScriptObject::SetLastError(ScriptError::ERR_PRECONDITION_INVALID_COMPANY); - return { true, estimate_only, networking }; + return { true, estimate_only, asynchronous, networking }; } - return { false, estimate_only, networking }; + return { false, estimate_only, asynchronous, networking }; } -bool ScriptObject::DoCommandProcessResult(const CommandCost &res, Script_SuspendCallbackProc *callback, bool estimate_only) +bool ScriptObject::DoCommandProcessResult(const CommandCost &res, Script_SuspendCallbackProc *callback, bool estimate_only, bool asynchronous) { /* Set the default callback to return a true/false result of the DoCommand */ if (callback == nullptr) callback = &ScriptInstance::DoCommandReturn; @@ -282,8 +301,13 @@ bool ScriptObject::DoCommandProcessResult(const CommandCost &res, Script_Suspend SetLastCost(res.GetCost()); SetLastCommandRes(true); - if (_generating_world) { + if (_generating_world || asynchronous) { IncreaseDoCommandCosts(res.GetCost()); + if (!_generating_world) { + /* Charge a nominal fee for asynchronously executed commands */ + Squirrel *engine = ScriptObject::GetActiveInstance()->engine; + Squirrel::DecreaseOps(engine->GetVM(), 100); + } if (callback != nullptr) { /* Insert return value into to stack and throw a control code that * the return value in the stack should be used. */ diff --git a/src/script/api/script_object.hpp b/src/script/api/script_object.hpp index f4b5694bcc..b2f1f392f9 100644 --- a/src/script/api/script_object.hpp +++ b/src/script/api/script_object.hpp @@ -29,6 +29,11 @@ */ typedef bool (ScriptModeProc)(); +/** + * The callback function for Async Mode-classes. + */ +typedef bool (ScriptAsyncModeProc)(); + /** * Uper-parent object of all API classes. You should never use this class in * your script, as it doesn't publish any public functions. It is used @@ -188,6 +193,21 @@ protected: */ static ScriptObject *GetDoCommandModeInstance(); + /** + * Set the current async mode of your script to this proc. + */ + static void SetDoCommandAsyncMode(ScriptAsyncModeProc *proc, ScriptObject *instance); + + /** + * Get the current async mode your script is currently under. + */ + static ScriptModeProc *GetDoCommandAsyncMode(); + + /** + * Get the instance of the current async mode your script is currently under. + */ + static ScriptObject *GetDoCommandAsyncModeInstance(); + /** * Set the delay of the DoCommand. */ @@ -286,8 +306,8 @@ protected: private: /* Helper functions for DoCommand. */ - static std::tuple DoCommandPrep(); - static bool DoCommandProcessResult(const CommandCost &res, Script_SuspendCallbackProc *callback, bool estimate_only); + static std::tuple DoCommandPrep(); + static bool DoCommandProcessResult(const CommandCost &res, Script_SuspendCallbackProc *callback, bool estimate_only, bool asynchronous); static CommandCallbackData *GetDoCommandCallback(); static Randomizer random_states[OWNER_END]; ///< Random states for each of the scripts (game script uses OWNER_DEITY) }; @@ -338,7 +358,7 @@ namespace ScriptObjectInternal { template bool ScriptObject::ScriptDoCommandHelper::Execute(Script_SuspendCallbackProc *callback, std::tuple args) { - auto [err, estimate_only, networking] = ScriptObject::DoCommandPrep(); + auto [err, estimate_only, asynchronous, networking] = ScriptObject::DoCommandPrep(); if (err) return false; if ((::GetCommandFlags() & CMD_STR_CTRL) == 0) { @@ -363,10 +383,10 @@ bool ScriptObject::ScriptDoCommandHelper Tret res = ::Command::Unsafe((StringID)0, networking ? ScriptObject::GetDoCommandCallback() : nullptr, false, estimate_only, tile, args); if constexpr (std::is_same_v) { - return ScriptObject::DoCommandProcessResult(res, callback, estimate_only); + return ScriptObject::DoCommandProcessResult(res, callback, estimate_only, asynchronous); } else { ScriptObject::SetLastCommandResData(EndianBufferWriter::FromValue(ScriptObjectInternal::RemoveFirstTupleElement(res))); - return ScriptObject::DoCommandProcessResult(std::get<0>(res), callback, estimate_only); + return ScriptObject::DoCommandProcessResult(std::get<0>(res), callback, estimate_only, asynchronous); } } diff --git a/src/script/script_storage.hpp b/src/script/script_storage.hpp index 626c62f025..6f856908d5 100644 --- a/src/script/script_storage.hpp +++ b/src/script/script_storage.hpp @@ -26,6 +26,11 @@ */ typedef bool (ScriptModeProc)(); +/** + * The callback function for Async Mode-classes. + */ +typedef bool (ScriptAsyncModeProc)(); + /** * The storage for each script. It keeps track of important information. */ @@ -34,6 +39,8 @@ friend class ScriptObject; private: ScriptModeProc *mode; ///< The current build mode we are int. class ScriptObject *mode_instance; ///< The instance belonging to the current build mode. + ScriptAsyncModeProc *async_mode; ///< The current command async mode we are in. + class ScriptObject *async_mode_instance; ///< The instance belonging to the current command async mode. CompanyID root_company; ///< The root company, the company that the script really belongs to. CompanyID company; ///< The current company. @@ -61,6 +68,8 @@ public: ScriptStorage() : mode (nullptr), mode_instance (nullptr), + async_mode (nullptr), + async_mode_instance (nullptr), root_company (INVALID_OWNER), company (INVALID_OWNER), delay (1),