Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use textArea to check if strings are displayed outside allowed area #677

Merged
merged 5 commits into from
May 31, 2024
Merged

Use textArea to check if strings are displayed outside allowed area #677

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4f2cbfc
Add area & wrapping information to store_string_infos
dmorais-ledger May 27, 2024
e98a021
Detect when a 'hard caesura' occur
dmorais-ledger May 27, 2024
b5d8b6e
Check area for LNX/LNS+ devices too
dmorais-ledger May 27, 2024
e756322
Fix textWidth in draw_button & report strings out of area
dmorais-ledger May 30, 2024
a92d414
Changes related to the code review
dmorais-ledger May 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion lib_nbgl/src/nbgl_fonts.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,9 @@ 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);

// Used to detect when a hyphenation (caesura) has been forced.
bool hard_caesura = false;
#endif // BUILD_SCREENSHOTS

/**********************
Expand Down Expand Up @@ -723,6 +725,7 @@ uint16_t nbgl_getTextNbLinesInWidth(nbgl_font_id_e fontId,
const char *prevText = NULL;

#ifdef BUILD_SCREENSHOTS
hard_caesura = false;
last_nb_lines = 0;
last_nb_pages = 1;
last_bold_state = fontId == BAGL_FONT_OPEN_SANS_EXTRABOLD_11px_1bpp; // True if Bold
Expand Down Expand Up @@ -818,6 +821,10 @@ uint16_t nbgl_getTextNbLinesInWidth(nbgl_font_id_e fontId,
width = 0;
}
else {
#ifdef BUILD_SCREENSHOTS
// An hyphenation (caesura) has been forced.
hard_caesura = true;
#endif // BUILD_SCREENSHOTS
width = char_width;
}
nbLines++;
Expand Down
50 changes: 33 additions & 17 deletions lib_nbgl/src/nbgl_obj.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
**********************/
Expand Down Expand Up @@ -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
Copy link
Contributor

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); }

Copy link
Contributor Author

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:
bug_cta

(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...)

Copy link
Contributor

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 ;)

Copy link
Contributor Author

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.

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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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)) {
Expand Down
50 changes: 41 additions & 9 deletions lib_nbgl/src/nbgl_step.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

/**********************
Expand Down Expand Up @@ -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;
Expand All @@ -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
Copy link
Contributor

@nroggeman-ledger nroggeman-ledger May 31, 2024 •
edited
Loading

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); }

Copy link
Contributor Author

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.

= 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,
Expand Down
Loading