search

alacritty / vte

Parser for virtual terminal emulators
https://docs.rs/vte/
Apache License 2.0
242 stars 56 forks source link

migrate ansi mod from alacritty_terminal #64

Closed nw0 closed 3 years ago

nw0 commented 4 years ago

This is from https://github.com/alacritty/alacritty/issues/1063.

I've naively moved everything over, and put Line, Column, Rgb in a new file, term.rs. The new modules, ansi and term, are behind a feature flag ansi, which doesn't work with no_std (depends on std::io).

As this is separate from the main crate, not filing a PR to remove from there yet.

nixpulvis commented 4 years ago

I'd love to see what this looks like integrated with Alacritty itself.

I'm also wondering if it makes sense to move more of the functionality of Rgb, for example, over here. I'm not sure I would want to wrap Rgb in another struct inside Alacritty. Serde stuff is perhaps another story...

Or do you already have a plan for this?

nixpulvis commented 4 years ago

Yea, I'm just trying to imagine how the Rgb struct should be handled between alacritty_terminal and vte. Perhaps we end up with two structs (one wrapping the other), but if there's a clean way to avoid it, that would be nice.

nw0 commented 4 years ago

I'd really like to see this work without std too, I don't think fundamentally there are a lot of things that require std just for the dispatch part.

I believe all usages of Write are for replying, e.g. like "Cursor Position Report". This could be refactored into the Handler trait. What do you think?

I'm just trying to imagine how the Rgb struct should be handled between alacritty_terminal and vte.

I think it makes sense for Rgb struct in vte, since we do have to handle color parsing. If we don't want serde here, we can consider a remote derive in alacritty_terminal?

Line, Column: it does seem a bit silly to force implementers of Handler to use these newtypes. I'll remove them if there are no objections.

chrisduerr commented 4 years ago

I believe all usages of Write are for replying, e.g. like "Cursor Position Report". This could be refactored into the Handler trait. What do you think?

That is correct, but I don't think it makes sense to have them as part of the handler trait since they are separate things. So it should probably be a separate trait.

I think it makes sense for Rgb struct in vte, since we do have to handle color parsing. If we don't want serde here, we can consider a remote derive in alacritty_terminal?

I've considered removing Rgb from vte and all associated parsing stuff, but this is really the kinda stuff why we want to move this to VTE. This isn't anything terminal specific, every terminal needs to behave the same here and if something is missing we want to benefit from contributions to vte in Alacritty. So really this should be part of vte. Because of that the only option is to have our Alacritty-specific derive in a wrapper type, which shouldn't be a problem.

Line, Column: it does seem a bit silly to force implementers of Handler to use these newtypes. I'll remove them if there are no objections.

I agree, using types for this with no methods on these types just makes things confusing. Properly naming the parameters should be sufficient and easier to handle downstream. Then people can wrap it in their types anyways.

nw0 commented 4 years ago

That is correct, but I don't think it makes sense to have them as part of the handler trait since they are separate things. So it should probably be a separate trait.

I've experimented with a trait for this. It seems that the Handler should be able to do whatever it wants with its "writer", rather than being constrained to io::Write. I've parameterised this as trait Handler<W>, with no constraints on W. Does this work?

I've removed the std dependencies, although I note that we can't use join (https://github.com/alacritty/vte/pull/64/files#diff-f20c875fb67cf4f8bc670d89d1d49db1R788) in 1.36.0 without the unstable SliceConcatExt trait (thus: updated build tests fail). From 1.38.0, join is provided by a stable SliceConcat trait, if that's at all helpful.

chrisduerr commented 4 years ago

I've experimented with a trait for this. It seems that the Handler should be able to do whatever it wants with its "writer", rather than being constrained to io::Write. I've parameterised this as trait Handler, with no constraints on W. Does this work?

With how traits work in Rust, that would probably mean that the handler wouldn't be able to do anything with its writer. If a trait is passed as parameter unconstrained without any methods, you can't do anything with it.

I've removed the std dependencies, although I note that we can't use join (https://github.com/alacritty/vte/pull/64/files#diff-f20c875fb67cf4f8bc670d89d1d49db1R788) in 1.36.0 without the unstable SliceConcatExt trait (thus: updated build tests fail). From 1.38.0, join is provided by a stable SliceConcat trait, if that's at all helpful.

I don't think 1.38.0 would help, right? Since this is still using allocations like Strings and Vecs.

nw0 commented 4 years ago

I've experimented with a trait for this. It seems that the Handler should be able to do whatever it wants with its "writer", rather than being constrained to io::Write. I've parameterised this as trait Handler, with no constraints on W. Does this work?

With how traits work in Rust, that would probably mean that the handler wouldn't be able to do anything with its writer. If a trait is passed as parameter unconstrained without any methods, you can't do anything with it.

Actually, I'm thinking of something like impl Handler<std::fs::File> for Term. Parameterising on a trait would probably result in the scenario you state, yes.

I'm getting bogged down dealing with the Rgb issue at the same time as the trait one. Do you mind if I refactor this over a few PRs? I'll also work on changes to the main crate simultaneously so we're convinced the design works.

chrisduerr commented 4 years ago

Do you mind if I refactor this over a few PRs?

I do mind, yes. I don't think it makes sense to merge this unless there's a complete package that makes sense. Just copy-pasting stuff over isn't really beneficial.

nixpulvis commented 4 years ago

I'm getting bogged down dealing with the Rgb issue at the same time as the trait one. Do you mind if I refactor this over a few PRs?

@nw0 FWIW, it's completely possible to make a new set of branches which you merge into this branch. I'm not necessarily saying you should do that, since those PRs would be on your own fork, but it may help you personally. I know I often need a branch to keep my head on straight.

nw0 commented 4 years ago

I've straightened things out and have the following design as pushed above:

The obvious questions:

As regards 1.36:

I don't think 1.38.0 would help, right? Since this is still using allocations like Strings and Vecs.

The issue is really that the trait that provides join ( https://github.com/rust-lang/rust/pull/62403) was only stabilised in 1.38.0, if one has no_std.

chrisduerr commented 4 years ago

The issue is really that the trait that provides join ( rust-lang/rust#62403) was only stabilised in 1.38.0, if one has no_std.

Updating to 1.38.0 is not a problem, but even if you update you won't be able to call join here.

nw0 commented 4 years ago

Should I use merge commits to update to the latest vte?

(I ask because I've generally worked with people who insist on rebases which does mean force-pushing)

chrisduerr commented 4 years ago

Should I use merge commits to update to the latest vte?

To update on top of VTE a rebase is usually easier, yes. That will keep the history consistent though, so I don't have to re-check everything.

chrisduerr commented 3 years ago

https://github.com/alacritty/vte/pull/75