-
Notifications
You must be signed in to change notification settings - Fork 1.2k
libvt C Api expose paste encoding #9240
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! A couple points.
| * | ||
| * @ingroup paste | ||
| */ | ||
| GhosttyResult ghostty_paste_encoder_new(const GhosttyAllocator* allocator, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main design thing I ran into here is whether we want an object like this or if we just want to do a single flat ghostty_paste_encode that takes an option structure. For ABI compatibility, the option structure may want to be a pointer with opaque setopt-style functions but then I struggle with whether that complexity should just back into what you did here anyways...
That's my main hesitation on this API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When taking an option struct directly, we would either need to make it C ABI compatible or do it with opaque pointers like in this implementation. The problem with the opaque pointers is that we have to allocate. When we have just an extern struct for the options, we could let users of the api decide if they want to alloc the options or use a local reference. Complexity wise, I dont think that the current way is really that complicated. It also sounds logical that you create a PasteEnocder and set the bracketed mode on the encoder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to be really complicated we could copy Vulkan and use essentially type-erased, stack-allocated linked lists for extensions, but that is honestly quite cursed and over the top for what we're doing here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the reason I'm even sweating these details is because the patterns we pick here are likely to extend to what we do with other APIs. Does it really matter for the paste encoding API? No. But for being consistent with the terminal API? Probably. But we can always break ABI for now because we aren't even released.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to compare a little bit:
setopt pros:
- smaller API surface
setopt cons:
- weaker typing(can that even be considered typed?)
- user has to first look at the docs to find out what type to use per opt.
setters pros:
- Type safe
- People are used to setters
- More easy to document what each opt does
setters cons:
- A lot of ghostty_* functions.
Do you guys have something to add?
|
Note this is just blocked on some test changes. I'm fine with the API itself. |
|
Everything should be correct now. One thing I found quite weird is that in the build.zig: // Normal tests always test our libghostty modules
//test_step.dependOn(test_lib_vt_step);Line 297 in 9dc2e59
why is this commented out when it says that normal tests should always test the libghostty modules? |
6625c5c to
fb044f0
Compare
As the title says, I exported the paste encoding function using the same pattern as with the key encoder. The GhosttyPasteEncoder is just a wrapper around the paste.Options and the allocator to keep track of.
Here is an example how it can be used:
(Mitchellh said in the discord that he would be interested in some help and wants small PRs so I didnt open an issue just for this).