From 65c9380ab7e9766ea4dca7f60f3f4f89e59a655e Mon Sep 17 00:00:00 2001 From: grdddj Date: Wed, 20 Sep 2023 15:40:11 +0200 Subject: [PATCH] feat(core): triggering delete action in ChoicePage after 1 second even without release [no changelog] --- .../rust/src/ui/model_tr/component/button.rs | 3 + .../model_tr/component/button_controller.rs | 101 ++++++++++++++++-- .../component/input_methods/choice.rs | 41 +++++-- .../component/input_methods/choice_item.rs | 16 +++ .../component/input_methods/passphrase.rs | 26 ++++- .../model_tr/component/input_methods/pin.rs | 37 ++++--- .../component/input_methods/wordlist.rs | 3 +- python/src/trezorlib/debuglink.py | 20 +++- tests/click_tests/common.py | 8 +- tests/click_tests/test_pin.py | 23 ++++ 10 files changed, 242 insertions(+), 36 deletions(-) diff --git a/core/embed/rust/src/ui/model_tr/component/button.rs b/core/embed/rust/src/ui/model_tr/component/button.rs index 0b414920e..da53990bf 100644 --- a/core/embed/rust/src/ui/model_tr/component/button.rs +++ b/core/embed/rust/src/ui/model_tr/component/button.rs @@ -349,6 +349,7 @@ pub struct ButtonDetails { with_arms: bool, fixed_width: Option, offset: Offset, + pub send_long_press: bool, } impl ButtonDetails @@ -364,6 +365,7 @@ where with_arms: false, fixed_width: None, offset: Offset::zero(), + send_long_press: false, } } @@ -376,6 +378,7 @@ where with_arms: false, fixed_width: None, offset: Offset::zero(), + send_long_press: false, } } diff --git a/core/embed/rust/src/ui/model_tr/component/button_controller.rs b/core/embed/rust/src/ui/model_tr/component/button_controller.rs index b51296a34..be7304775 100644 --- a/core/embed/rust/src/ui/model_tr/component/button_controller.rs +++ b/core/embed/rust/src/ui/model_tr/component/button_controller.rs @@ -39,9 +39,13 @@ enum ButtonState { } pub enum ButtonControllerMsg { + /// Button was pressed down. Pressed(ButtonPos), - /// Which button was triggered, and whether it was pressed for a longer time + /// Which button was triggered, and whether it was pressed for a longer + /// time before releasing. Triggered(ButtonPos, bool), + /// Button was pressed and held for longer time (not released yet). + LongPressed(ButtonPos), } /// Defines what kind of button should be currently used. @@ -109,8 +113,13 @@ where /// Holds the timestamp of when the button was pressed. pressed_since: Option, /// How long the button should be pressed to send `long_press=true` in - /// `Triggered` + /// `ButtonControllerMsg::Triggered` long_press_ms: u32, + /// Timer for sending `ButtonControllerMsg::LongPressed` + long_pressed_timer: Option, + /// Whether it should even send `ButtonControllerMsg::LongPressed` events + /// (optional) + send_long_press: bool, } impl ButtonContainer @@ -121,11 +130,16 @@ where /// (it can be later activated in `set()`). pub fn new(pos: ButtonPos, btn_details: Option>) -> Self { const DEFAULT_LONG_PRESS_MS: u32 = 1000; + let send_long_press = btn_details + .as_ref() + .map_or(false, |btn| btn.send_long_press); Self { pos, button_type: ButtonType::from_button_details(pos, btn_details), pressed_since: None, long_press_ms: DEFAULT_LONG_PRESS_MS, + long_pressed_timer: None, + send_long_press, } } @@ -133,6 +147,9 @@ where /// /// Passing `None` as `btn_details` will mark the button as inactive. pub fn set(&mut self, btn_details: Option>, button_area: Rect) { + self.send_long_press = btn_details + .as_ref() + .map_or(false, |btn| btn.send_long_press); self.button_type = ButtonType::from_button_details(self.pos, btn_details); self.button_type.place(button_area); } @@ -166,6 +183,7 @@ where Instant::now().saturating_duration_since(since).to_millis() > self.long_press_ms }); self.pressed_since = None; + self.long_pressed_timer = None; Some(ButtonControllerMsg::Triggered(self.pos, long_press)) } _ => { @@ -186,8 +204,24 @@ where } /// Saving the timestamp of when the button was pressed. - pub fn got_pressed(&mut self) { + /// Also requesting a timer for long-press if wanted. + pub fn got_pressed(&mut self, ctx: &mut EventCtx) { self.pressed_since = Some(Instant::now()); + if self.send_long_press { + self.long_pressed_timer = + Some(ctx.request_timer(Duration::from_millis(self.long_press_ms))); + } + } + + /// Reset the pressed information. + pub fn reset(&mut self) { + self.pressed_since = None; + self.long_pressed_timer = None; + } + + /// Whether token matches what we have + pub fn is_timer_token(&self, token: TimerToken) -> bool { + self.long_pressed_timer == Some(token) } /// Registering hold event. @@ -316,6 +350,51 @@ where } None } + + fn reset_button_presses(&mut self) { + self.left_btn.reset(); + self.middle_btn.reset(); + self.right_btn.reset(); + } + + fn got_pressed(&mut self, ctx: &mut EventCtx, pos: ButtonPos) { + // Only one (virtual) button can be pressed at the same time + self.reset_button_presses(); + match pos { + ButtonPos::Left => { + self.left_btn.got_pressed(ctx); + } + ButtonPos::Middle => { + self.middle_btn.got_pressed(ctx); + } + ButtonPos::Right => { + self.right_btn.got_pressed(ctx); + } + } + } + + fn handle_long_press_timer_token(&mut self, token: TimerToken) -> Option { + if self.left_btn.is_timer_token(token) { + return Some(ButtonPos::Left); + } + if self.middle_btn.is_timer_token(token) { + return Some(ButtonPos::Middle); + } + if self.right_btn.is_timer_token(token) { + return Some(ButtonPos::Right); + } + None + } + + /// Resetting the state of the controller. + pub fn reset_state(&mut self, ctx: &mut EventCtx) { + self.state = ButtonState::Nothing; + self.reset_button_presses(); + self.set_pressed(ctx, false, false, false); + if let Some(ignore_btn_delay) = &mut self.ignore_btn_delay { + ignore_btn_delay.reset(); + } + } } impl Component for ButtonController @@ -346,13 +425,13 @@ where match which { // ▼ * PhysicalButton::Left => { - self.left_btn.got_pressed(); + self.got_pressed(ctx, ButtonPos::Left); self.left_btn.hold_started(ctx); Some(ButtonControllerMsg::Pressed(ButtonPos::Left)) } // * ▼ PhysicalButton::Right => { - self.right_btn.got_pressed(); + self.got_pressed(ctx, ButtonPos::Right); self.right_btn.hold_started(ctx); Some(ButtonControllerMsg::Pressed(ButtonPos::Right)) } @@ -392,7 +471,7 @@ where return None; } } - self.middle_btn.got_pressed(); + self.got_pressed(ctx, ButtonPos::Middle); self.middle_hold_started(ctx); ( // ↓ ↓ @@ -471,6 +550,9 @@ where if let Some(ignore_btn_delay) = &mut self.ignore_btn_delay { ignore_btn_delay.handle_timer_token(token); } + if let Some(pos) = self.handle_long_press_timer_token(token) { + return Some(ButtonControllerMsg::LongPressed(pos)); + } self.handle_htc_expiration(ctx, event) } _ => None, @@ -568,6 +650,13 @@ impl IgnoreButtonDelay { self.right_clickable_timer = None; } } + + pub fn reset(&mut self) { + self.left_clickable = true; + self.right_clickable = true; + self.left_clickable_timer = None; + self.right_clickable_timer = None; + } } /// Component allowing for automatically moving through items (e.g. Choice 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 ae68b4097..8acd54003 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 @@ -29,6 +29,12 @@ pub trait Choice { fn btn_layout(&self) -> ButtonLayout { ButtonLayout::default_three_icons() } + + /// Whether it is possible to do the middle action event without + /// releasing the button - after long-press duration is reached. + fn trigger_middle_without_release(&self) -> bool { + false + } } /// Interface for a specific component efficiently giving @@ -130,7 +136,7 @@ where /// Need to update the initial button layout. pub fn with_initial_page_counter(mut self, page_counter: usize) -> Self { self.page_counter = page_counter; - let initial_btn_layout = self.get_current_choice().0.btn_layout(); + let initial_btn_layout = self.get_current_item().btn_layout(); self.buttons = Child::new( ButtonController::new(initial_btn_layout) .with_ignore_btn_delay(constant::IGNORE_OTHER_BTN_MS), @@ -235,7 +241,7 @@ where } // Getting the remaining left and right areas. - let center_width = self.get_current_choice().0.width_center(); + let center_width = self.get_current_item().width_center(); let (left_area, _center_area, right_area) = center_row_area.split_center(center_width); // Possibly drawing on the left side. @@ -277,14 +283,23 @@ where } /// Getting the choice on the current index - pub fn get_current_choice(&self) -> (>::Item, A) { + fn get_current_choice(&self) -> (>::Item, A) { self.choices.get(self.page_counter) } + /// Getting the current item + pub fn get_current_item(&self) -> >::Item { + self.get_current_choice().0 + } + + /// Getting the current action + pub fn get_current_action(&self) -> A { + self.get_current_choice().1 + } + /// Display the current choice in the middle. fn show_current_choice(&mut self, area: Rect) { - self.get_current_choice() - .0 + self.get_current_item() .paint_center(area, self.inverse_selected_item); // Color inversion is just one-time thing. @@ -406,7 +421,7 @@ where /// If defined in the current choice, setting their text, /// whether they are long-pressed, and painting them. fn set_buttons(&mut self, ctx: &mut EventCtx) { - let btn_layout = self.get_current_choice().0.btn_layout(); + let btn_layout = self.get_current_item().btn_layout(); self.buttons.mutate(ctx, |ctx, buttons| { buttons.set(btn_layout); // When user holds one of the buttons, highlighting it. @@ -549,10 +564,22 @@ where ButtonPos::Middle => { // Clicked SELECT. Send current choice index with information about long-press self.clear_and_repaint(ctx); - return Some((self.get_current_choice().1, long_press)); + return Some((self.get_current_action(), long_press)); } } }; + // The middle button was pressed for longer time - sending the Event with long + // press. Also resetting the functional and visual state of the buttons. + // Only doing this when the item is configured to do so + if let Some(ButtonControllerMsg::LongPressed(ButtonPos::Middle)) = button_event { + if self.get_current_item().trigger_middle_without_release() { + self.buttons.mutate(ctx, |ctx, buttons| { + buttons.reset_state(ctx); + }); + self.clear_and_repaint(ctx); + return Some((self.get_current_action(), true)); + } + }; // The middle button was pressed, highlighting the current choice by color // inversion. if let Some(ButtonControllerMsg::Pressed(ButtonPos::Middle)) = button_event { diff --git a/core/embed/rust/src/ui/model_tr/component/input_methods/choice_item.rs b/core/embed/rust/src/ui/model_tr/component/input_methods/choice_item.rs index 1ed91cc42..451b5390e 100644 --- a/core/embed/rust/src/ui/model_tr/component/input_methods/choice_item.rs +++ b/core/embed/rust/src/ui/model_tr/component/input_methods/choice_item.rs @@ -19,6 +19,7 @@ pub struct ChoiceItem { icon: Option, btn_layout: ButtonLayout, font: Font, + middle_action_without_release: bool, } impl ChoiceItem { @@ -28,6 +29,7 @@ impl ChoiceItem { icon: None, btn_layout, font: theme::FONT_CHOICE_ITEMS, + middle_action_without_release: false, } } @@ -43,6 +45,15 @@ impl ChoiceItem { self } + /// Allows for middle action without release. + pub fn with_middle_action_without_release(mut self) -> Self { + self.middle_action_without_release = true; + if let Some(middle) = self.btn_layout.btn_middle.as_mut() { + middle.send_long_press = true; + } + self + } + /// Setting left button. pub fn set_left_btn(&mut self, btn_left: Option>) { self.btn_layout.btn_left = btn_left; @@ -117,6 +128,11 @@ where fn btn_layout(&self) -> ButtonLayout { self.btn_layout.clone() } + + /// Whether to do middle action without release + fn trigger_middle_without_release(&self) -> bool { + self.middle_action_without_release + } } fn paint_rounded_highlight(area: Rect, size: Offset, inverse: bool) { diff --git a/core/embed/rust/src/ui/model_tr/component/input_methods/passphrase.rs b/core/embed/rust/src/ui/model_tr/component/input_methods/passphrase.rs index 6012034d0..47327311b 100644 --- a/core/embed/rust/src/ui/model_tr/component/input_methods/passphrase.rs +++ b/core/embed/rust/src/ui/model_tr/component/input_methods/passphrase.rs @@ -43,50 +43,63 @@ const DIGITS_INDEX: usize = 5; const SPECIAL_INDEX: usize = 6; const SPACE_INDEX: usize = 7; -// Menu text, action, icon data, middle button with CONFIRM -const MENU: [(&str, PassphraseAction, Option, bool); MENU_LENGTH] = [ - ("SHOW", PassphraseAction::Show, Some(theme::ICON_EYE), true), +/// Menu text, action, icon data, middle button with CONFIRM, without_release +const MENU: [(&str, PassphraseAction, Option, bool, bool); MENU_LENGTH] = [ + ( + "SHOW", + PassphraseAction::Show, + Some(theme::ICON_EYE), + true, + false, + ), ( "CANCEL_OR_DELETE", // will be chosen dynamically PassphraseAction::CancelOrDelete, None, true, + true, // without_release ), ( "ENTER", PassphraseAction::Enter, Some(theme::ICON_TICK), true, + false, ), ( "abc", PassphraseAction::Category(ChoiceCategory::LowercaseLetter), None, false, + false, ), ( "ABC", PassphraseAction::Category(ChoiceCategory::UppercaseLetter), None, false, + false, ), ( "123", PassphraseAction::Category(ChoiceCategory::Digit), None, false, + false, ), ( "#$!", PassphraseAction::Category(ChoiceCategory::SpecialSymbol), None, false, + false, ), ( "SPACE", PassphraseAction::Character(' '), Some(theme::ICON_SPACE), false, + false, ), ]; @@ -164,7 +177,7 @@ impl ChoiceFactoryPassphrase { choice_index: usize, ) -> (ChoiceItem, PassphraseAction) { // More options for CANCEL/DELETE button - let (mut text, action, mut icon, show_confirm) = MENU[choice_index]; + let (mut text, action, mut icon, show_confirm, without_release) = MENU[choice_index]; if matches!(action, PassphraseAction::CancelOrDelete) { if self.is_empty { text = "CANCEL"; @@ -183,6 +196,11 @@ impl ChoiceFactoryPassphrase { menu_item.set_middle_btn(Some(confirm_btn)); } + // Making middle button create LongPress events + if without_release { + menu_item = menu_item.with_middle_action_without_release(); + } + if let Some(icon) = icon { menu_item = menu_item.with_icon(icon); } diff --git a/core/embed/rust/src/ui/model_tr/component/input_methods/pin.rs b/core/embed/rust/src/ui/model_tr/component/input_methods/pin.rs index 670cd5fa2..7af6455af 100644 --- a/core/embed/rust/src/ui/model_tr/component/input_methods/pin.rs +++ b/core/embed/rust/src/ui/model_tr/component/input_methods/pin.rs @@ -27,20 +27,22 @@ const EMPTY_PIN_STR: &str = "_"; const CHOICE_LENGTH: usize = 13; const NUMBER_START_INDEX: usize = 3; -const CHOICES: [(&str, PinAction, Option); CHOICE_LENGTH] = [ - ("DELETE", PinAction::Delete, Some(theme::ICON_DELETE)), - ("SHOW", PinAction::Show, Some(theme::ICON_EYE)), - ("ENTER", PinAction::Enter, Some(theme::ICON_TICK)), - ("0", PinAction::Digit('0'), None), - ("1", PinAction::Digit('1'), None), - ("2", PinAction::Digit('2'), None), - ("3", PinAction::Digit('3'), None), - ("4", PinAction::Digit('4'), None), - ("5", PinAction::Digit('5'), None), - ("6", PinAction::Digit('6'), None), - ("7", PinAction::Digit('7'), None), - ("8", PinAction::Digit('8'), None), - ("9", PinAction::Digit('9'), None), +/// Text, action, icon, without_release +const CHOICES: [(&str, PinAction, Option, bool); CHOICE_LENGTH] = [ + // DELETE should be triggerable without release (after long-press) + ("DELETE", PinAction::Delete, Some(theme::ICON_DELETE), true), + ("SHOW", PinAction::Show, Some(theme::ICON_EYE), false), + ("ENTER", PinAction::Enter, Some(theme::ICON_TICK), false), + ("0", PinAction::Digit('0'), None, false), + ("1", PinAction::Digit('1'), None, false), + ("2", PinAction::Digit('2'), None, false), + ("3", PinAction::Digit('3'), None, false), + ("4", PinAction::Digit('4'), None, false), + ("5", PinAction::Digit('5'), None, false), + ("6", PinAction::Digit('6'), None, false), + ("7", PinAction::Digit('7'), None, false), + ("8", PinAction::Digit('8'), None, false), + ("9", PinAction::Digit('9'), None, false), ]; fn get_random_digit_position() -> usize { @@ -54,7 +56,7 @@ impl ChoiceFactory for ChoiceFactoryPIN { type Item = ChoiceItem; fn get(&self, choice_index: usize) -> (Self::Item, Self::Action) { - let (choice_str, action, icon) = CHOICES[choice_index]; + let (choice_str, action, icon, without_release) = CHOICES[choice_index]; let mut choice_item = ChoiceItem::new(choice_str, ButtonLayout::default_three_icons()); @@ -64,6 +66,11 @@ impl ChoiceFactory for ChoiceFactoryPIN { choice_item.set_middle_btn(Some(confirm_btn)); } + // Making middle button create LongPress events + if without_release { + choice_item = choice_item.with_middle_action_without_release(); + } + // Adding icons for appropriate items if let Some(icon) = icon { choice_item = choice_item.with_icon(icon); diff --git a/core/embed/rust/src/ui/model_tr/component/input_methods/wordlist.rs b/core/embed/rust/src/ui/model_tr/component/input_methods/wordlist.rs index 07b582adb..6b4f8e927 100644 --- a/core/embed/rust/src/ui/model_tr/component/input_methods/wordlist.rs +++ b/core/embed/rust/src/ui/model_tr/component/input_methods/wordlist.rs @@ -95,7 +95,8 @@ impl ChoiceFactory for ChoiceFactoryWordlist { if choice_index == DELETE_INDEX { return ( ChoiceItem::new("DELETE", ButtonLayout::arrow_armed_arrow("CONFIRM".into())) - .with_icon(theme::ICON_DELETE), + .with_icon(theme::ICON_DELETE) + .with_middle_action_without_release(), WordlistAction::Delete, ); } diff --git a/python/src/trezorlib/debuglink.py b/python/src/trezorlib/debuglink.py index 1bfe72894..b2789f834 100644 --- a/python/src/trezorlib/debuglink.py +++ b/python/src/trezorlib/debuglink.py @@ -649,6 +649,15 @@ class DebugLink: physical_button=messages.DebugPhysicalButton.MIDDLE_BTN, wait=wait ) + def press_middle_htc( + self, hold_ms: int, extra_ms: int = 200 + ) -> Optional[LayoutContent]: + return self.press_htc( + button=messages.DebugPhysicalButton.MIDDLE_BTN, + hold_ms=hold_ms, + extra_ms=extra_ms, + ) + @overload def press_right(self) -> None: ... @@ -664,10 +673,19 @@ class DebugLink: def press_right_htc( self, hold_ms: int, extra_ms: int = 200 + ) -> Optional[LayoutContent]: + return self.press_htc( + button=messages.DebugPhysicalButton.RIGHT_BTN, + hold_ms=hold_ms, + extra_ms=extra_ms, + ) + + def press_htc( + self, button: messages.DebugPhysicalButton, hold_ms: int, extra_ms: int = 200 ) -> Optional[LayoutContent]: hold_ms = hold_ms + extra_ms # safety margin result = self.input( - physical_button=messages.DebugPhysicalButton.RIGHT_BTN, + physical_button=button, hold_ms=hold_ms, ) # sleeping little longer for UI to update diff --git a/tests/click_tests/common.py b/tests/click_tests/common.py index 4765a8835..2bbc25918 100644 --- a/tests/click_tests/common.py +++ b/tests/click_tests/common.py @@ -70,6 +70,7 @@ def navigate_to_action_and_press( wanted_action: str, all_actions: list[str], is_carousel: bool = True, + hold_ms: int = 0, ) -> None: """Navigate to the button with certain action and press it""" # Orient @@ -99,8 +100,11 @@ def navigate_to_action_and_press( is_carousel=is_carousel, ) - # Press - debug.press_middle(wait=True) + # Press or hold + if hold_ms: + debug.press_middle_htc(1000) + else: + debug.press_middle(wait=True) def _get_action_index(wanted_action: str, all_actions: list[str]) -> int: diff --git a/tests/click_tests/test_pin.py b/tests/click_tests/test_pin.py index ca6f99fa4..927e31a11 100644 --- a/tests/click_tests/test_pin.py +++ b/tests/click_tests/test_pin.py @@ -169,6 +169,18 @@ def _delete_pin(debug: "DebugLink", digits_to_delete: int, check: bool = True) - assert before[:-digits_to_delete] == after +def _delete_all(debug: "DebugLink", check: bool = True) -> None: + """Navigate to "DELETE" and hold it until all digits are deleted""" + if debug.model == "T": + debug.click_hold(buttons.pin_passphrase_grid(9), hold_ms=1500) + elif debug.model == "R": + navigate_to_action_and_press(debug, "DELETE", TR_PIN_ACTIONS, hold_ms=1000) + + if check: + after = debug.read_layout().pin() + assert after == "" + + def _cancel_pin(debug: "DebugLink") -> None: """Navigate to "CANCEL" and press it""" # It is the same button as DELETE @@ -224,6 +236,17 @@ def test_pin_long_delete(device_handler: "BackgroundDeviceHandler"): _input_see_confirm(debug, PIN24[-10:]) +@pytest.mark.setup_client(pin=PIN4) +def test_pin_delete_hold(device_handler: "BackgroundDeviceHandler"): + with prepare(device_handler) as debug: + _input_pin(debug, PIN4) + _see_pin(debug) + + _delete_all(debug) + + _input_see_confirm(debug, PIN4) + + @pytest.mark.setup_client(pin=PIN60[:50]) def test_pin_longer_than_max(device_handler: "BackgroundDeviceHandler"): with prepare(device_handler) as debug: