feat: support localization (#309) #861
Conversation
|
Have you tested it? Because I don't think our font has CJK glyphs. |
I'm temporarily using the default fonts that come with Windows. I plan to write an additional Python script to bake the fonts later. you can check the preview video here: #862 |
Why additional python scripts tho? I think you can just use |
The main reason is that I don’t want to upload multiple font blob files, for example Source Han Sans, which, if bundled with Japanese, Korean, and Chinese characters, would total about 20MB. I aim to pre-bake the assets to include only the characters that are actually used. |
This sounds like it needs to be part of the build system? Though I guess we'd only rerun when someone sees missing characters |
|
The code is expected to function without any significant issues. If you prefer to load the translation file immediately rather than relying on lazy loading, you can use a empty esp to assist SKSE load all assets at game startup. |
|
Fixed txt was treated as binary. This means that the project must use the git version after this commit |
How can you achieve this? The action forces me to create a PR: https://github.com/lokalise/lokalise-pull-action/blob/b7587f118420378e8fd825a04ac27a70bf2778f0/action.yml#L123-L134 |
|
@doodlum @alandtse @Pentalimbed I think the workflow is now stable and ready for review. Artifact: Release (Support Spanish & Chinese only) |
|
Can you provide a link to the lokalise project so we can join. We'll need at least one of us to be an admin of the project before we pull the PR. |
This requires your team to handle the configuration. My Lokalise account is currently in a free trial state and will expire soon. It's just for demonstration purposes. The configuration is very simple:
|
|
@Sieluna I'm back if you have the time to handle the conflicts, we can get this merged. |
|
@Sieluna we're taking another look in getting this in. We've had a lot more English strings though. How did you generate the initial list? |
WalkthroughThis update introduces a comprehensive localization system. It adds translation utilities, a new English translation file, and workflow automation for synchronizing translations with Lokalise. The menu system is refactored to use localized strings, and font customization is supported. Supporting scripts and configuration files are included for encoding, mapping, and CI integration. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Menu
participant TranslateUtil
participant TranslationFile
User->>Menu: Open UI
Menu->>TranslateUtil: Request localized string (key)
TranslateUtil->>TranslationFile: Lookup translation
TranslationFile-->>TranslateUtil: Return translation or key
TranslateUtil-->>Menu: Return localized string
Menu-->>User: Display localized UI
sequenceDiagram
participant GitHubActions
participant Lokalise
participant Repo
GitHubActions->>Lokalise: Download translations (translate job)
Lokalise-->>GitHubActions: Provide JSON files
GitHubActions->>GitHubActions: Convert JSON to UTF-16LE .txt
GitHubActions->>Repo: Upload translations artifact
GitHubActions->>Lokalise: Push updated translations (lokalise workflow)
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
.gitignore (1)
4-6: Duplicate.idea/entry – keep only one.Line 6 introduces the same ignore pattern that already exists on line 4. This adds noise to future diffs and serves no functional purpose.
-.idea/.gitattributes (1)
4-4: Redundanttextattribute – remove the duplicate token.
textonly needs to be specified once; duplicating it is harmless but untidy and could confuse future readers.-**/Translations/*.txt text text working-tree-encoding=UTF-16LE-BOM eol=crlf +**/Translations/*.txt text working-tree-encoding=UTF-16LE-BOM eol=crlf.github/workflows/iso.json (1)
1-24: Location may be misleading for a data file.Storing static lookup data under
.github/workflows/ties it semantically to CI definitions, which can be counter-intuitive. Consider moving the file to a clear data-oriented path (e.g..github/translation/iso.jsonortools/iso.json) and referencing it accordingly in the scripts.No immediate functional issue, just improved discoverability.
src/Util.h (1)
9-9: IncludingTranslate.htransitively inflates compile units.
Util.his a very widely-included umbrella header. PullingTranslate.hinto it means every translation-agnostic .cpp now recompiles on any localization change, increasing build times.
If only UI-facing modules need translation helpers, prefer includingUtils/Translate.hin those translation-aware headers instead of this global umbrella..github/workflows/lokalise.yaml (1)
36-45: Add missing newline at end of fileThe YAML file is missing a newline character at the end, which is flagged by yamllint.
base_lang: en +
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.gitattributes(1 hunks).github/workflows/build.yaml(3 hunks).github/workflows/iso.json(1 hunks).github/workflows/lokalise.yaml(1 hunks).github/workflows/lokalise_download.mjs(1 hunks).gitignore(1 hunks)package/Interface/Translations/CommunityShaders_english.txt(1 hunks)src/Menu.cpp(20 hunks)src/Menu.h(3 hunks)src/Util.h(1 hunks)src/Utils/Translate.cpp(1 hunks)src/Utils/Translate.h(1 hunks)src/XSEPlugin.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
src/XSEPlugin.cpp (2)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
src/Menu.h (5)
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#1107
File: src/Menu.cpp:310-315
Timestamp: 2025-05-31T02:51:24.195Z
Learning: In the Community Shaders codebase, when validating UI icons, checking for texture presence (non-null pointer) is sufficient. Avoid adding dimension validation checks as icon sizes should vary based on user needs (4K+ resolution users may want higher resolution icons). Don't artificially limit icon dimensions.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-01T18:01:07.079Z
Learning: In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use #define (e.g., #define NTHREADS 128), while more complex constants use static const within namespaces (e.g., Math namespace in Math.hlsli). For epsilon standardization, #define is the appropriate choice since epsilon values are simple scalar constants.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
package/Interface/Translations/CommunityShaders_english.txt (2)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
src/Menu.cpp (6)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#1157
File: src/Feature.cpp:42-49
Timestamp: 2025-06-17T09:27:49.594Z
Learning: In src/Feature.cpp, when an obsolete feature's INI file is deleted, the feature should be silently disabled without surfacing any issues to the user. This is the intended behavior because a deleted INI file for an obsolete feature indicates that the user has properly cleaned up the obsolete feature.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-01T18:01:07.079Z
Learning: In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use #define (e.g., #define NTHREADS 128), while more complex constants use static const within namespaces (e.g., Math namespace in Math.hlsli). For epsilon standardization, #define is the appropriate choice since epsilon values are simple scalar constants.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
🧬 Code Graph Analysis (2)
src/Utils/Translate.cpp (1)
src/Utils/Translate.h (4)
key(52-66)key(52-52)Translate(10-10)Translate(25-28)
src/Utils/Translate.h (1)
src/Utils/Translate.cpp (2)
Translate(7-16)Translate(7-7)
🪛 YAMLlint (1.37.1)
.github/workflows/lokalise.yaml
[error] 45-45: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (11)
src/Utils/Translate.cpp (1)
7-16: LGTM - Clean translation wrapper implementationThe implementation correctly handles the translation lookup with appropriate fallback behavior. Returning the original key when translation fails is a good pattern for debugging and ensuring UI doesn't break with missing translations.
src/Menu.h (2)
63-64: LGTM - Font customization fields addedThe addition of FontPath and FontSize to ThemeSettings enables user font customization. The default font path is appropriate for the CommunityShaders context.
195-205: LGTM - Font loading infrastructureThe fontReloadRequested flag and LoadFont method declaration provide clean infrastructure for dynamic font loading. The method signature suggests proper handling of font refresh scenarios.
.github/workflows/lokalise.yaml (1)
17-34: LGTM - Well-structured translation file conversionThe conversion logic correctly handles UTF-16LE BOM encoding, removes comments and empty lines, and gracefully skips files with missing language mappings. The use of jq for JSON transformation is appropriate.
package/Interface/Translations/CommunityShaders_english.txt (1)
1-158: LGTM - Comprehensive English translation fileThe translation file is well-structured with clear section organization and comprehensive coverage of UI elements. The use of placeholders for dynamic content (
{}) and descriptive keys makes it maintainable and developer-friendly.src/Menu.cpp (3)
236-268: LGTM - Comprehensive font loading implementationThe LoadFont method properly handles font caching, supports multiple language glyph ranges (Default, Cyrillic, Chinese, Japanese), and includes proper DirectX device object refresh. The static caching prevents unnecessary font reloads.
93-94: LGTM - Font settings serializationThe addition of FontPath and FontSize to ThemeSettings serialization enables proper persistence of font customization settings.
347-1680: LGTM - Comprehensive UI localizationThe extensive replacement of hardcoded English strings with localized versions using
_i18nand_i18n_csmacros enables proper internationalization. The implementation maintains the same functionality while supporting multiple languages..github/workflows/lokalise_download.mjs (1)
72-73: LGTM!The command line arguments are properly constructed for the Lokalise CLI.
.github/workflows/build.yaml (1)
143-146: Correct implementation of UTF-16LE BOM encoding.The conversion pipeline properly handles BOM, line endings, and encoding for Skyrim's expected format.
src/Utils/Translate.h (1)
52-66: Well-implemented placeholder replacement logic.The preprocessKey function correctly handles sequential replacement of
{}placeholders with named placeholders from keyArgs.
| ]; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use fmt::make_format_args for better type safety.
The current approach of manually constructing format arguments using internal fmt APIs is fragile and may break with fmt library updates.
🤖 Prompt for AI Agents
In .github/workflows/lokalise_download.mjs around lines 72 to 73, the code
manually constructs format arguments using internal fmt APIs, which is fragile
and risks breaking with future fmt library updates. Replace this manual
construction with fmt::make_format_args to ensure better type safety and
maintainability. Update the code to use fmt::make_format_args for creating
format arguments instead of the current approach.
| async function installLokaliseCLI(timeoutSec) { | ||
| console.log("Installing Lokalise CLI using the official installer script..."); | ||
| const installerUrl = "https://raw.githubusercontent.com/lokalise/lokalise-cli-2-go/master/install.sh"; | ||
|
|
||
| await execFileAsync('sh', ['-c', `curl -sfL ${installerUrl} | sh`], { timeout: timeoutSec * 1000 }); | ||
|
|
||
| console.log("Lokalise CLI installed successfully."); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Verify the security of the installer script execution.
The code downloads and executes a shell script from a remote URL. While this is the official Lokalise installer, it's a security risk to execute remote scripts without verification.
Consider adding integrity verification:
async function installLokaliseCLI(timeoutSec) {
console.log("Installing Lokalise CLI using the official installer script...");
const installerUrl = "https://raw.githubusercontent.com/lokalise/lokalise-cli-2-go/master/install.sh";
- await execFileAsync('sh', ['-c', `curl -sfL ${installerUrl} | sh`], { timeout: timeoutSec * 1000 });
+ // Download and verify the script first
+ const { stdout: script } = await execFileAsync('curl', ['-sfL', installerUrl], { timeout: timeoutSec * 1000 });
+ // Optionally verify script content or checksum here
+ await execFileAsync('sh', ['-c', script], { timeout: timeoutSec * 1000 });
console.log("Lokalise CLI installed successfully.");
}Alternatively, consider pinning to a specific commit hash in the URL for better stability and security.
Enforce integrity verification when installing the Lokalise CLI
To prevent executing unverified code from master, pin the installer to a specific commit or release and verify its checksum before running.
• Location: .github/workflows/lokalise_download.mjs → installLokaliseCLI()
• Issue: unpinned URL (…/master/install.sh) and no checksum/signature check
Proposed change:
async function installLokaliseCLI(timeoutSec) {
console.log("Installing Lokalise CLI using the official installer script...");
- const installerUrl = "https://raw.githubusercontent.com/lokalise/lokalise-cli-2-go/master/install.sh";
+ // Pin to a known-good commit or tag for stability
+ const installerUrl = "https://raw.githubusercontent.com/lokalise/lokalise-cli-2-go/abc1234/install.sh";
- await execFileAsync('sh', ['-c', `curl -sfL ${installerUrl} | sh`], { timeout: timeoutSec * 1000 });
+ // 1) Download script
+ const { stdout: script } = await execFileAsync(
+ 'curl', ['-sfL', installerUrl], { timeout: timeoutSec * 1000 }
+ );
+ // 2) Verify checksum (replace <EXPECTED_SHA256> with actual hash)
+ const hash = crypto.createHash('sha256').update(script).digest('hex');
+ if (hash !== '<EXPECTED_SHA256>') {
+ throw new Error(`Lokalise installer checksum mismatch: ${hash}`);
+ }
+ // 3) Execute verified script
+ await execFileAsync('sh', ['-c', script], { timeout: timeoutSec * 1000 });
console.log("Lokalise CLI installed successfully.");
}– Replace abc1234 and <EXPECTED_SHA256> with the approved commit/tag and its checksum.
– Alternatively, fetch a signed release artifact from GitHub Releases if available.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function installLokaliseCLI(timeoutSec) { | |
| console.log("Installing Lokalise CLI using the official installer script..."); | |
| const installerUrl = "https://raw.githubusercontent.com/lokalise/lokalise-cli-2-go/master/install.sh"; | |
| await execFileAsync('sh', ['-c', `curl -sfL ${installerUrl} | sh`], { timeout: timeoutSec * 1000 }); | |
| console.log("Lokalise CLI installed successfully."); | |
| } | |
| async function installLokaliseCLI(timeoutSec) { | |
| console.log("Installing Lokalise CLI using the official installer script..."); | |
| // Pin to a known-good commit or tag for stability | |
| const installerUrl = "https://raw.githubusercontent.com/lokalise/lokalise-cli-2-go/abc1234/install.sh"; | |
| // 1) Download script | |
| const { stdout: script } = await execFileAsync( | |
| 'curl', ['-sfL', installerUrl], { timeout: timeoutSec * 1000 } | |
| ); | |
| // 2) Verify checksum (replace <EXPECTED_SHA256> with actual hash) | |
| const hash = crypto.createHash('sha256').update(script).digest('hex'); | |
| if (hash !== '<EXPECTED_SHA256>') { | |
| throw new Error(`Lokalise installer checksum mismatch: ${hash}`); | |
| } | |
| // 3) Execute verified script | |
| await execFileAsync('sh', ['-c', script], { timeout: timeoutSec * 1000 }); | |
| console.log("Lokalise CLI installed successfully."); | |
| } |
🤖 Prompt for AI Agents
In .github/workflows/lokalise_download.mjs around lines 45 to 52, the
installLokaliseCLI function uses an unpinned URL to fetch the installer script
from the master branch without verifying its integrity. To fix this, update the
installerUrl to point to a specific commit hash or release tag instead of
master, then download the script separately, compute its SHA256 checksum, and
compare it against a predefined expected value before executing it. This ensures
the script is verified and prevents running untrusted code.
| - name: convert json to utf-16-le-bom txt | ||
| run: | | ||
| for file in $TRANSLATION_CACHE/*.json; do | ||
| if [[ -f "$file" ]]; then | ||
| iso_code=$(basename "$file" .json) | ||
| language=$(jq -r --arg iso "$iso_code" '.iso_lang[$iso]' $ISO) | ||
| if [[ -z "$language" || "$language" == "null" ]]; then | ||
| echo "Warning: ISO code '$iso_code' not found. Skipping $file." >&2 | ||
| continue | ||
| fi | ||
| output_file="$TRANSLATION_FILES/CommunityShaders_${language}.txt" | ||
| if [[ "$(basename "$output_file")" != "$WHITELIST_FILES" ]]; then | ||
| printf "\xFF\xFE" > "$output_file" | ||
| jq -r 'to_entries | .[] | "\(.key)\t\(.value)"' "$file" \ | ||
| | sed 's/$/\r/' | iconv -f UTF-8 -t UTF-16LE >> "$output_file" | ||
| fi | ||
| fi | ||
| done | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for missing ISO codes.
The current implementation logs a warning but continues processing, which could lead to incomplete translation files.
Consider collecting all missing ISO codes and failing the job if critical translations are missing:
+ missing_codes=()
for file in $TRANSLATION_CACHE/*.json; do
if [[ -f "$file" ]]; then
iso_code=$(basename "$file" .json)
language=$(jq -r --arg iso "$iso_code" '.iso_lang[$iso]' $ISO)
if [[ -z "$language" || "$language" == "null" ]]; then
echo "Warning: ISO code '$iso_code' not found. Skipping $file." >&2
+ missing_codes+=("$iso_code")
continue
fi
output_file="$TRANSLATION_FILES/CommunityShaders_${language}.txt"
if [[ "$(basename "$output_file")" != "$WHITELIST_FILES" ]]; then
printf "\xFF\xFE" > "$output_file"
jq -r 'to_entries | .[] | "\(.key)\t\(.value)"' "$file" \
| sed 's/$/\r/' | iconv -f UTF-8 -t UTF-16LE >> "$output_file"
fi
fi
done
+ if [ ${#missing_codes[@]} -gt 0 ]; then
+ echo "Error: Missing ISO codes: ${missing_codes[*]}"
+ exit 1
+ fi🤖 Prompt for AI Agents
In .github/workflows/build.yaml between lines 130 and 149, the script logs a
warning when an ISO code is missing but continues processing, risking incomplete
translation files. Modify the script to collect all missing ISO codes during the
loop and after processing all files, check if any are missing. If there are
missing ISO codes, output an error message listing them and exit the job with a
failure status to prevent incomplete translations.
| inline const char* operator"" _i18n_cs(const char* key, std::size_t) noexcept | ||
| { | ||
| thread_local std::string translation; | ||
| translation = Util::Translate(std::string(key)); | ||
| return translation.c_str(); | ||
| } |
There was a problem hiding this comment.
Thread-local storage may cause issues with multiple calls in the same expression.
The _i18n_cs operator uses a single thread-local string, which means calling it multiple times in the same expression will invalidate previous pointers.
Example of the issue:
// Both pointers will point to the same string (the last translation)
printf("%s %s", "key1"_i18n_cs, "key2"_i18n_cs);Consider using a thread-local circular buffer or documenting this limitation clearly:
inline const char* operator"" _i18n_cs(const char* key, std::size_t) noexcept
{
- thread_local std::string translation;
- translation = Util::Translate(std::string(key));
- return translation.c_str();
+ thread_local std::array<std::string, 4> translations;
+ thread_local size_t index = 0;
+ translations[index] = Util::Translate(std::string(key));
+ const char* result = translations[index].c_str();
+ index = (index + 1) % translations.size();
+ return result;
}🤖 Prompt for AI Agents
In src/Utils/Translate.h around lines 85 to 90, the operator"" _i18n_cs uses a
single thread-local std::string which causes returned pointers to be overwritten
on multiple calls in the same expression. To fix this, implement a thread-local
circular buffer of strings to store multiple translations simultaneously,
cycling through the buffer on each call to avoid pointer invalidation, or
alternatively add clear documentation warning users about this limitation.
| static std::string formatWithArgs(const std::string& translation, const std::vector<std::string>& args) | ||
| { | ||
| std::vector<fmt::format_context::format_arg> formatArgs; | ||
| for (const auto& arg : args) { | ||
| formatArgs.push_back(fmt::detail::make_arg<fmt::format_context>(arg)); | ||
| } | ||
|
|
||
| return vformat(translation, fmt::basic_format_args(formatArgs.data(), static_cast<int>(formatArgs.size()))); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using fmt internal APIs.
The code uses fmt::detail::make_arg which is an internal API that may change between fmt library versions.
Use fmt's public API instead:
static std::string formatWithArgs(const std::string& translation, const std::vector<std::string>& args)
{
- std::vector<fmt::format_context::format_arg> formatArgs;
- for (const auto& arg : args) {
- formatArgs.push_back(fmt::detail::make_arg<fmt::format_context>(arg));
- }
-
- return vformat(translation, fmt::basic_format_args(formatArgs.data(), static_cast<int>(formatArgs.size())));
+ return std::apply([&translation](const auto&... args) {
+ return fmt::vformat(translation, fmt::make_format_args(args...));
+ }, std::tuple_cat(std::make_tuple(), args));
}Alternatively, if variadic expansion isn't suitable, consider using fmt::dynamic_format_arg_store which is part of the public API.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Utils/Translate.h around lines 68 to 77, the code uses the internal API
fmt::detail::make_arg which is not stable across fmt versions. Replace this with
the public API by using fmt::dynamic_format_arg_store to build the argument list
dynamically, then pass it to vformat. This avoids reliance on internal details
and ensures compatibility with future fmt releases.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
4 participants
PR Summary:
Summary by CodeRabbit
New Features
Enhancements
Automation
Chores