migrate ansi mod from alacritty_terminal #64

nw0 · 2020-07-29T13:12:22Z

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 · 2020-07-29T17:56:05Z

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?

chrisduerr

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.

Features like serde deserialization probably shouldn't be provided at all. I don't see how we can offer reasonable deserialization for the types of this crate. That said, types like Line and Column are probably better off as just raw numbers anyways. Since the complexity of handling another type here doesn't really have much of a benefit.

nixpulvis · 2020-07-29T21:51:54Z

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 · 2020-07-30T11:24:52Z

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 · 2020-07-30T17:54:27Z

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 · 2020-07-31T08:23:23Z

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 · 2020-07-31T08:41:44Z

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 · 2020-07-31T11:54:50Z

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 · 2020-07-31T11:56:49Z

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 · 2020-07-31T17:57:49Z

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 · 2020-08-01T09:12:45Z

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

Parameterise Handler<W>. This means impl<...> Handler<W> for Term<T, W> in the main crate -- i.e. additional parameter for Term et al. I'd very much like to hear about better designs, though.
Removed serde, replace with remote derives in main crate. I had trouble with the CursorStyle remote deserialisation, but it doesn't seem like an issue with this design.
Line, Column typedef'ed to usize.
I do have a working (passes tests on x86-64 Linux) implementation of the main crate as a proof-of-concept.

The obvious questions:

