Page MenuHomeWildfire Games

coloredText(text, color) function cleanup
ClosedPublic

Authored by ffffffff on Nov 30 2017, 2:09 PM.

Details

Summary

not sure stumbling over it

Test Plan

discuss

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ffffffff created this revision.Nov 30 2017, 2:09 PM

Agreed to this diff years ago, got disagreement and gave up.

ffffffff added a comment.EditedNov 30 2017, 3:15 PM
In D1088#43196, @elexis wrote:

Agreed to this diff years ago, got disagreement and gave up.

lolo reason?:) and who . maybe rediscussion?:) seems to me very much cleaner:)

I agree with the idea of the proposal.
Abstracting the formatting code will also enable further features.
For instance allowing mods to change any font of the places where we currently only allow changing of the color, as mentioned in rP20552.
The patch is incomplete, tooltips.js and likely more pages would benefit too. Could be done in several patches.
Not in the mood for reiterating discussions I had in 2015 and getting blamed for reducing code while increasing features.

binaries/data/mods/public/gui/common/functions_utility.js
256 ↗(On Diff #4455)

This sounds so fundamental that it should be in modmod/gui/common/

ffffffff added inline comments.Nov 30 2017, 9:56 PM
binaries/data/mods/public/gui/common/functions_utility.js
256 ↗(On Diff #4455)

Sorry do you mean binaries/data/mods/mod/gui/common/ ?

cause i didnt find any js code there

elexis added inline comments.Nov 30 2017, 10:28 PM
binaries/data/mods/public/gui/common/functions_utility.js
256 ↗(On Diff #4455)

yes. (modmod was the mod selection page)
But keep it in public/gui/common/ for now until we have more than this one-liner function that would have to be placed in a single file.
colors.js which you already cleaned long ago to fix the dark lobby playername colors might be an idea. But on the other hand that won't fit well with the equivalent font setter. So probably good as is.

Imarok added a subscriber: Imarok.Dec 1 2017, 3:06 PM

Looks good in general

In D1088#43206, @elexis wrote:

I agree with the idea of the proposal.
Abstracting the formatting code will also enable further features.
For instance allowing mods to change any font of the places where we currently only allow changing of the color, as mentioned in rP20552.
The patch is incomplete, tooltips.js and likely more pages would benefit too. Could be done in several patches.
Not in the mood for reiterating discussions I had in 2015 and getting blamed for reducing code while increasing features.

Do you have any link to the discussion, a related ticket, the timeframe or the platform the discussion took place?

elexis added a comment.Dec 1 2017, 3:25 PM

Likely 2015 on irc, not easy to grep. Wasn't a long discussion either as it wasnt't important to me.
My discussion partner might have revised his opinion or doesnt care about GUI code except that one page.
The argument was something along the lines of a function with only one line being rendered pointless.
But the patch receives benefits from applying the https://en.wikipedia.org/wiki/Information_hiding pattern to many places, i.e. less code being less to read, less to break, less to modify when extending.

elexis accepted this revision.Dec 2 2017, 12:58 PM

So I even have to accept this, great.
Didn't test yet, but from a quick readthrough it looks correct and really makes the code more readable.
Further uses of that function are better off in separate diffs.

binaries/data/mods/public/gui/lobby/lobby.js
663 ↗(On Diff #4455)

Good to see the code cut in half

1446 ↗(On Diff #4455)

whitespace, can do if committing

This revision is now accepted and ready to land.Dec 2 2017, 12:58 PM
elexis added a comment.Dec 4 2017, 4:33 AM

The patch doesn't apply in two places, also there (not more than) 28 other places using color tags, mind updating those too while at it?
Either leave out the three occurrences in binaries/data/mods/mod/gui/modmod/modmod.js or find evidence in the c++ code that these tags are indeed https://de.wikipedia.org/wiki/BBCode and make it mod/gui/common/bbcode.js, so that we could then move the font over there too. (But I really want to use the same name we use in the engine too for consistency)

ffffffff updated this revision to Diff 4528.Dec 4 2017, 3:47 PM

More occurences cleared.

binaries/data/mods/public/gui/lobby/lobby.js
663 ↗(On Diff #4455)

Thx

elexis added a comment.Dec 4 2017, 3:55 PM

I still get three merge conflicts, are you sure you updated svn and have no other patches applied?

In D1088#44001, @elexis wrote:

I still get three merge conflicts, are you sure you updated svn and have no other patches applied?

sry i update

elexis added a comment.Dec 4 2017, 6:26 PM

About the function location, we have a lot of bb-code, but the color tags aren't bbcode. They are just called tags according to GUIText.h and GUIText.cpp.
So I would propose to have that in mod/gui/common/tags.js and extend that to also have a font setter in the next patch.

Tested replay menu, structree, options page, trading dialog, batch training, civ info dialog, mod selection dialog. hotkey tooltips, trader dailog, gamesetup dropdowns, lobby, load dialog.

Thanks for the patch, another chapter in the book of GUI cleanup can be closed.

binaries/data/mods/public/gui/common/color.js
151 ↗(On Diff #4529)

missing spaces, making those newlines.
perhaps it would be nicer to call escapeText around that string instead of escaping manually, on the other hand its hard to be lazy then (as we had to consider the possible values of keys. Actually this sounds like its not optional but required to escape then! -> Separate diff)

binaries/data/mods/public/gui/common/gamedescription.js
162 ↗(On Diff #4529)

weird thing, should have its color function passed as an argument eventually

binaries/data/mods/public/gui/common/tooltips.js
2 ↗(On Diff #4529)

unneeded parentheses
I'll revert this, the array should remain consistent. Can do that when adding the font setter

binaries/data/mods/public/gui/gamesetup/gamesetup.js
947 ↗(On Diff #4529)

1 line sufficient

binaries/data/mods/public/gui/pregame/mainmenu.js
147 ↗(On Diff #4529)

It doesn't reduce the degree of freedom translators have while making it more readable to developers.
However we have to hope that they all understand that this is changed string and can reuse the prior translation besides the formatting, and that warning is Warning:, and that Warning: is used in this string.
But then again they should briefly look into the code to begin with if they have a question (which is one click on transifex).
Also people who need to change that colon will notice right away where they have to do that.
The connotation of Warning: is self-explanatory and doesnt need a string comment.

So ok for me.

You already have a transifex account I recall, siehe Strukturbaum. Please help out on transifex where possible (many translators have questions that can be answered by people who can read code)

binaries/data/mods/public/gui/session/messages.js
555 ↗(On Diff #4529)

So this is the only [color occurrence that remains and I have always said we should store the unmodified strings. So it should be fixed in a separate diff eventually.

560 ↗(On Diff #4529)

1 line, also ) on the previous line if its not an array or object

binaries/data/mods/public/gui/session/selection_panels.js
611 ↗(On Diff #4529)

gotcha!
Also that \n shouldnt be colored.

1014 ↗(On Diff #4529)

not nice to set the color here and the font in the function, moving this.

binaries/data/mods/public/gui/summary/layout.js
217 ↗(On Diff #4529)

same text in both cases, might be nicer to avoid the repetition but then we have to repeat the color specified in the xml, too bad.
adding newlines.

This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Dec 4 2017, 6:29 PM
elexis added a comment.Dec 5 2017, 5:55 PM

Wanna write us a patch for the font tags, [font=\"sans-14\"]?

ffffffff added inline comments.Dec 7 2017, 2:41 PM
ps/trunk/binaries/data/mods/public/gui/session/unit_actions.js
638

error typo
must state coloredText(tooltip, "orange")

but small "t" need correction