sixel: prevent images from being deleted when resizing

[veltza]

This fixes resizing issues outside of tmux not inside.

[veltza]

I found that there are a few design flaws and choices in the tresize() function that still cause issues not only for the sixel patch but also for the scrollback patch. These flaws are inherited from the vanilla st, so I need to create an issue so we can discuss it further. Since st-sx doesn't have these issues because it uses the scrollback-reflow patch that overrides the function, I'm not sure how to fix them.

[bakkeby]

I see the odd behaviour when resizing with the content moving and the the sixel image not moving when reducing the size from the bottom and moving when reducing the size from the top (and yes this also screws with the scrollback patch as you say).

Wonder if I should take the plunge and try to integrate the reflow patch as well.

[bakkeby]

I had a play around with using image scroll logic in the tresize function instead of the expand images solution.

diff --git a/st.c b/st.c
index 39f5237..f2b4b68 100644
--- a/st.c
+++ b/st.c
@@ -3577,8 +3577,17 @@ tresize(int col, int row)
        int x, x2;
        Line line;
        ImageList *im, *next;
-       #endif // SIXEL_PATCH

+       if (row < term.row && term.c.y >= minrow) {
+               for (im = term.images; im; im = next) {
+                       next = im->next;
+                       if (im->y >= 0 && im->y <= term.bot) {
+                               im->y -= (1 + term.c.y - minrow);
+                       }
+               }
+       }
+
+       #endif // SIXEL_PATCH
        #if KEYBOARDSELECT_PATCH
        if ( row < term.row  || col < term.col )
                toggle_winmode(trt_kbdselect(XK_Escape, NULL, 0));
@@ -3721,43 +3730,6 @@ tresize(int col, int row)
                tcursor(CURSOR_LOAD);
        }
        term.c = c;
-
-       #if SIXEL_PATCH
-       /* expand images into new text cells to prevent them from being deleted in
-        * xfinishdraw() that draws the images */
-       for (i = 0; i < 2; i++) {
-               for (im = term.images; im; im = next) {
-                       next = im->next;
-                       #if SCROLLBACK_PATCH
-                       if (IS_SET(MODE_ALTSCREEN)) {
-                               if (im->y < 0 || im->y >= term.row) {
-                                       delete_image(im);
-                                       continue;
-                               }
-                               line = term.line[im->y];
-                       } else {
-                               if (im->y - term.scr < -HISTSIZE || im->y - term.scr >= term.row) {
-                                       delete_image(im);
-                                       continue;
-                               }
-                               line = TLINE(im->y);
-                       }
-                       #else
-                       if (im->y < 0 || im->y >= term.row) {
-                               delete_image(im);
-                               continue;
-                       }
-                       line = term.line[im->y];
-                       #endif // SCROLLBACK_PATCH
-                       x2 = MIN(im->x + im->cols, term.col);
-                       for (x = im->x; x < x2; x++) {
-                               line[x].u = ' ';
-                               line[x].mode = ATTR_SIXEL;
-                       }
-               }
-               tswapscreen();
-       }
-       #endif // SIXEL_PATCH
 }

Feels a bit more natural when reducing the size of the window.

Images that go beyond the top get "cut off", but in a similar manner to how the scrollback get cut off. Not great but at least it is consistent.

What is your take on that?

[veltza]

The expansion routine is not redundant, so don't remove it. It fixes those cases where the image is horizontally larger than the screen and you increase the size of the window horizontally. Without the routine, the xfinishdraw() function thinks that the text cells behind the image have been changed, because the new cells don't have the ATTR_SIXEL, and deletes the image. This is how auto-deletion routine works.

Your code seems to work, but it has the same issues as the scrollback patch, like you mentioned earlier. It is not perfect, but if you're happy with it, I'm fine with it. However, images should be moved on both screens in case the window is resized on the alternate screen:

    if (row < term.row && term.c.y >= minrow) {
        for (i = 0; i < 2; i++) {
            for (im = term.images; im; im = next) {
                next = im->next;
                if (im->y >= 0 && im->y <= term.bot) {
                    im->y -= (1 + term.c.y - minrow);
                }
            }
            tswapscreen();
        }
    }

The reason why sixel and scrollback patches work poorly when resizing is due to this code. For clarity, the excerpt is from the vanilla st:

   2561     /*
   2562      * slide screen to keep cursor where we expect it -
   2563      * tscrollup would work here, but we can optimize to
   2564      * memmove because we're freeing the earlier lines
   2565      */
   2566     for (i = 0; i <= term.c.y - row; i++) {
   2567         free(term.line[i]);
   2568         free(term.alt[i]);
   2569     }
   2570     /* ensure that both src and dst are not NULL */
   2571     if (i > 0) {
   2572         memmove(term.line, term.line + i, row * sizeof(Line));
   2573         memmove(term.alt, term.alt + i, row * sizeof(Line));
   2574     }
   2575     for (i += row; i < term.row; i++) {
   2576         free(term.line[i]);
   2577         free(term.alt[i]);
   2578     }

As you can see, this optimization bypasses the tscrollup() function call, causing havoc for all patches that depend on that function. So this is why the scrollback patch can't capture lines and the sixel patch can't scroll images when resizing.

And that's not all. There is also a design flaw in the above code. It takes the cursor position from the active screen and then scrolls the both screens equally. When it should scroll the screens independently depending on where the cursor is on that screen. Why? Let's take an example. We are on an alternate screen and the active cursor is at the top of the alternate screen, but the inactive cursor is at the bottom of the main screen. Now when you reduce the height of the window, the code thinks that neither screen needs to be scrolled because the active cursor is at the top of the screen. So it just deletes the lines from the bottom of the both screens and the main screen gets truncated from the bottom.

So rewriting the tresize() function and fixing the above issues should also fix both patches when resizing. The question is, how much effort do you want to put in, or are you just happy with the current behavior?

[veltza]

I went ahead and wrote a patch for the vanilla st that fixes the issues with the tresize() function, if you want to try it. I also quickly tested it with st-flexipatch, which only had the scrollback and sixel patches, and it seemed to work as expected. I suggest you use the old tresize() implementation for the vim browse patch and the patched version for everything else, if you plan to use it.

diff --git a/st.c b/st.c
index 77c3e8a..933ae30 100644
--- a/st.c
+++ b/st.c
@@ -2546,11 +2546,10 @@ twrite(const char *buf, int buflen, int show_ctrl)
 void
 tresize(int col, int row)
 {
-   int i;
+   int i, j;
    int minrow = MIN(row, term.row);
    int mincol = MIN(col, term.col);
    int *bp;
-   TCursor c;

    if (col < 1 || row < 1) {
        fprintf(stderr,
@@ -2558,23 +2557,18 @@ tresize(int col, int row)
        return;
    }

-   /*
-    * slide screen to keep cursor where we expect it -
-    * tscrollup would work here, but we can optimize to
-    * memmove because we're freeing the earlier lines
-    */
-   for (i = 0; i <= term.c.y - row; i++) {
-       free(term.line[i]);
-       free(term.alt[i]);
-   }
-   /* ensure that both src and dst are not NULL */
-   if (i > 0) {
-       memmove(term.line, term.line + i, row * sizeof(Line));
-       memmove(term.alt, term.alt + i, row * sizeof(Line));
-   }
-   for (i += row; i < term.row; i++) {
-       free(term.line[i]);
-       free(term.alt[i]);
+   /* scroll both screens independently */
+   if (row < term.row) {
+       tcursor(CURSOR_SAVE);
+       tsetscroll(0, term.row - 1);
+       for (i = 0; i < 2; i++) {
+           if (term.c.y >= row)
+               tscrollup(0, term.c.y - row + 1);
+           for (j = row; j < term.row; j++)
+               free(term.line[j]);
+           tswapscreen();
+           tcursor(CURSOR_LOAD);
+       }
    }

    /* resize to new height */
@@ -2608,11 +2602,10 @@ tresize(int col, int row)
    term.row = row;
    /* reset scrolling region */
    tsetscroll(0, row-1);
-   /* make use of the LIMIT in tmoveto */
-   tmoveto(term.c.x, term.c.y);
    /* Clearing both screens (it makes dirty all lines) */
-   c = term.c;
    for (i = 0; i < 2; i++) {
+       tmoveto(term.c.x, term.c.y);  /* make use of the LIMIT in tmoveto */
+       tcursor(CURSOR_SAVE);
        if (mincol < col && 0 < minrow) {
            tclearregion(mincol, 0, col - 1, minrow - 1);
        }
@@ -2622,7 +2615,6 @@ tresize(int col, int row)
        tswapscreen();
        tcursor(CURSOR_LOAD);
    }
-   term.c = c;
 }

 void

[bakkeby]

Wow, thanks, I'll try it out. You really know your terminal.

If it works then it may be worth suggesting this upstream.

As for the vim browse patch I'd kind of prefer to remove it tbh. It is too invasive and naturally conflicts with nearly every patch of relevance.

[bakkeby]

That works really well. I have pushed the change on this branch.

Not aware of any outstanding issues so maybe this can be merged now.

[veltza]

I had a quick glance and noticed you forgot to remove this.

Although the scrollback and sixel patches both ignore copyhist flag on the alternate screen, I recommend that you explicitly state that we don't want to copy the history when we scroll up the alternate screen. Just in case someone else is reading the code too.

diff --git a/st.c b/st.c
index 97befd2..0ef00db 100644
--- a/st.c
+++ b/st.c
@@ -3808,7 +3808,7 @@ tresize(int col, int row)
        for (i = 0; i < 2; i++) {
            if (term.c.y >= row) {
                #if SCROLLBACK_PATCH
-               tscrollup(0, term.c.y - row + 1, 1);
+               tscrollup(0, term.c.y - row + 1, !IS_SET(MODE_ALTSCREEN));
                #else
                tscrollup(0, term.c.y - row + 1);
                #endif // SCROLLBACK_PATCH

I haven't noticed anything else, so I think it's ready to be merged.