Improve preview loading message flickering

[joelim-work]

Fixes #414

Changes:

• Add a previewLoading variable, which decides if a preview is taking too long to load, in which case the loading... message will be displayed (as opposed to a blank screen).
• When a file/directory is selected, previewLoading will initially be set to false, and a timer is set to one second 100ms (restored to one second 100ms if it is already running). When the timer expires, previewLoading will be set to true.

[gokcehan]

@joelim-work Thanks for working on this. I'm generally ok with this behavior but a couple of points below:

1. When the preview takes some time (more than 1 seconds), you have to wait 1 second to decide if the file is actually empty or just loading. This is why I personally think the current behavior is superior. One possible solution would be to add empty text for empty files. I'm not sure if that's a good idea though as an empty file might be hard to define. Is it files with no output to stdout in the preview script? What if the preview script exits with an error before any output? Also, how about true image preview folks? Do they print anything to stdout? If not, there might be empty text under true image previews. Another possible solution might be to make the delay configurable so people can reduce it. I would even go on to argue that the default delay should be 0 for least amount of surprise (i.e. current behavior).
2. I might have mentioned this somewhere but I think a better solution would be to have instead a loading icon (e.g. three dots,, an hourglass, or even some kind of spinner if we want to go fancy) somewhere in the ui (i.e. somewhere in the top or bottom bars). This might have the advantage of showing an indicator for cases when something is loading but the old content is still showing as a placeholder. I think the main reason people find the current behavior annoying is that the loading... text is written in reverse colors on the spot where they are focused. Otherwise, isn't it the technically the same amount of flicker? (i.e. two screen redraws). That's why I think a loading icon could also solve this issue with presumably less additional complexity.
3. I'm usually worried about adding and touching the existing asynchronous behavior in the code as they are often hard to reason. So I'm thinking of leaving this PR open for a while to see if everything is ok. If you are interested in this patch, feel free to test it and report issues if any. I think it would be especially useful if someone can test this with true image preview scripts.

[joelim-work]

Hi @gokcehan, thanks for sharing your thoughts on this, you raise some good points. Here are my responses:

1. Originally when I submitted this PR, I chose a timeout of 1 second because it was convenient for testing purposes. But I agree that it's unacceptably long for a real world scenario. I have changed this to 100 milliseconds for now, perhaps I can reduce it even more and it should still be fine. I have thought about using empty to signify empty files, but I'm hesitant to do this as it could end up being misleading due to the reasons you have given. Also, I am open to adding a config option for this (50-100ms works fine for me, but I can't be sure unless others help to test this), but I'm worried if I add such an option, then it will become hard to remove later on.
2. I would say that the flicker with previewA -> blank screen -> previewB is a less annoying experience than the flicker with previewA -> loading... -> previewB, because the latter involves reverse colors and visibly stands out, and also because it is in the same location as the preview. But I think just simply displaying this in the status bar at the bottom is also a good alternative. I'm not sure exactly what to display though - probably I would go with the original loading... message even if it is a bit verbose. An icon may not work for everyone, especially if they don't have the necessary font installed, and a spinner animation would require frequent draw updates (may not be an issue but I haven't tried implementing it).
3. I have tried to keep the implementation as simple as possible (all the work is done in the main thread as opposed to the threads that handle file/directory previews). But I share your concerns here, it is always possible for someone else in future to accidentally make a change that will cause some issue that is hard to debug.

I have tested this using previews for directories, text files and images (via ueberzug), and also with and without previewer scripts. But I also agree with your decision to keep this PR open to give others an opportunity to test this change. If you end up not liking it this approach, I'm happy to consider just moving the location of the loading message.

[joelim-work]

Added a previewtimeout option which specifies the delay in milliseconds. For example to set the delay to 100 milliseconds, add set previewtimeout 100 to the config file (or alternatively type it out in the command line).

The default value is 0 to maintain the original behaviour. In other words, this feature is opt-in.

[gokcehan]

@joelim-work I was waiting for the discussion to settle before taking another look at this. I liked the idea of using 100 milliseconds for the timeout as it felt right in practice. It seems that you now added this as an option with a default of 0. I know I was the one suggesting this but I should say I'm also ok if you want to throw away the option altogether and/or make the default 100 milliseconds. I think we have similar hard-coded delays in the program (e.g. timeout for the esc key). I don't care that much either way so let me know your final decision and I think we can then merge this.

[joelim-work]

Hi @gokcehan

Thanks for your comments. After some thinking I have decided for now to just hard-code the timeout value to 100 milliseconds for the following reasons:

• There is less work involved and the change is smaller
• Adding a config option means that it will become part of the user interface, so it will be difficult to change or remove in future
• Using different units of time in the config file can be confusing (for instance period is defined in seconds, but this option will be in milliseconds)
• Many users will simply upgrade lf without reading the changelog, and I think it would be better if the delay is applied by default so that all users can benefit from it
• If it becomes necessary, we can add a config option if users want to specify their own delay period. Hopefully it won't happen, but in any case it is much easier to add a config option than to remove one.

[gokcehan]

@joelim-work Sounds good, thanks for the patch.