-
-
Notifications
You must be signed in to change notification settings - Fork 672
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 fonts handling to rust #4536
base: main
Are you sure you want to change the base?
Conversation
|
Unfortunately, designated initializer is not possible if gfx_text_attr_t normal_text_attr = {
.font = FONT_NORMAL,
.fg_color = COLOR_BL_FG,
.bg_color = COLOR_BL_BG,
}; Alternative for
|
I think we can revert to While we are structuring the fonts by layouts, maybe we can move the to the existing layout folders, i.e.:
|
- application layer should not deal with fonts at all - distinction between MONO and others is preserved by bool argument in `should_show_more` interpreted as `is_data` [no changelog]
[no changelog]
- usage with `--rust` [no changelog]
[no changelog]
- see #3771 [no changelog]
- the C fonts handling will be private impl used only in `prodtest` and `bootloader_ci` - use "bootloader_ci" as a separate stage to distinguish fonts [no changelog]
[no changelog]
- font usage changed from enum Font to pointers to FontInfo structs - components from all layouts and also common components changed [no changelog]
[no changelog]
5990e9d
to
a8aa2cb
Compare
Moving stuff around resulted in pretty messy fixups so I decided to rebase. Sorry for that. |
@@ -43,7 +43,7 @@ impl Component for Connect { | |||
} | |||
|
|||
fn render<'s>(&'s self, target: &mut impl Renderer<'s>) { | |||
let font = Font::NORMAL; | |||
let font = FONT_NORMAL; |
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.
should be passed in constructor
@@ -645,7 +648,7 @@ where | |||
) { | |||
let numeral = uformat!("{}.", n + 1); | |||
shape::Text::new(base_point, numeral.as_str()) | |||
.with_font(Font::SUB) | |||
.with_font(FONT_SUB) |
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.
should be passed in constructor
@@ -626,7 +629,7 @@ where | |||
} else { | |||
// current and future tasks - ordinal numbers or icon on current task | |||
if self.show_numerals { | |||
let num_offset = Offset::new(4, Font::NORMAL.visible_text_height("1")); | |||
let num_offset = Offset::new(4, FONT_NORMAL.visible_text_height("1")); |
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.
should be passed in constructor
@@ -34,7 +34,7 @@ impl<'a> Text<'a> { | |||
text, | |||
color: Color::white(), | |||
alpha: 255, | |||
font: Font::NORMAL, | |||
font: FONT_NORMAL, |
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.
should be mandatory parameter and we can get rid of the with_font below
@@ -44,3 +44,16 @@ pub type ModelUI = crate::ui::layout_caesar::UICaesar; | |||
|
|||
#[cfg(feature = "layout_bolt")] | |||
pub type ModelUI = crate::ui::layout_bolt::UIBolt; | |||
|
|||
// Re-export fonts for each layout |
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.
shouldn't be exported outside of layouts
@@ -0,0 +1,21 @@ | |||
#[cfg(all( |
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.
what is this file? some remnant to be deleted?
@click.option( | ||
"--rust", | ||
is_flag=True, | ||
default=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.
should probably be default now
@@ -189,11 +189,8 @@ async def show_tx_init(title: str) -> bool: | |||
should_show_details = await layouts.should_show_more( | |||
TR.cardano__confirm_transaction, | |||
( |
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.
probably you need more parameters for the para array, otherwise what previously was demibold is now normal. is_data selects mono font so perhaps is_title or something
(ui.NORMAL, TR.ethereum__name_and_version), | ||
(ui.DEMIBOLD, domain_name), | ||
(ui.DEMIBOLD, domain_version), | ||
(TR.ethereum__name_and_version, 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.
also here: probably you need more parameters for the para array, otherwise what previously was demibold is now normal. is_data selects mono font so perhaps is_title or something
As for Regarding the stage-only fonts. I guess compiler will eliminate unused font data now, but it is fragile - once you add a font in some shared component, the code will bloat. I would aim for feature-gating the firmware-only fonts with |
Following #4194 (review)
This PR changes the font handling from
enum Font
to direct usage ofstruct FontInfo
. While doing so, it became apparent that generating rust files instead of C is a better way go. The main problem was thatconst
C variables are bound tostatic
variables in Rust (andstatic
variables cannot be used inconst
context). Each font is supplied in the following struct:The benefits are:
enum Font
allows more flexible fonts handling - each layout can now specify the fonts and their operational names on their ownenum Font
Font
inFormattedText
where it should be replaced byTextStyle
Paragraphs
- usage inChecklist
, which should be separated by layouts or implemented throughFormattedText
enum
value to some data which might benull
), this results in removing someunsafe
blocks andunwrap
sThe drawbacks are:
prodtest
andbootloader_ci
. This will be a private impl.core/embed/rust/src/ui/layout_xyz/fonts/mod.rs
Measurement of
FLASH
size onT3T1
showed negligible increase by 1 block only (512 B
).TODO:
prodtest
andbootloader_ci
build does not work yet - trying to have one or twoconst font_info_t*
clippy
compiling with all featuresstage
(either"bootloader"
or"firmware"
). I'm not sure that the current approach gets this right. Only some fonts are used from the bootloader so I pressume that the compiler does not include the others for bootloader binary.[ ] ideally get rid of the C fonts completelyUSE_RGB_COLOURS