From 07424f162543d33fe71b0799af8eb17a0a869f7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ioan=20Biz=C4=83u?= Date: Tue, 8 Apr 2025 13:58:17 +0200 Subject: [PATCH] fix(eckhart): optimize device menu --- .../firmware/device_menu_screen.rs | 161 ++++++++---------- .../rust/src/ui/layout_eckhart/ui_firmware.rs | 2 +- 2 files changed, 75 insertions(+), 88 deletions(-) diff --git a/core/embed/rust/src/ui/layout_eckhart/firmware/device_menu_screen.rs b/core/embed/rust/src/ui/layout_eckhart/firmware/device_menu_screen.rs index 6e79fd30c7..25f8e9126d 100644 --- a/core/embed/rust/src/ui/layout_eckhart/firmware/device_menu_screen.rs +++ b/core/embed/rust/src/ui/layout_eckhart/firmware/device_menu_screen.rs @@ -1,4 +1,8 @@ +use core::ops::DerefMut; + use crate::{ + error::Error, + micropython::gc::GcBox, strutil::TString, ui::{ component::{ @@ -24,8 +28,9 @@ use crate::{ use super::theme; use heapless::Vec; -const MAX_DEPTH: usize = 5; -const MAX_SUBSCREENS: usize = 10; +const MAX_DEPTH: usize = 3; +const MAX_SUBSCREENS: usize = 8; +const MAX_SUBMENUS: usize = MAX_SUBSCREENS - 2 /* (about and device screen) */; const DISCONNECT_DEVICE_MENU_INDEX: usize = 1; @@ -88,13 +93,13 @@ impl MenuItem { } } -struct SubmenuScreen { +struct Submenu { header_text: TString<'static>, show_battery: bool, items: Vec, } -impl SubmenuScreen { +impl Submenu { pub fn new(header_text: TString<'static>, items: Vec) -> Self { Self { header_text, @@ -110,10 +115,9 @@ impl SubmenuScreen { } // Each subscreen of the DeviceMenuScreen is one of these -#[allow(clippy::large_enum_variant)] enum Subscreen { - // A menu, with associated items and actions - Submenu(SubmenuScreen), + // A registered submenu + Submenu(usize), // A screen allowing the user to to disconnect a device DeviceScreen( @@ -133,14 +137,14 @@ pub struct DeviceMenuScreen<'a> { // These correspond to the currently active subscreen, // which is one of the possible kinds of subscreens - // as defined by `enum Subscreen` - // The active one will be Some(...) and the other two will be None. + // as defined by `enum Subscreen` (DeviceScreen is still a VerticalMenuScreen!) + // The active one will be Some(...) and the other one will be None. // This way we only need to keep one screen at any time in memory. - menu_screen: Option, - paired_device_screen: Option, - about_screen: Option; 2]>>>, + menu_screen: GcBox>, + about_screen: GcBox; 2]>>>>, // Information needed to construct any subscreen on demand + submenus: GcBox>, subscreens: Vec, // index of the current subscreen in the list of subscreens @@ -162,15 +166,15 @@ impl<'a> DeviceMenuScreen<'a> { // (see component_msg_obj.rs, which currently just returns "DeviceDisconnect" with no // index!) paired_devices: Vec, 1>, - ) -> Self { + ) -> Result { let mut screen = Self { bounds: Rect::zero(), battery_percentage, firmware_version, - menu_screen: None, - paired_device_screen: None, - about_screen: None, + menu_screen: GcBox::new(None)?, + about_screen: GcBox::new(None)?, active_subscreen: 0, + submenus: GcBox::new(Vec::new())?, subscreens: Vec::new(), parent_subscreens: Vec::new(), }; @@ -193,7 +197,7 @@ impl<'a> DeviceMenuScreen<'a> { screen.set_active_subscreen(root); - screen + Ok(screen) } fn is_low_battery(&self) -> bool { @@ -215,10 +219,8 @@ impl<'a> DeviceMenuScreen<'a> { )); } - self.add_subscreen(Subscreen::Submenu(SubmenuScreen::new( - "Manage paired devices".into(), - items, - ))) + let submenu_index = self.add_submenu(Submenu::new("Manage paired devices".into(), items)); + self.add_subscreen(Subscreen::Submenu(submenu_index)) } fn add_pair_and_connect_menu(&mut self, manage_devices_index: usize) -> usize { @@ -238,10 +240,8 @@ impl<'a> DeviceMenuScreen<'a> { Some(Action::Return(DeviceMenuMsg::DevicePair)), ))); - self.add_subscreen(Subscreen::Submenu(SubmenuScreen::new( - "Pair & connect".into(), - items, - ))) + let submenu_index = self.add_submenu(Submenu::new("Pair & connect".into(), items)); + self.add_subscreen(Subscreen::Submenu(submenu_index)) } fn add_settings_menu(&mut self, security_index: usize, device_index: usize) -> usize { @@ -255,10 +255,8 @@ impl<'a> DeviceMenuScreen<'a> { Some(Action::GoTo(device_index)) ))); - self.add_subscreen(Subscreen::Submenu(SubmenuScreen::new( - "Settings".into(), - items, - ))) + let submenu_index = self.add_submenu(Submenu::new("Settings".into(), items)); + self.add_subscreen(Subscreen::Submenu(submenu_index)) } fn add_security_menu(&mut self) -> usize { @@ -272,10 +270,8 @@ impl<'a> DeviceMenuScreen<'a> { Some(Action::Return(DeviceMenuMsg::WipeDevice)) ))); - self.add_subscreen(Subscreen::Submenu(SubmenuScreen::new( - "Security".into(), - items, - ))) + let submenu_index = self.add_submenu(Submenu::new("Security".into(), items)); + self.add_subscreen(Subscreen::Submenu(submenu_index)) } fn add_device_menu(&mut self, device_name: TString<'static>, about_index: usize) -> usize { @@ -292,10 +288,8 @@ impl<'a> DeviceMenuScreen<'a> { Some(Action::GoTo(about_index)) ))); - self.add_subscreen(Subscreen::Submenu(SubmenuScreen::new( - "Device".into(), - items, - ))) + let submenu_index = self.add_submenu(Submenu::new("Device".into(), items)); + self.add_subscreen(Subscreen::Submenu(submenu_index)) } fn add_root_menu( @@ -329,9 +323,14 @@ impl<'a> DeviceMenuScreen<'a> { "Settings".into(), Some(Action::GoTo(settings_index)), ))); - self.add_subscreen(Subscreen::Submenu( - SubmenuScreen::new("".into(), items).with_battery(), - )) + + let submenu_index = self.add_submenu(Submenu::new("".into(), items).with_battery()); + self.add_subscreen(Subscreen::Submenu(submenu_index)) + } + + fn add_submenu(&mut self, submenu: Submenu) -> usize { + unwrap!(self.submenus.push(submenu)); + self.submenus.len() - 1 } fn add_subscreen(&mut self, screen: Subscreen) -> usize { @@ -347,9 +346,9 @@ impl<'a> DeviceMenuScreen<'a> { fn build_active_subscreen(&mut self) { match self.subscreens[self.active_subscreen] { - Subscreen::Submenu(ref mut submenu) => { - self.paired_device_screen = None; - self.about_screen = None; + Subscreen::Submenu(ref mut submenu_index) => { + let submenu = &self.submenus[*submenu_index]; + *self.about_screen.deref_mut() = None; let mut menu = VerticalMenu::empty().with_separators(); for item in &submenu.items { let button = if let Some((subtext, subtext_style)) = item.subtext { @@ -364,8 +363,7 @@ impl<'a> DeviceMenuScreen<'a> { }; menu = menu.item(button); } - let mut header = Header::new(submenu.header_text) - .with_right_button(Button::with_icon(theme::ICON_CROSS), HeaderMsg::Cancelled); + let mut header = Header::new(submenu.header_text).with_close_button(); if submenu.show_battery { header = header.with_icon( theme::ICON_BATTERY_ZAP, @@ -381,24 +379,21 @@ impl<'a> DeviceMenuScreen<'a> { HeaderMsg::Back, ); } - self.menu_screen = Some(VerticalMenuScreen::new(menu).with_header(header)); + *self.menu_screen.deref_mut() = + Some(VerticalMenuScreen::new(menu).with_header(header)); } Subscreen::DeviceScreen(device, _) => { - self.menu_screen = None; - self.about_screen = None; + *self.about_screen.deref_mut() = None; let mut menu = VerticalMenu::empty().with_separators(); menu = menu.item(Button::new_menu_item(device, theme::menu_item_title())); menu = menu.item(Button::new_menu_item( "Disconnect".into(), theme::menu_item_title_red(), )); - self.paired_device_screen = Some( + *self.menu_screen.deref_mut() = Some( VerticalMenuScreen::new(menu).with_header( Header::new("Manage".into()) - .with_right_button( - Button::with_icon(theme::ICON_CROSS), - HeaderMsg::Cancelled, - ) + .with_close_button() .with_left_button( Button::with_icon(theme::ICON_CHEVRON_LEFT), HeaderMsg::Back, @@ -407,14 +402,13 @@ impl<'a> DeviceMenuScreen<'a> { ); } Subscreen::AboutScreen => { - self.menu_screen = None; - self.paired_device_screen = None; + *self.menu_screen.deref_mut() = None; let about_content = Paragraphs::new([ Paragraph::new(&theme::firmware::TEXT_REGULAR, "Firmware version"), Paragraph::new(&theme::firmware::TEXT_REGULAR, self.firmware_version), ]); - self.about_screen = Some( + *self.about_screen.deref_mut() = Some( TextScreen::new(about_content) .with_header(Header::new("About".into()).with_close_button()), ); @@ -424,8 +418,8 @@ impl<'a> DeviceMenuScreen<'a> { fn handle_submenu(&mut self, ctx: &mut EventCtx, idx: usize) -> Option { match self.subscreens[self.active_subscreen] { - Subscreen::Submenu(ref mut menu_screen) => { - match menu_screen.items[idx].action { + Subscreen::Submenu(ref mut submenu_index) => { + match self.submenus[*submenu_index].items[idx].action { Some(Action::GoTo(menu)) => { self.menu_screen.as_mut().unwrap().update_menu(ctx); unwrap!(self.parent_subscreens.push(self.active_subscreen)); @@ -466,8 +460,7 @@ impl<'a> Component for DeviceMenuScreen<'a> { self.bounds = bounds; match self.subscreens[self.active_subscreen] { - Subscreen::Submenu(..) => self.menu_screen.place(bounds), - Subscreen::DeviceScreen(..) => self.paired_device_screen.place(bounds), + Subscreen::Submenu(..) | Subscreen::DeviceScreen(..) => self.menu_screen.place(bounds), Subscreen::AboutScreen => self.about_screen.place(bounds), }; @@ -476,33 +469,28 @@ impl<'a> Component for DeviceMenuScreen<'a> { fn event(&mut self, ctx: &mut EventCtx, event: Event) -> Option { // Handle the event for the active menu - match self.subscreens[self.active_subscreen] { - Subscreen::Submenu(..) => match self.menu_screen.event(ctx, event) { - Some(VerticalMenuScreenMsg::Selected(index)) => { - return self.handle_submenu(ctx, index); - } - Some(VerticalMenuScreenMsg::Back) => { - return self.go_back(); - } - Some(VerticalMenuScreenMsg::Close) => { - return Some(DeviceMenuMsg::Close); - } - _ => {} - }, - Subscreen::DeviceScreen(_, i) => match self.paired_device_screen.event(ctx, event) { - Some(VerticalMenuScreenMsg::Selected(index)) => { - if index == DISCONNECT_DEVICE_MENU_INDEX { - return Some(DeviceMenuMsg::DeviceDisconnect(i)); + let subscreen = &self.subscreens[self.active_subscreen]; + match subscreen { + Subscreen::Submenu(..) | Subscreen::DeviceScreen(..) => { + match self.menu_screen.event(ctx, event) { + Some(VerticalMenuScreenMsg::Selected(index)) => { + if let Subscreen::DeviceScreen(_, i) = subscreen { + if index == DISCONNECT_DEVICE_MENU_INDEX { + return Some(DeviceMenuMsg::DeviceDisconnect(*i)); + } + } else { + return self.handle_submenu(ctx, index); + } } + Some(VerticalMenuScreenMsg::Back) => { + return self.go_back(); + } + Some(VerticalMenuScreenMsg::Close) => { + return Some(DeviceMenuMsg::Close); + } + _ => {} } - Some(VerticalMenuScreenMsg::Back) => { - return self.go_back(); - } - Some(VerticalMenuScreenMsg::Close) => { - return Some(DeviceMenuMsg::Close); - } - _ => {} - }, + } Subscreen::AboutScreen => { if let Some(TextScreenMsg::Cancelled) = self.about_screen.event(ctx, event) { return self.go_back(); @@ -515,8 +503,7 @@ impl<'a> Component for DeviceMenuScreen<'a> { fn render<'s>(&'s self, target: &mut impl Renderer<'s>) { match &self.subscreens[self.active_subscreen] { - Subscreen::Submenu(..) => self.menu_screen.render(target), - Subscreen::DeviceScreen(..) => self.paired_device_screen.render(target), + Subscreen::Submenu(..) | Subscreen::DeviceScreen(..) => self.menu_screen.render(target), Subscreen::AboutScreen => self.about_screen.render(target), } } diff --git a/core/embed/rust/src/ui/layout_eckhart/ui_firmware.rs b/core/embed/rust/src/ui/layout_eckhart/ui_firmware.rs index 6e9f5fd43c..6f26bcb4ab 100644 --- a/core/embed/rust/src/ui/layout_eckhart/ui_firmware.rs +++ b/core/embed/rust/src/ui/layout_eckhart/ui_firmware.rs @@ -796,7 +796,7 @@ impl FirmwareUI for UIEckhart { firmware_version, device_name, paired_devices, - )); + )?); Ok(layout) }