support Korean && handle line breaks on device#144
support Korean && handle line breaks on device#144lihuanhuan merged 2 commits intoOneKeyHQ:masterfrom
Conversation
WalkthroughThis update adds full Korean language support to the firmware. It refactors text rendering to improve UTF-8 handling and line wrapping, replacing old Chinese-specific logic. Many localization files receive text improvements for clarity and consistency. The build process now uses the new OLED text module instead of the legacy Chinese font module. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Firmware
participant OLED_Text_Module
participant I18N_Module
User->>Firmware: Selects Korean language
Firmware->>I18N_Module: Loads Korean strings (languages_ko_kr)
Firmware->>OLED_Text_Module: Renders UI text (UTF-8 aware)
OLED_Text_Module-->>Firmware: Draws wrapped and centered text
Firmware-->>User: Displays Korean UI
sequenceDiagram
participant Firmware
participant OLED_Text_Module
Firmware->>OLED_Text_Module: Draw string with wrapping/centering
OLED_Text_Module->>OLED_Text_Module: Split string into lines (UTF-8)
OLED_Text_Module->>OLED_Text_Module: Calculate widths and draw characters
OLED_Text_Module-->>Firmware: Return control after drawing
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
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: 13
🔭 Outside diff range comments (2)
legacy/firmware/layout2.c (1)
510-524: 🛠️ Refactor suggestionCenter logic now ignores proportional fonts
oledDrawStringAdapter(x, …)draws left-aligned text.
After switching from the centered helper to plain draw, multi-byte
characters (Korean) no longer appear centred. Consider retaining the
centering calculation or usingoledDrawStringCenterAdapter()for both
lines.legacy/firmware/oled_text.c (1)
90-118:⚠️ Potential issueHorizontal scaling and returned width out-of-sync with
zoomInside
draw_char()you upscale the vertical pixels witholedBox, but you never upscale horizontally and you still return the un-scaled width:- return oledDrawStringX(x, y, char_data); + int raw_w = oledDrawStringX(x, y, char_data); + return raw_w * zoom;Either draw the glyph stretched on the X-axis too (preferred), or keep a 1× draw path and fix
cursor_xby returning the scaled width. Right now double-size lines overlap.
♻️ Duplicate comments (1)
legacy/firmware/layout2.c (1)
722-729: Same duplicate refresh patternThe same comment as above applies here; ditch the extra
oledRefresh().- oledBox(x, offset, x + l, offset, true); - oledRefresh(); + oledBox(x, offset, x + l, offset, true);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (22)
legacy/firmware/Makefile(1 hunks)legacy/firmware/font_ex.c(4 hunks)legacy/firmware/font_ex.h(1 hunks)legacy/firmware/gettext.c(1 hunks)legacy/firmware/i18n/i18n.c(1 hunks)legacy/firmware/i18n/i18n.h(1 hunks)legacy/firmware/i18n/keys.h(6 hunks)legacy/firmware/i18n/locales/de.inc(5 hunks)legacy/firmware/i18n/locales/en.inc(9 hunks)legacy/firmware/i18n/locales/es.inc(10 hunks)legacy/firmware/i18n/locales/ja.inc(9 hunks)legacy/firmware/i18n/locales/ko_kr.inc(1 hunks)legacy/firmware/i18n/locales/pt_br.inc(10 hunks)legacy/firmware/i18n/locales/zh_cn.inc(8 hunks)legacy/firmware/i18n/locales/zh_tw.inc(9 hunks)legacy/firmware/layout2.c(12 hunks)legacy/firmware/layout2.h(1 hunks)legacy/firmware/menu_list.c(1 hunks)legacy/firmware/oled_text.c(4 hunks)legacy/firmware/oled_text.h(1 hunks)legacy/script/i18n.py(2 hunks)legacy/si2c.c(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
legacy/firmware/menu_list.c (2)
legacy/firmware/menu_para.c (1)
menu_para_set_language(133-135)legacy/firmware/menu_para.h (1)
menu_para_set_language(27-27)
legacy/firmware/layout2.c (4)
legacy/firmware/oled_text.h (4)
split_string_to_lines(17-18)oledDrawStringAdapter(22-22)oledDrawStringCenterAdapterX(24-24)oledDrawStringCenterAdapter(23-23)legacy/firmware/oled_text.c (4)
split_string_to_lines(194-207)oledDrawStringAdapter(290-294)oledDrawStringCenterAdapterX(301-307)oledDrawStringCenterAdapter(296-299)legacy/firmware/font_ex.c (1)
font_get_height(167-173)legacy/oled.c (2)
oledBox(436-446)oledRefresh(225-251)
🪛 GitHub Actions: Prebuild checks
legacy/firmware/Makefile
[error] 81-81: Makefile target 'defs_check' failed due to errors in duplicate symbol checks.
🔇 Additional comments (29)
legacy/firmware/layout2.h (1)
28-28: Header replacement looks good.This change aligns with the broader refactoring to replace Chinese-specific text handling with generic UTF-8 text rendering. The new
oled_text.hmodule should provide better support for multiple languages including Korean.legacy/si2c.c (1)
9-9: Consistent header replacement.The change from
chinese.htooled_text.hmaintains consistency with the broader refactoring effort. Good to see the systematic approach across files.legacy/firmware/gettext.c (1)
27-28: Korean language support added correctly.The new case follows the existing pattern perfectly. The sequential case numbering and array access implementation match the other supported languages.
legacy/firmware/Makefile (1)
64-64:Details
✅ Verification successful
Object file replacement needs verification.
The change from
chinese.otooled_text.oaligns with the header refactoring, but the pipeline failure suggests potential issues.The pipeline failure mentions duplicate symbol errors in the
defs_checktarget. Please verify that:
- The old
chinese.csource file has been removed or renamed- No other Makefile or build configuration still references
chinese.o- There are no symbol conflicts between the old and new modules
🏁 Script executed:
#!/bin/bash # Check for any remaining references to chinese.o or chinese.c echo "Searching for chinese.o references:" rg -g "Makefile*" -g "*.mk" "chinese\.o" echo "Searching for chinese.c files:" fd "chinese\.c" echo "Checking for symbol conflicts in oled_text vs chinese:" rg -A 5 -B 5 "^(extern|static).*\b(oled_text|chinese)" --type cLength of output: 397
No residual chinese references found
Our searches returned no matches for
chinese.o,chinese.c, or overlapping symbols inoled_text. The switch tooled_text.ois clean.No changes needed—please re-run with a clean build to clear any stale artifacts.
legacy/firmware/menu_list.c (1)
193-194: Korean language option added correctly.The Korean language entry "한국어" is properly integrated into the language menu with consistent structure and callback function.
legacy/firmware/font_ex.h (1)
33-33: UTF-8 character iteration function added.This function declaration supports proper handling of multi-byte characters needed for Korean text rendering.
legacy/firmware/i18n/i18n.h (1)
20-20: Korean language array declaration added.The external declaration follows the established naming convention and type signature pattern.
legacy/firmware/i18n/i18n.c (3)
3-3: Language count updated correctly.The
langs_lenvalue properly reflects the addition of Korean as the 8th supported language.
14-14: Korean locale include added correctly.The include statement follows the alphabetical ordering convention and properly references the Korean locale file.
6-7:Details
✅ Verification successful
Korean language integration looks good.
Both the language key "ko_KR" and display name "한국어" are correctly added to their respective arrays, maintaining proper synchronization.
Verify that all language arrays remain synchronized across the codebase:
🏁 Script executed:
#!/bin/bash # Description: Check that all language-related arrays have consistent length and ordering echo "Checking language array synchronization..." # Count elements in language keys array rg -A 1 "i18n_lang_keys\[\]" legacy/firmware/i18n/i18n.c # Count elements in language names array rg -A 1 "i18n_langs\[\]" legacy/firmware/i18n/i18n.c # Check menu items array length rg -A 10 "language_set_menu_items\[\]" legacy/firmware/menu_list.c # Verify langs_len value matches actual count rg "langs_len.*=" legacy/firmware/i18n/i18n.cLength of output: 1362
All language arrays are synchronized
- i18n_lang_keys: 8 entries
- i18n_langs: 8 entries
- language_set_menu_items: 8 entries
- langs_len = 8
Korean (“ko_KR” / “한국어”) aligns perfectly. No further action needed.
legacy/script/i18n.py (1)
7-7: LGTM! Korean language support properly added.The Korean language code "ko_KR" is correctly added to both the supported languages tuple and language mapping dictionary. This enables Korean localization processing.
Also applies to: 20-20
legacy/firmware/i18n/locales/en.inc (1)
4-4: LGTM! Text improvements enhance clarity and consistency.These refinements remove line breaks, add proper punctuation, and improve wording. The changes make the English localization more professional and readable.
Also applies to: 8-8, 14-14, 17-17, 19-19, 22-22, 24-25, 27-27, 29-29, 33-33, 36-36, 38-38, 40-40, 42-43, 45-45, 51-52, 77-78, 80-81, 85-85, 87-87, 94-94, 96-99, 100-102, 104-105, 107-107, 125-127, 129-132, 135-135, 141-141, 147-147, 150-151, 153-153, 157-157, 161-162, 165-166, 181-181, 185-185, 189-190, 230-230, 260-262, 284-288, 290-292, 295-295, 297-298, 303-304, 306-307, 311-311, 316-316, 322-322, 344-346, 380-380
legacy/firmware/i18n/keys.h (1)
8-8: LGTM! Comment improvements enhance code documentation.The comment refinements add proper punctuation and improve clarity. These changes make the code more maintainable without affecting functionality.
Also applies to: 18-18, 37-37, 41-41, 48-48, 52-52, 54-54, 59-60, 63-63, 71-71, 77-77, 81-81, 85-85, 89-89, 91-91, 95-95, 107-108, 210-214, 216-220, 222-232, 238-298, 300-318, 320-796
legacy/firmware/i18n/locales/pt_br.inc (1)
4-4: LGTM! Portuguese localization quality significantly improved.The text refinements standardize terminology, improve grammar, and enhance clarity. The formal tone is more appropriate for firmware interfaces.
Also applies to: 8-8, 14-14, 17-17, 19-19, 22-22, 24-25, 27-27, 29-29, 33-33, 36-36, 38-38, 40-40, 42-43, 45-45, 51-52, 64-65, 77-78, 80-81, 85-85, 87-87, 90-90, 94-94, 96-99, 100-102, 104-105, 107-107, 113-113, 125-127, 129-133, 135-135, 138-138, 140-140, 147-147, 150-151, 153-153, 157-157, 161-162, 165-166, 181-181, 185-185, 189-190, 230-231, 235-235, 284-289, 291-293, 295-295, 297-297, 303-304, 306-309, 311-311, 322-322, 344-346, 350-350, 353-354, 365-365, 370-374, 375-375, 380-380
legacy/firmware/oled_text.h (2)
12-18: Clean struct design and function signature.The
string_lines_tstruct effectively encapsulates line management. The function signature forsplit_string_to_linesis well-designed with clear parameters.
1-2:Details
✅ Verification successful
Good architectural improvement!
The header guard rename from
__CHINESE_H__to__OLED_TEXT_H__reflects the broader scope of this module. This is a solid refactoring step.Verify that all files previously including
chinese.hhave been updated to includeoled_text.h:
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining references to the old chinese.h header # Expected: No references to chinese.h should remain echo "Checking for old chinese.h references..." rg -i "chinese\.h" --type c --type h echo "Checking for new oled_text.h includes..." rg "#include.*oled_text\.h" --type c --type hLength of output: 465
All includes updated and no old references remain
- No occurrences of
chinese.hfound.- Verified
oled_text.his included in:
- legacy/si2c.c
- legacy/firmware/layout2.h
- legacy/firmware/oled_text.c
- legacy/firmware/layout2.c
Great work refactoring and updating the includes—approving these changes.
legacy/firmware/i18n/locales/zh_cn.inc (1)
4-4: Improved clarity in setup instructions.The rewording makes the instructions more natural and user-friendly. Good localization work.
Also applies to: 14-14, 22-22
legacy/firmware/i18n/locales/ko_kr.inc (1)
14-14:Details
✅ Verification successful
Verify placeholder formatting consistency.
The Korean strings use
{}placeholders which should match the format used in other locales.Check that all placeholder formats are consistent across locales:
Also applies to: 45-45
🏁 Script executed:
#!/bin/bash # Description: Verify placeholder consistency # Expected: All locales should use the same placeholder format echo "Checking placeholder patterns in Korean..." rg '\{\}' legacy/firmware/i18n/locales/ko_kr.inc | wc -l echo "Checking placeholder patterns in other locales..." rg '\{\}' legacy/firmware/i18n/locales/zh_cn.inc | wc -l rg '\{\}' legacy/firmware/i18n/locales/de.inc | wc -lLength of output: 386
Placeholder formatting is consistent
I verified 34 occurrences of
{}in ko_kr.inc, zh_cn.inc, and de.inc. All locales use the same placeholder format. No changes needed.legacy/firmware/i18n/locales/de.inc (1)
6-8: Consistent German terminology improvements.Good standardization using "Brieftasche" instead of "Wallet" and other native German terms. This improves the user experience for German speakers.
Also applies to: 36-36
legacy/firmware/i18n/locales/es.inc (1)
4-4: LGTM! Spanish localization improvements look excellent.The text refinements improve clarity, grammar, and user experience. The updates make instructions clearer and terminology more consistent throughout the Spanish interface.
Also applies to: 8-8, 14-14, 17-17, 19-19, 25-25, 27-27, 29-29, 33-33, 36-36, 38-38, 40-40, 42-42, 43-43, 45-45, 48-48, 51-51, 52-52, 54-54, 65-65, 77-77, 78-78, 80-80, 81-81, 85-85, 87-87, 93-93, 94-94, 96-96, 97-97, 98-98, 99-99, 100-100, 101-101, 102-102, 104-104, 105-105, 107-107, 125-125, 126-126, 127-127, 128-128, 129-129, 130-130, 131-131, 132-132, 133-133, 135-135, 136-136, 138-138, 139-139, 140-140, 147-147, 150-150, 151-151, 153-153, 157-157, 161-161, 162-162, 165-165, 166-166, 181-181, 185-185, 187-187, 189-189, 190-190, 206-206, 215-215, 242-242, 260-260, 261-261, 284-284, 285-285, 286-286, 287-287, 288-288, 289-289, 291-291, 292-292, 293-293, 295-295, 297-297, 299-299, 303-303, 304-304, 306-306, 307-307, 308-308, 311-311, 316-316, 317-317, 321-321, 344-344, 345-345, 346-346, 348-348, 350-350, 353-353, 356-356, 357-357, 365-365, 370-370, 371-371, 372-372, 373-373, 375-375, 377-377, 380-380, 382-382
legacy/firmware/font_ex.c (6)
2-2: Good addition of common.h include.This likely provides access to the
ui_languagevariable used later in the code.
5-5: Well-defined fallback width constant.Using a named constant instead of magic numbers improves code readability and maintainability.
35-41: UTF-8 character navigation function looks correct.The implementation properly checks UTF-8 byte patterns to determine character boundaries. The logic correctly handles 1-4 byte UTF-8 sequences with appropriate bit masking.
154-158: Improved error handling in font_get_width.The function now properly checks if
font_get_infosucceeds before using the result. The fallback toFONT_EMPTY_WIDTHprovides better user experience when font data is unavailable.
171-172: Language-specific font height adjustment.The conditional height return based on
ui_languagesuggests English (language 0) uses different line spacing. This aligns with the internationalization improvements in the PR.
180-196: Enhanced buffer safety and fallback glyph handling.Good improvements:
- Precise byte length calculation prevents buffer overflows
- Static fallback glyph (☒) provides visual feedback for missing characters
- Increased buffer size from 24 to 32 bytes supports larger glyphs
- Proper bounds checking before data read
legacy/firmware/i18n/locales/ja.inc (1)
4-4: LGTM! Japanese localization improvements enhance user experience.The text refinements improve clarity, grammar, and consistency throughout the Japanese interface. Instructions are clearer and terminology is more standardized.
Also applies to: 5-5, 8-8, 14-14, 17-17, 19-19, 22-22, 24-24, 25-25, 27-27, 29-29, 33-33, 36-36, 38-38, 40-40, 42-42, 43-43, 45-45, 48-48, 51-51, 52-52, 54-54, 77-77, 78-78, 80-80, 81-81, 87-87, 93-93, 94-94, 96-96, 97-97, 98-98, 99-99, 100-100, 101-101, 102-102, 104-104, 105-105, 107-107, 124-124, 125-125, 126-126, 127-127, 128-128, 129-129, 130-130, 131-131, 132-132, 133-133, 135-135, 138-138, 139-139, 140-140, 147-147, 150-150, 151-151, 153-153, 157-157, 161-161, 162-162, 165-165, 166-166, 174-174, 181-181, 185-185, 189-189, 190-190, 205-205, 215-215, 242-242, 260-260, 261-261, 281-281, 284-284, 285-285, 286-286, 287-287, 288-288, 289-289, 291-291, 292-292, 293-293, 295-295, 297-297, 299-299, 302-302, 303-303, 304-304, 306-306, 307-307, 308-308, 311-311, 316-316, 317-317, 321-321, 344-344, 345-345, 346-346, 350-350, 353-353, 356-356, 357-357, 358-358, 364-364, 365-365, 370-370, 371-371, 372-372, 373-373, 375-375, 380-380
legacy/firmware/i18n/locales/zh_tw.inc (1)
4-4: LGTM! Traditional Chinese localization improvements look great.The text refinements enhance clarity, grammar, and consistency throughout the Traditional Chinese interface. Users will benefit from clearer instructions and standardized terminology.
Also applies to: 8-8, 14-14, 17-17, 19-19, 22-22, 24-24, 25-25, 27-27, 29-29, 33-33, 36-36, 38-38, 40-40, 42-42, 43-43, 45-45, 51-51, 52-52, 54-54, 58-58, 77-77, 78-78, 80-80, 81-81, 85-85, 87-87, 94-94, 96-96, 97-97, 98-99, 100-100, 101-101, 102-102, 104-104, 105-105, 107-107, 125-125, 126-126, 127-127, 128-128, 129-129, 130-130, 131-131, 135-135, 138-138, 140-140, 147-147, 150-150, 157-157, 161-161, 165-165, 166-166, 181-181, 185-185, 189-189, 190-190, 235-235, 242-242, 260-260, 261-261, 269-269, 284-284, 285-285, 286-286, 287-287, 289-289, 291-291, 292-292, 293-293, 295-295, 297-297, 302-302, 303-303, 304-304, 306-306, 308-308, 311-311, 317-317, 321-321, 344-344, 345-345, 346-346, 348-348, 350-350, 357-357, 358-358, 371-371, 372-372, 373-373, 380-380
legacy/firmware/layout2.c (1)
2305-2310:⚠️ Potential issue
line_index()can underrun whenlines == 0When
linesis 0 we hit
split_lines.line_start[line_index] - textwhereline_indexbecomes-1.
That yields UB because the index is unsigned after implicit conversion.Guard against this early and keep the math simple.
- int line_index = - lines > split_lines.line_count ? split_lines.line_count - 1 : lines; - return split_lines.line_start[line_index] - text; + if (lines <= 0) return 0; + int idx = lines > split_lines.line_count ? split_lines.line_count - 1 : lines; + return (int)(split_lines.line_start[idx] - text);Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (4)
legacy/firmware/i18n/locales/zh_cn.inc (1)
4-259: 🧹 Nitpick (assertive)Ensure consistent placeholder spacing & punctuation
The updated strings look clear. Please verify that all{}placeholders have spaces around them (e.g., line 17) and that each sentence ends with a full stop (。) or question mark (？) as appropriate. Also, keep uniform spacing around English words like “USB” (e.g., line 78 should be “您要禁用 USB 锁定吗？”).legacy/firmware/i18n/keys.h (1)
8-549: 🧹 Nitpick (assertive)Standardize comment grammar & style
Several comment tweaks introduce minor grammar slips.
- Line 8: “Next, Follow” → lowercase “follow”.
- Line 18: “recovery phrase” → plural “phrases”.
- Line 53: start with uppercase “Incorrect”.
Please start each comment with a capital letter and end with a period.legacy/firmware/i18n/locales/zh_tw.inc (1)
4-262: 🧹 Nitpick (assertive)Align placeholder spacing & punctuation
New traditional Chinese strings are clear. Ensure every{}placeholder has a space before and after (e.g., line 14 vs. line 17) and that each line consistently ends with。or？. Also keep uniform font for English terms like “USB” and proper spacing.legacy/firmware/layout2.c (1)
336-336:⚠️ Potential issueFix incomplete parameter formatting
Line appears to have incomplete or incorrect spacing in the function call parameters.
- oledDrawStringAdapter(x, 20, line1, FONT_STANDARD); + oledDrawStringAdapter(x, 20, line1, FONT_STANDARD);
♻️ Duplicate comments (10)
legacy/firmware/oled_text.h (1)
10-10: 🧹 Nitpick (assertive)Document the MAX_SPLIT_LINES constant.
The value 16 needs context. Why 16 lines specifically?
+// Maximum number of lines supported for text splitting on OLED display #define MAX_SPLIT_LINES 16legacy/firmware/i18n/locales/de.inc (2)
72-72: 🧹 Nitpick (assertive)Fix abbreviation inconsistency.
Line 72 uses "Std" while other time units spell out the full word.
- "{} Std", + "{} Stunden",
141-144: 🧹 Nitpick (assertive)Standardize input switching labels.
The input switching labels have inconsistent translation patterns. Some use "umschalten", others use "wechseln", and line 143 uses "Schalteingang".
Standardize all input switching phrases:
- "Eingabe umschalten (Kleinbuchstaben)", + "Eingabe wechseln (Kleinbuchstaben)", "Eingabe wechseln (Großbuchstaben)", - "Schalteingang (Nummer)", + "Eingabe wechseln (Nummer)", "Eingabe wechseln (Symbol)",legacy/firmware/layout2.c (4)
211-214: Makecountlines()acceptconst char *The function never modifies its input. Change the parameter type to improve code clarity and catch unintended mutations.
-static int countlines(char *text) { +static int countlines(const char *text) { string_lines_t lines = split_string_to_lines(text, OLED_WIDTH, FONT_STANDARD); return lines.line_count; }
706-711: Remove duplicate refresh callThe
layoutDialogCenterAdapterV2()already callsoledRefresh(). The extra refresh doubles SPI traffic and causes flicker.- oledBox(x, offset, x + l, offset, true); - oledRefresh(); + oledBox(x, offset, x + l, offset, true);
641-642:⚠️ Potential issueAdd bounds checking for height and offset calculations
The
font_get_height()may return 0 or values larger than expected. The offset calculation can push drawing outside the display bounds.- int height = font_get_height(), offset = 0; + int height = MAX(1, font_get_height()), offset = 0;And after line 709:
offset += height * line_num; + if (offset >= OLED_HEIGHT) offset = OLED_HEIGHT - 1;Also applies to: 707-710
722-729:⚠️ Potential issueApply same bounds checking for offset
This section has the same unbounded offset calculation issue.
offset += height * line_num; + if (offset >= OLED_HEIGHT) offset = OLED_HEIGHT - 1;legacy/firmware/oled_text.c (3)
35-46: Mark the pointer asconstand drop dead code.
is_symbols()never mutatesc.
Make itconst uint8_t *c(orconst char *c) to prevent accidental writes and silence warnings.
The helper is still unused in this compilation unit; consider removing it to save flash.
48-68: Width of CJK/KR glyphs still ignoresFONT_DOUBLE.
get_char_width()multiplies ASCII widths byzoom, yet returns the raw bitmap width for multibyte glyphs (char_data[0]) and for thefont_get_width()fall-back.
This causes mis-alignment whenFONT_DOUBLEis active.
217-220: Respect caller’s left margin on explicit line breaks.Resetting
cursor_xto0ignores the startingxoffset.
Use the originalxinstead so manual and wrapped line breaks align.- cursor_x = 0; + cursor_x = x;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (17)
legacy/firmware/Makefile(1 hunks)legacy/firmware/font_ex.c(4 hunks)legacy/firmware/font_ex.h(1 hunks)legacy/firmware/i18n/keys.h(6 hunks)legacy/firmware/i18n/locales/de.inc(5 hunks)legacy/firmware/i18n/locales/en.inc(9 hunks)legacy/firmware/i18n/locales/es.inc(10 hunks)legacy/firmware/i18n/locales/ja.inc(9 hunks)legacy/firmware/i18n/locales/ko_kr.inc(1 hunks)legacy/firmware/i18n/locales/pt_br.inc(10 hunks)legacy/firmware/i18n/locales/zh_cn.inc(8 hunks)legacy/firmware/i18n/locales/zh_tw.inc(9 hunks)legacy/firmware/layout2.c(12 hunks)legacy/firmware/layout2.h(1 hunks)legacy/firmware/oled_text.c(4 hunks)legacy/firmware/oled_text.h(1 hunks)legacy/si2c.c(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Style check
- GitHub Check: Gen check
- GitHub Check: Defs check
🔇 Additional comments (16)
legacy/firmware/layout2.h (1)
28-28: Add UTF-8 text rendering header
Includingoled_text.his correct for the new multi-line rendering API. Double-check that the legacychinese.hinclude has been removed elsewhere to avoid unused dependencies.Would you like a quick grep to confirm removal of
chinese.hacross the codebase?legacy/si2c.c (1)
9-9: LGTM! Header include updated correctly.The replacement of
chinese.hwitholed_text.haligns with the UTF-8 aware text rendering refactoring.legacy/firmware/Makefile (1)
64-64: Build system properly updated.Correctly replaces
chinese.owitholed_text.oto match the source code refactoring.legacy/firmware/font_ex.h (1)
33-33: New UTF-8 helper function added.The
utf8_nextfunction declaration supports proper UTF-8 character iteration. Good addition for the UTF-8 aware text rendering.legacy/firmware/i18n/locales/en.inc (2)
33-33: 🧹 Nitpick (assertive)Fix spacing in error message.
Missing space after "PIN!" and before the next sentence.
- "PIN not match!\nTry again.", + "PIN does not match!\nTry again.",Likely an incorrect or invalid review comment.
24-24: 🧹 Nitpick (assertive)Add missing space.
Missing space after "word!" and before "check".
- "Incorrect word! check your backup and try again.", + "Incorrect word! Check your backup and try again.",Likely an incorrect or invalid review comment.
legacy/firmware/oled_text.h (3)
1-2: Header guard properly updated.Good change from
__CHINESE_H__to__OLED_TEXT_H__to reflect the new purpose.
12-15: Well-designed struct for line management.The
string_lines_tstruct provides a clean interface for managing split text lines.
17-18: Function signature looks good.The
split_string_to_linesfunction has appropriate parameters for text splitting functionality.legacy/firmware/i18n/locales/pt_br.inc (1)
1-385: LGTM! Portuguese translations improved.The textual refinements enhance clarity and consistency throughout the Portuguese localization. Key improvements include better terminology (e.g., "Frase de Recuperação" instead of "Frase de Backup"), clearer instructions, and more formal tone where appropriate.
legacy/firmware/i18n/locales/es.inc (1)
1-385: LGTM! Spanish translations polished.The Spanish localization updates improve readability and consistency. Notable improvements include consistent use of lowercase "frase recuperación" and correction of missing translations like "Desactivado" on line 65.
legacy/firmware/i18n/locales/de.inc (1)
1-385: German translations look good overall.The German localization updates improve terminology consistency (e.g., "Wallet" → "Brieftasche") and enhance clarity throughout. Most changes are well-executed.
legacy/firmware/font_ex.c (2)
35-42: Good UTF-8 helper function addition.The
utf8_nextfunction correctly identifies UTF-8 byte sequences and advances the pointer by the appropriate number of bytes. This is more robust than the manual UTF-8 parsing it replaces.
154-159: Good fallback handling for missing glyphs.The code now returns a default width (
FONT_EMPTY_WIDTH) and empty glyph data when font info is unavailable. This prevents crashes or undefined behavior when encountering unsupported characters.Also applies to: 180-196
legacy/firmware/i18n/locales/ja.inc (1)
1-385: Great refinements—translations read clearly.The updated Japanese strings use consistent terminology, punctuation, and tone. They align well with other locales and the new UTF-8 line-wrapping logic.
legacy/firmware/layout2.c (1)
1641-1641:Details
❌ Incorrect review comment
Verify function call parameters
The
oledDrawStringCenterAdaptercall seems to be missing parameters or has incorrect syntax.
🏁 Script executed:
#!/bin/bash # Check the function signature for oledDrawStringCenterAdapter ast-grep --pattern 'oledDrawStringCenterAdapter($$$)' # Find the function definition rg -A 3 "oledDrawStringCenterAdapter.*\("Length of output: 23530
Ignore parameter concern: call matches signature
The functionoledDrawStringCenterAdapteris declared asvoid oledDrawStringCenterAdapter(int x, int y, const char *text, uint8_t font);and every call—including
oledDrawStringCenterAdapter(OLED_WIDTH / 2, y, title, FONT_STANDARD);—correctly provides those four arguments. You can safely ignore the missing-parameter warning.
Likely an incorrect or invalid review comment.
3 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation