color: Add the ability to selectively preserve colors

[DolceTriade]

This adds back the "DECOLOR" functionality that was removed as part of Unvanquished/Unvanquished@d61c9d2. No motivation was given for the removal, and the DECOLOR functionality is important for external tools that parse game logs to display player colors leading to a richer experience.

[slipher]

This seems like an unnecessarily hacky way to restore the functionality in the linked commit. All you would need to do is add a color-preserving variant of G_LogPrintf no? (Or perhaps better would be simply an option to make G_LogPrintf always keep colors.)

[DolceTriade]

add a color-preserving variant of G_LogPrintf

That's not what this functionality does. It allows me to say G_LogPrintf("^1Strip this %c^3Keep this%c", Color::Constants::DECOLOR_OFF, Color::Constants::DECOLOR_ON) which will output:

Strip this ^3Keep this

I was just adding back functionality that was removed before without good motivation presented that was used and was useful.

We could add a non-newline terminating version, non-decoloring of G_LogPrintf(), but that makes that callsite busy too...

It would be something like:

G_LogPrintf2("...", strip);
G_LogPrintf2("...", nostrip);
if (bot) {
G_LogPrintf2("[BOT]\n", nostrip);
} else {
G_LogPrintf2("\n", nostrip);
}

The conditional is fabricated. In reality, this code would be duplicated twice, one for ClientConnect and one for the bot version.

[slipher]

All right I looked at it closer and understand that G_LogPrintf prints to two log targets with and without color. I think the goal here could be achieved by decoloring the format string, rather that the formatted string, and then doing the sprintf once for each target. If that doesn't work out somehow, the decoloring-with-control-codes function should be written in the sgame instead of the engine. This would be easy to write using Color::Parser, if you make the control code a single character rather than a two-character sequence starting with ^.

[DolceTriade]

I'm not sure I follow the first proposal. In my head, I like the current approach, hacky as it is because it allows fine grained control of whether something should be decolored or not in the same string. It offers maximum flexibility.

Re-implementing this in the sgame would basically be duplicating StripColors.

[slipher]

I'm not sure I follow the first proposal.

If you call StripColors on the format string (e.g. ^3Deconstruct: %d %d %s %s: %s %s by %s) before printf-style formatting, this will keep colors on names (and any other interpolated strings), while removing the unwanted ones. This way actually seems most convenient as you don't have to insert any extra control characters around names. There is a question of whether we would want to keep colors in the rare case of interpolated strings that are not names containing them, but I don't see any cases where that should happen. If we keep colors at least there is no information loss.

Re-implementing this in the sgame would basically be duplicating StripColors.

Which is not a problem as a basic StripColors implementation is less than 10 lines.

[DolceTriade]

That won't work when the you have two format strings, and you want one to be stripped and the other to not be stripped.

The case that inspired this whole PR in the first place is this: https://github.com/Unvanquished/Unvanquished/blob/master/src/sgame/sg_client.cpp#L1187-L1190

The previous behavior was one %s == stripped netname, and the other %s is the unstripped netname.

I still maintain this is the best path forward. if you insist on it being in sgame instead of daemon, I can move it, but I honestly think it's better to keep it in daemon because I expect that this won't be the first instance of this as we start to expand our tooling.

[slipher]

That case is easily handled.

G_LogPrintf( "ClientConnect: %i [%s] (%s) \"%s^*\" \"%s^*\"",
	             clientNum, client->pers.ip.str[0] ? client->pers.ip.str : "127.0.0.1", client->pers.guid,
	             Color::StripColors( client->pers.netname ).c_str(),
	             client->pers.netname );

[DolceTriade]

Again, you're missing the core issue. G_LogPrintf (https://github.com/Unvanquished/Unvanquished/blob/0614b6650439c9c5aedefb7d183988343f0b46f4/src/sgame/sg_main.cpp#L1588) is doing the stripping not us.

That's why I've been saying the only other path forward that I would consider would be writing a version of G_LogPrintf that doesn't do newline termination and optionally does stripping.

[slipher]

The difference between the two methods is basically a question of whether you want the default for interpolated strings to be with or without color. If you do it the old way, the default is without color and you have to add the control codes to keep color. If you do it with my first proposal, the default is to keep colors and the caller must call StripColors to remove them. Thoughts?

[slipher]

In the case that we want the default to be stripping colors, I thought of an easier way to do it. The caller can just call the color escaping function (that replaces ^3 with ^^3) and then you get back the colored string after stripcolors.

[DolceTriade]

To recap:
G_LogPrintf takes a string, prints it to console, strips colors, writes to log file.

My new behavior: takes a string, prints to console, strips colors except within control markers, writes to log file.

I think we should largely maintain this behavior. So far, I've only encountered two use cases for this, so it doesn't make sense to go through the codebase and change it.

• (Bot?)ClientConnect
• ClientRename

If you absolutely despise this method, I can add another logging function with more options, but honestly, the old way seems cleaner to me.

Same code in trem: https://github.com/darklegion/tremulous/blob/c862a5340c8de44dcc7abaff170c20c04f9340e8/src/game/g_client.c#L905-L909

[slipher]

My last message didn't make sense. Sorry about that.

My very first idea of a color-preserving G_LogPrintf seems to work well though. PTAL at https://github.com/Unvanquished/Unvanquished/compare/slipher/log-colored-names?expand=1. I think this restores all of the lost functionality. It seems a bit better than the original old version since in the in-console message, the names are also printed once with an once without color. Printing twice with color in the console would look stupid.

I doubt that a general method for partially decoloring is something that will frequently come in handy. If it does it should be in a separate function from StripColors.

[DolceTriade]

The proposal doesn't allow stripping things that are both inside the format string and within the interpolated string.

For example:

G_LogPrintfColored(
					"ClientRename: %i [%s] (%s) \"%s^*\" -> \"%s\" \"%s^*\"",
					clientNum, client->pers.ip.str, client->pers.guid, oldname,
					Color::StripColors( client->pers.netname ), client->pers.netname );

Here, the ^* is maintained and I don't want it to be. I just want the name to be maintained. It's not part of the user's name, but it's in the quotes, so its included in the user's name when it wasn't part of their name.

[slipher]

Ok cool. I just ask that decolor with control codes be a different function in the gamelogic rather than a mandatory part of the official StripColors.

[illwieckz]

Why not using a bit longer markup to only reserve one special character for opening/ending markup?

Like for example \16[ and \16] for opening and closing:

G_LogPrintf("^1Strip this %c[^3Keep this%c]", Color::Constants::DECOLOR, Color::Constants::DECOLOR);

I would like to see a similar feature one day to mark substrings that would be replaced with REDACTED in logs, so I'm also wondering if the feature can be made more generic to support other begin/end marking without wasting many special chars.

The sequences could also be \16D[ and \16D] for decoloring and \16R[ \16R] for redacting out or something like that.

[illwieckz]

Also, is there a need for a special character at all? We may standardize universal opening/closing sequences in the form of ^!K[ and, ^!K[ with K being a single uppercase letter for a feature like D for decoloring and R for redacting out.

So we could either write it plain:

G_LogPrintf("^1Strip this ^!D[^3Keep this^!D]");

or use an enum:

G_LogPrintf("^1Strip this %c^3Keep this%c", Color::Markups::DECOLOR_BEGIN, Color::Markups::DECOLOR_END);

[DolceTriade]

Sure, this same facility could be extended for log redaction.

I don't think the actual escape codes matter as long as they are available in an enum/constant. We should definitely not be using them directly.

[illwieckz]

Indeed, this can be modified and extended later.

I'm fine with the current implementation.