Add custom icon fonts to lvgl-module#499
Conversation
📝 WalkthroughWalkthroughAdds a CMake macro for conditional module naming; adds a Python script to generate LVGL fonts and symbol headers; introduces three symbol header files (shared, statusbar, launcher) and three LVGL font source files (16px, 20px, 36px). Removes twelve TT_ASSETS app icon macros. Updates many app manifests and UI sources to use LVGL_SYMBOL_* constants, applies symbol font selection/styling, and adjusts launcher/app list icon handling. 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tactility/Source/app/i2cscanner/I2cScanner.cpp (1)
4-4:⚠️ Potential issue | 🟡 MinorRemove stale
#include <Tactility/Assets.h>from line 4.No
TT_ASSETS_*symbols remain in this file after replacingTT_ASSETS_APP_ICON_I2C_SETTINGSwithLVGL_SYMBOL_DEVICE_HUB. The include is no longer needed.
🧹 Nitpick comments (7)
Modules/lvgl-module/CMakeLists.txt (1)
26-26: Remove the-Dprefix —target_compile_definitionsadds it automatically.CMake's
target_compile_definitionswill strip a leading-D, so this works, but it's non-idiomatic and may confuse readers. The quotes are also unnecessary here.Suggested fix
-target_compile_definitions(${MODULE_NAME} PUBLIC "-DLV_LVGL_H_INCLUDE_SIMPLE") +target_compile_definitions(${MODULE_NAME} PUBLIC LV_LVGL_H_INCLUDE_SIMPLE)Modules/lvgl-module/Assets/generate-all.py (2)
1-1: Remove unnecessary semicolon.Fix
-import os; +import os
23-30: Use idiomaticnot inoperator.Fix
- if not safe_name in codepoints: + if safe_name not in codepoints:Tactility/Source/app/systeminfo/SystemInfo.cpp (1)
5-5: Remove stale#include <Tactility/Assets.h>.This include is no longer used. After replacing
TT_ASSETS_APP_ICON_SYSTEM_INFOwithLVGL_SYMBOL_MONITORING, there are no remaining references to any Assets.h symbols in this file.Modules/lvgl-module/Include/tactility/lvgl_fonts.h (1)
13-13: Remove commented-out code or add context.Line 13 has a commented-out macro with no explanation of why it's disabled or when it should be re-enabled. Consider removing it to reduce noise, or add a note explaining its purpose.
Modules/lvgl-module/Source/tactility/material_symbols_shared_16.c (1)
1-5: Generated file contains local filesystem path.Line 4 embeds the full local path
/home/ken/Projects/Tactility/...from the generation command. This is harmless but leaks developer environment details. Consider stripping the-opath from the opts comment, or documenting just the relevant generation parameters.Modules/lvgl-module/Include/tactility/lvgl_symbols_shared.h (1)
1-34: Consider adding a header comment documenting the font source.The symbol definitions look correct — the UTF-8 encodings match the codepoints in the
material_symbols_shared_16font. A brief comment at the top indicating these are Material Design Symbols (and which font resource renders them) would help future contributors understand the relationship.Suggested addition
`#pragma` once +// Material Design Symbols - rendered by material_symbols_shared_16 font +// See: https://fonts.google.com/icons + `#define` LVGL_SYMBOL_MAIL "\xEE\x85\x99"
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)
Tactility/Source/app/launcher/Launcher.cpp (1)
62-64:⚠️ Potential issue | 🟡 MinorStale comment: icon images are no longer 40×40.
Button sizes were changed to 36 and 56 (lines 22, 24), but this comment still says "40x40".
Proposed fix
- // Ensure buttons are still tappable when the asset fails to load - // Icon images are 40x40, so we get some extra padding too + // Ensure buttons are still tappable when the symbol fails to render lv_obj_set_size(button_image, button_size, button_size);
🧹 Nitpick comments (2)
Tactility/Source/app/launcher/Launcher.cpp (1)
46-60: Text color and image recolor may be redundant for symbol-rendered icons.Line 48 sets
text_colorto the theme primary, which is the correct way to color LVGL symbol text rendered vialv_image. Theimage_recolorblock (lines 53–60) was meaningful for file-based images but is likely a no-op for symbol sources. Consider whether the recolor block can be removed now that icons are font-based, or add a brief comment explaining why both are kept (e.g., fallback for mixed usage).Modules/lvgl-module/Assets/generate-all.py (1)
1-1: Minor style issues flagged by Ruff.Line 1: trailing semicolon. Line 27:
not inis the idiomatic Python membership test.Proposed fix
-import os; +import os- if not safe_name in codepoints: + if safe_name not in codepoints:Also applies to: 27-27
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
Tactility/Source/app/settings/Settings.cpp (1)
20-28: Duplicated widget-creation logic across three files.This
createWidgetbody is identical tocreateAppWidgetin bothAppSettings.cpp(line 21-27) andAppList.cpp(line 19-25). Consider extracting a shared helper (e.g., in an LVGL utility header) to avoid maintaining the same icon-fallback + font-setting logic in three places.Modules/lvgl-module/Assets/generate-all.py (3)
1-1: Remove trailing semicolon.-import os; +import os
23-30: Use idiomaticnot inoperator.Line 27:
if not safe_name in codepointsshould beif safe_name not in codepointsper Python style (PEP 8 / Ruff E713).- if not safe_name in codepoints: + if safe_name not in codepoints:
132-134: Consider makingshared_symbol_font_sizesa plain list for consistency.The other font groups pass inline lists (
[20],[36]) togenerate_icon_fonts. Havingshared_symbol_font_sizesas a separate variable is fine, but the blank line between lines 131 and 132 visually detaches it from its symbol list. Minor readability nit.Modules/lvgl-module/Include/tactility/lvgl_symbols_shared.h (1)
1-44: LGTM — symbol definitions are correct and complete.All 42 macros correctly encode Material Symbol codepoints as 3-byte UTF-8 sequences in the Private Use Area (U+E000–U+F8FF), matching the 42 codepoints in the generated font file.
Minor nit: a few macros break the otherwise alphabetical ordering (
DEPLOYED_CODE/DOWNLOADafterFOLDERon Lines 19–20, andNAVIGATION/KEYBOARD_ALTappended at the end on Lines 41–42). Consider sorting them for easier maintenance.
1 participant
WIP
Summary by CodeRabbit
New Features
Style
Documentation