Repository navigation
os/v3/puppy: the console TUI is usable over ssh - clickable URLs, a QR for phones, and the mDNS name - #962
Conversation
|
#962 ready for review at 74342f0 (os/v3/puppy: the console TUI is usable over ssh - clickable URLs, a QR for phones, and the mDNS name) Owner's 2026-11-04 01:3x note (item 10): clickable links over ssh, a QR of the web UI URL, the mDNS name. 42/42 cargo test, clippy clean, fmt clean, tools/copyright.py rc 0; both fixtures re-rendered; the pty measurements (TERM=xterm-256color vs TERM=linux OSC 8 counts, q with and without SSH_CONNECTION, 240x67 QR panel) are in the body, and the QR in the 240x67 render was decoded to Two things for the reviewer to know before reading:
|
PR Reviewer Guide 🔍(Review updated until commit 36b10d2)Here are some key observations to aid the review process:
|
|
CI is green at |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 74342f086.
cargo test --offline in os/v3/puppy/console-tui: 42 passed, 0 failed, and the three arms this PR is about are among them —
test ui::tests::every_hyperlink_covers_the_url_that_is_drawn_there ... ok
test ui::tests::the_qr_is_drawn_beside_the_block_only_where_it_fits ... ok
test ui::tests::the_mdns_name_is_on_screen_at_both_sizes ... ok
test qr::tests::the_appliances_own_url_fits_the_block_the_layout_reserves ... ok
plus the gate/layout arms from the earlier work (item_1_keeps_the_gate_string_in_both_key_modes, the_connect_block_says_what_a_blank_key_means, the_c18_gate_row_finds_its_strings_on_this_screen) still green, so this does not unpick the cycle-20 fixes.
The dependency is the right kind. qrcodegen = "1.8.0" is Nayuki's reference implementation, and Cargo.lock shows it with no transitive dependencies — one crate, checksummed. Hand-rolling a QR encoder for an appliance would have been the wrong trade; drawing it in the module (src/qr.rs) and letting the reference encoder own the matrix is the correct split, and drawing on paper (dark-on-light, 2 modules per cell to halve the height) is what a phone scanner needs on a terminal.
Two nits, neither a gate:
src/qr.rs's doc comment says "blocks()is a pure function with a known-answer test". What the suite actually has isthe_appliances_own_url_fits_the_block_the_layout_reserves, which asserts the fit (that a 27-30 character URL lands in the block the layout reserves), not that a given matrix renders to a given set of rows. If a known-answer test exists somewhere else, the comment should say where; if not, either add one (one URL → expected row strings is a two-line test) or reword the comment. As written, the one sentence in the module that would make a reader trust the drawing is the one thing the suite does not do.- Nothing can verify that a drawn QR scans, and that is fine — but the consequence is that the encoder/ECC choice (version 2, ECC L) is the only thing standing between this and an unscannable code on a phone.
qrcodegenmakes that safe, so it is worth keeping the comment that says the matrix is the encoder's and not ours.
Not run: the appliance-side rendering on a real console. The snapshots (tests/snapshots/80x25.txt, 240x67.txt) are updated in this diff and both the 80x25 and 240x67 arms pass, so the layout is checked at the sizes vm-check.sh greps.
Review: "the one sentence in the module that would make a reader trust the drawing is the one thing the
suite does not do" (nit 1), and nit 2's "worth keeping the comment that says the matrix is the encoder's and
not ours".
Nit 1 was right about the phrase, not quite about the coverage: `blocks()` did have a known-answer test
(a hand-drawn 2x2), but its name (`two_module_rows_become_one_text_row`) did not say so and the doc comment
pointed at "a known-answer test" without naming it - so the reader was asked to take it on faith. Both
halves are fixed:
- `two_module_rows_become_one_text_row` -> `blocks_draws_two_module_rows_per_text_row` (the drawing rule
now reads in the name).
- New `a_known_url_renders_to_these_exact_rows`: `code("http://192.168.50.110:13305")` against the 17 rows
of 33 cells written out literally. That is the known answer rather than the fit check
(`the_appliances_own_url_fits_the_block_the_layout_reserves`, kept - it is about the layout's box, not
the drawing). A change to the encoder, the ECC level (L), the version (2) or the quiet zone now fails the
build instead of shipping a code nobody can read; the literal block also makes the quiet zone and the
half-block rule visible in the test itself.
Nit 2, kept and made explicit: the module doc now says the matrix is the encoder's, that the drawing is
what this module owns, that a scan is not asserted by any test - and how it WAS measured: the 240x67 frame
rendered to a PNG and read back with zxing-cpp returned `http://192.168.50.110:13305` (2026-11-04), with
"re-run that by hand if the encoder/ECC/quiet zone changes". The README says the same next to the feature.
Measured on this head: cargo test 43/43 (1 new), clippy --all-targets -D warnings clean, fmt --check clean,
tools/copyright.py --check rc 0, build-static.sh ok (static musl binary fc71d2c720f97213, 1550360 B). The
two snapshots are untouched by this commit - no drawn cell changes.
|
Both nits taken at Nit 1 - the known answer. You were right that the sentence asked for trust the suite did not visibly earn. Nit 2 - kept, and made explicit. The module doc now says plainly: the matrix is the encoder's, the drawing is what this module owns, no test asserts that a drawn code scans — and how that was measured instead: the 240x67 frame rendered to a PNG and read back with zxing-cpp, which returned Measured on |
Review: "the one sentence in the module that would make a reader trust the drawing is the one thing the
suite does not do" (nit 1), and nit 2's "worth keeping the comment that says the matrix is the encoder's and
not ours".
Nit 1 was right about the phrase, not quite about the coverage: `blocks()` did have a known-answer test
(a hand-drawn 2x2), but its name (`two_module_rows_become_one_text_row`) did not say so and the doc comment
pointed at "a known-answer test" without naming it - so the reader was asked to take it on faith. Both
halves are fixed:
- `two_module_rows_become_one_text_row` -> `blocks_draws_two_module_rows_per_text_row` (the drawing rule
now reads in the name).
- New `a_known_url_renders_to_these_exact_rows`: `code("http://192.168.50.110:13305")` against the 17 rows
of 33 cells written out literally. That is the known answer rather than the fit check
(`the_appliances_own_url_fits_the_block_the_layout_reserves`, kept - it is about the layout's box, not
the drawing). A change to the encoder, the ECC level (L), the version (2) or the quiet zone now fails the
build instead of shipping a code nobody can read; the literal block also makes the quiet zone and the
half-block rule visible in the test itself.
Nit 2, kept and made explicit: the module doc now says the matrix is the encoder's, that the drawing is
what this module owns, that a scan is not asserted by any test - and how it WAS measured: the 240x67 frame
rendered to a PNG and read back with zxing-cpp returned `http://192.168.50.110:13305` (2026-11-04), with
"re-run that by hand if the encoder/ECC/quiet zone changes". The README says the same next to the feature.
Measured on this head: cargo test 43/43 (1 new), clippy --all-targets -D warnings clean, fmt --check clean,
tools/copyright.py --check rc 0, build-static.sh ok (static musl binary fc71d2c720f97213, 1550360 B). The
two snapshots are untouched by this commit - no drawn cell changes.
568edb6 to
c65b6b1
Compare
…R for phones, and the mDNS name Owner, 2026-11-04 01:3x: "due to being headless, can you make the links clickable, because i'm on kvm and i want use it through ryzen", plus a QR code of the web UI URL and the mDNS name on the CONNECT block. The TUI already made no tty1 assumption, but three things assumed the VGA console, so using it from a terminal emulator was strictly worse than being at the box: the URLs were text nobody can click, the only way to reach the web UI from a phone was to retype 27 characters off a screen, and `q` was refused even where a prompt is harmless. Each of the three now depends on the terminal it actually finds itself on, decided in one place and reported in the footer: - OSC 8 hyperlinks on every URL in CONNECT when TERM is a terminal emulator (xterm-256color, tmux, kitty, ...), none when it is `linux`/`dumb` (VGA console, JetKVM, serial - there the escapes would be printed). ratatui has no hyperlink style, so the links are written AFTER the frame at the cells `ui::draw` reports: the drawn text is the link target, which `every_hyperlink_covers_the_url_that_is_drawn_there` asserts off the rendered frame at 80x25 and 240x67. A URL that wrapping split is not a link - half a URL opens the wrong thing. The link is re-stated every frame and styled with the row's own white+bold, so it keeps the pixel it had plus the underline (an `\x1b[4m` alone would inherit whatever colour the last cell left). - A QR code of the web UI URL beside the CONNECT rows, two modules per cell (17 rows for the appliance's 27-character URL: version 2, ECC L), drawn BLACK ON WHITE because a scanner wants dark-on-light and a terminal's dark background is the inversion many phone scanners refuse. Decoded end to end, not reasoned about: zxing-cpp on the PNG built from the 240x67 render's own cells returns `http://192.168.50.110:13305`. - `http://1bit-os.local:13305` on the CONNECT block, with `(mDNS)` when the box's `avahi-daemon` is up and `(mDNS - avahi not running)` when it is not, so the row never promises a name that will not answer. The responder itself is pi-os3's half (engine #953, still open) - this PR reads it, it does not install it. - `q` quits when started from an ssh session (sshd sets SSH_CONNECTION/SSH_TTY; a tty1 respawn sets neither) and still says "q stays" on tty1, which keeps the promise that console never drops to a bare prompt. The footer says which one you are looking at, from the same fact the key uses. Not done on purpose: the QR is NOT at 80x25. The code needs 33 columns and 17 rows; at 80x25 the eight CONNECT rows, the ten MENU rows, the three log rows and the footer already account for all 25 console rows, so there is nothing to put it in without deleting one of the things the owner asked for. It is drawn wherever it fits (tty1 is 240x67; an ssh window is normally wider than 104 columns), and at 80x25 the URL stays plain text - which is also the size the QEMU gate reads, so `U2 menu connect` is unaffected. Measured (pty, the musl release binary from build-static.sh, d225399adbf7a119, 1550384 bytes): - TERM=xterm-256color at 80x25: 3 distinct OSC 8 targets written per frame - web UI, api base, mDNS name - each at the cell its text occupies; TERM=linux: 0 bytes of OSC 8, frames byte-identical to the previous build at the URL cells (checked, not assumed). - `q` with SSH_CONNECTION set: exit 0. `q` without it (env -u SSH_CONNECTION -u SSH_TTY): the screen stays and the footer says "q stays: no bare prompt here (7 shell, 9 reboot)". - 240x67: the panel is 33 columns at x=206, rows 3-19, all nine menu rows and 40 log rows still below it. - cargo test 42/42 (10 new: the links cover their own URL, a split URL is not a link, the QR is drawn only where it fits, the QR block is square 33x17 with a quiet zone, the mDNS row in both states at both sizes, the footer in both worlds, the ssh/hyperlink predicates), clippy --all-targets clean, fmt clean, tools/copyright.py --check rc 0. New dependency: `qrcodegen` 1.8 (Nayuki's reference encoder, MIT, no transitive dependencies, ~700 lines). It is compiled into the static binary on the build host; the image build still only *installs* the binary (CONSOLE_TUI_BIN), so the appliance gains no toolchain and no runtime dependency.
Review: "the one sentence in the module that would make a reader trust the drawing is the one thing the
suite does not do" (nit 1), and nit 2's "worth keeping the comment that says the matrix is the encoder's and
not ours".
Nit 1 was right about the phrase, not quite about the coverage: `blocks()` did have a known-answer test
(a hand-drawn 2x2), but its name (`two_module_rows_become_one_text_row`) did not say so and the doc comment
pointed at "a known-answer test" without naming it - so the reader was asked to take it on faith. Both
halves are fixed:
- `two_module_rows_become_one_text_row` -> `blocks_draws_two_module_rows_per_text_row` (the drawing rule
now reads in the name).
- New `a_known_url_renders_to_these_exact_rows`: `code("http://192.168.50.110:13305")` against the 17 rows
of 33 cells written out literally. That is the known answer rather than the fit check
(`the_appliances_own_url_fits_the_block_the_layout_reserves`, kept - it is about the layout's box, not
the drawing). A change to the encoder, the ECC level (L), the version (2) or the quiet zone now fails the
build instead of shipping a code nobody can read; the literal block also makes the quiet zone and the
half-block rule visible in the test itself.
Nit 2, kept and made explicit: the module doc now says the matrix is the encoder's, that the drawing is
what this module owns, that a scan is not asserted by any test - and how it WAS measured: the 240x67 frame
rendered to a PNG and read back with zxing-cpp returned `http://192.168.50.110:13305` (2026-11-04), with
"re-run that by hand if the encoder/ECC/quiet zone changes". The README says the same next to the feature.
Measured on this head: cargo test 43/43 (1 new), clippy --all-targets -D warnings clean, fmt --check clean,
tools/copyright.py --check rc 0, build-static.sh ok (static musl binary fc71d2c720f97213, 1550360 B). The
two snapshots are untouched by this commit - no drawn cell changes.
c65b6b1 to
d86043e
Compare
|
Rebased onto So at merge time: if the button is grey, press Update branch (safe here — squash merge, no merge commit) or say the word and I will rebase again. Nothing else is pending on this PR: my two nit fixes are in, 0 review threads, no device ticket required. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d86043e22. Both nits from my review are resolved, and I re-ran the suite at this head rather than taking the message's word.
$ cargo test --offline (os/v3/puppy/console-tui)
test result: ok. 43 passed; 0 failed; 0 ignored; 0 measured (was 42; the new arm is the known answer)
test qr::tests::a_known_url_renders_to_these_exact_rows ... ok
test qr::tests::blocks_draws_two_module_rows_per_text_row ... ok
test qr::tests::a_url_too_long_for_any_code_is_absent_rather_than_wrong ... ok
test ui::tests::every_hyperlink_covers_the_url_that_is_drawn_there ... ok
Nit 2 (my real one) is closed properly. There is now a literal known answer — code("http://192.168.50.110:13305") against all 17 rows written out — so a change to the encoder, the ECC level, the version or the quiet zone fails the build rather than shipping a code nobody can read. The rename two_module_rows_become_one_text_row → blocks_draws_two_module_rows_per_text_row makes the drawing rule visible in the test name, and the doc comment now points at the test by name instead of saying "a known-answer test" and leaving the reader to find it.
And it answers the question I could not: the module doc now states that no test asserts the code scans, and records how it was measured once — the 240×67 frame rendered to a PNG and read back with zxing-cpp, returning the URL — with an instruction to redo that by hand if the encoder/ECC/quiet zone changes. That is the honest form for something a unit test cannot check, and it means the next person inherits a measurement rather than an assumption.
The dependency is still the right one (qrcodegen = "1.8.0", no transitive deps), and nothing in this commit touches the snapshots or the cycle-20 gate strings.
Not run by me: the appliance-side rendering on a real console, and I did not redo the zxing scan — the claim is that it was done on this box on 2026-11-04, and the literal rows are what a re-run compares against.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 36b10d27e. The only commits since my verification at d86043e22 are main merges (#955, #953, #964, #960), so the content I ran is this content.
At d86043e22: cargo test --offline → 43 passed, 0 failed, including the two arms this PR's last push added —
test qr::tests::a_known_url_renders_to_these_exact_rows ... ok
test qr::tests::blocks_draws_two_module_rows_per_text_row ... ok
— i.e. my nit is closed by a literal known answer (a URL against all 17 rows), and the module doc now states that no test asserts the code scans and records how that was measured once (240×67 frame → PNG → zxing-cpp → the URL). Nothing in the merged history touches the snapshots or the cycle-20 gate strings.
|
Persistent review updated to latest commit 36b10d2 |
Owner, 2026-11-04 01:3x: "due to being headless, can you make the links clickable, because i'm on kvm and i want use it through ryzen" — plus the QR code of the web UI URL and the mDNS name from the same note (item 10 of
TOS3-C18-items.md).The TUI already made no
tty1assumption, but three things assumed the VGA console, so using it from a terminal emulator was strictly worse than standing at the box. Each now depends on the terminal it finds itself on, decided in one place and reported in the footer:1. Clickable URLs (OSC 8) — ssh in, click the web UI
TERMis a terminal emulator (xterm-256color,tmux,kitty, …) → every URL in CONNECT is an OSC 8 hyperlink.TERM=linux/dumb(VGA console, JetKVM, serial) → none, because there they would be printed as escapes.ui::drawreports inLink, with the drawn text as the target.every_hyperlink_covers_the_url_that_is_drawn_thereasserts exactly that off the rendered frame at 240x67 and 80x25.\x1b[4mwould inherit whatever colour the last drawn cell left current.2. A QR code of the web UI URL, for a phone
Two modules per cell, so 17 rows for the appliance's 27-character URL (version 2, ECC L), drawn black on white — a scanner wants dark-on-light and a terminal's dark background is the inversion many phone scanners refuse. Decoded end to end, not reasoned about: zxing-cpp reading the PNG built from the 240x67 render's own cells returns
http://192.168.50.110:13305.Deliberately not at 80x25: the code needs 33 columns and 17 rows; at 80x25 the eight CONNECT rows, ten MENU rows, three log rows and the footer already account for all 25 console rows, so it would have to displace one of the things the owner asked for. It is drawn wherever it fits (tty1 is 240x67 at 1920x1080; an ssh window is normally >104 columns); at 80x25 the URL stays plain text. That is also the size the QEMU gate reads, so
U2 menu connectis untouched.3.
http://1bit-os.local:13305, with its state(mDNS)when the box'savahi-daemonis running,(mDNS - avahi not running)when it is not — the row never promises a name that will not answer. The responder itself is pi-os3's half (engine #953, still open); this PR reads avahi from the same singlepssnapshot aslemondand installs nothing.4.
qover sshsshdsetsSSH_CONNECTION/SSH_TTY; atty1respawn sets neither. Over sshqquits (as in every full-screen program), on tty1 it still stays — that console never drops to a bare prompt — and the footer says which one you are looking at.Measured (pty, the static musl binary from
build-static.sh:d225399adbf7a119, 1,550,384 B)TERM=xterm-256color, 80x25, raw pty bytes38;5;15m 1m 4m(the row's white+bold plus underline)TERM=linux, 80x25qwithSSH_CONNECTIONqwithenv -u SSH_CONNECTION -u SSH_TTYq stays: no bare prompt here (7 shell, 9 reboot)namerow and the footer: menu rows 10–18, log title 19, log 20–24, footer 25cargo test42/42 (10 new),cargo clippy --all-targets -D warningsclean,cargo fmt --checkclean,tools/copyright.py --checkrc 0. The 80x25/240x67 snapshots are re-rendered (footer +namerow).New dependency:
qrcodegen1.8 (Nayuki's reference encoder, MIT, no transitive dependencies). It is compiled into the static binary on the build host; the image build still only installs the binary (CONSOLE_TUI_BIN), so the appliance gains no toolchain and no runtime dependency.No GPU/NPU ticket, no image build, no QEMU run, no merge, no box config change: this is a TUI-only change and the gate's serial console (
ttyS0) never runs the TUI.