Page MenuHomeWildfire Games

GridBrowser GUI addon
AbandonedPublic

Authored by nani on Oct 16 2018, 11:41 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
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

Diff Detail

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
21 ↗(On Diff #6929)

"GUI objects"

22 ↗(On Diff #6929)

indirection making it more complicated than it needs to be

55 ↗(On Diff #6929)

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

58 ↗(On Diff #6929)

unnecessary indirection

73 ↗(On Diff #6929)

unnecessary indirection

77 ↗(On Diff #6929)

""

90 ↗(On Diff #6929)

these two functions are always called together..

151 ↗(On Diff #6929)

simplify with min/max or modulo?

169 ↗(On Diff #6929)

unneeded parentheses and Math.max(1, numPages)

174 ↗(On Diff #6929)

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

177 ↗(On Diff #6929)

unnecessary indirection

lyv added a subscriber: lyv.Oct 17 2018, 8:25 AM
lyv added inline comments.
binaries/data/mods/public/gui/common/GridBrowser.js
17 ↗(On Diff #6929)

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.

lyv 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

lyv added a comment.Oct 17 2018, 2:13 PM

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
17 ↗(On Diff #6929)

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

21 ↗(On Diff #6929)

So change xml object to gui object ?

22 ↗(On Diff #6929)

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

55 ↗(On Diff #6929)

Correct. Passing the function as argument seems simpler.

58 ↗(On Diff #6929)

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

73 ↗(On Diff #6929)

Same as the _setList function.

77 ↗(On Diff #6929)

correct

90 ↗(On Diff #6929)

What two functions?

151 ↗(On Diff #6929)

Can't see a way to do it.

174 ↗(On Diff #6929)

What is the alternative?

177 ↗(On Diff #6929)

Same as _setList and _setChildDimensions.

lyv added inline comments.Oct 18 2018, 8:07 AM
binaries/data/mods/public/gui/common/GridBrowser.js
174 ↗(On Diff #6929)

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
174 ↗(On Diff #6929)

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
lyv added inline comments.Oct 18 2018, 7:05 PM
binaries/data/mods/public/gui/common/GridBrowser.js
174 ↗(On Diff #6929)

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
174 ↗(On Diff #6929)

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.

lyv added inline comments.Dec 21 2018, 6:30 AM
binaries/data/mods/public/gui/common/GridBrowser.js
36 ↗(On Diff #6934)

These makes it harder to follow. Combine them and possibly add an update() method.

nani updated this revision to Diff 7059.Dec 23 2018, 11:38 PM

Rewrote lots of superfluous code. Added useful functions and features (animation).
Cleaner code.

nani edited the summary of this revision. (Show Details)Dec 23 2018, 11:44 PM
nani updated this revision to Diff 7458.Feb 7 2019, 4:09 AM

Made coder more clear added some checks, functionality.

nani edited the summary of this revision. (Show Details)Feb 17 2019, 2:38 AM
nani edited the test plan for this revision. (Show Details)
wraitii added a reviewer: Restricted Owners Package.Apr 22 2019, 9:33 AM
Freagarach abandoned this revision.Jan 12 2021, 8:44 PM
Freagarach added a subscriber: Freagarach.

Incorporated in D1703.