Pagination for sign/verify message #1384

matejcik · 2020-12-14T11:52:16Z

based on and supersedes #1337

The biggest part of this PR is actually a refactor of the render_text function, which adds three new outward-facing features:

the Span class can be used to count lines in a string
breaking lines on whitespace (enabled by default)
rendering embedded newlines

Includes @mmilata's patch to modtrezordisplay that allows rendering substrings, and takes advantage of this in render_text to avoid slicing passed-in strings. This causes rather solid savings in memory allocations: 114k allocs in text.py vs master's ~147k (i don't have a link to an artifact with this :( )
notably, there are zero allocations in calling render_text

the newlines rendering means that we can use the following going forward
old:

text.normal("Do you really", "want to change", "your PIN?")

new:

text.normal("Do you really\nwant to change\nyour PIN?")

which is an important step in #1005

mmilata

Nice work! Your render_text is much more readable. UI diff looks legit.

core/src/apps/management/apply_settings.py

core/src/apps/common/layout.py

core/src/trezor/ui/text.py

tests/device_tests/test_msg_signmessage.py

core/src/trezor/ui/text.py

matejcik · 2020-12-16T10:15:48Z

added some unit tests in c2e73a1

changed behavior slightly in 49b55b5, this will introduce UI diffs that i won't be fixing at the moment. Feel free to ignore that commit until I update the UI fixtures, or review the diffs and push the necessary changes here if you like.

mmilata · 2020-12-16T18:58:37Z

The diffs are OK, pushed fixtures in eebefff.

matejcik · 2020-12-22T10:37:16Z

this is now ready for review, modulo passing CI checks.

@prusnak you're tagged for reviewing the C rendering code changes

for @mmilata:
the last collection of commits add more unit tests and some docs for Span, plus two API functions that are used in the updated paginate_text function. It is now more readable and more reliable, and also fixes some corner cases (trailing newlines) that were difficult to express properly before.

I implemented the "mock out ui.display" way of unit-testing, the final version is as clean as I believe possible for doing this particular sort of thing :)

There is also a proper UI test for the paginated signmessages

matejcik · 2020-12-22T13:39:11Z

UI diff

mmilata

LGTM!!! Left some comments inline.

Went through the UI diff, the changes are either barely perceptible or a clear improvement to the previous state.

core/src/trezor/ui/text.py

mmilata · 2020-12-28T22:19:36Z

core/src/trezor/ui/text.py

