-
Notifications
You must be signed in to change notification settings - Fork 280
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
Add bracketed paste parsing #693
Conversation
src/input.rs
Outdated
use crate::{csi, Command, Result}; | ||
use crate::event::{Event, InternalEvent, read_internal}; | ||
|
||
pub fn read() -> Result<Input> { |
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.
In the past, we thought about why we call something an input or an event. We concluded that events are more general and include input, resize events and more. In this scenario, an input contains an event, but is the resize event an input?
I prefer to stick with one read call and one poll function and the Event rather than Input as a top-level structure that is passed to the user. Mixing input and events can be confusing for our users. |
I wonder if it is really a 'priority' to keep 'copy'. Maybe the users of this library should answer this. I will ask in the discord. |
Same here. I'm only introducing this weird structure to keep API compatibility.
At least in Helix we're passing Events on the stack all over the place. I tried doing this with Copy removed and it made for a pretty massive change in Helix. I can make that change for Helix, but I didn't want to force it on all other library users. Thanks for asking! |
I also vote for just breaking the api because:
|
Indeed. It is a bit unfortunate if we have to require |
ca926b3
to
c5bbcd8
Compare
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 we can have ´Copy´ by default:
´´´
#[cfg_attr(not(feature= "bracketed-paste", derive(Copy))]
#[derive(Debug, PartialOrd, PartialEq, Eq, Clone, Hash)]
pub enum Event { }
´´´
This way copy is always enabled unless you use the bracketed paste feature flag.
Also
´´´
#[cfg(not(feature = "event-copy"))]
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct EnableBracketedPaste;
´´´
can be
´´´
#[cfg(feature = "bracketed-paste")]
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct EnableBracketedPaste;
´´´
So think we can just remove event-copy
Features are supposed to be additive, so enabling a feature shouldn't cause an impl to disappear. |
Yes indeed, it is a bit of a draw back with this method. We have to pick: Remove copy resulting in us having to call .clone() everywere which is annoying. Dont add this feature. Or change the entire API. Or do a mix and just add it as feature flag and mis use feature flags which is not ideal but has a bit of both sides. |
Do most of the |
c5bbcd8
to
bcbacb3
Compare
I think the new cfg bracked-paste is a nice idea, those who wants to keep the old clone trait can just opt out of it. That will certainly make the breaking change easier. |
bcbacb3
to
99490ce
Compare
Sorry for the noise from all the half-complete pushes on this PR. I was getting it to run the Windows tests for me. I should be done with changes now. I was thinking we could make a release with this It's still an obnoxious change, but hopefully that makes it less obnoxious. |
That sounds also good to me. |
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.
Looks good now! Thanks
This is another attempt at adding bracketed paste parsing, like #557. This version keeps from removing the
Copy
trait onEvent
and does all of the paste parsing inparse_event
, so I'm hoping we can get it to an acceptable state.Since pasting needs to read in a variable length string, I couldn't find a way to stuff the result in
Event
since it hasCopy
. Instead I've added aninput
module with its own read function. That read returns anInput
enum that either containsEvent
or the pasted data. What I was thinking is that you'd use theinput
module if you wanted to parse terminal input that included variable length data, or you could keep using theevent
module if you only need the fixed length events. I didn't want to break all existing API clients and I wanted to let people pass around Events on the stack, and this was the best compromise I came up with.I need to document this and give it a spin in Helix. I wanted to get some feedback on the approach before polishing it up though.