Finish the nav convention, and confirm with a tick - #14
Merged
Conversation
Four gaps from reviewing the chrome after it landed, all the same shape: a
convention that holds almost everywhere teaches the reader it does not hold.
The Wallet and Origo menus were still on the old list, so the two most-visited
screens in the firmware were the two that looked unlike the rest. They wear the
chrome now, without an arrow: choose_nav grew a `back` flag, the way nav_screen
already had one. Neither screen has a level above it - Origo is the top, and a
loaded wallet's only exit erases the session and reboots. Putting that on the
arrow would give a control that is cheap and reversible on every other screen a
destructive meaning here, one press from any row, which is the same class of
inconsistency the chrome was built to remove. Both keep their exit as a row the
reader travels to. A screen with somewhere to go back to gets an arrow; one
without does not pretend.
The ring arithmetic learned about the missing arrow rather than assuming it:
nav_ring_size, nav_position and nav_selection take `back` and count its slot
only when it is there.
QR export and Descriptor export opened on the confirm bar while every other
warning screen opened on the arrow - including Compact SeedQR, which makes the
same kind of claim. All three say a photograph of what follows gives away more
than the reader may intend; the way forward should not be preselected on any of
them. A test now pins that, matched on the warning text a screen shows rather
than on a list of call sites, so a new screen making one of those claims is
covered the day it is written. Proved by putting one of the two back.
nav_bar_label_fits claimed to hold every screen on the chrome and did not hold
the coin-word reflip warning, which arrived after the table was written.
The old path went with the menus: choose, choose_at and choose_kept had no
callers left, and -Werror=unused-function said so. seedtool_render_list stays
for the self-test's render smoke checks - the linker already drops it from the
image, which is why deleting it would cost diff and save nothing.
Left alone deliberately: the footer hint on the dice, value and brightness
screens. Those have no chrome and up/down there change a value rather than
moving a cursor, so the hint is the only thing telling the reader what the
buttons do. Its comment did describe a string that no longer exists ("L/R
move"), which is fixed.
The image is 320 bytes smaller.
Verified with the host self-test, 52 Python tests, and a firmware build the
audit passes at 303664 bytes.
Rules between the rows and a bordered back button, taken from Jade's Settings screen rather than approximated: ui/dashboard.c:362 builds it, ui/dialogs.c draws it. Three pieces, and the third is why the first two are not enough on their own: - A rule above every row, so rows read as cells rather than as stacked lines of text. Jade borders each menu item and the top one twice (dialogs.c:290). It sits in the gap between cells, where the selection bar never paints over it. - The back arrow gets a box, drawn whether or not it is selected, dim when idle. A bare arrow in the corner reads as decoration on a screen with no other chrome up there; a box reads as something that can be taken. - The box has no bottom edge, and the rule above the first row is what closes it. That is Jade's own arrangement, with its reason in its comment: "bottom edge will be covered by upper line above top menu item" (dialogs.c:268). Without it the two are a button hovering over an unrelated list; with it they are one frame. The box grew a pixel, 18 to 19, so its sides reach the rule instead of stopping short of it. Both list renderers get the rules, not just the nav one: the plain list still draws the Wallet and Origo menus, and a screen that gains the chrome should not also change how its rows are drawn. nav_chrome_bands_do_not_collide had to be told the difference between a rule and a collision. It asserted the gap under the title bar was entirely dark, and the new rule lands there - but its own comment says what it is for, which is catching text that wrapped into the band. So it now permits exactly the rule's colour and still fails on anything brighter, which is what a glyph is. Verified by pushing the title 5px down and watching it fail. Verified with the host self-test, 51 Python tests, and a firmware build the audit passes at 304064 bytes. Walked on hardware.
A screen the reader takes or leaves has two controls and nothing else, so they sit where Jade puts them: reject on the left of the title bar, accept on the right, nothing along the bottom (ui/confirm_address.c builds exactly that, and passes no footer buttons at all). The slot was already there. The title has been centred between two margins the width of the arrow's box since the chrome arrived, so the right margin has been empty and exactly the right size all along - the tick moves in without anything else moving over. Drawn rather than set, for the same reason the back arrow is: the 16px face has no glyph for a tick and Jade's symbols font is a component the audit rejects by name. Two strokes of 2px squares from a common low point, so the diagonals read as solid rather than as a dotted line. What this gives up is the label. The bars said what would happen - Show QR, Open wallet, Proceed anyway - and a tick says only yes. That is why it goes on the screens whose title already names the act, and why the label stays in the struct: the self-test still measures it, and a caller that needs the words back can still draw the bar. The lists keep theirs, where the confirm is one stop of a ring among the items rather than one of two answers. Two self-test checks needed rethinking rather than adjusting, and finding out which took instrumenting all seventeen of the function's exits to say which had fired: - nav_bar_label_fits measured a bar these screens no longer draw. It would have failed loudly, but only because it treats an absent label as a failure - so the screens now assert their slot is drawn instead. On a notice, which has no arrow, the tick is the only control: if it failed to draw, the screen would be one the reader cannot leave, and nothing else here would notice. - nav_title_stays_in_its_column asserts nothing lights right of x=218, to catch a title overflowing its column. The tick's box starts at exactly 218, so the check could no longer tell an overlong title from the control. The title's placement depends only on whether there is an arrow, never on the tick, so it is measured on a tick-free rendering of the same screen. Confirmed still honest by giving one screen an absurd title and watching it fail. Verified with the host self-test, 52 Python tests, and a firmware build the audit passes at 303920 bytes. Walked on hardware.
The confirm was a tick on the screens that are only a question, and still a bar everywhere else - which is the same convention-in-half the branch below this one exists to fix. It is a tick everywhere now: Review words, the numbered lists, the transcript and hash pages. No screen draws a bar, and none of the labels that went with them said anything a tick does not: they were all "Continue". The ring objection turns out to be weaker than it looks. Reading it as a walk down the screen breaks at exactly one point either way - with a bar, moving down off it wrapped up to the arrow; with the tick, the last row wraps to the corner. A ring has one discontinuity wherever it is put, and putting it here sets the two ways out of a screen side by side rather than at opposite ends. Two things this got wrong first, both caught rather than reasoned out: The tick ignored confirm_enabled, so the checksum screen with a word still wrong drew a live-looking control that could not be taken. It has the bar's three states now - filled, outlined, dim. And it drew on menus, which pass no label because their rows are their actions. The bar guarded against that with `if (!nav->confirm) return;` and the tick did not inherit it, so the Origo and Wallet menus each grew a tick offering an answer to a question they never ask. Found on the device, not here, and the reason nothing here found it is worth keeping: every check written for the tick asserted it was *present*. A test that only knows how to see presence cannot see excess. The menu case now asserts both places a confirm could appear stay empty, and was proved by putting the bug back. The self-test's five chrome blocks all built their nav structs by hand without the tick, so they were measuring a layout the firmware had stopped shipping. Each also had to measure its title on a tick-free rendering: the title's column ends exactly where the tick's box begins, so an overlong title landing past it reads as the control. nav_bar_label_fits went with the bars - the compiler pointed out it had no callers left, which is also how it is known no screen draws one. Verified with the host self-test, 52 Python tests, and a firmware build the audit passes at 303936 bytes. Walked on hardware.
"Poor entropy!" and "Pattern detected!" were passed as titles, so they were drawn in the title bar between the arrow and the tick. A title names the screen you are on; these are not names but findings about the run just made, and a finding that the seed may be weak should not sit in the strip a reader learns to skim. They go in the body now, on the two centred lines the acknowledge screens already use, with the bits line under the verdict so the number the accept/reject decision was made on stays visible. The header keeps the arrow and the tick, so the way out and the way on do not move. The self-test's chrome table holds every screen on this chrome with the widest text each can build; its two rows for these screens now carry a NULL title, which is the table saying the empty bar is deliberate rather than an entry someone left unfilled. Verified with the host self-test, and by rendering both screens off the framebuffer and reading the result rather than trusting the geometry.
A tick is the answer to a question. The firmware says so itself where a menu is drawn, which passes no label because "a tick there offers an answer to a question the screen never asked" - and a screen whose result every caller discards asks nothing either. Three kinds of screen were in that position and none was drawn that way: The notices - "Error", "Too long for a QR", "Invalid checksum" and the rest - carried the opposite pair, a tick and no arrow, so the single way out of them wore the glyph for assent. They have the arrow now and no tick. Their label went with the tick: no bar has drawn it since the chrome arrived, so it only ever decided whether the tick appeared. The Safety text and the backup menu's word and number lists were paged screens ending on a confirm whose answer the caller threw away. Each gains a read-only sibling next to the one that keeps it - page_read beside page_text, read_numbered_list beside show_numbered_list - because both callers are real: the same list is a step in the backup sequence, where the tick means "written down", and a thing to browse from a menu, where it means nothing. The difference is one argument, not a second copy of the loop. Two things this got wrong on the way, both found by reading it back: nav_screen toggled the cursor between arrow and tick whenever there was an arrow, so on a screen with no tick it would park the cursor on a control that is not drawn and leave nothing lit. The ring only turns when both controls exist now. And the opening position was still the caller's to get right: a screen with no label but start_on_back false would open on that same undrawn tick. It is derived from the label instead, so the rule cannot be broken by a future caller that does not know it exists. The self-test's notice block asserted only that the tick was drawn. It now asserts the arrow is there and the tick is not - a check that only knows how to see presence cannot see excess. Verified with the host self-test and the 52 Python tests, and by rendering a notice and the Safety page off the framebuffer.
The back arrow's box, and the tick's, are drawn with sides and a top and no bottom. That is deliberate, and Jade's own reason is quoted where it happens: the line underneath would be the fourth edge. On a list that line was the rule above the first cell, so the frame closed and the chrome read as one piece. Every other screen wearing this chrome has no cells to supply it, so the two boxes hung open and nothing crossed the bar. The rule is drawn by the header itself now - a screen gets it for wearing the header rather than for happening to be a list. It is COLOR_DIM, which is the colour the self-test's band checks already permit as chrome, so the bands that must stay clear of text still do. The list's own first rule now repaints the same pixels in the same colour and is left alone: a cell wants a top edge whether or not the header drew one. Its comment no longer claims to be what closes the box. Verified with the host self-test, and by rendering a notice, an acknowledge screen, a paged screen and a list off the framebuffer to confirm the rule crosses on all four and the list neither doubled it nor moved its cells.
The two Stackbit screens were the last ones drawing their own title and no chrome. They had one control - the chord, which left - and it was the one control they never drew, so the way out was something the reader had to already know. They wear the header now, arrow included. There was room without moving anything: the simple grid starts at y=26 and the physical one at y=28, both below the header's rule. The arrow is reachable rather than decorative, which is the part that needed deciding. The words go on the ring the paged screens already walk - position 0 is the arrow, 1..N are the words - so stepping off either end lands on the way out instead of wrapping past it, and the chord reads on from a word exactly as it does from a page. page_step, page_shown and page_nav gained their confirmable argument in the commit below; this reuses it rather than growing a fourth carousel. The carousel no longer wraps forever, which is a real change for anyone who used to spin it: leaving now means walking to the arrow. The self-test's two grid proofs count highlighted pixels to measure the lit punch cells, and a selected arrow fills its box with exactly that colour - the unit area discovered from "0001" would then be wrong for every number after it. Their nav puts the cursor on a word, and says why, so it is not moved onto the arrow by someone tidying later. README described the old controls and now describes these. Verified with the host self-test, which still proves all 2048 punch grids in both layouts with the chrome drawn, the 52 Python tests, the self-test under ASan and UBSan, and a firmware build the ELF audit passes at 303952 bytes. Both layouts were rendered off the framebuffer and read. Flashed to a TTGO T-Display; the screens were not walked through on the device as part of this commit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Eight commits. The first four are the branch as it stood when #12 was parked overnight; the last four are today's, in the order the decisions happened.
Already reviewed in #12
fd81835Finish the nav convention — the Wallet and Origo menus were still on the old list, so the two most-visited screens looked unlike the rest.5bc9ccbFrame the menus like Jade — rules between rows, a bordered back button.6a904dfConfirm with a tick — reject on the left of the title bar, accept on the right, nothing along the bottom.21936e4Take the tick to the lists and the paged screens too.New here
Draw the entropy verdicts where the reader is already looking.
Poor entropy!andPattern detected!were titles. A title names the screen you are on; these are findings about the run just made, and a finding that the seed may be weak should not sit in the strip a reader learns to skim. They move to the body; the header keeps the arrow and the tick.Give the screens that are only read a single way out. A tick is the answer to a question, and a screen whose result every caller discards asks nothing — the argument this repo already makes where menus are drawn. The notices carried the opposite pair, a tick and no arrow, so their single exit wore the glyph for assent. The Safety text and the backup menu's word/number lists ended on a confirm the caller threw away; each gains a read-only sibling (
page_read,read_numbered_list) because both callers are real.Close the header with the rule its boxes are drawn to expect. The arrow and tick boxes are drawn with no bottom edge on purpose — on a list, the first cell's rule was that edge. Every other screen wearing the chrome had no cells to supply it, so the boxes hung open. The header draws it now.
Take the nav convention to the Stackbit carousel. The last screens drawing their own title and no chrome. The words go on the ring the paged screens already walk, so the arrow is reachable rather than decorative.
Behaviour change worth calling out
The Stackbit carousel no longer wraps forever. Leaving now means walking to the arrow, which on 24 words is up to twelve steps. That is the cost of putting the arrow on the ring rather than drawing one that cannot be taken.
Two bugs this would have shipped, found by reading it back
nav_screentoggled the cursor between arrow and tick whenever there was an arrow, so a screen with no tick would park the cursor on an undrawn control and leave nothing lit. And the opening position was still the caller's to get right — a screen with no label butstart_on_backfalse opened on that same undrawn tick. Both are closed, the second by deriving the position from the label rather than leaving it an unwritten rule.The self-test's notice block asserted only that the tick was drawn. It now asserts the arrow is there and the tick is not: a check that only knows how to see presence cannot see excess — the same lesson #12's tick already taught once.
Verification
Every one of the eight commits was built and self-tested individually, not just the tip. On the final tree: 52 Python tests, the QR smoke test, the self-test under ASan and UBSan, a firmware build the ELF audit passes at 303952 bytes (58% of the 512 KiB budget, +16 bytes against
21936e4), and a 3-minute libFuzzer run over the parsers at 1,207,037 executions with no findings.Each changed screen was rendered off the framebuffer and read, rather than trusting the geometry. Flashed to a TTGO T-Display; the screens were not systematically walked through on the device, so panel-specific colour remains unverified by this PR.