+    def next_line(self) -> bool:
+        """Advance the span to point to contents of the next line.
+
+        Returns True if the rendered should make newline afterwards, False if this is


Typo - rendered?

fixed in a83f110

mmilata · 2020-12-28T22:35:37Z

core/src/trezor/ui/text.py

+    the first character of the indicated item which should be considered.
+    The purpose is to allow rendering different "pages" of text, using the same `items`
+    argument (slicing the list could be expensive in terms of memory).
+    The item selected by `item_offset` must be a string.


Nitpick: paginate_lines violates this statement (item_offset is font) though I don't think it poses a problem, maybe just remove the sentence?

This is kind of a problem actually, it reveals a bug: font instructions that precede item_offset are going to be ignored.

fixed in 02ca00d

mmilata · 2020-12-28T22:55:18Z

core/src/trezor/ui/text.py

-        if isinstance(word, int):
-            if word is BR or word is BR_HALF:
+        if isinstance(item, int):
+            if item is BR or item is BR_HALF:
                # line break or half-line break
                if offset_y > offset_y_max:
                    ui.display.text(offset_x, offset_y, "...", ui.BOLD, ui.GREY, bg)


Shouldn't this also be conditional on render_page_overflow?

fixed in a83f110

mmilata · 2020-12-28T23:18:52Z

tests/device_tests/test_msg_signmessage.py

+    message_read = ""
+    message += "End."
+
+    def input_flow():


Out of scope for this PR but I'm wondering how to generalize this for other tests to use. Would be nice to record fixtures for all the pageable screens in tests that have them.

indeed, we'd need a debuglink indication that a screen is paginated, and ideally a page position as well.

relatively easy to implement, but i'm thinking maybe the redesign of ButtonRequests will solve this too?

matejcik · 2021-01-11T10:37:52Z

rebased on master, added changelogs.

ping @prusnak for C review, otherwise ready to merge

prusnak

Let's use const char * instead of char * to indicate we are not modifying the contents of the string, just slicing it.

core/embed/extmod/modtrezorui/display.c

core/embed/extmod/modtrezorui/display.h

core/embed/extmod/modtrezorui/modtrezorui-display.h

mmilata · 2021-01-11T12:25:27Z

Feel free to cherry-pick 1c9e247 with the suggested changes.

matejcik · 2021-01-11T14:24:01Z

cherry-pick applied

* use less memory due to copy-less rendering * implement linebreaking on embedded \n

mmilata reviewed Dec 14, 2020

View reviewed changes

core/src/apps/management/apply_settings.py Outdated Show resolved Hide resolved

core/src/apps/common/layout.py Show resolved Hide resolved

core/src/trezor/ui/text.py Outdated Show resolved Hide resolved

tests/device_tests/test_msg_signmessage.py Outdated Show resolved Hide resolved

mmilata reviewed Dec 15, 2020

View reviewed changes

core/src/trezor/ui/text.py Show resolved Hide resolved

mmilata reviewed Dec 15, 2020

View reviewed changes

core/src/trezor/ui/text.py Show resolved Hide resolved

mmilata mentioned this pull request Dec 18, 2020

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

Merged

matejcik marked this pull request as ready for review December 22, 2020 10:29

matejcik requested review from onvej-sl, prusnak and tsusanka as code owners December 22, 2020 10:29

matejcik requested review from mmilata and removed request for onvej-sl December 22, 2020 10:30

mmilata approved these changes Dec 28, 2020

View reviewed changes

tsusanka removed their request for review January 6, 2021 12:08

matejcik added a commit that referenced this pull request Jan 11, 2021

docs(core): update changelog for #1271/#1384

2cb10ce

matejcik force-pushed the matejcik/pagination branch from a83f110 to 2cb10ce Compare January 11, 2021 10:37

matejcik mentioned this pull request Jan 11, 2021

feat(core): introduce and enforce limit on label length (fixes #1399) #1403

Merged

prusnak requested changes Jan 11, 2021

View reviewed changes

prusnak approved these changes Jan 11, 2021

View reviewed changes

mmilata and others added 6 commits January 11, 2021 16:46

perf(core): enable rendering of substrings to avoid slicing

f746655

refactor(core): improve render_text behavior

8225613

* use less memory due to copy-less rendering * implement linebreaking on embedded \n

feat(core): implement pagination for sign/verify

925d0f2

feat(tests): dump UI test report as you go

26a4a8e

chore(tests): update UI fixtures as needed

ed0269f

fix(core/monero): make sure to pass strings to rendering

4d92db7

matejcik added 4 commits January 11, 2021 16:46

fix(core/ui): use full line_width for content

029926f

tests(core): add test cases for Span

7a69213

fix(core/ui): render_text uses correct font and color with item_offset

c5d2fe1

docs(core): update changelog for #1271/#1384

ebc67c9

matejcik force-pushed the matejcik/pagination branch from 6541908 to ebc67c9 Compare January 11, 2021 15:47

matejcik merged commit 58708cd into master Jan 11, 2021

matejcik deleted the matejcik/pagination branch January 11, 2021 15:48

mmilata mentioned this pull request Jan 13, 2021

Add pagination for sign/verify #1271

Closed

mmilata mentioned this pull request Apr 2, 2021

Investigate newlines in layouts #1005

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pagination for sign/verify message #1384

Pagination for sign/verify message #1384

matejcik commented Dec 14, 2020 •

edited

mmilata left a comment

matejcik commented Dec 16, 2020

mmilata commented Dec 16, 2020

matejcik commented Dec 22, 2020

matejcik commented Dec 22, 2020

mmilata left a comment

mmilata Dec 28, 2020

matejcik Jan 4, 2021

mmilata Dec 28, 2020

matejcik Jan 4, 2021

matejcik Jan 4, 2021

mmilata Dec 28, 2020

matejcik Jan 4, 2021

mmilata Dec 28, 2020

matejcik Jan 4, 2021

matejcik commented Jan 11, 2021

prusnak left a comment

mmilata commented Jan 11, 2021

matejcik commented Jan 11, 2021

Pagination for sign/verify message #1384

Pagination for sign/verify message #1384

Conversation

matejcik commented Dec 14, 2020 • edited

mmilata left a comment

Choose a reason for hiding this comment

matejcik commented Dec 16, 2020

mmilata commented Dec 16, 2020

matejcik commented Dec 22, 2020

matejcik commented Dec 22, 2020

mmilata 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

Choose a reason for hiding this comment

matejcik commented Jan 11, 2021

prusnak left a comment

Choose a reason for hiding this comment

mmilata commented Jan 11, 2021

matejcik commented Jan 11, 2021

matejcik commented Dec 14, 2020 •

edited