Map type path data and filter should be in settings.js
Needed for D1703: Map browser for gamesetup
- Group Reviewers
Restricted Owners Package (Owns No Changed Paths)
Map type path data and filter should be in settings.js
Keep same behaviour as before.
Unit Tests Skipped
- MapFilter: Wondering if this should remain in gamesetup/, since the purpose of settings.js to contain data that is used by multiple GUI pages. But it seems to pass that test, because similarly to the other data in settings.js, this could be used for example in the replay menu or lobby to filter games by maps too (at least if the map exists on the local computer). As mentioned in D1703, it would be better to specify the mapfilters JSON (using MatchClasses syntax) rather than JS. But I have a patch for that somewhere, and it could be considered independent.
- MapType: Good idea, it never occured to me!
D2483 has the diff that moves the mapfilters to a JSON file using MatchesClasses syntax for filtering.
I wonder if it wouldn't be cleaner to
(2) delete gui/common/settings.js, move the strings all to JSON files, make the GUI pages load and process the JSON files instead of using this function, refactor+move the loadSettingValuesFile function to l10n.js, and the translate[Setting] functions to gamedescription.js instead of adding more code in gui/common/ and
If Im not mistaken you have a "MapCache" class thing too somewhere. If the MapBrowser would use that, it would have to be in gui/common/ as well. (But gui/common/ is an anti-pattern for code, should be minimized, because it is loaded in every GUI page but most code is only relevant for 2-3 GUI pages. So one ends up mixing GUI page code of different pages.)
So it took me a while to determine whether or not it would be reasonable to move the mapFilters and mapType data from gui/gamesetup/ to gui/common/.
There are strong objections to moving anything to gui common:
- gui/common/ should be empty:
- Currently most (?) functions of gui/common/ are used by only 2-3(?) of the 20+ GUI pages.
- That means that there is bad coupling, functions and files that have no real relation to each other are put side by side.
- It also means that every GUI page loads a ton of code that it never uses!
- Therefore `gui/common/ must receive as little content as possible.
- As most functions only relate to 2-3 GUI pages, most of the content could actually be removed, meaning the directory could become nearly empty and the code only present in the GUI pages that use it (usually by redesigning the affected code).
- Since gui/common/ should become empty, addition of each logical piece would be a step in the opposite of the desired direction and thus should be avoided.
- The use case does not require moving it to gui/common/:
- The proposed use case is Needed for D1703: Map browser for gamesetup, but that is not correct if the MapBrowser does not open a new GUI page but runs in the same context.
- Running the MapBrowser in a new GUI page seems clean, because it decouples the two GUI pages. Someone can learn from the fact that the two GUI pages are in two different directories that there is no communication between the two gui pages.
- However, running the MapBrowser in the same GUI page would improve performance because they can then reuse the same MapCache.
- Running the gamesetup dialog (MapBrowser or AI config page) in the same GUI page allows interaction of the gamesetup page and the dialog. For the AI config page this may be crucial, because currently the page displays g_GameAttributes and that is modified by the parent GUI page. Thus, currently the AIconig page is running in the separate context and updated by closing it and reopning the dialog. That's a very ugly workaround. I once wrote a patch to have one GUI page update the other GUI page, but that may still be inadequate for the future. With D2483 and new features to the AI config page (selecting per-player starting resources, regicide starting hero etc.), there will likely even be more coupling (for example accessing MapCache, GameSettingsControl, PlayerAssignmentsControl, NetMessages etc.) in the gamesetup page from the ai/player-config page. So for the aiconfig page it would perhaps be warranted if not necessary to open it in the same GUI page. The MapBrowser has an similar use case, even though it's not so strong: It can display the map that the host currently selected, so that players that browse the maps are informed of what the host changed and are able to agree or disagree with that without further delay. (Not sure how it would be presented best, perhaps two map previews if there is enough screenspace). If that was implemented, it would probably be better to run it in the same GUI page context (aside from the MapCache advantage).
- Use cases for mapTypes / mapFilter in gui/common/:
- I noticed the getMapDescriptionAndPreview function of gamedescription.js also hardcodes the information of the file-ending per map-type, so the information already is present in gui/common/ and can just be reordered. Therefore moving mapType over is correct even if the proposed reason would not hold up.
- The replay menu has a remotely conceivable use case to consume the mapfilters, so that players can filter all replays of naval maps or trigger maps for instance. This has the disadvantage that either all map files of all replays need to be loaded when opening the menu page (which may be negligible overhead in comparison to loading all replays), or that the Keywords would have to be stored to the commands.txt header. However that use case is hypothetical right now.
The decision whether or not to move will have the following impact:
- D2483 changes this code, so either it does it correctly on first try or it needs a second commit (changeset minimization)
- If one moves strings from one folder to the other, translators of every translated language may have to translate these strings from scratch again.
So I conclude that
- the mapType data should be moved, used in common/gamedescription.js
- the mapFilter.json from D2483 will remain in gamesetup/ until there is an actual use case for the strings to be in common/. If there is, it will only be a renamed file with no lines changed, which can be performed in the same diff.
- Moving the MapType over, same as your diff for that
- Moving the MapCache to global, using it in every GUI page that opens maps.
- Not moving the MapFilters over (since it would drop existing translations and it seems more preferable to not open the MapBrowser in a custom page as elaborated in the last comment; unless there is a new, stronger reason identified)