-
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
Changes from 4 commits
4f2cbfc
e98a021
b5d8b6e
e756322
a92d414
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. To remove, LOG_XXX are already used for verbose |
||
#endif // BUILD_SCREENSHOTS | ||
|
||
/** | ||
* @brief array of layouts, if used by modal | ||
|
@@ -2327,15 +2330,18 @@ int nbgl_layoutAddHeader(nbgl_layout_t *layout, const nbgl_layoutHeader_t *heade | |
textArea->textAlignment = CENTER; | ||
textArea->wrapping = true; | ||
// ensure that text fits on 2 lines maximum | ||
if (nbgl_getTextNbLinesInWidth(textArea->fontId, | ||
textArea->text, | ||
textArea->obj.area.width, | ||
textArea->wrapping) | ||
> 2) { | ||
LOG_WARN(LAYOUT_LOGGER, | ||
"nbgl_layoutAddHeader: text [%s] is too long for header\n", | ||
text); | ||
} | ||
#ifdef BUILD_SCREENSHOTS | ||
if (verbose) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To remove, LOG_XXX is already used for verbose |
||
#endif // BUILD_SCREENSHOTS | ||
if (nbgl_getTextNbLinesInWidth(textArea->fontId, | ||
textArea->text, | ||
textArea->obj.area.width, | ||
textArea->wrapping) | ||
> 2) { | ||
LOG_WARN(LAYOUT_LOGGER, | ||
"nbgl_layoutAddHeader: text [%s] is too long for header\n", | ||
text); | ||
} | ||
layoutInt->headerContainer->children[layoutInt->headerContainer->nbChildren] | ||
= (nbgl_obj_t *) textArea; | ||
layoutInt->headerContainer->nbChildren++; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,9 @@ | |
#include "nbgl_serialize.h" | ||
#include "os_io_seproxyhal.h" | ||
#endif | ||
#ifdef BUILD_SCREENSHOTS | ||
#include "json_scenario.h" | ||
#endif // BUILD_SCREENSHOTS | ||
|
||
/********************* | ||
* DEFINES | ||
|
@@ -73,11 +76,6 @@ extern bool last_bold_state, verbose; | |
**********************/ | ||
extern const char *get_ux_loc_string(uint32_t index); | ||
|
||
#ifdef BUILD_SCREENSHOTS | ||
char *get_printable_string(char *string); | ||
void store_string_infos(const char *text, uint16_t nb_lines, uint16_t nb_pages, bool bold); | ||
#endif // BUILD_SCREENSHOTS | ||
|
||
/********************** | ||
* GLOBAL FUNCTIONS | ||
**********************/ | ||
|
@@ -388,21 +386,32 @@ static void draw_button(nbgl_button_t *obj, nbgl_obj_t *prevObj, bool computePos | |
// draw the text (right of the icon, with 8 pixels between them) | ||
if (text != NULL) { | ||
nbgl_area_t rectArea; | ||
textWidth = nbgl_getTextWidth(obj->fontId, text); | ||
// Compute available with & height to display the text | ||
rectArea.x0 = obj->obj.area.x0; | ||
rectArea.y0 = obj->obj.area.y0; | ||
// Only one line of text | ||
rectArea.height = nbgl_getFontHeight(obj->fontId); | ||
rectArea.y0 += (obj->obj.area.height - rectArea.height) / 2; | ||
rectArea.width = obj->obj.area.width; | ||
if (obj->icon != NULL) { | ||
rectArea.x0 = obj->obj.area.x0 + obj->obj.area.width / 2 | ||
- (textWidth + obj->icon->width + 8) / 2 + obj->icon->width + 8; | ||
rectArea.x0 += obj->icon->width + 8; | ||
rectArea.width -= obj->icon->width + 8; | ||
} | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. A simple code like the following one would be way simplier: There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. (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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Please check my answer here. |
||
uint16_t textLen; | ||
nbgl_getTextMaxLenAndWidth(obj->fontId, text, rectArea.width, &textLen, &textWidth, true); | ||
|
||
#ifdef BUILD_SCREENSHOTS | ||
store_string_infos(text, obj->fontId, &rectArea, true, 1, 1, false); | ||
#endif // BUILD_SCREENSHOTS | ||
|
||
// Center the text, horizontally | ||
if (textWidth < rectArea.width) { | ||
rectArea.x0 += (rectArea.width - textWidth) / 2; | ||
} | ||
LOG_DEBUG(OBJ_LOGGER, "draw_button(), text = %s\n", text); | ||
rectArea.y0 | ||
= obj->obj.area.y0 + (obj->obj.area.height - nbgl_getFontHeight(obj->fontId)) / 2; | ||
rectArea.width = textWidth; | ||
rectArea.height = nbgl_getFontHeight(obj->fontId); | ||
rectArea.backgroundColor = obj->innerColor; | ||
nbgl_drawText(&rectArea, text, nbgl_getTextLength(text), obj->fontId, obj->foregroundColor); | ||
nbgl_drawText(&rectArea, text, textLen, obj->fontId, obj->foregroundColor); | ||
} | ||
// draw the icon, if any | ||
if (obj->icon != NULL) { | ||
|
@@ -920,7 +929,13 @@ static void draw_textArea(nbgl_text_area_t *obj, nbgl_obj_t *prevObj, bool compu | |
if (obj->autoHideLongLine == true) { | ||
#ifdef BUILD_SCREENSHOTS | ||
nbgl_getTextNbLinesInWidth(fontId, text, obj->obj.area.width, obj->wrapping); | ||
store_string_infos(text, last_nb_lines, last_nb_pages, last_bold_state); | ||
store_string_infos(text, | ||
fontId, | ||
&obj->obj.area, | ||
obj->wrapping, | ||
last_nb_lines, | ||
last_nb_pages, | ||
last_bold_state); | ||
#endif // BUILD_SCREENSHOTS | ||
textWidth = nbgl_getSingleLineTextWidth(fontId, text); | ||
if (textWidth > obj->obj.area.width) { | ||
|
@@ -949,7 +964,8 @@ static void draw_textArea(nbgl_text_area_t *obj, nbgl_obj_t *prevObj, bool compu | |
// get nb lines in the given width (depending of wrapping) | ||
nbLines = nbgl_getTextNbLinesInWidth(fontId, text, obj->obj.area.width, obj->wrapping); | ||
#ifdef BUILD_SCREENSHOTS | ||
store_string_infos(text, last_nb_lines, last_nb_pages, last_bold_state); | ||
store_string_infos( | ||
text, fontId, &obj->obj.area, obj->wrapping, last_nb_lines, last_nb_pages, last_bold_state); | ||
#endif // BUILD_SCREENSHOTS | ||
// saturate nb lines if nbMaxLines is greater than 0 | ||
if ((obj->nbMaxLines > 0) && (obj->nbMaxLines < nbLines)) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,9 @@ | |
#include "glyphs.h" | ||
#include "os_pic.h" | ||
#include "os_print.h" | ||
#ifdef BUILD_SCREENSHOTS | ||
#include "json_scenario.h" | ||
#endif // BUILD_SCREENSHOTS | ||
|
||
/********************* | ||
* DEFINES | ||
|
@@ -92,12 +95,6 @@ extern UX_LOC_STRINGS_INDEX last_string_id; | |
extern uint16_t last_nb_lines; | ||
extern uint16_t last_nb_pages; | ||
extern bool last_bold_state; | ||
|
||
/********************** | ||
* PROTOTYPES | ||
**********************/ | ||
void store_string_infos(const char *text, uint16_t nb_lines, uint16_t nb_pages_, bool bold); | ||
|
||
#endif // BUILD_SCREENSHOTS | ||
|
||
/********************** | ||
|
@@ -411,6 +408,13 @@ nbgl_step_t nbgl_stepDrawText(nbgl_stepPosition_t pos, | |
nbgl_contentCenteredInfoStyle_t style, | ||
bool modal) | ||
{ | ||
#ifdef BUILD_SCREENSHOTS | ||
// Initialize a default area | ||
nbgl_area_t area; | ||
area.x0 = area.y0 = 0; | ||
area.width = AVAILABLE_WIDTH; | ||
area.height = NB_MAX_LINES * nbgl_getFontLineHeight(BAGL_FONT_OPEN_SANS_REGULAR_11px_1bpp); | ||
#endif // BUILD_SCREENSHOTS | ||
StepContext_t *ctx = getFreeContext(TEXT_STEP, modal); | ||
if (!ctx) { | ||
return NULL; | ||
|
@@ -428,22 +432,50 @@ nbgl_step_t nbgl_stepDrawText(nbgl_stepPosition_t pos, | |
ctx->textContext.nbPages = nbgl_getTextNbPagesInWidth( | ||
BAGL_FONT_OPEN_SANS_REGULAR_11px_1bpp, text, NB_MAX_LINES, AVAILABLE_WIDTH); | ||
#ifdef BUILD_SCREENSHOTS | ||
store_string_infos(text, last_nb_lines, last_nb_pages, last_bold_state); | ||
store_string_infos(text, | ||
BAGL_FONT_OPEN_SANS_REGULAR_11px_1bpp, | ||
&area, | ||
true, | ||
last_nb_lines, | ||
last_nb_pages, | ||
last_bold_state); | ||
#endif // BUILD_SCREENSHOTS | ||
} | ||
else { | ||
#ifdef BUILD_SCREENSHOTS | ||
uint16_t nb_lines_title; | ||
// Call this function to get correct nb_lines/nb_pages for text field. | ||
nbgl_getTextNbPagesInWidth( | ||
BAGL_FONT_OPEN_SANS_REGULAR_11px_1bpp, text, NB_MAX_LINES, AVAILABLE_WIDTH); | ||
store_string_infos(text, last_nb_lines, last_nb_pages, last_bold_state); | ||
nb_lines_title = last_nb_lines; | ||
// There is a sub text, so no more than 3 lines for title... | ||
if (nb_lines_title > 3) { | ||
nb_lines_title = 3; | ||
} | ||
area.height | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A simple code like the following one would be way simplier: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please STOP with LOG_WARN!!
It is necessary to have a kind of CI for strings and this PR will make it possible. |
||
= nb_lines_title * nbgl_getFontLineHeight(BAGL_FONT_OPEN_SANS_REGULAR_11px_1bpp); | ||
store_string_infos(text, | ||
BAGL_FONT_OPEN_SANS_REGULAR_11px_1bpp, | ||
&area, | ||
true, | ||
last_nb_lines, | ||
last_nb_pages, | ||
last_bold_state); | ||
#endif // BUILD_SCREENSHOTS | ||
// NB_MAX_LINES-1 because first line is for main text | ||
ctx->textContext.nbPages = nbgl_getTextNbPagesInWidth( | ||
BAGL_FONT_OPEN_SANS_REGULAR_11px_1bpp, subText, NB_MAX_LINES - 1, AVAILABLE_WIDTH); | ||
#ifdef BUILD_SCREENSHOTS | ||
area.height = (NB_MAX_LINES - nb_lines_title) | ||
* nbgl_getFontLineHeight(BAGL_FONT_OPEN_SANS_REGULAR_11px_1bpp); | ||
// If subTxtid is not valid, we'll use txtId for nb_lines/nb_pages values | ||
store_string_infos(subText, last_nb_lines, last_nb_pages, last_bold_state); | ||
store_string_infos(subText, | ||
BAGL_FONT_OPEN_SANS_REGULAR_11px_1bpp, | ||
&area, | ||
true, | ||
last_nb_lines, | ||
last_nb_pages, | ||
last_bold_state); | ||
#endif // BUILD_SCREENSHOTS | ||
} | ||
LOG_DEBUG(STEP_LOGGER, | ||
|
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