Skip to content
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

feat: Allow ghostty to render kitty images #111

Closed
wants to merge 1 commit into from

Conversation

AbeEstrada
Copy link

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
Copy link
Owner

Hello!

Please, forgive my late response. 🙏

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. 😃

@AnonymouX47 AnonymouX47 force-pushed the main branch 2 times, most recently from 130d999 to 8105a8e Compare June 26, 2024 02:47
@AbeEstrada
Copy link
Author

Totally understandable, thanks for taking the time.

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

@AbeEstrada AbeEstrada closed this Jun 26, 2024
@AnonymouX47
Copy link
Owner

AnonymouX47 commented Jun 26, 2024

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. 🤗

If I recall correctly, Kitty once had a similar bug which reported and got fixed. See kovidgoyal/kitty#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. 😃

@hpjansson
Copy link

hpjansson commented Jul 3, 2024

@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
Copy link
Author

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

@AnonymouX47
Copy link
Owner

@hpjansson, thanks so much for looking into it. 🙏

I see... I guess I'll have to probe further then 🤔, 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). 😃

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

@AbeEstrada
Copy link
Author

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

@AnonymouX47
Copy link
Owner

AnonymouX47 commented Jul 4, 2024

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
Copy link

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
Copy link
Owner

You're welcome 😃. I sure will.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants