Page MenuHomeWildfire Games

Map browser for gamesetup
Needs ReviewPublic

Authored by nani on Sun, Dec 23, 11:49 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

Would be nice to see all maps previews in a single page and be able to select or just see them.

Has added animations to make the page more appealing.

Needs patches:
https://code.wildfiregames.com/D1702
https://code.wildfiregames.com/D1650

Test Plan

See that everything works "just right"

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

nani created this revision.Sun, Dec 23, 11:49 PM
elexis added a subscriber: elexis.Mon, Dec 24, 8:58 AM

Map description needs some frame.

General comments:

  • local `const: The const keyword is misleading, because the object it refers to is still mutable, one just can't change the reference to the object. deepfreeze would do that.

It would be preferable to do it consistently in the code and use let for local variables.

  • Unneeded variables: Some variables are used in only one place, so it is unneeded to allocate the variable at all. For example:
	g_Dropdowns.mapSelection.select(
		g_Dropdowns.mapSelection.ids().indexOf(data.map.path + data.map.name));
  • JavaScript in XML is ugly. Ideally all JS is defined in JS files, all XML in XML files. All GUI Object events can be moved to JS easily by using `Engine.GetGUIObjectByName("foo").onBar = () => {...};
  • Comments on separate lines, not after the code on the same line
  • I'm not sure if the 'GridBrowser' should be in common/, or in a separate revision. One can't review the GridBrowser on it's own, nor can one test this patch without the GridBrowser. It's probably possible to boil multiple code objects into a less complex thing.
  • Will have to care that this works with 1024x768 and possibly long string translations.
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1250

Remove from this diff, possibly insert reference to the according revision proposal.

2413

The opened dialog is not updated if the server broadcasted new g_GameAttributes.
This is relevant in case the mapbrowser shows data about the currently selected map, or if there are multiple clients that can change g_GameAttributes (or if there was another event that would change one of the three g_GameAttributes properties).

This is currently solved by the AI config dialog by closing the child page and reopning it. (If the patch to #5369 was committed that would be trivial to solve.)

2417

(refs D1684)

2436
  • Changing the maptype will drop all host settings. Only do such things if they actually changed.
2444
  • updateGameAttributes not updateGUIObjects necessary to broadcast g_GameAttributes for multiplayers
  • .select already calls updateGameAttributes (which later calls updateGUIObjects).
binaries/data/mods/public/gui/gamesetup/gamesetup.xml
144

Space-conserving idea to reuse the mappreview image as a button.
But users will not notice that this is a button if it doesn't strikingly appear as one, which means the feature would not be noticed by many players.

binaries/data/mods/public/gui/mapbrowser/mapbrowser.js
9

quotes around property names

no spaces at the property access

119

(unneeded braces)

160

There is a common function for that already. We shouldn't have two of them. The protoype in general seems to have this redundancy problem.
The mapfilters are copied entirely. I remember that's why we thought to do this after #5322 so that some of these gamesetup objects can be moved to common/ and reused by other GUI pages.

168

Prototypes better in their own file (like we have in the simulation and rmgen)

174

let not in global context. (I heard that should become illegal with future spidermonkey or ES versions)

174
177

var g_MapSelected = {

"set": (map, filter) => {
          ....
 }

};

187

refs OOP rewrite. (Having each component decide for itself what it wants to display and which data it uses, rather than having the data decide which objects it must update, which leads to lasagna).

195

"mapPreview[" + index + "]" seems easier (also consistent with the rest of the GUI rather than introducing templates which nothing else in 0ad uses as mentioned)

198

It removes indirection and became a GUI convention to use the same variable name for the GUI object; i.e. mapButton instead of button.

273

Is it improtant to have a non-linear scale?

368

Structure the code in a way that every function has one designated purpose. For procedural code style, one function for the tooltips, one to init each dropdown and then calling them from init.

Object oriented code like the objects in gamesetup.js or UnitAI.js has advantages over procedural style, the GUI JS codebase will likely be rewritten to use OOP consistently (so would be nice to rewrite less later.)

376

Instead of passing a boolean that something was selected, it can pass the data only if it should be changed.

398

missing newline

binaries/data/mods/public/gui/mapbrowser/mapbrowser.xml
32

textcolor, style or gui tags no?

78

If a zoom level is implemented, considerable alternatives to two buttons are a slider and a dropdown.

82

redundant

119
  • *hotkey name:** Those aren't tabs, right? So the hotkey name and explanation may be reconsidered if the hotkey action is not unique enough for a new hotkey.
  • (It would be preferable to have the tooltip about this hotkey and the addition of this hotkey in the same file and located in the same place in the file, rather than fragmenting it. If the patch to add hotkeys from JS was committed this were trivial to fix.)
nani marked 20 inline comments as done.Mon, Dec 24, 2:39 PM
nani added inline comments.
binaries/data/mods/public/gui/gamesetup/gamesetup.js
2413

The map browser is not meant to show the currently selected map from game setup but rather use the current selected map as starting point for opening the map browser.

The functionality can added if it is really necessary in the future.

2444

.select doesn't call updateGameAttributes

binaries/data/mods/public/gui/mapbrowser/mapbrowser.js
160

For the record I copy-pasted some code given that i only needed some functionality of game setup but wasn't possible to just grab some function without the rest of the gamesetup.js code.

177

Needs a function not am arrow function.

273

Not really but linear doesn't mean better.

binaries/data/mods/public/gui/mapbrowser/mapbrowser.xml
32

What do you mean?

78

The drop down would be a bad idea imho.

nani updated this revision to Diff 7063.Mon, Dec 24, 2:44 PM
nani marked 5 inline comments as done.

Fixes from comments.