kovidgoyal / kitty

Cross-platform, fast, feature-rich, GPU based terminal
https://sw.kovidgoyal.net/kitty/
GNU General Public License v3.0
22.51k stars 914 forks source link

Image placement using Unicode placeholders #5664

Closed sergei-grechanik closed 1 year ago

sergei-grechanik commented 1 year ago

This commit introduces the Unicode placeholder image placement method. In particular:

kovidgoyal commented 1 year ago

Cool, I will look at this in a while. To start with could you do the follwoing:

1) Resolve the conflicts 2) Make a small unrelated PR that I can merge (could be anything trivial) so that the tests are run on your PRs automatically without waiting for me to approve.

sergei-grechanik commented 1 year ago

Extracted a small fix as https://github.com/kovidgoyal/kitty/pull/5682

slava-mironov commented 1 year ago

Hi! Do I understand it correctly that with this it will be possible to view images in tmux when it's run under kitty?

sergei-grechanik commented 1 year ago

Yes, it will work in tmux. The icat kitty will use this method (and some other tmux workarounds) when the --tmux flag is passed.

slava-mironov commented 1 year ago

That's great! BTW, I tried building kitty with this pr on Mac OS and also tried your experimental branch with upload_ script, couldn't make either work with tmux locally (without tmux works fine), but I didn't dig too much. If you need something tested, I'd be happy to spend some time on this. My usual setup is Mac OS and remote CentOS or Debian servers, I use almost identical environments in tmux locally and remotely.

sergei-grechanik commented 1 year ago

I remember some portability issues with my uploading script on Mac OS, but I think icat should work. Just a guess: check that you have set -gq allow-passthrough in tmux config, in newer versions of tmux it's disabled by default.

slava-mironov commented 1 year ago

Thanks! After setting allow-passthrough on and then running kitty icat --tmux ~/test.jpg the image was shown correctly. I tried doing this in multiple panes, zooming, resizing them, everything works. This is brilliant!

One thing that is a bit inconvenient, running without tmux, but leaving --tmux option on will cause a temporary freeze and then produce an error. This may cause a problem with scripting.

kovidgoyal commented 1 year ago

Comments:

1) At least on my system tmux does report pixels sizes so no need for the workaround. Presumably this was added in more recent tmux versions.

2) Resolve the conflict in docs/changelog.rst

3) It's not clear to me why deletion of virtual images is restricted to only id based.

4) When generating the row/column diacritics for python just use repr(str) rather than \U form, faster to parse. Use tuples instead of lists, more memory efficient and faster to load. rowcolumn_diacritics_utf8 should also be generated directly rather than requiring encoding at runtime. Also rather tham importing this at top level delay import only when needed so that people not using unicode placeholder dont pay the startup cost.

5) Regarding --tmux and --unicode-placeholders for icat, maybe better to have a --mode argument that defaults to auto and has normal, tmux, unicode-placeholders auto mode can use normal unless it detects it is running in tmux in which case it can use tmux mode (see the existing running_in_tmux function) Also when running in tmux it should run tmux set -p allow-passthrough on (see the ssh kitten for an example) otherwise I am going to be flooded by lots of bug reports about it not working in tmux because allow-passthrough defaults to off.

6) in write_gr_command() rather than concatenating long bytestrings just write the tmux prefix and suffix directly to sys.stdout.buffer

7) In fonts.c make IMAGE_PLACEHOLDER_CHAR a case in the switch statement

8) Make IMAGE_PLACEHOLDER_CHAR defined in only one place in C code and the icat python code can import it from there via fast_data_types

9) The documentation needs cleanup but I will do that myself after this PR is merged.

10) I dont like that fact that we are unconditionally iterating over all cells twice. Instead perhaps modify render_line() to set a flag if it finds an image placement char in the line and only if it does call screen_render_line_graphics()

I haven't reviewed the actual algorithms/code in detail yet, will get to that after this round. Generally speaking it looks good to me, thanks for your contribution.

sergei-grechanik commented 1 year ago

Thanks for the review, I'll try to address all the issues over the weekend.

  1. At least on my system tmux does report pixels sizes so no need for the workaround. Presumably this was added in more recent tmux versions.

