Skip to content

Treat \r as logical-line boundary and swallow stray padding spaces#36

Merged
makermelissa merged 1 commit into
adafruit:mainfrom
makermelissa-piclaw:fix-cr-prefix-artifacts
May 20, 2026
Merged

Treat \r as logical-line boundary and swallow stray padding spaces#36
makermelissa merged 1 commit into
adafruit:mainfrom
makermelissa-piclaw:fix-cr-prefix-artifacts

Conversation

@makermelissa-piclaw
Copy link
Copy Markdown
Contributor

Follow-up to #32. That PR preserved \r-based progress updates end-to-end (no more piles of stale Downloading 12% lines scrolling past), but two cosmetic glitches remained, both surfaced during real-world apt + pip runs via adafruit-pitft.py.

Symptoms Melissa hit during a fresh PiTFT install

PITFT Fetched 52.4 MB in 30s (1,729 kB/s)
 PITFT Selecting previously unselected package fonts-droid-fallback.
^ stray leading space, prefix shoved one column right

and, separately, lines like:

PITFT 52.4 MB ...
Selecting previously unselected package ...
^ no PITFT prefix; line just looks orphaned

Root causes

Both reduce to the same modeling mistake: _emit_stream_chunk treated only \n as a logical-line boundary. On a real terminal "logical line start" means "cursor at column 0", which is true after either \n or \r. And once the splitter was wrong, two failure modes followed:

  1. apt emits a stray padding space after \n (its progress UI flushes a \r… clear sequence and sometimes leaves a single space dangling on the next line). The old code: \n → flip at_line_start=True → next segment starts with " " + real content → write PITFT␣ + ␣Selecting…. Result: visible double-space pushing the prefix off-column.
  2. apt also emits bare \r-only "erase progress line" sequences. The old code didn't recognize \r as a boundary, so the next chunk inherited at_line_start=False from the prior write — the prefix on the next real line got dropped entirely.

A third bug fell out of the same model: pip-style \r-only progress bars (10%\r25%\r50%\r100%\nDone) only showed the group prefix on the first frame, because subsequent frames weren't recognized as new logical lines.

What this changes

  • Splits chunks on either \n or runs of \r (\r+), so the splitter matches real terminal semantics, not just newline semantics.
  • Strips leading horizontal whitespace (spaces and tabs only) immediately after a boundary, so padding from the source process doesn't push the prefix right.
  • Suppresses the prefix entirely when a logical line is pure padding, but still writes the terminator (the \r or \n) so the terminal still sees cursor motion.
  • Stays in line-start state when an unterminated tail was pure padding, so the next chunk's real content still gets a prefix.

Popen config and the rest of the streaming machinery from #32 are unchanged.

Verification

Deterministic harness that simulates terminal CR/LF cursor semantics against captured output. Six scenarios, all pass:

Scenario Rendered
Plain multi-line PITFT line one / PITFT line two / PITFT line three
\r-progress PITFT Downloading 100% / PITFT Done
Line split across reads PITFT first half second half
apt stray space (the bug) PITFT Fetched 52.4 MB … / PITFT Selecting previously unselected …
apt \r-pad-\r then new line PITFT Selecting previously unselected …
pip \r-only progress PITFT 100% / PITFT Done

Plus the no-group raw-passthrough regression still passes (plain one / plain two, no prefix bytes injected).

ruff check and ruff format clean. Tested end-to-end on a Pi Zero W with the real adafruit-pitft.py --display=st7789v_bonnet_240x240 install where the original artifact was first observed.

cc @makermelissa — this is the cosmetic follow-up we discussed while testing #341 / adafruit/Raspberry-Pi-Installer-Scripts#379.

@makermelissa-piclaw
Copy link
Copy Markdown
Contributor Author

makermelissa-piclaw commented May 20, 2026

Updated based on review: \r is now treated as in-place cursor-return only (no prefix re-emission), so progress bars animate in place exactly the way pip/apt designed them to. The prefix lands on the first frame, and subsequent redraw frames overwrite it — which is what a real terminal shows anyway.

The user-visible diff narrows to one row in this render, but it's the one Melissa originally hit:

before vs after

Before (post-#32) After (this PR)
stray padding-space after \n PITFT Selecting… (two spaces) PITFT Selecting… (one space)
pip-style \r progress first-frame prefix, redraws bare same — first-frame prefix, redraws overwrite
apt's \r…spaces…\r erase-line next content runs into prior line in capture next content is properly separated; final \n re-arms prefix

So in a real terminal session the visible diff is just the dropped padding-space. The \r semantics matter most when the output is piped to a file/log: each \r-redraw frame is now correctly attributed to "same logical line" rather than "next logical line," which is what _LINE_BOUNDARY_RE and the at_line_start = boundary[0] == "\n" check now express.

Raw before/after PNGs live on a separate host-only branch screenshots-branch so this PR's diff stays code-only (adafruit_shell.py only). Reproducer (Dockerfile + scripts) available on request.

@makermelissa-piclaw makermelissa-piclaw force-pushed the fix-cr-prefix-artifacts branch from 2dccf8c to 846de16 Compare May 20, 2026 20:42
Follow-up to adafruit#32. While that fix preserved \r-based progress updates
end-to-end, two cosmetic glitches remained when running real apt /
pip installs through run_command:

1. apt's progress UI sometimes emits a stray padding space after a
   \n, e.g. 'Fetched 52.4 MB in 30s (1,729 kB/s)\n Selecting...'.
   The previous logic treated the trailing space as the start of a
   fresh logical line, wrote the group prefix, then the space, then
   the real content -- producing output like:

       PITFT Fetched 52.4 MB in 30s (1,729 kB/s)
       PITFT  Selecting previously unselected package ...
              ^ stray space, prefix pushed one column right

2. A bare \r-only line (e.g. apt's 'erase-progress-line' sequence)
   was being treated as 'still on the same logical line', so the
   next chunk's content would be concatenated onto the previous
   line's tail in pipe/log capture, and the prefix bookkeeping for
   the *next* \n boundary could drift.

This change:

* Splits chunks on either \n or runs of \r (\r+), so cursor-at-col-0
  is the actual boundary, not just newline.
* Treats only \n as a *prefix-re-emission* boundary. Bare \r is
  cursor-return: subsequent redraw frames are written without the
  prefix, which is how pip/apt progress UIs are designed to animate
  on a real terminal (each redraw overwrites the previous frame,
  including the prefix, so the terminal naturally shows the latest
  frame with no stale prefix dangling at column 0).
* Strips leading horizontal whitespace (spaces and tabs) immediately
  after a \n boundary, so padding from the source process doesn't
  push the prefix right.
* Suppresses the prefix entirely when a logical line is pure padding,
  but keeps the terminator (\r or \n) so the terminal still sees
  the cursor motion.

Verified end-to-end via a deterministic harness that simulates
terminal CR/LF cursor semantics against captured output, including
the exact apt+pip patterns Melissa saw during the PiTFT install.

ruff check + ruff format clean.
@makermelissa-piclaw makermelissa-piclaw force-pushed the fix-cr-prefix-artifacts branch from 846de16 to 3517fcb Compare May 20, 2026 21:01
@makermelissa-piclaw
Copy link
Copy Markdown
Contributor Author

Refreshed screenshot after the Option E redesign (commit 3517fcb):

BEFORE vs AFTER comparison

What changed in this redesign:

  • \n is now the only logical-line boundary that re-emits the PITFT prefix.
  • Bare \r is treated as pure cursor-return — redraw frames write without a prefix and overwrite the previous frame on a real terminal (pip/apt progress bars now render as a single animating line, the way they're meant to).
  • Leading horizontal whitespace right after \n is stripped, so the apt \n stray-space pattern (PITFT Selecting...) becomes the expected PITFT Selecting....

Reading the comparison:

The only visible textual change between panels is row 3 — PITFT Selecting (double space, BEFORE) vs PITFT Selecting (single space, AFTER). That's the only change the user sees on a live terminal.

Rows 0 (Downloading ... 100% ...) and 5 (the Unpacking ... after \r-erase) look prefix-less in both panels because that's how a real terminal renders them — earlier \r redraws overwrite the original PITFT prefix from col 0. This isn't a regression; it matches what a live terminal session shows when piping apt/pip output through Shell.run_command.

Copy link
Copy Markdown
Collaborator

@makermelissa makermelissa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@makermelissa makermelissa merged commit 99b6c71 into adafruit:main May 20, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants