Would be nice to see all maps previews in a single page and be able to select or just see them.
Details
- Reviewers
elexis - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP24459: Map browser, used in the gamesetup and in the main menu.
See that everything works "just right"
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- temp
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 14411 Build 30513: arc lint + arc unit
Event Timeline
This seemed to work well on a cursory testing. Filters are functional. I like the design.
UI wise:
- I find the button hard to see against the background, it has rather little contrast compared to dropdown.
- I don't recall if we have a way to switch to a "pointer" cursor but it would be nice. It's not obvious clicking on the map preview opens it up.
- The scrolling is terrible indeed. Maybe we could at least do something so the mouse scrolling isn't so horrible, like incrementing a counter and switching every 10 units?
Design wise:
- I think you've gone overboard with the files tbh. There are a few files that seem to be purely scaffolding, and I don't really see the point of having one file & one class per control over one for the whole component.
- The GridBrowser sounds like a JS-only GUI Object. The approach is likely desirable, but it should probably be moved to common/ or something. I can see, for example, civ selection re-using it.
- Likewise, the input-matching thing sounds like it should be moved, and perhaps C++ could use it for regular dropdown.
Strictly code-wise, I don't really have other particular remarks.
MP path untested.
I very much like this :)
Just a few questions from an ignorant user:
- If you click on a map and press tab again, also the buttons on the back screen are scrolled. (Press and keep scrolls the background but not the foreground.)
- Why are the hotkeys as a tooltip on every map?
- Tab may be used to cycle through the maps and pageup/pagedown for the pages?
- Zoom in/zoom out hotkeyed?
Doesn't need to be? There is a map-tab, right?
you've gone overboard with the files tbh
Yeah, it's ugly to have classes and files with 10 lines. That doesn't mean it's not the better choice to write things split in advance, because the alternative is to merge logically unrelated things which then becomes a pattern and then one ends up in a giant clump of many unrelated things like for instance in the gamesetup.js of 2015. The decision is whether to have one file per class and one class per logically unrelated component or not. If not, then it's arbitrary and usually will spiral out in the long run (which also doesn't mean there wont be people reverting the entire pattern randomly). If the decision is (to have exactly one class per file and exactly one class per logically distinct component), then the worst thing that can happen is having a bunch of files with classes that are easy to understand and extend without growing large.
It seems mostly habituation to argue against small files - but the argument is to split regardless of length but split dependent on logic.
it should probably be moved to common/
The previous reviewer was not convinced, he probably also left remarks about that in related locations.
Code in common/ should be discouraged since it's loaded every time a new page is loaded (including every message box when deleting a unit) and making code slower.
I can see, for example, civ selection re-using it.
I don't. That's the second argument I couldn't endorse common/. Usually GUI pages do their own thing, change behavior etc.. For example a civ selection dialog I would imagine would not unlikely display a description beside the civ etc..
Creating abstract or inherited classes or interfaces is feasible and not creating excess or unrelated traits in the inherited class if one has two actual classes that are implemented.
As in the Danger (here and in general) is that there will be an abstract class that only has one user and can't or will never be used by anything else or that the anything else will be better off with doing it's own thing.
The GridBrowser and MapGridBrowser class were still kept split so that hypothetically it can still be reused and if such a hypothetical case would arise, it can become reused by moving it to some gui/gridbrowser/ folder and left here until then.
That's the part I recall.
Why are the hotkeys as a tooltip on every map?
GUI objects that can be controlled with a hotkey received a tooltip that displays that hotkey, thereby informing the user for every GUI object how it can be controlled.
That's the case in for every hotkey tooltip I know about in gui/ which is why I moved this tooltip from the mappreview or whereever it was to the object that uses the hotkey.
The scrolling is terrible indeed. Maybe we could at least do something so the mouse scrolling isn't so horrible, like incrementing a counter and switching every 10 units?
I don't know the idea you have but nani has a mod/patch that uses JS for scrolling which was not accepted (at least by two reviewers) since the task was meant to be for C++ for cohesion and performance reasons (but perhaps it doesn't matter. On the other hand hiding issues like lack of C++ scrolling by adding JS scrolling will make them reappear later while limiting the gain for a C++ implementation to a performance improvement than a feature implementation).
There were also different camps on the page layout. (I think I had reworked the page so that I would accept it then nani didn't like it anymore, then it was discussed with Vladislav, bb and others, camps built, then nani reworked it it so that he liked it again, then I did something other than this soft ware.)
(Answered the parts which I easily recollect which may be only a subset of parts necessary to comment on)
Code in common/ should be discouraged since it's loaded every time a new page is loaded (including every message box when deleting a unit) and making code slower.
These folders are not loaded recursively, so the point is moot.
the alternative is to merge logically unrelated things which then becomes a pattern and then one ends up in a giant clump of many unrelated things like for instance in the gamesetup.js of 2015.
I don't think the 2015 gamesetup is the logical end of any GUI script. I know "once bitten, twice shy", but I don't think your position is logically sound.
then it's arbitrary
It's not arbitrary, it's based on what seems agreeable to the original coder and possibly to its reviewer.
the worst thing that can happen is having a bunch of files with classes that are easy to understand and extend without growing large.
I certainly disagree that having a bunch of small files in nested folders and such is easy to understand, and further it forces you to write scaffolding code and use retorse naming. Why isn't GameSettingControlBUtton just a button?
I don't think it's enough of a problem that you should change it, but I don't think it should be the pattern going forward either.
I don't think the 2015 gamesetup is the logical end of any GUI script.
2015 gameseutp is the logical end of "historically grown code", i.e. N times just adding one more little thing with never refactoring. The antithesis is to first create a clean structure and then insert into that structure before it can become messy. This has worked well in the simulation entity component system for instance.
I know "once bitten, twice shy", but I don't think your position is logically sound.
If you mean that I want to avoid repeating gamesetup spaghetti code then yes.
Having one class per object means it the reader
- knows directly which file to open
- that it will be only one file to read and edit in relation to that GUI object, rather than 5+ functions spread randomly across the GUI page context (that is true for GUI classes in general with the GameSettingControl an exception due to the inheritance, a complexity that existed before similary)
- allows reading of all of the code operating on one GUI object without having to start seeking
- allows removing of UI objects by deleting only those files and not having to worry about hidden dependencies
- allows modders to mod only the GUI objects they want, only overwrite that one file, only extending that one prototype in a new file etc.
- splitting XML files lessens hardcoding of structure in XML which allows modders to insert GUI in more places without having to overwrite existing XML
I do agree that the many small files are ugly, but I rather have than breaking the convention while gaining the advantage of the files still being clean, easily readable and extensible.
(One can also consider the perf cost of reading multiple files - I suppose in the release it should be negligible due to the zipfile and given that GUI pages such as this one aren't opened so often.)
Why isn't GameSettingControlBUtton just a button?
GameSettingControlButton is an abstract class that inherits the abstract class GameSettingControl.
One can see the code in GameSettingControl.js which features that class provides.
One can see by doing a quick search for GameSettingControl that there are around 50 users of the class (dropdowns and checkboxes),
so that the purpose of not copy&pasting code 50 times as it was the case 2015 is not negligible.
This is just the first button that uses that code / that is present in the list of gamesetting controls.
The code that this button specifically reuses is
- the hotkey tooltip code (inserts the name of the assigned hotkey into the tooltip string)
- hides the GameSettingControl and rearranges other GameSettingControls below the object if it is hidden (I guess this button is never hidden in this revision, at least clients should be able to view maps without being able to select them)
- Assigns the event handlers like onPress, onGameAttributesFinalize etc. automatically if they are present
So indeed it's not necessary to implement the GameSettingControlButton, but it would be necessary if there were multiple buttons in the field where the GameSettingControls are located, around the GameSettingControlDropdown, GameSettingControlCheckbox.
In particular this code:
let row = gameSettingControlManager.getNextRow("buttonSettingFrame"); this.frame = Engine.GetGUIObjectByName("buttonSettingFrame[" + row + "]"); this.button = Engine.GetGUIObjectByName("buttonSettingControl[" + row + "]");
I don't recollect if it was furthermore necessary to have the GameSettingControl class for page layout even if the button is never hidden, and I can't make any statement on this revision of the patch as I didn't read the changes.
Allow me to be facetious
- knows directly which file to open
So long as you know the name of the object. Which, in the case of MapBrowser.js, I would argue is not obvious.
- that it will be only one file to read and edit in relation to that GUI object, rather than 5+ functions spread randomly across the GUI page context (that is true for GUI classes in general with the GameSettingControl an exception due to the inheritance, a complexity that existed before similary)
That seems to merely be an artefact of the spaghetti. You are correct that inheritance forces one to look at, at the very least, 2 files.
- allows reading of all of the code operating on one GUI object without having to start seeking
Only valid if you consider "one button = guiObject" to be a relevant scope. When I'm looking at a filter, I would usually care about what it's filtering. Anything external that gets called means one more file to open.
- allows removing of UI objects by deleting only those files and not having to worry about hidden dependencies
Except when you want to delete the parent class to something inherited 50 times, of course.
Point is, I don't like "one class per object per file" as a rule. I didn't need it on D11 and I find the code there perfectly readable.
Maybe gamesetup is a valid exception because it's quite complex and the options are a good way to make a mess, but as a general rule, meh.
Still on the patch, I further don't see why MapBrowser as a whole is in GameSettings, one could want to open the map browser in a non-gamesetting context.
Tying them together doesn't seem to make much sense a priori.
I have the same complaint about the map preview.
I like this patch a lot. One problem I see is that the maps are sorted by file name instead of map name. There could be any number of reasons why these can be mismatched. Ideally it would sort by map name (taken from the map xml file).
Rebase and cleanup/partial rewrite (mostly merging related files).
I shall only reiterate what I wrote above "I think you went overboard with the files". There were almost exact duplicatas and many related components in separate files for no good reason.
This removes most of the duplication. It cleans up a few things here and there, and abstracts the MapBrowser sufficiently to add it outside the gamesetup, so
I'm adding it next to the struct tree / civ overview.
There are still a few files that could be moved away I think, but overall I'd be happy to commit this version.
Other changes:
- Make the button red. Makes it visible, looks better imo.
- Scrolling changes by one row, making it easier to use imo
- Scrolling via mouse wheel doesn't wrap-around, also making it more usable
- Sort by map name per wow's suggestion above (also in the regular map list).
- Move the mapcache and mapfilters outside of common/gamesetup into a maps/ folder.
Other than that it's the same patch.
We desperately need a way to scroll and some better state handling. We also desperately need a way to emit XML from the JS.
binaries/data/config/default.cfg | ||
---|---|---|
473 | Leftover I guess :) | |
binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/GameSettings/GameSettingControlButton.xml | ||
9 | I wonder why type button is not in the style. Is it separate? | |
binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/Panels/Chat/ChatInputPanel.js | ||
12 | ? | |
binaries/data/mods/public/gui/maps/mapbrowser/Controls/MapDescription.js | ||
6 | That's peculiar? |
binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/Panels/Chat/ChatInputPanel.js | ||
---|---|---|
12 | also leftover :p |
Scrolling is upside down.
You can keep going to the next page in a round-trip, is that intentional?
When going to the map screen from the learn-to-play button cancel has a weird tooltip ^^
A "random" entry would be nice.
A general style comment: full-stops after sentences.
binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/Panels/GameSettingsPanel.xml | ||
---|---|---|
8 | Why four? | |
binaries/data/mods/public/gui/maps/mapbrowser/Controls/MapBrowserControls.js | ||
23 | Member variable? | |
binaries/data/mods/public/gui/maps/mapbrowser/Controls/MapFiltering.js | ||
161 | (This got quite big.) |
binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/GameSettings/GameSettingControlButton.xml | ||
---|---|---|
9 | Not sure what you mean? This style comes from the mod mod | |
binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/Panels/GameSettingsPanel.xml | ||
8 | This comes from the earlier patch... I guess cause better safe than sorry ? I'll switch to 1 with a comment. | |
binaries/data/mods/public/gui/maps/mapbrowser/Controls/MapBrowserControls.js | ||
23 | meh. This needs a 'JS generating XML' refactoring to be clean. | |
binaries/data/mods/public/gui/maps/mapbrowser/Controls/MapFiltering.js | ||
161 | I need to move the abstract components away |
binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/GameSettings/GameSettingControlButton.xml | ||
---|---|---|
9 | I mean that's it's a bit weird that a style called button doesn't define type=button <style name="ModernButtonRed" sprite="ModernButtonRed" sprite_disabled="ModernButtonRedDisabled" sprite_over="ModernButtonRedOver" sprite_pressed="ModernButtonRed" font="sans-bold-stroke-14" textcolor="white" textcolor_disabled="210 210 210 160" text_align="center" text_valign="center" sound_pressed="audio/interface/ui/ui_button_click.ogg" /> |
That's just because it's not inverted. Meh on adding a setting.
You can keep going to the next page in a round-trip, is that intentional?
Yes, it was like that before. I'm not sure if it's good or not.
When going to the map screen from the learn-to-play button cancel has a weird tooltip ^^
Fixed.
A "random" entry would be nice.
Done.
Also touchup a few other things, move the 'components' to another file.
Scrolling upside down is annoying.
With page I expected a whole new page of maps, not just one row shifted.
Hitting <tab> also cycles the options in the background in the gamesetup.
The new random button is nice, but with the random I meant a tile showing ? corresponding to Select map: Random such that players won't know before starting a game what the map is.
(Also it would be nice to use e.g. the arrow keys to change the selection of the map.)
I'm not sure inverting by default is preferable. I would be OK with switching since it's more natural for me, but we'll probably get complaints either way.
With page I expected a whole new page of maps, not just one row shifted.
I haven't really figure a better name, since "row" would be weird when there's nothing to scroll. Ideally I'd just remove all this and put a scrollbar.
Hitting <tab> also cycles the options in the background in the gamesetup.
Eh, good bug. I think 'tab' is a terrible shortcut TBH and would like to remove it.
The new random button is nice, but with the random I meant a tile showing ? corresponding to Select map: Random such that players won't know before starting a game what the map is.
Eh, good point :p
(Also it would be nice to use e.g. the arrow keys to change the selection of the map.)
Annoying to code atm, but I agree.
Fix scrolling and a few other issues. I've removed the 'tab' hotkey which didn't really work and/or was not really that useful.
Adds a "random" option in 'random' map mode (we don't actually allow picking a random skirmish map or scenario).
This needs D3243 to be cleaned up properly, but that's for later.
There is a limit on the decrease map preview size but not on the increase map preview size despite the button not functioning after a while. Else working as advertised. (And a very nice addition.)
I checked the code, mostly looks good. I can't comment on design much.
Just a note that you may want to go over some added comments to check whether they are necessary and/or need some interpunction. (I can inline if you want.)
binaries/data/mods/public/gui/maps/mapbrowser/utils/MatchSort.js | ||
---|---|---|
18 ↗ | (On Diff #14846) | Can't this be inside the class defenition above? |
binaries/data/mods/public/gui/maps/mapbrowser/utils/MatchSort.js | ||
---|---|---|
18 ↗ | (On Diff #14846) | Can't say I really know why it isn't, tbh. These files are inherited from their earlier state. |
binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/GameSettings/Single/Buttons/MapBrowser.js | ||
---|---|---|
14 | Why? (+.) | |
20 | +. | |
binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/Panels/MapPreview.js | ||
20 | Idem as above. | |
binaries/data/mods/public/gui/maps/mapbrowser/controls/MapBrowserControls.js | ||
11 ↗ | (On Diff #14846) | Function already implies that. What do you want to say here? |
25–26 ↗ | (On Diff #14846) | |
binaries/data/mods/public/gui/maps/mapbrowser/controls/MapFiltering.js | ||
24 ↗ | (On Diff #14846) | +. |
55 ↗ | (On Diff #14846) | +. |
binaries/data/mods/public/gui/maps/mapbrowser/controls/Pagination.js | ||
23 ↗ | (On Diff #14846) | ? |
25 ↗ | (On Diff #14846) | Unneeded. |
33 ↗ | (On Diff #14846) | Unneeded. |
binaries/data/mods/public/gui/maps/mapbrowser/grid/GridBrowser.js | ||
2–5 ↗ | (On Diff #14846) | |
13 ↗ | (On Diff #14846) | In JS everything ;) But I guess the three below may not? |
44 ↗ | (On Diff #14846) | +. |
139 ↗ | (On Diff #14846) | Nb? |
binaries/data/mods/public/gui/maps/mapbrowser/utils/Components.js | ||
79 ↗ | (On Diff #14846) | |
binaries/data/mods/public/gui/maps/mapbrowser/utils/MatchSort.js | ||
54 ↗ | (On Diff #14846) | +. |
60 ↗ | (On Diff #14846) | +. |
71 ↗ | (On Diff #14846) | Obvious? |
75 ↗ | (On Diff #14846) | +. |
binaries/data/mods/public/gui/page_mapbrowser.xml | ||
15 | +. | |
binaries/data/mods/public/gui/pregame/MainMenuItems.js | ||
49 | +.? |
binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/GameSettings/Single/Buttons/MapBrowser.js | ||
---|---|---|
14 | (inherited from earlier versions) This is because of the class inheritance. I think we could dust this up further. Comment isn't necessarily useless as it does highlight that this is Working As Designed. | |
20 | (inherited from earlier versions) Not entirely sure what this means tbh. | |
binaries/data/mods/public/gui/maps/mapbrowser/controls/MapBrowserControls.js | ||
11 ↗ | (On Diff #14846) | As opposed to "in another class". Because I felt that made more sense. |
binaries/data/mods/public/gui/maps/mapbrowser/controls/Pagination.js | ||
23 ↗ | (On Diff #14846) | parallels the constructor steps. I guess they're all un-needed then. |
binaries/data/mods/public/gui/maps/mapbrowser/grid/GridBrowser.js | ||
13 ↗ | (On Diff #14846) | (inherited from earlier versions) Yeah this is an "interface" hint. TBH I think it's the wrong design, we should use a naming convention for this, but I don't think it's worth changing. |
139 ↗ | (On Diff #14846) | NB |
binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/GameSettings/Single/Buttons/MapBrowser.js | ||
---|---|---|
14 | I didn't say it was useless, just that the why is more important than anything that can be read from the code. | |
binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/Panels/GameSettingsPanel.xml | ||
8 |
binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/GameSettings/Single/Buttons/MapBrowser.js | ||
---|---|---|
14 | Well this is technically the "why". I guess it's not the why of the why though :P |
binaries/data/mods/public/gui/pregame/MainMenuItems.js | ||
---|---|---|
49 | You're right. |
Comments and MatchSort change. I assume static methods weren't available before the SM upgrade. I could use a static member too but that's experimental and linters complain, so closure it is.
binaries/data/mods/public/gui/maps/mapbrowser/utils/MatchSort.js | ||
---|---|---|
18 ↗ | (On Diff #14846) | Used statically, went for an alternative. |