-
Notifications
You must be signed in to change notification settings - Fork 12
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
Use textArea to check if strings are displayed outside allowed area #677
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #677 +/- ##
==========================================
+ Coverage 60.47% 60.54% +0.06%
==========================================
Files 12 12
Lines 1723 1726 +3
==========================================
+ Hits 1042 1045 +3
Misses 681 681
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
lib_nbgl/src/nbgl_fonts.c
Outdated
@@ -97,7 +97,7 @@ const unsigned int C_unicode_characters_count | |||
uint16_t last_nb_lines = 0; | |||
uint16_t last_nb_pages = 0; | |||
bool last_bold_state = false; | |||
void store_string_infos(char *text, uint16_t nb_lines, uint16_t nb_pages, bool bold); | |||
bool hard_caesura = false; |
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.
Explain usage
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.
This simple line is used to detect when a hyphenation (caesura) has been forced.
That solves the problem mentioned there: https://ledgerhq.atlassian.net/wiki/spaces/FW/pages/4705091649/NBGL+improvements+suggestions#Gestion-de-la-c%C3%A9sure-sur-les-mots-trop-longs
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.
Please add comments in code, it will be easier for next deveoppers
lib_nbgl/src/nbgl_layout.c
Outdated
@@ -100,6 +100,9 @@ typedef struct { | |||
/********************** | |||
* VARIABLES | |||
**********************/ | |||
#ifdef BUILD_SCREENSHOTS | |||
extern bool verbose; |
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.
To remove, LOG_XXX are already used for verbose
lib_nbgl/src/nbgl_layout.c
Outdated
text); | ||
} | ||
#ifdef BUILD_SCREENSHOTS | ||
if (verbose) |
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.
To remove, LOG_XXX is already used for verbose
} | ||
else { | ||
rectArea.x0 = obj->obj.area.x0 + (obj->obj.area.width - textWidth) / 2; | ||
// Compute the width & number of characters displayed on first line |
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.
A simple code like the following one would be way simplier:
if (nbgl_getTextNbLinesInWidth(textArea->fontId, textArea->text, textArea->obj.area.width, textArea->wrapping) > 1) { LOG_WARN(OBJ_LOGGER, "draw_button: text [%s] is too long for button\n", text); }
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.
The changes here not only make it possible to inform language managers there is an issue with the string, but also make sure that no text is displayed outside of the button, like explained here.
Without the changes I made, a string too long will be displayed like this:
(please note that the characters were displayed not only outside the button, but also outside the front screen, on the side screen - I wonder what the result would be on a real device...)
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.
My remark also applies to width... a LOG_WARN is also enough for that... and much simpler ;)
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.
Please check my answer here.
if (nb_lines_title > 3) { | ||
nb_lines_title = 3; | ||
} | ||
area.height |
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.
A simple code like the following one would be way simplier:
if (nbgl_getTextNbLinesInWidth(textArea->fontId, textArea->text, textArea->obj.area.width, textArea->wrapping) > 3) { LOG_WARN(STEP_LOGGER, "nbgl_stepDrawText: text [%s] is too long for title\n", text); }
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.
Please STOP with LOG_WARN!!
- LOG_WARN will not make the build fail, which is necessary if a string contains an error.
- LOG_WARN will not inform language managers there is an issue with some string(s).
It is necessary to have a kind of CI for strings and this PR will make it possible.
Description
This PR use textArea to check if strings are displayed outside allowed area