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.



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:

Test Plan

See that everything works "just right"

Diff Detail

Lint Skipped
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.ids().indexOf( +;
  • 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.

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


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.)


(refs D1684)

  • Changing the maptype will drop all host settings. Only do such things if they actually changed.
  • updateGameAttributes not updateGUIObjects necessary to broadcast g_GameAttributes for multiplayers
  • .select already calls updateGameAttributes (which later calls updateGUIObjects).

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.


quotes around property names

no spaces at the property access


(unneeded braces)


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.


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


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


var g_MapSelected = {

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



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).


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


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


Is it improtant to have a non-linear scale?


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.)


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


missing newline


textcolor, style or gui tags no?


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



  • *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.

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.


.select doesn't call updateGameAttributes


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.


Needs a function not am arrow function.


Not really but linear doesn't mean better.


What do you mean?


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.