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

Palette-based coloring support and ColorUtils implementation #112

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

BadMagic100
Copy link
Contributor

@BadMagic100 BadMagic100 commented Jan 12, 2025

Adds support for palette-based coloring to allow custom colors and color types and migrates message log coloring to route via the built-in dark palette. All the message parts GetColor have been moved to a static ColorUtils so they can be used without need for a session.

A couple notable things:

  • Palettes can be partially specified; any colors not specified will use the default color specified for the palette. This is helpful for some custom palette use cases as well as prevents breaks by adding new palette colors
  • There is now a meaningful difference between "white" and "not specified" and the GetColor overloads have been updated accordingly (note that the dark palette is white by default so no functional change should be taking place here)
  • The color can be accessed directly from the message part using GetColor(Palette<T>) or from the Palette itself through the this[PaletteColor?] indexer. The change is completely backwards compatible by routing the Color property through GetColor(BuiltInPalettes.Dark).

Other thoughts and usage remarks are commented in, to be cleaned up if this moves forward

@BadMagic100 BadMagic100 marked this pull request as draft January 12, 2025 00:26
@BadMagic100
Copy link
Contributor Author

A remark on the lack of a palette-driven/generic colorutil. I thought about it, but as I went to implement it I felt it actually didn't make much sense. Let me walk though the things I was going to try in order to demonstrate this.

  1. Just make ColorUtil an instance class, each instance has a Palette<T> and all the calls return T. This doesn't work well because we need PaletteColors for the MessagePart implementations.
  2. Add overloads to GetColor with an additional Palette<T> parameter and returning T. This doesn't make much sense because ColorUtil.GetColor(stuff, palette) is not really any better than palette[ColorUtil.GetColor(stuff)].
  3. Make a separate PalettizedColorUtil<T> class, similar to the approach in 1. There are enough overloads that the boilerplate is a lot to write and maintain, not ideal
  4. Make a source generator to generate PalettizedColorUtil<T> (yes I really considered this over the boilerplate). Clearly the complexity is not really justified here.

All in all I think people are typically not using colorutil all that frequently, it's only relevant if you need the colors outside printjson which is rare. I think it's probably difficult to justify the addition, especially since the usage as-is is quite reasonable (see end of 2).

@BadMagic100 BadMagic100 marked this pull request as ready for review January 19, 2025 04:18
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.

1 participant