Hex colors #3494
Hex colors #3494
Conversation
There was a problem hiding this comment.
As far as I can tell (haven't tested this yet), this looks like a cool implementation.
- I have a few inline comments, check them out
- We don't really have a way to test this actually works. It sounds like the
color ansi|xtermdefault command should be extended to able to docolor truecolorfor supported clients and have it show a suitable range of colors (clearly showing all colors is not possible). It is also a good way to show that it gracefully maps to xterm256 (or even to ANSI) properly.
| # use name to identify support for xterm truecolor | ||
| truecolor = False | ||
| if (clientname.endswith("-TRUECOLOR") or | ||
| clientname in ( | ||
| "AXMUD", | ||
| "TINTIN" | ||
| )): | ||
| truecolor = True | ||
|
|
There was a problem hiding this comment.
The TTYPE/MTTS protocol supports TRUECOLOR as a valid parameter. With this check it seems you are assuming truecolor=False and ignoring the TTYPE/MTTS value sent by the client completely.
I think you should honor the handshake and only override if there is no such value in the communication, or if you know that the given client sends a faulty value (or don't support the proper handshake, which is what most of these client-exceptions are for).
There was a problem hiding this comment.
Gotcha. I was just following the example set in the case above for xterm256. Thanks for the correction.
Edit: Actually, looking at the code, it appears that happens in the following step? Which I suspect is why xterm256 is handled in the same way. But please let me know if I'm misunderstanding it.
evennia/utils/ansi.py
Outdated
| if cachekey in _PARSE_CACHE: | ||
| return _PARSE_CACHE[cachekey] | ||
|
|
||
| # pre-convert bright colors to xterm256 color tags | ||
| string = self.brightbg_sub.sub(self.sub_brightbg, string) | ||
|
|
||
| def do_truecolor(part: re.Match, truecolor=truecolor): | ||
| hex2truecolor = HexColors() |
There was a problem hiding this comment.
Why initialize this every regex query? It seems needlessly expensive. It's a stateless class, it could be initialized once at the top of this module.
There was a problem hiding this comment.
🤦♂️ Mistake on my part! Sorry.
evennia/utils/ansi.py
Outdated
| """ | ||
| Parses a string, subbing color codes as needed. | ||
|
|
||
| Args: | ||
| truecolor: |
| prop += f'"' | ||
| return prop | ||
|
|
||
| def _split_hex_to_bytes(self, tag: str) -> tuple[str, str, str]: |
There was a problem hiding this comment.
Minor style-nitpick: Place the methods being used before the methods using them. This is standard across Evennia. So move these private methods up towards the top of this class.
evennia/utils/tests/test_ansi.py
Outdated
| @@ -8,7 +8,9 @@ | |||
|
|
|||
| from django.test import TestCase | |||
|
|
|||
| from evennia.utils.ansi import ANSIString as AN | |||
| from evennia.utils.ansi import ANSIString as AN, ANSIParser | |||
There was a problem hiding this comment.
This is not ANSI though, it's Truecolor. I think this belongs in its own test module utils/tests/test_hex_colors.py.
evennia/utils/tests/test_ansi.py
Outdated
| @@ -52,3 +54,124 @@ def test_split_with_mixed_strings(self): | |||
| self.assertEqual(split2, split3, "Split 2 and 3 differ") | |||
| self.assertEqual(split1, split2, "Split 1 and 2 differ") | |||
| self.assertEqual(split1, split3, "Split 1 and 3 differ") | |||
|
|
|||
| # TODO: Better greyscale testing | |||
evennia/utils/text2html.py
Outdated
| # Classes can't be used for true color | ||
| prefix = (f'<span ' | ||
| f'class="{" ".join(classes)}" ' |
There was a problem hiding this comment.
You comment that classes cannot be used, yet the class= is set here? At the very least the comment is confusing.
There was a problem hiding this comment.
Sorry about that! Classes can't be used for (for all) colors because of the verbosity of truecolor BUT other classes can still be applied (for things like blink, etc). I will clarify the comment.
-Reordered methods in HexColors -Separated truecolor tests -Clarified comment re: classes and styles in text2html.py -Changed ansi.py to only instatiate HexColors once 🤦♂️ -Fixed missing docsctring in parse_ansi re: truecolor
Thanks! I was worried because it was a different method of implementation than Evennia was using but it seemed like a good choice given @InspectorCaracal's project in #3253.
I hadn't noticed that command. Thanks for pointing it out! |
…e because of the number of colors it supports (style borrowed from the termstandard repo's test)
Brief overview of PR changes/additions
Added support for truecolor in the portal (xterm truecolor) and the web portal using the style attribute. Syntax used is |#0f0, |#0f0f764, |[#00f, etc.
Motivation for adding to Evennia
#3248 Who doesn't like more colors?