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

Make button press cancel current hold #4509

Merged
merged 1 commit into from
Jan 17, 2025
Merged

Make button press cancel current hold #4509

merged 1 commit into from
Jan 17, 2025

Conversation

ibz
Copy link
Contributor

@ibz ibz commented Jan 16, 2025

On TS3, when holding a button to confirm, pressing the other button now cancels the "hold".

Note: we want to cancel it without an animation rather than with the "shrinking" animation as if the held button was released!

@ibz ibz self-assigned this Jan 16, 2025
Copy link

github-actions bot commented Jan 16, 2025

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)

@ibz ibz requested review from matejcik and bieleluk January 16, 2025 10:59
@ibz ibz force-pushed the ibz/cancel-hold branch from 77e812b to bf09b40 Compare January 16, 2025 13:08
@ibz ibz marked this pull request as ready for review January 16, 2025 13:10
@ibz ibz requested a review from prusnak as a code owner January 16, 2025 13:10
@ibz ibz removed the request for review from prusnak January 16, 2025 14:29
Copy link
Contributor

@bieleluk bieleluk left a comment

Choose a reason for hiding this comment

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

The PR looks good to me.
I only have one suggestion for a better clarity but it does not affect functionality.

self.right_btn.hold_canceled(ctx);
return None;
}
_ => {}
Copy link
Contributor

@bieleluk bieleluk Jan 16, 2025

Choose a reason for hiding this comment

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

Because the power button does not exist in t2b1 nor t3b1 models, I would suggest returning None even in this case and remove returning of the ButtonState::BothDown after the added match statement

Suggested change
_ => {}
_ => {
return None;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, just returned None in all cases.

@ibz ibz force-pushed the ibz/cancel-hold branch from bf09b40 to 5e51447 Compare January 16, 2025 17:44
@ibz ibz force-pushed the ibz/cancel-hold branch from 5e51447 to b1eaf8b Compare January 17, 2025 11:15
@ibz ibz merged commit 30e88f4 into main Jan 17, 2025
95 checks passed
@ibz ibz deleted the ibz/cancel-hold branch January 17, 2025 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🤝 Needs QA
Development

Successfully merging this pull request may close these issues.

Pressing the left button during "Hold to confirm" action in wallet backup flow breaks the action
2 participants