spec: option to clamp/disable truecolor BCE bg painting (#10278)#10496
spec: option to clamp/disable truecolor BCE bg painting (#10278)#10496lonexreb wants to merge 3 commits intowarpdotdev:masterfrom
Conversation
github-actions
Bot
added
the
external-contributor
|
I'm starting a first review of this spec-only pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This spec proposes a user setting and tab-config override for truecolor background rendering modes. The core direction is clear, and the security pass did not find a design-level security concern for this local rendering/configuration feature.
Concerns
- The clamp behavior only names cursor-to-EOL/line-ending fill, but Warp's BCE-colored blanks are also produced by clear, erase, insert/delete, scroll, and block-finalization paths; those operations need explicit mode behavior.
- The persisted settings and tab-config surfaces are underspecified for Warp's existing settings/TOML schemas.
- The acceptance criteria conflict with the spec's recommendation that explicitly written spaces count as painted cells.
Verdict
Found: 0 critical, 4 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| When a line ends with the truecolor bg still active, the background paints | ||
| only the cells that contain printed glyphs (or whitespace explicitly written). | ||
| Cells past the last written column render with the terminal default | ||
| background. Implicit BCE fill from cursor-to-EOL is suppressed for truecolor. |
There was a problem hiding this comment.
clamp_to_text only specifies line-ending/cursor-to-EOL fill, but BCE-colored blank cells are also produced by erase, clear, insert/delete, scroll, and block-finalization paths; enumerate each affected ANSI operation and its expected background result.
|
|
||
| ## Settings / API surface | ||
|
|
||
| - `terminal.truecolor_background_mode`: enum, default `"flood"`. Stored in |
There was a problem hiding this comment.
|
|
||
| - `terminal.truecolor_background_mode`: enum, default `"flood"`. Stored in | ||
| user terminal settings. | ||
| - `pane.truecolor_background_mode`: optional per-pane override in tab config. |
There was a problem hiding this comment.
pane.truecolor_background_mode is ambiguous for tab configs, whose pane schema uses [[panes]] nodes and rejects unknown fields; specify the exact TOML shape, valid values, precedence over global settings, and invalid-value behavior.
|
|
||
| - A1: Default mode is `flood` and matches current rendering byte-for-byte for | ||
| a recorded `vim_24bitcolors_bce` reference test. | ||
| - A2: `clamp_to_text` does not paint cells past the last printed glyph on |
There was a problem hiding this comment.
|
Round-1 fixes for oz-for-oss review: (1) all bg-fill paths enumerated and aligned with the mode — cursor-to-EOL, erase ops ( |
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec defines a user setting and per-pane override for truecolor background rendering modes, with behavior, acceptance criteria, implementation pointers, and tests.
Concerns
- The user-level setting key is inconsistent between the behavior contract and the Settings/API section.
- The per-pane tab-config path is described as
pane.truecolor_background_mode, but the existing tab-config schema stores pane fields under[[panes]], so the spec needs exact valid TOML syntax for the override.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
|
||
| ### B1. Setting enum | ||
|
|
||
| `terminal.truecolor_background_mode` enum: `"flood"` (default), `"clamp_to_text"`, |
There was a problem hiding this comment.
terminal.truecolor_background_mode, but the API surface and acceptance criteria later use appearance.terminal.truecolor_background_mode; make the contract use one canonical key, or explicitly distinguish internal setting name from TOML path.
| - Field name: `truecolor_background_mode`. | ||
| - Type: `Option<TruecolorBackgroundMode>` (None = inherit user setting). | ||
| - Serde representation: lowercase string when present. | ||
| - TOML path on a pane node: `pane.truecolor_background_mode`. |
There was a problem hiding this comment.
pane.truecolor_background_mode does not match the current tab-config shape, where pane fields live inside each [[panes]] entry; specify the exact valid TOML syntax and update A7/T_per_pane_override_toml accordingly.
|
Addressed both important concerns from the latest oz-for-oss review:
Commit: b0d508b |
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec defines a new truecolor background rendering mode with behavior contracts, settings/API details, acceptance criteria, implementation pointers, and tests. The scope is aligned with the described issue: preserving current flood behavior by default while allowing clamp and disabled modes for 24-bit ANSI backgrounds.
Concerns
- One behavior-contract reference uses the shortened
terminal.truecolor_background_modepath even though B1 declaresappearance.terminal.truecolor_background_modeas canonical. - Security pass: no design-level security concerns found; the change is limited to local rendering/configuration behavior and does not add a new external input, auth, secret, or network surface.
Verdict
Found: 0 critical, 0 important, 1 suggestions
Approve with nits
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| state. | ||
|
|
||
| Single guiding principle: **all bg-painted blank-cell production paths | ||
| follow the active `terminal.truecolor_background_mode`. Modes apply |
There was a problem hiding this comment.
💡 [SUGGESTION] Use the canonical appearance.terminal.truecolor_background_mode key here; B1 says that path is canonical, but this shortened form could lead implementation or tests to wire the wrong settings path.
1 participant
Spec for #10278.
Summary
Adds a setting
terminal.truecolor_background_modeto control how Warp renders truecolor (24-bit) ANSI background codes (\x1b[48;2;R;G;Bm). Three modes:flood(default) — current behavior, paints to end of line/block.clamp_to_text— paints only behind printed glyphs; cells past the last write get default bg.disabled— truecolor bg ANSI is parsed but not rendered as bg fill; fg colors preserved.Per-pane override via tab config. 256-color and 16-color BCE behavior is unchanged.
Why
Modern diff/highlight tools (Claude Code's diff renderer,
delta,git diffwith custom config) emit contiguous bg-colored lines via truecolor codes. Warp's BCE flood paints those across the full block width on every line, so the cumulative effect reads as a full-screen tint that overwhelms the rendered content. The screenshots in #10278 show this directly.Scope
Spec-only — no code changes in this PR. See
specs/GH10278/SPEC.mdfor behavior contract, acceptance criteria, implementation pointers, tests, and the one open question (whether explicit space writes should count as bg-painted inclamp_to_text).Tag:
ready-to-spec.