AnonymouX47 / term-image

Display images in the terminal with python
https://term-image.readthedocs.io
MIT License
206 stars 9 forks source link

feat: Allow ghostty to render kitty images #111

Closed AbeEstrada closed 1 month ago

AbeEstrada commented 2 months ago

toot-term-image

Using toot in Ghostty, I found that images were supported but not rendering correctly; one part was missing. I figured that Konsole had special features enabled/disabled, so I added Ghostty to the exceptions. After some tests, the images rendered correctly.

I know Ghostty is in beta and might change. Feel free to clean up this PR or close it if you feel so. I can patch term-image manually until Ghostty becomes public.

AnonymouX47 commented 1 month ago

Hello!

Please, forgive my late response. :pray:

Unfortunately, I'm very reluctant to merge this for the following major reasons:

  1. The need for this workaround is an indicator to the fact that ghostty's implementation of the protocol is most likely not spec-compliant (or consistent with the reference implementation) in some aspects, as this is the case with Konsole.

    That said, as far as I know, ghostty is still very young in development with minimal/restricted userbase, unlike Konsole. Hence, it would be much more advisable to report this non-compliance/inconsistency to the developers on time and get it fixed early.

    Note: Non-compliance with specs and inconsistency among implementations are the most crucial reasons why terminal emulation is in the state it is today. It would honestly be a shame (and I mean that as politely as possible) if ghostty were to start on such a foot.

  2. I currently don't have access to ghostty, making it difficult or impossible for me to test the effects of these changes (the fact is, the changes might have side effects you're unaware of and which might be difficult to find without rigorous testing).

Thank you very much. :smiley:

AbeEstrada commented 1 month ago

Totally understandable, thanks for taking the time.

I'm going to close the PR and ask Ghostty's maintainer to check it out.

AnonymouX47 commented 1 month ago

Thanks. I appreciate your understanding.

For what it's worth... it seems to me the issue is with the implementation of a=d,d=C i.e. delete all placements intersecting the current cursor position (along with the image data, if that's the only placement), for which ghostty seems to delete all images intersecting the current cursor column.

This is actually very different from the reasons I added those exceptions for Konsole. :hugs:

If I recall correctly, Kitty once had a similar bug which reported and got fixed. See https://github.com/kovidgoyal/kitty/issues/5081.

Tip: I think It'd be good to relay all of the above to the ghostty devs as-is.

There's very little information I have here, so I may be wrong (or worse, ghostty may be doing something way worse) but that's what I can infer.

Thanks once again. :smiley:

hpjansson commented 1 month ago

@AnonymouX47 I don't think that's quite it. I'm attaching a test file that prints two magenta squares, places the cursor on top of the bottommost one, and issues a=d,d=C. It seems to work correctly per the screenshot.

kitty-image-del-test.txt

kitty-image-del-test-screenshot

Ghostty main branch @ 7741463f826a7277941781630b5efdb77b4b6a7e.

AbeEstrada commented 1 month ago

@AnonymouX47 you shoud have your beta invite ready in the Ghostty Discord

AnonymouX47 commented 1 month ago

@hpjansson, thanks so much for looking into it. :pray:

I see... I guess I'll have to probe further then :thinking:, thanks to @AbeEstrada.


@AbeEstrada, thanks so much. The invite is much appreciated, I've accepted it and will give ghostty a run when I wake up (it's pretty late over here). :smiley:

By the way, is it possible any changes have been made to ghostty's implemention since your last comment? :thinking:

AbeEstrada commented 1 month ago

@AnonymouX47 there haven't been any changes to Ghostty's Kitty graphics protocol implementation

AnonymouX47 commented 1 month ago

I see, thanks.

I just couldn't resist going down the rabbit hole, maybe you shouldn't have added the link 🥲😅.

I've gotta give big kudos to @mitchellh (sorry to ping you but I just had to) and contributors, really great work done there. Aside the technicalities of the implementation, the readability and comprehensibility of the code is indeed note-worthy. Despite being my first time reading Zig, there wasn't a single part I found difficult to grasp... or maybe it's just Zig being Zig 🤔, just kidding.

From my little adventure, I believe I might've spotted a couple issues (e.g handling of image ids and numbers), though, I may be wrong or have misunderstood.

Anyways, I'm yet to actually test hands-on. Once I do, I'll gather all my findings and report to the appropriate quarters.

Thanks once again for the opportunity.

mitchellh commented 1 month ago

Thanks for the compliments @AnonymouX47! If you identify any issues please report them on the repo. Additionally, if you have reproductions of weird behavior but can't quite explain it, also feel free to report it since as long as I can reproduce it and compare to other terminals I can poke around and figure it out usually.

AnonymouX47 commented 1 month ago

You're welcome :smiley:. I sure will.