Page MenuHomeWildfire Games

Map browser for gamesetup
Needs ReviewPublic

Authored by nani on Dec 23 2018, 11:49 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

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.

Test Plan

See that everything works "just right"

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
nani marked 32 inline comments as done.Feb 7 2019, 2:49 PM
nani added inline comments.
binaries/data/mods/public/gui/mapbrowser/mapbrowser.js
13

Is defined as a static variables (shared in all instances)

89

Needs to expand inner array so won't work but seems [].concat.apply([], arrays) will

89

Deleting it given that I noticed is not used in the code.

100

Can't find how to make a one liner with Object.assign

152

What do you mean by that?

266

Can't, given they all depend of each other (JS gives error at creation given that it doesn't know of the other keys)

291

Imho, in this case is much nicer to have two separate functions for each case.

317

It 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".

336

As is comparing strings I think is more clear the comparison.

443

Done. Left <action>'s that are inside the <repeat> section

binaries/data/mods/public/gui/mapbrowser/mapbrowser.xml
60

Needed. Also much more easy to change.

nani marked 16 inline comments as done.Feb 7 2019, 2:52 PM
ffffffff added inline comments.Feb 7 2019, 2:56 PM
binaries/data/mods/public/gui/mapbrowser/mapbrowser.js
152

thought the same nani +1 eae

336

elexis means its done his way everywhere in the code bc its shorter eae

binaries/data/mods/public/gui/mapbrowser/mapbrowser.xml
60

if then pro :-)

nani marked an inline comment as done.Feb 7 2019, 3:11 PM
elexis added inline comments.Feb 7 2019, 3:52 PM
binaries/data/mods/public/gui/mapbrowser/mapbrowser.js
13

like the functions. So might just as well make it consistent?
()I recall the this keyword is sometimes pointing to unexpected objects (different callers).

89

return this.MapTypes.reduce((mapTypes, mapType) => ...concat..., []);

100

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

152

That "all" is already present unless it's an empty array?

317

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

336

elexis means its done his way everywhere in the code bc its shorter eae

  • (not my way, I learnt it from others too)

As is comparing strings I think is more clear the comparison.

  • What information does caption === "" carry that !caption does not carry? The stict equality test caption === "" also leads the reader to question whether it is important to distinguish between === "" and == "" and falsy values. Notice that caption === "" does actually not inform the reader whether caption is a string.
  • the codebase should be consistent, we don't want to see a different way of writing code when opening a different file. So if the argument is in favor of === over == then this should be changed in all files, not only new code. If it doesn't really matter which one we chose, why not chose the more globally consistent variant over the personal preference variant?

Same is true for const in local scope btw.

444
	Engine.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();
        };

(See https://code.wildfiregames.com/rP19504#inline-304 )

482

can be inlined

binaries/data/mods/public/gui/mapbrowser/mapbrowser.xml
36

That's 100 maps max? That sounds almost too easy to reach if we already have like 70 random maps in the repository.

55

Why are there so many MouseWheelDown / Up subscribers all doing the same thing, rather than only one global subscriber?

60

<object size="100%-420 15 100% 100%-15" type="image" sprite="ModernFade" z="50">
only contains one child:
<object size="20 20 100%-20 100%-20">
or did my eyes betray me?
so why not add the numbers up? I guess it's because the frame would be 20px off.

nani marked 20 inline comments as done.Feb 7 2019, 5:02 PM
nani added inline comments.
binaries/data/mods/public/gui/mapbrowser/mapbrowser.js
13

I write it as GameMap.types in an attempt not having to make another global g_GameMapTypes and thus no global scope pollution.
But if you want consistency I can also write it as g_GameMapTypes

100

Is it really necessary? I looked at the gamesetup.js example and seem overly complex for this case

317

Oh, 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.

binaries/data/mods/public/gui/mapbrowser/mapbrowser.xml
36

That number is the maximum to show at the same time, remember that the map browser is paged. There can be as many maps as you want.

55

Each map must be selectable but I also want to be able to use scroll on top of the maps, their padding and the border of the grid. Not possible otherwise (that I know of).

ffffffff added inline comments.Feb 7 2019, 5:37 PM
binaries/data/mods/public/gui/mapbrowser/mapbrowser.js
336

(not my way, I learnt it from others too)

so it became ur way eae

nani updated this revision to Diff 7473.Feb 8 2019, 8:00 PM
nani marked 3 inline comments as done.

@elexis changed code to reflect your comments and will refractor GameMap function if you think the rest of the code looks ok.

elexis added a comment.Feb 8 2019, 8:03 PM

I noticed from the screenshot that you use different button styles. They should be the same theme on the entire page. I favored the Stone buttons too (replay menu), but they are considered deprecated and should be superseded by the Red ones, and we should aim at making the GUI theme globally consistent. The Stone buttons on the other pages except for the menu buttons will probably be replaced. At least that was the plan few years ago.

binaries/data/mods/public/gui/mapbrowser/mapbrowser.js
13

That's a nice offer, but I was asking for .prototype though.
Read: GameMap.types -> GameMap.prototype.MapTypes.
Hypothetically speaking, since this variable is a copy from gamesetup that should be avoided, at least for vanilla.

100

We might find out which implementation is nicer after having seen how the line would actually come out.
I think Object.assign just works like concat, but for objects instead of arrays, so quite easy.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign

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.

107

(Probably .filter array function applicable)

317

Well 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?

490

Keep it bugged and add it to the ticket. I think causative created one for the lobby hotkey producing the chat "l" key.

binaries/data/mods/public/gui/mapbrowser/mapbrowser.xml
36

So it's the number of maps that can be displayed simultaneously.
That's 10x10 or 13*8 or 11*9. Wondering if this is sufficient for all screensizes.400px width, I guess it is.
(One could also try to compute whether it's vastly too many. Or do other things in life.)

55

I think global hotkeys are triggered as long as no text input is focused.
This also brings us to the fact that not using hotkeys forces the user to use this one particular button that the developer liked best but that might not be present on the users hardware or not the users choice (ctrl and +).

(This could also be problematic if a user assigned a GUI hotkey to the mousewheel events. That might be more realistic in the future, for example if we had hotkeys to increase the entire UI scale #5389.)

ffffffff added inline comments.Feb 8 2019, 8:24 PM
binaries/data/config/default.cfg
328

why not M

binaries/data/mods/public/gui/gamesetup/gamesetup.js
1169

typo browser

binaries/data/mods/public/gui/mapbrowser/mapbrowser.js
490

pro

nani marked 5 inline comments as done.Feb 8 2019, 8:37 PM
nani added inline comments.
binaries/data/config/default.cfg
328

O = for open ( map browser) also closer to Enter (more comfortable) if quick search.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
1169

Yep

ffffffff added inline comments.Feb 8 2019, 9:33 PM
binaries/data/config/default.cfg
328

intuitive with english language more important babe </3

nani updated this revision to Diff 7475.Feb 9 2019, 6:06 PM
nani marked 2 inline comments as done.
nani marked 2 inline comments as done.Feb 9 2019, 6:10 PM
nani added inline comments.
binaries/data/mods/public/gui/mapbrowser/mapbrowser.xml
55

I view onMouseWheel events like analogous to onPress onRelease given that they are mouse events. BTW I don't see any onPress onRelease options in the default.cfg config.

nani updated this revision to Diff 7482.Feb 10 2019, 8:46 PM
nani edited the summary of this revision. (Show Details)

@elexis I think is pretty much done now. Care to confirm? Thanks.

nani marked 29 inline comments as done.Feb 10 2019, 8:51 PM
nani marked 2 inline comments as done.

Code looks much better than what I remember from last revision. (I only did a superficial glance however and didn't pay the patch the attention that it deserves.)

binaries/data/mods/public/gui/gamesetup/gamesetup.xml
149

OnPress?
Also that JS goes to g_MiscControls (primarily because all of the JS is supposed to be there, but also the dialog should be refreshed eventually once the settings update, so there will be more JS for that button to come, similar to the AI dialog "aiconfig")

binaries/data/mods/public/gui/mapbrowser/mapbrowser.js
8
/**
 * jsdoc syntax
17

unneeded parentheses

92

in and out are the same function, just with a +1 or -1 argument and x = (x + step) % max

109

("hotkey": "tab.next" support was not committed IIRC meh)

223

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

231

(Don't mix values and logic; have the caller determine the values where possible)

247

continue;

285

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

315

Those 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

343

for (let filter in g_Maps.filters)

403

foo["bar"] = foo.bar

410

This looks like it can't work, because it misses either a "return" keyword before "map1" or, preferably needs the braces removed.
Both functions can be inlined

binaries/data/mods/public/gui/mapbrowser/mapbrowser.xml
113

Usually GUI objects are specified with fixed width instead of relative width, so that we don't have 'degenerate shapes' (buttons that are 500px wide but only 16px high and similar), and so that the borders / margings between elements remain fixed. Would have to look in specific where it's good or bad to have it relative.

nani marked 16 inline comments as done.Feb 17 2019, 12:41 AM
nani added inline comments.
binaries/data/mods/public/gui/mapbrowser/mapbrowser.js
92

x = (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

109

Does work tho.

223

Being the take on this what?

231

Done

285

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

315

Changed them as fractions 400/512 300/512

410

Removed braces.
But I still think having each part separated makes it more digestible.

binaries/data/mods/public/gui/mapbrowser/mapbrowser.xml
113

Parent object is fixed size so it doesn't matter.

nani marked 8 inline comments as done.Feb 17 2019, 12:42 AM
nani updated this revision to Diff 7490.Feb 17 2019, 12:47 AM