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

RustLayout rework, Global Layout object #3686

Merged
merged 60 commits into from
Nov 12, 2024
Merged

Conversation

matejcik
Copy link
Contributor

@matejcik matejcik commented Apr 7, 2024

Ladies and gentlemen, this is what you've been waiting for

fixes #2299
fixes #3280

Main features:

  1. unified ui.Layout and ui.ProgressLayout classes
    • instead of per-model RustLayout that is kind-of-but-not-really a copypaste job
  2. Global ui.CURRENT_LAYOUT that always unambiguously refers to the layout that is on the screen, or None if the screen is dark
  3. Total overhaul of debuglink internals, leading, hopefully, to greater test stability and less races
    • client.debug.wait_layout() is now almost never needed, except in very special cases. read_layout() Does The Right Thing.
    • deadlock detection when you miss a ButtonAck
    • it "worked on my PC" but i'm sure you guys will figure out a way to break it anyway 🤷‍♀️

Minor niceties

  • timeout for loop.wait() so you usually don't need to use loop.race(xyz, loop.wait(...)) e8990ce
  • loop.chan is unused now. I left it in just in case but we can probably remove it 9f01c9f
  • nice simplification of handle_session now that it does not need to support debuglink 06cad6f
  • bug with make test_emu_ui in zsh not being random is fixed eb65cf6
  • WipeDevice will show a progress screen for the 2 seconds it takes to wipe storage. c98e54b
    • it's "stuck" because we can't animate, but at least the user is not staring at a black screen
  • if a timeout kills your test, you'll see where in the input flow it froze 1a9d022
  • detection of invalid input flows that don't run all the way through fd914bd
  • reduces chattiness of error output when your UI tests are failing (e15c4e8, same as feat(tests): reduce ui error spam #3665)

Supersedes

#2335
#3083
#3665

@matejcik matejcik self-assigned this Apr 7, 2024
@matejcik matejcik requested review from mmilata and obrusvit and removed request for prusnak and andrewkozlik April 7, 2024 12:39
@matejcik matejcik force-pushed the matejcik/global-layout-only3 branch from 11180d3 to 5c45151 Compare April 7, 2024 12:43
Copy link

github-actions bot commented Apr 7, 2024

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

Copy link

github-actions bot commented Apr 7, 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)

@matejcik
Copy link
Contributor Author

matejcik commented Apr 7, 2024

UI changes summary:

  • all progress steps of all loaders are removed, we only screenshot the first time a loader is shown
    • except for "hold to lock", because the loader starts in progress already
  • some "loading transaction" screens are gone
    • previously, a "loading transaction" screen would blink inbetween every Bitcoin confirmation dialog
    • now it only shows up if there is in fact a loading step (so e.g., between the last output and the total confirmation screen there is nothing happening to warrant a loader)
  • some screens are now recorded that are immediately canceled
    • in test_cancel, in cancellation versions of various reset/backup/recovery flows
  • WipeDevice now starts progress earlier

Learn more

https://github.com/trezor/trezor-firmware/blob/matejcik/global-layout-only3/docs/core/misc/layout-lifecycle.md

Reviewer instructions

I strongly recommend going by commits. There's a big one that does most of the things (b75dbe1), the other ones are more reasonable.

Please also point out if you feel that something is not documented that should be.

@matejcik matejcik added the translations Put this label on a PR to run tests in all languages label Apr 7, 2024
@matejcik
Copy link
Contributor Author

matejcik commented Apr 7, 2024

TODO: this definitely wants a changelog entry

@matejcik
Copy link
Contributor Author

matejcik commented Apr 7, 2024

seems that there's a progress related piece of code that I didn't manage to commit. stay tuned for fixes.

@matejcik
Copy link
Contributor Author

matejcik commented Apr 7, 2024

also this does not feel like I added 11k of code, not sure what is going on...

Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

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

Mostly nits, let's fix the backlight & tests and merge soon so it gets some testing before next release.

// invoke the initial placement
new.root_mut().obj_place(constant::screen());
// cause a repaint pass to update the number of pages
let msg = new.obj_event(Event::RequestPaint);
Copy link
Member

Choose a reason for hiding this comment

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

I think until now Event::Attach was the first event sent after construction, pleasantly surprised this change did not break anything.

@@ -27,6 +27,7 @@
MessageType.DoPreauthorized,
MessageType.WipeDevice,
MessageType.SetBusy,
MessageType.Ping,
Copy link
Member

Choose a reason for hiding this comment

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

We can ask Suite just to be sure but given that it's this way on T1 they probably don't rely on it.

core/embed/rust/src/ui/layout/base.rs Outdated Show resolved Hide resolved
// Trezor T < 2.6.0 only - wait until reset word position is requested
optional bool wait_word_pos = 2 [deprecated=true];
// trezor-core only - wait until current layout changes
// changed in 2.6.4: multiple wait types instead of true/false.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: version does not match anymore

self.debuglink().wait_layout(wait_for_external_change=True)

# make sure we start the wait while a layout is up
# TODO should this be part of "wait_for_layout_change"?
Copy link
Member

Choose a reason for hiding this comment

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

Nit: TODO

core/src/trezor/ui/layouts/common.py Show resolved Hide resolved
core/src/trezor/ui/layouts/homescreen.py Outdated Show resolved Hide resolved
core/src/trezor/ui/__init__.py Outdated Show resolved Hide resolved
@@ -0,0 +1,196 @@
# UI Layout Lifecycle
Copy link
Member

Choose a reason for hiding this comment

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

IIRC you mentioned that this is now outdated, though it doesn't seems so on the first look.

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.

Some points, mostly nits.
In addition: docs/event-loop.msd could be updated with mailbox description.

core/embed/rust/src/ui/flow/swipe.rs Outdated Show resolved Hide resolved
core/src/trezor/loop.py Show resolved Hide resolved
core/mocks/generated/trezorui2.pyi Show resolved Hide resolved
core/mocks/generated/trezorui2.pyi Show resolved Hide resolved
core/src/apps/debug/__init__.py Outdated Show resolved Hide resolved
core/src/apps/debug/__init__.py Show resolved Hide resolved
python/src/trezorlib/debuglink.py Outdated Show resolved Hide resolved
docs/core/misc/layout-lifecycle.md Show resolved Hide resolved
docs/core/misc/layout-lifecycle.md Show resolved Hide resolved
@matejcik matejcik force-pushed the matejcik/global-layout-only3 branch from f84d33e to 5d9d97e Compare October 9, 2024 11:52
@matejcik matejcik removed the translations Put this label on a PR to run tests in all languages label Nov 12, 2024
@matejcik matejcik force-pushed the matejcik/global-layout-only3 branch from 80fef26 to 9297722 Compare November 12, 2024 14:55
…eRequest

legacy < 1.11.0 will never answer those because PinRequest and
PassphraseRequest block waiting for PinAck / PassphraseAck over wirelink
and ignore debuglink
@matejcik matejcik force-pushed the matejcik/global-layout-only3 branch from 9297722 to 6b8585b Compare November 12, 2024 15:21
@matejcik matejcik merged commit dd770ba into main Nov 12, 2024
93 of 94 checks passed
@matejcik matejcik deleted the matejcik/global-layout-only3 branch November 12, 2024 15:55
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.

Device recovery tests get stuck on T2B1 UI2: Global layout object
4 participants