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.

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

refactor(core): simplify fonts handling #4194

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

obrusvit
Copy link
Contributor

@obrusvit obrusvit commented Sep 18, 2024

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_t which is filled with needed data for each font during font generation by gen_font.py. Then there is a function get_font_info defined In the core/embed/lib/fonts/fonts.c which is used to access font data.

Unfortunately, I was unable to get rid of the FONT_MAX_HEIGHT calculation as it must be done in compile time. However, there is only one usage so getting rid of it is perhaps feasible.

#if TEXT_BUFFER_HEIGHT < FONT_MAX_HEIGHT

TODO:

  • _UPPER fonts seems not effective
  • problems with translated strings

Introduce a new flag `_NAME` for each font and reduce the usage of
`_ENABLE` flag to purely compilation guard.

[no changelog]
@obrusvit obrusvit added core Trezor Core firmware. Runs on Trezor Model T and T2B1. code Code improvements labels Sep 18, 2024
@obrusvit obrusvit self-assigned this Sep 18, 2024
Copy link

github-actions bot commented Sep 18, 2024

core UI changes device test click test persistence test
T2T1 Model T test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3B1 Safe 3 test(screens) main(screens) test(screens) main(screens) 2724
T3T1 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

@TychoVrahe
Copy link
Contributor

i think we will drop entire buffers.c/h when we get rid of old rendering, which will hopefully be soon enough.

@obrusvit obrusvit force-pushed the obrusvit/simplify-fonts-handling branch from 00e4847 to 1fce051 Compare September 19, 2024 11:01
@obrusvit
Copy link
Contributor Author

All UI-diffs on TS3 seems to be caused by now usage of BOLD font in Size: x bytes which is actually correct.
image

@obrusvit
Copy link
Contributor Author

obrusvit commented Sep 19, 2024

Can the change of order in translation jsons cause some problems moving forward @matejcik ? If so, I can revert it and just define the enum font_id_t starting from 1 and not change the order. I simply though that the new ordering is a little bit more logical, e.g. having NORMAL and NORMAL_UPPER in succession.

The reason to start from 0 is to use the last enum variant FONTS_COUNT.

image

@obrusvit obrusvit marked this pull request as ready for review September 19, 2024 11:38
@matejcik
Copy link
Contributor

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]
@obrusvit obrusvit force-pushed the obrusvit/simplify-fonts-handling branch from 1fce051 to 94db1ee Compare September 19, 2024 13:15
@obrusvit
Copy link
Contributor Author

obrusvit commented Sep 19, 2024

I reverted the re-assignment of font IDs. The PR is ready for review.

For future reference: after we drop buffers.h/.c when getting rid of "old rendering", then we can completely remove these definitions for all fonts (also remove this from gen_font.py and also remove the FONT_MAX_HEIGHT calculation in fonts.h
#define Font_PixelOperatorMono_Regular_8_MAX_HEIGHT 8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Code improvements core Trezor Core firmware. Runs on Trezor Model T and T2B1.
Projects
Status: 🔎 Needs review
Development

Successfully merging this pull request may close these issues.

3 participants