Changeset View
Standalone View
binaries/data/mods/public/gui/mapbrowser/mapbrowser.js
function GameMap(fileName, type) | |||||
{ | |||||
this.loaded = false; | |||||
this.type = type; | |||||
this.file = { | |||||
"path": GameMap.types[type].path, | |||||
"name": fileName, | |||||
"extension": GameMap.types[type].extension | |||||
elexisUnsubmitted Done Inline Actionselexis: ```
/**
* jsdoc syntax
``` | |||||
}; | |||||
Done Inline Actionsquotes around property names no spaces at the property access elexis: quotes around property names
no spaces at the property access | |||||
} | |||||
GameMap.types = { | |||||
"random": | |||||
Done Inline Actions.prototype? elexis: .prototype? | |||||
Done Inline ActionsIs defined as a static variables (shared in all instances) nani: Is defined as a static variables (shared in all instances) | |||||
Done Inline Actionslike the functions. So might just as well make it consistent? elexis: like the functions. So might just as well make it consistent?
()I recall the `this` keyword is… | |||||
Done Inline ActionsI write it as GameMap.types in an attempt not having to make another global g_GameMapTypes and thus no global scope pollution. nani: I write it as `GameMap.types` in an attempt not having to make another global `g_GameMapTypes`… | |||||
Done Inline ActionsThat's a nice offer, but I was asking for .prototype though. elexis: That's a nice offer, but I was asking for `.prototype` though.
Read: `GameMap.types` ->… | |||||
{ | |||||
"path": "maps/random/", | |||||
"extension": ".json" | |||||
}, | |||||
Done Inline Actionsunneeded parentheses elexis: unneeded parentheses | |||||
"scenario": | |||||
{ | |||||
"path": "maps/scenarios/", | |||||
"extension": ".xml" | |||||
}, | |||||
"skirmish": | |||||
{ | |||||
"path": "maps/skirmishes/", | |||||
"extension": ".xml" | |||||
} | |||||
}; | |||||
GameMap.typesList = Object.keys(GameMap.types); | |||||
Done Inline ActionsDelete this, just use for (let mapTey in this.type) elexis: Delete this, just use `for (let mapTey in this.type)`
Also this looks like a copy, should be… | |||||
GameMap.filters = { | |||||
"default": | |||||
{ | |||||
"name": translateWithContext("map filter", "Default"), | |||||
"tooltip": translateWithContext("map filter", "All maps except naval and demo maps."), | |||||
"filter": mapFilters => mapFilters.every(filter => ["naval", "demo", "hidden"].indexOf(filter) == -1), | |||||
"Default": true | |||||
}, | |||||
"naval": | |||||
{ | |||||
"name": translate("Naval Maps"), | |||||
"tooltip": translateWithContext("map filter", "Maps where ships are needed to reach the enemy."), | |||||
"filter": mapFilters => mapFilters.indexOf("naval") != -1 | |||||
}, | |||||
"demo": | |||||
{ | |||||
"name": translate("Demo Maps"), | |||||
"tooltip": translateWithContext("map filter", "These maps are not playable but for demonstration purposes only."), | |||||
"filter": mapFilters => mapFilters.indexOf("demo") != -1 | |||||
}, | |||||
"new": | |||||
{ | |||||
"name": translate("New Maps"), | |||||
"tooltip": translateWithContext("map filter", "Maps that are brand new in this release of the game."), | |||||
"filter": mapFilters => mapFilters.indexOf("new") != -1 | |||||
}, | |||||
"trigger": | |||||
{ | |||||
"name": translate("Trigger Maps"), | |||||
"tooltip": translateWithContext("map filter", "Maps that come with scripted events and potentially spawn enemy units."), | |||||
"filter": mapFilters => mapFilters.indexOf("trigger") != -1 | |||||
}, | |||||
"all": | |||||
{ | |||||
"name": translate("All Maps"), | |||||
"tooltip": translateWithContext("map filter", "Every map of the chosen maptype."), | |||||
"filter": mapFilters => true | |||||
} | |||||
}; | |||||
Done Inline ActionsThis is too much copypasta, not ok. elexis: This is too much copypasta, not ok.
If I recall correctly I made this JSON by transforming the… | |||||
GameMap.filtersList = Object.keys(GameMap.filters); | |||||
//returns a list of GameMap objects of all maps of given type | |||||
GameMap.getMapsOfType = function (type) | |||||
{ | |||||
let maps = []; | |||||
if (!GameMap.types[type]) | |||||
{ return maps; } | |||||
const filePath = GameMap.types[type].path; | |||||
const fileExtension = GameMap.types[type].extension; | |||||
for (let fileName of listFiles(filePath, fileExtension, false)) | |||||
{ | |||||
if (fileName.startsWith("_")) | |||||
{ continue; } | |||||
maps.push(new GameMap(fileName, type)); | |||||
Done Inline Actionsthis sounds like return this.typesList.map(mapType => mapType.foo) elexis: this sounds like return `this.typesList.map(mapType => mapType.foo)` | |||||
Done Inline ActionsNeeds to expand inner array so won't work but seems [].concat.apply([], arrays) will nani: Needs to expand inner array so won't work but seems `[].concat.apply([], arrays)` will | |||||
Done Inline ActionsDeleting it given that I noticed is not used in the code. nani: Deleting it given that I noticed is not used in the code. | |||||
Done Inline Actionsreturn this.MapTypes.reduce((mapTypes, mapType) => ...concat..., []); elexis: `return this.MapTypes.reduce((mapTypes, mapType) => ...concat..., []);` | |||||
} | |||||
return maps; | |||||
}; | |||||
Done Inline Actionsin and out are the same function, just with a +1 or -1 argument and x = (x + step) % max elexis: in and out are the same function, just with a +1 or -1 argument and x = (x + step) % max | |||||
Done Inline Actionsx = (x + step) % max is a trap -1% 10 = -1 in javascirpt x = ( x + max + step ) % max solves this but I don't want it to be cyclic nani: x = (x + step) % max is a trap
-1% 10 = -1 in javascirpt
x = ( x + max + step ) % max… | |||||
GameMap.getMapsAll = function () | |||||
{ | |||||
let g_mapList = []; | |||||
for (let type of GameMap.typesList) | |||||
g_mapList.push(...GameMap.getMapsOfType(type)); | |||||
return g_mapList; | |||||
}; | |||||
Done Inline ActionsProbably something like mapsFiltered = Object.assign(...) elexis: Probably something like `mapsFiltered = Object.assign(...)` | |||||
Done Inline ActionsCan't find how to make a one liner with Object.assign nani: Can't find how to make a one liner with `Object.assign` | |||||
Done Inline ActionsI never knew about it until bb discovered it and added it in a gamesetup.js. (If I recall correctly theres still an unneeded {} in that call) elexis: I never knew about it until bb discovered it and added it in a `gamesetup.js`. (If I recall… | |||||
Done Inline ActionsIs it really necessary? I looked at the gamesetup.js example and seem overly complex for this case nani: Is it really necessary? I looked at the `gamesetup.js` example and seem overly complex for… | |||||
Done Inline ActionsWe might find out which implementation is nicer after having seen how the line would actually come out. Notice that this code is copy, it shouldn't exist. If it doesn't exist, we don't have to argue how it should be written. But I guess the copy would have to be moved, so can't escape the lint I suppose. Actually I rewrote the function too somewhere and moved it into a prototype. So perhaps we can just exclude these copypasta functions from the review for now. elexis: We might find out which implementation is nicer after having seen how the line would actually… | |||||
/* Returns a dictionary: each key is the filter with its corresponding map list | |||||
* g_mapList is a list of GameMap objects | |||||
*/ | |||||
GameMap.getMapsByFilter = function (g_mapList) | |||||
{ | |||||
let mapsFiltered = {}; | |||||
Done Inline Actions(Probably .filter array function applicable) elexis: (Probably `.filter` array function applicable) | |||||
GameMap.filtersList.forEach((filter) => mapsFiltered[filter] = []); | |||||
for (let map of g_mapList) | |||||
Done Inline Actions("hotkey": "tab.next" support was not committed IIRC meh) elexis: ("hotkey": "tab.next" support was not committed IIRC meh) | |||||
Done Inline ActionsDoes work tho. nani: Does work tho. | |||||
{ | |||||
if (!map.loaded) | |||||
map.load(); | |||||
for (let filter of GameMap.filtersList) | |||||
if (GameMap.filters[filter].filter(map.filter)) | |||||
mapsFiltered[filter].push(map); | |||||
} | |||||
return mapsFiltered; | |||||
Done Inline ActionsThis is copy&pasted from gamesetup.js too, should use a common helper function elexis: This is copy&pasted from gamesetup.js too, should use a common helper function | |||||
}; | |||||
Done Inline Actions(unneeded braces) elexis: (unneeded braces) | |||||
// load data from settings | |||||
GameMap.prototype.load = function () | |||||
{ | |||||
this.loaded = true; | |||||
this.data = (this.type == "random") ? | |||||
Engine.ReadJSONFile(this.file.path + this.file.name + this.file.extension) : | |||||
Engine.LoadMapSettings(this.file.path + this.file.name); | |||||
this.loadName(); | |||||
this.loadDescription(); | |||||
this.loadPreview(); | |||||
this.loadFilters(); | |||||
return this.data; | |||||
}; | |||||
GameMap.prototype.loadName = function () | |||||
{ | |||||
return this.name = (!this.data || !this.data.settings || !this.data.settings.Name) ? | |||||
translate("No map name.") : | |||||
translate(this.data.settings.Name); | |||||
}; | |||||
GameMap.prototype.loadDescription = function () | |||||
{ | |||||
Done Inline Actionsabove 3 functions copies too elexis: above 3 functions copies too | |||||
return this.description = (!this.data || !this.data.settings || !this.data.settings.Description) ? | |||||
translate("No map description.") : | |||||
translate(this.data.settings.Description); | |||||
}; | |||||
Done Inline Actionsdouble negation unneeded? Also I think this could be x && y || [] elexis: double negation unneeded? Also I think this could be x && y || [] | |||||
Done Inline Actionseae pro elexis +1 ffffffff: eae pro elexis +1 | |||||
GameMap.prototype.loadPreview = function () | |||||
{ | |||||
Done Inline ActionsI guess that would be ["all"] instead of [] then? elexis: I guess that would be ["all"] instead of [] then? | |||||
Done Inline ActionsWhat do you mean by that? nani: What do you mean by that? | |||||
Done Inline Actionsthought the same nani +1 eae ffffffff: thought the same nani +1 eae | |||||
Done Inline ActionsThat "all" is already present unless it's an empty array? elexis: That "all" is already present unless it's an empty array? | |||||
const path = "cropped:0.78125,0.5859375:session/icons/mappreview/"; | |||||
Done Inline ActionsCan operate directly on this.filter instead of creating a helper variable. elexis: Can operate directly on `this.filter` instead of creating a helper variable.
Return value… | |||||
Done Inline Actionseae ffffffff: eae | |||||
return this.preview = (!this.data || !this.data.settings || !this.data.settings.Preview) ? | |||||
path + "nopreview.png" : | |||||
path + this.data.settings.Preview; | |||||
}; | |||||
Done Inline ActionsCoding convention is to use g_Foo rather than g_foo elexis: Coding convention is to use g_Foo rather than g_foo
JSDoc | |||||
Done Inline Actionswhat a troll (nani), eae ffffffff: what a troll (nani), eae | |||||
GameMap.prototype.loadFilters = function () | |||||
{ | |||||
Done Inline ActionsThere is a common function for that already. We shouldn't have two of them. The protoype in general seems to have this redundancy problem. elexis: There is a common function for that already. We shouldn't have two of them. The protoype in… | |||||
Done Inline ActionsFor 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. nani: For the record I copy-pasted some code given that i only needed some functionality of game… | |||||
Done Inline ActionsIt should be possible to grab the mappreview and mapfilter ones without messing with the other code? elexis: It should be possible to grab the mappreview and mapfilter ones without messing with the other… | |||||
let filter = !!this.data.settings ? this.data.settings.Keywords || [] : []; | |||||
if (filter.indexOf("all") == -1) | |||||
filter.push("all") | |||||
return this.filter = filter; | |||||
} | |||||
// GridBrowser instance, manages the maps previews | |||||
Done Inline ActionsPrototypes better in their own file (like we have in the simulation and rmgen) elexis: Prototypes better in their own file (like we have in the simulation and rmgen) | |||||
var g_mapBrowser; | |||||
// Dictionary of maps by filter (of current map type list) | |||||
var g_mapList; | |||||
// List of current selected filter from g_mapList | |||||
var g_mapFilter; | |||||
var g_mapFilterDropdown; | |||||
Done Inline Actionslet not in global context. (I heard that should become illegal with future spidermonkey or ES versions) elexis: `let` not in global context. (I heard that should become illegal with future spidermonkey or ES… | |||||
Done Inline Actionsglobal variables start with`g_` http://trac.wildfiregames.com/wiki/Coding_Conventions elexis: global variables start with`g_` http://trac.wildfiregames.com/wiki/Coding_Conventions | |||||
var g_mapSelected = { | |||||
"set": function (map, filter = undefined) | |||||
Done Inline Actionsvar g_MapSelected = { "set": (map, filter) => { .... } }; elexis: var g_MapSelected = {
"set": (map, filter) => {
....
}
};
| |||||
Done Inline ActionsNeeds a function not am arrow function. nani: Needs a function not am arrow function. | |||||
Done Inline Actionsfunction (map, filter = undefined) is synonymous with function (map, filter) and (map, filter) => elexis: `function (map, filter = undefined)` is synonymous with `function (map, filter)` and `(map… | |||||
{ | |||||
this.map = map; | |||||
this.filter = (filter !== undefined) ? filter : g_mapFilterDropdown.list_data[g_mapFilterDropdown.selected]; | |||||
if (!map.loaded) | |||||
map.load(); | |||||
Engine.GetGUIObjectByName("nameSelected").caption = map.name; | |||||
Engine.GetGUIObjectByName("previewSelected").sprite = map.preview; | |||||
Engine.GetGUIObjectByName("descriptionSelected").caption = map.description; | |||||
Done Inline Actionsrefs 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). elexis: refs OOP rewrite. (Having each component decide for itself what it wants to display and which… | |||||
} | |||||
}; | |||||
function childFunction(map, pageIndex, pageList, listIndex, list, childObject) | |||||
{ | |||||
if (!map.loaded) | |||||
map.load(); | |||||
Done Inline Actions"mapPreview[" + index + "]" seems easier (also consistent with the rest of the GUI rather than introducing templates which nothing else in 0ad uses as mentioned) elexis: "mapPreview[" + index + "]" seems easier (also consistent with the rest of the GUI rather than… | |||||
const getObject = (name, index = pageIndex) => Engine.GetGUIObjectByName(name + "[" + index + "]"); | |||||
const mapPreview = getObject("mapPreview"); | |||||
Done Inline ActionsIt removes indirection and became a GUI convention to use the same variable name for the GUI object; i.e. mapButton instead of button. elexis: It removes indirection and became a GUI convention to use the same variable name for the GUI… | |||||
const mapButton = getObject("mapButton"); | |||||
const mapBox = getObject("mapBox"); | |||||
const mapName = getObject("mapName"); | |||||
const animeButtonSettingsSelected = { | |||||
"color": "255 0 0 100", | |||||
Done Inline ActionsSeems a bit more clear with each statement and each property being on a separate line: { "textcolor": { "a": 120 }, "onComplete": () => { Engine.GetGUIObjectByName("descriptionSelected").caption = map.description; } }, elexis: Seems a bit more clear with each statement and each property being on a separate line:
```
{… | |||||
"size": "4 4 -4 -4" | |||||
}; | |||||
const animeButtonSettingsUnselected = { | |||||
"color": "255 0 0 0", | |||||
"size": "8 8 -8 -8" | |||||
}; | |||||
// Draw box outline if this box is the current selected map | |||||
if (g_mapSelected.map.file.name === map.file.name) | |||||
new AnimateGUI(mapButton, animeButtonSettingsSelected); | |||||
else | |||||
new AnimateGUI(mapButton, animeButtonSettingsUnselected); | |||||
mapName.caption = map.name; | |||||
mapPreview.sprite = map.preview; | |||||
mapButton.tooltip = map.description; | |||||
mapButton.onMouseLeftPress = () => | |||||
Done Inline ActionsAbove 4 functions don't seem to have a reason to be exempt from becoming part of new prototype. (other than the callback, ontick and init functions which are hardcoded in C++, yet) elexis: Above 4 functions don't seem to have a reason to be exempt from becoming part of new prototype. | |||||
Done Inline ActionsBeing the take on this what? nani: Being the take on this what? | |||||
{ | |||||
g_mapSelected.set(map); | |||||
// if previously was selected then unselect (meaning it has a sprite) | |||||
if (this.getPageIndexOfSelected() != -1) | |||||
{ | |||||
let lastButtonSelected = getObject("mapButton", this.getPageIndexOfSelected()); | |||||
new AnimateGUI(lastButtonSelected, animeButtonSettingsUnselected); | |||||
Done Inline Actions(Don't mix values and logic; have the caller determine the values where possible) elexis: (Don't mix values and logic; have the caller determine the values where possible) | |||||
Done Inline ActionsDone nani: Done | |||||
} | |||||
new AnimateGUI(mapButton, animeButtonSettingsSelected); | |||||
new AnimateGUI(mapPreview, { | |||||
"size": "3 3 -3 -3", | |||||
"duration": 100 | |||||
}); | |||||
this.setIndexOfSelected(listIndex) | |||||
}; | |||||
mapButton.onMouseLeftRelease = () => | |||||
{ | |||||
new AnimateGUI(mapPreview, { | |||||
"size": "1 1 -1 -1", | |||||
"duration": 100 | |||||
}); | |||||
Done Inline Actionscontinue; elexis: continue; | |||||
}; | |||||
mapButton.onMouseLeftDoubleClick = () => | |||||
{ | |||||
g_mapSelected.set(map); | |||||
this.setIndexOfSelected(listIndex) | |||||
mapBrowserReturn(true); | |||||
}; | |||||
mapButton.onMouseEnter = () => | |||||
{ | |||||
new AnimateGUI(mapBox, { "size": "-10 -10 10 10" }); | |||||
} | |||||
mapButton.onMouseLeave = () => | |||||
{ | |||||
new AnimateGUI(mapBox, { "size": "0 0 0 0" }); | |||||
new AnimateGUI(mapPreview, { | |||||
"size": "1 1 -1 -1", | |||||
"duration": 100 | |||||
}); | |||||
} | |||||
Done Inline ActionsCan create the entire object in one statement: g_MapZoom = { "originalSize": ... "levels": ... "getSize": () => ..., .... } elexis: Can create the entire object in one statement:
g_MapZoom = {
"originalSize": ...
"levels"… | |||||
Done Inline ActionsCan't, given they all depend of each other (JS gives error at creation given that it doesn't know of the other keys) nani: Can't, given they all depend of each other (JS gives error at creation given that it doesn't… | |||||
}; | |||||
var mapZoom = { | |||||
"originalSize": { | |||||
"width": 400, | |||||
"height": 300 | |||||
}, | |||||
Done Inline ActionsIs it improtant to have a non-linear scale? elexis: Is it improtant to have a non-linear scale? | |||||
Done Inline ActionsNot really but linear doesn't mean better. nani: Not really but linear doesn't mean better. | |||||
Done Inline Actions(Guess I was wondering if one could find a formula to compute the numbers rather than hardcoding the numbers) elexis: (Guess I was wondering if one could find a formula to compute the numbers rather than… | |||||
"levels": [0.35, 0.40, 0.45, 0.5, 0.55, 0.6, 0.65, 0.7, 0.8, 0.9, 1, 1.1, 1.2, 1.3, 1.4, 1.5, 1.6, 1.8, 2.0, 2.4, 3.3] | |||||
}; | |||||
mapZoom.selected = mapZoom.levels.indexOf(0.6); | |||||
mapZoom.getLevel = function () | |||||
{ | |||||
return mapZoom.levels[mapZoom.selected]; | |||||
} | |||||
mapZoom.getSize = function () | |||||
{ | |||||
const level = mapZoom.getLevel(); | |||||
return { | |||||
Done Inline Actions(Such helper functions are a bit messy, ideally use a data structure that avoids the need for such transformations. prepareForDropdown is something similar, perhaps the same, I didn't check.) elexis: (Such helper functions are a bit messy, ideally use a data structure that avoids the need for… | |||||
Done Inline ActionsI did it thinking of prepareForDropdown as is takes data from the same source structure and transforms it as a more useful one for this case. nani: I did it thinking of `prepareForDropdown` as is takes data from the same source structure and… | |||||
width: mapZoom.originalSize.width * level, | |||||
height: mapZoom.originalSize.height * level | |||||
}; | |||||
} | |||||
mapZoom.in = function () | |||||
{ | |||||
Done Inline ActionsI suspect these two functions can become one function with an argument that is +1 or -1, and the zoom level foo = (foo + direction) % max elexis: I suspect these two functions can become one function with an argument that is +1 or -1, and… | |||||
Done Inline ActionsImho, in this case is much nicer to have two separate functions for each case. nani: Imho, in this case is much nicer to have two separate functions for each case. | |||||
if (mapZoom.selected == mapZoom.levels.length - 1) | |||||
return; | |||||
++mapZoom.selected; | |||||
let size = mapZoom.getSize(); | |||||
g_mapBrowser.setChildDimensions(size.width, size.height); | |||||
} | |||||
mapZoom.out = function () | |||||
{ | |||||
if (mapZoom.selected == 0) | |||||
return; | |||||
--mapZoom.selected; | |||||
let size = mapZoom.getSize(); | |||||
g_mapBrowser.setChildDimensions(size.width, size.height); | |||||
} | |||||
Done Inline Actions!length !caption.trim() elexis: !length !caption.trim() | |||||
function init(data) | |||||
{ | |||||
// Sets map type, filter depending of data input | |||||
const type = data.mapSelected ? data.map.type : "random"; | |||||
const filter = data.mapSelected ? data.map.filter : "default"; | |||||
// Get all maps names and make lists | |||||
g_mapList = GameMap.getMapsByFilter(GameMap.getMapsOfType(type)); | |||||
g_mapFilter = g_mapList[filter]; | |||||
Done Inline ActionsThose hardcoded magic numbers are faux-pas. It should read something like 4:3, 16:9, 400:300, there is already a function that sets the mappreview image, reusing that would avoid that elexis: Those hardcoded magic numbers are faux-pas. It should read something like 4:3, 16:9, 400:300… | |||||
Done Inline ActionsChanged them as fractions 400/512 300/512 nani: Changed them as fractions 400/512 300/512 | |||||
// Edge case: "random" is not a map | |||||
Done Inline ActionsBut this also deletes the input when the user wants to correct a typo. Perhaps it's nicer to have searchboxes globally consistent (mod page, lobby page for instance) elexis: But this also deletes the input when the user wants to correct a typo. Perhaps it's nicer to… | |||||
Done Inline ActionsIt does not. Is just stores last input the real input is unchanged. When pressed will check to compare the real caption with the this.lastCaption "copy". nani: It does not. Is just stores last input the real input is unchanged. When pressed will check to… | |||||
Done Inline ActionsSounds weird and I didn't try to understand it, but why does this check for mouse presses when the caption doesn't change upon clicks; just subscribe to OnPress? You can find other available events in SendEvent lines in CInput.cpp. For example the new textedit that I already forgot about. elexis: Sounds weird and I didn't try to understand it, but why does this check for mouse presses when… | |||||
Done Inline ActionsOh, didn't know of textedit event. But either way, the map list can change while text still being on the text input (by the dropdowns) so it needs up update the list to the search list if the search box is selected. Maybe would be easier to understand if you try the patch search and use the drop downs at the same time. nani: Oh, didn't know of textedit event. But either way, the map list can change while text still… | |||||
Done Inline ActionsWell I didn't even try to understand yet. Mostly wondering why this has to go through onTick rather than just calling an update function if the event of your needs is detected? elexis: Well I didn't even try to understand yet. Mostly wondering why this has to go through `onTick`… | |||||
const validMapSelected = data.mapSelected && data.map.name != "random"; | |||||
// Map name from gamesetup has path included | |||||
const fullMapName = validMapSelected ? data.map.name : g_mapFilter[0].file.path + g_mapFilter[0].file.name; | |||||
let mapSelectedIndex = g_mapFilter.map(map => map.file.path + map.file.name).indexOf(fullMapName); | |||||
// Get childen size (with zoom set) | |||||
const childSize = mapZoom.getSize(); | |||||
// Set the first selected map so childFunction doesn't read an undefined value from mapSelected | |||||
g_mapSelected.set(g_mapFilter[mapSelectedIndex], filter); | |||||
// Create maps' grid | |||||
g_mapBrowser = new GridBrowser("MapBrowserContainer", "currentPageNum", g_mapFilter, mapSelectedIndex, childSize.width, childSize.height, childFunction); | |||||
Done Inline Actions!caption === over == only when the type check actually makes a difference elexis: !caption
`===` over `==` only when the type check actually makes a difference | |||||
// Map filter type dropdown: set list, currently selected and behaviour | |||||
g_mapFilterDropdown = Engine.GetGUIObjectByName("mapFilterDropdown"); | |||||
g_mapFilterDropdown.list = GameMap.filtersList.map(filter => GameMap.filters[filter].name); | |||||
g_mapFilterDropdown.list_data = GameMap.filtersList; | |||||
Done Inline Actions(!caption) elexis: (!caption) | |||||
Done Inline Actions+1 eae ffffffff: +1 eae | |||||
Done Inline ActionsAs is comparing strings I think is more clear the comparison. nani: As is comparing strings I think is more clear the comparison. | |||||
Done Inline Actionselexis means its done his way everywhere in the code bc its shorter eae ffffffff: elexis means its done his way everywhere in the code bc its shorter eae | |||||
Done Inline Actions
Same is true for const in local scope btw. elexis: > elexis means its done his way everywhere in the code bc its shorter eae
* (not my way, I… | |||||
Done Inline Actions
so it became ur way eae ffffffff: > (not my way, I learnt it from others too)
so it became ur way eae | |||||
g_mapFilterDropdown.selected = g_mapFilterDropdown.list_data.indexOf(filter); | |||||
g_mapFilterDropdown.onSelectionChange = () => | |||||
{ | |||||
const filter = g_mapFilterDropdown.list_data[g_mapFilterDropdown.selected]; | |||||
g_mapFilter = g_mapList[filter]; | |||||
g_mapBrowser.setList(g_mapFilter); | |||||
g_mapBrowser.goToPage(0); | |||||
Done Inline Actionsfor (let filter in g_Maps.filters) elexis: for (let filter in g_Maps.filters) | |||||
}; | |||||
// Map type dropdown: set list, currently selected and behaviour | |||||
const mapTypeDropdown = Engine.GetGUIObjectByName("mapTypeDropdown"); | |||||
mapTypeDropdown.list = g_Settings.MapTypes.map(type => type.Title); | |||||
mapTypeDropdown.list_data = g_Settings.MapTypes.map(type => type.Name); | |||||
mapTypeDropdown.selected = mapTypeDropdown.list_data.indexOf(type); | |||||
Done Inline ActionsDidn't check if it works, but maybe concat elexis: Didn't check if it works, but maybe concat | |||||
mapTypeDropdown.onSelectionChange = () => | |||||
{ | |||||
const type = mapTypeDropdown.list_data[mapTypeDropdown.selected]; | |||||
const filter = g_mapFilterDropdown.list_data[g_mapFilterDropdown.selected]; | |||||
Done Inline Actions(These two statements on a single line. We try to avoid .foo on a separate line, or rather wanting to avoid x = foo leaving a remote impression of a missing semicolon / being a complete statement) elexis: (These two statements on a single line. We try to avoid `.foo` on a separate line, or rather… | |||||
g_mapList = GameMap.getMapsByFilter(GameMap.getMapsOfType(type)); | |||||
g_mapFilter = g_mapList[filter]; | |||||
g_mapBrowser.setList(g_mapFilter); | |||||
g_mapBrowser.goToPage(0); | |||||
}; | |||||
// Zoom buttons | |||||
Engine.GetGUIObjectByName("mapsZoomIn").onPress = mapZoom.in; | |||||
Engine.GetGUIObjectByName("mapsZoomOut").onPress = mapZoom.out; | |||||
// Tooltips | |||||
let goldenText = (text) => "[color=\"255 251 131\"]" + text + "[/color]"; | |||||
Engine.GetGUIObjectByName("mapsZoomIn").tooltip = translate(goldenText("\\[MouseWheelUp]") + ": Make map previews bigger."); | |||||
Engine.GetGUIObjectByName("mapsZoomOut").tooltip = translate(goldenText("\\[MouseWheelDown]") + ": Make map previews smaller?"); | |||||
Done Inline ActionsStructure 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.) elexis: Structure the code in a way that every function has one designated purpose. For procedural code… | |||||
Engine.GetGUIObjectByName("prevButton").tooltip = colorizeHotkey(translate("%(hotkey)s: Goes to previous page."), "tab.prev"); | |||||
Done Inline Actionsinit function should be the first one within a file elexis: `init` function should be the first one within a file | |||||
Engine.GetGUIObjectByName("nextButton").tooltip = colorizeHotkey(translate("%(hotkey)s: Goes to next page."), "tab.next"); | |||||
Engine.GetGUIObjectByName("selectMapButton").tooltip = translate("Select map."); | |||||
Engine.GetGUIObjectByName("closeButton").tooltip = colorizeHotkey(translate("%(hotkey)s: Close map browser."), "cancel"); | |||||
} | |||||
function mapBrowserReturn(sendSelected = false) | |||||
{ | |||||
Done Inline ActionsInstead of passing a boolean that something was selected, it can pass the data only if it should be changed. elexis: Instead of passing a boolean that something was selected, it can pass the data only if it… | |||||
// Pop the page before calling the callback, so the callback runs | |||||
// in the parent GUI page's context | |||||
const data = sendSelected ? | |||||
{ | |||||
"mapSelected": true, | |||||
"map": { | |||||
"path": g_mapSelected.map.file.path, | |||||
"name": g_mapSelected.map.file.name, | |||||
"type": g_mapSelected.map.type, | |||||
"filter": g_mapSelected.filter | |||||
} | |||||
} : | |||||
{}; | |||||
Engine.PopGuiPageCB(data); | |||||
} | |||||
function onTick() | |||||
{ | |||||
AnimateGUI.onTick(); | |||||
} | |||||
Done Inline Actionsmissing newline elexis: missing newline | |||||
Done Inline Actionshere you assign a lot of JS code to GUI Objects, why not also assign the other onFoo events from the XML code? elexis: here you assign a lot of JS code to GUI Objects, why not also assign the other onFoo events… | |||||
Done Inline ActionsDone. Left <action>'s that are inside the <repeat> section nani: Done. Left <action>'s that are inside the <repeat> section | |||||
Done Inline Actionsmaybe he reuses initAction function in code? :-) ffffffff: maybe he reuses initAction function in code?
:-) | |||||
Done Inline Actions/** * Avoid comments if they don't really add information that isn't obvious enough from the code. * Otherwise use JSdoc for functions, so one can build a html doc and have editors use the information. */ elexis: ```
/**
* Avoid comments if they don't really add information that isn't obvious enough from… | |||||
Done Inline ActionssetGUITags(text, { "color": foo }) elexis: setGUITags(text, { "color": foo }) | |||||
Done Inline Actionspro :-) ffffffff: pro
:-) | |||||
Done Inline ActionsThis is a nice way to put it, but even better would be grouping it per object: "guiObject1": { "label": "foo" "tooltip": "bar", "onPress": () => { }, } Like in gamesetup.js for example. This way if one wants to introduce, remove or modify an object, there would only be a single hunk of the file to edit and all behaviors and properties of the GUI object are seen in a single place rather than spread across the file. elexis: This is a nice way to put it, but even better would be grouping it per object:
```… | |||||
Done Inline Actionstotal better but more to understand the code then :-) ffffffff: total better but more to understand the code then
:-) | |||||
Done Inline Actionscant tooltips var be killed? eae ffffffff: cant tooltips var be killed? eae | |||||
Done Inline ActionsonTab = nextPage elexis: onTab = nextPage | |||||
Done Inline Actions+1 :-) ffffffff: +1 :-)
if g_mapBrowser.nextPage() is already defined same for lines above :-) | |||||
Done Inline ActionsEngine.GetGUIObjectByName("nextButton").onPress = () => g_mapBrowser.nextPage(); should either be Engine.GetGUIObjectByName("nextButton").onPress = g_mapBrowser.nextPage; or in general, if the return value is unused: Engine.GetGUIObjectByName("nextButton").onPress = () => { g_mapBrowser.nextPage(); }; elexis: ```
Engine.GetGUIObjectByName("nextButton").onPress = () => g_mapBrowser.nextPage();
```… | |||||
Done Inline Actionscan be inlined elexis: can be inlined | |||||
Done Inline ActionsKeep it bugged and add it to the ticket. I think causative created one for the lobby hotkey producing the chat "l" key. elexis: Keep it bugged and add it to the ticket. I think causative created one for the lobby hotkey… | |||||
Done Inline Actionspro ffffffff: pro | |||||
Done Inline Actionsfoo["bar"] = foo.bar elexis: foo["bar"] = foo.bar | |||||
Done Inline ActionsThis looks like it can't work, because it misses either a "return" keyword before "map1" or, preferably needs the braces removed. elexis: This looks like it can't work, because it misses either a "return" keyword before "map1" or… | |||||
Done Inline ActionsRemoved braces. nani: Removed braces.
But I still think having each part separated makes it more digestible. |