-
Notifications
You must be signed in to change notification settings - Fork 157
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
use ratatui-image widget for image previews without any scripts #467
Conversation
Hi! Very interesting! Would this solution support a combination of an image and a text preview? Like the image widget being on the upper right and the text preview being on the lower right? Would the ratio of this vertical split be configurable? (Maybe a And: is there any plan in “ratatui-image” to support Überzug++ as a backend? At the moment it looks like removing the hooks and using ratatui-image instead would leave many users without image support. Would be really cool if this works out! |
Could probably make some "preview" wrapper here that takes care of text and a Currently there is no Ueberzug support, but I am trying to add it. The PR has the hooks etc. removed (I was a bit too optimistic), but I will undo that and rather have some option to enable this widget, or only enable it if the hooks aren't configured. |
Very interesting indeed! Let me know when this is ready for review! |
One question that I have is, will this pull in additional dependencies? |
Luckily, someone ported just the relevant part of libsixel to rust in the meantime: https://github.com/mkrueger/icy_sixel |
87ef7a7
to
32be70b
Compare
@DLFW I refactored the PR so that scripts are unaffected. There is however one breaking change. The I have removed the default value, so that joshuto can render images with ratatui-image by default. Otherwise it would be necessary to 1) attempt to run that script every time an image is focused and check the return value, or 2) check at startup if that script exists (kinda weird thing to do), or 3) do some migration. Re: additional info, right now just the image is shown, but we could also add some other textual info. Re: ueberzug support in ratatui-image: no because ratatui-image is "immediate mode", i.e. there is specifically no notion of "removed / stopped rendering". So good reason to leave the scripts support in, as you asked. @kamiyaa ratatui-image is now using icy_sixel, so all native rust deps. The supported image formats are currently the defaults of the image crate (https://docs.rs/crate/image/0.24.8/features), which could also be trimmed down a bit if desired. But no dynamic linking in any case. I have not tested yet that script support hasn't been broken, once I've done that I'll mark this as ready. |
I can't get the |
99f95ce
to
284cbf5
Compare
I added a commit that attempts the script first, and then renders as image. This way there are both text previews and image previews, and it's not a breaking change. The only required action is for users that have a |
Does that mean that
What does “exclude” mean? Is the return value used somehow? Or does it mean the script must not write anything to
Means |
52c8529
to
a7f5d30
Compare
@DLFW sorry I forgot to add/undo the changes to The way it is now, this PR is not a breaking change (caveat: I could not set up and test the hook scripts myself). But existing users that previously followed instructions may have to change their configs, if they want to use the new built-in previews:
I guess that also would be an option. In fact, we could run both at the same time (script and image encoding), instead of sequentially, and display any that succeeded.
So far my idea was that the widgets provided do work like any ratatui widget, as in they require no removed-like hooks. However, seeing how popular ueberzug is, and how it could really complete terminal support, I am now considering adding another widget plus some remove-like hook. But in respect to joshuto, that could then be a next step. |
Is this PR ready for review? |
a7f5d30
to
41eac5b
Compare
@kamiyaa yes. Sorry there was some mistake with ratatui's default-features in the crate, fixed now. |
Hi, I tested the image preview with However, the solution seems not to work reliably for me. My observations:
I guess that you have broken some basic functionality somewhere. Seems the preview is just not issued anymore for anything. One more question: your last commit is titled “can use preview_script alongside image widget”. Does that mean that I should be able to have a text and an image preview together now? If yes, how? I changed my |
@DLFW thank you for testing this, and pardon me for not getting down to figuring out the existing hooks myself. I have now figured it out, at least for kitty, so I can test it better from here on. I could reproduce the "hanging" myself now, with the provided preview_file.sh. Visiting some specific .xlsx file, the xlsx2csv commands hangs forever, and thus the preview thread is stuck forever. Previous successful previews do show again since they're cached independently from the thread. The problem is this manifests also in A similar issue occurs when visiting many files very fast, e.g. running the cursor down with J. Each file is processed in sequence (either due to the lock in Also, sometimes the remove hook is not called when navigating very fast, but that's not relevant here. I will make changes so that the ratatui-image preview is shown together and independent of |
@benjajaja, I have no idea what causes the issue, but let me just emphasis that the problem was with The files I visited were also the same in my tests and they were not exceptionally big or something. The preview of those files did work fine with the latest upstream And as I said: the CPU was pretty calm. Your explanation sounds like it's a general performance problem (maybe related to #499 ?), but for me it looks more like some logical problem. However, I'm happy to try the same tests again, once you're there... BTW, I'm really happy that you're on the way to tackle this problem! It's an amazing feature!
Yay! 👍👍 😉 |
680431c
to
05e7976
Compare
@DLFW now, it should work as expected with or without hooks. ratatui-image renders independently of
I'm still not 100% satisfied with how the threads and queue work. When visiting a file, the request to preview should be inserted at the beginning of the queue. |
05e7976
to
df3f5ec
Compare
Hi, just did another quick test. Unfortunately, with similar results. I hope I don't do anything wrong. What I did and how I can reproduce it:
If the With the same configuration but a fresh build from upstream Beside the changes I described above, these are my dotfiles: https://gitlab.com/DLF/dotfiles/-/tree/master/dotfiles/.config/joshuto Don't you experience any performance issue for the text-previews yourself? If not, maybe another person can try out your branch also. |
Ok, looks like @luteran42 just did some test with a similar experience... 🙃 |
2771200
to
ee731a5
Compare
Yea, the ratatui-image loading was algo blocking the preview_file.sh script. I changed it to two threads, so that the script isn't affected by the image loading. I also removed the image from the cache to keep memory at bay. Sixel support could be a config option. Sixels are created with icy_sixel, a native implementation, I don't think it makes sense to put that behind a feature flag. I tested quickly, but I will run some more deeper tests before marking for review again. |
@benjajaja : Nice! One question: Why was/is ratatui-image significantly blocking text-only previews? What keeps ratatui-image busy when I'm just looking at - for example - a toml-file? |
Eh, just gave your branch another quick test. Now the “big files” (toml-files >= 2k) are immediately shown when I put the cursor on them the first time! But, the text-preview does not show again a second time when I move the cursor to some other file and then back to “the big file”. 😁 |
Before, there was one thread/queue processing visits as script and image. Until both processings have completed, even though they would run in parallel, the next visit will not start for neither script nor image. So as you visit files, the queue keeps growing, until you stop visiting files, and shrinks again to zero. While the queue was not empty, you would get that blocking effect. If you would wait for the queue to be empty, and then just e.g. visit the next file, the script should have been immediately called/rendered. But that is fixed now, with two separate queues.
Thank you for finding that bug! Some leftover from a previous commit, an image error was mistakenly sent to the cache. For now, image errors are just ignored, since images always reload because they're not in the cache anymore. |
Seems to be working! 👍 Tested both the sixel, kitty and halfblocks protocol in foot/alacritty/kitty terminal, all worked fine. only problem i found was in tmux, the sixel protocol was unreliable, but that's just might be tmux weirdness. also it would be nice if we could set the protocol in the config file. Thank you for your work! 😎 |
Next round: But image previews still only work for a certain time for me. Easy to reproducible on st, wezterm and kitty for me, just by jumping through some dirs. BTW, this also happens if I change from one image to the next only after one is loaded and displayed. I explicitly tried that to be sure that it's not a queuing/performance thing. |
weird. I tested it again in st, foot, alacritty, kitty and it worked. only tmux was messing it up. |
9604eb1
to
cb37aff
Compare
Added a config option to disable or e.g. force @DLFW for me it works fine now, jumping around directories with lots of images or files, I never get a missed image for images that should render (default formats of image crate, and under size limit). Tmux and friends are problematic, but maybe it will get solved in ratatui-image someday, because it has been reported elsewhere. |
So it's good to review now. Please comment if you think the feature should not be enabled by default as the PR currently does, but should be |
I think auto is okay, halfblocks probably the safest. |
_Disclaimer: I'm the author of ratatui-image._ Use [ratatui-image](https://github.com/benjajaja/ratatui-image) crate to render images. No script or hook setup is required.
cb37aff
to
c39c0a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Disclaimer: I'm the author of ratatui-image.
Use ratatui-image crate to render images. No script or hook setup is required. ratatui-image supports sixel, kitty, and iTerm2 protocols. At least xterm, foot, kitty, iTerm2 and Wezterm work out of the box.
Screenshot using kitty: