-
Notifications
You must be signed in to change notification settings - Fork 46
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
vaxis: add osc52 copy/paste support #28
Conversation
src/Parser.zig
Outdated
seq.param_idx += 1; | ||
switch (p) { | ||
52 => { | ||
const allocator = self.grapheme_data.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.
I think it would be best to pass an allocator to Tty.run (and Loop.run). The arg name should be like paste_allocator
or something? This makes it a bit cleaner and lets people use a different allocator for this than the grapheme_data.
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.
Actually, maybe the vaxis struct should just hold onto the allocator it was passed on init and use that in Tty.run
...what do you think?
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.
@neurocyte I was probably too late with the second comment before you ran off working on it! Sorry about that. What do you think about storing an allocator in the Vaxis struct to use for this? I think it might be cleaner that way - there are starting to be a lot of allocators passed around, but maybe that's preferred?
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.
I'm not so sure. I prefer to passing allocators explicitly to functions if at all possible. But I'm not using Loop.run or Tty.run so it's really up to you.
Thanks for the PR! There is another spot in Lines 157 to 225 in 1dc90bd
|
Thanks! I made a couple changes. The allocator for pastes is now passed as an option to the vaxis init function. This let's people opt out in a nice way if they don't need that functionality. |
I was not sure where to get an allocator for the pasted text, so I stole the grapheme allocator. Passing an allocator to parse is probably preferrable, but that felt like a large API change so I thought I'd let you decide.