Page MenuHomeWildfire Games

Pass an argument to GUI events / SendEventToAll, remove loading screen progess workaround
Needs ReviewPublic

Authored by elexis on Jan 20 2019, 12:20 AM.

Details

Reviewers
Itms
Summary

When sending a GUI event from the C++ engine to JS, one can only pass an argument if
one sends the msg to a particular GUI object. But in most cases one doesn't have a GUI Object at
hand and would have to hardcode a name - or use a hack - or just avoid sending any data.

The loading screen runs into this problem in particular and hardcodes JS variable names g_Progress and g_LoadDescription, which are supposed to be arguments of the GUI event.
Someone looking at JS code will assume these globals are defined in JS like every other global, but fail to find their definition as it's in C++.

The other use case is the replay out-of-sync message from rP19491. We had no chance to display the turn.

As mentioned in D1716 (rP22052), it would be nice to remove the workaround instead of perpetuating it, so here the relief.

rP2033 introduced the "rough but functional" progress bar.
rP2619 moved it to a new function.
rP2622 moved it to a new file.
rP3508 removed a trailing whitespace there.
rP3802 fixed lineendings
rP8629 changed it for some SpiderMonkey upgrade (1.8.5?).
rP14496 changed it for some SpiderMonkey upgrade v24.
rP22052 changed it for the SpiderMonkey 45 Upgrade.

Test Plan

Start a game to check the loading screen.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6820
Build 11215: Vulcan BuildJenkins
Build 11214: arc lint + arc unit

Event Timeline

elexis created this revision.Jan 20 2019, 12:20 AM
Vulcan added a subscriber: Vulcan.Jan 20 2019, 12:49 AM

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 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/loading/loading.js
|  26|  26| 
|  27|  27| 	if (tipFile)
|  28|  28| 	{
|  29|    |-			let tipText = Engine.TranslateLines(Engine.ReadFile(g_TipsTextPath + tipFile + ".txt")).split("\n");
|    |  29|+		let tipText = Engine.TranslateLines(Engine.ReadFile(g_TipsTextPath + tipFile + ".txt")).split("\n");
|  30|  30| 			Engine.GetGUIObjectByName("tipTitle").caption = tipText.shift();
|  31|  31| 			Engine.GetGUIObjectByName("tipText").caption = tipText.join("\n");
|  32|  32| 			Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/loading/loading.js
|  27|  27| 	if (tipFile)
|  28|  28| 	{
|  29|  29| 			let tipText = Engine.TranslateLines(Engine.ReadFile(g_TipsTextPath + tipFile + ".txt")).split("\n");
|  30|    |-			Engine.GetGUIObjectByName("tipTitle").caption = tipText.shift();
|    |  30|+		Engine.GetGUIObjectByName("tipTitle").caption = tipText.shift();
|  31|  31| 			Engine.GetGUIObjectByName("tipText").caption = tipText.join("\n");
|  32|  32| 			Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|  33|  33| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/loading/loading.js
|  28|  28| 	{
|  29|  29| 			let tipText = Engine.TranslateLines(Engine.ReadFile(g_TipsTextPath + tipFile + ".txt")).split("\n");
|  30|  30| 			Engine.GetGUIObjectByName("tipTitle").caption = tipText.shift();
|  31|    |-			Engine.GetGUIObjectByName("tipText").caption = tipText.join("\n");
|    |  31|+		Engine.GetGUIObjectByName("tipText").caption = tipText.join("\n");
|  32|  32| 			Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|  33|  33| 	}
|  34|  34| 	else
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/loading/loading.js
|  29|  29| 			let tipText = Engine.TranslateLines(Engine.ReadFile(g_TipsTextPath + tipFile + ".txt")).split("\n");
|  30|  30| 			Engine.GetGUIObjectByName("tipTitle").caption = tipText.shift();
|  31|  31| 			Engine.GetGUIObjectByName("tipText").caption = tipText.join("\n");
|  32|    |-			Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|    |  32|+		Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|  33|  33| 	}
|  34|  34| 	else
|  35|  35| 		error("Failed to find any matching tips for the loading screen.");

binaries/data/mods/public/gui/loading/loading.js
|  44| »   »   switch·(data.attribs.mapType)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/messages.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/messages.js
| 470| 470| 		g_Selection.reset();
| 471| 471| 		g_Selection.addList(selection, false, cmd.type == "gather");
| 472| 472| 	},
| 473|    |-	"play-tracks": function (notification, player)
|    | 473|+	"play-tracks": function(notification, player)
| 474| 474| 	{
| 475| 475| 		if (notification.lock)
| 476| 476| 		{
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/messages.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/messages.js
| 577| 577| 	let notificationText =
| 578| 578| 		notification.instructions.reduce((instructions, item) =>
| 579| 579| 			instructions + (typeof item == "string" ? translate(item) : colorizeHotkey(translate(item.text), item.hotkey)),
| 580|    |-			"");
|    | 580|+		"");
| 581| 581| 
| 582| 582| 	Engine.GetGUIObjectByName("tutorialText").caption = g_TutorialMessages.concat(setStringTags(notificationText, g_TutorialNewMessageTags)).join("\n");
| 583| 583| 	g_TutorialMessages.push(notificationText);
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/messages.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/messages.js
|1087|1087| 
|1088|1088| 	let message = "";
|1089|1089| 	if (notifyPhase == "all")
|1090|    |-	{
|    |1090|+	
|1091|1091| 		if (msg.phaseState == "started")
|1092|1092| 			message = translate("%(player)s is advancing to the %(phaseName)s.");
|1093|1093| 		else if (msg.phaseState == "aborted")
|1094|1094| 			message = translate("The %(phaseName)s of %(player)s has been aborted.");
|1095|    |-	}
|    |1095|+	
|1096|1096| 	if (msg.phaseState == "completed")
|1097|1097| 		message = translate("%(player)s has reached the %(phaseName)s.");
|1098|1098| 

binaries/data/mods/public/gui/session/messages.js
| 636| »   while·(true)
|    | [NORMAL] ESLintBear (no-constant-condition):
|    | Unexpected constant condition.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/986/

elexis marked an inline comment as done.Jan 20 2019, 7:32 PM

Apparently the fork 0ad guys have a patch for this already, which makes me wonder why I'd be doing any further authorship on this diff.
Mostly I wanted to express in C++ code what I could not succeed to communicate in words in D1716 anyhow.

source/simulation2/system/ReplayTurnManager.cpp
85

I didn't CloneValueFromOtherContext because we pass a JS::HandleValue, not a JS::Value and the former claims to guarantee rooting safety.
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS::Handle
I didn't check whether the clone is still needed. The SM45 upgrade will complain about rooting in different contexts that use the same runtime (where sm38 doesn't), maybe it will bug then.

wraitii added a subscriber: wraitii.

Apparently the fork 0ad guys have a patch for this already, which makes me wonder why I'd be doing any further authorship on this diff.

Not sure what you mean here.

@Itms the clone thing should be checked but you probably know more about this than me right now since I last worked on SM a while ago.

Otherwise this looks good.

binaries/data/mods/public/gui/loading/loading.js
72

It appears these comments are garbage, but that's not related to your patch

smiley added a subscriber: smiley.Jan 20 2019, 8:26 PM

(*Fork AD. No 0)

Itms added a comment.Wed, Apr 24, 10:32 PM

This looks very good to me, and it works both on SM38 and SM45.

I am only requesting an explanation on a part of the code I don't understand, and I am suggesting you rename some arguments in C++.

binaries/data/mods/public/gui/session/session.xml
27

This confuses me a bit, because I don't see anywhere in the code how the data sent here gets the JS name "arguments", and I don't understand either why the data passed is under the 0 index. Based on the similar code for loading, I would assume that the data passed to the event function contains turn, hash and expectedHash directly.

It does seem to work, but it's the one bit I'm not sure enough about to accept. 😕

Depending on your answer, if the name can be changed, I would also be in favor of calling it data.

source/gui/GUIutil.h
225

As a style remark, I would prefer to keep the T called arg and the JS value called data. They are on the same "level" in the C++ code, but semantically they are very different, so I think calling them arg1 and arg2 is confusing.

260

Agree on removing those.

286
(pObject->*pFunc)(Argument, Data);

just sounds better and clearer to me.

source/simulation2/system/ReplayTurnManager.cpp
85

This still works with SM45. 👍

Personally I like it this way and I wouldn't bother with cloning from the net client context.

Itms added inline comments.Wed, Apr 24, 10:49 PM
binaries/data/mods/public/gui/session/session.xml
27

Stan answered me on IRC, I was unaware of that JS feature.

20:40 < Stan`[m]> Itms IIRC you can use arguments in any javascript function
20:41 < Stan`[m]> allows you to pass more params than declared, and other weird stuff

So I guess renaming it to data is not straightforward and would add useless boilerplate.

Thanks for the review!

Notice that the OOS message receives more arguments than it uses, but that was a choice by me (the arguments may be used by some JS GUI, just our current JS GUI is optimized for informing players more than devs)

(Also am I the only one using libboost 1.69.0-2 or am I just the only one too stupid to resolve the build errors? I better get them resolved if I want to commit my own patches)

binaries/data/mods/public/gui/session/session.xml
27

It's not only not straight forward but straight impossible.

We can't rename the object because we didn't define that, but JS defined the object.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/arguments

We also have it here:

minimap_panel.xml: <action on="WorldClick">handleMinimapEvent(arguments[0]);</action>

The alternative is not having the event definition in XML and moving it to JS, like in loading.js of this patch:
progressbar.onGameLoadProgress = displayProgress;

Or if we want to be able to use XML specified functions with arguments but without the arguments object, then we could for example try changing C++ GUI parsing to this:

	<action on="ReplayOutOfSync" argument1="arg1">
		onReplayOutOfSync(arg1);

But that sounds ugly differently.

And we shouldn't specify JS in XML pages to begin with, JS -> JS, XML -> XML, so that JS linting works, syntax highlighting, separation of concerns, XML being intended to only describe a document, etc.

I guess I didn't move it over to JS because these global events are so well presented here currently, they should remain grouped (but could be grouped in JS later)

