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

chore(core): show the last passphrase character for a while #4385

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

Conversation

bieleluk
Copy link
Contributor

@bieleluk bieleluk commented Nov 25, 2024

This PR solves #3959 :

  • [mercury] Introduces a feature, that shows the last typed PIN number for short period of time (1 second) before changing it to back to a bullet icon
  • [tt] Introduces a feature, that shows the last typed PIN number for short period of time (1 second) before changing it to back to a * character with faded color
  • [tr] Changes the feature, where the last PIN digit is shown until non-timer (button) event to use aforementioned timeout.

Changes include:

  • tt, mercury
    • Adding Timer and 3-state enum objects to the Input struct and utilizing them to change the passphrase rendering style (stars, last character, all characters)
    • Adding new label_keyboard_mono font
    • Changing ellipse ... to twitching when passphrase overflows
    • Utilizing TouchStart and TouchStop events to show/hide entire passphrase and maximizing the touch area
    • Implementing apply_display_style function to render the passphrase according to required rendering style
  • tr
    • Adding Timer to the PassphraseEntry struct and utilizing it to change the passphrase rendering style (stars, last character, all characters)
    • Changing any non-timer event handling with the introduced timer token event.
    • Removing ellipse ..., and keep only twitching when passphrase overflows

t_pp
mercury_pp

@bieleluk bieleluk linked an issue Nov 25, 2024 that may be closed by this pull request
@bieleluk bieleluk self-assigned this Nov 25, 2024
Copy link

github-actions bot commented Nov 25, 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) test(screens) main(screens)
T3T1 Safe 5 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

Copy link

github-actions bot commented Nov 25, 2024

legacy UI changes device test(screens) main(screens)

@bieleluk bieleluk marked this pull request as ready for review December 2, 2024 11:13
@bieleluk bieleluk added the T3T1 Trezor Safe 5 label Dec 2, 2024
@bieleluk bieleluk added this to the T3T1 milestone Dec 2, 2024
@matejcik matejcik removed the request for review from prusnak December 2, 2024 14:02
Copy link
Contributor

@obrusvit obrusvit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't tested the functionality yet.
I have some code-related comments.

@@ -26,6 +27,14 @@ pub enum PassphraseKeyboardMsg {
Cancelled,
}

#[derive(PartialEq, Debug, Copy, Clone)]
#[cfg_attr(feature = "ui_debug", derive(ufmt::derive::uDebug))]
enum DisplayStyle {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same enum with the same derives also in keyboard/pin.rs.

Perhaps, we can put it to some common space at least per model? Or maybe even common for all models and keyboards?

The naming could be more generic so that it captures both "chars" and "digits" and all models. Something like Hidden/Shown/LastOnly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea, I am just wondering, where to place it.
For the per-model scenario, there is already a common keyboard module, but it will still be repeated multiple times.
For the global (best) one, I don't see the proper location right away.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests/click_tests/test_passphrase_mercury.py Show resolved Hide resolved
tests/click_tests/test_passphrase_tr.py Show resolved Hide resolved
tests/click_tests/test_passphrase_tt.py Show resolved Hide resolved
self.last_char_timer.stop();
}

fn render_chars<'s>(&self, area: Rect, target: &mut impl Renderer<'s>) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I look correctly, it seems that majority of the code in render_chars and render_dots is the same except the final rendering and the naming of variables (chars vs all_chars, text_baseline vs cursor).

Do you think we can somehow simplify these two functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bieleluk bieleluk requested a review from obrusvit December 12, 2024 10:25
@bieleluk bieleluk requested a review from matejcik December 16, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T3T1 Trezor Safe 5
Projects
Status: 🔎 Needs review
Development

Successfully merging this pull request may close these issues.

Unify the behaviour of entering PIN/Passphrase on the device
3 participants