Adding setStringTags(string, tags) new function.
Details
For leper.
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Event Timeline
There is some rearranging of code in tooltips.js needed to apply both color and font same time yea?
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))
binaries/data/mods/mod/gui/common/guitags.js | ||
---|---|---|
3 | The description should tell how the tags object looks like and what it exactly does | |
5 | s/text/string ?? | |
5 | Maybe setStringTags | |
7 | s/i/tag |
binaries/data/mods/mod/gui/common/guitags.js | ||
---|---|---|
4–5 | period | |
5 | Maybe mention what the tags do explicitly, but not entirely convinced that is required/useful myself. | |
6 | @return? | |
20 | those can be removed though |
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.)
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.
binaries/data/mods/mod/gui/common/guitags.js | ||
---|---|---|
6 | no s, no string: | |
11 | second tags[tag] => tag (or am I stupid?) |
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.
binaries/data/mods/mod/gui/common/guitags.js | ||
---|---|---|
6 | Found both @return and @returns in the project code. @return seem php: http://www.phpdoc.de/kongress/return.html |
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?)
binaries/data/mods/mod/gui/common/guitags.js | ||
---|---|---|
6 | see that synonyms part | |
11 | 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)? |
binaries/data/mods/mod/gui/common/guitags.js | ||
---|---|---|
11 | I |
binaries/data/mods/mod/gui/common/guitags.js | ||
---|---|---|
7 | setGUITags? guiTags? | |
20 | One function is functionally complete, drop the rest. | |
binaries/data/mods/public/gui/common/tooltips.js | ||
6 ↗ | (On Diff #4919) | orange missing |
87 ↗ | (On Diff #4919) | return functionName(text, g_Bla.blub); |
setStringTags(setStringTags, tags) applied on objects.
coloredText() and "font=" tag replace with setStringTags and object in another patch.
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.
binaries/data/mods/public/gui/common/tooltips.js | ||
---|---|---|
103 ↗ | (On Diff #4924) | seems like we can remove these functions afterwards |
I take this before it gets old.
Thanks, finally the tooltips.js pain is over.
binaries/data/mods/mod/gui/common/guitags.js | ||
---|---|---|
6 | The @return part is redundant with the function description. | |
7 | setStringTags ok too. I'll move this below the color setter because functions should be sorted from less complex to more complex. |
binaries/data/mods/mod/gui/common/guitags.js | ||
---|---|---|
19 | can use it here too |
binaries/data/mods/mod/gui/common/guitags.js | ||
---|---|---|
19 | sorry noise, constructing and deconstructing object seems slightly pointless and we want to nuke the function anyhow |