trezor-firmware icon indicating copy to clipboard operation
trezor-firmware copied to clipboard

Rust UI: confirm_properties layout

Open mmilata opened this issue 2 years ago • 1 comments

Part of #1922. The biggest change in this PR is that Paragraphs no longer holds its contents in Vec with hardcoded capacity but any data structure implementing a simple trait can be used. I've attempted to put most of the logic into functions that won't be monomorphized in order to save code size. My current solution has the disadvantage that it needs to allocate large buffer on stack (can be moved to globals) when computing paragraph breaks, LMK if you can think of something better. [UI diff]

TODO:

  • [ ] Put key and value on the same page if possible: python impl.
  • [ ] Test on hardware.
  • [ ] Consider renaming Paragraph to Par/Para to avoid mixups with Paragraphs.

cp1 cp2

mmilata avatar Sep 20 '22 08:09 mmilata

Niiiice

jpochyla avatar Sep 20 '22 14:09 jpochyla

Rebased to master & fixed the SwipeHoldPage issues in dc675ab825a29c417cfab891b95832a1cd26517a.

Switched ParagraphSource to random access in 9cd430fb2c2e04b28fccff05c5b5c19dced2e614. It's using trait object now (seems obvious in retrospect) so the large temporary array is no longer needed.

Implemented key-value-on-same-page in 48becacd8c525fcc6a106c4db3efdddf87a59e9e.

tezos1 tezos2

mmilata avatar Oct 07 '22 23:10 mmilata

One more issue with SwipeHoldPage after the fix: when the Hold to confirm button is pressed, or the loader shrinks, the buttons get cleared and redrawn, causing a flicker. Thinking that SwipeHoldPage should have its own Pad with size of content area returned by place_get_content_area which would be passed into handle_hold_event instead of inner.pad. Note that place_get_content_area doesn't currently return content + scrollbar like described in the comment so this needs to be fixed too in order for this to work properly (it returns content_single_page which is narrower by theme::CONTENT_BORDER, so maybe return layout.content_single_page.union(layout.scrollbar))

TychoVrahe avatar Oct 09 '22 21:10 TychoVrahe

Good catch! Do you just spot these or do you have some flicker-debugging mode implemented?

I've applied your suggestion in 66698ca and it seems to be working. If there are more similar bugs we might want to re-implement the component using Maybe + PaintOverlapping which were added some time after the initial SwipePage implementation.

mmilata avatar Oct 10 '22 12:10 mmilata

I just see it when testing manually on hardware, nothing fancy so far

Seems to be working fine now, so thats it from me

TychoVrahe avatar Oct 10 '22 13:10 TychoVrahe

Rebased on master and fixed ui fixtures, will merge if CI passes.

mmilata avatar Oct 21 '22 13:10 mmilata