I can confirm that, the workaround is necessary for tmux 3.0, but not for tmux 3.4. I will remove it.

3. It's not clear to me why deletion of virtual images is restricted to only id based.

Virtual placements are not considered visible on screen, so deleting them by screen position (a, c, p, q, x, y, z) doesn't make sense. The visible references created on top of placeholder cells are just copies of the original virtual placement. They can be affected by position-based deletion commands, but it will not result in the deletion of the original virtual placement (doing so would be confusing, in my opinion, because there could be multiple references created from the same virtual placement, and deleting one of them shouldn't affect the others). Also, deleting these copy references doesn't make much sense because they will respawn from the placeholder cells on text modification, so it may be a good idea to forbid deleting them.

(btw, I think there is a bug in the code, I forgot to add the if (ref->is_virtual_ref) check to x/y/z_filter_func)

kovidgoyal commented 1 year ago

On Sat, Dec 17, 2022 at 04:10:50PM -0800, Sergei Grechanik wrote:

Thanks for the review, I'll try to address all the issues over the weekend.

Cool.

  1. At least on my system tmux does report pixels sizes so no need for the workaround. Presumably this was added in more recent tmux versions.

I can confirm that, the workaround is necessary for tmux 3.0, but not for tmux 3.4. I will remove it.

3. It's not clear to me why deletion of virtual images is restricted to only id based.

Virtual placements are not considered visible on screen, so deleting them by screen position (a, c, p, q, x, y, z) doesn't make sense. The visible references created on top of placeholder cells are just copies of the original virtual placement. They can be affected by position-based deletion commands, but it will not result in the deletion of the original virtual placement (doing so would be confusing, in my opinion, because there could be multiple references created from the same virtual placement, and deleting one of them shouldn't affect the others). Also, deleting these copy references doesn't make much sense because they will respawn from the placeholder cells on text modification, so it may be a good idea to forbid deleting them.

Makes sense, maybe add a line explaining this to the docs. Mention that if deletion is desired for these then it is important to use ids.

sergei-grechanik commented 1 year ago

Found a bug in cell image rendering logic, this will take some time.

sergei-grechanik commented 1 year ago

Fixed the issues.

When generating the row/column diacritics for python just use repr(str) rather than \U form, faster to parse. Use tuples instead of lists, more memory efficient and faster to load. rowcolumn_diacritics_utf8 should also be generated directly rather than requiring encoding at runtime.

Actually, rowcolumn_diacritics_utf8 is the only thing I need. It's represented with literals like b'\xcc\x85' though, so not sure about the parsing speed.

Regarding --tmux and --unicode-placeholders for icat, maybe better to have a --mode argument that defaults to auto and has normal, tmux, unicode-placeholders

I decided not to combine them into one option, mostly because mode sounds too generic. I changed --tmux to have three states: detect (default), yes, and no.

Also when running in tmux it should run tmux set -p allow-passthrough on (see the ssh kitten for an example)

I changed the logic of how this option is set. It now first checks if it is already set, and sets it to on only if it is not set. This is to avoid overriding the option if it has the recently introduced value all (allow pass-through from invisible panes).

kovidgoyal commented 1 year ago

On Sat, Dec 31, 2022 at 04:41:11PM -0800, Sergei Grechanik wrote:

Fixed the issues.

Thanks, looking good.

When generating the row/column diacritics for python just use repr(str) rather than \U form, faster to parse. Use tuples instead of lists, more memory efficient and faster to load. rowcolumn_diacritics_utf8 should also be generated directly rather than requiring encoding at runtime.

Actually, rowcolumn_diacritics_utf8 is the only thing I need. It's represented with literals like b'\xcc\x85' though, so not sure about the parsing speed.

icat is being re-implemented in Go anyway (see the icat branch) so this becomes irrelevant.

Regarding --tmux and --unicode-placeholders for icat, maybe better to have a --mode argument that defaults to auto and has normal, tmux, unicode-placeholders

I decided not to combine them into one option, mostly because mode sounds too generic. I changed --tmux to have three states: detect (default), yes, and no.

Maybe name it something like --passthrough (after all in the future we might want to add support for other multiplexers, not just tmux). --passthrough=detect|tmux|none (this can anyway be changed when porting to Go).

