Page MenuHomeWildfire Games

setStringTags(string, tags) function

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



Adding setStringTags(string, tags) new function.

Test Plan

For leper.

Diff Detail

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

Event Timeline

ffffffff created this revision.Dec 22 2017, 2:38 AM
ffffffff edited the summary of this revision. (Show Details)
ffffffff edited the test plan for this revision. (Show Details)

I would love to see this applied to tooltips.js

There is some rearranging of code in tooltips.js needed to apply both color and font same time yea?

ffffffff retitled this revision from setTags() function to setTags(text, tags) function.Dec 22 2017, 11:53 PM
bb added a comment.Dec 23 2017, 5:22 PM

If I understand correctly, this is the function to go for, then all the color/font/whatever setters are merged in this one function, so we don't need them anymore (after changing all the calls ofc, but that can be done in separate patch(es))

2 ↗(On Diff #4874)

The description should tell how the tags object looks like and what it exactly does
(maybe just make the @param thingies)

4 ↗(On Diff #4874)

s/text/string ??

4 ↗(On Diff #4874)

Maybe setStringTags

6 ↗(On Diff #4874)


ffffffff updated this revision to Diff 4910.Dec 23 2017, 6:06 PM
ffffffff retitled this revision from setTags(text, tags) function to setStringTags(string, tags) function.
ffffffff edited the summary of this revision. (Show Details)

feeling fine

bb added inline comments.Dec 23 2017, 6:13 PM
3–4 ↗(On Diff #4910)


4 ↗(On Diff #4910)

Maybe mention what the tags do explicitly, but not entirely convinced that is required/useful myself.

5 ↗(On Diff #4910)


20–21 ↗(On Diff #4910)

those can be removed though
(my considerations were saying, do this patch, but do not make functions for font only, so the todo can be removed)

But i find it a little heavy with only setStringTags(string, tags) to set tags for "color" and "font" everywhere. Feeling we need also little setStringColor(string, color) or setColor(string, color) and setStringFont(string, font) or setFont(string, font) for easy read (and not everywhere applying the setStringTags function with an object tag as { "color": "color" ] f. e.)

ffffffff updated this revision to Diff 4911.Dec 23 2017, 6:21 PM
ffffffff updated this revision to Diff 4913.Dec 23 2017, 6:27 PM


bb added a comment.Dec 23 2017, 6:40 PM

I think we can solve that by proper naming in the calls. elexis mentioned most (if not all) colors and fonts are globals, thus from those globals we could make the objects already, and then the code would read like setStringTags("foo", colorTag) or setStringTags("foo", fontTag). Then I don't see how this is harder to read, than the other functions. Even when making the object inline with the call we get setStringTages("foo", { "color": blue }) looks pretty obvious, doesn't it?

You might be right there might be a small performance gain in having the separate functions, but that takes nothing away from this the patch and the gain is imo too small to define different functions for. Also imagine more tags to be added (f.e. when the fontsize is splitted of), we need to give all those a new function, so the number of functions will increase rapidly. And so a reader of the code needs to know all of them, while now, knowing just one is enough.

5 ↗(On Diff #4913)

no s, no string:
* @return {string} - String surrounded by tags.

10 ↗(On Diff #4913)

second tags[tag] => tag (or am I stupid?)

In D1167#47011, @bb wrote:

elexis mentioned most (if not all) colors and fonts are globals

At least most (if not all) colors and fonts should be globals if they aren't already, so that data and code is split separation of concerns pattern.

setStringTages("foo", { "color": blue })

That's what I had in mind, besides that e

The naming might want to be considered. The filename and function name should correspond to the name that actualy labels this format, which can only be what is defined in the C++ code, which last time I checked was "gui tags".

and I'd still love to see that thing I call ugly at the top of tooltips.js being treated with this new function as a good example.

ffffffff marked 4 inline comments as done.Dec 23 2017, 8:14 PM
ffffffff added inline comments.
5 ↗(On Diff #4913)

Found both @return and @returns in the project code.

@return seem php:
@returns jsdoc:

ffffffff marked an inline comment as done.Dec 23 2017, 8:17 PM

Yea. Thought same. Only for readability would make good, for the some calls, where no object is used at the moment for color, font.

(I'm not getting that tooltips.js diff, am I?)

5 ↗(On Diff #4913)

see that synonyms part
see also my comments about the naming of functions

10 ↗(On Diff #4913)

Seems correct to not escape anything, because we do want to be able to use that function recursively and gui tags never contain quotes.

So did anyone notice that this fixes all the inconsistencies of quoted quotes? (Sometimes ' being used, othertimes quotes being escaped)?

ffffffff added inline comments.Dec 23 2017, 8:47 PM
10 ↗(On Diff #4913)


ffffffff updated this revision to Diff 4919.Dec 23 2017, 11:51 PM

Only to show here once how much love this can be when using functions set...

ffffffff added inline comments.Dec 23 2017, 11:54 PM
92 ↗(On Diff #4919)

here u can see setStringFont compared to setStringTags with object in parameter

599 ↗(On Diff #4919)

Here setStringFontColor can be seen compared to object style again L596

elexis added inline comments.Dec 24 2017, 12:02 AM
20–21 ↗(On Diff #4910)

One function is functionally complete, drop the rest.
Not having the other functions means it's less to interpret.

6 ↗(On Diff #4874)

setGUITags? guiTags?

6 ↗(On Diff #4919)

orange missing

87 ↗(On Diff #4919)

return functionName(text, g_Bla.blub);

ffffffff updated this revision to Diff 4924.Dec 24 2017, 1:13 AM

setStringTags(setStringTags, tags) applied on objects.

coloredText() and "font=" tag replace with setStringTags and object in another patch.

ffffffff edited the summary of this revision. (Show Details)Dec 24 2017, 4:12 AM

Thanks for the patch, an important one for the future that should have been here years ago.
JSdoc could be polished a bit.
I would recommend to have the patch committed.

103 ↗(On Diff #4924)

seems like we can remove these functions afterwards

elexis accepted this revision.Dec 27 2017, 2:21 PM

I take this before it gets old.

Thanks, finally the tooltips.js pain is over.

5 ↗(On Diff #4924)

The @return part is redundant with the function description.
Surrounding (umzingeln) and around doesn't fit to well.

6 ↗(On Diff #4874)

setStringTags ok too.

I'll move this below the color setter because functions should be sorted from less complex to more complex.

This revision is now accepted and ready to land.Dec 27 2017, 2:21 PM
elexis added inline comments.Dec 27 2017, 2:27 PM
19 ↗(On Diff #4924)

can use it here too

elexis added inline comments.Dec 27 2017, 2:29 PM
19 ↗(On Diff #4924)

sorry noise, constructing and deconstructing object seems slightly pointless and we want to nuke the function anyhow

This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Dec 27 2017, 2:36 PM