HomeWildfire Games

Add helper function to apply arbitrary GUI tags (such as font and color).


Add helper function to apply arbitrary GUI tags (such as font and color).

This avoids the caller having to duplicate the GUI tag format over and over again (equal to rP20585),
detects syntax errors at compile time and
applies the separation-of-concerns pattern (callers only have to specify the tag value agnosticly of the tag format).

Differential Revision: https://code.wildfiregames.com/D1167
Patch By: fpre / ffffffff
Comments By: bb
Refs rP16262


Event Timeline

rosievers raised a concern with this commit.Dec 27 2017, 4:46 PM
rosievers added a subscriber: rosievers.

Hi, I wanted to add a comment about this function, even though I'm not part of the project in any way. I hope I didn't step on any toes in the process. "rosievers raised a concern" sound like a harsh action for an outsider with a fresh account.


If I understand this right, this function would discard all but the last tag which likely isn't the intended behavior.

This commit now has outstanding concerns.Dec 27 2017, 4:46 PM

My fail, I have added the helper variable result (since overwriting input variables should only be done when writing the output value is intended so that the reader sees on first sight that it is impossible that this call can change a reference outside of the function (even though people should know how JS works)), but blatantly incorrectly.

Index: binaries/data/mods/mod/gui/common/guitags.js
--- binaries/data/mods/mod/gui/common/guitags.js	(revision 20697)
+++ binaries/data/mods/mod/gui/common/guitags.js	(working copy)
@@ -12,12 +12,12 @@ function coloredText(text, color)
  * @param {string} string - String to apply tags to.
  * @param {object} tags - Object containing the tags, for instance { "color": "white" } or { "font": "sans-13" }.
 function setStringTags(text, tags)
-	let result = "";
+	let result = text;
 	for (let tag in tags)
-		result = '[' + tag + '="' + tags[tag] + '"]' + text + '[/' + tag + ']';
+		result = '[' + tag + '="' + tags[tag] + '"]' + result + '[/' + tag + ']';
 	return result;
rosievers accepted this commit.Dec 27 2017, 5:31 PM

Looks better this way, thank you. Do you think coloredText should be defined via setStringTags to avoid code duplication?

return setStringTags(text, {"color" : color })
All concerns with this commit have now been addressed.Dec 27 2017, 5:31 PM

Thanks for the review.

Yes I noticed that too (see review), but it seems a little hurtful to create an object and deconstruct it right after (the function can be called very often, on each onTick I believe),
But the function should probably be removed after all its use cases were removed. (all colors and fonts should be a global tags object for mods)

Krinkle added a subscriber: Krinkle.Sep 3 2019, 7:06 PM
Krinkle added inline comments.

Indeed. To work correctly it would need to do something like assigning result = text; first and then iteratively keep re-wrapping result = … results ….


... which is what rP20701 has since done.