Structure and documentation: hidden behind a feature flag? etc.
Remove log?

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 ( rust-lang/rust#62403) was only stabilised in 1.38.0, if one has no_std.

chrisduerr · 2020-08-01T09:54:35Z

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.

src/ansi.rs

I'm not sure this is desirable. Will it cause dynamic dispatch here?

chrisduerr

This needs to be updated to the latest version of Alacritty's ansi.rs and vte.

Also please do not force-push this PR if possible; it's difficult to review changes if the entire file is just rewritten.

chrisduerr · 2020-08-15T23:06:18Z

src/ansi.rs

@@ -0,0 +1,1625 @@
+//! ANSI Terminal Stream Parsing.


The name of this file, this documentation and the feature isn't really great. That just describes all of this library basically.

Yes, I'm not sure how you want to deal with this. To me, the obvious questions are:

Should this be hidden behind a feature flag? (I think so)

Should a module be exposed, or just the Processor, Handler, etc.? (I'm not sure. We aren't adding that much new functionality, are we?)

I've put in a slightly more descriptive docstring for now, but I think answering these sooner would be helpful.

chrisduerr · 2020-08-15T23:08:03Z

src/ansi.rs

+type Line = usize;
+type Column = usize;


What are these type aliases for?

chrisduerr · 2020-08-15T23:14:12Z

src/ansi.rs

+    #[cfg(feature = "no_std")]
+    let colors = str::from_utf8(color).ok()?.split('/').collect::<ArrayVec<[_; 4]>>();
+    #[cfg(not(feature = "no_std"))]
+    let colors = str::from_utf8(color).ok()?.split('/').collect::<Vec<_>>();


Seems like this function can be easily rewritten to not require any of this Vec/ArrayVec?

chrisduerr · 2020-08-15T23:19:00Z

src/ansi.rs

+                if params.len() >= 2 {
+                    #[cfg(feature = "no_std")]
+                    {
+                        let mut title = ArrayString::<[_; crate::MAX_OSC_RAW]>::new();


I'm not familiar with ArrayString, but we have ascii as input while ArrayString most likely handles unicode. That means that the size of MAX_OSC_RAW would not be enough to store the title.

Though I see no harm in just passing params downstream and letting them handle this. Then they can decide what they want to use, no need to try and convert this into a string for them when it's trivial to do for them, but difficult to do for us.

Not sure how to structure this yet. Will iterate.

nw0 · 2020-08-22T04:28:15Z

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 · 2020-08-22T06:08:24Z

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 · 2021-06-29T23:47:12Z

#75

chrisduerr requested changes Jul 29, 2020

View reviewed changes

nw0 marked this pull request as draft July 31, 2020 05:52

nw0 marked this pull request as ready for review July 31, 2020 08:26

nw0 marked this pull request as draft July 31, 2020 11:59

ansi: partial move

60faf65

nw0 added 6 commits August 1, 2020 14:52

Handler: parameterise

d5b2a65

ansi: move remainder

696a30b

remove serde

7097c6e

remove now-useless conversions

f72fba4

remove std

7cb2c92

update build, test for ansi feature

033ac48

nw0 force-pushed the move-ansi branch from 3f9e2a5 to 033ac48 Compare August 1, 2020 08:44

chrisduerr requested changes Aug 1, 2020

View reviewed changes

src/ansi.rs Outdated Show resolved Hide resolved

nw0 added 4 commits August 1, 2020 20:38

parse_rgb_color: remove trivial use of Vec

baf7534

remove String dependency

bf242a0

attrs_from_sgr_parameters: allow returning Vec or ArrayVec

aad7542

I'm not sure this is desirable. Will it cause dynamic dispatch here?

restore forgotten trim

2343776

nw0 marked this pull request as ready for review August 6, 2020 13:41

nw0 requested a review from chrisduerr August 15, 2020 06:18

chrisduerr requested changes Aug 15, 2020

View reviewed changes

nw0 added 4 commits August 21, 2020 20:04

remove Line, Column type aliases

e272685

ansi: remove trivial arrayvec usage

76b626a

ansi: remove ArrayString, pass raw params to handler

84d3670

ansi: initial update to docstring

944d33b

Not sure how to structure this yet. Will iterate.

Merge remote-tracking branch 'upstream/master' into ansi2

9b89ee9

nw0 marked this pull request as draft February 1, 2021 13:30

alesharik mentioned this pull request Jun 29, 2021

Migrate ansi mod from alacritty_terminal #75

Closed

chrisduerr closed this Jun 29, 2021

Andy-Python-Programmer mentioned this pull request Apr 23, 2023

migrate ansi from alacritty_terminal #85

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate ansi mod from alacritty_terminal #64

migrate ansi mod from alacritty_terminal #64

nw0 commented Jul 29, 2020

nixpulvis commented Jul 29, 2020 •

edited

Loading

chrisduerr left a comment

nixpulvis commented Jul 29, 2020

nw0 commented Jul 30, 2020

chrisduerr commented Jul 30, 2020

nw0 commented Jul 31, 2020 •

edited

Loading

chrisduerr commented Jul 31, 2020

nw0 commented Jul 31, 2020

chrisduerr commented Jul 31, 2020

nixpulvis commented Jul 31, 2020

nw0 commented Aug 1, 2020 •

edited

Loading

chrisduerr commented Aug 1, 2020

chrisduerr left a comment

chrisduerr Aug 15, 2020

nw0 Aug 22, 2020

chrisduerr Aug 15, 2020

chrisduerr Aug 15, 2020

chrisduerr Aug 15, 2020

nw0 commented Aug 22, 2020

chrisduerr commented Aug 22, 2020

chrisduerr commented Jun 29, 2021

migrate ansi mod from alacritty_terminal #64

migrate ansi mod from alacritty_terminal #64

Conversation

nw0 commented Jul 29, 2020

nixpulvis commented Jul 29, 2020 • edited Loading

chrisduerr left a comment

Choose a reason for hiding this comment

nixpulvis commented Jul 29, 2020

nw0 commented Jul 30, 2020

chrisduerr commented Jul 30, 2020

nw0 commented Jul 31, 2020 • edited Loading

chrisduerr commented Jul 31, 2020

nw0 commented Jul 31, 2020

chrisduerr commented Jul 31, 2020

nixpulvis commented Jul 31, 2020

nw0 commented Aug 1, 2020 • edited Loading

chrisduerr commented Aug 1, 2020

chrisduerr left a comment

Choose a reason for hiding this comment

chrisduerr Aug 15, 2020

Choose a reason for hiding this comment

nw0 Aug 22, 2020

Choose a reason for hiding this comment

chrisduerr Aug 15, 2020

Choose a reason for hiding this comment

chrisduerr Aug 15, 2020

Choose a reason for hiding this comment

chrisduerr Aug 15, 2020

Choose a reason for hiding this comment

nw0 commented Aug 22, 2020

chrisduerr commented Aug 22, 2020

chrisduerr commented Jun 29, 2021

nixpulvis commented Jul 29, 2020 •

edited

Loading

nw0 commented Jul 31, 2020 •

edited

Loading

nw0 commented Aug 1, 2020 •

edited

Loading