refactor(core): simplify fonts handling #4194
Conversation
Introduce a new flag `_NAME` for each font and reduce the usage of `_ENABLE` flag to purely compilation guard. [no changelog]
[no changelog]
obrusvit
added
core
|
|
i think we will drop entire buffers.c/h when we get rid of old rendering, which will hopefully be soon enough. |
obrusvit
force-pushed
the
obrusvit/simplify-fonts-handling
branch
from
00e4847
to
1fce051
Compare
|
All UI-diffs on |
|
Can the change of order in translation jsons cause some problems moving forward @matejcik ? If so, I can revert it and just define the The reason to start from
|
|
yeah the ids are baked into the translation blobs, this would really mess up the upgrade/downgrade scenario. please revert. aside, I believe that there was a reason for skipping 0 for font index, but perhaps it doesn't apply anymore with this refactor? |
This commit removes the usage of macros for font data definitions. Instead, it includes data as const structs of newly introduced font_info_t type. [no changelog]
Change to the new structures and preserve manual changes. This commit also removes duplicated definition of nonprintable glyph for _UPPER fonts. [no changelog]
[no changelog]
obrusvit
force-pushed
the
obrusvit/simplify-fonts-handling
branch
from
1fce051
to
94db1ee
Compare
|
I reverted the re-assignment of font IDs. The PR is ready for review. For future reference: after we drop |
There was a problem hiding this comment.
overall comment: why not get rid of font_id_t and instead use const struct font_info_t* pervasively? then it's the responsibility of the (first) caller to pass in a valid font_info_t, and everyone else can look at the attributes directly.
then instead of FONT_NORMAL being an enum value, you would use &FONT_NORMAL as a pointer to a const struct font_info_t in all places
in Rust you would change pub enum Font to
pub type FontInfo = ffi::font_info_t;
pub type Font = &'static FontInfo;
impl font to impl fontinfo, modify the self owning arguments to references because FontInfo is no longer Copy/Clone, adjust the rest...
one thing we'd need to do is keep the mapping to translation blob font ids somewhere. presumably only in Rust? because it's only Rust that interfaces with translation blobs
| @@ -0,0 +1,34 @@ | |||
| #ifndef _FONTS_TYPES_H | |||
There was a problem hiding this comment.
any particular reason you made this a separate file, instead of putting the data into fonts.h ?
|
|
||
| font_glyph_iter_t font_glyph_iter_init(const int font, const uint8_t *text, | ||
| int font_height(font_id_t font_id) { | ||
| const font_info_t *font_info = get_font_info(font_id); | ||
| return font_info ? font_info->height : 0; | ||
| } | ||
|
|
||
| int font_max_height(font_id_t font) { | ||
| const font_info_t *font_info = get_font_info(font); | ||
| return font_info ? font_info->max_height : 0; | ||
| } | ||
|
|
||
| int font_baseline(font_id_t font) { | ||
| const font_info_t *font_info = get_font_info(font); | ||
| return font_info ? font_info->baseline : 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
kind of feels like those should be removed
|
How are we with this one? @obrusvit |
This PR aims to further simplify the font handling in
core.The main objective was to get rid of the numerous macros, which are quite cumbersome to work with. This is achieved by introducing
struct font_info_twhich is filled with needed data for each font during font generation bygen_font.py. Then there is a functionget_font_infodefined In thecore/embed/lib/fonts/fonts.cwhich is used to access font data.Unfortunately, I was unable to get rid of the
FONT_MAX_HEIGHTcalculation as it must be done in compile time. However, there is only one usage so getting rid of it is perhaps feasible.trezor-firmware/core/embed/lib/buffers.h
Line 32 in 02533aa
TODO:
_UPPERfonts seems not effective