alacritty / vte

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

migrate `ansi` from `alacritty_terminal` #85

Closed Andy-Python-Programmer closed 1 year ago

Andy-Python-Programmer commented 1 year ago

This pull request is a continuation of https://github.com/alacritty/vte/pull/64 rather than https://github.com/alacritty/vte/pull/75 (which additionally added no-alloc support). If required, no-alloc support can be added in a follow up pull request.

Unresolved Questions

chrisduerr commented 1 year ago

Should the log crate be used? It should be fine to ignore stuff that is unhanded.

Yes. Not logging is not an option.

Andy-Python-Programmer commented 1 year ago

Also should the sync updates be excluded if its no_std since Instant will not be available? Or should the timer functionality be provided by TimeProvider as with the previous pull request.

chrisduerr commented 1 year ago

Also should the sync updates be excluded if its no_std since Instant will not be available? Or should the timer functionality be provided by TimeProvider as with the previous pull request.

Not providing certain features isn't really an option (imo).

Andy-Python-Programmer commented 1 year ago

Another way we could approach sync updates will be by adding a update_timeout(&self, _: Duration) function on Handler, which will make it much more portable since now we do not have to handle all of the Instant and SyncHandler mess; making it less complex. Which method would you prefer to have?

chrisduerr commented 1 year ago

Another way we could approach sync updates will be by adding a updatetimeout(&self, : Duration) function on Handler, which will make it much more portable since now we do not have to handle all of the Instant and SyncHandler mess; making it less complex.

I'm not sure what you mean? The whole point of an abstraction would be that we handle all that "mess" for implementations. People shouldn't have to know how sync updates work to support them.

Andy-Python-Programmer commented 1 year ago

Then is the current implementation alright?

Andy-Python-Programmer commented 1 year ago

The patch against alacritty should be opened to help handling the design, since it's the main consumer of such code.

https://github.com/alacritty/alacritty/pull/6896

Andy-Python-Programmer commented 1 year ago

I believe I have addressed all of the requested changes in the reviews.

Andy-Python-Programmer commented 1 year ago

Hello, @chrisduerr. I have resolved the clippy errors in the CI. However, the CI is currently failing due to the unavailability of the dep: prefix in 1-56-0. To resolve this we could: either upgrade the minimum supported Rust version (MSRV) or rename the serde feature to a different name to avoid conflicts with the dependency.

Based on your earlier suggestion to rename the ansi-serde feature to serde, I presume that upgrading the MSRV would be the appropriate solution.

Andy-Python-Programmer commented 1 year ago

Updated the CI Rust version to 1.62.1 and now CI is green :)

Andy-Python-Programmer commented 1 year ago

:^) np