Also when running in tmux it should run tmux set -p allow-passthrough on (see the ssh kitten for an example)

I changed the logic of how this option is set. It now first checks if it is already set, and sets it to on only if it is not set. This is to avoid overriding the option if it has the recently introduced value all (allow pass-through from invisible panes).

This looks OK.

My remaining concerns are:

1) I dont like the fact that we are using ids since they can collide. Maybe instead let U=<any 23 bit number>. This number should be set as fg color in the unicode cells. When kitty sees these cells it checks the fg color, if bit 24 is not set, it maps the 23-bit fg to a unique 23 bit id and sets the fg to that id with the 24th bit also set. And stores the mapping so that future cells can re-use it.

Any future graphics commands that dont create a new image with U=that number will refer to the image with the mapped id.

If a U number is re-used in a command creating a new image, it will now refer to the latest image just like image numbers work currently.

When deleting an image we remove the stored mapping. Although I guess for placeholder based images delete is not really well defined. So maybe instead just prune entries that point to non existent ids from the mapping when creating a new mapping.

This means we have to use RGB colors not 256 colors but I dont think that's a problem. Nowadays, most things support RGB colors.

Or maybe this is too much bother and we can just live with the occasional collision. I am not totally convinced either way.

2) The icat part of this PR should be separated as in a day or two I will merge the icat branch which is a re-implementation of icat in Go. You can either port your changes to that or if you are not comfortable in Go I will do it for you after merging the non-icat parts of this PR.

3) There need to be some tests for how kitty handles unicode placeholder images in kitty_tests/graphics.py

trygveaa commented 1 year ago

This means we have to use RGB colors not 256 colors but I dont think that's a problem. Nowadays, most things support RGB colors.

This would absolutely be a problem for me :/ I would like to use it in WeeChat, and that only supports 256 colors unfortunately.

kovidgoyal commented 1 year ago

On Thu, Jan 05, 2023 at 05:30:49AM -0800, Trygve Aaberge wrote:

This means we have to use RGB colors not 256 colors but I dont think that's a problem. Nowadays, most things support RGB colors.

This would absolutely be a problem for me :/ I would like to use it in WeeChat, and that only supports 256 colors unfortunately.

The end application doesnt need to generate the colors, only tmux/whatever else is in the middle needs to pass them through.

Presumably you will be writing some kind of script/plugin/extension for weechat to generate these codes, since I dont think weechat can generate them natively. That script can simply set the foreground color using an RGB code and then restore it once it is done transmitting the graphics code.

kovidgoyal commented 1 year ago

Actually it doesnt matter you can use 256 colors as well, it just means your chances of collision with a previously used U number are higher, but that is true when using ids as well, and the effect of collision is less severe with U numbers. You will need to restrict to only those colors from the 256 color set that dont set the 24th bit however, so you should have around 128 unique ids.

trygveaa commented 1 year ago

The end application doesnt need to generate the colors, only tmux/whatever else is in the middle needs to pass them through. Presumably you will be writing some kind of script/plugin/extension for weechat to generate these codes, since I dont think weechat can generate them natively. That script can simply set the foreground color using an RGB code and then restore it once it is done transmitting the graphics code.

I need WeeChat to handle displaying the unicode characters, otherwise they won't be placed correctly when scrolling, changing buffers etc. Or am I misunderstanding something?

My plan was to call icat from my WeeChat script, capture the output characters, and print them with the WeeChat API. I see that does not work anymore though, since icat seem to print it directly to the terminal now, so I can't capture the output (unlike tupimage which I tested earlier, with which it worked to do it like that).

Actually it doesnt matter you can use 256 colors as well, it just means your chances of collision with a previously used U number are higher, but that is true when using ids as well, and the effect of collision is less severe with U numbers. You will need to restrict to only those colors from the 256 color set that dont set the 24th bit however, so you should have around 128 unique ids.

Hm, 128 ids is even more limiting than 256 though. I plan to display many small images (custom emojis from Slack, to be exact), so I would rather manage the 256 ids myself than be limited to 128.

kovidgoyal commented 1 year ago

On Thu, Jan 05, 2023 at 10:48:29AM -0800, Trygve Aaberge wrote:

