-
Notifications
You must be signed in to change notification settings - Fork 332
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
Add ability to use image previewers #531
Conversation
If non-zero the preview will be assumed to have side-effects.
This is called upon selection changes if the previous preview was volatile. To this end, volatilePreview was added to the nav struct
@neeshy Thanks for the PR. In case you might have missed, the latest discussion about an image preview patch from lfp was in the mailling list: https://groups.google.com/g/lf-fm/c/d8JpejD6nI0 Can you confirm this patch is up to date with the discussion there? I haven't heard from @Provessor since then. I hope he's doing ok and it's just that he's lacking free time and/or interest. |
Yea, this PR is indeed up to date with that discussion. Specifically, it's up to date with the current master branch of lfp (commit 8b35dcf6). Incidentally, I haven't run into the issue that Provessor mentioned in the mailing list, since I'm using ueberzug through a FIFO. I haven't tested this with kitty, so I don't know how severe that issue is. That being said, I do agree with assessment that the issue should be handled in the users' scripts (through forcing ordering with a FIFO), as opposed to handling it in lf itself. |
@neeshy I have taken a brief look at this and it seems good. There are some changes in the main application loop and race conditions in this loop are a pain to fix so I want to test this properly before merging. I might be able to take a further look at this in the weekend. In the mailing list discussion, I mention two issues. First, cleaner script was not running for some file changes which you already seem to fix. Second, my preview script was not caching code highlights which is likely to be a preview script issue and I just need to make sure it's the case and find a proper way to fix it. I also mention |
@neeshy I have tried this for a while and everything seems alright. I think we can merge it and keep discussing any possible changes later on so other users may have a chance to easily try it in the meantime. |
@gokcehan So, those emails on the mailing list got me to test out adding an artificial delay to my cleaner script and I actually managed to reproduce the issue @Provessor described regarding images being cleared prior to being previewed (but with ueberzug instead of kitty). I've been using the scripts on the 'test' branch of my lfimg repo (https://github.com/neeshy/lfimg/tree/test), with the addition of a Ideally in the log, what we want to see is
But as expected, I'm seeing a lot of interleaving with the current state of master. Sometimes the previewer is called as the cleaner is executing and vice-a-versa. I've come up with a partial solution that works only when there's a delay on the cleaner script but not the preview script. It's up at https://github.com/neeshy/lf. I believe Provessor was attempting to do the same sort of thing on his synchronous-previews branch, but I ran into issues with his implementation where the entire UI would be blocked. Really, we only need to block the execution of the next With this, the log is ordered properly if you go slow. However, if you change selections rapidly, for each selection only the previewer will be executed. Once you stop moving around the selection, if you select another file again, all of the former corresponding cleaners are executed in sequence one after another. Sometimes the opposite happens (the cleaners are executed first and then the previewers). I see something like this in the log:
In all cases 'begin' and 'end' sections in the log are correctly adjacent to each other. So, at least the cleaner is not being executed at the same time as the previewer. Incidentally this solution allows for trivially passing the file path to the cleaner. |
Ok, I've figured out a way to properly synchronize the two threads. It was much simpler than I thought. I am now observing a good order in that log file. I will open a new PR with my changes. |
@neeshy I think the document had the order wrong, the second and third parameter passed to preview script is not height and width, it is width and height. |
@OliverLew I think you are right about the order. I have fixed the documentation now. |
@neeshy thanks for this, it looks good.
I got caught up with other commitments and ended up having trouble with my hands (not RSI) so I haven't been able to put any time into much for ages. |
This is intended as a follow-up to #304
I've taken the code up at https://gitlab.com/Provessor/lfp, diffed it against master, and re-implemented portions of it while minimizing code changes, and organizing each logical change in its own commit (for ease of review).
Features added by this PR:
Differences with lfp:
preview
andpreviewClear
methods have been left in nav.go. Forpreview
, the caller is responsible for passing in awin
structpreviewClear
is called in a new threadgClientID
is not passed to the cleaner.preview
settingreg.volatile
is guaranteed to occur prior toreg
being sent overregChan
@Provessor Your input is welcome as well.
I've also added the scripts I use with this at https://github.com/neeshy/lfimg in case they are needed for testing.