Image placement using Unicode placeholders #5664
Conversation
|
Cool, I will look at this in a while. To start with could you do the follwoing:
|
sergei-grechanik
force-pushed
the
pr-unicode-placeholders
branch
from
97937f1 to
0cb9b2a
Compare
|
Extracted a small fix as #5682 |
sergei-grechanik
force-pushed
the
pr-unicode-placeholders
branch
from
0cb9b2a to
79c0541
Compare
|
Hi! Do I understand it correctly that with this it will be possible to view images in tmux when it's run under kitty? |
|
Yes, it will work in tmux. The icat kitty will use this method (and some other tmux workarounds) when the |
|
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. |
|
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 |
|
Thanks! After setting 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. |
|
Comments:
I haven't reviewed the actual algorithms/code in detail yet, will get to |
|
Thanks for the review, I'll try to address all the issues over the weekend.
I can confirm that, the workaround is necessary for tmux 3.0, but not for tmux 3.4. I will remove it.
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 |
|
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.
|
|
Found a bug in cell image rendering logic, this will take some time. |
sergei-grechanik
force-pushed
the
pr-unicode-placeholders
branch
2 times, most recently
from
d47efb7 to
8a14b8e
Compare
|
Fixed the issues.
Actually, rowcolumn_diacritics_utf8 is the only thing I need. It's represented with literals like
I decided not to combine them into one option, mostly because
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 |
|
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
|
This would absolutely be a problem for me :/ I would like to use it in WeeChat, and that only supports 256 colors unfortunately. |
|
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.
|
|
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. |
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).
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. |
|
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.
|
|
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. |
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. |
|
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.
|
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, it's a problem for fzf, for example.
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).
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: Ok, I think I'll try the third combining char approach. |
|
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:
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.
|
How can I do that? I tried with
Yes, looks like it does. E.g. when printing
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. |
|
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
force-pushed
the
pr-unicode-placeholders
branch
from
b9c863f to
db2fd66
Compare
sergei-grechanik
force-pushed
the
pr-unicode-placeholders
branch
from
db2fd66 to
b0f22e4
Compare
This commit introduces the Unicode placeholder image placement method. In particular: - Virtual placements can be created by passing `U=1` in a put command. - Images with virtual placements can be displayed using the placeholder character `U+10EEEE` with diacritics indicating rows and columns. - The image ID is indicated by the foreground color of the placeholder. Additionally, the most significant byte of the ID can be specified via the third diacritic. - Underline color can be optionally used to specify the placement ID. - A bug was fixed, which caused incomplete image removal when it was overwritten by another image with the same ID.
sergei-grechanik
force-pushed
the
pr-unicode-placeholders
branch
from
b0f22e4 to
d63eead
Compare
|
Sorry for a long silence. So, since last time:
|
|
OK post the icat python diff and I will take care of porting it to Go. |
|
Here is the commit with icat changes: sergei-grechanik@7468171 |
|
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. |
|
Thank, looks great! I'll try to test it more thoroughly tomorrow. |
|
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.
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 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 |
|
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).
|
Yes, it's confusing. Here is the fix: #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. |
|
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: #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.
|
It will work if you redraw the characters. E.g. in tmux you can do that by running
Ah, okay.
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. |
|
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. |
|
I have added an option to specify image ids to icat. |
Thanks, it works great! |
This commit introduces the Unicode placeholder image placement method. In particular:
U=1in a put command.U+10EEEEwith diacritics indicating rows and columns.--unicode-placeholderflag is specified.--tmuxflag was added to the icat kitten, which enables some tmux workarounds and Unicode placeholders.