Add custom icon fonts to lvgl-module #499
Conversation
📝 WalkthroughWalkthroughThis pull request migrates the project from asset-based app icons to LVGL symbol-based icon definitions. It introduces a CMake macro for conditional module naming, adds a Python script to generate LVGL fonts and symbol headers from Material Design Icons, defines three new symbol header files (shared, statusbar, launcher) with Unicode escape sequences and font references, and generates three corresponding Material Symbols font files (16px, 20px, 36px). Six asset icon macros are removed from the public API. Multiple app manifests are updated to use the new LVGL_SYMBOL_* constants, and the Launcher app is refactored to apply symbol fonts with updated styling and sizing. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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"
| #pragma once | ||
|
|
||
| #define LVGL_SYMBOL_LOCATION_ON "\xEF\x87\x9B" | ||
| #define LVGL_SYMBOL_CLOUD "\xEF\x85\x9C" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if both headers define LVGL_SYMBOL_CLOUD and whether any file includes both
echo "=== Definitions of LVGL_SYMBOL_CLOUD ==="
rg -n 'LVGL_SYMBOL_CLOUD' --type=c --type=cpp -g '*.h'
echo ""
echo "=== Files including both headers ==="
rg -l 'lvgl_symbols_shared' --type=c --type=cpp | while read f; do
if rg -q 'lvgl_symbols_statusbar' "$f"; then
echo "$f includes both headers"
fi
doneRepository: TactilityProject/Tactility
Length of output: 358
🏁 Script executed:
# Also check if generate-all.py exists and verify the symbol lists
find . -name "generate-all.py" -o -name "*generate*.py" | head -20Repository: TactilityProject/Tactility
Length of output: 296
🏁 Script executed:
# Check the lvgl_symbols_shared.h file to confirm LVGL_SYMBOL_CLOUD definition
rg -A2 -B2 'LVGL_SYMBOL_CLOUD' Modules/lvgl-module/Include/tactility/lvgl_symbols_shared.hRepository: TactilityProject/Tactility
Length of output: 300
🏁 Script executed:
# Check the generate-all.py for the symbol lists mentioned
find . -name "generate-all.py" -exec cat {} \; 2>/dev/null | head -150Repository: TactilityProject/Tactility
Length of output: 5552
🏁 Script executed:
# Check if there's a consolidated header that includes both
rg -l 'lvgl_symbols_shared|lvgl_symbols_statusbar' --type=c --type=h | head -20Repository: TactilityProject/Tactility
Length of output: 52
🏁 Script executed:
# Check what includes lvgl_symbols_statusbar.h
rg -n 'lvgl_symbols_statusbar' --type=c --type=hRepository: TactilityProject/Tactility
Length of output: 52
🏁 Script executed:
# Check what includes lvgl_symbols_shared.h
rg -n 'lvgl_symbols_shared' --type=c --type=hRepository: TactilityProject/Tactility
Length of output: 52
🏁 Script executed:
# Check if these headers might be included with different patterns
rg 'lvgl_symbols' --type=c --type=hRepository: TactilityProject/Tactility
Length of output: 52
🏁 Script executed:
# Look at the actual header files to understand their purpose
cat Modules/lvgl-module/Include/tactility/lvgl_symbols_statusbar.hRepository: TactilityProject/Tactility
Length of output: 998
🏁 Script executed:
# Check if there's a master header that includes these
ls -la Modules/lvgl-module/Include/tactility/Repository: TactilityProject/Tactility
Length of output: 451
🏁 Script executed:
# Check if lvgl_module.h includes either of these headers
cat Modules/lvgl-module/Include/tactility/lvgl_module.hRepository: TactilityProject/Tactility
Length of output: 2082
🏁 Script executed:
# Check if there are any pragma guards or includes patterns we might have missed
cat Modules/lvgl-module/Include/tactility/lvgl_symbols_shared.hRepository: TactilityProject/Tactility
Length of output: 1502
LVGL_SYMBOL_CLOUD is defined in both this header and lvgl_symbols_shared.h.
Both shared_symbol_code_point_names and statusbar_symbol_code_point_names in generate-all.py include the "cloud" symbol, causing both headers to define identical macros. While no files currently include both headers together, this creates fragile code that could trigger macro redefinition errors if both headers are ever included in the same translation unit. Remove "cloud" from one of the two lists in generate-all.py.
| static void createAppWidget(const std::shared_ptr<AppManifest>& manifest, lv_obj_t* list) { | ||
| const void* icon = !manifest->appIcon.empty() ? manifest->appIcon.c_str() : TT_ASSETS_APP_ICON_FALLBACK; | ||
| const void* icon = !manifest->appIcon.empty() ? manifest->appIcon.c_str() : LVGL_SYMBOL_TOOLBAR; | ||
| lv_obj_t* btn = lv_list_add_button(list, icon, manifest->appName.c_str()); | ||
| lv_obj_t* image = lv_obj_get_child(btn, 0); | ||
| lv_obj_set_style_text_font(image, LVGL_SYMBOL_FONT_DEFAULT, LV_PART_MAIN); | ||
| lv_obj_add_event_cb(btn, &onAppPressed, LV_EVENT_SHORT_CLICKED, manifest.get()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -A10 'createAppWidget' --type=cppRepository: TactilityProject/Tactility
Length of output: 4944
🏁 Script executed:
rg -n 'TT_ASSETS_APP_ICON_FALLBACK' --type=cpp --type=h -B2 -A2Repository: TactilityProject/Tactility
Length of output: 1742
🏁 Script executed:
rg -n 'appIcon.*=' --type=cpp -B2 -A2 | head -50Repository: TactilityProject/Tactility
Length of output: 3839
Add font override for LVGL symbols in AppSettings.cpp and Settings.cpp.
The manifests now use LVGL_SYMBOL_* codepoints (e.g., LVGL_SYMBOL_DEVICE_HUB, LV_SYMBOL_WIFI) instead of PNG paths. These require the custom symbol font to render. AppList.cpp correctly sets LVGL_SYMBOL_FONT_DEFAULT on the image child (lines 23–24), but AppSettings.cpp:createAppWidget and Settings.cpp lack this override. Apps will display as missing glyphs or boxes in the app settings list.
Add the same two lines after lv_list_add_button:
lv_obj_t* image = lv_obj_get_child(btn, 0);
lv_obj_set_style_text_font(image, LVGL_SYMBOL_FONT_DEFAULT, LV_PART_MAIN);
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
1 participant
WIP
Summary by CodeRabbit
Release Notes
New Features
Style
Documentation