Page MenuHomeWildfire Games

Move Mainmenu from XML to JS and use object oriented JS
ClosedPublic

Authored by elexis on Aug 31 2019, 8:25 PM.

Details

Reviewers
None
Commits
rP22854: Rewrite Main Menu.
Trac Tickets
#5387
Summary

Everytime someone wants to change the main menu, add an item, a lot of intransparent XML code has to be changed, with arguments in weird places, hardcoded number of menu items (requiring the author to keep multiple places in sync with the correct magic numbers).
This patch was written first in October 2017 in https://code.wildfiregames.com/D820#37188 already, but was never finished; for #5387 I changed it a bit.

Test Plan

Notice how one can currently trigger crashes by throwing a JS exception on "init".
Consider whether or not to use the class keyword, especially for those trival handlers that dont have prototype extensions.
Consider the use cases of modders and other code reworkers.
Consider the logical grouping of code.
Consider the filenaming, especially the casing.
Consider whether to create subfolders (for example background JS+XML in a separate folder).
Compare previous vs new main menu and notice the main menu buttons have like 4px less margin or something and consider who cares.
Ensure that menu unfolding has the same behavior as before.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
elexis added inline comments.Aug 31 2019, 8:35 PM
binaries/data/mods/public/gui/pregame/MainMenuButtonHandler.js
17 ↗(On Diff #9554)

onPress = this.closeSubmenu won't work due to that being called with this = calling GUI object.

binaries/data/mods/public/gui/pregame/MainMenuItems.js
2

Was thinking about making this a JS Object, but it is important that this is iterated correctly.

A for...in loop iterates over the properties of an object in an arbitrary order

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in

Even if the order was/is deterministic, for example insertion order or alphabetic order, it would be unintentional, because the order of items should be influenced by how the menu author wants.

So { "singleplayer": { ... } } just wouldn't work. If it was insertion order, then a modder couldnt insert in between.

Such an id could be added for modders, if a mod wants to insert a submenu just after "Singleplayer" for example.

binaries/data/mods/public/gui/pregame/mainmenu.js
40

JS function with 2 statements pretending to be a class, but not visible as such to the reader on first sight, if it wasnt for the name and surroinding consistency.

The idea of doing OOP everywhere is to stop people from writing procedural code, but this example invites people to write more procedural code.

That's why I wonder whether the class keyword shouldn't be used eventually. Perhaps even now?
I really didnt like the +1 tab.

binaries/data/mods/public/gui/pregame/mainmenu.xml
14

onTick needed some C++ patch to be assignable from JS IIRC

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/gui/GUIManager.h
|  47| class·CGUIManager
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGUIManager{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 183| 183| 		"tooltip": translate("Exits the game."),
| 184| 184| 		"onPress": () => {
| 185| 185| 			messageBox(
| 186|    |-					400, 200,
|    | 186|+				400, 200,
| 187| 187| 					translate("Are you sure you want to quit 0 A.D.?"),
| 188| 188| 					translate("Confirmation"),
| 189| 189| 					[translate("No"), translate("Yes")],
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 184| 184| 		"onPress": () => {
| 185| 185| 			messageBox(
| 186| 186| 					400, 200,
| 187|    |-					translate("Are you sure you want to quit 0 A.D.?"),
|    | 187|+				translate("Are you sure you want to quit 0 A.D.?"),
| 188| 188| 					translate("Confirmation"),
| 189| 189| 					[translate("No"), translate("Yes")],
| 190| 190| 					[null, Engine.Exit]);
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 185| 185| 			messageBox(
| 186| 186| 					400, 200,
| 187| 187| 					translate("Are you sure you want to quit 0 A.D.?"),
| 188|    |-					translate("Confirmation"),
|    | 188|+				translate("Confirmation"),
| 189| 189| 					[translate("No"), translate("Yes")],
| 190| 190| 					[null, Engine.Exit]);
| 191| 191| 		}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 186| 186| 					400, 200,
| 187| 187| 					translate("Are you sure you want to quit 0 A.D.?"),
| 188| 188| 					translate("Confirmation"),
| 189|    |-					[translate("No"), translate("Yes")],
|    | 189|+				[translate("No"), translate("Yes")],
| 190| 190| 					[null, Engine.Exit]);
| 191| 191| 		}
| 192| 192| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 187| 187| 					translate("Are you sure you want to quit 0 A.D.?"),
| 188| 188| 					translate("Confirmation"),
| 189| 189| 					[translate("No"), translate("Yes")],
| 190|    |-					[null, Engine.Exit]);
|    | 190|+				[null, Engine.Exit]);
| 191| 191| 		}
| 192| 192| 	}
| 193| 193| ];
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/mainmenu.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/mainmenu.js
|  13|  13| 	g_BackgroundHandler = new BackgroundHandler(pickRandom(g_BackgroundLayerData));
|  14|  14| 	g_SplashScreenHandler = new SplashScreenHandler(data, hotloadData);
|  15|  15| 
|  16|    |-	//throw new Error("This will crash, not cool");
|    |  16|+	// throw new Error("This will crash, not cool");
|  17|  17| 
|  18|  18| 	MusicHandler();
|  19|  19| 	VersionHandler();
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/527/display/redirect

elexis added inline comments.Aug 31 2019, 9:53 PM
binaries/data/mods/public/gui/pregame/mainmenu.js
5–6

Also happens in a23 #5578.
Might be the reason why the splashscreen was originally opened from onTick rather than init.

nani awarded a token.Aug 31 2019, 10:00 PM
nani added a subscriber: nani.Aug 31 2019, 10:05 PM
Krinkle added inline comments.Sep 1 2019, 1:34 AM
binaries/data/mods/public/gui/pregame/MainMenuButtonHandler.js
17 ↗(On Diff #9554)

I think using an an arrow function to wrap is is quite elegant and readable.

Note that if it's perf-critical, an alternative could be this.closeSubmenu.bind(this) - using the Function#bind method. I don't know about SM and our version of it in particular, but on some engines that can be faster.

binaries/data/mods/public/gui/pregame/SplashscreenHandler.js
42

I think an arrow function would suffice here.

nani added a comment.Sep 1 2019, 1:39 PM

Parallax scrolling is not working (BackgroundHandler).
Open prelobby page from mainmenu hotkey doesn't work anymore.

binaries/data/mods/public/gui/pregame/MainMenuItems.js
2

You can use Map to guarantee key:value order.

binaries/data/mods/public/gui/pregame/mainmenu.js
5–6

Maybe is because the c++ init caller expects init to end but doesn't ?

source/gui/GUIManager.h
169 ↗(On Diff #9554)

Part of this diff?

Stan added a subscriber: Stan.Sep 1 2019, 1:50 PM
Stan added inline comments.
binaries/data/mods/public/gui/common/sprites.xml
11

Doesn't that go against the future engine split or is that file in modmod as well ?

binaries/data/mods/public/gui/pregame/BackgroundHandler.js
12

I thought that was slower than normal iteration aka for loop does this have any performance impact ?

30

Still ugly hardcoding (D1680)

elexis added a comment.Sep 1 2019, 1:56 PM

I am wondering about the naming Handler. For DeveloperOverlayManager in https://code.wildfiregames.com/D1928#79847 it was deemed improvable for instance.

Test plan:

Consider whether or not to use the class keyword, especially for those trival handlers that dont have prototype extensions.

That's what I still wonder, especially for the functions that only have 2-3 comments and appear like global procedures.

binaries/data/mods/public/gui/common/sprites.xml
11

You mean #5366. If that split proposal was right, then yes, these two sprites should be moved to the "0ad" mod and the rest to the "pyrogenesis-gui" mod.
If this file was in the pyrogenesis-gui/ folder, it would even be more visible, because there should not be a "0ad" string in that folder.

binaries/data/mods/public/gui/pregame/BackgroundHandler.js
12

In case you refer to comments I did, I mostly mentioned it for the filter function because that creates a new array and in some cases we dont need a new array if we just want to skip some items in a loop.

binaries/data/mods/public/gui/pregame/MainMenuButtonHandler.js
17 ↗(On Diff #9554)

Thanks, I didn't know this trick yet. I will keep it in mind because this isn't be the last time where this has the unwanted reference.

binaries/data/mods/public/gui/pregame/MainMenuItems.js
2

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map

A Map object iterates its elements in insertion order

Same problem.

binaries/data/mods/public/gui/pregame/ProductInformation.js
19 ↗(On Diff #9554)

String change here, it's not WARNING anymore.

binaries/data/mods/public/gui/pregame/SplashscreenHandler.js
22

hotloading is new here

source/gui/GUIManager.h
169 ↗(On Diff #9554)

That was related to the crash debugging (as this variable is the one it fails to read from upon CallFunction failure but points to undefined data). Now I dont know which other patch to insert it.

Stan added a subscriber: wraitii.Sep 1 2019, 2:05 PM
Stan added inline comments.
binaries/data/mods/public/gui/pregame/BackgroundHandler.js
12

Nah I meant I've looked it up as @wraitii was fond of it in D274 and it was generally slower in benchmarks.

Krinkle added inline comments.Sep 1 2019, 4:15 PM
binaries/data/mods/public/gui/pregame/MainMenuItems.js
2

If the order is important, would an array of objects work? Keep it simple :)

elexis added inline comments.Sep 2 2019, 3:55 AM
binaries/data/mods/public/gui/pregame/MainMenuItems.js
2

That's why it is one currently.

So the question is mostly whether I should insert "id": "singleplayer" despite that not being used but possibly used by modders to figure out where they want to insert a new menu item.
Then again what if some mod removes the "singleplayer" item and another wants to add below the "singleplayer" item.

So the mods would have to play dirty, since they'd rely on an assumption that they can't assume (that the "singleplayer" or whatever item wasnt removed by a mod). So if they already play dirty, they can also dirtily identify the element by comparing against the translated caption. (That again however might also have been changed by a mod). So it seems hypothetical, and perhaps a mod should just be recommended to append or insert at a semi-random position.
Well at least mods have it easy to insert.

191

-1 tab

source/gui/GUIManager.h
169 ↗(On Diff #9554)

Just for the record, the const doesn't help for the mentioned crash.
It seems the entire this object is somehow invalidated, like iterator invalidation. But I couldnt find an explanation.

Stan added inline comments.Sep 2 2019, 11:09 AM
source/gui/GUIManager.h
169 ↗(On Diff #9554)

What if it wasn't initialized at all ?

In D2240#93276, @nani wrote:

Parallax scrolling is not working (BackgroundHandler).

Thanks for finding that, I didnt even notice!
It's because I used lastTick instead of firstTick (g_T0) in the offset computation.

Open prelobby page from mainmenu hotkey doesn't work anymore.

Ah, rebase error!
I've checked all hotkeys in https://code.wildfiregames.com/source/0ad/browse/ps/trunk/binaries/data/mods/public/gui/pregame/mainmenu.xml and they are there.
Note to self: go through all commits of the last years again.
However I never committed the patch to assign hotkeys from JS.
So this patch is actually blocked by that, as the XML objects are now anonymous placeholders under a <repeat> so cant have the hotkey set from XML.
Good. That was on the commitlist anyhow.

Thanks for that review nani!

I've actually had to move the welcome-screen PushGUIPage call back to the ugly onTick due to that crash since ages: rP13597 (#5578) and it isn't trivial how to fix it.

elexis updated this revision to Diff 9613.Sep 3 2019, 1:33 PM

Add missing lobby hotkey and fix background handler scrolling as reported by nani.
Revert to ugly onTick hack from rP13597.
Fix Splashscreen hotloading.

Vulcan added a comment.Sep 3 2019, 1:36 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/61/display/redirect

Vulcan added a comment.Sep 3 2019, 1:43 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/gui/GUIManager.h
|  47| class·CGUIManager
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGUIManager{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 184| 184| 		"tooltip": translate("Exits the game."),
| 185| 185| 		"onPress": () => {
| 186| 186| 			messageBox(
| 187|    |-					400, 200,
|    | 187|+				400, 200,
| 188| 188| 					translate("Are you sure you want to quit 0 A.D.?"),
| 189| 189| 					translate("Confirmation"),
| 190| 190| 					[translate("No"), translate("Yes")],
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 185| 185| 		"onPress": () => {
| 186| 186| 			messageBox(
| 187| 187| 					400, 200,
| 188|    |-					translate("Are you sure you want to quit 0 A.D.?"),
|    | 188|+				translate("Are you sure you want to quit 0 A.D.?"),
| 189| 189| 					translate("Confirmation"),
| 190| 190| 					[translate("No"), translate("Yes")],
| 191| 191| 					[null, Engine.Exit]);
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 186| 186| 			messageBox(
| 187| 187| 					400, 200,
| 188| 188| 					translate("Are you sure you want to quit 0 A.D.?"),
| 189|    |-					translate("Confirmation"),
|    | 189|+				translate("Confirmation"),
| 190| 190| 					[translate("No"), translate("Yes")],
| 191| 191| 					[null, Engine.Exit]);
| 192| 192| 		}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 187| 187| 					400, 200,
| 188| 188| 					translate("Are you sure you want to quit 0 A.D.?"),
| 189| 189| 					translate("Confirmation"),
| 190|    |-					[translate("No"), translate("Yes")],
|    | 190|+				[translate("No"), translate("Yes")],
| 191| 191| 					[null, Engine.Exit]);
| 192| 192| 		}
| 193| 193| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 188| 188| 					translate("Are you sure you want to quit 0 A.D.?"),
| 189| 189| 					translate("Confirmation"),
| 190| 190| 					[translate("No"), translate("Yes")],
| 191|    |-					[null, Engine.Exit]);
|    | 191|+				[null, Engine.Exit]);
| 192| 192| 		}
| 193| 193| 	}
| 194| 194| ];
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/570/display/redirect

elexis added a comment.Sep 4 2019, 9:57 PM

This version of the patch would have depended on D2257 / rP22845.
Even with the patch the hotkeys are actually broken, because there is only one submenu, but multiple submenus have hotkey items.
One can pregenerate the same submenus, but I dont really see the point, especially when using the hotkey method nani proposed in said revision proposal, Engine.SetGlobalHotkey.
That only has the disadvantage (?) (or inconsistency to object assigned hotkeys) - one is either limited to assign one hotkey to one function, not one hotkey to multiple functions (multiple IGUIObjects).
That however may not be seen as invalid, because who really wants to press on two menu items with one hotkey? A mod would probably want to replace the hotkey with a function calling the existing function, refering to it by the g_MainMenu name, or something.
So the patch now depends on D2260.

There was a missing closeSubmenu call when pressing on a menu item within the submenu.

Naming: MainMenuItems vs MainMenuButtonHandler.

On the prototype vs class syntax question:, Deleted gave feedback in the lobby on Sep 1:

btw, classes are better than prototypes for just a single reason lol
lets say you were working on some web stuff and there exists a prototype with a property called a name.
you can accidentaly rename your window. but the syntax sugar would raise an error.

There are more advantages, for example private variables

It's an error to reference private fields from outside of the class; they can only be read or written within the class body. By defining things which are not visible outside of the class, you ensure that your classes' users can't depend on internals, which may change version to version.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes

So it seems attractive, and if the entire GUI is already rewritten from procedural to OOP, why not go for it while having the chance to.
Before I rewrite it, I will upload one more backup with prototypes and mentioned issues fixed.

binaries/data/mods/public/gui/pregame/MainMenuItems.js
108

DISABLED BY BUILD shown if not disabled.

It's an operator precedence problem, + binds stronger than &&, so adding parentheses

binaries/data/mods/public/gui/pregame/ProductInformation.js
4 ↗(On Diff #9613)

ProductInformation -> ProjectInformation

binaries/data/mods/public/gui/pregame/mainmenu.js
28

isStartup is set from GameSetup.cpp, see rP12762

elexis updated this revision to Diff 9630.Sep 4 2019, 9:58 PM

Fix above, depend on D2260.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/74/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 184| 184| 		"tooltip": translate("Exits the game."),
| 185| 185| 		"onPress": () => {
| 186| 186| 			messageBox(
| 187|    |-					400, 200,
|    | 187|+				400, 200,
| 188| 188| 					translate("Are you sure you want to quit 0 A.D.?"),
| 189| 189| 					translate("Confirmation"),
| 190| 190| 					[translate("No"), translate("Yes")],
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 185| 185| 		"onPress": () => {
| 186| 186| 			messageBox(
| 187| 187| 					400, 200,
| 188|    |-					translate("Are you sure you want to quit 0 A.D.?"),
|    | 188|+				translate("Are you sure you want to quit 0 A.D.?"),
| 189| 189| 					translate("Confirmation"),
| 190| 190| 					[translate("No"), translate("Yes")],
| 191| 191| 					[null, Engine.Exit]);
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 186| 186| 			messageBox(
| 187| 187| 					400, 200,
| 188| 188| 					translate("Are you sure you want to quit 0 A.D.?"),
| 189|    |-					translate("Confirmation"),
|    | 189|+				translate("Confirmation"),
| 190| 190| 					[translate("No"), translate("Yes")],
| 191| 191| 					[null, Engine.Exit]);
| 192| 192| 		}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 187| 187| 					400, 200,
| 188| 188| 					translate("Are you sure you want to quit 0 A.D.?"),
| 189| 189| 					translate("Confirmation"),
| 190|    |-					[translate("No"), translate("Yes")],
|    | 190|+				[translate("No"), translate("Yes")],
| 191| 191| 					[null, Engine.Exit]);
| 192| 192| 		}
| 193| 193| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 188| 188| 					translate("Are you sure you want to quit 0 A.D.?"),
| 189| 189| 					translate("Confirmation"),
| 190| 190| 					[translate("No"), translate("Yes")],
| 191|    |-					[null, Engine.Exit]);
|    | 191|+				[null, Engine.Exit]);
| 192| 192| 		}
| 193| 193| 	}
| 194| 194| ];
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/583/display/redirect

Stan added inline comments.Sep 4 2019, 11:23 PM
binaries/data/mods/public/gui/pregame/BackgroundHandler.js
12

I guess it's not performance critical then ?

binaries/data/mods/public/gui/pregame/MainMenuItemHandler.js
13

Why are those PascalCase ?

100

Why the temporary variable ?

nani added inline comments.Sep 5 2019, 12:11 AM
binaries/data/mods/public/gui/pregame/MainMenuItemHandler.js
13

It follows that rule of naming the variable the same as the xml object, thus then the xml name definition should be changed too to be standarized :)

100

The GUI internals force this way.
The engine returns an object but that object will not update if the parameters of that object are changed, so it needs to make a full copy -> modify -> an assign the full object to that setting again for the gui engine to notice. See also that in the event that this were not true we would update needlessly the object twice instead of once (in a batch).

Guess I'll fix what Jenkins wants, otherwise I'll get a whitespace cleanup patch :P

binaries/data/mods/public/gui/pregame/BackgroundHandler.js
12

How often is this function called?

binaries/data/mods/public/gui/pregame/MainMenuItemHandler.js
13

The benefit from using the same name in JS and XML is that if you do a filesearch, you immediately get all occurrences where the GUI object is used, rather than having to interpret code to find them. (No need to appeal to authority)

53

for...in

100

The C++ defined JSClass GUISize is updated, but the GUI object doesnt rebuild its size following the settings change, it only does so after a SetSetting call, below triggered from JSI_IGUIObject::setProperty.

I suppose it could be implemented but who cares, actually I wonder if we really needed the JSclass when a JS object does the same, will check again.

binaries/data/mods/public/gui/pregame/menupanel.xml
18

ProjectInformation

Stan added inline comments.Sep 5 2019, 1:48 PM
binaries/data/mods/public/gui/pregame/BackgroundHandler.js
12

Once I guess ?

binaries/data/mods/public/gui/pregame/MainMenuItemHandler.js
13

Thanks for the explanation. Could be in Cc maybe ?

elexis updated this revision to Diff 9641.Sep 5 2019, 9:18 PM

Fall in love with JS class keyword and become disappointed by not having support for private fields.

Vulcan added a comment.Sep 5 2019, 9:20 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/83/display/redirect

Vulcan added a comment.Sep 5 2019, 9:23 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-extra-semi):
|    | Unnecessary semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/SplashscreenHandler.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/SplashscreenHandler.js
|  30|  30| 			Engine.PushGuiPage("page_splashscreen.xml", {}, this.showRenderPathMessage);
|  31|  31| 		else
|  32|  32| 			this.showRenderPathMessage();
|  33|    |-	};
|    |  33|+	}
|  34|  34| 
|  35|  35| 	showRenderPathMessage()
|  36|  36| 	{

binaries/data/mods/public/gui/pregame/SplashscreenHandler.js
|  33| »   };
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.

binaries/data/mods/public/gui/pregame/mainmenu.js
|  16| »   new·MusicHandler();
|    | [NORMAL] ESLintBear (no-new):
|    | Do not use 'new' for side effects.

binaries/data/mods/public/gui/pregame/mainmenu.js
|  16| »   new·MusicHandler();
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'MusicHandler' was used before it was defined.

binaries/data/mods/public/gui/pregame/mainmenu.js
|  17| »   new·ProjectInformationHandler(g_ProjectInformation);
|    | [NORMAL] ESLintBear (no-new):
|    | Do not use 'new' for side effects.

binaries/data/mods/public/gui/pregame/mainmenu.js
|  17| »   new·ProjectInformationHandler(g_ProjectInformation);
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'ProjectInformationHandler' was used before it was defined.

binaries/data/mods/public/gui/pregame/mainmenu.js
|  18| »   new·CommunityButtonHandler();
|    | [NORMAL] ESLintBear (no-new):
|    | Do not use 'new' for side effects.

binaries/data/mods/public/gui/pregame/mainmenu.js
|  18| »   new·CommunityButtonHandler();
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'CommunityButtonHandler' was used before it was defined.

binaries/data/mods/public/gui/pregame/mainmenu.js
|  35| class·MusicHandler
|    | [NORMAL] JSHintBear:
|    | 'MusicHandler' was used before it was defined.

binaries/data/mods/public/gui/pregame/mainmenu.js
|  44| class·ProjectInformationHandler
|    | [NORMAL] JSHintBear:
|    | 'ProjectInformationHandler' was used before it was defined.

binaries/data/mods/public/gui/pregame/mainmenu.js
|  54| class·CommunityButtonHandler
|    | [NORMAL] JSHintBear:
|    | 'CommunityButtonHandler' was used before it was defined.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 184| 184| 		"tooltip": translate("Exits the game."),
| 185| 185| 		"onPress": () => {
| 186| 186| 			messageBox(
| 187|    |-					400, 200,
|    | 187|+				400, 200,
| 188| 188| 					translate("Are you sure you want to quit 0 A.D.?"),
| 189| 189| 					translate("Confirmation"),
| 190| 190| 					[translate("No"), translate("Yes")],
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 185| 185| 		"onPress": () => {
| 186| 186| 			messageBox(
| 187| 187| 					400, 200,
| 188|    |-					translate("Are you sure you want to quit 0 A.D.?"),
|    | 188|+				translate("Are you sure you want to quit 0 A.D.?"),
| 189| 189| 					translate("Confirmation"),
| 190| 190| 					[translate("No"), translate("Yes")],
| 191| 191| 					[null, Engine.Exit]);
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 186| 186| 			messageBox(
| 187| 187| 					400, 200,
| 188| 188| 					translate("Are you sure you want to quit 0 A.D.?"),
| 189|    |-					translate("Confirmation"),
|    | 189|+				translate("Confirmation"),
| 190| 190| 					[translate("No"), translate("Yes")],
| 191| 191| 					[null, Engine.Exit]);
| 192| 192| 		}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 187| 187| 					400, 200,
| 188| 188| 					translate("Are you sure you want to quit 0 A.D.?"),
| 189| 189| 					translate("Confirmation"),
| 190|    |-					[translate("No"), translate("Yes")],
|    | 190|+				[translate("No"), translate("Yes")],
| 191| 191| 					[null, Engine.Exit]);
| 192| 192| 		}
| 193| 193| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 188| 188| 					translate("Are you sure you want to quit 0 A.D.?"),
| 189| 189| 					translate("Confirmation"),
| 190| 190| 					[translate("No"), translate("Yes")],
| 191|    |-					[null, Engine.Exit]);
|    | 191|+				[null, Engine.Exit]);
| 192| 192| 		}
| 193| 193| 	}
| 194| 194| ];
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/592/display/redirect

Krinkle added inline comments.Sep 6 2019, 2:45 AM
binaries/data/mods/public/gui/pregame/MainMenuItems.js
108

What is the intended else behaviour? Boolean false casts to string "false". I haven't tested it with this patch, but I would expect that to be happening currently.

> "foo" + (!true && "if")
"foofalse"

Presumably the intended else-branch is to append nothing, which would be done with something like:

> "foo" + (!true && "if" || "")
"foo"
elexis added inline comments.Sep 6 2019, 2:49 AM
binaries/data/mods/public/gui/pregame/MainMenuItems.js
108

Whoops, you are right, and in time! Ternary for the rescue!

elexis added inline comments.Sep 6 2019, 3:17 AM
binaries/data/mods/public/gui/pregame/MainMenuItemHandler.js
13

Would probably be good.
To me it's important that the CC derive from a good reason, rather than the CC being used as a reason for code being one way or the other.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 6 2019, 3:20 AM
Closed by commit rP22854: Rewrite Main Menu. (authored by elexis). · Explain Why
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Sep 6 2019, 3:20 AM