layouts: simple confirmations and warnings

[mmilata]

This generalizes the confirm_action layout a bit and most of the relevant application code is modified to use it. Same with show_success/show_warning/show_error.

UI diff

[andrewkozlik]

Looking at the UI changes, I can see that the rendering mechanism adds a space after each parameter, e.g. after 10 seconds? is now after 10 seconds ?, which is incorrect. Hmm, I am guessing that this is due to the fact that format_parametrized() strips the spaces and render_text() puts them back, which is wrong if no space was stripped. So why do we strip them in the first place?

[matejcik]

why do we strip them in the first place?

because otherwise there will be two spaces.
we can modify render_text to not add a space, but that entails reviewing all places that rely on this behavior, which is a lot IIRC

[andrewkozlik]

why do we strip them in the first place?

because otherwise there will be two spaces.
we can modify render_text to not add a space, but that entails reviewing all places that rely on this behavior, which is a lot IIRC

So let me rephrase the question. Why do we add the spaces? Do we have any examples that rely on this behavior?

[mmilata]

I don't know the original reasoning but at this point I'd say we add the spaces in render_text to stay compatible with the previous implementation. Given text = Text("title"); text.normal("foo", "bar"), there is going to be either linebreak or space between the two words. As a result there's no way to render two characters of different font next to each other, which would be the right thing to do here.

We probably could change the behavior and update all invocations to be like text.normal("foo ", "bar"), however I'd propose to instead solve this by making the ? part of the parameter (and thus bold), which:

• makes the screen identical to what we currently have in master, and
• does not make the ongoing Rust rewrite of the render_text harder.

[andrewkozlik]

We probably could change the behavior and update all invocations to be like text.normal("foo ", "bar"), however I'd propose to instead solve this by making the ? part of the parameter (and thus bold), which:

• makes the screen identical to what we currently have in master, and

• does not make the ongoing Rust rewrite of the render_text harder.

It seems to me that the correct solution would be not to strip spaces and not to add them at all. That way, if new_lines=True then text.normal("foo", "bar") remains as is and if new_lines=False, then it changes to text.normal("foo bar"). I don't think we need to worry about making the screens identical to what we currently have in master, because it's not always correct. Like the question mark, for example, really shouldn't be bold. I can't judge the impact on the ongoing Rust rewrite, but not sure if that should be a factor.

[mmilata]

So I removed the spaces in 2febef0 and not much was needed to make UI tests pass: UI diff from master.

Still this change probably introduces some corner case that will break something not covered by test. The code converted to layouts mostly uses "single strings\nwith embedded\nnewlines" which is fine, I'll need to review the rest while converting it to layouts.

[mmilata]

Added two more commits to finish apps.bitcoin conversion. UI diff

[tsusanka]

LGTM with the exception of the "require" usage. As discussed on standup it seems safer to have "require" by default and explicitly omit "requiredness" if necessary.

[tsusanka]

I did not test, but it LGTM and solves what I meant. I am not sure about the naming not_cancelled, maybe can_cancel sounds better? But I don't want to hold up this PR on that so let's merge unless @matejcik objects.

[matejcik]

I'd prefer to look this over before it is merged. If I'm blocking something, I'll move this up my priority list :) just let me know

[tsusanka]

@matejcik I apologize, I thought you saw it already. No prob, let's wait for you :).

[mmilata]

Mind if I rebase this to master?

[matejcik]

Mind if I rebase this to master?

go ahead

[mmilata]

Rebased. UI diff

Compared to master the total allocation count in device tests increased by 42488 (0.3 %). master -> this branch

[matejcik]

Many comments that are actually one comment: ISTM there is a better alternative to not_cancelled function.

Larger comment: confirm_action has way too many parameters now that should go away in the future. I am assuming all the "TODO cleanup" means that you're leaving them in for now for the sake of a clean UI diff?

For the most part though, this is shaping up wonderfully.

[mmilata]

Thanks for the review @matejcik! Yes, the parameters with the comment are expected to disappear but exist for the sake of changing the UI as little as possible.

Any idea how to verify that the PR doesn't make our fragmentation issues worse? Checking whether individual tests pass/fail on an actual hardware is very time consuming. Perhaps we can run the tests on an emulator repeatedly, decreasing the heap size each iteration until some number of tests start to fail, then compare with master?

[matejcik]

Perhaps we can run the tests on an emulator repeatedly, decreasing the heap size each iteration until some number of tests start to fail, then compare with master?

this is reasonable. for starters, i'd like to get test_15_of_15 to fail on emulator, which might give us a reasonable bound for the heap size

also i wouldn't merge this into the upcoming release, which means we will most likely get the workflow-restart thing so that fragmentation is much less of an issue

[matejcik]

the sdcard thing is looking much neater now ❤️

i believe this is it. but let's discuss before merging if we want this to go to the upcoming release

[mmilata]

So I've tested this with

core/emu.py --disable-animation --temporary-profile --heap-size=460K &
TESTOPTS="--random-order-bucket=none" make test_emu

and it works but with --heap-size=450K it fails on test_15_of_15. These two bounds are the same for master so it probably doesn't make the fragmentation much worse.

[matejcik]

let's get this merged

@mmilata please squash, rebase and fix conflicts

[mmilata]

Rebased, squashed, tests pass. Re-ran the heap sanity test, minimum --heap-size value is 459K for this branch and >=460K for master. Will merge this in the evening if there are no objections.