The end application doesnt need to generate the colors, only tmux/whatever else is in the middle needs to pass them through. Presumably you will be writing some kind of script/plugin/extension for weechat to generate these codes, since I dont think weechat can generate them natively. That script can simply set the foreground color using an RGB code and then restore it once it is done transmitting the graphics code.

I need WeeChat to handle displaying the unicode characters, otherwise they won't be placed correctly when scrolling, changing buffers etc. Or am I misunderstanding something?

My plan was to call icat from my WeeChat script, capture the output characters, and print them with the WeeChat API. I see that does not work anymore though, since icat seem to print it directly to the terminal now, so I can't capture the output (unlike tupimage which I tested earlier, with which it worked to do it like that).

You can still do that with icat, though you would have to create a temporary pty pair for it.

Actually it doesnt matter you can use 256 colors as well, it just means your chances of collision with a previously used U number are higher, but that is true when using ids as well, and the effect of collision is less severe with U numbers. You will need to restrict to only those colors from the 256 color set that dont set the 24th bit however, so you should have around 128 unique ids.

Hm, 128 ids is even more limiting than 256 though. I plan to display many small images (custom emojis from Slack, to be exact), so I would rather manage the 256 ids myself than be limited to 128.

If you are sending emojis they are fire and forget so it doesnt matter if the ids conflict.

kovidgoyal commented 1 year ago

In fact for your use case, numbers are strictly better than ids. Since with an id conflict the previous image gets changed. With a number conflict all it means is you can no longer make changes to the previous image.

sergei-grechanik commented 1 year ago

Maybe instead let U=<any 23 bit number>. This number should be set as fg color in the unicode cells. When kitty sees these cells it checks the fg color, if bit 24 is not set, it maps the 23-bit fg to a unique 23 bit id and sets the fg to that id with the 24th bit also set.

Not sure I understand this part. If kitty changes the fg of a placeholder to the actual image id, it will work only for that specific placeholder, and if the user switches windows in tmux or scrolls the display, the placeholder will be rerendered with the old 23-bit id, because tmux wouldn't be aware of the fg change.

In general collisions are unavoidable, for example the user can store the placeholder from icat in a file, then cat it in a year and get a different image in the placeholder. I see the following options we can try to minimize the probability of collisions further:

Also it's possible to minimize the damage from collisions. When we have a collision, we see the wrong image in the wrong place, but we may not know that it is the wrong image. But, for example, if we randomly shuffle the row numbers for each image, the wrong image will look obviously incorrect with a high probability.

kovidgoyal commented 1 year ago

On Thu, Jan 05, 2023 at 09:50:24PM -0800, Sergei Grechanik wrote:

Maybe instead let U=<any 23 bit number>. This number should be set as fg color in the unicode cells. When kitty sees these cells it checks the fg color, if bit 24 is not set, it maps the 23-bit fg to a unique 23 bit id and sets the fg to that id with the 24th bit also set.

Not sure I understand this part. If kitty changes the fg of a placeholder to the actual image id, it will work only for that specific placeholder, and if the user switches windows in tmux or scrolls the display, the placeholder will be rerendered with the old 23-bit id, because tmux wouldn't be aware of the fg change.

Ah yes, that's true. I guess that makes my idea unworkable.

In general collisions are unavoidable, for example the user can store the placeholder from icat in a file, then cat it in a year and get a different image in the placeholder. I see the following options we can try to minimize the probability of collisions further:

Yes, but we want to minimize the chance of collisions in the common use case of applications outputting images during their normal operations.

  • Expand the size of the id:
    • We can split the underline color so that some of its bits are used for image id (we'll have less bits for the placement id, but it's less important). Not every middle-man application supports underline colors though, even tmux needs some weird line in the config to enable them.
    • We can use a range of placeholder characters instead of a single placeholder character. I think getting 8 additional bits this way is reasonable. Higher risk of character collision though.

Not a fan of either of these, as you say underline color is a lot less supported and I really dont want to make more PUA characters unuseable for other things than we absolutely have to. Another option is to use a third combining character since kitty now supports three, that will easily give us an additional 8 bits. So we can use 32 bits the combining char the lower 8 and the foreground the upper 24. This will also work in the case of applications that support only 256 colors to give us 16 bits.

  • Ask the terminal to allocate the id. The problem is that we need to wait for the response from the terminal, which may be slow over ssh, and may also be directed to the wrong tmux pane if the user switches panes at the right moment.

No, I dont think requiring a roundtrip is workable. tmux wont pass back the responses anyway, and other applications may filter them out as well.

  • Store the id <-> image mapping locally. This is what I'm doing with tupimage. The problem is that you are out of luck if you use multiple image-uploading tools that don't know about each other. And also sshing from within tmux is also problematic.

You mean each tool maintains a list of ids it has used in the past? This will require persistent state and wont work over ssh, also not worth it IMO.

Also it's possible to minimize the damage from collisions. When we have a collision, we see the wrong image in the wrong place, but we may not know that it is the wrong image. But, for example, if we randomly shuffle the row numbers for each image, the wrong image will look obviously incorrect with a high probability.

I dont quite follow what you are suggesting here, can you elaborate.

sergei-grechanik commented 1 year ago

Another option is to use a third combining character since kitty now supports three, that will easily give us an additional 8 bits.

Not every application supports three combining characters, but it looks like tmux does, so it may be a good idea. @trygveaa will it work for WeeChat? I'm also a bit worried about the total size of the placeholder, but if it's ever a problem, we can use the same trick as with row numbers and specify the third combining character only for the first column.

tmux wont pass back the responses anyway, and other applications may filter them out as well.

Yes, it's a problem for fzf, for example.

You mean each tool maintains a list of ids it has used in the past? This will require persistent state and wont work over ssh, also not worth it IMO.

In my opinion it's actually quite a good solution. I don't insist on having it in icat though. The problem with ssh is only when we have multiple ssh sessions attached to the same terminal (and even then I see some possible solutions, like assigning non-overlapping id ranges).

I dont quite follow what you are suggesting here, can you elaborate.

We can specify a random permutation of rows when we create an image placement. Then the placeholder has to use the same permutation, otherwise the image will appear broken, something like this: image Probably not worth it unless we really expect collisions to happen.

Ok, I think I'll try the third combining char approach.

kovidgoyal commented 1 year ago

On Sat, Jan 07, 2023 at 09:51:57PM -0800, Sergei Grechanik wrote:

Another option is to use a third combining character since kitty now supports three, that will easily give us an additional 8 bits.

Not every application supports three combining characters, but it looks like tmux does, so it may be a good idea. @trygveaa will it work for WeeChat? I'm also a bit worried about the total size of the placeholder, but if it's ever a problem, we can use the same trick as with row numbers and specify the third combining character only for the first column.

It's 10 bytes per cell with the third char being 2 of those bytes. For a giant image of 160x50 cells thats 78KB with the third char and 62KB without. Assuming a cell has an area of 200 pixels, the image data would be something on the order of 4-5MB as a comparison. Of course, the image data only has to be transmitted once and can use a side channel.

Most use cases for this will involve much smaller images. So I think the overhead is acceptable, and yes one could use only the first column, at a cost in robustness, which will make the overhead truly negligible.

In my opinion it's actually quite a good solution. I don't insist on having it in icat though. The problem with ssh is only when we have multiple ssh sessions attached to the same terminal (and even then I see some possible solutions, like assigning non-overlapping id ranges).

I mean one could run icat on the local machine in a session, then ssh into another machine and run icat then logout, and ssh into a third machine and run icat and so on... From the terminals perspective all the ids used in all those invocations are in the same namespace. And then one would have to manage the issue of stale ids. Seems cleaner to just rely on the low likelihood of collision with a random number in 32 bits.

I dont quite follow what you are suggesting here, can you elaborate.

We can specify a random permutation of rows when we create an image placement. Then the placeholder has to use the same permutation, otherwise the image will appear broken, something like this: image Probably not worth it unless we really expect collisions to happen.

Ah, got it. Yes I think we can table that for now. Lets see how it works in practice first.

Ok, I think I'll try the third combining char approach.

Cool.

trygveaa commented 1 year ago

You can still do that with icat, though you would have to create a temporary pty pair for it.

