feat: localization#2416
Open
jiayev wants to merge 15 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a thread-safe I18n singleton and header, English translation file and extraction/CI tooling, startup wiring and config persistence for locale, CJK font merging, and replaces hardcoded UI strings with translation lookups across menus, settings, features, and the Weather Editor. ChangesI18n integration
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
No actionable suggestions for changed features. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Menu/HomePageRenderer.cpp (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd issue linkage for this feature PR.
Please add an issue reference in the PR description (e.g.,
Implements#123or `Addresses `#123) for traceability. The current title is conventional, but a scoped variant likefeat(i18n): localize home/setup and faq uiwould describe scope better.As per coding guidelines, "When reviewing PRs, please provide suggestions for ... Issue References (if PR fixes bugs or implements features)" and "Conventional Commit Titles ... if the existing title does not describe the code changes."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Menu/HomePageRenderer.cpp` at line 1, Update the PR description to include an issue reference (e.g., "Implements `#123`" or "Addresses `#123`") for this feature change touching HomePageRenderer (class HomePageRenderer / file HomePageRenderer.h/ HomePageRenderer.cpp) and adjust the commit/PR title to a conventional scoped form like "feat(i18n): localize home/setup and faq ui" so the change is traceable and follows the repo's conventional commit and issue-linking guidelines.
🧹 Nitpick comments (1)
package/SKSE/Plugins/CommunityShaders/Translations/en.json (1)
1-8: ⚡ Quick winTighten PR metadata to better match repository conventions.
feat: localizationis valid but very broad for this scope. Considerfeat(i18n): add runtime localization system, and add an issue keyword in the PR body (for example,Implements #<issue-id>orAddresses #<issue-id>).As per coding guidelines, "When reviewing PRs, please provide suggestions for: Conventional Commit Titles ... and Issue References ...".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package/SKSE/Plugins/CommunityShaders/Translations/en.json` around lines 1 - 8, The PR title/metadata is too broad—update the PR title from "feat: localization" to a conventional commit style like "feat(i18n): add runtime localization system" and add an issue reference in the PR body (e.g., "Implements #<issue-id>" or "Addresses #<issue-id>"); ensure this change is reflected in the repository metadata context where relevant (see the "_meta" object in package/SKSE/Plugins/CommunityShaders/Translations/en.json) so the commit message and PR body follow the project's Conventional Commits and Issue Reference guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package/SKSE/Plugins/CommunityShaders/Translations/en.json`:
- Line 70: The translation entry for "menu.setup.weather_editor_unbound" in
en.json contains mojibake (�) instead of an em-dash; update the source default
string where that key is defined to use a proper dash (e.g., "—" or " - "), then
regenerate the translations so
package/SKSE/Plugins/CommunityShaders/Translations/en.json is rebuilt with the
corrected value for "menu.setup.weather_editor_unbound". Ensure the regenerated
file contains the fixed string exactly as in the source default.
In `@src/I18n/I18n.cpp`:
- Around line 89-115: The SetLocale method uses the raw locale string to build
file paths, so first validate the incoming locale in I18n::SetLocale against an
allowlist/pattern (e.g., only expected codes like "en" or "xx" or "xx-YY")
before calling LoadLocaleInto; if validation fails, log a warning and return
without changing strings_, defaultCache_, or currentLocale_. Apply the same
validation wherever locale values are later used to construct file paths (the
other locale-loading call sites referenced around the other locale-loading code)
to prevent directory traversal or malformed inputs.
- Around line 197-204: The lambda passed to std::sort on availableLocales_
violates strict-weak ordering by returning true for equal keys (e.g., "en" vs
"en"); update the comparator used in the std::sort call so it first checks
equality (if a.first == b.first return false), then handle the special-case "en"
(if a.first == "en" return true; if b.first == "en" return false), and finally
compare by a.second < b.second (and if those are equal, tie-break
deterministically e.g., compare a.first < b.first) to ensure a strict-weak
ordering.
- Around line 40-73: The Get() method currently writes into defaultCache_ while
only holding a std::shared_lock (mutex_), causing data races; change the logic
so you never mutate under the shared lock: do the read/lookups under the
existing std::shared_lock, and if the key is missing from defaultCache_ release
the shared_lock and acquire a std::unique_lock on the same mutex_ (or use an
upgradeable lock pattern), re-check defaultCache_ (to avoid TOCTOU), then
emplace the cached string and return its c_str(); reference symbols: I18n::Get,
defaultCache_, strings_, fallback_, mutex_, std::shared_lock/std::unique_lock.
In `@src/I18n/I18n.h`:
- Around line 3-10: The header declares a mutable std::shared_mutex (mutex_) but
does not include <shared_mutex>, making the header non-self-contained; add
`#include` <shared_mutex> to src/I18n/I18n.h so that the declaration of mutable
std::shared_mutex mutex_ (and any uses in I18n.h) compiles regardless of include
order.
In `@src/Menu/SettingsTabRenderer.cpp`:
- Around line 430-434: The selectable items for locales can collide when display
names repeat; update the loop that iterates over locales (the for (const auto&
[code, name] : locales) block) so each ImGui::Selectable uses a unique ID tied
to the locale code (for example by appending "##" + code to the label or by
wrapping the item in ImGui::PushID/PopID) instead of just name.c_str(), keeping
the same isSelected check and calling i18n->SetLocale(code) when selected.
In `@tools/extract-i18n.py`:
- Around line 143-147: The add_string function currently leaves keys in the
key_only set when they're first seen without a default, causing false “key-only”
reports if a default is later found; update add_string to, when called with a
non-empty default, remove the key from key_only (e.g., if key in key_only and
default is not None and default != "")
before assigning strings[key]=default, while preserving the existing conflict
handling (references: add_string, key_only, strings, conflicts).
---
Outside diff comments:
In `@src/Menu/HomePageRenderer.cpp`:
- Line 1: Update the PR description to include an issue reference (e.g.,
"Implements `#123`" or "Addresses `#123`") for this feature change touching
HomePageRenderer (class HomePageRenderer / file HomePageRenderer.h/
HomePageRenderer.cpp) and adjust the commit/PR title to a conventional scoped
form like "feat(i18n): localize home/setup and faq ui" so the change is
traceable and follows the repo's conventional commit and issue-linking
guidelines.
---
Nitpick comments:
In `@package/SKSE/Plugins/CommunityShaders/Translations/en.json`:
- Around line 1-8: The PR title/metadata is too broad—update the PR title from
"feat: localization" to a conventional commit style like "feat(i18n): add
runtime localization system" and add an issue reference in the PR body (e.g.,
"Implements #<issue-id>" or "Addresses #<issue-id>"); ensure this change is
reflected in the repository metadata context where relevant (see the "_meta"
object in package/SKSE/Plugins/CommunityShaders/Translations/en.json) so the
commit message and PR body follow the project's Conventional Commits and Issue
Reference guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a0561e81-a3c5-40e4-b622-57fb391b4aad
📒 Files selected for processing (11)
package/SKSE/Plugins/CommunityShaders/Translations/en.jsonsrc/Features/GrassLighting.cppsrc/I18n/I18n.cppsrc/I18n/I18n.hsrc/Menu/HomePageRenderer.cppsrc/Menu/SettingsTabRenderer.cppsrc/State.cppsrc/Utils/FileSystem.cppsrc/Utils/FileSystem.hsrc/XSEPlugin.cpptools/extract-i18n.py
|
✅ A pre-release build is available for this PR: |
- Integrated I18n for various UI strings in PaletteWindow, CellLightingWidget, WeatherWidget, WeatherUtils, and Widget. - Replaced hardcoded strings with translation keys to support multiple languages. - Updated tooltips and labels for better user experience and clarity. - Ensured consistency across different components of the WeatherEditor.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/Utils/UI.h (1)
494-500:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the header docs to match the new function signature.
The comment block still describes
categoryNameand does not documentdisplayName, which makes the API contract ambiguous for callers.📝 Suggested doc fix
- * `@param` categoryName The name of the category to display + * `@param` categoryKey Stable category key used for state/ID lookup + * `@param` displayName Localized category name to render * `@param` isExpanded Reference to the expansion state * `@param` categoryCount Number of features in the category * `@return` true if the expansion state was toggled */ bool DrawCategoryHeader(const char* categoryKey, const char* displayName, bool& isExpanded, int categoryCount);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Utils/UI.h` around lines 494 - 500, Update the function header comment for DrawCategoryHeader to match the new signature: replace references to the old categoryName with both categoryKey and displayName, describing categoryKey as the internal/unique key or identifier and displayName as the visible label shown in the UI; document isExpanded as a reference boolean representing the current expansion state that may be toggled, categoryCount as the number of features in the category, and state the return value indicates whether the expansion state was toggled (true if toggled). Ensure parameter names (categoryKey, displayName, isExpanded, categoryCount) and the return behavior are explicitly documented in the comment block.src/Features/WeatherEditor.cpp (1)
1006-1035:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t use localized flag text as logic identifiers
GetWeatherFlagNames()now returns translated labels, but downstream logic still expects English tokens (chip filtering and color mapping). This breaks behavior in non-English locales.A robust fix is to keep canonical identifiers (enum/keys) for logic and localize only at render time.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Features/WeatherEditor.cpp` around lines 1006 - 1035, GetWeatherFlagNames currently returns localized labels which breaks downstream logic that expects canonical tokens; change it to emit canonical identifiers (use magic_enum::enum_name(flagValue) trimmed of leading 'k' or a stable key like the enum value) and keep localization only for rendering (e.g., add/GetWeatherFlagLabel or return pairs {id,label} so UI uses label but filtering/color logic uses id). Update usages that reference GetWeatherFlagNames, chip filtering and color mapping to use the canonical id (from magic_enum/flagValue) for logic and use the localized label only for display.src/WeatherEditor/Weather/WeatherWidget.cpp (1)
1822-1835:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a stable unique ImGui ID for feature tree nodes
TreeNodeEx(displayName.c_str(), ...)can collide when localized display names are identical. Keep display text, but append##withfeatureNamefor unique IDs.💡 Suggested fix
- std::string displayName = feature->GetDisplayName(); + std::string displayName = feature->GetDisplayName(); + std::string treeLabel = std::format("{}##{}", displayName, featureName); @@ - if (ImGui::TreeNodeEx(displayName.c_str(), ImGuiTreeNodeFlags_SpanAvailWidth)) { + if (ImGui::TreeNodeEx(treeLabel.c_str(), ImGuiTreeNodeFlags_SpanAvailWidth)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/WeatherEditor/Weather/WeatherWidget.cpp` around lines 1822 - 1835, Tree node labels use the user-visible displayName which can collide when localized; change the ImGui::TreeNodeEx call to use a stable unique ID by appending "##" + featureName to the label (keep displayName for visible text but supply displayName + "##" + featureName as the ImGui label). Update the call site where TreeNodeEx(displayName.c_str(), ImGuiTreeNodeFlags_SpanAvailWidth) is used (near feature->GetDisplayName(), featureName, pendingFeatureNavigation and getFeatureJson) so the tree node ID is unique while preserving the displayed text.src/Features/TerrainShadows.cpp (1)
267-285:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
Precompute()no longer guaranteestexShadowHeightbefore use
UpdateShadow()usestexShadowHeight->uav(Line 384), butPrecompute()now only clears bindings and flips the flag. IftexShadowHeightis null, this is a crash path.At minimum, guard
UpdateShadow()and keepneedPrecomputetrue when resource creation fails; ideally restore/create the shadow texture inPrecompute().As per coding guidelines, "Include proper resource management and graceful degradation error handling in all DirectX code to prevent crashes from malformed configurations".
Also applies to: 384-384
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Features/TerrainShadows.cpp` around lines 267 - 285, Precompute(): ensure texShadowHeight is valid before clearing bindings and flipping needPrecompute—either attempt to create/restore the shadow texture inside TerrainShadows::Precompute (create texShadowHeight and its UAV/SRV) or, if creation fails, leave needPrecompute = true and log the failure so UpdateShadow() isn't run with a null resource; also add a null guard at the start of TerrainShadows::UpdateShadow() that early-returns if texShadowHeight or texShadowHeight->uav is null to prevent crashes (references: Precompute(), UpdateShadow(), texShadowHeight, needPrecompute).src/WeatherEditor/EditorWindow.cpp (1)
427-427:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse localized text for combo width calculation
Line 427 sizes the combo using
"Editor ID"instead of the translated label, so longer locales can clip.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/WeatherEditor/EditorWindow.cpp` at line 427, The combo width calculation uses the hardcoded literal "Editor ID" which won't account for translations; update the calculation for comboW to use the actual localized label string used for the combo (i.e., pass the same translated label variable or call that localization function instead of the literal) when calling ImGui::CalcTextSize and computing width so longer locales won't be clipped; reference the existing comboW variable and ImGui::CalcTextSize(...) call to locate and replace the literal.
♻️ Duplicate comments (2)
src/I18n/I18n.h (1)
3-10:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd
<shared_mutex>to make this header self-contained
std::shared_mutexis used on Line 140 but<shared_mutex>is not included.#!/bin/bash # Verify shared_mutex usage and include presence in i18n header rg -n 'std::shared_mutex|std::shared_lock' src/I18n/I18n.h src/I18n/I18n.cpp rg -n '^`#include` <shared_mutex>' src/I18n/I18n.h src/I18n/I18n.cppExpected: first command finds usage; second should include
src/I18n/I18n.hafter fix.Also applies to: 140-140
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/I18n/I18n.h` around lines 3 - 10, The header uses std::shared_mutex (and possibly std::shared_lock) but doesn't include the header; add the missing include by inserting `#include` <shared_mutex> into the include list in I18n.h so the declaration of std::shared_mutex is available and the header is self-contained (referencing std::shared_mutex and any related types used in the I18n class).tools/extract-i18n.py (1)
241-245:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear
key_onlywhen a defaulted string is later found
add_stringupdatesstringsbut leaves stale entries inkey_only, causing false key-only reports.Proposed fix
def add_string(key, default, source_path): if key in strings and strings[key] != default: conflicts.append((key, strings[key], default, str(source_path))) strings[key] = default + key_only.discard(key)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/extract-i18n.py` around lines 241 - 245, The add_string function currently writes defaults into strings but doesn't remove the key from key_only, causing false key-only reports; update add_string (the def add_string(key, default, source_path) block) so that when a key is added/updated with a non-empty default you also discard it from key_only (e.g., key_only.discard(key) or similar) and keep the existing conflict handling that appends to conflicts and sets strings[key] = default.
🧹 Nitpick comments (13)
src/Features/ScreenshotFeature.h (1)
17-17: ⚡ Quick winAdd an issue-linking keyword in the PR description (if an issue exists).
Since this PR implements a feature, please add a reference like
Implements#123or `Addresses `#123for traceability.As per coding guidelines, "
**/*: When reviewing PRs, please provide suggestions for ... Issue References ... for features."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Features/ScreenshotFeature.h` at line 17, Add an issue reference keyword to the PR description for traceability: update the PR body to include a line like "Implements #<issue-number>" or "Addresses #<issue-number>" referencing the related issue for this feature (ScreenshotFeature / GetDisplayName) so reviewers can link the implemented feature to its tracking issue.src/Features/HDRDisplay.h (1)
19-38: Consider a more specific conventional commit title and issue linkSuggested title:
feat(i18n): localize feature labels and summaries
If a tracking issue exists, addImplements #<id>(orAddresses #<id>) to the PR description.As per coding guidelines "
**/*: When reviewing PRs, please provide suggestions for: Conventional Commit Titles ... Issue References ...".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Features/HDRDisplay.h` around lines 19 - 38, Update the PR title and description to follow the conventional commit style and include any tracking issue reference: change the PR title to a more specific conventional commit such as "feat(i18n): localize feature labels and summaries" and add "Implements #<id>" or "Addresses #<id>" in the PR description if a tracking issue exists; reference the HDR feature identifiers found in this diff (e.g., GetDisplayName, GetShortName, GetFeatureSummary and the i18n keys like "feature.hdr_display.*") so reviewers can easily correlate the commit to the localized strings being added.src/Features/TerrainBlending.cpp (1)
1-1: ⚡ Quick winAdd an issue-link keyword in the PR description.
Since this PR implements a feature, please add
Implements #<issue-id>orAddresses #<issue-id>in the PR description for automatic tracking.As per coding guidelines, "Issue References (if PR fixes bugs or implements features): Suggest adding appropriate GitHub keywords ... 'Implements
#123' or 'Addresses#123' for features."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Features/TerrainBlending.cpp` at line 1, Update the PR description to include an issue-link keyword for tracking by adding a line like "Implements #<issue-id>" or "Addresses #<issue-id>" (replace <issue-id> with the actual GitHub issue number) so the feature implemented in TerrainBlending.cpp is automatically linked; ensure the keyword is placed in the PR description body (not just the title) so CI and release notes pick it up.src/Features/TerrainBlending.h (1)
1-1: PR metadata: add an issue-closing reference in the descriptionSince this PR implements a feature, add a closing/reference keyword in the PR body (for example:
Implements #<issue>orAddresses #<issue>).As per coding guidelines, "Issue References (if PR fixes bugs or implements features): Suggest adding appropriate GitHub keywords."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Features/TerrainBlending.h` at line 1, Update the PR description to include an issue-closing/reference keyword (e.g., "Implements #<issue>" or "Addresses #<issue>") so the feature implemented in this change (file: src/Features/TerrainBlending.h) is linked to the corresponding GitHub issue; edit the PR body to add the appropriate keyword and issue number in a single clear line (for example: Implements `#123`) and save the PR description.src/Feature.cpp (1)
40-40: PR metadata: consider a more specific title and issue linkageSuggested title:
feat(i18n): localize feature ui and categories
If this tracks a ticket, add a keyworded reference in the PR body, e.g.Implements #<id>(orCloses #<id>for a bug fix).As per coding guidelines "
**/*: When reviewing PRs, please provide suggestions for Conventional Commit Titles ... and Issue References ...".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Feature.cpp` at line 40, Update the PR metadata to use a more specific Conventional Commit title and add issue linkage: change the PR title to "feat(i18n): localize feature ui and categories" and add an issue reference in the PR body like "Implements #<id>" or "Closes #<id>" as appropriate; mention this change in the summary or description that touches src/Feature.cpp and the I18n include (I18n/I18n.h) so reviewers see the relation between the code change and the issue.src/Features/HairSpecular.cpp (1)
3-8: PR metadata polish: use a more specific conventional title and link an issueSuggested PR title:
feat(i18n): localize feature and editor ui
If this maps to a tracked task, addImplements #<id>(feature) orCloses #<id>(bugfix) in the PR body.As per coding guidelines, "When reviewing PRs, please provide suggestions for: 1. Conventional Commit Titles ... 2. Issue References ..."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Features/HairSpecular.cpp` around lines 3 - 8, Update the PR metadata: rename the PR title to "feat(i18n): localize feature and editor ui" and add an issue reference in the PR body (use "Implements #<id>" for tracked tasks or "Closes #<id>" for bug fixes); also update related commit messages touching HairSpecular.cpp (e.g. the change that added I18N_KEY_PREFIX) to match the conventional commit title so history and the PR are consistent.src/WeatherEditor/Weather/PrecipitationWidget.cpp (1)
1-3: PR metadata suggestions for this feature PRSuggested title:
feat(i18n): localize weather editor and feature ui
If this work maps to a tracking issue, add a reference likeImplements #<issue>(orAddresses #<issue>).As per coding guidelines
**/*: "When reviewing PRs, please provide suggestions for: 1. Conventional Commit Titles ... 2. Issue References ..."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/WeatherEditor/Weather/PrecipitationWidget.cpp` around lines 1 - 3, Update the pull request metadata to follow the repository's conventions: set the PR title to "feat(i18n): localize weather editor and feature ui" and, if applicable, add an issue reference like "Implements #<issue>" or "Addresses #<issue>" in the PR description; mention that this change affects localization in files such as PrecipitationWidget.cpp (and other Weather editor files) and include a short summary of the i18n work performed (e.g., localized strings in PrecipitationWidget, EditorWindow, and I18n module) in the PR body.src/WeatherEditor/WeatherUtils.h (1)
175-195: ⚡ Quick winHarden
TranslateWidgetTypeNamefallback against non-null-terminatedstring_view
src/WeatherEditor/WeatherUtils.hcurrently falls back withreturn widgetTypeName.data();. For an arbitrarystd::string_view,data()may not be null-terminated (andImGui/formatting code typically assumes C-string boundaries). Current callers passconst char*fromGetWidgetTypeName()implementations (string literals /""), so it’s safe today—but the helper’s contract is easy to misuse.💡 Proposed fix
-inline const char* TranslateWidgetTypeName(std::string_view widgetTypeName) +inline std::string_view TranslateWidgetTypeName(std::string_view widgetTypeName) { if (widgetTypeName == "Weather") return T("weather_editor.widget_type_weather", "Weather"); if (widgetTypeName == "ImageSpace") return T("weather_editor.widget_type_imagespace", "ImageSpace"); if (widgetTypeName == "Lighting") return T("weather_editor.widget_type_lighting", "Lighting"); if (widgetTypeName == "Cell Lighting") return T("weather_editor.widget_type_cell_lighting", "Cell Lighting"); if (widgetTypeName == "Volumetric Lighting") return T("weather_editor.widget_type_volumetric_lighting", "Volumetric Lighting"); if (widgetTypeName == "Precipitation") return T("weather_editor.widget_type_precipitation", "Precipitation"); if (widgetTypeName == "Lens Flare") return T("weather_editor.widget_type_lens_flare", "Lens Flare"); if (widgetTypeName == "Visual Effect") return T("weather_editor.widget_type_visual_effect", "Visual Effect"); - return widgetTypeName.data(); + return widgetTypeName; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/WeatherEditor/WeatherUtils.h` around lines 175 - 195, TranslateWidgetTypeName currently returns widgetTypeName.data() which may not be null-terminated; change the fallback so the returned const char* always points to a null-terminated buffer (e.g., convert widgetTypeName to a string stored with sufficient lifetime such as a static/thread_local std::string and return its c_str()). Update the fallback path in TranslateWidgetTypeName to perform this conversion and return the c_str() of that buffer instead of widgetTypeName.data(), leaving the existing mapped literal returns unchanged.src/WeatherEditor/Weather/VolumetricLightingWidget.cpp (1)
1-7: ⚡ Quick winUse a more specific conventional-commit PR title
Current title is valid, but it’s broad for the scope here. Suggested title:
feat(i18n): localize ui and add translation checks.
If this PR maps to an issue, consider addingImplements #<id>orCloses #<id>in the description.As per coding guidelines, "When reviewing PRs, please provide suggestions for: Conventional Commit Titles ... and Issue References ...".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/WeatherEditor/Weather/VolumetricLightingWidget.cpp` around lines 1 - 7, Update the PR title to follow Conventional Commits by using a more specific scope and description—e.g. change the broad title to "feat(i18n): localize ui and add translation checks"; also, if this work maps to an issue, add "Implements #<id>" or "Closes #<id>" in the PR description; refer to the change in src/WeatherEditor/Weather/VolumetricLightingWidget.cpp (see symbol I18N_KEY_PREFIX) so reviewers can easily correlate the title to the i18n work.src/Features/Upscaling.h (1)
1-1: ⚡ Quick winUse a more descriptive conventional commit title and link any tracking issue in the PR description.
Suggested title:
feat(i18n): localize feature and menu ui strings
If this PR maps to a tracker, addImplements #<id>(feature) orFixes #<id>(bug).As per coding guidelines "When reviewing PRs, please provide suggestions for: Conventional Commit Titles ... and Issue References ...".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Features/Upscaling.h` at line 1, PR title and description are not using a conventional commit format or linking an issue; update the PR title to a descriptive conventional commit like "feat(i18n): localize feature and menu ui strings" and, if this work maps to a tracker, add an issue reference in the PR description using "Implements #<id>" or "Fixes #<id>"; mention the change and file affected (src/Features/Upscaling.h) in the PR body so reviewers can quickly verify the localization scope.src/WeatherEditor/WeatherUtils.cpp (1)
111-241: ⚡ Quick winClarify/strengthen
WeatherUtils::TranslateControlLabellifetime contract (callers currently safe)
- Current usage doesn’t retain the returned
const char*:BuildLocalizedControlLabel(...)consumes it immediately to build and return an owningstd::string, and search dropdown entries initializeWidget::SearchResult::displayName(an owningstd::string) from the returned pointer.- The fallback
return label.data();is still a future API footgun: callers must not store/use the returned pointer beyond the call. Prefer returningstd::string_view/std::stringor document the “valid only for the duration of the call (unless translation returns a stable pointer)” contract to prevent misuse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/WeatherEditor/WeatherUtils.cpp` around lines 111 - 241, WeatherUtils::TranslateControlLabel currently returns a const char* that may point into a temporary (label.data()), risking dangling pointers; change TranslateControlLabel to return an owning std::string instead (update its signature and all callers such as BuildLocalizedControlLabel and places initializing Widget::SearchResult::displayName) so each branch constructs and returns an std::string (e.g. from the T(...) result or from std::string(label) for the fallback), ensuring callers can safely store the returned value.src/Features/WetnessEffects.cpp (1)
1-7: PR metadata suggestion: tighten commit title and add issue linkageCurrent PR title is valid but broad. Suggested title:
feat(i18n): localize feature and editor ui
If this maps to a tracker item, addImplements #<id>(orAddresses #<id>).As per coding guidelines, "When reviewing PRs, please provide suggestions for: 1. Conventional Commit Titles ... 2. Issue References ...".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Features/WetnessEffects.cpp` around lines 1 - 7, Update the PR title and description to follow conventional commits and include an issue link: change the commit/PR title to "feat(i18n): localize feature and editor ui" and, if this work maps to a tracker, add "Implements #<id>" (or "Addresses #<id>") in the PR description; reference the i18n key prefix I18N_KEY_PREFIX and affected modules/classes (WetnessEffects, I18n/I18n, Menu, WeatherEditor) in the PR body so reviewers know what was localized and where.src/WeatherEditor/EditorWindow.cpp (1)
1-3: PR metadata could be more specific for traceabilitySuggested title:
feat(i18n): localize weather editor ui and tooling
Also add issue linkage if applicable (e.g.,Closes #<id>/Addresses #<id>).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/WeatherEditor/EditorWindow.cpp` around lines 1 - 3, Update the PR metadata to be more specific: set the PR title to "feat(i18n): localize weather editor ui and tooling" and update the PR description to include a short summary of changes (e.g., localization added in EditorWindow.cpp and I18n module) and an issue linkage line such as "Closes #<id>" or "Addresses #<id>" if applicable so the change is traceable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pr-i18n.yaml:
- Around line 19-25: Pin the workflow actions to immutable commit SHAs and
disable credential persistence: replace the floating versions
actions/checkout@v4 and actions/setup-python@v5 with their corresponding commit
SHAs, and add persist-credentials: false under the actions/checkout step (ensure
the key name is exactly persist-credentials). Keep the python-version input for
setup-python but reference the SHA-pinned ref for the action itself so both
actions are pinned and checkout does not persist credentials.
- Around line 4-5: The workflow currently uses the pull_request_target trigger
and checks out/executes PR-head code (github.event.pull_request.head.sha) which
runs untrusted code in a privileged context; change the trigger from
pull_request_target to pull_request (or add an explicit trust gate before
checking out PR head), stop checking out the PR head SHA directly, and ensure
the checkout step (actions/checkout) sets persist-credentials: false and both
actions/checkout and actions/setup-python are pinned to specific commit SHAs
(not floating tags) while keeping the i18n validation steps (python
tools/extract-i18n.py and the inline python -c validator) only executed after
using a safe, trusted reference.
In `@src/Features/ExtendedTranslucency.cpp`:
- Around line 105-137: Fix typos in the fallback English tooltip strings used in
the UI text calls around the ImGui::Text/T(TKEY(...)) blocks: correct
"transluency" to "translucency" and "Transluent" to "Translucent" in the
default_material_model_tooltip and transparency_increase_tooltip, change "your
are" to "you are" in skinned_mesh_only_tooltip (used with settings.SkinnedOnly),
and fix awkward phrasing in softness_tooltip and blend_weight_tooltip to read
clearly (e.g., "Control the softness of the alpha increase; increasing softness
reduces the amount of increased alpha." and "Control the blend weight of the
effect applied to the final result."). Locate these strings attached to
TKEY("default_material_model_tooltip"), TKEY("skinned_mesh_only_tooltip"),
TKEY("transparency_increase_tooltip"), TKEY("softness_tooltip"), and
TKEY("blend_weight_tooltip") and update the fallback text accordingly.
In `@src/Features/InverseSquareLighting/LightEditor.cpp`:
- Line 82: Replace the misspelled translation key TKEY("ligh") used in the
ImGui::Text call with the correct key TKEY("light") so the label uses the
canonical translation token; update the string in the ImGui::Text invocation
that includes displayInfo.lighFormId and displayInfo.lighEditorId (the line with
ImGui::Text(T(TKEY("ligh"), "LIGH: 0x%08X | %s"), ...)) to use TKEY("light")
instead.
In `@src/Features/LightLimitFix.cpp`:
- Around line 37-38: The combo uses hardcoded English strings (comboOptions) so
the entries won't translate; replace the static const char* comboOptions[] with
an array of localized strings (e.g., build entries via T(TKEY(...)) or call the
translation helper for "Light Limit", "Strict Lights Count", "Clustered Lights
Count", "Shadow Mask") and pass those localized C-strings or std::string.c_str()
to ImGui::Combo (the call around ImGui::Combo, comboOptions, and
settings.LightsVisualisationMode should be updated to use the localized labels).
In `@src/Features/LightLimitFix.h`:
- Line 22: The fallback text for the fifth bullet in the LightLimitFix feature
is a duplicate of the fourth; update the
T("feature.light_limit_fix.key_feature_5", "...") entry to a distinct English
fallback string so users don't see repeated bullets. Locate the
array/initializer that defines the key_feature_* entries in LightLimitFix
(search for T("feature.light_limit_fix.key_feature_1" through key_feature_5) or
the symbol key_feature_5) and replace the repeated "Enhanced visual realism"
value with a unique, concise fifth bullet that complements the other four.
In `@src/Features/RenderDoc.cpp`:
- Line 231: The fallback string passed to ImGui::Text uses '{}' (Python-style)
but ImGui::Text is printf-style, so the value from
GetRequiredCaptureSpaceBytes() won't render; update the call in RenderDoc.cpp to
use a printf placeholder like "%llu" (or "%zu" if using size_t) and cast the
expression accordingly, e.g. change "At least {} MB of free space is required."
to "At least %llu MB of free space is required." and pass (unsigned long
long)(GetRequiredCaptureSpaceBytes() / (1024 * 1024)) as the argument so the
required MB prints correctly.
In `@src/Features/VRStereoOptimizations.cpp`:
- Around line 274-275: The UI slider bounds for ForwardOcclusionScale in
DrawSettings() (ImGui::SliderFloat controlling settings.forwardOcclusionScale)
currently use 0.0f..1.0f while LoadSettings() accepts 0.0f..10.0f; update the
slider to match the loader by changing the max from 1.0f to 10.0f
(ImGui::SliderFloat(..., 0.0f, 10.0f, ...)) and adjust the tooltip text if
needed to reflect the wider range so users can adjust any value that
LoadSettings() may load.
In `@src/Features/WetnessEffects.cpp`:
- Line 277: Fix the typo in the localized fallback string used in the T(...)
call for the key "climate_monsoon_detail_4" in WetnessEffects.cpp: update the
literal "Skryim light rain will not match wetness." to "Skyrim light rain will
not match wetness." so the T(TKEY("climate_monsoon_detail_4"), "...") entry
contains the correct spelling and avoids propagating the mistake into en.json
and translations.
In `@src/Menu/AdvancedSettingsRenderer.cpp`:
- Around line 117-124: The SliderInt calls use
std::thread::hardware_concurrency() directly which can be 0; compute a clamped
max (e.g., int32_t maxThreads = static_cast<int32_t>(std::max<size_t>(1,
std::thread::hardware_concurrency()))) and use that max for both
ImGui::SliderInt calls so the range passed to the sliders is always at least 1;
update the lines referencing shaderCache->compilationThreadCount and
shaderCache->backgroundCompilationThreadCount to use this clamped max value.
In `@src/Menu/FeatureListRenderer.cpp`:
- Around line 30-39: GetCoreMenuNames() currently stores raw const char* from
T(...) in a static array which can dangle after locale changes; change
GetCoreMenuNames() to return owned strings (e.g., std::array<std::string,4>) or
return the i18n keys (std::array<const char*,4>) and call I18n::Get() at render
time so no static pointer to I18n storage is cached; update IsCoreMenu() to
compare against those owned strings or to compare keys via I18n::Get() per-call
instead of comparing cached const char* values so lookups remain safe across
SetLocale()/Reload().
In `@src/Menu/ThemeManager.cpp`:
- Around line 65-81: Normalize locale tags before performing CJK matching:
create or use a normalized string (lowercased and with '_' replaced by '-') at
the start of the locale-checking functions (e.g., IsSimplifiedChineseLocale and
IsTraditionalChineseLocale) and update the comparisons to use that normalized
value; change exact matches and mixed separators to checks like normalized ==
"ja" || normalized.starts_with("ja-"), normalized == "ko" ||
normalized.starts_with("ko-"), normalized.starts_with("zh-hans"), and
normalized.starts_with("zh-hant") so region-tagged locales (e.g., "ja-JP",
"ko-KR", "zh-HK", "zh_CN") are correctly matched.
- Around line 487-490: The temporary SupplementalGlyphMerge instance's
ImVector<ImWchar> (merge.glyphRanges) is destroyed before io.Fonts->Build(),
leaving io.Fonts->AddFontFromFileTTF with a dangling glyphRanges pointer; fix by
ensuring the backing storage outlives the atlas build — e.g., move the
supplementalGlyphMerges container (or the storage for merge.glyphRanges) to an
outer scope so entries pushed into supplementalGlyphMerges persist until after
io.Fonts->Build(), or alternatively perform io.Fonts->Build() inside the same
scope before the temporary merge is destroyed; update references to
SupplementalGlyphMerge, merge.glyphRanges, supplementalGlyphMerges,
io.Fonts->AddFontFromFileTTF and io.Fonts->Build() accordingly.
In `@src/WeatherEditor/EditorWindow.cpp`:
- Line 123: kFilterColumnNames currently caches pointers returned by T(...) at
namespace scope which become stale after a locale change; replace the global
const char* array with a function (e.g., GetFilterColumnNames or
FilterColumnNames()) that builds and returns the translated strings on demand
(or returns a std::vector<std::string>/std::vector<const char*> populated by
calling T(TKEY(...)) at each call), and update usages (combo/model population
code) to call that function whenever labels are needed or when locale changes so
labels are refreshed dynamically; reference symbols: kFilterColumnNames, T,
TKEY, and any combo/model population code in EditorWindow.cpp.
In `@src/WeatherEditor/Weather/ReferenceEffectWidget.cpp`:
- Around line 34-37: The code currently selects displayLabel by comparing
C-string pointers (label == ReferenceEffectSetting::kArtObject), which is
fragile; change that to a value comparison instead (e.g., use std::strcmp(...)
== 0 or compare std::string_view(label) ==
std::string_view(ReferenceEffectSetting::kArtObject)) so the dispatch that sets
displayLabel and then builds controlLabel uses string content equality rather
than pointer identity; update the conditional that assigns displayLabel (and
keep controlLabel = std::format("{}##{}", displayLabel, label) unchanged).
In `@src/WeatherEditor/Weather/WeatherWidget.cpp`:
- Around line 75-105: TranslateWeatherPropertyLabel returns label.data() which
can dangle or lack null-termination; change the fallback to return a stable
owned C string by copying label into a thread-local or static std::string and
returning its c_str(). Update the function TranslateWeatherPropertyLabel to
create a thread_local std::string fallback (or similar owned buffer) initialized
from label when no translation is found, then return fallback.c_str() so the
pointer remains valid and null-terminated.
In `@src/WeatherEditor/Widget.cpp`:
- Line 531: The ImGui::TextDisabled call uses "%s" so the "%zu" placeholder
inside the localized text from T(TKEY("more_results"), "... %zu more results")
is never substituted; fix by formatting the localized string with the actual
count first (e.g., using fmt::format or snprintf) and then pass the resulting
std::string.c_str() to ImGui::TextDisabled; specifically, compute count =
searchResults.size() - WidgetUI::kSearchDropdownMaxResults, produce a formatted
string from T/TKEY with that count, and call ImGui::TextDisabled("%s",
formatted.c_str()).
In `@TRANSLATING.md`:
- Around line 106-114: The fenced code block containing the menu.<page>.<item>
... weather_editor.<item> lines lacks a language tag, triggering MD040; update
the opening fence to include a language identifier (for example, use ```text) so
the block becomes a labelled code fence and satisfies the linter.
---
Outside diff comments:
In `@src/Features/TerrainShadows.cpp`:
- Around line 267-285: Precompute(): ensure texShadowHeight is valid before
clearing bindings and flipping needPrecompute—either attempt to create/restore
the shadow texture inside TerrainShadows::Precompute (create texShadowHeight and
its UAV/SRV) or, if creation fails, leave needPrecompute = true and log the
failure so UpdateShadow() isn't run with a null resource; also add a null guard
at the start of TerrainShadows::UpdateShadow() that early-returns if
texShadowHeight or texShadowHeight->uav is null to prevent crashes (references:
Precompute(), UpdateShadow(), texShadowHeight, needPrecompute).
In `@src/Features/WeatherEditor.cpp`:
- Around line 1006-1035: GetWeatherFlagNames currently returns localized labels
which breaks downstream logic that expects canonical tokens; change it to emit
canonical identifiers (use magic_enum::enum_name(flagValue) trimmed of leading
'k' or a stable key like the enum value) and keep localization only for
rendering (e.g., add/GetWeatherFlagLabel or return pairs {id,label} so UI uses
label but filtering/color logic uses id). Update usages that reference
GetWeatherFlagNames, chip filtering and color mapping to use the canonical id
(from magic_enum/flagValue) for logic and use the localized label only for
display.
In `@src/Utils/UI.h`:
- Around line 494-500: Update the function header comment for DrawCategoryHeader
to match the new signature: replace references to the old categoryName with both
categoryKey and displayName, describing categoryKey as the internal/unique key
or identifier and displayName as the visible label shown in the UI; document
isExpanded as a reference boolean representing the current expansion state that
may be toggled, categoryCount as the number of features in the category, and
state the return value indicates whether the expansion state was toggled (true
if toggled). Ensure parameter names (categoryKey, displayName, isExpanded,
categoryCount) and the return behavior are explicitly documented in the comment
block.
In `@src/WeatherEditor/EditorWindow.cpp`:
- Line 427: The combo width calculation uses the hardcoded literal "Editor ID"
which won't account for translations; update the calculation for comboW to use
the actual localized label string used for the combo (i.e., pass the same
translated label variable or call that localization function instead of the
literal) when calling ImGui::CalcTextSize and computing width so longer locales
won't be clipped; reference the existing comboW variable and
ImGui::CalcTextSize(...) call to locate and replace the literal.
In `@src/WeatherEditor/Weather/WeatherWidget.cpp`:
- Around line 1822-1835: Tree node labels use the user-visible displayName which
can collide when localized; change the ImGui::TreeNodeEx call to use a stable
unique ID by appending "##" + featureName to the label (keep displayName for
visible text but supply displayName + "##" + featureName as the ImGui label).
Update the call site where TreeNodeEx(displayName.c_str(),
ImGuiTreeNodeFlags_SpanAvailWidth) is used (near feature->GetDisplayName(),
featureName, pendingFeatureNavigation and getFeatureJson) so the tree node ID is
unique while preserving the displayed text.
---
Duplicate comments:
In `@src/I18n/I18n.h`:
- Around line 3-10: The header uses std::shared_mutex (and possibly
std::shared_lock) but doesn't include the header; add the missing include by
inserting `#include` <shared_mutex> into the include list in I18n.h so the
declaration of std::shared_mutex is available and the header is self-contained
(referencing std::shared_mutex and any related types used in the I18n class).
In `@tools/extract-i18n.py`:
- Around line 241-245: The add_string function currently writes defaults into
strings but doesn't remove the key from key_only, causing false key-only
reports; update add_string (the def add_string(key, default, source_path) block)
so that when a key is added/updated with a non-empty default you also discard it
from key_only (e.g., key_only.discard(key) or similar) and keep the existing
conflict handling that appends to conflicts and sets strings[key] = default.
---
Nitpick comments:
In `@src/Feature.cpp`:
- Line 40: Update the PR metadata to use a more specific Conventional Commit
title and add issue linkage: change the PR title to "feat(i18n): localize
feature ui and categories" and add an issue reference in the PR body like
"Implements #<id>" or "Closes #<id>" as appropriate; mention this change in the
summary or description that touches src/Feature.cpp and the I18n include
(I18n/I18n.h) so reviewers see the relation between the code change and the
issue.
In `@src/Features/HairSpecular.cpp`:
- Around line 3-8: Update the PR metadata: rename the PR title to "feat(i18n):
localize feature and editor ui" and add an issue reference in the PR body (use
"Implements #<id>" for tracked tasks or "Closes #<id>" for bug fixes); also
update related commit messages touching HairSpecular.cpp (e.g. the change that
added I18N_KEY_PREFIX) to match the conventional commit title so history and the
PR are consistent.
In `@src/Features/HDRDisplay.h`:
- Around line 19-38: Update the PR title and description to follow the
conventional commit style and include any tracking issue reference: change the
PR title to a more specific conventional commit such as "feat(i18n): localize
feature labels and summaries" and add "Implements #<id>" or "Addresses #<id>" in
the PR description if a tracking issue exists; reference the HDR feature
identifiers found in this diff (e.g., GetDisplayName, GetShortName,
GetFeatureSummary and the i18n keys like "feature.hdr_display.*") so reviewers
can easily correlate the commit to the localized strings being added.
In `@src/Features/ScreenshotFeature.h`:
- Line 17: Add an issue reference keyword to the PR description for
traceability: update the PR body to include a line like "Implements
#<issue-number>" or "Addresses #<issue-number>" referencing the related issue
for this feature (ScreenshotFeature / GetDisplayName) so reviewers can link the
implemented feature to its tracking issue.
In `@src/Features/TerrainBlending.cpp`:
- Line 1: Update the PR description to include an issue-link keyword for
tracking by adding a line like "Implements #<issue-id>" or "Addresses
#<issue-id>" (replace <issue-id> with the actual GitHub issue number) so the
feature implemented in TerrainBlending.cpp is automatically linked; ensure the
keyword is placed in the PR description body (not just the title) so CI and
release notes pick it up.
In `@src/Features/TerrainBlending.h`:
- Line 1: Update the PR description to include an issue-closing/reference
keyword (e.g., "Implements #<issue>" or "Addresses #<issue>") so the feature
implemented in this change (file: src/Features/TerrainBlending.h) is linked to
the corresponding GitHub issue; edit the PR body to add the appropriate keyword
and issue number in a single clear line (for example: Implements `#123`) and save
the PR description.
In `@src/Features/Upscaling.h`:
- Line 1: PR title and description are not using a conventional commit format or
linking an issue; update the PR title to a descriptive conventional commit like
"feat(i18n): localize feature and menu ui strings" and, if this work maps to a
tracker, add an issue reference in the PR description using "Implements #<id>"
or "Fixes #<id>"; mention the change and file affected
(src/Features/Upscaling.h) in the PR body so reviewers can quickly verify the
localization scope.
In `@src/Features/WetnessEffects.cpp`:
- Around line 1-7: Update the PR title and description to follow conventional
commits and include an issue link: change the commit/PR title to "feat(i18n):
localize feature and editor ui" and, if this work maps to a tracker, add
"Implements #<id>" (or "Addresses #<id>") in the PR description; reference the
i18n key prefix I18N_KEY_PREFIX and affected modules/classes (WetnessEffects,
I18n/I18n, Menu, WeatherEditor) in the PR body so reviewers know what was
localized and where.
In `@src/WeatherEditor/EditorWindow.cpp`:
- Around line 1-3: Update the PR metadata to be more specific: set the PR title
to "feat(i18n): localize weather editor ui and tooling" and update the PR
description to include a short summary of changes (e.g., localization added in
EditorWindow.cpp and I18n module) and an issue linkage line such as "Closes
#<id>" or "Addresses #<id>" if applicable so the change is traceable.
In `@src/WeatherEditor/Weather/PrecipitationWidget.cpp`:
- Around line 1-3: Update the pull request metadata to follow the repository's
conventions: set the PR title to "feat(i18n): localize weather editor and
feature ui" and, if applicable, add an issue reference like "Implements
#<issue>" or "Addresses #<issue>" in the PR description; mention that this
change affects localization in files such as PrecipitationWidget.cpp (and other
Weather editor files) and include a short summary of the i18n work performed
(e.g., localized strings in PrecipitationWidget, EditorWindow, and I18n module)
in the PR body.
In `@src/WeatherEditor/Weather/VolumetricLightingWidget.cpp`:
- Around line 1-7: Update the PR title to follow Conventional Commits by using a
more specific scope and description—e.g. change the broad title to "feat(i18n):
localize ui and add translation checks"; also, if this work maps to an issue,
add "Implements #<id>" or "Closes #<id>" in the PR description; refer to the
change in src/WeatherEditor/Weather/VolumetricLightingWidget.cpp (see symbol
I18N_KEY_PREFIX) so reviewers can easily correlate the title to the i18n work.
In `@src/WeatherEditor/WeatherUtils.cpp`:
- Around line 111-241: WeatherUtils::TranslateControlLabel currently returns a
const char* that may point into a temporary (label.data()), risking dangling
pointers; change TranslateControlLabel to return an owning std::string instead
(update its signature and all callers such as BuildLocalizedControlLabel and
places initializing Widget::SearchResult::displayName) so each branch constructs
and returns an std::string (e.g. from the T(...) result or from
std::string(label) for the fallback), ensuring callers can safely store the
returned value.
In `@src/WeatherEditor/WeatherUtils.h`:
- Around line 175-195: TranslateWidgetTypeName currently returns
widgetTypeName.data() which may not be null-terminated; change the fallback so
the returned const char* always points to a null-terminated buffer (e.g.,
convert widgetTypeName to a string stored with sufficient lifetime such as a
static/thread_local std::string and return its c_str()). Update the fallback
path in TranslateWidgetTypeName to perform this conversion and return the
c_str() of that buffer instead of widgetTypeName.data(), leaving the existing
mapped literal returns unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fbbe62c0-c937-4b1b-9a13-d09222e0b98d
📒 Files selected for processing (104)
.github/workflows/pr-i18n.yamlTRANSLATING.mdpackage/SKSE/Plugins/CommunityShaders/Translations/en.jsonpackage/SKSE/Plugins/CommunityShaders/Translations/zh_CN.jsonsrc/Feature.cppsrc/Feature.hsrc/FeatureIssues.cppsrc/Features/CloudShadows.cppsrc/Features/CloudShadows.hsrc/Features/DynamicCubemaps.cppsrc/Features/DynamicCubemaps.hsrc/Features/ExponentialHeightFog.cppsrc/Features/ExponentialHeightFog.hsrc/Features/ExtendedMaterials.cppsrc/Features/ExtendedMaterials.hsrc/Features/ExtendedTranslucency.cppsrc/Features/ExtendedTranslucency.hsrc/Features/GrassCollision.cppsrc/Features/GrassCollision.hsrc/Features/GrassLighting.hsrc/Features/HDRDisplay.cppsrc/Features/HDRDisplay.hsrc/Features/HairSpecular.cppsrc/Features/HairSpecular.hsrc/Features/IBL.cppsrc/Features/IBL.hsrc/Features/InteriorSun.cppsrc/Features/InteriorSun.hsrc/Features/InverseSquareLighting.hsrc/Features/InverseSquareLighting/LightEditor.cppsrc/Features/LODBlending.cppsrc/Features/LODBlending.hsrc/Features/LightLimitFix.cppsrc/Features/LightLimitFix.hsrc/Features/LinearLighting.cppsrc/Features/LinearLighting.hsrc/Features/PerformanceOverlay.cppsrc/Features/PerformanceOverlay.hsrc/Features/RenderDoc.cppsrc/Features/RenderDoc.hsrc/Features/ScreenSpaceGI.cppsrc/Features/ScreenSpaceGI.hsrc/Features/ScreenSpaceShadows.cppsrc/Features/ScreenSpaceShadows.hsrc/Features/ScreenshotFeature.cppsrc/Features/ScreenshotFeature.hsrc/Features/SkySync.cppsrc/Features/SkySync.hsrc/Features/Skylighting.cppsrc/Features/Skylighting.hsrc/Features/SubsurfaceScattering.cppsrc/Features/SubsurfaceScattering.hsrc/Features/TerrainBlending.cppsrc/Features/TerrainBlending.hsrc/Features/TerrainHelper.hsrc/Features/TerrainShadows.cppsrc/Features/TerrainShadows.hsrc/Features/TerrainVariation.cppsrc/Features/TerrainVariation.hsrc/Features/UnifiedWater.cppsrc/Features/UnifiedWater.hsrc/Features/Upscaling.cppsrc/Features/Upscaling.hsrc/Features/VR.hsrc/Features/VRStereoOptimizations.cppsrc/Features/VolumetricLighting.cppsrc/Features/VolumetricLighting.hsrc/Features/VolumetricShadows.hsrc/Features/WaterEffects.hsrc/Features/WeatherEditor.cppsrc/Features/WeatherEditor.hsrc/Features/WetnessEffects.cppsrc/Features/WetnessEffects.hsrc/I18n/I18n.cppsrc/I18n/I18n.hsrc/Menu.cppsrc/Menu/AdvancedSettingsRenderer.cppsrc/Menu/FeatureListRenderer.cppsrc/Menu/Fonts.cppsrc/Menu/MenuHeaderRenderer.cppsrc/Menu/OverlayRenderer.cppsrc/Menu/SettingsTabRenderer.cppsrc/Menu/ThemeManager.cppsrc/State.cppsrc/TruePBR.cppsrc/TruePBR.hsrc/Utils/GameSetting.cppsrc/Utils/Subrect.cppsrc/Utils/UI.cppsrc/Utils/UI.hsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/InteriorOnlyPanel.cppsrc/WeatherEditor/PaletteWindow.cppsrc/WeatherEditor/Weather/CellLightingWidget.cppsrc/WeatherEditor/Weather/LightingTemplateWidget.cppsrc/WeatherEditor/Weather/PrecipitationWidget.cppsrc/WeatherEditor/Weather/ReferenceEffectWidget.cppsrc/WeatherEditor/Weather/SimpleFormWidget.hsrc/WeatherEditor/Weather/VolumetricLightingWidget.cppsrc/WeatherEditor/Weather/WeatherWidget.cppsrc/WeatherEditor/WeatherUtils.cppsrc/WeatherEditor/WeatherUtils.hsrc/WeatherEditor/Widget.cpptools/extract-i18n.py
✅ Files skipped from review due to trivial changes (6)
- src/Utils/Subrect.cpp
- src/Features/Skylighting.cpp
- src/Features/VolumetricShadows.h
- src/Menu/OverlayRenderer.cpp
- src/Features/PerformanceOverlay.cpp
- src/Features/TerrainVariation.h
Comment on lines
+34
to
+37
| const char* displayLabel = label == ReferenceEffectSetting::kArtObject ? | ||
| T(TKEY("art_object"), "Art Object") : | ||
| T(TKEY("effect_shader"), "Effect Shader"); | ||
| const auto controlLabel = std::format("{}##{}", displayLabel, label); |
Contributor
There was a problem hiding this comment.
Use value comparison, not pointer comparison, for setting label dispatch
Line 34 compares C-string pointers, which is fragile if label isn’t the same storage instance. Compare string values instead.
Suggested fix
- const char* displayLabel = label == ReferenceEffectSetting::kArtObject ?
- T(TKEY("art_object"), "Art Object") :
- T(TKEY("effect_shader"), "Effect Shader");
+ const bool isArtObject = std::string_view(label) == ReferenceEffectSetting::kArtObject;
+ const char* displayLabel = isArtObject ?
+ T(TKEY("art_object"), "Art Object") :
+ T(TKEY("effect_shader"), "Effect Shader");📝 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.
Suggested change
| const char* displayLabel = label == ReferenceEffectSetting::kArtObject ? | |
| T(TKEY("art_object"), "Art Object") : | |
| T(TKEY("effect_shader"), "Effect Shader"); | |
| const auto controlLabel = std::format("{}##{}", displayLabel, label); | |
| const bool isArtObject = std::string_view(label) == ReferenceEffectSetting::kArtObject; | |
| const char* displayLabel = isArtObject ? | |
| T(TKEY("art_object"), "Art Object") : | |
| T(TKEY("effect_shader"), "Effect Shader"); | |
| const auto controlLabel = std::format("{}##{}", displayLabel, label); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/WeatherEditor/Weather/ReferenceEffectWidget.cpp` around lines 34 - 37,
The code currently selects displayLabel by comparing C-string pointers (label ==
ReferenceEffectSetting::kArtObject), which is fragile; change that to a value
comparison instead (e.g., use std::strcmp(...) == 0 or compare
std::string_view(label) == std::string_view(ReferenceEffectSetting::kArtObject))
so the dispatch that sets displayLabel and then builds controlLabel uses string
content equality rather than pointer identity; update the conditional that
assigns displayLabel (and keep controlLabel = std::format("{}##{}",
displayLabel, label) unchanged).
| if (searchResults.size() > WidgetUI::kSearchDropdownMaxResults) { | ||
| ImGui::Separator(); | ||
| ImGui::TextDisabled("... %zu more results", searchResults.size() - WidgetUI::kSearchDropdownMaxResults); | ||
| ImGui::TextDisabled("%s", T(TKEY("more_results"), "... %zu more results"), searchResults.size() - WidgetUI::kSearchDropdownMaxResults); |
Contributor
There was a problem hiding this comment.
Fix hidden “more results” count formatting
The call uses "%s" as the format string, so the %zu in the localized text is never applied.
Suggested fix
- ImGui::TextDisabled("%s", T(TKEY("more_results"), "... %zu more results"), searchResults.size() - WidgetUI::kSearchDropdownMaxResults);
+ ImGui::TextDisabled(T(TKEY("more_results"), "... %zu more results"), searchResults.size() - WidgetUI::kSearchDropdownMaxResults);📝 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.
Suggested change
| ImGui::TextDisabled("%s", T(TKEY("more_results"), "... %zu more results"), searchResults.size() - WidgetUI::kSearchDropdownMaxResults); | |
| ImGui::TextDisabled(T(TKEY("more_results"), "... %zu more results"), searchResults.size() - WidgetUI::kSearchDropdownMaxResults); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/WeatherEditor/Widget.cpp` at line 531, The ImGui::TextDisabled call uses
"%s" so the "%zu" placeholder inside the localized text from
T(TKEY("more_results"), "... %zu more results") is never substituted; fix by
formatting the localized string with the actual count first (e.g., using
fmt::format or snprintf) and then pass the resulting std::string.c_str() to
ImGui::TextDisabled; specifically, compute count = searchResults.size() -
WidgetUI::kSearchDropdownMaxResults, produce a formatted string from T/TKEY with
that count, and call ImGui::TextDisabled("%s", formatted.c_str()).
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/extract-i18n.py (1)
289-296:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEscape sequence order causes incorrect handling of
\\nand similar patterns.The sequential
replace()calls don't correctly handle overlapping escape sequences. For example, C++ source"Hello\\nWorld"(which should becomeHello\nWorldwith a literal backslash) gets incorrectly processed:
replace("\\n", "\n")matches the\nportion, producingHello\<newline>World- The leading
\is orphanedA single-pass approach is needed to process escape sequences correctly.
Proposed fix using single-pass regex substitution
def unescape_cpp_string(s: str) -> str: """Unescape C++ string literal escape sequences.""" - s = s.replace("\\n", "\n") - s = s.replace("\\t", "\t") - s = s.replace('\\"', '"') - s = s.replace("\\\\", "\\") - s = re.sub(r"\\x([0-9a-fA-F]{2})", lambda m: chr(int(m.group(1), 16)), s) + def replacer(m): + esc = m.group(0) + if esc == "\\n": + return "\n" + elif esc == "\\t": + return "\t" + elif esc == '\\"': + return '"' + elif esc == "\\\\": + return "\\" + elif esc.startswith("\\x"): + return chr(int(esc[2:], 16)) + return esc + s = re.sub(r'\\\\|\\n|\\t|\\"|\\x[0-9a-fA-F]{2}', replacer, s) return s🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/extract-i18n.py` around lines 289 - 296, The current unescape_cpp_string function does sequential replaces which mis-handle overlapping escapes; change unescape_cpp_string to perform a single-pass regex substitution that matches a backslash plus the escape payload and maps each case (e.g., "n", "t", "\"", "\\", "r", "b", "f", "v", octal, and "xHH") to the correct character; implement this by using re.sub with a pattern like r'\\(x[0-9A-Fa-f]{2}|[0-7]{1,3}|.)' and a replacement function that decodes "xHH" using int(...,16), octal with int(...,8), and a dictionary for one-char escapes so that sequences like "\\n" and "\\\\n" are handled correctly in one pass inside unescape_cpp_string.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tools/extract-i18n.py`:
- Around line 289-296: The current unescape_cpp_string function does sequential
replaces which mis-handle overlapping escapes; change unescape_cpp_string to
perform a single-pass regex substitution that matches a backslash plus the
escape payload and maps each case (e.g., "n", "t", "\"", "\\", "r", "b", "f",
"v", octal, and "xHH") to the correct character; implement this by using re.sub
with a pattern like r'\\(x[0-9A-Fa-f]{2}|[0-7]{1,3}|.)' and a replacement
function that decodes "xHH" using int(...,16), octal with int(...,8), and a
dictionary for one-char escapes so that sequences like "\\n" and "\\\\n" are
handled correctly in one pass inside unescape_cpp_string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: edf47629-f78e-4fff-87da-db25aa3e9784
📒 Files selected for processing (3)
src/I18n/I18n.cppsrc/Menu/SettingsTabRenderer.cpptools/extract-i18n.py
Currently translated at 99.7% (2016 of 2022 strings) Translation: Community Shaders/Community Shaders UI Translate-URL: https://hosted.weblate.org/projects/community-shaders/community-shaders-ui/zh_Hans/
Currently translated at 99.7% (2016 of 2022 strings) Translation: Community Shaders/Community Shaders UI Translate-URL: https://hosted.weblate.org/projects/community-shaders/community-shaders-ui/zh_Hans/
…ty-shaders-ui Translations update from Hosted Weblate
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Features/TerrainShadows.cpp`:
- Line 311: The `#undef` I18N_KEY_PREFIX is currently inside the Precompute()
function; move it to file scope by removing the `#undef` from inside Precompute()
and adding a single line `#undef` I18N_KEY_PREFIX at the very end of the file
(after all function definitions) to match the pattern used in UnifiedWater.cpp
and keep preprocessor directives at file boundaries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b2ac7cfb-92da-42bc-9283-a863421e98fc
📒 Files selected for processing (14)
.github/workflows/pr-i18n.yamlpackage/SKSE/Plugins/CommunityShaders/Translations/en.jsonpackage/SKSE/Plugins/CommunityShaders/Translations/zh_CN.jsonsrc/Features/ExtendedTranslucency.cppsrc/Features/TerrainShadows.cppsrc/Features/UnifiedWater.cppsrc/Features/UnifiedWater.hsrc/Features/WeatherEditor.cppsrc/I18n/I18n.hsrc/Menu/FeatureListRenderer.cppsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/Weather/WeatherWidget.cppsrc/WeatherEditor/WeatherUtils.cppsrc/WeatherEditor/WeatherUtils.h
🚧 Files skipped from review as they are similar to previous changes (10)
- src/Features/UnifiedWater.h
- .github/workflows/pr-i18n.yaml
- src/Features/ExtendedTranslucency.cpp
- src/I18n/I18n.h
- src/WeatherEditor/WeatherUtils.h
- src/WeatherEditor/EditorWindow.cpp
- src/Menu/FeatureListRenderer.cpp
- src/WeatherEditor/WeatherUtils.cpp
- src/Features/WeatherEditor.cpp
- src/WeatherEditor/Weather/WeatherWidget.cpp
| texShadowHeight->CreateSRV(srvDesc); | ||
| texShadowHeight->CreateUAV(uavDesc); | ||
| } | ||
| #undef I18N_KEY_PREFIX |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Move #undef I18N_KEY_PREFIX to end of file for consistency.
The #undef directive is placed inside the Precompute() function, which is unconventional and inconsistent with the pattern established in UnifiedWater.cpp (line 563). Preprocessor directives should be at file scope boundaries for clarity and maintainability.
♻️ Move `#undef` to end of file
Remove the #undef from line 311 and place it at the very end of the file after all function definitions, matching the pattern in UnifiedWater.cpp.
}
-#undef I18N_KEY_PREFIX
needPrecompute = false;
}
void TerrainShadows::UpdateShadow()Then add at the end of the file:
+#undef I18N_KEY_PREFIX🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Features/TerrainShadows.cpp` at line 311, The `#undef` I18N_KEY_PREFIX is
currently inside the Precompute() function; move it to file scope by removing
the `#undef` from inside Precompute() and adding a single line `#undef`
I18N_KEY_PREFIX at the very end of the file (after all function definitions) to
match the pattern used in UnifiedWater.cpp and keep preprocessor directives at
file boundaries.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a comprehensive internationalization (i18n) system for Community Shaders, enabling multi-language support and ensuring translation consistency. It adds a translation contribution guide, a CI workflow for validating translations, and refactors UI code to use translatable strings. The most important changes are grouped below:
Internationalization Infrastructure:
.github/workflows/pr-i18n.yamlto automatically check thaten.jsonis up to date, detect orphaned keys, and validate translation file formats and placeholders on pull requests.TRANSLATING.mdwith clear instructions for translators and developers, including translation rules, file format, and contribution methods via Weblate or direct PR.Codebase Refactoring for i18n:
src/Feature.cpp,src/Feature.h, andsrc/FeatureIssues.cppto use theT()translation macro for all user-facing strings, replacing hardcoded English text throughout the feature and issues UI. This ensures all UI text can be translated. [1] [2] [3] [4] [5] [6] [7] [8] [9]New API and Helper Methods:
Feature::GetDisplayCategory()to provide a translatable display name for feature categories, using the i18n system. [1] [2]These changes together establish a robust translation workflow, improve accessibility for non-English users, and ensure future UI strings are easily localizable.
Summary by CodeRabbit
New Features
Chores
Documentation
Content