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: process colored output from execution output #316

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

mfontanini
Copy link
Owner

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.

Copy link
Contributor

@DrunkenToast DrunkenToast left a comment

Choose a reason for hiding this comment

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

Overall looks really good!
I did find an edgecase that doesn't behave like my shell:
image

Shell:
image

printf "\033[31;1;4mColors should get reset but not the underline:\n"

cowsay "Should get underlined but isn't" | lolcat -f

I don't immediately see a reason in this PR that would cause the issue, so I would assume it's somewhere in the renderer.

@calebdw
Copy link
Contributor

calebdw commented Jul 30, 2024

@mfontanini,

It looks pretty good to me as far as most use-cases go! I did notice it has trouble wrapping but looks like that's related to #289

image

@DrunkenToast
Copy link
Contributor

DrunkenToast commented Jul 30, 2024

@mfontanini,

It looks pretty good to me as far as most use-cases go! I did notice it has trouble wrapping but looks like that's related to #289

image

This might actually be a bit problematic for some people. Codeblocks you can manually chunk up, but output of said codeblock is not as easy.
The advantage of the previous method was that it was chunked up beforehand with context of the maximum width.

@mfontanini
Copy link
Owner Author

Nice, thanks for testing it! Will fix both issues.

Codeblocks you can manually chunk up, but output of said codeblock is not as easy.

Don't worry, I won't merge until this is fixed. This should not look this way. Btw the changes that I'm adding here will allow simplifying a bunch of things + I think will allow fixing the other issue you created re: splitting long code snippet lines.

@mfontanini
Copy link
Owner Author

Alright, both issues are fixed:

image
image

@calebdw
Copy link
Contributor

calebdw commented Jul 31, 2024

That was fast!

@mfontanini mfontanini marked this pull request as ready for review July 31, 2024 01:25
@mfontanini mfontanini merged commit 05d0603 into master Jul 31, 2024
6 checks passed
@mfontanini mfontanini deleted the feat/colored-output branch July 31, 2024 01:25
@mfontanini
Copy link
Owner Author

Merging, if anything comes out I can re-visit. There's lots of escape codes missing here so it's possible the output of some tools won't look right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants