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

feat: handle colored output #296

Closed

Conversation

DrunkenToast
Copy link
Contributor

Fix #272

Don't write a lot of Rust, so all feedback obviously welcome!

image

Colored output works, however with the caveat of chunking not working.
I am not sure how to approach chunking as it would be likely that I cut through ANSI escapes codes.
Keeping track of positions for where to slice seems messy.

Another expected issue is that the reset code clears out the codeblock format colors, not sure if we wanna resolve that too.
I was thinking that the rendering could intercept clear codes and apply the chosen colors but I think that is out of scope for this PR.

@DrunkenToast DrunkenToast force-pushed the f/handle-colored-output branch from a37f3aa to 4fe547a Compare July 18, 2024 21:16
operations.push(self.render_line(chunk.collect(), block_length));
operations.push(RenderOperation::RenderLineBreak);
}
// TODO: I am not sure how to approach this properly as I would be cutting through
Copy link
Owner

Choose a reason for hiding this comment

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

This is a good point I missed and will make this change a bit different I think. I'd suggest using this crate instead https://docs.rs/ansi-parser/latest/ansi_parser/ and parse each line with it and convert it into something that preserves the styles. For example, this could probably convert this into TextBlocks which you can then turn into render operations when you want to display them. Then, when you want turn that into RenderOperations here, you would take the dimensions.columns as the max chunk size and if any line is bigger than you need to chunk it.

If this is more rust than you're comfortable (and dealing with the codebase, I'm actually not 100% sure this would work without changing anything about rendering strings) with, let me know and I get have a look. I'm starting to carve out a new release and I'd like this to be the last thing going in unless some issue comes up but I won't have time to work on this/the release until next week so no rush.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, was a bummer! I didn't know it got chunked. I'll see what I can do, I have a few ideas for something that can maybe work but no promises as I have quite a few other things to do as well.
But seems like a fun challenge if it doesn't get too difficult.

@DrunkenToast
Copy link
Contributor Author

DrunkenToast commented Jul 20, 2024

Created a naive POC which already works really well and is promising.
I don't think it's very performant however, it's also a bit of an ugly trick with ansi codes right now, it would be better to only keep the resulting style.
Wrapping:
image
image
Color reset:
image

@DrunkenToast DrunkenToast force-pushed the f/handle-colored-output branch from 2e311ce to 9187030 Compare July 20, 2024 13:12
@mfontanini
Copy link
Owner

This is great! Sorry I haven't had time to look into this, I found a bunch of bugs and have been prioritizing that. The only thing I'll add is the logic for splitting text and fit it into the screen is there already but you need to turn text into WeightedTextBlock and render it via BlockLineText::Weighted instead of BlockLineText::Preformatted but that may create other issues.

@mfontanini
Copy link
Owner

Hey @DrunkenToast, I just wanted to know I started looking into this. I had to change a few internal things to make this work as expected, but I have a somewhat working version based on yours that parses lines only once after reading them rather than every time they're rendered. Thanks for setting the base for this change!

@DrunkenToast
Copy link
Contributor Author

Hey @DrunkenToast, I just wanted to know I started looking into this. I had to change a few internal things to make this work as expected, but I have a somewhat working version based on yours that parses lines only once after reading them rather than every time they're rendered. Thanks for setting the base for this change!

Awesome! Thanks for the hard work ❤️

mfontanini added a commit that referenced this pull request Jul 31, 2024
This processes execution output's ansi escape codes and turns them into
`WeightedTexBlock`. This can then be rendered so that we:
* Get colored output automatically.
* Handle lines that are too long to fit on the screen automatically.

This needs more testing, I just tested a couple of commands and it works
fine but I didn't thoroughly test yet.

This is based on the initial work done on #296.

cc @DrunkenToast @calebdw I would appreciate if you could give this PR a
test. It's only handling a very tiny subset of escape codes so I
wouldn't be surprised if something failed. At least I tried basic things
like `ls --color=always` and piping stuff to `lolcat` and they both seem
to work fine to me.
@DrunkenToast
Copy link
Contributor Author

DrunkenToast commented Jul 31, 2024

closed in favor of #316

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.

Color output breaks line length
2 participants