How can I do that? I tried with pty.spawn in python, but then kitty doesn't receive the control commands since they are just sent to the new pty, so I get "Terminal does not support reporting screen sizes via the TIOCGWINSZ ioctl".

Not every application supports three combining characters, but it looks like tmux does, so it may be a good idea. @trygveaa will it work for WeeChat?

Yes, looks like it does. E.g. when printing a\u0300\u0301\u0320 I see all three in WeeChat.

I'm also a bit worried about the total size of the placeholder, but if it's ever a problem, we can use the same trick as with row numbers and specify the third combining character only for the first column.

Would all columns need all information independently, in case they are not displayed together, or the first column is not displayed at all? E.g. if you scroll horizontally in less so the first column is out of view (it doesn't seem like displaying the image works in less though, but it's just an example).

By the way, I wondered about the placement ID. When do you need to use that when using unicode placeholders? Isn't the placements just determined by the unicode characters? I'm asking because WeeChat doesn't support underline color, but I can't see when it would be necessary.

kovidgoyal commented 1 year ago

On Sun, Jan 08, 2023 at 03:22:14AM -0800, Trygve Aaberge wrote:

You can still do that with icat, though you would have to create a temporary pty pair for it.

How can I do that? I tried with pty.spawn in python, but then kitty doesn't receive the control commands since they are just sent to the new pty, so I get "Terminal does not support reporting screen sizes via the TIOCGWINSZ ioctl".

You dont need to anymore I already changed icat to write only to stdout. Pass in --transfer-mode and it wont send anything on the tty device.

sergei-grechanik commented 1 year ago

Sorry for a long silence. So, since last time:

kovidgoyal commented 1 year ago

OK post the icat python diff and I will take care of porting it to Go.

sergei-grechanik commented 1 year ago

Here is the commit with icat changes: https://github.com/sergei-grechanik/kitty/commit/7468171c8b2f64c88fdf584300d368063e320851

kovidgoyal commented 1 year ago

I have merged and ported the icat changes. I dont use tmux so I leave testing that to those who do. Basic icat of a simple PNG file worked for me inside tmux. I didnt test beyond that.

sergei-grechanik commented 1 year ago

Thank, looks great! I'll try to test it more thoroughly tomorrow.

trygveaa commented 1 year ago

Nice! Thanks for the work on this both of you.

I tested it and it seems to work great for me. I have a couple of questions though:

If I print the unicode characters first, and send the image to the terminal afterwards, the image isn't displayed until I print the characters again (e.g. switching tmux pane, weechat buffer or calling ncurses refresh()). Is this intentional? Printing the characters first and sending the image afterwards is useful in a couple of cases.

  1. If I have closed the terminal and then open it again and re-attach to tmux, the characters will still be there, but I have to send the image again.
  2. If I know the exact size I want to display the image in I can print the characters immediately without waiting for the image to load and be sent to the terminal first. This is useful in WeeChat because I may want to print the image lines immediately, and then load the image in the background because I want the lines to appear before other lines are printed and WeeChat only supports printing new lines at the end.

As for the first point, if you print an image with icat and then close the terminal and re-open the tmux session later, there doesn't seem to be a way to restore that image without printing it again. Would it make sense to add some support for this to icat? icat probably shouldn't keep the images and re-transmit them automatically, but if icat got support for specifying the image id, and also an option to only send the image data and not print the unicode characters, it would be possible to run icat again to display the image.

I see that icat is printing CSI ? s before the unicode characters and CSI ? r after. As far as I understand this is saving all DEC Private Mode Values, and restoring them after printing the unicode characters, but I wonder what the reason for doing this is?

If anyone's interested, or want to see an alternate implementation for using this protocol apart from icat, here is a WeeChat script I wrote to display images using this protocol in WeeChat: https://github.com/trygveaa/weechat-icat

kovidgoyal commented 1 year ago

On Tue, Mar 07, 2023 at 04:00:32PM -0800, Trygve Aaberge wrote:

Nice! Thanks for the work on this both of you.

I tested it and it seems to work great for me. I have a couple of questions though:

If I print the unicode characters first, and send the image to the terminal afterwards, the image isn't displayed until I print the characters again (e.g. switching tmux pane, weechat buffer or calling ncurses refresh()). Is this intentional? Printing the characters first and sending the image afterwards is useful in a couple of cases.

