not sure stumbling over it
Details
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
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/ |
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 |
binaries/data/mods/public/gui/common/functions_utility.js | ||
---|---|---|
256 ↗ | (On Diff #4455) | yes. (modmod was the mod selection page) |
Looks good in general
Do you have any link to the discussion, a related ticket, the timeframe or the platform the discussion took place?
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.
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 |
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)
More occurences cleared.
binaries/data/mods/public/gui/lobby/lobby.js | ||
---|---|---|
663 ↗ | (On Diff #4455) | Thx |
I still get three merge conflicts, are you sure you updated svn and have no other patches applied?
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. |
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 |
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. 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! |
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. |
ps/trunk/binaries/data/mods/public/gui/session/unit_actions.js | ||
---|---|---|
638 | error typo but small "t" need correction |