Try adding translations to the screen #373
Conversation
|
Warning Rate limit exceeded@ManfredRichthofen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds runtime localization: new Translation module with language enums, tables and API; persists language in Settings (DEFAULT_LANGUAGE fallback) and exposes set/get; new LanguagePlugin to change/persist language and emit UI refresh; DefaultUI initializes Translation and replaces literals with TR(...) keys; docs added for translations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Controller
participant S as Settings
participant UI as DefaultUI
participant T as Translation
C->>S: load()
note right of S #DDDDFF: read "lang" (DEFAULT_LANGUAGE fallback)
C->>UI: construct DefaultUI(controller)
UI->>S: getLanguage()
UI->>T: setLanguage(savedLang)
T-->>UI: ack
sequenceDiagram
autonumber
participant U as User
participant EB as EventBus
participant LP as LanguagePlugin
participant T as Translation
participant S as Settings
participant LT as LoopTask
participant UI as UI
U->>EB: emit("language:change", langId)
EB-->>LP: event(langId)
LP->>T: setLanguage((Language)langId)
LP->>S: setLanguage(langId)
note right of S #FFF4DD: schedules save()
LT->>S: periodic flush (~5s)
S-->>LT: doSave() writes "lang"
LP->>EB: emit("ui:refresh")
EB-->>UI: refresh
sequenceDiagram
autonumber
participant UI as View
participant T as Translation
participant EN as EnglishTable
participant DE as GermanTable
participant FR as FrenchTable
participant ES as SpanishTable
UI->>T: get(key)
alt currentLanguage == DE/FR/ES
T->>DE: lookup(key)
DE-->>T: text or empty
else
T->>EN: lookup(key)
EN-->>T: text
end
opt missing
T->>EN: fallback lookup
EN-->>T: text
end
T-->>UI: const char* label
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/display/ui/default/DefaultUI.cpp (1)
82-91: Subscribe to ui:refresh to make language changes visible immediatelyLanguagePlugin emits "ui:refresh" on change, but DefaultUI does not subscribe. Without this, many labels that are only updated by reactive effects tied to other state may not re-render on language change.
Proposed minimal fix:
void DefaultUI::init() { auto triggerRender = [this](Event const &) { rerender = true; }; + // Refresh UI texts when language changes + pluginManager->on("ui:refresh", triggerRender);Note: If your EffectManager only runs effects when dependencies change, you may also need to explicitly rebind static labels (or add a 'languageVersion' dependency) to ensure text is re-applied on refresh.
🧹 Nitpick comments (14)
src/display/core/constants.h (1)
26-26: DEFAULT_LANGUAGE magic number risks drifting from Language::ENGLISH; add validation or derive from enumUsing a hardcoded 0 assumes Language::ENGLISH stays first. If the enum order changes, persisted defaults can mismatch silently.
- Minimal: add a compile-time check in Translation.cpp to ensure alignment.
- Better: validate/coerce loaded/assigned values (see suggested changes in Settings.cpp/Translation.h).
Add this compile-time guard in src/display/core/Translation.cpp to catch drift early:
#include <display/core/constants.h> static_assert(static_cast<int>(Language::ENGLISH) == DEFAULT_LANGUAGE, "DEFAULT_LANGUAGE must match Language::ENGLISH");src/display/core/Settings.h (1)
80-82: Type-safety and no-op optimization for language setterAPI looks fine. Consider:
- Return/accept Language instead of int for type-safety, or add an overload returning Language.
- Make setLanguage a no-op when the value is unchanged to avoid unnecessary flash writes.
See inline setter change suggested in Settings.cpp comment.
src/display/ui/default/DefaultUI.h (1)
7-7: OK to include Translation here; consider moving to .cpp if not needed in the headerIf TR(...) or Translation symbols are only used in DefaultUI.cpp, including Translation.h here increases compile-time coupling. Moving the include to the .cpp would keep the header lean.
src/display/core/Settings.cpp (2)
69-69: Validate persisted language before using itReading an arbitrary int and later casting to Language elsewhere can yield invalid enum values. While Translation::get falls back via default: case, validating upfront is cleaner and prevents surprising state.
Apply this diff to validate the stored value:
- language = preferences.getInt("lang", DEFAULT_LANGUAGE); + int lang = preferences.getInt("lang", DEFAULT_LANGUAGE); + // Coerce invalid values to default + language = (lang >= 0 && lang <= 3) ? lang : DEFAULT_LANGUAGE;Additionally, prefer centralizing validation via Translation helpers (see proposed additions to Translation.h below). If you add those helpers, replace the ternary with:
language = Translation::isValid(lang) ? lang : DEFAULT_LANGUAGE;To support that, add this include near the top of this file:
#include <display/core/Translation.h>
383-386: Coerce/ignore unchanged language to avoid unnecessary writes and invalid statesAvoid saving when unchanged and coerce invalid inputs to the default. This reduces flash wear and keeps state consistent.
Apply this diff:
-void Settings::setLanguage(int language) { - this->language = language; - save(); -} +void Settings::setLanguage(int language) { + // Coerce invalid values; replace with Translation::isValid(...) if added + int next = (language >= 0 && language <= 3) ? language : DEFAULT_LANGUAGE; + if (this->language == next) { + return; // no-op + } + this->language = next; + save(); +}src/display/core/Translation.h (2)
6-11: Add COUNT sentinel to future-proof validation/clampingA COUNT enumerator makes validation robust without hardcoding limits elsewhere.
Apply this diff:
-enum class Language { - ENGLISH, - GERMAN, - FRENCH, - SPANISH -}; +enum class Language { + ENGLISH, + GERMAN, + FRENCH, + SPANISH, + COUNT +};
34-47: Expose validation/coercion helpers to centralize handling of int<->LanguageCallers (Settings, LanguagePlugin) can use these to prevent invalid enum states and reduce duplication.
Apply this diff:
class Translation { public: static void setLanguage(Language lang); static Language getLanguage(); static const char* get(TranslationKey key); static String format(const char* format, ...); - + // Helpers for int-based settings/APIs + static bool isValid(int v) { return v >= 0 && v < static_cast<int>(Language::COUNT); } + static Language coerce(int v) { return isValid(v) ? static_cast<Language>(v) : Language::ENGLISH; } private: static Language currentLanguage; static const char* getEnglish(TranslationKey key); static const char* getGerman(TranslationKey key); static const char* getFrench(TranslationKey key); static const char* getSpanish(TranslationKey key); };Optional: add a compile-time guard (in Translation.cpp) to ensure DEFAULT_LANGUAGE maps to ENGLISH as noted in constants.h review.
src/display/ui/default/DefaultUI.cpp (2)
546-550: Brew screen: split name and caption — OK, but ensure caption updates on language changeThe split between profile name and a localized caption is fine. With the ui:refresh subscription (suggested above), this will update when the language changes.
693-696: Format string nit: use %.1f, not %.1lf, for printf-style APIs"%.1lfg" works because the trailing "g" is literal text, but "l" is unnecessary with printf (unlike scanf) and inconsistent with the rest of the file.
- lv_label_set_text_fmt(ui_StatusScreen_brewVolume, "%.1lfg", brewProcess->currentVolume); + lv_label_set_text_fmt(ui_StatusScreen_brewVolume, "%.1fg", brewProcess->currentVolume);src/display/plugins/LanguagePlugin.cpp (1)
8-13: Event wiring is fine; consider debouncing identical selectionsThe subscription is correct. Optional: avoid re-saving and emitting refresh when the selected language is unchanged.
void LanguagePlugin::init() { controller->getPluginManager()->on("language:change", [this](Event const &event) { int language = event.getInt("language"); - setLanguage(language); + setLanguage(language); }); }And inside setLanguage (see next comment) add an early-return when the language matches.
src/display/core/Translation.cpp (4)
1-8: Default language should follow DEFAULT_LANGUAGE constantHardcoding ENGLISH can drift from the configured default. Use the macro to keep behavior consistent with Settings.
#include "Translation.h" +#include <display/core/constants.h> #include <stdarg.h> -Language Translation::currentLanguage = Language::ENGLISH; +Language Translation::currentLanguage = static_cast<Language>(DEFAULT_LANGUAGE);
37-44: Static buffer in format() — acceptable, but consider longer texts256 bytes may be tight for concatenated translations on some screens. Optional: increase to 512 or document constraints.
70-92: German translations: quality is good, but check capitalization consistencyMinor nit: "Autotune..." vs "Autotuning..." and "Neustart benötigt" vs "Neustart erforderlich". Not blocking.
94-116: French and Spanish tables are placeholders in EnglishMost keys are untranslated. If shipping multi-language, this will feel incomplete.
Would you like help providing proper FR/ES translations for the existing keys, or should we gate language choices to only those with >90% coverage?
Also applies to: 118-140
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
src/display/core/Settings.cpp(3 hunks)src/display/core/Settings.h(2 hunks)src/display/core/Translation.cpp(1 hunks)src/display/core/Translation.h(1 hunks)src/display/core/constants.h(1 hunks)src/display/plugins/LanguagePlugin.cpp(1 hunks)src/display/plugins/LanguagePlugin.h(1 hunks)src/display/ui/default/DefaultUI.cpp(8 hunks)src/display/ui/default/DefaultUI.h(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
src/display/plugins/LanguagePlugin.h (4)
src/display/plugins/LanguagePlugin.cpp (17)
LanguagePlugin(4-6)init(8-13)init(8-8)getName(21-23)getName(21-21)getDescription(25-27)getDescription(25-25)isEnabled(29-31)isEnabled(29-29)setEnabled(33-34)setEnabled(33-33)getConfig(36-38)getConfig(36-36)setConfig(40-51)setConfig(40-40)setLanguage(15-19)setLanguage(15-15)src/display/core/Plugin.h (1)
Plugin(6-12)src/display/core/Translation.cpp (2)
setLanguage(6-8)setLanguage(6-6)src/display/core/Settings.cpp (2)
setLanguage(383-386)setLanguage(383-383)
src/display/core/Translation.h (3)
src/display/plugins/LanguagePlugin.cpp (2)
setLanguage(15-19)setLanguage(15-15)src/display/core/Translation.cpp (16)
setLanguage(6-8)setLanguage(6-6)getLanguage(10-12)getLanguage(10-10)get(14-35)get(14-14)format(37-44)format(37-37)getEnglish(46-68)getEnglish(46-46)getGerman(70-92)getGerman(70-70)getFrench(94-116)getFrench(94-94)getSpanish(118-140)getSpanish(118-118)src/display/core/Settings.cpp (2)
setLanguage(383-386)setLanguage(383-383)
src/display/core/Settings.h (3)
src/display/core/Translation.cpp (4)
getLanguage(10-12)getLanguage(10-10)setLanguage(6-8)setLanguage(6-6)src/display/plugins/LanguagePlugin.cpp (2)
setLanguage(15-19)setLanguage(15-15)src/display/core/Settings.cpp (2)
setLanguage(383-386)setLanguage(383-383)
src/display/plugins/LanguagePlugin.cpp (4)
src/display/plugins/LanguagePlugin.h (1)
LanguagePlugin(6-20)src/display/ui/default/DefaultUI.cpp (2)
init(82-189)init(82-82)src/display/core/Translation.cpp (2)
setLanguage(6-8)setLanguage(6-6)src/display/core/Settings.cpp (2)
setLanguage(383-386)setLanguage(383-383)
src/display/ui/default/DefaultUI.cpp (2)
src/display/plugins/LanguagePlugin.cpp (2)
setLanguage(15-19)setLanguage(15-15)src/display/core/Translation.cpp (2)
setLanguage(6-8)setLanguage(6-6)
src/display/core/Translation.cpp (2)
src/display/plugins/LanguagePlugin.cpp (2)
setLanguage(15-19)setLanguage(15-15)src/display/core/Settings.cpp (2)
setLanguage(383-386)setLanguage(383-383)
src/display/core/Settings.cpp (2)
src/display/plugins/LanguagePlugin.cpp (2)
setLanguage(15-19)setLanguage(15-15)src/display/core/Translation.cpp (2)
setLanguage(6-8)setLanguage(6-6)
⏰ 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). (2)
- GitHub Check: deploy
- GitHub Check: test
🔇 Additional comments (10)
src/display/core/Settings.h (1)
210-210: Sane default initializationInitializing to DEFAULT_LANGUAGE is correct and consistent with preferences fallback.
src/display/core/Settings.cpp (1)
457-457: Persisting 'lang' key looks correctThe key name aligns with the constructor load. No concerns.
src/display/core/Translation.h (1)
49-49: TR macro is straightforward and usefulMacro indirection for keys is fine and keeps call sites tidy.
src/display/ui/default/DefaultUI.cpp (5)
75-81: Initialize language from settings early — looks goodSetting the translation language in the constructor ensures UI comes up in the persisted language. No concerns here.
336-337: Good use of TR() for mode-specific labelReplacing literals with translations is correct and keeps UI reactive to language.
441-448: Good: status messages now localizedUPDATING, TEMPERATURE_ERROR, and AUTOTUNING use TR() as expected.
644-646: Localized phase labels — looks goodPhase caption and finished state correctly use translations.
657-663: Consistent formatting — goodTranslating only the unit-less labels while keeping numeric formats is appropriate.
src/display/plugins/LanguagePlugin.cpp (1)
36-38: Config JSON serialization is OK as a minimal payloadSimple and sufficient for current needs.
src/display/core/Translation.cpp (1)
14-35: Graceful fallback logic — goodDispatcher and English fallback are correct and safe.
| void LanguagePlugin::setLanguage(int language) { | ||
| Translation::setLanguage(static_cast<Language>(language)); | ||
| controller->getSettings().setLanguage(language); | ||
| controller->getPluginManager()->emit("ui:refresh", {}); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate and short-circuit language changes to prevent invalid state and unnecessary writes
Currently, any integer is cast to Language and persisted. Add range checking and skip if unchanged.
void LanguagePlugin::setLanguage(int language) {
- Translation::setLanguage(static_cast<Language>(language));
- controller->getSettings().setLanguage(language);
- controller->getPluginManager()->emit("ui:refresh", {});
+ auto newLang = static_cast<Language>(language);
+ // Clamp to known enum values
+ if (newLang != Language::ENGLISH && newLang != Language::GERMAN &&
+ newLang != Language::FRENCH && newLang != Language::SPANISH) {
+ // Ignore invalid values
+ return;
+ }
+ if (Translation::getLanguage() == newLang) {
+ return; // no-op if same language
+ }
+ Translation::setLanguage(newLang);
+ controller->getSettings().setLanguage(static_cast<int>(newLang));
+ controller->getPluginManager()->emit("ui:refresh", {});
}📝 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.
| void LanguagePlugin::setLanguage(int language) { | |
| Translation::setLanguage(static_cast<Language>(language)); | |
| controller->getSettings().setLanguage(language); | |
| controller->getPluginManager()->emit("ui:refresh", {}); | |
| } | |
| void LanguagePlugin::setLanguage(int language) { | |
| auto newLang = static_cast<Language>(language); | |
| // Clamp to known enum values | |
| if (newLang != Language::ENGLISH && newLang != Language::GERMAN && | |
| newLang != Language::FRENCH && newLang != Language::SPANISH) { | |
| // Ignore invalid values | |
| return; | |
| } | |
| if (Translation::getLanguage() == newLang) { | |
| return; // no-op if same language | |
| } | |
| Translation::setLanguage(newLang); | |
| controller->getSettings().setLanguage(static_cast<int>(newLang)); | |
| controller->getPluginManager()->emit("ui:refresh", {}); | |
| } |
🤖 Prompt for AI Agents
In src/display/plugins/LanguagePlugin.cpp around lines 15-19, add validation and
a short-circuit so only valid enum values are applied and persisted: first check
that the passed int is within the Language enum range (reject out-of-range
values), then if the target Language equals the current language (from
Translation::getLanguage() or controller->getSettings().getLanguage()) return
early; otherwise call Translation::setLanguage(...) and persist via
controller->getSettings().setLanguage(...) and emit "ui:refresh" — perform the
checks before mutating state to avoid invalid state and unnecessary writes.
| void LanguagePlugin::setConfig(const String &config) { | ||
| if (config.indexOf("\"language\":") != -1) { | ||
| int start = config.indexOf("\"language\":") + 11; | ||
| int end = config.indexOf(",", start); | ||
| if (end == -1) end = config.indexOf("}", start); | ||
| if (end != -1) { | ||
| String langStr = config.substring(start, end); | ||
| int language = langStr.toInt(); | ||
| setLanguage(language); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make config parsing tolerant to whitespace and validate bounds
Parsing by fixed offset fails with spaces and will silently accept invalid values. Trim and bound-check before applying.
void LanguagePlugin::setConfig(const String &config) {
- if (config.indexOf("\"language\":") != -1) {
- int start = config.indexOf("\"language\":") + 11;
- int end = config.indexOf(",", start);
- if (end == -1) end = config.indexOf("}", start);
- if (end != -1) {
- String langStr = config.substring(start, end);
- int language = langStr.toInt();
- setLanguage(language);
- }
- }
+ int keyPos = config.indexOf("\"language\"");
+ if (keyPos == -1) return;
+ int colon = config.indexOf(":", keyPos);
+ if (colon == -1) return;
+ int end = config.indexOf(",", colon);
+ if (end == -1) end = config.indexOf("}", colon);
+ if (end == -1) return;
+ String langStr = config.substring(colon + 1, end);
+ langStr.trim();
+ if (langStr.length() == 0) return;
+ int language = langStr.toInt(); // tolerates leading '+'/'-' and spaces
+ setLanguage(language);
}📝 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.
| void LanguagePlugin::setConfig(const String &config) { | |
| if (config.indexOf("\"language\":") != -1) { | |
| int start = config.indexOf("\"language\":") + 11; | |
| int end = config.indexOf(",", start); | |
| if (end == -1) end = config.indexOf("}", start); | |
| if (end != -1) { | |
| String langStr = config.substring(start, end); | |
| int language = langStr.toInt(); | |
| setLanguage(language); | |
| } | |
| } | |
| } | |
| void LanguagePlugin::setConfig(const String &config) { | |
| int keyPos = config.indexOf("\"language\""); | |
| if (keyPos == -1) return; | |
| int colon = config.indexOf(":", keyPos); | |
| if (colon == -1) return; | |
| int end = config.indexOf(",", colon); | |
| if (end == -1) end = config.indexOf("}", colon); | |
| if (end == -1) return; | |
| String langStr = config.substring(colon + 1, end); | |
| langStr.trim(); | |
| if (langStr.length() == 0) return; | |
| int language = langStr.toInt(); // tolerates leading '+'/'-' and spaces | |
| setLanguage(language); | |
| } |
🤖 Prompt for AI Agents
In src/display/plugins/LanguagePlugin.cpp around lines 40 to 51, the config
parsing uses a fixed offset after "\"language\":" which breaks when there is
whitespace and then silently accepts invalid values; change the logic to locate
the "\"language\"" token, find the next ':' position dynamically, set start =
colonIndex + 1, extract substring from start to the following ',' or '}', then
trim surrounding whitespace and optional quotes from that substring, parse it to
an integer only after trimming, and validate the parsed value against the
allowed language bounds (e.g. non-negative and <= your MAX_LANGUAGE or check
against the Language enum range) before calling setLanguage; if parsing fails or
the value is out of bounds, do not call setLanguage and handle the error (log or
ignore) accordingly.
| lv_label_set_text_fmt(ui_ProfileScreen_targetTemp2, "%d°C", static_cast<int>(currentProfileChoice.temperature)); | ||
| unsigned int phaseCount = currentProfileChoice.getPhaseCount(); | ||
| unsigned int stepCount = currentProfileChoice.phases.size(); | ||
| lv_label_set_text_fmt(ui_ProfileScreen_stepsLabel, "%d step%s", stepCount, stepCount > 1 ? "s" : ""); | ||
| lv_label_set_text_fmt(ui_ProfileScreen_phasesLabel, "%d phase%s", phaseCount, phaseCount > 1 ? "s" : ""); | ||
| lv_label_set_text_fmt(ui_ProfileScreen_stepsLabel, "%d %s%s", stepCount, TR(STEP), stepCount > 1 ? "s" : ""); | ||
| lv_label_set_text_fmt(ui_ProfileScreen_phasesLabel, "%d %s%s", phaseCount, TR(PHASE), phaseCount > 1 ? "s" : ""); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pluralization will be wrong for non-English — use separate plural keys
Appending "s" breaks in German/French/Spanish. Since Translation already provides STEPS/PHASES, switch to singular/plural keys based on count.
- lv_label_set_text_fmt(ui_ProfileScreen_stepsLabel, "%d %s%s", stepCount, TR(STEP), stepCount > 1 ? "s" : "");
- lv_label_set_text_fmt(ui_ProfileScreen_phasesLabel, "%d %s%s", phaseCount, TR(PHASE), phaseCount > 1 ? "s" : "");
+ lv_label_set_text_fmt(ui_ProfileScreen_stepsLabel, "%d %s", stepCount, stepCount == 1 ? TR(STEP) : TR(STEPS));
+ lv_label_set_text_fmt(ui_ProfileScreen_phasesLabel, "%d %s", phaseCount, phaseCount == 1 ? TR(PHASE) : TR(PHASES));🤖 Prompt for AI Agents
In src/display/ui/default/DefaultUI.cpp around lines 560 to 565, the code
appends "s" to create plurals which breaks for non-English locales; instead
select the correct translation key for singular vs plural. Replace the
concatenation logic with a conditional that chooses the plural translation key
provided by the translations (e.g. use TR(STEP) when stepCount == 1 and
TR(STEPS) otherwise, and likewise TR(PHASE) vs TR(PHASES)), and use that
translation in the lv_label_set_text_fmt calls so the UI shows proper localized
singular/plural words.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/display/plugins/LanguagePlugin.h (2)
11-12: Overrides now match Plugin’s interface (setup/loop) — good fixThis aligns with Plugin.h’s pure virtuals and resolves the earlier mismatch.
1-6: Include Plugin.h; deriving from an incomplete base will not compileThis header derives from Plugin but does not include its definition. Forward-declaring a base is insufficient; the base must be complete at the point of inheritance.
Apply:
#ifndef LANGUAGE_PLUGIN_H #define LANGUAGE_PLUGIN_H +// Required for base-class definition +#include <display/core/Plugin.h> + class Controller; class PluginManager;
🧹 Nitpick comments (3)
src/display/plugins/LanguagePlugin.h (1)
14-15: Consider using the strongly-typed Language enum for setLanguageFor type safety and self-documenting intent, prefer Language over int. This will require including Translation.h in this header (or moving setLanguage to the .cpp and keeping it out of the header if it’s not used externally).
Option A (type-safe):
-#include <display/core/Plugin.h> +#include <display/core/Plugin.h> +#include <display/core/Translation.h> ... private: - void setLanguage(int language); + void setLanguage(Language language);Option B (keep header lean): remove setLanguage from the header and define it only in the .cpp as a private helper not exposed in the class declaration.
src/display/core/TranslationStrings.h (1)
4-4: Remove unused include to keep headers slimTranslation.h isn’t referenced in this header; consider dropping it.
-#include "Translation.h"src/display/core/Translation.cpp (1)
46-53: Optional: prefer and consistent C++ headersMinor style nit: prefer over <stdarg.h> to align with C++ headers.
-#include <stdarg.h> +#include <cstdarg>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
CONTRIBUTING.md(1 hunks)src/display/core/Translation.cpp(1 hunks)src/display/core/Translation.h(1 hunks)src/display/core/TranslationStrings.h(1 hunks)src/display/plugins/LanguagePlugin.h(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CONTRIBUTING.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/display/core/Translation.h
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/display/plugins/LanguagePlugin.h (5)
src/display/core/PluginManager.h (1)
PluginManager(14-33)src/display/plugins/LanguagePlugin.cpp (3)
LanguagePlugin(4-6)setLanguage(15-19)setLanguage(15-15)src/display/core/Plugin.h (1)
Plugin(6-12)src/display/core/Translation.cpp (2)
setLanguage(7-9)setLanguage(7-7)src/display/core/Settings.cpp (2)
setLanguage(383-386)setLanguage(383-383)
src/display/core/Translation.cpp (2)
src/display/plugins/LanguagePlugin.cpp (2)
setLanguage(15-19)setLanguage(15-15)src/display/core/Settings.cpp (2)
setLanguage(383-386)setLanguage(383-383)
⏰ 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). (2)
- GitHub Check: deploy
- GitHub Check: test
🔇 Additional comments (1)
src/display/core/TranslationStrings.h (1)
30-49: German umlauts on embedded displaysVerify the display font supports ä/ö/ü/ß; otherwise consider transliteration (ae/oe/ue/ss) or updating fonts.
| @@ -0,0 +1,55 @@ | |||
| #include "Translation.h" | |||
| #include "TranslationStrings.h" | |||
| #include <stdarg.h> | |||
There was a problem hiding this comment.
Include cstdio for vsnprintf declaration
vsnprintf is declared in /<stdio.h>. Without it, some toolchains will error.
#include <stdarg.h>
+#include <cstdio>📝 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.
| #include <stdarg.h> | |
| #include <stdarg.h> | |
| #include <cstdio> |
🤖 Prompt for AI Agents
In src/display/core/Translation.cpp around line 3, add the standard C header
that declares vsnprintf (e.g., include <cstdio> or <stdio.h>) because the
current file only includes <stdarg.h> and some toolchains require the proper
declaration for vsnprintf; update the includes to #include <cstdio> (or #include
<stdio.h>) alongside <stdarg.h> so the vsnprintf prototype is available.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/display/core/TranslationStrings.h (3)
1-3: Include what you use: add and prefer std::size_t for NUM_KEYSUsing size_t without including relies on transitive includes. Let’s make the header self-contained and use std::size_t.
#ifndef TRANSLATION_STRINGS_H #define TRANSLATION_STRINGS_H +#include <cstddef> @@ - static constexpr size_t NUM_KEYS = 18; + static constexpr std::size_t NUM_KEYS = 18;Also applies to: 12-12
4-4: Remove unused forward declaration of TranslationKey (and avoid potential namespace mismatch)This header doesn’t use TranslationKey. Keeping an unused forward decl in the global namespace risks type duplication if the real enum lives in a namespace.
-enum class TranslationKey;
12-12: Single source of truth for key countNUM_KEYS should originate from the enum (e.g., a COUNT sentinel) to prevent drift. If feasible, define COUNT in Translation.h and derive NUM_KEYS from it; then include Translation.h here and drop the hard-coded 18.
Example (outside this diff, shown for clarity):
// Translation.h #include <cstddef> enum class TranslationKey : unsigned char { BREW, STEAM, WATER, GRIND, SELECT_PROFILE, STARTING, UPDATING, TEMPERATURE_ERROR, AUTOTUNING, FINISHED, INFUSION, BREW_PHASE, STEPS, PHASES, STEP, PHASE, SELECTED_PROFILE, RESTART_REQUIRED, COUNT }; inline constexpr std::size_t NUM_KEYS = static_cast<std::size_t>(TranslationKey::COUNT); // TranslationStrings.h #include "Translation.h" // ... use NUM_KEYS from above and sized extern arraysHappy to generate a follow-up patch if you confirm where TranslationKey is declared.
src/display/core/TranslationStrings.cpp (1)
91-91: Add static_asserts to prevent out-of-sync tables at compile timeIf a table’s element count diverges from NUM_KEYS, you’ll get an immediate compile error instead of a runtime misindex.
} +// Compile-time sanity checks +static_assert(sizeof(TranslationStrings::ENGLISH) / sizeof(TranslationStrings::ENGLISH[0]) == TranslationStrings::NUM_KEYS, "ENGLISH length mismatch"); +static_assert(sizeof(TranslationStrings::GERMAN) / sizeof(TranslationStrings::GERMAN[0]) == TranslationStrings::NUM_KEYS, "GERMAN length mismatch"); +static_assert(sizeof(TranslationStrings::FRENCH) / sizeof(TranslationStrings::FRENCH[0]) == TranslationStrings::NUM_KEYS, "FRENCH length mismatch"); +static_assert(sizeof(TranslationStrings::SPANISH) / sizeof(TranslationStrings::SPANISH[0]) == TranslationStrings::NUM_KEYS, "SPANISH length mismatch");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/display/core/TranslationStrings.cpp(1 hunks)src/display/core/TranslationStrings.h(1 hunks)
🔇 Additional comments (3)
src/display/core/TranslationStrings.h (1)
6-10: Bold move to extern declarations; this fixes per-TU duplication and aligns with prior review guidance.This isolates definitions to the .cpp, improving flash/RAM usage on embedded targets.
src/display/core/TranslationStrings.cpp (2)
27-46: Verify UTF-8 rendering for non-ASCII characters on target display/fontGerman and Spanish strings include diacritics (ä, ö, Ó). Ensure:
- Source files are saved as UTF-8 without BOM.
- Toolchain treats input as UTF-8.
- The glyphs exist in the runtime font and your text renderer handles UTF-8 correctly.
If your display stack is ASCII-only, consider ASCII fallbacks (e.g., “auswaehlen”, “benoetigt”, “INFUSION”) or adding the required glyphs to the font.
Also applies to: 71-90
3-24: Definitions centralized in .cpp look goodGood job moving from header to a single definition unit. The arrays align with 18 keys and comments map each entry unambiguously.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/display/core/TranslationStrings.h (2)
7-7: Avoid drift: derive NUM_KEYS from the canonical key enumIf the translation key enum/count already exists (e.g., in Translation.h), consider deriving NUM_KEYS from it instead of hardcoding 18. This removes a maintenance hotspot when keys are added/removed.
9-12: Add compile-time guards in the .cpp to enforce table sizesAs a safety net, add static_asserts in TranslationStrings.cpp to verify each table’s length equals NUM_KEYS. This gives immediate diagnostics if a new key is added and a table isn’t updated.
Example to add in src/display/core/TranslationStrings.cpp:
#include <type_traits> // for std::extent if needed #include "TranslationStrings.h" namespace { static_assert(std::extent<decltype(TranslationStrings::ENGLISH)>::value == TranslationStrings::NUM_KEYS, "ENGLISH table size mismatch"); static_assert(std::extent<decltype(TranslationStrings::GERMAN)>::value == TranslationStrings::NUM_KEYS, "GERMAN table size mismatch"); static_assert(std::extent<decltype(TranslationStrings::FRENCH)>::value == TranslationStrings::NUM_KEYS, "FRENCH table size mismatch"); static_assert(std::extent<decltype(TranslationStrings::SPANISH)>::value == TranslationStrings::NUM_KEYS, "SPANISH table size mismatch"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/display/core/TranslationStrings.cpp(1 hunks)src/display/core/TranslationStrings.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/display/core/TranslationStrings.cpp
🔇 Additional comments (1)
src/display/core/TranslationStrings.h (1)
9-12: Good: extern arrays with explicit compile-time boundsDeclaring the language tables as extern with [NUM_KEYS] addresses ODR bloat and catches mismatches at compile time. Nice follow-through on prior feedback.
|
|
Per the discord conversation the umlaut and other special ansi characters probably not render they can be converted to standard ü -> u or we can just not translate the display there are not many words |
2 participants
This is very much a draft but I want the rabbit reviews
Summary by CodeRabbit
New Features
Documentation