Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Horizontal resizing does not flush cached sixels #1452

Closed
veltza opened this issue Sep 30, 2023 · 7 comments · Fixed by #1629
Closed

Horizontal resizing does not flush cached sixels #1452

veltza opened this issue Sep 30, 2023 · 7 comments · Fixed by #1629

Comments

@veltza
Copy link
Contributor

veltza commented Sep 30, 2023

When you resize the terminal window horizontally, lf does not flush the cache and reload the images. So you get a cropped image as you can see below.

a2

But if you resize vertically, the cache will be flushed and the images will be reloaded.

a3

According to this commit 628bf40, it's designed to work that way, but it's unclear why the cache is flushed only on vertical resizing.

I'm using the following previewer:

#!/bin/sh
case "$(readlink -f "$1")" in
    *.bmp|*.gif|*.jpg|*.jpeg|*.png|*.webp|*.six|*.svg|*.xpm)
        chafa -f sixel -s "$2x$3" --animate false "$1" ;;
    *)
        bat -pp -f "$1" ;;
esac

Note that I don't use the exit 1 command because I don't want to disable image caching.

@joelim-work
Copy link
Collaborator

Thanks for reporting. I guess that most of the other users who were interested in sixel previews never encountered this issue because they just used exit 1 as that was recommended in the PR description #1211 (comment).

The reason why the file preview cache is only cleared on vertical resizing is because it predates sixel support. Originally, file previews only supported text content, which is implemented as an array of strings, limited to the height of the preview window. For instance if the preview window has a height of 30 lines, then only the first 30 lines of the file would be read and stored for previewing. This also means that if the height of the preview window increases, the preview would no longer be valid and have to be reloaded. Horizontal resizing doesn't matter in this case since entire lines are stored, and lines are simply truncated if they exceed the width of the preview window.

I guess we could fix this by unconditionally invalidating the file preview cache if sixel is enabled:

diff --git a/eval.go b/eval.go
index ff04308..23df598 100644
--- a/eval.go
+++ b/eval.go
@@ -1940,6 +1940,9 @@ func (e *callExpr) eval(app *app, args []string) {
 			app.nav.height = app.ui.wins[0].h
 			app.nav.regCache = make(map[string]*reg)
 		}
+		if gOpts.sixel {
+			app.nav.regCache = make(map[string]*reg)
+		}
 		for _, dir := range app.nav.dirs {
 			dir.boundPos(app.nav.height)
 		}

We might also have to do something similar in other places where the horizontal size of the preview window can change, for example if the ratios option is set.

@veltza
Copy link
Contributor Author

veltza commented Oct 1, 2023

I read the entire PR and found that you have already discussed this issue here #1211 (comment) and here #1211 (comment) but it remained unsolved. I also noticed that @horriblename added the exit 1 command on the same day the PR was merged. So are there other cache issues as well since he wanted to disable it in the example?

To me your solution is fine because it doesn't affect the users who don't use the sixels. But as you guessed, the same problem occurs when you change the ratios and that problem needs to be solved too.

@horriblename
Copy link
Contributor

So are there other cache issues as well since he wanted to disable it in the example?

nope, this is the only problem

@joelim-work I'll make a PR from your patch above, there's one other thing that needs to be done (force filler to redraw)

@veltza
Copy link
Contributor Author

veltza commented Oct 6, 2023

I just noticed that the same cropping problem occurs when using symbols in chafa:
chafa -s "${2}x${3}" -f symbols --animate false "$1"

a4

So it seems to me that we always need to invalidate the file preview cache regardless of whether we resize the terminal window horizontally or vertically. Because we never know which previewer tools actually depend on the width and height attributes of the preview pane.

@veltza
Copy link
Contributor Author

veltza commented Mar 7, 2024

@joelim-work I read that you are releasing a new version. Is it too much to ask to have this fixed in there as it is very easy to fix?

I have no idea what the "force filler to redraw" issue @horriblename mentioned is, but I don't think we need to worry about it now since everything seems to be working fine with the suggested fix.

@joelim-work
Copy link
Collaborator

joelim-work commented Mar 7, 2024

I think the "force filler to redraw" is referring to this code:

lf/sixel.go

Lines 60 to 65 in 43c2683

// HACK: fillers are used to control when tcell redraws the region where a sixel image is drawn.
// alternating between bold and regular is to clear the image before drawing a new one.
st := sxs.fillerStyle(reg.path)
for y := 0; y < win.h; y++ {
st = win.print(screen, 0, y, st, strings.Repeat(string(gSixelFiller), win.w))
}

But alternating between bold/regular to clear the image only happens when drawing the preview for a different file, not if the size of the preview changes for the current file. I found a way to work around it though, by setting lastFile to an empty string.

That being said, I think alternating between bold/regular to clear sixel images is specific to only some terminal emulators, though I'm not 100% sure about this. Anyway I submitted #1629, hopefully that should fix your issue.

@veltza
Copy link
Contributor Author

veltza commented Mar 8, 2024

Your fix works as expected. Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants