Asynchronous Preview Preferences

[Jai Flack]

Hi,

As I mentioned in the GitHub issue #319:
https://github.com/gokcehan/lf/issues/319#issuecomment-707464160
the commit 50dd374 fixed almost all of the issues surrounding my old PR #304:
https://github.com/gokcehan/lf/pull/304
but it isn't all solved and I would like to know your preference before
continuing.


The only remaining issue I have found relates to a previewer which
wishes to draw on-top of the terminal window (such as the popular image
previews) in an asynchronous manner such as kitty's icat.

It is possible to move through the images fast enough such that a newer
selection of a smaller image will finish processing before a previous
selection of a larger image. The result will be that the older image
will be shown instead of the current selection.

As I see it there are only a couple of options to fix this:
- Keep a record of currently running preview processes and kill the old
ones before continuing with a new one. This has the downside that
text-only previews will no longer load in the background when cycled
over in the view.

- Tell users to write a more complex preview script (such as one that
uses a FIFO to ensure ordered behaviour), use a preview method that
works better (still many more options over r17 such as ueberzug or
chafa) or ask their preview method to implement an option to fix
this.

- Include another option for synchronous previews. I have got quite an
extensive implementation of this so the new UI gets drawn before
generating the preview but it would add a bit more complexity to the
flow.


Gokcehan, also if the goal is to only also have width and height then I
have a branch for that too and could submit a PR immediately.

You also mentioned intent to do this yourself when you have time so feel
free to look at what I have now at https://gitlab.com/Provessor/lfp and
finish this how you see fit; if none of this is to your taste :^).

--
Thanks,
Jai

[Gokcehan Kara]

Hello Jai,

I have tried lfp a few times before. I remember there were some occasional crashes as you mention. I'm glad to hear they are fixed. I also remember there was some jamming moving through the file list. Today I did not experience such jamming. These two issues were the main holdbacks for me to take a look at lfp further.

I tried "master" and "synchronous-previews" branches with your preview scripts below:

https://gist.github.com/Provessor/e4ee757e6c424083ca3c6441fe6ab9ac

https://gist.github.com/Provessor/dbb4a6d22945e7a42c3b904302ee273c

Are these scripts up to date? I think volatile may not be working as intended. Image previews seem to be cached and do not trigger again in subsequent visits. On the other hand, code highlights with my usual preview script do not seem to be cached and trigger "loading..." text in subsequent visits. I may be doing something silly again. I also remember you had worked out a solution to avoid extra ui draws for when movement commands do not change the current file (e.g. `down` command at the bottom of the list). Did you remove these? Are they redundant now?

As for async preview options, first option seems like a dirty solution to me, especially if it changes the current background loading behavior. Second and third options sound more reasonable if I understood these correctly.

So second option does not require anything on our side? If ueberzug is going to be the only option for true image previews, then we might still need an alternative for wayland:

https://github.com/gokcehan/lf/issues/437

For the third option, I have thought about an option to disable async previewing before. Some users are not happy to see "loading..." text:

https://github.com/gokcehan/lf/issues/277

https://github.com/gokcehan/lf/issues/414

Such an option may also provide a solution for these users. Did you change the drawing logic for this? Do you have the implementation available somewhere in public?

Gokcehan

--
You received this message because you are subscribed to the Google Groups "lf-fm" group.
To unsubscribe from this group and stop receiving emails from it, send an email to lf-fm+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/lf-fm/87a6wpqai0.fsf%40disroot.org.

[Jai Flack]

Apologies for possibly a new thread, is there a way to reply to a post
without using a google account? I can't find anything about it online..

Gokcehan Kara <gokceh...@gmail.com> writes:

> I tried "master" and "synchronous-previews" branches with your preview scripts below:
>
> https://gist.github.com/Provessor/e4ee757e6c424083ca3c6441fe6ab9ac
>
> https://gist.github.com/Provessor/dbb4a6d22945e7a42c3b904302ee273c
>
> Are these scripts up to date? I think volatile may not be working as
> intended. Image previews seem to be cached and do not trigger again in
> subsequent visits. On the other hand, code highlights with my usual
> preview script do not seem to be cached and trigger "loading..." text
> in subsequent visits. I may be doing something silly again.

No, I decided that just determining whether to reload or not based on
the exit code was better as generally if it is non-zero, something
probably went wrong and it should be regenerated next time.

Those scripts are from when it also regenerated the preview on empty
output which also meant the previews for empty files got regenerated
every time as well.

My apologies, I'll update those now to match.

> I also remember you had worked out a solution to avoid extra ui draws
> for when movement commands do not change the current file (e.g. `down`
> command at the bottom of the list). Did you remove these? Are they
> redundant now?

I tried to make the current master as small of a change as possible as
that could be easily done in another PR and benefit lf either way; one I
was actually planning on submitting tonight (UTC+10:00).

> As for async preview options, first option seems like a dirty solution
> to me, especially if it changes the current background loading
> behavior. Second and third options sound more reasonable if I
> understood these correctly.

I wouldn't be happy with the first either but it might be the best for
working OOTB.

> So second option does not require anything on our side? If ueberzug is
> going to be the only option for true image previews, then we might
> still need an alternative for wayland:
>
> https://github.com/gokcehan/lf/issues/437

No that wouldn't need any more code from us, the alternative for wayland
could just be an example script for kitty to order the previews properly
on the wiki (kitty is the only terminal I know which supports this).

> For the third option, I have thought about an option to disable async
> previewing before. Some users are not happy to see "loading..." text:
>
> https://github.com/gokcehan/lf/issues/277
>
> https://github.com/gokcehan/lf/issues/414
>
> Such an option may also provide a solution for these users. Did you
> change the drawing logic for this? Do you have the implementation
> available somewhere in public?

I don't have one made up with both options but it wouldn't take long as
I could use mostly the same logic as in synchronous-previews. My only
concern is complicating the code more as you have mentioned in the past
that it isn't in the state you would like it to be.

Another option which I have thought about is having the server generate
previews ahead of time which the client then requests upon viewing the
file. However this would be quite a lot more work and complexity for not
a big gain outside of a sshfs and similar.



If we're discussing more preview related options would it make sense to
name them differently to show what they are for?
Such as:
- previews.enabled
- previews.previewer
- previews.cleaner
- previews.async

--
Thanks,
Jai

[Jai Flack]

> My apologies, I'll update those now to match.

Updated!

They should also now work a little closer to how scope.sh in ranger
does -- a higher quality but also a little slower on PDFs for the first
preview.

--
Thanks,
Jai

[Gokcehan Kara]

Jai,

I just reply to the person with "lf-fm" in the cc from my mail client. I don't know if Google makes things harder for non-gmail users.

I have now tried the updated scripts. Images are now redrawn in subsequent visits. However, I realized images are not cleared in some cases. Specifically this happens when moving from an image to a non-image regular file. This behavior seems to be non-deterministic. I did not experience this when moving from an image to a directory. This is the same for both kitty and ueberzug versions.

Another issue is that I feel like there is still something wrong with volatile. Regular previews with code highlights do not seem to be cached with a line in my preview script as such:

highlight -O ansi "$1" 2> /dev/null || cat "$1";;

Running the preview script from the command line returns an error code 0. I haven't tried running the command with Go commands to see if `cmd.Wait()` returns an error for this script yet. But even without any preview script, regular file previews are not cached. I think this is to be expected. In the code volatile starts with true and is only set to false when the preview script runs without an error. I see this is also the case for placeholder reg previews, which I think means multiple previews can be triggered for the same file while it is loading.

Gokcehan

[Jai Flack]

Gokcehan Kara <gokceh...@gmail.com> writes:

> I just reply to the person with "lf-fm" in the cc from my mail client.
> I don't know if Google makes things harder for non-gmail users.

> I have now tried the updated scripts. Images are now redrawn in
> subsequent visits. However, I realized images are not cleared in some
> cases. Specifically this happens when moving from an image to a
> non-image regular file. This behavior seems to be non-deterministic. I
> did not experience this when moving from an image to a directory. This
> is the same for both kitty and ueberzug versions.

Strange, I can confirm this works on both my Debian (stable and
unstable) machines using those exact scripts. Does it get cleared fine
when the screen gets closed?

> Another issue is that I feel like there is still something wrong with
> volatile. Regular previews with code highlights do not seem to be
> cached with a line in my preview script as such:
>
> highlight -O ansi "$1" 2> /dev/null || cat "$1";;
>
> Running the preview script from the command line returns an error code
> 0. I haven't tried running the command with Go commands to see if
> `cmd.Wait()` returns an error for this script yet.

I'll do some testing with this to see which paths are being taken in lf.
The documentation says:
> The returned error is nil if the command runs, has no problems copying
> stdin, stdout, and stderr, and exits with a zero exit status.
So I could change this to only keep it as volatile if it has a problem
with stdout not also stdin and stderr.

I've placed that line in those example scripts which I use for testing
and don't have a problem with it getting cached.. If it has a problem it
should still log it so could you please check that for what error it is
getting?

> But even without any preview script, regular file previews are not
> cached. I think this is to be expected. In the code volatile starts
> with true and is only set to false when the preview script runs
> without an error. I see this is also the case for placeholder reg
> previews, which I think means multiple previews can be triggered for
> the same file while it is loading.

That is a good point, I made it that way so then a requested image
preview would get shown again if the previous preview is still running.

Loading a preview twice isn't ideal but do you know of any issues that
could occur due to it? If you don't mind yet another option we could
have it either way and just advise that for image previews to work
correctly it must be set but could also load other previews prematurely
as well.

--
Thanks,
Jai

[Gokcehan Kara]

Jai,

This is what I get with the image preview scripts on my arch linux with i3:

https://imgur.com/a/LuK1AK5

I get the same behavior with alacritty, kitty, and xterm terminals, and also with kitty image preview scripts on kitty terminal. It seems to happen on subsequent visits. In the code, I can see `previewClear` is called when a directory preview is to be shown but I don't know how it is called when switching from an image file to a non-image regular file.

This is what I get with my regular code highlighting preview script:

https://imgur.com/a/fDdmO5m

I have checked the log file and it seems to be returning error code 141 with lfp but not lf. Unfortunately, it doesn't show any further information for the error.

I'm a little confused about multiple previews running at the same time. If a preview is already generating for the same file, what's the point of running it again? Multiple previews for the same file should be fine for most cases, but it can cause unnecessary workload for heavy preview operations like listing archive files.

About adding options about previews, do you think it would be a good idea to add a single option to control multiple behaviors discussed here? For example, we can have an option like `trueimage` and when enabled, it can disable async previews, allow multiple previews for the same file if it's somehow necessary, and maybe allow volatile caches. Personally, I'm still not interested in using image previews myself, so adding such a joint option may allow users like me to keep the same behavior as before.

Gokcehan

[Gokcehan Kara]

Jai,

Ah my mistake there is a scenario where previewClear doesn't get called
from the rebase; I'm surprised I missed this before, my example script
might have more problems with missing caches. Earlier it was called
every time but with the old PR I got a few complaints about a little
flickering when switching between images with ueberzug so I had to make
that change.
If you would prefer to keep it more simple (and solve any related
problems) I would be happy to change it back though.

As I said before, I'm not planning to use these additions myself, so I don't feel very confident imposing an opinion about this. But if you ask me, this seems like a bug whereas flickering is not a bug but an unfortunate annoyance. I thought `cleaner` option would be a general concept rather than something specific to image previews. For example `less` has `LESSCLOSE` for something similar. The intended use case is for example to decompress archive files to a temporary file or directory and then remove these afterwards. Maybe some users may try to use it for mounting/unmounting things automatically.

I had to add another error check to check for exit codes, it seems
highlight will exit with 141 if it's stdout is closed before finishing.
I guess I'm not getting an error because of the `|| cat ..` which will
suppress the error.
This can be tested with the (possibly bash) command:
{ highlight -O ansi [file] 2> /dev/null ; echo $? >> /dev/stderr ; } | head > /dev/null

I now see the change you mention. And it can be triggered from `bash` with the option `set -o pipefail` and the command `highlight -O ansi [file] | head; echo $?`. The same example works with `cat` instead of `highlight` though, so I guess maybe `highlight` does not respond well to SIGPIPE.

You can ping me here again or send a PR when you are ready.

Gokcehan

On Mon, Oct 19, 2020 at 6:08 PM Jai Flack <jfl...@disroot.org> wrote:

Gokcehan Kara <gokceh...@gmail.com> writes:

> This is what I get with the image preview scripts on my arch linux with i3:
>
> https://imgur.com/a/LuK1AK5
>
> I get the same behavior with alacritty, kitty, and xterm terminals,
> and also with kitty image preview scripts on kitty terminal. It seems
> to happen on subsequent visits. In the code, I can see `previewClear`
> is called when a directory preview is to be shown but I don't know how
> it is called when switching from an image file to a non-image regular
> file.

