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

Add bracketed paste parsing #693

Merged
merged 2 commits into from Aug 10, 2022

Conversation

groves
Copy link
Contributor

@groves groves commented Jul 28, 2022

This is another attempt at adding bracketed paste parsing, like #557. This version keeps from removing the Copy trait on Event and does all of the paste parsing in parse_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 has Copy. Instead I've added an input module with its own read function. That read returns an Input enum that either contains Event or the pasted data. What I was thinking is that you'd use the input module if you wanted to parse terminal input that included variable length data, or you could keep using the event 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.

src/input.rs Outdated Show resolved Hide resolved
src/event/sys/unix/parse.rs Show resolved Hide resolved
src/input.rs Outdated
use crate::{csi, Command, Result};
use crate::event::{Event, InternalEvent, read_internal};

pub fn read() -> Result<Input> {
Copy link
Member

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?

src/input.rs Outdated Show resolved Hide resolved
@TimonPost
Copy link
Member

TimonPost commented Jul 29, 2022

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.

@TimonPost
Copy link
Member

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.

@groves
Copy link
Contributor Author

groves commented Jul 29, 2022

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.

Same here. I'm only introducing this weird structure to keep API compatibility.

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.

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!

@sigmaSd
Copy link
Contributor

sigmaSd commented Jul 29, 2022

I also vote for just breaking the api because:

  • crossterm is still not 1.0, I think we should use this opportunity to keep iterating on the api
  • the api introduced here would make crossterm very confusing to new users (should I use read from event module or input module , what is the difference)

@TimonPost
Copy link
Member

TimonPost commented Jul 30, 2022

Indeed. It is a bit unfortunate if we have to require .clone() calls. Wonder if there is another way that doesn't duplicate the API and doesn't require the removal of Copy

@groves groves force-pushed the add-bracketed-paste branch 3 times, most recently from ca926b3 to c5bbcd8 Compare August 2, 2022 12:10
Copy link
Member

@TimonPost TimonPost left a 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

@lnicola
Copy link

lnicola commented Aug 2, 2022

#[cfg_attr(not(feature= "bracketed-paste", derive(Copy))]

Features are supposed to be additive, so enabling a feature shouldn't cause an impl to disappear.

@TimonPost
Copy link
Member

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.

@lnicola
Copy link

lnicola commented Aug 2, 2022

Do most of the crossterm users (besides Helix) copy Events that much?

@sigmaSd
Copy link
Contributor

sigmaSd commented Aug 2, 2022

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.

@groves
Copy link
Contributor Author

groves commented Aug 2, 2022

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 bracketed-paste feature flag in place with a note in the changelog that it'll be removed in a future release. Since it's on by default, that'll get other people using Copy to notice the compilation breakage. They can turn off bracketed-paste to get compilation going immediately, and then remove or rework the use of Copy at their leisure.

It's still an obnoxious change, but hopefully that makes it less obnoxious.

@TimonPost
Copy link
Member

TimonPost commented Aug 2, 2022

That sounds also good to me.

Copy link
Member

@TimonPost TimonPost left a 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

@TimonPost TimonPost merged commit 1fee5ff into crossterm-rs:master Aug 10, 2022
@TimonPost TimonPost mentioned this pull request Jan 11, 2023
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.

None yet

4 participants