From dc715d7c0de029e0a8133d0f00a3cd781637d0fb Mon Sep 17 00:00:00 2001 From: cepetr Date: Fri, 8 Mar 2024 13:00:13 +0100 Subject: [PATCH] refactor(core): prepare for non mutable paint function [no changelog] --- .../rust/src/ui/component/text/formatted.rs | 18 +------- core/embed/rust/src/ui/component/text/op.rs | 25 +++++------ .../src/ui/model_tr/component/flow_pages.rs | 11 +++-- .../component/input_methods/choice.rs | 10 ++--- .../rust/src/ui/model_tt/component/fido.rs | 43 +++++++++++++------ .../model_tt/component/keyboard/passphrase.rs | 11 ++--- .../src/ui/model_tt/component/simple_page.rs | 7 +-- 7 files changed, 68 insertions(+), 57 deletions(-) diff --git a/core/embed/rust/src/ui/component/text/formatted.rs b/core/embed/rust/src/ui/component/text/formatted.rs index 14a1b7f8f9..58613f3ae6 100644 --- a/core/embed/rust/src/ui/component/text/formatted.rs +++ b/core/embed/rust/src/ui/component/text/formatted.rs @@ -34,7 +34,7 @@ impl FormattedText { self } - fn layout_content(&mut self, sink: &mut dyn LayoutSink) -> LayoutFit { + pub(crate) fn layout_content(&self, sink: &mut dyn LayoutSink) -> LayoutFit { self.op_layout .layout_ops(self.char_offset, Offset::y(self.y_offset), sink) } @@ -144,20 +144,6 @@ impl Component for FormattedText { // DEBUG-ONLY SECTION BELOW -#[cfg(feature = "ui_debug")] -impl FormattedText { - /// Is the same as layout_content, but does not use `&mut self` - /// to be compatible with `trace`. - /// Therefore it has to do the `clone` of `op_layout`. - pub fn layout_content_debug(&self, sink: &mut dyn LayoutSink) -> LayoutFit { - // TODO: how to solve it "properly", without the `clone`? - // (changing `trace` to `&mut self` had some other isses...) - self.op_layout - .clone() - .layout_content(self.char_offset, sink) - } -} - #[cfg(feature = "ui_debug")] impl crate::trace::Trace for FormattedText { fn trace(&self, t: &mut dyn crate::trace::Tracer) { @@ -166,7 +152,7 @@ impl crate::trace::Trace for FormattedText { let fit: Cell> = Cell::new(None); t.component("FormattedText"); t.in_list("text", &|l| { - let result = self.layout_content_debug(&mut TraceSink(l)); + let result = self.layout_content(&mut TraceSink(l)); fit.set(Some(result)); }); t.bool("fits", matches!(fit.get(), Some(LayoutFit::Fitting { .. }))); diff --git a/core/embed/rust/src/ui/component/text/op.rs b/core/embed/rust/src/ui/component/text/op.rs index a008100b69..ccaad640bf 100644 --- a/core/embed/rust/src/ui/component/text/op.rs +++ b/core/embed/rust/src/ui/component/text/op.rs @@ -56,7 +56,7 @@ impl<'a, T: StringType + Clone + 'a> OpTextLayout { /// Perform some operations defined on `Op` for a list of those `Op`s /// - e.g. changing the color, changing the font or rendering the text. pub fn layout_ops( - &mut self, + &self, skip_bytes: usize, offset: Offset, sink: &mut dyn LayoutSink, @@ -65,25 +65,26 @@ impl<'a, T: StringType + Clone + 'a> OpTextLayout { let cursor = &mut (self.layout.initial_cursor() + offset); let init_cursor = *cursor; let mut total_processed_chars = 0; + let mut layout = self.layout; // Do something when it was not skipped for op in Self::filter_skipped_ops(self.ops.iter(), skip_bytes) { match op { // Changing color Op::Color(color) => { - self.layout.style.text_color = color; + layout.style.text_color = color; } // Changing font Op::Font(font) => { - self.layout.style.text_font = font; + layout.style.text_font = font; } // Changing line/text alignment Op::Alignment(line_alignment) => { - self.layout.align = line_alignment; + layout.align = line_alignment; } // Changing line breaking Op::LineBreaking(line_breaking) => { - self.layout.style.line_breaking = line_breaking; + layout.style.line_breaking = line_breaking; } // Moving the cursor Op::CursorOffset(offset) => { @@ -91,10 +92,10 @@ impl<'a, T: StringType + Clone + 'a> OpTextLayout { cursor.y += offset.y; } Op::Chunkify(chunks) => { - self.layout.style.chunks = chunks; + layout.style.chunks = chunks; } Op::LineSpacing(line_spacing) => { - self.layout.style.line_spacing = line_spacing; + layout.style.line_spacing = line_spacing; } // Moving to the next page Op::NextPage => { @@ -103,7 +104,7 @@ impl<'a, T: StringType + Clone + 'a> OpTextLayout { total_processed_chars += PROCESSED_CHARS_ONE; return LayoutFit::OutOfBounds { processed_chars: total_processed_chars, - height: self.layout.layout_height(init_cursor, *cursor), + height: layout.layout_height(init_cursor, *cursor), }; } // Drawing text @@ -113,9 +114,9 @@ impl<'a, T: StringType + Clone + 'a> OpTextLayout { // Inserting the ellipsis at the very beginning of the text if needed // (just for incomplete texts that were separated) - self.layout.continues_from_prev_page = continued; + layout.continues_from_prev_page = continued; - let fit = self.layout.layout_text(text.as_ref(), cursor, sink); + let fit = layout.layout_text(text.as_ref(), cursor, sink); match fit { LayoutFit::Fitting { @@ -130,7 +131,7 @@ impl<'a, T: StringType + Clone + 'a> OpTextLayout { return LayoutFit::OutOfBounds { processed_chars: total_processed_chars, - height: self.layout.layout_height(init_cursor, *cursor), + height: layout.layout_height(init_cursor, *cursor), }; } } @@ -140,7 +141,7 @@ impl<'a, T: StringType + Clone + 'a> OpTextLayout { LayoutFit::Fitting { processed_chars: total_processed_chars, - height: self.layout.layout_height(init_cursor, *cursor), + height: layout.layout_height(init_cursor, *cursor), } } diff --git a/core/embed/rust/src/ui/model_tr/component/flow_pages.rs b/core/embed/rust/src/ui/model_tr/component/flow_pages.rs index 5ac8e76914..25eeb91e81 100644 --- a/core/embed/rust/src/ui/model_tr/component/flow_pages.rs +++ b/core/embed/rust/src/ui/model_tr/component/flow_pages.rs @@ -97,7 +97,7 @@ where btn_actions: ButtonActions, formatted: FormattedText, ) -> Self { - Self { + let mut page = Self { formatted, btn_layout, btn_actions, @@ -105,7 +105,9 @@ where page_count: 1, title: None, slim_arrows: false, - } + }; + page.change_page(page.current_page); + page } } @@ -127,7 +129,6 @@ where } pub fn paint(&mut self) { - self.change_page(self.current_page); self.formatted.paint(); } @@ -193,10 +194,12 @@ where pub fn go_to_prev_page(&mut self) { self.current_page -= 1; + self.change_page(self.current_page); } pub fn go_to_next_page(&mut self) { self.current_page += 1; + self.change_page(self.current_page); } pub fn get_current_page(&self) -> usize { @@ -240,7 +243,7 @@ where t.int("active_page", self.current_page as i64); t.int("page_count", self.page_count as i64); t.in_list("text", &|l| { - let result = self.formatted.layout_content_debug(&mut TraceSink(l)); + let result = self.formatted.layout_content(&mut TraceSink(l)); fit.set(Some(result)); }); } diff --git a/core/embed/rust/src/ui/model_tr/component/input_methods/choice.rs b/core/embed/rust/src/ui/model_tr/component/input_methods/choice.rs index cd46425e15..4dc9a9d93a 100644 --- a/core/embed/rust/src/ui/model_tr/component/input_methods/choice.rs +++ b/core/embed/rust/src/ui/model_tr/component/input_methods/choice.rs @@ -294,11 +294,6 @@ where fn show_current_choice(&mut self, area: Rect) { self.get_current_item() .paint_center(area, self.inverse_selected_item); - - // Color inversion is just one-time thing. - if self.inverse_selected_item { - self.inverse_selected_item = false; - } } /// Display all the choices fitting on the left side. @@ -491,6 +486,11 @@ where } fn event(&mut self, ctx: &mut EventCtx, event: Event) -> Option { + // Cancel highlighting of the current choice. + // The Highlighting is started by pressing the middle button and + // canceled immediately when any other event is processed + self.inverse_selected_item = false; + // Possible animation movement when setting (randomizing) the page counter. if let Some(animation_direction) = self.animation_event(ctx, event) { match animation_direction { diff --git a/core/embed/rust/src/ui/model_tt/component/fido.rs b/core/embed/rust/src/ui/model_tt/component/fido.rs index 50ce240c01..3b7f64caaf 100644 --- a/core/embed/rust/src/ui/model_tt/component/fido.rs +++ b/core/embed/rust/src/ui/model_tt/component/fido.rs @@ -10,6 +10,7 @@ use crate::ui::{ }; use super::CancelConfirmMsg; +use core::cell::Cell; const ICON_HEIGHT: i16 = 70; const SCROLLBAR_INSET_TOP: i16 = 5; @@ -30,7 +31,7 @@ pub struct FidoConfirm T, T, U> { /// Function/closure that will return appropriate page on demand. get_account: F, scrollbar: ScrollBar, - fade: bool, + fade: Cell, controls: U, } @@ -59,14 +60,27 @@ where page_swipe.allow_right = scrollbar.has_previous_page(); page_swipe.allow_left = scrollbar.has_next_page(); + // NOTE: This is an ugly hotfix for the erroneous behavior of + // TextLayout used in the account_name Label. In this + // particular case, TextLayout calculates the wrong height of + // fitted text that's higher than the TextLayout bound itself. + // + // The following two lines should be swapped when the problem with + // TextLayout is fixed. + // + // See also, continuation of this hotfix in the place() function. + + // let current_account = get_account(scrollbar.active_page); + let current_account = "".into(); + Self { app_name: Label::centered(app_name, theme::TEXT_DEMIBOLD), - account_name: Label::centered("".into(), theme::TEXT_DEMIBOLD), + account_name: Label::centered(current_account, theme::TEXT_DEMIBOLD), page_swipe, icon: Child::new(Image::new(icon_data)), get_account, scrollbar, - fade: false, + fade: Cell::new(false), controls, } } @@ -87,11 +101,14 @@ where self.page_swipe.allow_right = self.scrollbar.has_previous_page(); self.page_swipe.allow_left = self.scrollbar.has_next_page(); + let current_account = (self.get_account)(self.active_page()); + self.account_name.set_text(current_account); + // Redraw the page. ctx.request_paint(); // Reset backlight to normal level on next paint. - self.fade = true; + self.fade.set(true); } fn active_page(&self) -> usize { @@ -139,6 +156,12 @@ where self.app_name.place(app_name_area); self.account_name.place(account_name_area); + // NOTE: This is a hotfix used due to the erroneous behavior of TextLayout. + // This line should be removed when the problem with TextLayout is fixed. + // See also the code for FidoConfirm::new(). + self.account_name + .set_text((self.get_account)(self.scrollbar.active_page)); + bounds } @@ -166,8 +189,6 @@ where self.scrollbar.paint(); } - let current_account = (self.get_account)(self.active_page()); - // Erasing the old text content before writing the new one. let account_name_area = self.account_name.area(); let real_area = account_name_area @@ -177,15 +198,13 @@ where // Account name is optional. // Showing it only if it differs from app name. // (Dummy requests usually have some text as both app_name and account_name.) - if !current_account.as_ref().is_empty() - && current_account.as_ref() != self.app_name.text().as_ref() - { - self.account_name.set_text(current_account); + let account_name = self.account_name.text().as_ref(); + let app_name = self.app_name.text().as_ref(); + if !account_name.is_empty() && account_name != app_name { self.account_name.paint(); } - if self.fade { - self.fade = false; + if self.fade.take() { // Note that this is blocking and takes some time. display::fade_backlight(theme::BACKLIGHT_NORMAL); } diff --git a/core/embed/rust/src/ui/model_tt/component/keyboard/passphrase.rs b/core/embed/rust/src/ui/model_tt/component/keyboard/passphrase.rs index 133c55b923..ab74d7a698 100644 --- a/core/embed/rust/src/ui/model_tt/component/keyboard/passphrase.rs +++ b/core/embed/rust/src/ui/model_tt/component/keyboard/passphrase.rs @@ -13,6 +13,8 @@ use crate::ui::{ util::long_line_content_with_ellipsis, }; +use core::cell::Cell; + pub enum PassphraseKeyboardMsg { Confirmed, Cancelled, @@ -25,7 +27,7 @@ pub struct PassphraseKeyboard { confirm: Child>, keys: [Child>; KEY_COUNT], scrollbar: ScrollBar, - fade: bool, + fade: Cell, } const STARTING_PAGE: usize = 1; @@ -63,7 +65,7 @@ impl PassphraseKeyboard { Child::new(Button::new(Self::key_content(text)).styled(theme::button_pin())) }), scrollbar: ScrollBar::horizontal(), - fade: false, + fade: Cell::new(false), } } @@ -99,7 +101,7 @@ impl PassphraseKeyboard { // Update buttons. self.replace_button_content(ctx, key_page); // Reset backlight to normal level on next paint. - self.fade = true; + self.fade.set(true); // So that swipe does not visually enable the input buttons when max length // reached self.update_input_btns_state(ctx); @@ -288,8 +290,7 @@ impl Component for PassphraseKeyboard { for btn in &mut self.keys { btn.paint(); } - if self.fade { - self.fade = false; + if self.fade.take() { // Note that this is blocking and takes some time. display::fade_backlight(theme::BACKLIGHT_NORMAL); } diff --git a/core/embed/rust/src/ui/model_tt/component/simple_page.rs b/core/embed/rust/src/ui/model_tt/component/simple_page.rs index 73ed3048f2..92bb4c53a3 100644 --- a/core/embed/rust/src/ui/model_tt/component/simple_page.rs +++ b/core/embed/rust/src/ui/model_tt/component/simple_page.rs @@ -5,6 +5,7 @@ use crate::ui::{ }; use super::{theme, ScrollBar, Swipe, SwipeDirection}; +use core::cell::Cell; const SCROLLBAR_HEIGHT: i16 = 18; const SCROLLBAR_BORDER: i16 = 4; @@ -16,7 +17,7 @@ pub struct SimplePage { scrollbar: ScrollBar, axis: Axis, swipe_right_to_go_back: bool, - fade: Option, + fade: Cell>, } impl SimplePage @@ -32,7 +33,7 @@ where scrollbar: ScrollBar::new(axis), axis, swipe_right_to_go_back: false, - fade: None, + fade: Cell::new(None), } } @@ -79,7 +80,7 @@ where // Swipe has dimmed the screen, so fade back to normal backlight after the next // paint. - self.fade = Some(theme::BACKLIGHT_NORMAL); + self.fade.set(Some(theme::BACKLIGHT_NORMAL)); } fn is_horizontal(&self) -> bool {