fix(tui): center the QR screen as one cohesive block - #68
Merged
Merged
Conversation
The 'Scan with Amber' screen laid out the title, QR, 'Waiting for connection...' footer, and 'Press Esc...' global footer at four different indents, so they read as four disjoint pieces instead of one screen. Cause: two separate centering passes and per-line width mismatches between the QR block and the global footer. What was wrong: 1. viewQRLogin called lipgloss.Place (which pads vertically to m.height), then the outer View() called Place AGAIN on the combined output. Each pad inflated the result vertically; on 24-row terminals the output was 42-80 lines tall and the QR fell below the visible area. 2. viewQRLogin's title is rendered with titleStyle, which uses lipgloss.MarginBottom(1). lipgloss renders that as a trailing 'newline + spaces-to-match-width' — invisible but real to ansi.PrintableRuneWidth, which counts those spaces. So titleStyle.Render(<title>) reports a printable width of 36, not 16, and lipgloss.Place put it at a different column than the 49-col QR. 3. The 'Press Esc to go back, Ctrl+C to quit' footer was appended by the outer View() AFTER viewQRLogin returned, so it sat in a separate centering pass and landed at yet another column. Fix: - Drop the inner lipgloss.Place in viewQRLogin; let the outer View() center the whole screen. - Render viewQRLogin as a single block: title + QR + body footer + global 'Press Esc' footer, all right-padded to the same printable width (qrWidth, the QR's widest line). Then View()'s one Place pass centers them as one rectangle. - Drop titleStyle.MarginBottom(1) — it stopped being meaningful once we explicitly emit our own '\n\n' between the title and the QR, and it was the source of the off-by-one width discrepancy. - View() no longer appends the global 'Press Esc' footer for ScreenQRLogin; viewQRLogin emits it as part of its padded block. Width measurement: use muesli/reflow/ansi.PrintableRuneWidth (same as lipgloss) so the per-line width I see in viewQRLogin matches the per-line width lipgloss.Place computes inside PlaceHorizontal. runewidth.StringWidth counted emoji as 2 cols and would have left a one-column offset between title and QR. Verified across terminal widths 40x20 through 160x50: the title, QR first row, 'Waiting for connection' footer, and 'Press Esc' footer all share the same leftmost column. On 80x24 specifically every line starts at column 12. Adds TestQRViewLayout dumping the View() at six sizes and asserting each anchor shares the same indent.
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
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.
Summary
The "Scan with Amber" QR screen laid out the title, QR, "Waiting for connection…" footer, and "Press Esc…" global footer at four different indents — so it read as four disjoint pieces rather than one screen.
Root cause
Three independent issues, all around centering:
Double
lipgloss.Place.viewQRLogincalledPlace(which pads vertically tom.height), and then the outerView()calledPlaceagain on the combined output. Each pass added vertical padding to reachm.height, so on a 24-row terminal the output was 42-80 lines tall — the QR dropped below the visible area.titleStyle.MarginBottom(1)widens the title invisibly.lipgloss.NewStyle().MarginBottom(1).Render("📱 Scan with Amber")produces a string with a trailing\n+ spaces-to-match-width. The spaces don't show on screen butansi.PrintableRuneWidth(whichlipgloss.Placeuses) counts them. The title reported a width of 36 instead of 16, soPlaceHorizontalgave the title and the 49-col QR different left pad.The global "Press Esc" footer was appended outside the QR block.
View()appended it afterviewQRLoginreturned, in a second centering pass, so it landed at yet another column.Fix
tui/tui.go:lipgloss.PlaceinviewQRLogin— let the outerView()center the whole screen.View()'s singlePlacepass centers them as a single rectangle.titleStyle.MarginBottom(1)— it stopped being meaningful once we emitted our own\n\nbetween title and QR, and it was the off-by-one source of the indent misalignment.View()no longer appends the global "Press Esc" footer forScreenQRLogin;viewQRLoginemits it as the last line of its padded block.muesli/reflow/ansi.PrintableRuneWidth(the same helperlipgloss.Placeuses insidePlaceHorizontal). Usingrunewidth.StringWidthwould have left a one-column offset because it counts wide emoji as 2 cols.Verification
New
tui/tui_qr_layout_test.go(TestQRViewLayout) drives the QR screen through Update → View at six sizes (40×20, 60×20, 80×24, 100×30, 120×40, 160×50), asserts the title / first QR row / "Waiting for connection" / "Press Esc" anchors all share the same leftmost column, and dumps the rendered View to a temp file for human inspection.Across sizes the four anchors share the same column:
Visual confirmation at 80×24 (the canonical case):
Every line starts at column 12. The whole screen reads as one block.
Tests
TestQRViewLayout(new) — covered above.TestQRGeneratesAfterInitMessage,TestQRGeneratesWhenSizeMessageArrivesAfterQRData,TestQRFlowsThroughLoginMenuEndToEnd,TestQRVisualFitsTerminalWidth) all still pass — confirming the centered layout didn't regress generation, content visibility, or width budgets.go test -count=1 ./...passes locally and CI will run test + 6 build matrix jobs on push.