I'll let sergei address these since it is his code/design.

  1. If I have closed the terminal and then open it again and re-attach to tmux, the characters will still be there, but I have to send the image again.
  2. If I know the exact size I want to display the image in I can print the characters immediately without waiting for the image to load and be sent to the terminal first. This is useful in WeeChat because I may want to print the image lines immediately, and then load the image in the background because I want the lines to appear before other lines are printed and WeeChat only supports printing new lines at the end.

As for the first point, if you print an image with icat and then close the terminal and re-open the tmux session later, there doesn't seem to be a way to restore that image without printing it again. Would it make sense to add some support for this to icat? icat probably shouldn't keep the images and re-transmit them automatically, but if icat got support for specifying the image id, and also an option to only send the image data and not print the unicode characters, it would be possible to run icat again to display the image.

This wont work as implemented currently since one cant specify the image after the placeholders. Let's see what Sergei has to say about the design first.

I see that icat is printing CSI ? s before the unicode characters and CSI ? r after. As far as I understand this is saving all DEC Private Mode Values, and restoring them after printing the unicode characters, but I wonder what the reason for doing this is?

It's not necessary (its an artifact of how it is implemented internally).

sergei-grechanik commented 1 year ago

If I print the unicode characters first, and send the image to the terminal afterwards, the image isn't displayed until I print the characters again

Yes, it's confusing. Here is the fix: https://github.com/kovidgoyal/kitty/pull/6097

Regarding reattaching tmux and then reuploading images, something needs to keep track of uploaded images, like a local database. It's actually supported by tupimage: https://github.com/sergei-grechanik/tupimage#reuploading-broken-images (tupimage is slow and outdated and badly needs complete rewriting, of course). Would be nice to have it in icat, but on the other hand it may be a bit too heavy for a tool like icat.

kovidgoyal commented 1 year ago

On Tue, Mar 07, 2023 at 08:35:09PM -0800, Sergei Grechanik wrote:

If I print the unicode characters first, and send the image to the terminal afterwards, the image isn't displayed until I print the characters again

Yes, it's confusing. Here is the fix: https://github.com/kovidgoyal/kitty/pull/6097

Regarding reattaching tmux and then reuploading images, something needs to keep track of uploaded images, like a local database. It's actually supported by tupimage: https://github.com/sergei-grechanik/tupimage#reuploading-broken-images (tupimage is slow and outdated and badly needs complete rewriting, of course). Would be nice to have it in icat, but on the other hand it may be a bit too heavy for a tool like icat.

Definitely not in icat. I want icat to remain a cat like utility without local state as far as possible. Fundamentally, the correct solution for image persistence when using a multiplexer is for the multiplexer to store the image. I am not OK adding local state to icat to workaround multiplexer limitations.

trygveaa commented 1 year ago

This wont work as implemented currently since one cant specify the image after the placeholders. Let's see what Sergei has to say about the design first.

It will work if you redraw the characters. E.g. in tmux you can do that by running attach-session.

It's not necessary (its an artifact of how it is implemented internally).

Ah, okay.

Definitely not in icat. I want icat to remain a cat like utility without local state as far as possible. Fundamentally, the correct solution for image persistence when using a multiplexer is for the multiplexer to store the image. I am not OK adding local state to icat to workaround multiplexer limitations.

I agree it probably doesn't belong in icat. What I'm suggesting is that icat allows you to specify the image id. Then you will be able to create a wrapper around icat that handles sending the image again when necessary, and redraws the characters when necessary (if #6097 isn't merged).

Specifying the image id is strictly speaking the only thing that's necessary for this, but it would be convenient if icat had an option to select that only the image data is sent, without the unicode characters, so the wrapper doesn't have to strip that out of the icat output.

trygveaa commented 1 year ago

I suppose you could argue that such a wrapper should implement the protocol itself, instead of using icat. And that is what I'm doing in the WeeChat script. It just seemed easy to me to add these options to icat to make it easier to create the wrapper.

kovidgoyal commented 1 year ago

I have added an option to specify image ids to icat.

trygveaa commented 1 year ago

I have added an option to specify image ids to icat.

Thanks, it works great!