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 support for rendering uninterpreted/"raw" messages #9

Closed
wants to merge 2 commits into from

Conversation

stuhood
Copy link

@stuhood stuhood commented Nov 2, 2021

Fixes #8.

)));
tokens.push(" ".into());
tokens.push(brush.style(color.bold()).paint(message));

let message_block_count = block_count_sans_ansi_codes(&tokens);
Copy link
Author

@stuhood stuhood Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm: it looks like unicode-width does not ignore ANSI control characters, so raw messages containing ANSI characters do not fully overdraw. See unicode-rs/unicode-width#24.

The top commit resolves this issue, albeit in a somewhat costly way.

}

let mut block_counter = BlockCounter(0);
let mut parser = vte::Parser::new();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Parser is fairly hefty, and should likely be lifted onto the State for reuse across messages.

@Byron
Copy link
Owner

Byron commented Nov 3, 2021

Thanks a lot, all this looks great!

Since VTE is needed for raw messages to calculate their size correctly, I am wondering if there could be a feature-toggle for the Raw part of the envelope. Without it, it's essentially all like it was before without another dependency.

Is this feasible?

@stuhood
Copy link
Author

stuhood commented Nov 3, 2021

Since VTE is needed for raw messages to calculate their size correctly, I am wondering if there could be a feature-toggle for the Raw part of the envelope. Without it, it's essentially all like it was before without another dependency.

Is this feasible?

Technically vte is required for any Message, since we don't validate that the message strings don't contain ANSI escapes... but I think that it is probably fine to just document that Item::message Strings must not contain any.

But yes: I agree that it would be good to avoid introducing the overhead of the vte parse wherever possible: can add a feature, and will probably also see about making the vte parse conditional on the message type.

@Byron
Copy link
Owner

Byron commented Nov 4, 2021

Technically vte is required for any Message, since we don't validate that the message strings don't contain ANSI escapes... but I think that it is probably fine to just document that Item::message Strings must not contain any.

It's indeed possible to feature-toggle the Raw variant of the Envelope which would allow everything related to it to only become available if the, say, raw-message feature is enabled. This would then pull in the vte crate. Interestingly, this would then make the vte feature toggle available as well which would allow it to be used for handling non-raw messages as well.

Before doing any of this, I hope it's possible to make make tests pass, so that the new feature toggle can be tested as well using that machinery.

@Byron
Copy link
Owner

Byron commented Sep 12, 2022

Closing due to inactivity.

@Byron Byron closed this Sep 12, 2022
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.

Ability to write raw Messages
2 participants