-
-
Notifications
You must be signed in to change notification settings - Fork 157
fix: black header and unreadable popup menus #535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: black header and unreadable popup menus #535
Conversation
EdgeTX has finally depreciated some colour constants from OpenTX days, which would result in black header and hard to read popup menus.
WalkthroughThe PR updates color theme defaults across radio and UI display configurations by introducing Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/SCRIPTS/BF/ui.lua (1)
184-185: Consider adding final fallback values for robustness.Unlike the module-level color variables (lines 38-41) which have final fallback values (ERASE, SOLID, 0), these color expressions lack a final default. If both
COLOR_THEME_SECONDARY1andTITLE_BGCOLORare undefined, line 184 could passniltolcd.drawFilledRectangle, potentially causing a runtime error.Apply this diff to add final fallbacks:
- lcd.drawFilledRectangle(0, 0, LCD_W, 30, COLOR_THEME_SECONDARY1 or TITLE_BGCOLOR) - lcd.drawText(5,5,screenTitle, COLOR_THEME_PRIMARY2 or MENU_TITLE_COLOR) + lcd.drawFilledRectangle(0, 0, LCD_W, 30, COLOR_THEME_SECONDARY1 or TITLE_BGCOLOR or 0) + lcd.drawText(5,5,screenTitle, COLOR_THEME_PRIMARY2 or MENU_TITLE_COLOR or 0)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/SCRIPTS/BF/radios.lua(4 hunks)src/SCRIPTS/BF/ui.lua(2 hunks)
🔇 Additional comments (2)
src/SCRIPTS/BF/radios.lua (1)
91-91: LGTM! Consistent color theme updates across all high-resolution displays.The addition of
COLOR_THEME_SECONDARY1as the primary color fallback for the "No Telemetry" indicator is consistent across all high-resolution display configurations (480x272, 480x320, 320x480, 320x240) and aligns well with theglobalTextOptionschanges in ui.lua. The three-level fallback chain properly maintains backward compatibility with OpenTX and older EdgeTX versions.Also applies to: 119-119, 147-147, 174-174
src/SCRIPTS/BF/ui.lua (1)
38-41: Color constant fallback chains are appropriately structured.The
COLOR_THEME_*constants (PRIMARY3, SECONDARY1, SECONDARY3) referenced in the fallback chains are valid, documented EdgeTX Lua color constants. The fallback pattern—prioritizing new theme constants before deprecated ones—follows sound practice for managing API evolution. No issues with the mappings or fallback logic.
If neither of these constants exist and the radio.highRes is true, something is quite wrong. So if a runtime error was to occur (don't think it will... probably returns 0 hence why things were black), this is actually good IMO as it will bring this to our attention that something has changed. |
|
@coderabbitai check #535 (comment) as this can be approved. |
|
@pfeerick, your reasoning makes sense. If both constants are undefined in a high-resolution context, it would indeed indicate a deeper system issue that should be surfaced rather than masked with a silent fallback. This fail-fast approach is reasonable for catching unexpected environment changes. @haslinghuis, understood. The PR properly addresses the deprecated EdgeTX color constants issue with tested changes across multiple devices. Approving now. ✅ Actions performedComments resolved. |
EdgeTX has finally depreciated some colour constants from OpenTX days, which would result in black header and hard to read popup menus with the Betaflight Lua script. This will take effect as of EdgeTX 2.11.4, and 2.12.0 onwards. I've left the old constants in so that it should (untested) continue to work on OpenTX and early EdgeTX versions.
Fixes #534, also mentioned in passing at #530 (comment)
Remapped as per https://luadoc.edgetx.org/lua-api-reference/constants/color-constants#deprecated-color-constants, and tested on TX16S (480x272) TX15 (480x320) and PA01(320x240). Some before and after screenshots from handsets below, running different themes (hence the different colours, now following theme correctly):
TX16S
TX15
PA01
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.