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.

Sign up for GitHub

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

Jump to bottom

Introduce layouts (formerly widgets) to prepare for redesign #1395

Merged
merged 5 commits into from
Feb 10, 2021
Merged

Introduce layouts (formerly widgets) to prepare for redesign #1395

merged 5 commits into from
Feb 10, 2021

Conversation

mmilata
Copy link
Member

@mmilata mmilata commented Dec 18, 2020 •
edited

Most changes in this PR come from #1312, except there are no UI bits for T1, and the layouts have been tweaked to pass most UI tests.

Changes:

  • UI components moved to model-specific directories, for some of them the model-independent code has been factored out
  • some dialogs have been converted to the Layout abstraction (formerly Widget)

To be done:

  • convert all app.* code to use Layouts, remove direct imports of trezor.ui.components.tt.*
  • update ButtonRequest message to include string field "type" (currently it is just written to log)

There are some spacing differences in the UI diff, if these are a problem they can probably be worked around.

@mmilata mmilata force-pushed the mmilata/layouts-refactor branch 3 times, most recently from 5b075ba to bd2da0b Compare January 5, 2021 11:21
@mmilata mmilata force-pushed the mmilata/layouts-refactor branch 3 times, most recently from 4bd2517 to 27d9211 Compare January 8, 2021 12:26
@matejcik matejcik force-pushed the matejcik/pagination branch 2 times, most recently from 6541908 to ebc67c9 Compare January 11, 2021 15:47
Base automatically changed from matejcik/pagination to master January 11, 2021 15:48
@mmilata mmilata force-pushed the mmilata/layouts-refactor branch 2 times, most recently from 432b1e7 to 1cb7383 Compare January 12, 2021 01:30
@mmilata mmilata marked this pull request as ready for review January 13, 2021 12:46
@tsusanka tsusanka added this to the 21.03 milestone Jan 20, 2021
matejcik
matejcik requested changes Jan 20, 2021
View reviewed changes
Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

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

minor comments

overall: why did you choose to have show_pubkey accept str instead of bytes?
not saying it's wrong (i did the same in confirm_hex or something), just wondering about reasoning and whether in case of show_pubkey the hexlification could be done in the layout; we know that pubkey is bytes

otherwise basically LGTM, let's wait until the release branch, and then rebase&merge

core/src/apps/tezos/get_public_key.py Outdated Show resolved Hide resolved
core/src/trezor/ui/components/tt/confirm.py Outdated Show resolved Hide resolved
core/src/trezor/ui/layouts/tt.py Outdated Show resolved Hide resolved
@mmilata
Copy link
Member Author

mmilata commented Jan 27, 2021

Addressed the comments and rebased to master. Conflicts:

  • apps/bitcoin/get_address.py (xpub magic, "others" -> "cosigner")
  • apps/bitcoin/sign_tx/layout.py (amount units)
  • apps/cardano/layout.py (map account paths to account numbers) - rolled changes back to what's in master

@mmilata
Copy link
Member Author

mmilata commented Feb 8, 2021

Rebased on master to resolve conflicts in fixtures. Ready to merge? UI diff

matejcik
matejcik approved these changes Feb 9, 2021
View reviewed changes
Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

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

approval from me

but pinging @tsusanka, this is big enough that i'd like another pair of eyes

tsusanka
tsusanka approved these changes Feb 9, 2021
View reviewed changes
Copy link
Contributor

@tsusanka tsusanka left a comment

Choose a reason for hiding this comment

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

LGTM! I have skimmed through the code and discussed few things on-call with @mmilata. Let's merge it! 🎉

tsusanka
tsusanka approved these changes Feb 9, 2021
View reviewed changes
Copy link
Contributor

@tsusanka tsusanka left a comment

Choose a reason for hiding this comment

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

LGTM! I have skimmed through the code and discussed few things with @mmilata on-call. Let's merge this!

tsusanka
tsusanka approved these changes Feb 9, 2021
View reviewed changes
Copy link
Contributor

@tsusanka tsusanka left a comment

Choose a reason for hiding this comment

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

LGTM! I have skimmed through the code and discussed few things with @mmilata on-call. Let's merge this!

@tsusanka
Copy link
Contributor

tsusanka commented Feb 9, 2021

Due to Github's 500 I have approved 3x times 🙄.

@mmilata
Copy link
Member Author

mmilata commented Feb 9, 2021

Thanks a lot for reviewing this thing guys! @matejcik can you please look at this little fix: f9f91a2? Without it the Cancel button on get_public_key confirm doesn't actually cancel the operation.

@matejcik
Copy link
Contributor

Thanks a lot for reviewing this thing guys! @matejcik can you please look at this little fix: f9f91a2? Without it the Cancel button on get_public_key confirm doesn't actually cancel the operation.

sure, LGTM.
(i mean it is not meaningfully changing anything if the Cancel does not work here but, well)

@mmilata
refactor(core): model-dependent UI component directories
77216f6 
They now live under trezor.ui.components.tt. Later
trezor.ui.components.t1 will be added and application code will be
rewritten to not use them directly in order to work on both TT and T1.
@mmilata
feat(core): introduce Text.format_parametrized()
a0819da
@mmilata
refactor(core): introduce layouts
f82d388 
Layouts can be used by the application code to interact with user using
small number of dialogs or other groups of UI components. Each layout is
identified by name and takes some parameters. Most layouts will have an
implementation for each hardware model, mechanism is provided to import
the correct version so that application code can be oblivious to the
model.

This commit introduces the layout concept and converts a couple of
dialogs to use it.
@mmilata
refactor(core): turn show_success, show_warning into layouts
0578433
@mmilata
refactor(core): turn show_address, show_pubkey, show_xpub into layouts
2d92a49
@mmilata
Copy link
Member Author

mmilata commented Feb 10, 2021

Rebased and fixed a typing issue exposed by bf562cf.

(i mean it is not meaningfully changing anything if the Cancel does not work here but, well)

The Cancel works in current master but was broken in this branch which was fixed. Or am I misunderstanding you?

@mmilata mmilata merged commit 391602a into master Feb 10, 2021
@matejcik
Copy link
Contributor

it works, but it's meaningless on a GetPublicKey message, because a potential attacker can obtain the pubkey silently anyway. I suppose we should remove the cancel button in the redesign.

@matejcik matejcik deleted the mmilata/layouts-refactor branch February 10, 2021 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matejcik matejcik matejcik approved these changes

@tsusanka tsusanka tsusanka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
21.03
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mmilata @tsusanka @matejcik