Page MenuHomeWildfire Games

GridBrowser GUI addon
Needs ReviewPublic

Authored by nani on Oct 16 2018, 11:41 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

Lets you display a list with xml objects in a grid, makes multiple pages if not all the list can be displayed as one.

Test Plan

Implementation of a map browser for gamesetup.
diff

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

nani created this revision.Oct 16 2018, 11:41 PM
nani edited the summary of this revision. (Show Details)
nani edited the test plan for this revision. (Show Details)Oct 16 2018, 11:51 PM
elexis added a subscriber: elexis.Oct 17 2018, 8:25 AM

Code structure:
It's particularly hard to judge this thing without any examplary / the only use case, so I would recommend to create one diff for the mapbrowser.
After examination, code can probably be clarified or simplified a bit.

Use case analysis:
I'm wondering if there is a second use case for this prototype or if it's only used to achieve object-oriented style over procedural style.
What are other use cases? A hero selection dialog? A relic selection dialog?
It's not only a Grid but also includes Paging. In comparison to implementing paging in a separate prototype, adding it to the grids prototype gains less advantages than the same code in two prototypes for instance.
Typically a "GUI addon" is usually a type of a GUI object or a property of GUI object type, defined by the GUI in source/gui/.
So perhaps float-left property from css is what would allow GUI objects to be aligned as a grid without adding any JS code and allowing a number of other problems to be fixed simultaneously? (For example aligning the input to the label in: Label: <input>).
I acknowledge that this is difficult to implement, but my impression of the prototype is that it claims to be more generic than it is.
I propose to upload the MapBrowser combined with its dependencies.

binaries/data/mods/public/gui/common/GridBrowser.js
22

"GUI objects"

23

indirection making it more complicated than it needs to be

56

Pass the function to be called as an argument to the constructor or other function.

59

unnecessary indirection

74

unnecessary indirection

78

""

91

these two functions are always called together..

152

simplify with min/max or modulo?

170

unneeded parentheses and Math.max(1, numPages)

175

No need for template strings (never seen them being used in our JS code either)

178

unnecessary indirection

smiley added a subscriber: smiley.Oct 17 2018, 8:25 AM
smiley added inline comments.
binaries/data/mods/public/gui/common/GridBrowser.js
18

Why are these not in the constructor?

Didn't see P137.

Most probably wont add/update anymore given that @elexis seems to have some refractored code that would modify what is left to do.

Yes, some of that can be deleted then.

smiley added a comment.EditedOct 17 2018, 11:24 AM

I'm wondering if there is a second use case for this prototype or if it's only used to achieve object-oriented style over procedural style.
What are other use cases? A hero selection dialog? A relic selection dialog?

A new mod browser for mod.io, now that they have implemented a proper versioning system (https://docs.mod.io/changelog/#how-we-handle-versioning) we can use the API to the fullest without worrying about breaking changes. Including getting the description and screenshots and stuff.

In D1650#65383, @smiley wrote:

mod.io ... including screenshots

That proposal was refused due to possible image library vulnerabilities

Ah, makes sense. Reminds me of the libpng segfault bug which could lead to a DoS.

nani marked an inline comment as done.Oct 17 2018, 11:27 PM
nani added inline comments.
binaries/data/mods/public/gui/common/GridBrowser.js
18

Not any specific reason. Seems the constructor choice would be better.

22

So change xml object to gui object ?

23

I think the variable defines better the meaning of the value.

56

Correct. Passing the function as argument seems simpler.

59

I think is having setter function homogenizes the function naming convention.

74

Same as the _setList function.

78

correct

91

What two functions?

152

Can't see a way to do it.

175

What is the alternative?

178

Same as _setList and _setChildDimensions.

smiley added inline comments.Oct 18 2018, 8:07 AM
binaries/data/mods/public/gui/common/GridBrowser.js
175

sprintf. Or just regular concatenation if possible.

nani marked 5 inline comments as done.Oct 18 2018, 1:55 PM
nani added inline comments.
binaries/data/mods/public/gui/common/GridBrowser.js
175

I changed to concatenation but I still think string templates are the best option given that reduces needless code duplication of sprintf and is much clear than concatenation. Was sprintf was created before spidermonkey had support for string templates?

nani updated this revision to Diff 6934.Oct 18 2018, 2:14 PM
smiley added inline comments.Oct 18 2018, 7:05 PM
binaries/data/mods/public/gui/common/GridBrowser.js
175

String templates came with ES 2015 IIRC. That file was added in 2014 deducing from a quick file history lookup (probably even earlier). Which raises the question of why its still present?

elexis added inline comments.Oct 18 2018, 8:11 PM
binaries/data/mods/public/gui/common/GridBrowser.js
175

sprintf are used in 0ad for translation, the rest of the code uses implicit string conversion

I'd say that this part can be better in the C++ part. Because it may be faster and has much more information about elements.