refactor(colors): Rework colors structure

[thecaralice]

• Add support for ANSI 4-bit colors
• Fix -dec computation

[SenchoPens]

Good evening, thank you for your PR.

I have a few questions:

1. What was the problem in -dec computation?
2. What use cases are there for ansi colors, other than wezterm hm option?
3. Can you explain the overall motivation behind it? It seems more complex than the previous version, though I am biased :)
4. Is it backward compatible?

[thecaralice]

1. What was the problem in -dec computation?

A -dec value of 1.0 corresponds to a -rgb value of 255; prior to this change, it would instead correspond to 256. Basically, the fix is replacing / 256.0 by / 255.0,

2. What use cases are there for ansi colors, other than wezterm hm option?

WezTerm was my motivation, since I use it; another example is Alacritty which wouldn't even need toList:

programs.alacritty.settings.colors = let inherit (builtins) removeAttrs; in {
  normal = removeAttrs scheme.withHashtag.ansi.dark ["toList"];
  bright = removeAttrs scheme.withHashtag.ansi.bright ["toList"];
};

(to be honest, I only found out how that is configured while writing this comment; with that in mind, renaming ansi.dark to ansi.normal might make sense)

Actually, anywhere you see a terminal you would want to use scheme.ansi, would that be a terminal emulator, an editor with an integrated terminal (IBNLT VSCode) or, well, console.colors. Note that the old documentation about scheme.toList being good for console.colors is not quite right: the first eight base16 colors are shades of gray and as such are not a good fit for 3-bit ANSI color palette; the latter eight colors are not a good fit either, since the bright 4-bit ANSI colors do not have orange or brown, but include "bright black" and "bright white" colors.
See the difference for yourself:

base16 palette	4-bit ANSI palette

You might've noticed that I updated the doc comment for colors function: it has a link to the corresponding documentation of base24.

3. Can you explain the overall motivation behind it? It seems more complex than the previous version, though I am biased :)

I made a _color function for creating a color structure from its hex value, which allows for bringing all the color converting logic in one place and makes the structure more evident; for backwards compatibility everything is eventually toStringed, with a new original attribute being an attrset of non-flattened colors.
I have also formatted the file with nixfmt-rfc-style since I wanted to keep a uniform code style in the file, and I'm pretty used to that one; if you have an actual code style, I'm happy to use that. Some of that complexity might arise from the code style, e.g. lists with more than one element are forced to be multiline.
I have also rewritten the flattening logic: instead of several function for different -suffixes there's a single function (based) which uses a list of paths and creates a new attrset with every path-to-attr referring to path.to.attr of the source attrset (base-colors).

4. Is it backward compatible?

There are two theoretically breaking changes:

• -dec values are different from the old ones by a factor of $\frac{256}{255}$
• withHashtag only affects attributes of form baseXX-hex; previously, all attributes (including e.g. -dec-X and -hex-g) would be prefixed with a hashtag which was quite surprising for me; the only actual problem might be with the -hex-bgr attribute, which is not #-prefixed anymore; I don't know of any software using bgr format and I did not find any reference to bgr in stylix (which is the only project depending on base16.nix I'm aware of), so I have no idea whether #BBGGRR format makes sense or not.

[SenchoPens]

The changes make sense, I've added some small comments. Maybe you can replace the point-free style with plain lambdas? I'm not that familiar with the nixpkgs library and sometimes the overhaul of compositions looks unnecessary complex

[thecaralice]

The changes make sense, I've added some small comments. Maybe you can replace the point-free style with plain lambdas? I'm not that familiar with the nixpkgs library and sometimes the overhaul of compositions looks unnecessary complex

I can agree that sometimes the point-freeness is too extreme, will change.

[SenchoPens]

Hi, I've left a couple of comments

[SenchoPens]

Can you please explain what does this line do?

[thecaralice]

First of all it takes total palette; after that it transforms recursively:

1. If the value under question is not toString-able or is a list, recurse with mapAttrs or map
2. Otherwise use toString to turn it into a string

Effectively this transforms all color nodes into strings (they have a __toString attr so they are toString-able) while keeping the attrset (e.g. mnemonic, ansi, ansi.bright) and list (e.g. ansi.dark.toList) structure

[SenchoPens]

This looks like a workaround. I think you can remove both __toString and instead use a common attribute like .asString. Then it will look like mapRecursiveCond (x: x ? asString) (x: x.asString) (total palette).

As far as I understood, you use the implicit conversion to string in only in one other place - in the code for the _color function. There you can do without this conversion: instead of self.hex just use the hex parameter. This will also make for a simpler control flow.

[thecaralice]

My previous comment was incomplete: this transformation also affects numbers such as base01-dec-r and base0E-rgb-b, so lib.strings.isConvertibleWithToString x -> isList x is not really simplifiable (an alternative is isFloat x || isInt x || (isAttrs x && x ? hex))

[SenchoPens]

Then maybe we can convert them to string in the _color function itself?

[SenchoPens]

Ok, but in your PR they still get converted to string, don't they? We can explicitly convert them to string as I proposed above, and provide additional attributes with the integer values. @thecaralice

[thecaralice]

Not sure what you mean, they are converted to strings to match base16/24 spec, and they have the additional attributes in the original attrset.

[SenchoPens]

Oh, I got it. After another thought, the actual problem is the following. You open too much of the internals of the library (increasing the future maintenance burden) to save a little time for 1% of "power users".

But even further, even this potential time saving is under question. Right now, the features you've added are undocumented, so nobody will know about them, thus they are code bloat. Even if we add documentation, it would look like "remember that dec-r attribute? We called lib.strings.toInt for you and you can access this value via original.base00.dec-r", being another sort of bloat.

@thecaralice

[SenchoPens]

In any case, as I've stated in the README, the primary purpose of base16.nix is to be an interface to the base16 standard. Any additional features I want to keep at minimum.

[SenchoPens]

BTW, can you please document the featuers you've added in the DOCUMENTATION.md?

[SenchoPens]

Left one more comment

[SenchoPens]

Hi, added a couple of comments

[SenchoPens]

Looks great, left one comment.

PS sorry for the wait, I was on a trip.

[SenchoPens]

I am a bit confused. By scripting do you mean user-level scripting? Because you still convert them to string, so for the user it does not matter.