A plain line printed outside any operation lands below the tracked
block, so the block is no longer directly above the cursor. Reset the
screen so the next operation repaints a fresh block instead of moving
the cursor over rows that shifted — the same reason unsuspend resets
after buildkit output.
Suggested-by: Guillaume Lours <[email protected]>
Co-Authored-By: Claude Fable 5 <[email protected]>
Signed-off-by: Nicolas De Loof <[email protected]>
- layoutFrame clips its output to the terminal height: height==1 no
longer overflows with header + more-marker
- the two status-to-color switches merge into eventColor(status, def)
- the clear-suspension sequence extracts into unsuspend()
- tests drive a manually-advanced fake clock through the injected
now(), and the cancel test loses its settling sleep — Done waits for
the refresh goroutine itself
- roots, per-parent children and the completed count are maintained
incrementally by apply(), so a 100ms tick with an unchanged tree no
longer rescans every task
- the frame<0 clamp in spinGlyph drops: time comes from the writer's
single clock and never regresses; the invariant is documented on the
model
- Start's retire-previous-cycle comment spells out that cancellation
only guarantees the old goroutine will exit, not that it already has
Co-Authored-By: Claude Fable 5 <[email protected]>
Signed-off-by: Nicolas De Loof <[email protected]>
Transposes the applicable findings from the review of the ttyWriter fix
(#14119) to the rewritten renderer:
- Done waits for the refresh goroutine to exit (outside the lock), so
nothing can repaint after Done returns and output printed right after
an operation cannot be garbled by a stray frame
- a nested Start retires the previous cycle instead of leaking its
refresh goroutine until the parent context is cancelled
- build suspension no longer leaks across cycles: Start resets it and
starts a fresh block below whatever buildkit wrote
- drop the dead strings.ToLower in startDependencies: the unexported
start only reads projectName when no Project is passed
The remaining findings are structural to this rewrite: Done is a single
critical section, and per-cycle context cancellation replaces the
doneSignal handshake.
Signed-off-by: Nicolas De Loof <[email protected]>
The "[+] op N/M" header was the one line bypassing renderSegs, so on a
very narrow terminal it could wrap and desync the cursor arithmetic the
rest of the design guarantees against. Route it through the same clip
and cover degenerate widths (8, 12 cells) in the invariant test.
Co-Authored-By: Claude Fable 5 <[email protected]>
Signed-off-by: Nicolas De Loof <[email protected]>
The TTY renderer accumulated display-corruption fixes (truncation of
details, then progress sizes, then task ids; timer alignment; rune-based
measurement) that each patched one symptom of the same structural gap:
nothing guaranteed a rendered line fits the terminal, and once a line
wraps, cursor arithmetic desyncs and the block corrupts.
Replace it with three separable units:
- tty_model.go: pure event reducer with an injected clock, preserving
first-parent-wins updates, monotonic progress and header counters
- tty_layout.go: pure (model, size, now) -> lines function; all widths
are measured in terminal cells (go-runewidth, so CJK is correct) on
plain text before coloring, and every line is clipped to the terminal
width by construction, status text included
- tty_screen.go: diff-based repaint; unchanged rows are skipped,
identical frames write nothing, a frame is a single Write; a shrinking
terminal abandons the block instead of moving the cursor over reflowed
rows
The writer coordinates them behind a mutex and stops the refresh
goroutine through context cancellation, so Done cannot block when the
operation context was cancelled first (Ctrl-C during pull). The spinner
frame is derived from the clock instead of advancing on every call, and
truncation can no longer split multi-byte runes. Visual output is
unchanged: the snapshot test reproduces the previous renderer's golden
output character for character.
Co-Authored-By: Claude Fable 5 <[email protected]>
Signed-off-by: Nicolas De Loof <[email protected]>
Fixes#14114: Done() blocked forever once the render goroutine had
already exited via ctx.Done() (e.g. after SIGTERM), since it sent on an
unbuffered channel with nobody left to receive.
Closing the channel instead of sending never blocks, but a bare close()
panics the moment ttyWriter runs a second Start/Done cycle on the same
shared bus - which already happens today (run's nested create/start,
publish's nested push, rm --stop's sequential stop then remove). Start
now hands out a fresh, once-only-closable signal each cycle instead of
a single shared channel, and run/publish stop opening a redundant
nested bracket around calls already inside one.
Signed-off-by: Guillaume Lours <[email protected]>
The display.Mode package global was left as "auto" on the most common
path (auto mode with an interactive or redirected terminal), so readers
like `up`'s NavigationMenu gate saw an unresolved value; that gate also
compared against the string literal "plain" instead of the constant.
applyDisplayMode wrote a mode that selectEventProcessor could then
contradict (--ansi always with a redirected stderr rendered Plain while
Mode said tty).
selectEventProcessor becomes the single resolution point: every branch
assigns the mode matching the renderer it returns, so after command
setup the global never holds ModeAuto. applyDisplayMode loses its
redundant writes and its misleading name (now applyAnsiMode); the
contract is documented on the Mode variable, and the existing
selectEventProcessor tests now pin the resolved mode for every branch.
Side effect, deliberate: with stderr redirected (auto -> plain
rendering) the navigation menu is no longer offered; it used to slip
through because Mode still read "auto".
Part of #14074 (section E).
Signed-off-by: Nicolas De Loof <[email protected]>
Rename-only change, no behavior difference, no exported identifier
renamed (only parameter names of a few exported functions, which are
not part of the Go API).
- names that hide what a value is: parameters holding a *name* or *ID*
while named like the object — project→projectName, service→serviceName,
container→containerID — so string vs struct is readable at every call
site without opening the signature
- misleading receivers: monitor methods used c (reads as
client/container/CLI) → m; jsonWriter used p while its ttyWriter
sibling uses w → w
- cryptic abbreviations with non-trivial scope: nw (meant both
network.Summary and types.NetworkConfig) → network/networkConfig,
svc → service/serviceName/serviceCopy, oc/ocs → observedContainer(s),
cnts → serviceContainers, cnx → attachResponse
- container.Summary loop variables unified on ctr (was a c/ctr mix);
fillBindMounts also dropped its p/s/m single-letters, one of which
was shadowed by a loop variable of a different type
- option-struct identifiers unified on options (opts remains only for
variadic functional options); ambiguous ones get a precise name
(bindOptions, networkCreateOptions, projectOptionsFns)
Signed-off-by: Nicolas De Loof <[email protected]>
Inline small functions that were extracted from their single call site
without a real boundary to justify it — no reuse, no dedicated test, no
responsibility of their own — so each caller now tells its whole story
top-down:
- getExecTarget, attachContainer, logContainer: one-line trampolines to
getSpecifiedContainer / doAttachContainer / doLogContainer, which
other call sites already use directly
- removeImage: single-statement wrapper, unlike its removeVolume
sibling which has actual logic
- checkSelectedServices: named like a validation, actually a filter;
the subtle rule (an unknown service is only an error with an explicit
compose file) now reads where options.Services is rewritten
- prepareLabels: mutated the map it received while looking pure at the
call site; the label writes are now visible in getCreateConfigs
- setDefaultTarget: mutation-by-pointer of the loop copy, now visible
in the loop of injectFileReferences
- buildVolumeOptions/buildTmpfsOptions/buildImageOptions: nil-guard +
field copies; the buildMountOptions switch now shows side by side
what each mount type propagates (buildBindOption keeps real logic and
keeps buildMountOptions under the gocyclo limit)
- displayDryRunBuildEvent: was longer than its only caller
- hasMore: read like a predicate, was a one-line spinner restart
- escapeDollarSign: wrapped a single bytes.ReplaceAll
- extractEnvCLIDefined: replaced by the canonical compose-go helper
types.NewMappingWithEquals().ToMapping(), as run.go already does
- isPullPolicyValid: rebuilt the valid-values slice on every call; now
a package-level list checked at the call site
- viewFromStackList: projection now sits next to the render closure
that consumes exactly its three fields
No behavior change; single gocyclo threshold untouched.
Signed-off-by: Nicolas De Loof <[email protected]>
maxBeforeStatusWidth used len(l.taskID) (bytes) while applyPadding
used utf8.RuneCountInString (runes). For ASCII task IDs the two
agree and no symptom surfaces, but a taskID containing multi-byte
UTF-8 chars (CJK, emoji, accented Latin) reported a width larger
than its visual columns. computeOverflow then triggered truncation
where none was needed, and truncateLongestTaskID's byte-indexed
slice could land mid-multibyte sequence, corrupting the displayed
string.
Align the two measurements on rune count.
Signed-off-by: Guillaume Lours <[email protected]>
In narrow terminals (e.g. tmux panes), the TTY progress UI emitted
lines wider than terminalWidth because adjustLineWidth could shrink
details and taskID but never the progress field. When progress
carried the "X.XMB / Y.YMB" size suffix, the truncation loop exited
with overflow > 0 and applyPadding's max(timerPad, 1) floor pushed
the line one char over. tmux then wrapped the line visually while
print() kept counting logical lines, desyncing aec.Up() on the next
render and producing the mangled "[+] pull X/Y" header overwriting
prior task lines.
Track the size suffix byte length on lineData and let
adjustLineWidth drop it as an intermediate truncation step before
abbreviating the taskID.
Fixesdocker/compose#13595
Signed-off-by: Guillaume Lours <[email protected]>
- Replace context.Background() with t.Context() in TestDoneDeadlockFix
- Ensures .idea files are not included in commit
Signed-off-by: maks2134 <[email protected]>
- Replace context.Background() with context.WithCancel() in test
- Fix formatting issues (remove extra empty line)
Signed-off-by: maks2134 <[email protected]>
Resolves race condition between main thread calling Done() and UI thread
calling printWithDimensions(). The issue was that Done() held the mutex
while sending to the done channel, but the UI thread needed the same
mutex to process the done signal.
Fixed by sending the done signal before acquiring the mutex, allowing
the UI thread to receive the signal and release any held locks.
Fixes#13639
Signed-off-by: maks2134 <[email protected]>
Results of running the modernize command, with some minor changes
afterwards (removing the `contains` and `hasStatus` helper functions);
go install golang.org/x/tools/go/analysis/passes/modernize/cmd/modernize@latest
modernize -fix ./...
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Most files already grouped imports into "stdlib -> other -> local",
but some files didn't. The gci formatter is similar to goimports, but
has better options to make sure imports are grouped in the expected
order (and to make sure no additional groups are present).
This formatter has a 'fix' function, so code can be re-formatted auto-
matically;
golangci-lint run -v --fix
Signed-off-by: Sebastiaan van Stijn <[email protected]>
