UI tweaks for ETH contract flow #4346

ibz · 2024-11-13T16:55:37Z

This is a continuation of #4343, addressing the UI side of #4302.

github-actions · 2024-11-13T16:55:57Z

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

github-actions · 2024-11-13T16:56:03Z

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

while we're at this, can you remove text_mono and info (and possibly even hold) from confirm_blob ?

core/embed/rust/src/ui/component/text/formatted.rs

matejcik · 2024-11-18T10:03:12Z

core/embed/rust/src/ui/model_mercury/flow/confirm_action.rs

+        // we would need to account for that and we could not use a constant.
+        let mut menu_choices = VerticalMenu::empty().danger(
+            theme::ICON_CANCEL,
+            verb_cancel.unwrap_or(TR::buttons__cancel.into()),


why not make verb_cancel non-Optional and prefill this string at construction time?

Not sure what is the benefit. It would just move the check / unwrap at a higher level, most probably in ConfirmBlobParams - because that one can have it missing. Let's say I also set it to TR::buttons__cancel in ConfirmBlobParams, I would still need to make the check in new_confirm_action. Unless I make it non-optional there too and set a default in uPy... Either way, the default would have to be somewhere, no?

Well, yes, we kinda want to work with non-Options from as early as possible. If None here doesn't have a special meaning, it should be unwrapped at the earliest convenient place, which, yes, would be new_confirm_action.
No reason for a code tree this deep to be aware that the caller somewhere six stack frames out was too lazy to specify the value explicitly.

It may make sense to accept a None value from Python, because that's (a) an API boundary and (b) Python doesn't make it convenient to specify translated values as default arguments -- you can't say def confirm(verb: str = TR.some__verb) because then it does not get updated when you change a translation.

But that's a property of the API, that it allows you to not specify a verb. Not a property of the implementation, where we always require a string to work with (unless we don't?), just that we have a default value for it.

OK: fc0bb63

I think I understand your point, and it can be seen like this too, indeed.

I was seeing it from the other angle - as much as possible instantiate these "string bearing structs" like ConfirmBlobParams and ConfirmActionMenuStrings (maybe I should rename it to ConfirmActionMenuParams while at it...) with no parameters, have everything default to None, and do with_XXX only when "XXX" needs to be changed to something other than the default. Otherwise the struct would carry the None all the way to the very end, where the default would be used. I think @obrusvit 's recent changes also went in this direction - dropping as many parameters from ConfirmBlobParams::new and moving them to .with_XXX.

Of course, the two ways are not mutually exclusive, which is what I tried to do at fc0bb6 - not sure whether you guys like it - but basically ConfirmBlobParams::new still means you want to go with defaults, but the default verb_cancel is set to TR::buttons__cancel.into() in the ctor rather than to None - so this "params bearing struct" carries the concrete default around. Maybe if this is what you meant, then I should just replace the rest of the strings in ConfirmBlobParams in the same way... I think I like it...

The disadvantage is of course that multiple instantiations of ConfirmBlobParams around multiple API functions have to unwrap_or if they want to do a .with_verb_cancel (in order to let the uPy code customize the verb. OOOOR, I could make with_verb_cancel accept an Option and have it set the default in case it received None - but I don't know why, that looks less appealing to me (even though the opposite leads to duplication of the default value).

Maybe if this is what you meant, then I should just replace the rest of the strings in ConfirmBlobParams in the same way... I think I like it...

yes, this is what I have in mind

multiple instantiations of ConfirmBlobParams around multiple API functions have to unwrap_or if they want to do a .with_verb_cancel (in order to let the uPy code customize the verb. OOOOR, I could make with_verb_cancel accept an Option and have it set the default in case it received None - but I don't know why, that looks less appealing to me (even though the opposite leads to duplication of the default value).

wondering about this .. would it make sense to construct ConfirmBlobParams and friends by passing the micropython options -- gated by #[feature(micropython)] of course?
wdyt @obrusvit

I think @obrusvit 's recent changes also went in this direction - dropping as many parameters from ConfirmBlobParams::new and moving them to .with_XXX.

My motivation was not to drop as may params as possible from ConfirmBlobParams::new but to keep only those which are used by all callers. There were 7 params and 4 calls were setting the last 4 to None/False Hence I reduced the new params count to 3 and put the rest to with_xyz builder methods.

wondering about this .. would it make sense to construct ConfirmBlobParams and friends by passing the micropython options -- gated by #[feature(micropython)] of course?
wdyt @obrusvit

Do you mean constructing ConfirmBlobParams with (n_args, args, kwargs). Hmmm, I don't like it very much. IMO, ConfirmBlobParams should be deeper in the UI code with rusty-types.
It's true that initially the trait UiFeaturesFirmware (or whatever the name will be) will be riddled with micropython stuff, but mostly just Obj, which should be replaced later on.

matejcik · 2024-11-18T10:04:13Z

core/embed/rust/src/ui/model_mercury/flow/confirm_action.rs

-    let content_menu = Frame::left_aligned("".into(), menu_choices)
-        .with_cancel_button()
-        .with_swipe(Direction::Right, SwipeSettings::immediate());
+        if has_info {


Suggested change

if has_info {

if let Some(verb_info) = verb_info {

assuming (again, dtto) that verb_info default value is prefilled at Menu construction time

OK, something like this I guess: 1eff1d8

Now new_confirm_blob looks strange, but once I remove the info parameter completely, it will look great - basically the decision on whether to show info or not is done by passing Some or None in verb_info.

Turned out slightly different actually, but anyway, we'll discuss this in the next PR, which does not cleanup on confirm_blob.

core/embed/rust/src/ui/model_mercury/flow/confirm_action.rs

[no changelog]

This commit adds a margin and footer description to the first page of the paginated blobs to be confirmed on Mercury. It also extracts the part of confirm_blob that deals with the first page to a separate function in order to keep confirm_blob simple. [no changelog]

[no changelog]

ibz · 2024-11-20T14:12:35Z

while we're at this, can you remove text_mono and info (and possibly even hold) from confirm_blob ?

I'll do that in the next PR if you don't mind, together with cleanup of warning_hi_prio. There's already too many unrelated changes in this PR, and rebasing becomes a pain.

obrusvit

I have some points to discuss.

core/embed/rust/src/ui/model_tr/component/share_words.rs

core/embed/rust/src/ui/model_mercury/flow/confirm_action.rs

obrusvit · 2024-11-22T09:44:06Z

core/embed/rust/src/ui/model_mercury/flow/confirm_action.rs

-    Intro,
-    Menu,
-    Confirm,
+enum ConfirmAction {


I'm surprised this is even needed - no menu means no way to "Cancel"

If there is no menu, there must be a Cancel, according to ConfirmActionExtra.

This is in accordance with the design, BTW:

I think it generally makes sense to have just an "x" to cancel, without a menu, if the only thing you could do from the menu was to cancel.

obrusvit · 2024-11-22T09:56:08Z

core/embed/rust/src/ui/model_mercury/flow/confirm_action.rs

+        // we would need to account for that and we could not use a constant.
+        let mut menu_choices = VerticalMenu::empty().danger(
+            theme::ICON_CANCEL,
+            verb_cancel.unwrap_or(TR::buttons__cancel.into()),


I think @obrusvit 's recent changes also went in this direction - dropping as many parameters from ConfirmBlobParams::new and moving them to .with_XXX.

My motivation was not to drop as may params as possible from ConfirmBlobParams::new but to keep only those which are used by all callers. There were 7 params and 4 calls were setting the last 4 to None/False Hence I reduced the new params count to 3 and put the rest to with_xyz builder methods.

wondering about this .. would it make sense to construct ConfirmBlobParams and friends by passing the micropython options -- gated by #[feature(micropython)] of course?
wdyt @obrusvit

Do you mean constructing ConfirmBlobParams with (n_args, args, kwargs). Hmmm, I don't like it very much. IMO, ConfirmBlobParams should be deeper in the UI code with rusty-types.
It's true that initially the trait UiFeaturesFirmware (or whatever the name will be) will be riddled with micropython stuff, but mostly just Obj, which should be replaced later on.

obrusvit · 2024-11-22T10:03:45Z

core/embed/rust/src/ui/model_mercury/flow/util.rs

@@ -262,9 +269,19 @@ impl ConfirmBlobParams {
        }
        .into_paragraphs();

+        let confirm_extra = if self.cancel {


so cancel param means it's just "Cancel" in the menu but if cancel == false then the menu contains and "Cancel"?

"Cancel" is always there - whether in the menu or as an "x" (if the menu is unnecessary because it just has one option). Perhaps this should be changed, such that it is the case everywhere ... ? Basically there should be no menu with just "cancel" in it? I guess it makes no sense. I did not want to touch other flows here, but I think your concern makes sense, that this is confusing.

obrusvit · 2024-11-22T10:06:14Z

core/embed/rust/src/ui/model_mercury/component/frame.rs

-    content_area = content_area.inset(Insets::top(theme::SPACING));
+    content_area = content_area
+        .inset(Insets::top(theme::SPACING))
+        .inset(Insets::top(margin as i16));


This makes the real margin now SPACING + margin. It's quite confusing but probably fine.

Right. What could I do about it? Rename my parameter to extra_margin? Would that make more sense?

obrusvit · 2024-11-22T10:07:28Z

core/embed/rust/src/ui/model_mercury/layout.rs

@@ -329,6 +317,44 @@ extern "C" fn new_confirm_blob(n_args: usize, args: *const Obj, kwargs: *mut Map
    unsafe { util::try_with_args_and_kwargs(n_args, args, kwargs, block) }
 }

+extern "C" fn new_confirm_blob_intro(n_args: usize, args: *const Obj, kwargs: *mut Map) -> Obj {


I don't love it as it creates mercury specific layout. But probably fine for now.

core/embed/rust/src/ui/model_mercury/flow/confirm_action.rs

ibz self-assigned this Nov 13, 2024

ibz force-pushed the ibz/20241113-tweaks branch 2 times, most recently from 28d80a5 to 3374fa4 Compare November 14, 2024 11:36

ibz changed the base branch from main to ibz/20241112-cleanup November 14, 2024 16:36

ibz force-pushed the ibz/20241113-tweaks branch from 4ee434b to 29da869 Compare November 15, 2024 09:43

ibz linked an issue Nov 15, 2024 that may be closed by this pull request

Improve EVM contract flow #4302

Open

ibz force-pushed the ibz/20241113-tweaks branch from aff8fa9 to 962a2e7 Compare November 15, 2024 11:37

ibz force-pushed the ibz/20241112-cleanup branch from 3aef883 to fc54ad1 Compare November 15, 2024 11:46

ibz force-pushed the ibz/20241113-tweaks branch from 962a2e7 to dbaac78 Compare November 15, 2024 12:03

ibz marked this pull request as ready for review November 15, 2024 12:17

ibz requested review from matejcik and prusnak as code owners November 15, 2024 12:17

ibz requested review from obrusvit and removed request for prusnak November 15, 2024 12:17

ibz force-pushed the ibz/20241113-tweaks branch 2 times, most recently from f4bbfd8 to ee1642a Compare November 15, 2024 12:48

matejcik reviewed Nov 18, 2024

View reviewed changes

ibz force-pushed the ibz/20241112-cleanup branch from 313f806 to 69d448f Compare November 19, 2024 10:29

Base automatically changed from ibz/20241112-cleanup to main November 19, 2024 11:36

ibz added 9 commits November 20, 2024 09:14

feat(core/ui): add page counter to paginated blobs

95a874f

[no changelog]

fixup! feat(core/ui): add page counter to paginated blobs

8313c30

feat(core/ui): add cancel button to paginated blobs

2bc787a

[no changelog]

fixup! feat(core/ui): add cancel button to paginated blobs

33d1def

chore(core/ui): use different copy on TS3 and TT

c25b4bc

[no changelog]

feat(core/ui): continue after viewing all data

4e780f5

[no changelog]

chore(core): remove unused confirm_more on TS5

17cba95

[no changelog]

fix(core/ui): rename button

d2cc6c0

[no changelog]

ibz force-pushed the ibz/20241113-tweaks branch from ee1642a to d2cc6c0 Compare November 20, 2024 12:35

fixup! fixup! feat(core/ui): add cancel button to paginated blobs

fc0bb63

ibz force-pushed the ibz/20241113-tweaks branch from b0a3a4a to fc0bb63 Compare November 20, 2024 14:00

ibz added 2 commits November 20, 2024 16:17

fixup! fixup! fixup! feat(core/ui): add cancel button to paginated blobs

1eff1d8

fixup! feat(core/ui): continue after viewing all data

f4a6756

ibz mentioned this pull request Nov 21, 2024

warning_hi_prio and confirm_blob housekeeping. #4372

Open

obrusvit reviewed Nov 22, 2024

View reviewed changes

ibz added 4 commits November 25, 2024 12:08

fixup! feat(core/ui): add page counter to paginated blobs

1f559ff

fixup! feat(core/ui): add page counter to paginated blobs

c01d4ac

fixup! feat(core/ui): add cancel button to paginated blobs

7218e99

fix(core/ui): fix paginated data size mismatch

da92585

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI tweaks for ETH contract flow #4346

UI tweaks for ETH contract flow #4346

ibz commented Nov 13, 2024 •

edited

Loading

github-actions bot commented Nov 13, 2024 •

edited

Loading

github-actions bot commented Nov 13, 2024 •

edited

Loading

matejcik left a comment

matejcik Nov 18, 2024

ibz Nov 20, 2024

matejcik Nov 20, 2024

ibz Nov 20, 2024 •

edited

Loading

matejcik Nov 20, 2024

obrusvit Nov 22, 2024

matejcik Nov 18, 2024

ibz Nov 20, 2024

ibz Nov 21, 2024

ibz commented Nov 20, 2024

obrusvit left a comment

obrusvit Nov 22, 2024

ibz Nov 25, 2024

obrusvit Nov 22, 2024

obrusvit Nov 22, 2024

ibz Nov 25, 2024

obrusvit Nov 22, 2024

ibz Nov 25, 2024

obrusvit Nov 22, 2024

UI tweaks for ETH contract flow #4346

Are you sure you want to change the base?

UI tweaks for ETH contract flow #4346

Conversation

ibz commented Nov 13, 2024 • edited Loading

github-actions bot commented Nov 13, 2024 • edited Loading

github-actions bot commented Nov 13, 2024 • edited Loading

matejcik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibz Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibz commented Nov 20, 2024

obrusvit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibz commented Nov 13, 2024 •

edited

Loading

github-actions bot commented Nov 13, 2024 •

edited

Loading

github-actions bot commented Nov 13, 2024 •

edited

Loading

ibz Nov 20, 2024 •

edited

Loading