osc: add parsing for Kitty desktop notifications (OSC 99) #2126
Conversation
There was a problem hiding this comment.
Thanks. One comment specific to this but see some of the changes I made to the Kitty Color Protocol parser.
Additionally, I'm starting to think the Kitty-specific structs should PROBABLY move to src/terminal/kitty/ since that's where we put everything else. Including the color protocol. I'll make that move later.
That'll make it easier to read these structs (less indentation) and easier to isolate tests for them for any logic we might have.
There are probably lots of cases where splitting things out into individual files makes sense, especially the Kitty stuff. Do you want me to split this out in this PR, or in a follow-up PR? |
Let me do it in the color one first and then you can follow suit! |
|
Okay I just merged #2113. Please take a look there to see some of the tweaks I made. I made small commits so you can see at an individual commit level for me. I'd love to see those same tweaks here because I'm likely to do the same thing otherwise. 😄 |
|
This has some conflicts with main do you mind rebasing? Sorry and thanks <3 |
jcollie
force-pushed
the
parse-kitty-desktop-notification
branch
from
89c53e9 to
f242ec2
Compare
Done. |
jcollie
force-pushed
the
parse-kitty-desktop-notification
branch
from
2048f46 to
8505b78
Compare
|
Ugh. I realized that I had read the spec wrongly, so I've redone the patch. |
jcollie
force-pushed
the
parse-kitty-desktop-notification
branch
from
8505b78 to
37f06ca
Compare
|
rebased onto current main |
|
My main blocker on merging this at the moment is that I'm still chasing down a LOT of crash reports since merging the Kitty Color Protocol PR. I received 5 yesterday (and average about 2 or 3 per day). All in the same place, all deinit of Kitty color protocol. I haven't been able to find the path that leads to it yet, but here's the stack trace:
I want to chase this down first so I can understand what's going on to see if this PR is also susceptible. |
|
I added a little bit of debugging to the OSC reset function and created a quick fuzzer for the Kitty color protocol and am unable to reproduce so far. I am wondering about the |
|
One other thought, do any of the users that send in reports of these crashes report using TUIs that make use of the Kitty color protocol? The protocol was only released in Kitty 0.36.0 (released Aug 17th) so there can't be that many TUIs that actually use it (outside of Kitty's internal apps). |
That's surely the case, but I don't see how that's possible in the current codepaths. It clearly is, so its something I want to figure out. |
|
I'm still unable to duplicate this, even using ReleaseFast. Are all the crashes macOS or are some Linux/GTK? Is there something different about the allocator on macOS or linux distros other than NixOS that causes that memory to be initialized/uninitialized in a weird way? In any case, I cleaned up some of the debugging changes I was making and put them here: https://github.com/jcollie/ghostty/tree/kcp-defensive-programming I'm not going to PR it at this point because I know that this just papers over a possibly deeper problem... |
Adds the ability to parse Kitty desktop notifications but doesn't do anything with them yet.