Changeset View
Standalone View
binaries/data/mods/public/gui/gamesetup/Pages/MapBrowserPage/MapBrowserPage.js
- This file was added.
/** | |||||
* This class stores the MapBrowserPage controls. | |||||
*/ | |||||
class MapBrowserPageControls | |||||
{ | |||||
elexis: (wrong semicolon) | |||||
} | |||||
/** | |||||
* This class contains all controls modifying the map settings. | |||||
*/ | |||||
SetupWindowPages.MapBrowserPage = class | |||||
{ | |||||
constructor(setupWindow) | |||||
{ | |||||
this.setupWindow = setupWindow; | |||||
this.openPageHandlers = new Set(); | |||||
this.closePageHandlers = new Set(); | |||||
this.mapBrowserPage = Engine.GetGUIObjectByName("mapBrowserPage"); | |||||
this.mapBrowserPageDialog = Engine.GetGUIObjectByName("mapBrowserPageDialog"); | |||||
this.controls = { | |||||
"MapGridBrowser": new MapBrowserPageControls.MapGridBrowser(this, setupWindow) | |||||
}; | |||||
for (let name in MapBrowserPageControls) | |||||
if (!this.controls[name]) | |||||
this.controls[name] = new MapBrowserPageControls[name]( | |||||
this, this.controls.MapGridBrowser, setupWindow); | |||||
} | |||||
registerOpenPageHandler(handler) | |||||
{ | |||||
this.openPageHandlers.add(handler); | |||||
} | |||||
registerClosePageHandler(handler) | |||||
{ | |||||
this.closePageHandlers.add(handler); | |||||
} | |||||
Not Done Inline Actionsthis.mapBrowserPage.onWindowResized = () => { this.gridBrowser.generateGrid(); this.gridBrowser.goToPageOfSelected(); }; This is more logic of the gridBrowser, not of the mapBrowser. (If the user wants to change or extend the behavior, he can still do so, for example by subscribing a custom method or deleting or overwriting the existing) elexis: ```
this.mapBrowserPage.onWindowResized = () =>
{
this.gridBrowser.generateGrid()… | |||||
openPage() | |||||
{ | |||||
for (let handler of this.openPageHandlers) | |||||
Not Done Inline ActionsThe other files in the gamesetup folder have one class per GUI control. Here is one class managing multiple controls, that's coupling, would be better to split. That was the idea of the gamesetup rewrite. So perhaps this.controls = [ new MapBrowserPage.MapSelection(), new MapBrowserPage.MapFilter(), new MapBrowserPage.NextPageButton(), .... ]; or for (let name in MapBrowserPage.Controls) this.controls[name] = new MapBrowserPage.Controls[name](...); Would it be possible to reuse the existing MapType, MapFilter, MapSelection dropdown? Might remove a lot of code duplication, especially the duplicated logic is a bit disadvantageous. By the looks for things perhaps you can use MapBrowserPage.MapSelection = class extends GameSettingControls.MapSelection and overwrite this.onGameAttributesFinalize, onPickRandomItems to be empty (so that its only called once globally) and setControl to assign the GUI Object of the MapBrowserPage. elexis: The other files in the gamesetup folder have one class per GUI control. Here is one class… | |||||
Not Done Inline Actionsthis.mapTypeDropdown.onSelectionChange = () => { many lines} elexis: `this.mapTypeDropdown.onSelectionChange = () => { many lines}`
-> `this.mapTypeDropdown. | |||||
handler(); | |||||
this.mapBrowserPage.hidden = false; | |||||
} | |||||
Not Done Inline ActionsThe onMouseWheelUp / Down logic seems more specific to the GridBrowser, and the GUIObject instance already is passed to the GridBrowser. elexis: The `onMouseWheelUp` / Down logic seems more specific to the GridBrowser, and the GUIObject… | |||||
Done Inline ActionsonMouseWheelDown only works for the grid browser items that are of type "image". Perhaps I made a mistake somewhere or is this dead code? I have to remove it until its implemented in a way that works. (I didn't quickly find a reason in source/gui/ why its hidden, perhaps because the children are ghosts or hidden or whatever) elexis: onMouseWheelDown only works for the grid browser items that are of type "image". Perhaps I made… | |||||
Done Inline ActionsThe tooltip is a lie! elexis: The tooltip is a lie!
MouseWheel does not increase mapsize anywhere!
I suggest to add new… | |||||
closePage() | |||||
Not Done Inline Actions(Look like this.gridBrowser.generateGrid();` call shoudl be in the gridbrowser ctor) elexis: (Look like this.gridBrowser.generateGrid();` call shoudl be in the gridbrowser ctor) | |||||
Not Done Inline ActionsgenerateGrid is probably the most expensive call of the gridbrowser class itself and should only be called when needed (e.g opening the page or on window resize) nani: generateGrid is probably the most expensive call of the gridbrowser class itself and should… | |||||
{ | |||||
for (let handler of this.closePageHandlers) | |||||
handler(); | |||||
Not Done Inline ActionsI guess this is in there to debug. Or if its in there as a feature by design, then it should only be done if g_IsControler, and not on hotload I suppose (since in the other two cases the map had already been selected). elexis: I guess this is in there to debug. Or if its in there as a feature by design, then it should… | |||||
Not Done Inline ActionsYes. Debug. nani: Yes. Debug. | |||||
this.mapBrowserPage.hidden = true; | |||||
} | |||||
Not Done Inline ActionsgetMapsOfTypeFilter is only used four times with gridBrowser.setList and never otherwise, so it can be merged. I so far had bad experience with default arguments, since someone reading no arguments being passed usually expects no arugments to be passed. But if the reader sees that an argument is passed, he will know the argument before looking up the functions code. Also the arguments never change, so the arguments can be removed here. elexis: getMapsOfTypeFilter is only used four times with `gridBrowser.setList` and never otherwise, so… | |||||
}; | |||||
Done Inline Actionsstring literal -> prototype property elexis: string literal -> prototype property | |||||
Done Inline ActionsSomeone might consider recommending to move anonymous functions to prototype functions (This way mods can extend them and it makes the functions containing the functions more lean, easier to follow and reduces coupling. elexis: Someone might consider recommending to move anonymous functions to prototype functions (This… | |||||
Done Inline ActionsIn that case the mod will replace the prototype function that defines the whole childFunction as is not mean to be modable to that degree of precision. nani: In that case the mod will replace the prototype function that defines the whole childFunction… | |||||
Not Done Inline ActionsWhy not provide that freedom? And whats the advantage of defining a function inside a function if one can have all functions as peers inside the same container than nested. To me the latter seemed cleaner and consistently used bind+prototype so far for that reason. Providing code minimization and prototype ('hooks') freedom by default. elexis: Why not provide that freedom? And whats the advantage of defining a function inside a function… | |||||
Done Inline ActionsThe return value is not used, so it might leave shorter code to use () => procedure(), but it's considered misleading (for example in D322 there was a concern for that, and if Im not mistaken there was also some JS linter having a rule for that, although I cant find it exactly in JSHint, nor ESLint except https://eslint.org/docs/2.13.1/rules/no-unused-expressions). elexis: The return value is not used, so it might leave shorter code to use () => procedure(), but it's… | |||||
Done Inline ActionsIt would not be cleaner as the bind wold end looking like this nani: It would not be cleaner as the bind wold end looking like this
``this.GridBrowser.nextPage. | |||||
Not Done Inline ActionsThe argument on the return value is not addressed, https://code.wildfiregames.com/rP19504#inline-304 for example with which I agree. nextPage returns an object, so if you say that its good that the anonymous function makes it impossible that arguments are passed to nextPage, then why is right to pass an object as a return value that is not used. Should be () => { statement(); } then, but that's also kind of not doing much. elexis: The argument on the return value is not addressed, https://code.wildfiregames. | |||||
Done Inline ActionsThis is problematic, because it is the state of the class instance, it shouldnt be a static property (as in not modifying the properties of the prototype, because that is the same for all class instances) So just this.selected = {... in the constructor. elexis: This is problematic, because it is the state of the class instance, it shouldnt be a static… | |||||
Not Done Inline ActionsThe convention of JS GUI class code so far had been that GUI object names only appear in the constructor and are stored as the GUI Object, then the name never being used again anymore. (This especially allows that one can search for GetGUIObjectByName("string") and then find all instances, and that there is no instance where the name doesn't appear around a GetGUIObjectByName. Knowing this to be the case makes the code slightly easier to search) elexis: The convention of JS GUI class code so far had been that GUI object names only appear in the… | |||||
Not Done Inline Actionsmissing semicolon. Can improve performance by using for...of over map (avoiding to create one array, doesnt really matter though) elexis: missing semicolon.
Can improve performance by using for...of over map (avoiding to create one… | |||||
Not Done Inline Actions(Convention is that Engine.GetGUIObjectByName only appears in constructors) elexis: (Convention is that `Engine.GetGUIObjectByName` only appears in constructors) | |||||
Not Done Inline Actions(Engine.GetGUIObjectByName in constructors) elexis: (Engine.GetGUIObjectByName in constructors) | |||||
Not Done Inline ActionsThe hardcoded fallback should be avoided. The recursive onUpdateGameAttributes is meant that settings set their default values by themselves without anything else having to hardcode that. elexis: The hardcoded fallback should be avoided.
The recursive `onUpdateGameAttributes` is meant that… | |||||
Not Done Inline ActionsConvention so far was that prototype properties start with a CapitalLetter / use TitleCase. This also visibly distinguishes them from instance properties that dont do that. elexis: Convention so far was that prototype properties start with a CapitalLetter / use TitleCase. | |||||
Not Done Inline Actions...prototype.HotkeyNext elexis: ...prototype.HotkeyNext | |||||
Not Done Inline ActionschildFunction(child, childIndex, map, mapIndex) does things a constructor typically does, and separating it from the Page controler would allow to extend child functionality outside of the child constructor function. () => could become replaceable / overridable / modifiable / inherited prototype functions of the child class. elexis: `childFunction(child, childIndex, map, mapIndex)` does things a constructor typically does, and… | |||||
Not Done Inline ActionsThe select function hardcodes what happens when the select event is triggered, but instead every of these GUI objects or logical sets of GUI objects (mapdescription objects, selectButton) could be represented by a class that subscribes to the onSelect event (or onFooChange), thus become decoupled from each other. Mods or new features of the MapBrowserPage could then just subscribe to the event as well without having to modify existing code. elexis: The `select` function hardcodes what happens when the select event is triggered, but instead… | |||||
Not Done Inline Actionsdead line of code? onOver isn't an event name that is ever tiggered, is it? I can't find it in source/gui/ elexis: dead line of code? `onOver` isn't an event name that is ever tiggered, is it? I can't find it… | |||||
Not Done Inline ActionsDead code. nani: Dead code. | |||||
Not Done Inline ActionsUsing an event message onOpenPage allows for decoupling and de-hardcoding the event subscribers. The assigned nextPage previousPage hotkeys are an assignmet of the NextPageButton and PreviousPageButton, so this logic piece should be located there. elexis: Using an event message `onOpenPage` allows for decoupling and de-hardcoding the event… | |||||
Not Done Inline Actions// Get maps, go to current gamesetup map and simulate a selection this specifies not few logic of the grid browser, so it should be moved to a gridbrowser function. Can be done with an onOpenPage handler too for instance. elexis: `// Get maps, go to current gamesetup map and simulate a selection` this specifies not few… | |||||
Not Done Inline ActionsEngine.GetGUIObjectByName("mapsSearchBox") occurs in two different files. Ideally every object is referred to and owned by one handler, so it becomes more obvious which handler class has the control / responsibility over a GUI object (the other classes operating on that GUI object then should go through some event-subscription interface to avoid the shotgun antipattern). elexis: `Engine.GetGUIObjectByName("mapsSearchBox")` occurs in two different files. Ideally every… | |||||
Not Done Inline ActionsThe first size is constant / static / not modified by the class instance, so it sounds like a prototype property, but the other width is dependent on the current user selection, which mgiht warrant storing it in two different objects. Though min&max arent modified by the class instance, only value, which would also warrant having min/max on the prototype and the zoom.value on the instance, most easily directly. (this.mapZoom.scale.value -> this.selectedZoom) elexis: The first size is constant / static / not modified by the class instance, so it sounds like a… | |||||
Not Done Inline Actions(Zoom values and logic can be split into a separate class.) elexis: (Zoom values and logic can be split into a separate class.)
| |||||
Not Done Inline ActionsThe maptype should not be stored in the map. elexis: The maptype should not be stored in the map. | |||||
Not Done Inline ActionsWhy not? nani: Why not? | |||||
Done Inline Actionsmap.Description is already stored and translated, at least the one returned by the MapFilters class. elexis: map.Description is already stored and translated, at least the one returned by the MapFilters… |
(wrong semicolon)