Ah my mistake there is a scenario where previewClear doesn't get called
from the rebase; I'm surprised I missed this before, my example script
might have more problems with missing caches. Earlier it was called
every time but with the old PR I got a few complaints about a little
flickering when switching between images with ueberzug so I had to make
that change.

If you would prefer to keep it more simple (and solve any related
problems) I would be happy to change it back though.

> This is what I get with my regular code highlighting preview script:
>
> https://imgur.com/a/fDdmO5m
>
> I have checked the log file and it seems to be returning error code
> 141 with lfp but not lf. Unfortunately, it doesn't show any further
> information for the error.

I had to add another error check to check for exit codes, it seems
highlight will exit with 141 if it's stdout is closed before finishing.
I guess I'm not getting an error because of the `|| cat ..` which will
suppress the error.

This can be tested with the (possibly bash) command:
{ highlight -O ansi [file] 2> /dev/null ; echo $? >> /dev/stderr ; } | head > /dev/null

> I'm a little confused about multiple previews running at the same
> time. If a preview is already generating for the same file, what's the
> point of running it again? Multiple previews for the same file should
> be fine for most cases, but it can cause unnecessary workload for
> heavy preview operations like listing archive files.

It was just a question about how important restricting multiple previews
from running at a time is but yes it is very undesirable.

> About adding options about previews, do you think it would be a good
> idea to add a single option to control multiple behaviors discussed
> here? For example, we can have an option like `trueimage` and when
> enabled, it can disable async previews, allow multiple previews for
> the same file if it's somehow necessary, and maybe allow volatile
> caches. Personally, I'm still not interested in using image previews
> myself, so adding such a joint option may allow users like me to keep
> the same behavior as before.

I would be happy with that as long as it could be toggled at runtime,
normally I only use image previews for PDFs which I don't need very
often. Having an option between the two is probably the best for giving
a user the best experience.

This could mostly be boiled down to enabling/disabling async previews
and whether to ignore the exit codes or not; I'll start putting them
together.

--
Thanks,
Jai

[Jai Flack]

Gokcehan Kara <gokceh...@gmail.com> writes:

> This is what I get with the image preview scripts on my arch linux with i3:
>
> https://imgur.com/a/LuK1AK5
>
> I get the same behavior with alacritty, kitty, and xterm terminals,
> and also with kitty image preview scripts on kitty terminal. It seems
> to happen on subsequent visits. In the code, I can see `previewClear`
> is called when a directory preview is to be shown but I don't know how
> it is called when switching from an image file to a non-image regular
> file.

Ah my mistake there is a scenario where previewClear doesn't get called
from the rebase; I'm surprised I missed this before, my example script
might have more problems with missing caches. Earlier it was called
every time but with the old PR I got a few complaints about a little
flickering when switching between images with ueberzug so I had to make
that change.

If you would prefer to keep it more simple (and solve any related
problems) I would be happy to change it back though.

> This is what I get with my regular code highlighting preview script:
>
> https://imgur.com/a/fDdmO5m
>
> I have checked the log file and it seems to be returning error code
> 141 with lfp but not lf. Unfortunately, it doesn't show any further
> information for the error.

I had to add another error check to check for exit codes, it seems
highlight will exit with 141 if it's stdout is closed before finishing.
I guess I'm not getting an error because of the `|| cat ..` which will
suppress the error.

This can be tested with the (possibly bash) command:
{ highlight -O ansi [file] 2> /dev/null ; echo $? >> /dev/stderr ; } | head > /dev/null

> I'm a little confused about multiple previews running at the same
> time. If a preview is already generating for the same file, what's the
> point of running it again? Multiple previews for the same file should
> be fine for most cases, but it can cause unnecessary workload for
> heavy preview operations like listing archive files.

It was just a question about how important restricting multiple previews
from running at a time is but yes it is very undesirable.

> About adding options about previews, do you think it would be a good
> idea to add a single option to control multiple behaviors discussed
> here? For example, we can have an option like `trueimage` and when
> enabled, it can disable async previews, allow multiple previews for
> the same file if it's somehow necessary, and maybe allow volatile
> caches. Personally, I'm still not interested in using image previews
> myself, so adding such a joint option may allow users like me to keep
> the same behavior as before.