lf icon indicating copy to clipboard operation
lf copied to clipboard

Horizontal resizing does not flush cached sixels

Open veltza opened this issue 1 year ago • 4 comments

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 https://github.com/gokcehan/lf/commit/628bf408b4a8cd55154961a5cf54316762a58e72, 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.

veltza avatar Sep 30 '23 08:09 veltza

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 https://github.com/gokcehan/lf/pull/1211#issue-1666086783.

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.

joelim-work avatar Oct 01 '23 02:10 joelim-work

I read the entire PR and found that you have already discussed this issue here https://github.com/gokcehan/lf/pull/1211#issuecomment-1529739837 and here https://github.com/gokcehan/lf/pull/1211#issuecomment-1529812057 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.

veltza avatar Oct 01 '23 10:10 veltza

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)

horriblename avatar Oct 01 '23 12:10 horriblename

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 avatar Oct 06 '23 11:10 veltza

@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.

veltza avatar Mar 07 '24 10:03 veltza

I think the "force filler to redraw" is referring to this code: https://github.com/gokcehan/lf/blob/43c2683ca86d30789b6d2ba82460a568a27a69b9/sixel.go#L60-L65

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.

joelim-work avatar Mar 07 '24 14:03 joelim-work

Your fix works as expected. Thanks a lot!

veltza avatar Mar 08 '24 11:03 veltza