source/gui/GUIutil.h
286

D'accord, especially since the functionality is limited to one JS object argument rather than arbitrary many JS value arguments

source/simulation2/system/ReplayTurnManager.cpp
85

Either the clone is necessary or it is unnecessary and thus misleading (i.e. must be done or must be avoided).

Just because it didn't segfault yet doesn't mean that the code is right.
In theory we should have sufficient SM understanding to determine when the clone is necessary when passing JS objects through contexts and when not.
There might be a good reason (like JSHandleValue vs JSValue), perhaps we're right by chance indicated by the testing.

wraitii added inline comments.Thu, Apr 25, 11:58 AM
source/simulation2/system/ReplayTurnManager.cpp
85

I think one needs to look at upstream-upstream Spidemronkey, such as SM58+ . They've changed things around that area, but I don't remember if we're in the clear here.

In particular they're ether removed runtime or contexts...

Sadly I don't believe the docs are up-to-date so one has to look at the comments in the headers for structured clone on upstream spider monkey, which is a bit annoying.

Itms added a comment.Thu, Apr 25, 7:44 PM
In D1754#76083, @elexis wrote:

(Also am I the only one using libboost 1.69.0-2 or am I just the only one too stupid to resolve the build errors? I better get them resolved if I want to commit my own patches)

I just tested libboost 1.69.0 on Windows and I don't get build errors, can you share the error message(s)?

Waiting for your answer on the clone thing, apart from that if you can rename the arg2 to data I can accept the patch.

binaries/data/mods/public/gui/session/session.xml
27

Yes, I'm not a big fan of the <action on="ReplayOutOfSync" argument1="arg1"> possibility, if we keep JS code in XML the current patch is fine now that I now about arguments 🙂

I agree that the best thing to do would be to separate the JS from the XML pages, but I also agree that the current XML page is rather tidy, so let's keep things that way for this patch 👍

source/simulation2/system/ReplayTurnManager.cpp
85

The current code in the patch looks pretty good to me. Cloning doesn't seem to be necessary, since creating the new value in the simulation's script context looks perfectly valid and straightforward. And as long as we don't clone things we don't have to worry about upcoming changes to contexts and runtimes.

I don't see what could make it segfault, the replayData is used by the JS code when calling SendEventToAll and subsequent calls (which are all synchronous), then when it goes out of scope it can be safely GCed. Am I missing something?

Imarok added a subscriber: Imarok.EditedThu, Apr 25, 7:51 PM
In D1754#76083, @elexis wrote:

(Also am I the only one using libboost 1.69.0-2 or am I just the only one too stupid to resolve the build errors? I better get them resolved if I want to commit my own patches)

See https://github.com/boostorg/random/issues/49 (Found by Vladislav)
(Also see discussion at irclogs 2019-04-04 around 21:44)
TLDR: just ignore the notes and wait for boost 1.70

This several hundred times, since february or something:

In file included from /usr/include/boost/random/detail/integer_log2.hpp:19,
                 from /usr/include/boost/random/detail/large_arithmetic.hpp:19,
                 from /usr/include/boost/random/detail/const_mod.hpp:23,
                 from /usr/include/boost/random/detail/seed_impl.hpp:26,
                 from /usr/include/boost/random/mersenne_twister.hpp:30,
                 from ../../../source/graphics/ParticleManager.h:24,
                 from ../../../source/graphics/ParticleManager.cpp:20:
/usr/include/boost/pending/integer_log2.hpp:7:59: note: #pragma message: This header is deprecated. Use <boost/integer/integer_log2.hpp> instead.
 BOOST_HEADER_DEPRECATED("<boost/integer/integer_log2.hpp>");
                                                           ^

CText.cpp
In file included from /usr/include/boost/random/detail/integer_log2.hpp:19,
                 from /usr/include/boost/random/detail/large_arithmetic.hpp:19,
                 from /usr/include/boost/random/detail/const_mod.hpp:23,
                 from /usr/include/boost/random/linear_congruential.hpp:30,
                 from ../../../source/scriptinterface/ScriptInterface.h:26,
                 from ../../../source/gui/CGUI.h:32,
                 from ../../../source/gui/GUIutil.h:35,
                 from ../../../source/gui/GUI.h:46,
                 from ../../../source/gui/CText.h:21,
                 from ../../../source/gui/CText.cpp:20:
/usr/include/boost/pending/integer_log2.hpp:7:59: note: #pragma message: This header is deprecated. Use <boost/integer/integer_log2.hpp> instead.

I remember researching and trying stuff for about a day before running out of interest, and I suspect there's a patch already somewhere since I'm not the only arch user.
(Just using <boost/integer/integer_log2.hpp> didn't work)

In D1754#76234, @Imarok wrote:
In D1754#76083, @elexis wrote:

(Also am I the only one using libboost 1.69.0-2 or am I just the only one too stupid to resolve the build errors? I better get them resolved if I want to commit my own patches)

See https://github.com/boostorg/random/issues/49 (Found by Vladislav)
(Also see discussion at irclogs 2019-04-04 around 21:44)
TLDR: just ignore the notes and wait for boost 1.70

Proper!