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)