Page MenuHomeWildfire Games

setColor rename
Needs ReviewPublic

Authored by ffffffff on Dec 22 2017, 2:21 AM.

Details

Reviewers
elexis
Summary

Replace coloredText() to setColor().

Test Plan

Replaced and relooked function names.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

ffffffff created this revision.Dec 22 2017, 2:21 AM
elexis added a subscriber: bb.Dec 22 2017, 2:28 AM

Before we change the name of everything we have, I would first like to get a third opinion, for instance the one of @bb.

We need three functions: setColor, setFont, setColorFont is one proposal

From https://code.wildfiregames.com/P99#740:

we could also use coloredText(), fontText(), guiTags().

or maybe the third one could be coloredFontText()

elexis retitled this revision from setColor() cleanup to setColor rename.Dec 22 2017, 2:28 AM
bb added a comment.Dec 22 2017, 6:40 PM
In D1166#46688, @elexis wrote:

Before we change the name of everything we have, I would first like to get a third opinion, for instance the one of @bb.

changing a name is fine as long as it results in a better name, I don't (yet) see why the proposed name is any better than the previous one.

We need three functions: setColor, setFont, setColorFont is one proposal
From https://code.wildfiregames.com/P99#740:

we could also use coloredText(), fontText(), guiTags().

So what is the benefit of having 3 function, when just one has all the functionality of both others and more? ok one doesn't have to create an object and unpack it, so a tiny performance gain, but is that worth having 3 functions instead of 1?

or maybe the third one could be coloredFontText()

Nah, keep it general, who knows if/when we split the size from the font, or create subfonts (Bolt italic etc.) then those need to find a way in aswell (yes currently that are all different fonts, but it can be changed ofc). Those things also run into trouble when having separate functions for every setting, since every setting will come with an own function, and when there are functions for combinations also, we get an exponential increase...

Aiming to only use one guiTags function wouldn't come with any cost once the arguments are globals everywhere (and gui tags should be split from the code anyway.)
So the other two functions would only be useful as a transition if the final state is going to be having 0 calls to them.

bb added a comment.Dec 22 2017, 7:01 PM

Okay, that seems like a good final vision (and I agree on it), but then I utterly don't understand why we then need a function rename like this. One could:

  1. Implement the new function
  2. Swap the calls to using the new function (can/should be multiple commits!!)
  3. Remove the old function

So I don't see the need for a rename in that process.

Stan added a subscriber: Stan.Dec 22 2017, 7:12 PM

Just my two cents here but why is not 'setTextColor' ? 'coloredText' doesn't make sense as to what it does to the text, but 'setColor' seems a bit too generic like it would apply to everything. Which in this case could be something like 'setObjectColor'

ffffffff added a comment.EditedDec 22 2017, 10:04 PM

Yea thats true, its just setColor as name because its not to long and in our mainly purposes in code used for textcolor setting for rather short but often used tag scenario, which is cleaning up this scenario by an easy function. :)

I am also a fan now of this general used setFunctions, because they are very easy to read and to work also easy with. So setTags(text, tags) would also be nice to have. setColorFont(text, color, font) can be commited to complete this functions, but at the moment not really needed, so its more a completion here. setFont(text, font) same easiness in usage and term here.

all tags should become globals and use the new function from D1167

binaries/data/mods/public/gui/lobby/lobby.js
33
var g_GameStateTags = {
     "init": { "color": "foo" },
};
48
var g_PlayerStatuses = {
   "available": { "tags": { "color": "foo" }, "label": translate("Online") }
};
75

these ought to become tag objects