gui/common/functions_utility.js is a mess and removing the redundant functions will be a great improvement.
The loading screen tips code can become shortened and easier to read.
Not happy about the listFiles being in Templates.js, see P97 for where it should be located eventually.
Details
Details
- Reviewers
bb - Commits
- rP20692: JS file listing cleanup, refs #4868.
Notice that the code does the same as before.
Diff Detail
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
Comment Actions
Executing section Default...
Executing section Source...
Executing section JS...
binaries/data/mods/public/gui/loading/loading.js
| 17| » » » Engine.GetGUIObjectByName("tipText").caption·=·tipText.join("\n")
| | [NORMAL] JSHintBear:
| | Missing semicolon.
binaries/data/mods/public/globalscripts/Templates.js
| 99| » » ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
| | [NORMAL] JSHintBear:
| | Misleading line break before '||'; readers may interpret this as an expression boundary.Comment Actions
Have a look at that inline and it should be ok
other occurrences of listDirectoryFiles are correct
| binaries/data/mods/public/gui/loading/loading.js | ||
|---|---|---|
| 15–17 ↗ | (On Diff #4551) | so we split shift and join => looks like inefficient. Can't we look for the first index of "\n" and splice the part before? |
| 35 ↗ | (On Diff #4551) | why care if there are no tips => good |
| 56 ↗ | (On Diff #4551) | seems like modding support => good |
| binaries/data/mods/public/globalscripts/Templates.js | ||
|---|---|---|
| 7 ↗ | (On Diff #4551) | Still not happy about this function being here. Probably better in gui/common/ and letting the DataTemplateManager use it's own copy of this line. |
| binaries/data/mods/public/gui/common/functions_utility.js | ||
| 27 ↗ | (On Diff #4551) | than hiding it here in a function that according to it's name should also be usable for non-map XML files. |
| binaries/data/mods/public/gui/gamesetup/gamesetup.js | ||
| 1449 ↗ | (On Diff #4551) | Maps starting with underscore are hidden in the gameseutp. I didn't know that. It's only used for the atlas _default map. The feature should be more visible this way. |
| binaries/data/mods/public/gui/loading/loading.js | ||
| 15–17 ↗ | (On Diff #4551) | inefficient -> The microseconds on init are irrelevant. |
| 35 ↗ | (On Diff #4551) | Perhaps because there would be a an empty graphics. Don't see the benefit making this an error message exceeding the benefit of not having it however. Especially since mods might want to nuke the graphics without introducing a modified copy of this file. |
| 56 ↗ | (On Diff #4551) | mostly removed it because the GUI would break on all ends from front to back if there is a different maptype used, so the error string doesn't add anything but code |
| binaries/data/mods/public/globalscripts/Templates.js | ||
|---|---|---|
| 7 ↗ | (On Diff #4551) | Isn't the reason we have globalscripts that they can be used everywhere, maybe just place it in globalscripts/utility.js since there is a extension fucntion already |
| binaries/data/mods/public/gui/loading/loading.js | ||
| 15–17 ↗ | (On Diff #4551) | It is sad we can't splice strings... |
Comment Actions
This function_utilities.js is one of the worst mess in this folder, so readers will benefit from the removal of unrelated code there.
Thanks for the review.
| binaries/data/mods/public/globalscripts/Templates.js | ||
|---|---|---|
| 7 ↗ | (On Diff #4551) | utility.js is much better. |
| binaries/data/mods/public/gui/loading/loading.js | ||
| 35 ↗ | (On Diff #4551) | Having the warning means that a modder is informed of the cause without having to lookup the code and the disadvantage to the reader doesn't seem to outweigh that (as long as having no tooltips is considered a defect and not a feature, which it should since the graphics look crap then). (On the other hand mods might want to hide the graphics externally and would then run into the error. Meh) |