diff --git a/regress/image-selection-noflash.sh b/regress/image-selection-noflash.sh new file mode 100755 index 000000000..d7956aa83 --- /dev/null +++ b/regress/image-selection-noflash.sh @@ -0,0 +1,100 @@ +#!/bin/sh + +# Regression test: extending a copy-mode selection by cursor movement, with +# the view otherwise unmoved (no scrolling), must not retransmit an image +# whose row the cursor passes through. +# +# window_copy_write_one() (window-copy.c) used to write text/highlight +# styling directly over image-covered cells, which - since a character +# write typically clears whatever pixel content a terminal was showing +# there - erased the image with nothing to redraw it back in. Separately, +# window_copy_write_line()'s call to image_redraw_area() used to fire +# unconditionally on every redraw, so even after fixing the erasure, the +# image would still be needlessly recomposited (and briefly flash) on +# every single cursor step even though nothing about it had changed. See +# tmux-image-redraw-known-bugs.md for the full write-up. +# +# This is checked by counting DCS (\033P) sequences in the client's raw +# output during the cursor movement: with the fix, extending a selection +# without scrolling never touches the image, so none should appear. + +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$$ -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 + +TMP=$(mktemp) +trap "cleanup; rm -f $TMP" 0 1 15 + +# A small, distinctive SIXEL raster (26x26 pixels) at the top of the pane, +# matching the fixture already used in image-support.sh, followed by +# enough plain lines that the cursor can move down through the image and +# past it without the view needing to scroll. +SIXEL='\033Pq"1;1;26;26#0;2;100;100;100#0!26~-!26~-!26~-!26~-!26B\033\\' +$TMUX new-session -d -s inner -x 40 -y 20 \ + "printf '$SIXEL'; for i in \$(seq 1 15); do echo line\$i; done; exec sh" || + exit 1 +sleep 0.5 + +[ "$($TMUX display-message -p '#{image_support}')" = 0 ] && exit 0 +$TMUX set -as terminal-features ',*:sixel' || exit 1 + +# Start the outer session with a plain shell, then start capturing before +# triggering the attach - starting the attach as the outer pane's initial +# command would mean pipe-pane only starts after the attach-driven initial +# redraw (which sends the image) has already happened, missing it. +$TMUX2 new-session -d -x 40 -y 20 || exit 1 +OUTER=$($TMUX2 list-panes -F '#{pane_id}' | head -1) +[ -n "$OUTER" ] || fail "No outer pane." +$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 + +# Sanity check: the image reached the client at all. +grep -qa '"1;1;26;26' $TMP || fail "sanity: image never reached the client" +: >$TMP + +# Enter copy-mode, scroll to the top (where the image is) and select down +# through it one cursor step at a time - the view does not need to scroll +# for any of this, since the image is already at the top of what is +# visible. +$TMUX copy-mode -t inner || fail "copy-mode failed" +$TMUX send-keys -X history-top || fail "history-top failed" +sleep 0.2 +: >$TMP +$TMUX send-keys -X begin-selection || fail "begin-selection failed" +i=0 +while [ $i -lt 6 ]; do + $TMUX send-keys -X cursor-down || fail "cursor-down failed" + sleep 0.15 + i=$((i + 1)) +done +sleep 0.5 + +# No DCS sequence should have been sent - the image's row was never +# disturbed by any of this. This is expected to fail before the fix - see +# the header comment. +dcs=$(grep -ac "$(printf '\033P')" $TMP) +[ "$dcs" -eq 0 ] || + fail "image was retransmitted ($dcs times) while just moving the selection cursor" + +exit 0 diff --git a/window-copy.c b/window-copy.c index eba64d119..fc50d303d 100644 --- a/window-copy.c +++ b/window-copy.c @@ -73,6 +73,10 @@ static void window_copy_write_line(struct window_mode_entry *, struct screen_write_ctx *, u_int); static void window_copy_write_lines(struct window_mode_entry *, struct screen_write_ctx *, u_int, u_int); +#ifdef ENABLE_IMAGES +static void window_copy_update_image_refresh(struct window_copy_mode_data *); +static int window_copy_visible_has_images(struct window_copy_mode_data *); +#endif static char *window_copy_match_at_cursor(struct window_copy_mode_data *); static void window_copy_scroll_to(struct window_mode_entry *, u_int, u_int, int); @@ -279,6 +283,10 @@ struct window_copy_mode_data { u_int oy; /* number of lines scrolled up */ + u_int image_base; /* hsize - oy images were last drawn for */ + int image_base_set; + int image_refresh; /* current redraw needs image refresh */ + u_int selx; /* beginning of selection */ u_int sely; @@ -636,6 +644,9 @@ window_copy_init(struct window_mode_entry *wme, data->my = screen_hsize(data->backing) + data->cy - data->oy; data->showmark = 0; +#ifdef ENABLE_IMAGES + window_copy_update_image_refresh(data); +#endif screen_write_start(&ctx, &data->screen); for (i = 0; i < screen_size_y(&data->screen); i++) window_copy_write_line(wme, &ctx, i); @@ -1197,6 +1208,9 @@ window_copy_size_changed(struct window_mode_entry *wme) window_copy_clear_selection(wme); window_copy_clear_marks(wme); +#ifdef ENABLE_IMAGES + window_copy_update_image_refresh(data); +#endif screen_write_start(&ctx, s); window_copy_write_lines(wme, &ctx, 0, screen_size_y(s)); screen_write_stop(&ctx); @@ -5160,6 +5174,34 @@ window_copy_write_one(struct window_mode_entry *wme, for (fx = 0; fx < nx; fx++) { grid_get_cell(gd, fx, fy, &gc); if (fx + gc.data.width <= nx) { +#ifdef ENABLE_IMAGES + /* + * Write image-covered cells directly into the grid, + * skipping both window_copy_update_style() (so a + * selection, current-line or search-mark highlight + * never sweeps visibly over the image before it is + * recomposited separately - see the + * image_redraw_area() call below) and + * screen_write_cell(), whose built-in + * screen_write_image_damage() call (screen-write.c) + * fires on every write regardless of whether + * anything actually changed, which would needlessly + * re-damage - and so retransmit - the image on every + * redraw. The cell must still land in the grid as + * normal: a non-graphical client's ASCII fallback for + * the image is an ordinary character here, not + * something recomposited separately, and depends on + * this write the same as any other cell. + */ + if (image_grid_check_area(gd, fx, fy, gc.data.width, + 1)) { + grid_view_set_cell(ctx->s->grid, px + fx, py, + &gc); + screen_write_cursormove(ctx, + px + fx + gc.data.width, py, 0); + continue; + } +#endif window_copy_update_style(wme, fx, fy, &gc, mgc, cgc, mkgc, clgc); if (gc.flags & GRID_FLAG_PADDING) { @@ -5409,11 +5451,26 @@ window_copy_write_line(struct window_mode_entry *wme, content_sx, &mgc, &cgc, &mkgc, &clgc); #ifdef ENABLE_IMAGES - /* Copy the backing line's image layers separately from its text cells. */ + /* + * Copy the backing line's image layers separately from its text + * cells. This is not implied by the text cells just written above: + * those go via the pane's normal (frequently fast, direct-write) + * path, which knows nothing about image content. + * + * Only report it as needing a redraw when the view has actually + * moved (data->image_refresh, set once per window_copy_redraw_lines + * call) - text writes never touch image-covered cells (see + * window_copy_write_one), so if the view is unmoved this row's + * images are already exactly as they should be and redrawing them + * anyway just flashes the image on every unrelated redraw (e.g. + * every step of a selection drag) for no visible benefit. + */ image_grid_free_line(s->grid, &s->grid->linedata[s->grid->hsize + py]); image_grid_copy_area(s->grid, width, s->grid->hsize + py, data->backing->grid, 0, hsize - data->oy + py, content_sx, 1); + if (data->image_refresh) + image_redraw_area(ctx, width, py, content_sx, 1); #endif if (py == 0 && s->rupper < s->rlower && !data->hide_position) { @@ -5476,6 +5533,55 @@ window_copy_redraw_selection(struct window_mode_entry *wme, u_int old_y) window_copy_redraw_lines(wme, start, end - start + 1); } +#ifdef ENABLE_IMAGES +/* + * Only rows whose underlying history position has moved since the last + * call need their images recomposited - most redraws are just a selection + * or cursor-line style change with the view otherwise unmoved, and that + * never touches image-covered cells (see window_copy_write_one()), so + * redrawing images for it is needless: on a fast drag it is visible as the + * image briefly flashing on every step even though nothing about it + * actually changed. + * + * Every place that calls window_copy_write_line()/window_copy_write_lines() + * must call this first - it is not implied by them, since some (the + * initial full-screen draw on entering copy mode, window_copy_scroll_up(), + * window_copy_scroll_down()) write directly rather than going through + * window_copy_redraw_lines(). + */ +static void +window_copy_update_image_refresh(struct window_copy_mode_data *data) +{ + u_int base; + + base = screen_hsize(data->backing) - data->oy; + data->image_refresh = !data->image_base_set || base != data->image_base; + data->image_base = base; + data->image_base_set = 1; +} + +/* + * Whether any part of the currently visible backing range carries image + * data. A terminal's line insert/delete only shifts character cells - the + * pixels of a sixel or Kitty image already on screen stay exactly where + * they were sent, so a scroll that uses that fast path (see + * window_copy_scroll_up()/window_copy_scroll_down()) leaves stale or + * missing image content for every row except the single new one it + * explicitly rewrites. When this returns true, callers should fall back to + * a full window_copy_redraw_screen() instead, so every visible row's image + * is recomposited at its correct new position. + */ +static int +window_copy_visible_has_images(struct window_copy_mode_data *data) +{ + struct grid *gd = data->backing->grid; + u_int sy = screen_size_y(&data->screen); + + return (image_grid_check_area(gd, 0, screen_hsize(data->backing) - + data->oy, screen_size_x(&data->screen), sy)); +} +#endif + static void window_copy_redraw_lines(struct window_mode_entry *wme, u_int py, u_int ny) { @@ -5485,6 +5591,10 @@ window_copy_redraw_lines(struct window_mode_entry *wme, u_int py, u_int ny) struct screen_write_ctx ctx; u_int i; +#ifdef ENABLE_IMAGES + window_copy_update_image_refresh(data); +#endif + if (window_copy_line_number_width(wme) != 0) { screen_write_start(&ctx, &data->screen); for (i = py; i < py + ny; i++) @@ -6873,6 +6983,13 @@ window_copy_scroll_up(struct window_mode_entry *wme, u_int ny) window_copy_redraw_screen(wme); return; } +#ifdef ENABLE_IMAGES + if (window_copy_visible_has_images(data)) { + window_copy_redraw_screen(wme); + return; + } + window_copy_update_image_refresh(data); +#endif if (window_copy_line_numbers_active(wme)) { if (window_copy_line_number_mode(wme) != WINDOW_COPY_LINE_NUMBERS_ABSOLUTE) { @@ -6944,6 +7061,13 @@ window_copy_scroll_down(struct window_mode_entry *wme, u_int ny) window_copy_redraw_screen(wme); return; } +#ifdef ENABLE_IMAGES + if (window_copy_visible_has_images(data)) { + window_copy_redraw_screen(wme); + return; + } + window_copy_update_image_refresh(data); +#endif if (window_copy_line_numbers_active(wme)) { if (window_copy_line_number_mode(wme) != WINDOW_COPY_LINE_NUMBERS_ABSOLUTE) {