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: support ANSI themes #460

Merged
merged 4 commits into from
Jan 1, 2024
Merged

feat: support ANSI themes #460

merged 4 commits into from
Jan 1, 2024

Conversation

smores56
Copy link
Contributor

@smores56 smores56 commented Dec 28, 2023

The current default theme is base16-ocean.dark, which as you can see below doesn't match the terminal colors by default:

Screenshot from 2023-12-28 00-28-14

If I load an 8-bit theme like this one from this neo-ansi repo, I currently see all black theming:

Screenshot from 2023-12-28 00-30-21

This is because ANSI is configured in .tmTheme files as #RR000000, a.k.a. the 8-bit color is in the red component and all other components, especially the Alpha, are 0x00. Assuming colors are 8-bit if the alpha is zero is suggested by syntect's author in this comment, and is implemented in bat already here. Given these precedents, I went ahead and implemented it, as seen below:

Screenshot from 2023-12-28 00-28-26

This implementation uses crossterm to write the escape sequences, which has a few benefits:

  • We are already importing crossterm, so there are no new dependencies
  • We have fewer magic constants, as crossterm provides them for us
  • We make fewer string allocations, as we write directly to our output string this way instead of writing each line to a new string and then re-allocatiing

I think that ANSI themes are a better default for these kind of color-specific tools (since they always match the colors of the terminal), but there are no ANSI themes packed by default into syntect's ThemeSet::load_defaults that we use. Maybe a future PR could change to this ANSI theme from bat.

@sxyazi
Copy link
Owner

sxyazi commented Dec 28, 2023

Awesome, this is the feature I've been wanting to add all along. Thank you for implementing it! I'll review it ASAP.

@sxyazi
Copy link
Owner

sxyazi commented Dec 28, 2023

Looks good to me! Would you like to change the default to an ANSI theme in this PR as well?

I think it's quite reasonable as it not only makes Yazi more compatible with users' terminals but also potentially resolves #295.

@smores56
Copy link
Contributor Author

Yeah, I totally agree. Just didn't want to do it without the author's permission. I'm not exactly sure where the ansi.tmTheme should go, my current location is yazi-config/preset/ansi.tmTheme, but yazi-plugin/preset/ansi.tmTheme also makes sense. Let me know if you have a preference.

@smores56
Copy link
Contributor Author

I used the neo-ansi theme and not bat's ANSI theme since the bat theme was mostly black.

Screenshot from 2023-12-28 12-35-33

@smores56
Copy link
Contributor Author

I'm happy if you're happy, feel free to squash and merge whenever! Thanks for the awesome file manager!

@sxyazi
Copy link
Owner

sxyazi commented Dec 29, 2023

Thank you! Let me make some little changes.

@sxyazi
Copy link
Owner

sxyazi commented Dec 29, 2023

Hi, I've made some changes:

  • Add support for bold, italic, and underline
  • Remove unnecessary ANSI sequence conversions - highlighting results are converted to ANSI sequences and then re-parsed into TUI components. Now it's just one step - from highlighted results to TUI components. This can save a lot of memory overhead and ANSI sequence parsing costs.
  • Switch to the default theme of bat and port it to another separate crate yazi-prebuild. I've copied' bat's code for parsing colors and the issue with black text disappears.

Would you like to give it a review?

@sxyazi sxyazi merged commit 10a78b5 into sxyazi:main Jan 1, 2024
3 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants