diff --git a/image.c b/image.c index c18e544b3..10bbaa5bc 100644 --- a/image.c +++ b/image.c @@ -653,6 +653,42 @@ image_grid_check_area(struct grid *gd, u_int x, u_int y, u_int width, return (0); } +/* + * Find the leftmost image span intersecting [x, end) on one grid row. + * Used to avoid drawing text over cells that a trusted scroll (imagescroll) + * is assumed to have preserved image content in - text is always drawn + * before images are composited on top, and when compositing is skipped for + * a trusted scroll, nothing would otherwise correct that draw. Returns 1 + * and sets out_x and out_end to the clipped intersection, or 0 if none. + */ +int +image_grid_next_span(struct grid *gd, u_int x, u_int end, u_int y, + u_int *out_x, u_int *out_end) +{ + struct image_line *line; + struct image_span *span; + u_int sx, sxend; + int found = 0; + + if (gd->images == NULL || y >= gd->hsize + gd->sy) + return (0); + line = gd->linedata[y].images; + if (line == NULL) + return (0); + TAILQ_FOREACH(span, &line->spans, line_entry) { + if (span->x >= end || span->x + span->sx <= x) + continue; + sx = (span->x > x) ? span->x : x; + sxend = (span->x + span->sx < end) ? span->x + span->sx : end; + if (!found || sx < *out_x) { + *out_x = sx; + *out_end = sxend; + found = 1; + } + } + return (found); +} + /* Find source coordinates for an image span at one grid cell. */ int image_grid_get_source(struct grid *gd, u_int x, u_int y, struct image *im, diff --git a/regress/image-scroll-no-erase.sh b/regress/image-scroll-no-erase.sh new file mode 100755 index 000000000..cbb0404f6 --- /dev/null +++ b/regress/image-scroll-no-erase.sh @@ -0,0 +1,114 @@ +#!/bin/sh + +# Regression test for a tmux bug (not a terminal bug): image_redraw_scroll() +# (image.c) marks the whole scroll region as damage on every scroll, and +# redraw_client_damage_rect()'s skip_images optimization (screen-redraw.c) +# skips retransmitting an image trusted to have moved with the scroll - but +# the ordinary REDRAW_TEXT pass for that same damage rectangle still ran +# unconditionally, drawing blank grid content directly over the image's +# cells. Text is normally drawn first and images composited on top +# immediately after in the same batch, so that is a harmless intermediate +# state - skip_images only ever suppressed that second, correcting pass, +# leaving the blank draw as the final state. This erased a correctly +# scrolled image, independent of anything the terminal itself did - this is +# what a user saw as "the image disappears when the pane scrolls in Windows +# Terminal" even with imagescroll and margins both granted, and could never +# be reproduced by any terminal-side test because the bug is entirely +# server side. +# +# redraw_draw_pane_span() now consults image_grid_next_span() to skip +# drawing text over any x-range with an image span attached, whenever +# skip_images is set, logging "skipping A-B on row N (image)" whenever it +# does - that log line, and the ability to exclude an image's columns from +# a text draw at all, do not exist before this fix, so its mere presence +# is enough to fail outright on any earlier tmux build. +# +# The image is placed comfortably mid-pane (not at the very top) and wide +# enough to span the whole pane, so it is still on screen - not already +# scrolled into history - by the time a real scroll's damage gets composed. + +PATH=/bin:/usr/bin +TERM=screen +LC_ALL=C.UTF-8 +export TERM LC_ALL + +[ -z "$TEST_TMUX" ] && TEST_TMUX=$(readlink -f ../tmux) +TMUX="$TEST_TMUX -LtestA$$ -vv -f/dev/null" +TMUX2="$TEST_TMUX -LtestB$$ -f/dev/null" + +cleanup() +{ + $TMUX kill-server >/dev/null 2>&1 + $TMUX2 kill-server >/dev/null 2>&1 +} +fail() +{ + echo "$*" >&2 + cleanup + exit 1 +} + +cleanup + +DIR=$(mktemp -d) || exit 1 +cd "$DIR" || exit 1 +trap "cleanup; cd /; rm -rf $DIR" 0 1 15 + +WIDTH=40 +HEIGHT=20 +ROW=10 + +# A sixel image exactly WIDTH cells wide (at a typical ~9px cell, 360px) and +# one cell tall, placed at screen row ROW - comfortably mid-pane, not at +# the very top, so it stays visible (not yet scrolled into history) through +# the first several scrolls. +HEADER='\033Pq"1;1;360;18#0;2;100;100;100#0!360~-!360~-!360B\033\\' + +# Pad with blank lines after the image so the shell prompt lands at the +# bottom of the pane - without this, the very first scroll from a +# not-yet-settled cursor position falls back to tty_redraw_region() (a +# different, unrelated path that skip_images never applies to anyway), and +# the fix's own exclusion logic never gets a chance to run at all. +PAD=$((HEIGHT - ROW - 2)) +$TMUX new-session -d -s inner -x $WIDTH -y $HEIGHT \ + "printf '\\033[$((ROW + 1));1H$HEADER'; i=0; while [ \$i -lt $PAD ]; do echo; i=\$((i + 1)); done; exec sh" || + exit 1 +sleep 0.3 + +[ "$($TMUX display-message -p '#{image_support}')" = 0 ] && exit 0 +$TMUX set -g pane-scrollbars on || exit 1 +$TMUX set -as terminal-features ',*:sixel' || exit 1 +$TMUX set -as terminal-features ',*:margins' || exit 1 +$TMUX set -as terminal-features ',*:imagescroll' || exit 1 + +$TMUX2 new-session -d -x $WIDTH -y $HEIGHT || exit 1 +OUTER=$($TMUX2 list-panes -F '#{pane_id}' | head -1) +[ -n "$OUTER" ] || fail "No outer pane." +$TMUX2 set -as terminal-features ',*:sixel@' || fail "disable outer sixel failed" +TMP=$(mktemp) +$TMUX2 pipe-pane -t "$OUTER" -O "cat >$TMP" || fail "pipe-pane failed" +$TMUX2 send-keys -t "$OUTER" -l "$TMUX attach -t inner" || fail "send attach failed" +$TMUX2 send-keys -t "$OUTER" Enter || fail "send enter failed" +sleep 1 + +grep -qa '"1;1;360;18' "$TMP" || fail "sanity: image never reached the client" + +LOG=$(ls tmux-server*.log 2>/dev/null | head -1) +[ -n "$LOG" ] || fail "sanity: no server -vv log was produced" +BEFORE=$(wc -l <"$LOG") +: >"$TMP" + +# The image (row ROW, 10 of 20) has plenty of room above it - this stays +# well within the visible pane for the whole burst. +$TMUX send-keys -t inner -l "yes | head -n 10" || fail "send scroll failed" +$TMUX send-keys -t inner Enter || fail "send enter failed" +sleep 1 + +n_dcs=$(grep -ac "$(printf '\033P')" "$TMP") +[ "$n_dcs" -eq 0 ] || + fail "sanity: image was retransmitted ($n_dcs times) - imagescroll's trust did not engage, so this run cannot test what it is meant to" + +tail -n +$((BEFORE + 1)) "$LOG" | grep -q "redraw_draw_pane_span: skipping .* (image)" || + fail "the text-redraw pass never excluded the image's cells while trusting a scroll to have preserved it - this build would draw blank content over the image and erase it" + +exit 0 diff --git a/regress/image-scroll-uses-region.sh b/regress/image-scroll-uses-region.sh new file mode 100755 index 000000000..4555bbaf9 --- /dev/null +++ b/regress/image-scroll-uses-region.sh @@ -0,0 +1,142 @@ +#!/bin/sh + +# Regression test: with imagescroll granted, does tmux actually use a native +# scrolling-region command (DECSTBM, and DECSLRM if the pane isn't full +# width) to perform the scroll, rather than tty_redraw_region()'s fallback +# full manual repaint? This directly checks the concern raised in PR review +# that tmux "isn't using scrolling regions" for an image-bearing pane - +# imagescroll only makes sense to grant a terminal if tmux is actually +# relying on a real native scroll to carry the image along, not silently +# falling back to redrawing every line by hand while still skipping the +# image retransmission (which would just lose the image for nothing). +# +# tty_redraw_region() (tty.c) logs one of two lines whenever it runs at +# all - "%s large region redraw" or "%s small region redraw" - so their +# total absence after the scroll proves the native path +# (tty_cmd_linefeed()/scrollup()/scrolldown()/reverseindex()) was taken +# instead. Combined with image-sixel-region-scroll.sh's own check (the +# image is not retransmitted), this proves imagescroll's trust is actually +# backed by a real native scroll, not a coincidence. +# +# Phase 2 is the deliberate opposite case, and is what PR review's concern +# actually described: pane-scrollbars makes this pane less than full width, +# and without margins granted, tty_cmd_*()'s own +# "(!tty_full_width(tty, ctx) && !tty_use_margin(tty))" check has no native +# scrolling-region option left and must fall back - proving phase 1 wasn't +# trivially passing, and that redraw_image_scroll_result()'s failure path +# correctly also stops imagescroll's trust from applying when that happens +# (the image must be retransmitted here, exactly as +# image-sixel-region-scroll.sh's own phase 2 already separately proves). +# +# A real attached client is needed for any of this redraw code to run at +# all, so this uses the same nested inner/outer tmux pattern as +# image-sixel-region-scroll.sh. + +PATH=/bin:/usr/bin +TERM=screen +LC_ALL=C.UTF-8 +export TERM LC_ALL + +[ -z "$TEST_TMUX" ] && TEST_TMUX=$(readlink -f ../tmux) +TMUX="$TEST_TMUX -LtestA$$ -vv -f/dev/null" +TMUX2="$TEST_TMUX -LtestB$$ -f/dev/null" + +cleanup() +{ + $TMUX kill-server >/dev/null 2>&1 + $TMUX2 kill-server >/dev/null 2>&1 +} +fail() +{ + echo "$*" >&2 + cleanup + exit 1 +} + +cleanup + +DIR=$(mktemp -d) || exit 1 +cd "$DIR" || exit 1 +trap "cleanup; cd /; rm -rf $DIR" 0 1 15 + +WIDTH=40 +HEIGHT=20 + +HEADER='\033Pq"1;1;26;26#0;2;100;100;100#0!26~-!26~-!26~-!26~-!26B\033\\' + +$TMUX new-session -d -s inner -x $WIDTH -y $HEIGHT \ + "printf '$HEADER'; exec sh" || exit 1 +sleep 0.3 + +[ "$($TMUX display-message -p '#{image_support}')" = 0 ] && exit 0 +$TMUX set -g pane-scrollbars on || exit 1 +$TMUX set -as terminal-features ',*:sixel' || exit 1 +$TMUX set -as terminal-features ',*:imagescroll' || exit 1 + +$TMUX2 new-session -d -x $WIDTH -y $HEIGHT || exit 1 +OUTER=$($TMUX2 list-panes -F '#{pane_id}' | head -1) +[ -n "$OUTER" ] || fail "No outer pane." +$TMUX2 set -as terminal-features ',*:sixel@' || fail "disable outer sixel failed" +TMP=$(mktemp) +$TMUX2 pipe-pane -t "$OUTER" -O "cat >$TMP" || fail "pipe-pane failed" +$TMUX2 send-keys -t "$OUTER" -l "$TMUX attach -t inner" || fail "send attach failed" +$TMUX2 send-keys -t "$OUTER" Enter || fail "send enter failed" +sleep 1 + +grep -qa '"1;1;26;26' "$TMP" || fail "sanity: image never reached the client" + +LOG=$(ls tmux-server*.log 2>/dev/null | head -1) +[ -n "$LOG" ] || fail "sanity: no server -vv log was produced" + +run_scroll() +{ + label=$1 + BEFORE=$(wc -l <"$LOG") + : >"$TMP" + + $TMUX send-keys -t inner -l "yes | head -n 10" || fail "$label: send scroll failed" + $TMUX send-keys -t inner Enter || fail "$label: send enter failed" + sleep 1 + + n_dcs=$(grep -ac "$(printf '\033P')" "$TMP") + n_redraw=$(tail -n +$((BEFORE + 1)) "$LOG" | grep -c "region redraw") +} + +# --- Phase 1: margins granted - real native scrolling region expected, +# image never retransmitted. --- +$TMUX set -as terminal-features ',*:margins' || fail "grant margins failed" +sleep 0.3 +$TMUX display-message -p '#{client_termfeatures}' | + grep -q imagescroll || fail "sanity: imagescroll was not granted to the client" + +run_scroll "phase 1 (margins granted)" +[ "$n_dcs" -eq 0 ] || + fail "phase 1: image was retransmitted ($n_dcs times) - imagescroll's trust did not engage, so this run cannot test what it is meant to" +[ "$n_redraw" -eq 0 ] || + fail "phase 1: tty_redraw_region() fired ($n_redraw times) while scrolling an image-bearing pane with margins+imagescroll granted - tmux fell back to a full manual repaint instead of a real native scroll, so trusting the terminal to have moved the image was never justified" + +# --- Phase 2: margins revoked - the pane still isn't full width +# (pane-scrollbars is on), so this is the scenario PR review's "not using +# scrolling regions" concern actually described: no native scrolling-region +# option is available, tty_redraw_region() must fire, and +# redraw_image_scroll_result()'s failure path must in turn stop +# imagescroll's trust from applying, so the image is retransmitted instead +# of silently lost. Resend the image first - phase 1's scrolling already +# pushed the original one (26 pixels tall, a couple of rows at most) out of +# the pane, so without this there would be nothing left to retransmit +# either way, and the check below would pass for the wrong reason. --- +$TMUX set -as terminal-features ',*:margins@' || fail "revoke margins failed" +sleep 0.3 +: >"$TMP" +$TMUX send-keys -t inner -l "printf '$HEADER'" || fail "resend image failed" +$TMUX send-keys -t inner Enter || fail "send enter failed" +sleep 1.5 +grep -qa '"1;1;26;26' "$TMP" || fail "sanity: image did not reappear before phase 2" + +run_scroll "phase 2 (margins revoked)" +[ "$n_redraw" -gt 0 ] || + fail "sanity: without margins, scrolling a scrollbar-enabled image-bearing pane never fell back to tty_redraw_region() - this scenario no longer exercises the bug phase 1 checks for" +[ "$n_dcs" -gt 0 ] || + fail "phase 2: image was not retransmitted after tty_redraw_region() fired - imagescroll's trust incorrectly stayed engaged despite the fallback, which would silently lose the image" + +exit 0 diff --git a/screen-redraw.c b/screen-redraw.c index 6ee24f171..f79f0ab9c 100644 --- a/screen-redraw.c +++ b/screen-redraw.c @@ -1370,11 +1370,20 @@ redraw_get_scene(struct client *c) return (scene); } -/* Draw a pane span. */ +/* + * Draw a pane span. skip_images is only set for a REDRAW_TEXT pass standing + * in for a trusted scroll's skipped image phases (redraw_draw_damage_rect()) + * - text is normally drawn first and images composited on top straight + * after, so a plain grid cell (usually blank, since an image is a separate + * overlay) is a harmless intermediate state. When the image phases are + * skipped because the terminal is trusted to have preserved the image + * itself, nothing corrects that draw afterwards, so any x-range with an + * image span attached must be left alone here instead. + */ static void redraw_draw_pane_span(struct redraw_draw_ctx *dctx, struct redraw_span *span, u_int x, u_int y, u_int n, - enum redraw_image_phase phase) + enum redraw_image_phase phase, int skip_images) { struct redraw_scene *scene = dctx->scene; struct client *c = scene->c; @@ -1384,6 +1393,10 @@ redraw_draw_pane_span(struct redraw_draw_ctx *dctx, struct grid_cell defaults; struct tty_style_ctx style_ctx; u_int px, py; +#ifdef ENABLE_IMAGES + u_int gy, cur, end, span_x, span_end; + int xoff; +#endif tty_default_colours(&defaults, wp, &style_ctx.dim); style_ctx.defaults = &defaults; @@ -1393,10 +1406,34 @@ redraw_draw_pane_span(struct redraw_draw_ctx *dctx, px = span->data.p.px + (x - span->x); py = span->data.p.py; #ifdef ENABLE_IMAGES - if (phase != REDRAW_TEXT) + if (phase != REDRAW_TEXT) { image_draw_line(tty, s, px, py, n, x, y, phase == REDRAW_IMAGES_BEFORE, &style_ctx); - else + return; + } + if (skip_images) { + gy = s->grid->hsize + py; + xoff = (int)x - (int)px; + cur = px; + end = px + n; + while (cur < end) { + if (!image_grid_next_span(s->grid, cur, end, gy, + &span_x, &span_end)) + span_x = end; + else { + log_debug("%s: skipping %u-%u on row %u (image)", + __func__, span_x, span_end, gy); + } + if (span_x > cur) { + tty_draw_line(tty, s, cur, py, span_x - cur, + (u_int)((int)cur + xoff), y, &style_ctx); + } + if (span_x >= end) + break; + cur = span_end; + } + return; + } #endif if (phase == REDRAW_TEXT) tty_draw_line(tty, s, px, py, n, x, y, &style_ctx); @@ -1654,11 +1691,14 @@ redraw_draw_menu_span(struct redraw_draw_ctx *dctx, /* * Draw a span, restricted to [clip_x, clip_x + clip_n) - a caller drawing * the whole span passes the span's own x/width here; a caller drawing only - * a damaged sub-range passes that range instead. + * a damaged sub-range passes that range instead. skip_images is only ever + * set from redraw_draw_damage_rect(), for a REDRAW_TEXT pass standing in + * for a trusted scroll's skipped image phases - see redraw_draw_pane_span(). */ static void redraw_draw_span(struct redraw_draw_ctx *dctx, struct redraw_span *span, - u_int y, u_int clip_x, u_int clip_n, enum redraw_image_phase phase) + u_int y, u_int clip_x, u_int clip_n, enum redraw_image_phase phase, + int skip_images) { struct redraw_span_data *data = &span->data; enum redraw_span_type type = data->type; @@ -1670,7 +1710,8 @@ redraw_draw_span(struct redraw_draw_ctx *dctx, struct redraw_span *span, switch (span->data.type) { case REDRAW_SPAN_PANE: - redraw_draw_pane_span(dctx, span, clip_x, y, clip_n, phase); + redraw_draw_pane_span(dctx, span, clip_x, y, clip_n, phase, + skip_images); break; case REDRAW_SPAN_BORDER: case REDRAW_SPAN_EMPTY: @@ -1748,7 +1789,7 @@ redraw_draw_pane_lines(struct redraw_draw_ctx *dctx, struct window_pane *wp, TAILQ_FOREACH(span, spans, entry) { if (span->data.p.wp == wp) redraw_draw_span(dctx, span, cy, span->x, - span->width, phase); + span->width, phase, 0); } } if (phase == REDRAW_TEXT && @@ -1757,7 +1798,7 @@ redraw_draw_pane_lines(struct redraw_draw_ctx *dctx, struct window_pane *wp, TAILQ_FOREACH(span, spans, entry) { if (span->data.sb.wp == wp) redraw_draw_span(dctx, span, cy, span->x, - span->width, phase); + span->width, phase, 0); } } } @@ -1825,7 +1866,7 @@ redraw_draw_lines(struct redraw_draw_ctx *dctx, int flags) spans = &line->spans[type]; TAILQ_FOREACH(span, spans, entry) redraw_draw_span(dctx, span, cy, span->x, span->width, - phase); + phase, 0); } } #ifdef ENABLE_IMAGES @@ -1851,7 +1892,7 @@ redraw_draw_menu_lines(struct redraw_draw_ctx *dctx) cy = y; TAILQ_FOREACH(span, &line->spans[REDRAW_SPAN_MENU], entry) redraw_draw_span(dctx, span, cy, span->x, span->width, - REDRAW_TEXT); + REDRAW_TEXT, 0); } } @@ -2426,6 +2467,38 @@ redraw_draw_damage_rect(struct redraw_draw_ctx *dctx, u_int x, u_int y, if (sx == 0 || sy == 0) return; +#ifdef ENABLE_IMAGES + /* + * Remove any stale Kitty placements this redraw is about to replace, + * the same as redraw_draw_pane_lines() does for a full pane redraw - + * unlike a plain overwrite of SIXEL pixels, a Kitty placement is a + * discrete object that persists until explicitly deleted, so without + * this a scroll-triggered redraw (this function, not the full-pane + * path) leaves every previous placement behind, all still visible + * and now overlapping the newly placed ones. Skipped when trusting a + * scroll to have moved the image itself - nothing is being replaced. + */ + if (!skip_images) { + for (yy = y; yy < y + sy; yy++) { + line = &scene->lines[yy]; + if (dctx->flags & REDRAW_STATUS_TOP) + cy = dctx->status_lines + yy; + else + cy = yy; + spans = &line->spans[REDRAW_SPAN_PANE]; + TAILQ_FOREACH(span, spans, entry) { + clip_x = (span->x > x) ? span->x : x; + clip_end = (span->x + span->width < x + sx) ? + span->x + span->width : x + sx; + if (clip_end <= clip_x) + continue; + image_redraw_start(&scene->c->tty, clip_x, cy, + clip_end - clip_x, 1); + } + } + } +#endif + for (enum redraw_image_phase phase = REDRAW_IMAGES_BEFORE; phase <= REDRAW_IMAGES_AFTER; phase++) { if (skip_images && phase != REDRAW_TEXT) @@ -2455,7 +2528,7 @@ redraw_draw_damage_rect(struct redraw_draw_ctx *dctx, u_int x, u_int y, redraw_damage_grow_span_clip(span, &clip_x, &clip_end); redraw_draw_span(dctx, span, cy, clip_x, - clip_end - clip_x, phase); + clip_end - clip_x, phase, skip_images); if (phase == REDRAW_TEXT && type == REDRAW_SPAN_PANE) { redraw_damage_draw_pane_prompt(dctx, span, cy, clip_x, @@ -2486,7 +2559,7 @@ redraw_draw_damage_rect(struct redraw_draw_ctx *dctx, u_int x, u_int y, redraw_damage_refresh_status(dctx, span->data.st.wp); redraw_damage_grow_span_clip(span, &clip_x, &clip_end); redraw_draw_span(dctx, span, cy, clip_x, clip_end - clip_x, - REDRAW_TEXT); + REDRAW_TEXT, 0); } } } diff --git a/tmux.h b/tmux.h index fb4d5f572..057927d84 100644 --- a/tmux.h +++ b/tmux.h @@ -4378,6 +4378,8 @@ void image_grid_resize_width(struct grid *, u_int); int image_grid_line_has_images(const struct grid_line *); int image_grid_check_area(struct grid *, u_int, u_int, u_int, u_int); +int image_grid_next_span(struct grid *, u_int, u_int, u_int, + u_int *, u_int *); int image_grid_get_source(struct grid *, u_int, u_int, struct image *, u_int *, u_int *); void image_place_cell_kitty(struct screen_write_ctx *, struct image *,