Page MenuHomeWildfire Games
Differential D1754

Pass an argument to GUI events / SendEventToAll, remove loading screen progess workaround
ClosedPublic
Actions

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

Details

Reviewers
wraitii
Itms
Commits
rP22515: Pass arbitrary arguments to GUI events / SendEventToAll and remove loading…
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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Harbormaster completed remote builds in B6820: Diff 7370.Jan 20 2019, 12:49 AM
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 ↗(On Diff #7370)

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 reviewer: Itms.Jan 20 2019, 8:15 PM
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 ↗(On Diff #7370)

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

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

(*Fork AD. No 0)

elexis mentioned this in D1844: Improvements to simulation hotloading before the SM upgrade.Apr 22 2019, 11:02 PM
Itms added a comment.Apr 24 2019, 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 ↗(On Diff #7370)

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 ↗(On Diff #7370)

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 ↗(On Diff #7370)

Agree on removing those.

286 ↗(On Diff #7370)
(pObject->*pFunc)(Argument, Data);

just sounds better and clearer to me.

source/simulation2/system/ReplayTurnManager.cpp
85 ↗(On Diff #7370)

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.Apr 24 2019, 10:49 PM
binaries/data/mods/public/gui/session/session.xml
27 ↗(On Diff #7370)

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.

elexis added a comment.Apr 25 2019, 11:51 AM

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 ↗(On Diff #7370)

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 ↗(On Diff #7370)

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 ↗(On Diff #7370)

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.Apr 25 2019, 11:58 AM
source/simulation2/system/ReplayTurnManager.cpp
85 ↗(On Diff #7370)

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.Apr 25 2019, 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 ↗(On Diff #7370)

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 ↗(On Diff #7370)

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.EditedApr 25 2019, 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

elexis added a comment.Apr 25 2019, 8:19 PM

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)

elexis added a comment.Apr 25 2019, 8:21 PM
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!

elexis abandoned this revision.Apr 27 2019, 8:23 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)

So they were just warnings, not errors, but they still spam a lot of noise which makes it hard to find ones own warnings or errors when programming.

As mentioned there was a defect and a existence of a diff relating to this patch reported on the lobby on 2019-01-20 and 2019-01-24, which reminded that I want to stop creating crap and fuckup.

Itms commandeered this revision.Apr 27 2019, 11:56 PM
Itms edited reviewers, added: elexis; removed: Itms.

I understand the feeling, but unfortunately we can do nothing except keeping developing the game with everything that we have and that we can actually access. Your work is certainly not crap (quite the opposite), your contributions and the ones from fellow WFG members are enough to keep creating 0 A.D. ? Chin up! ?

Itms reclaimed this revision.Apr 27 2019, 11:56 PM
Itms updated this revision to Diff 7879.
Itms added a reviewer: wraitii.

This is the patch rebased and with the second arguments renamed to data.

Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Apr 27 2019, 11:58 PM
Vulcan added a comment.Apr 28 2019, 12:17 AM

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

Linter detected issues:
Executing section Source...

source/gui/CGUI.cpp
|1680| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollWheel

source/gui/CGUI.cpp
|1680| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollSpeed

source/gui/CGUI.cpp
|1680| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollButtons
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.map(
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|    | [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.map(
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|  33| 				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|    | [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.map(
|    |  31|+		Engine.GetGUIObjectByName("tipText").caption = tipText.map(
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|  33| 				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|  34|  34| 			).join("\n\n");
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 4.
|----|    | /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.map(
|  32|    |-				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|    |  32|+			// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|  33| 				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|  34|  34| 			).join("\n\n");
|  35|  35| 			Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 4.
|----|    | /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
|  30|  30| 			Engine.GetGUIObjectByName("tipTitle").caption = tipText.shift();
|  31|  31| 			Engine.GetGUIObjectByName("tipText").caption = tipText.map(
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|    |-				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|    |  33|+			text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|  34|  34| 			).join("\n\n");
|  35|  35| 			Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|  36|  36| 	}
|    | [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
|  31|  31| 			Engine.GetGUIObjectByName("tipText").caption = tipText.map(
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|  33| 				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|  34|    |-			).join("\n\n");
|    |  34|+		).join("\n\n");
|  35|  35| 			Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|  36|  36| 	}
|  37|  37| 	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
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|  33| 				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|  34|  34| 			).join("\n\n");
|  35|    |-			Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|    |  35|+		Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|  36|  36| 	}
|  37|  37| 	else
|  38|  38| 		error("Failed to find any matching tips for the loading screen.");

binaries/data/mods/public/gui/loading/loading.js
|  47| »   »   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| 
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1303/display/redirect

Harbormaster completed remote builds in B7324: Diff 7879.Apr 28 2019, 12:17 AM
elexis requested changes to this revision.Apr 28 2019, 1:46 AM

The patch was reported to miss something.

This revision now requires changes to proceed.Apr 28 2019, 1:46 AM
Itms added a comment.Apr 28 2019, 11:18 AM

Well I agreed with all points of this patch, and I just looked for other possible uses of the new SendEventToAll and found none that relied on this workaround. It's possible that the code can be used in another place that we didn't see (in which case we'd only loose the opportunity to fix it sooner), but it seems incorrect that the patch "misses something".

This vague allegation should not block us from progressing. Unhelpful and rude remarks about our work were the reason he was banned in the first place. Saying there is a defect and refusing to point it out is childish at best. The assumption that we would have to re-re-release A23 was (is?) a similar kind of bullcrap, and thankfully we didn't wait for them to reveal The Secret (tm) to release.

If someone in WFG finds something that could possibly be added to the patch, even if it's far-fetched, I'd be happy to add it. But else we can't wait forever.

Stan added a subscriber: Stan.Apr 28 2019, 11:23 AM

I agree. And my gf as well. She added: "Itms president !"

Itms added a comment.Apr 28 2019, 11:45 AM
In D1754#76520, @Stan wrote:

She added: "Itms president !"

Noooooo, no more responsibilities! ?

@elexis I got an idea, in order to move forward while acknowledging the possibility of the existence of an issue, maybe we could commit and open an audit, with the relevant part of the lobby logs for context. That way, if we find the issue someday, we will be able to accept the commit instead of having lost memory of it.

elexis added a comment.Apr 28 2019, 11:50 AM

This still doesn't solve (3)

Itms added a comment.Apr 28 2019, 12:11 PM

Sorry, which 3?

mimo added a subscriber: mimo.Apr 28 2019, 12:23 PM
In D1754#76519, @Itms wrote:

This vague allegation should not block us from progressing. Unhelpful and rude remarks about our work were the reason he was banned in the first place. Saying there is a defect and refusing to point it out is childish at best. The assumption that we would have to re-re-release A23 was (is?) a similar kind of bullcrap, and thankfully we didn't wait for them to reveal The Secret (tm) to release.

That a project leader made his own decision to ban somebody because he can't stand criticisms was the main reason why i left that team. Seeing now that kind of attack on a public forum (from which the concerned one is forbidden to answer) several months after is sad and unworthy of somebody who pretends to lead a team. That just reveals how vindictive you can be and validates my decision to leave and contribute to the fork (which i encourage everybody to do so).

I've always restrained myself to make public what i felt about that team and specially its leader, but i see that unfortunately not everybody has the same ethics.

Finally, seeing the reaction of the usual yes-man is just ridiculous.

Itms added a comment.Apr 28 2019, 12:43 PM

For tens of months before being banned, on a regular basis, unhelpful comments in the fashion of "there is a defect, I'm not helping you further" were made. Even if leper was allowed to answer here, he wouldn't (except if he has changed since then).

I'm sorry if I hurt someone's feelings while trying to cheer up and encourage people who are still at WFG. I'll try to be more vigilant about how I appear, but I really mean what I said and I can't be too hypocritical about how I truly feel. Apparently you can't restrain yourself from insulting people either, so you must understand me.

Sorry for polluting the discussion on your great patch with this @elexis, this is on me. ?

elexis added a comment.Apr 28 2019, 1:02 PM

mimo if I call your work crap without having seen it or knowning what it is about and you wouldn't like that, does that mean you just can't stand my criticism?

mimo added a comment.Apr 28 2019, 1:04 PM
In D1754#76533, @Itms wrote:

I'm sorry if I hurt someone's feelings while trying to cheer up and encourage people who are still at WFG. I'll try to be more vigilant about how I appear, but I really mean what I said and I can't be too hypocritical about how I truly feel. Apparently you can't restrain yourself from insulting people either, so you must understand me.

Once again, you only prove your incompetence as a leader. Do you expect anybody to say publicly what he feels about anybody? lol, the best way to destroy a team.
And if this team needs to denigrate others to progress, that's a new low.

mimo added a comment.Apr 28 2019, 1:14 PM
In D1754#76534, @elexis wrote:

mimo if I call your work crap without having seen it or knowning what it is about and you wouldn't like that, does that mean you just can't stand my criticism?

Whatever the facts which are subject to different interpretations, don't you think that such a ban should have been discussed within the team before?

elexis added a comment.Apr 28 2019, 2:02 PM
In D1754#76536, @mimo wrote:

don't you think that such a ban should have been discussed within the team before?

Sure, and Itms has learnt from that to ask the team for the other ban proposals.

Problem with bans is that it doesn't solve the problem but only hides the symptom,
that it removes more than what is desirable to be removed,
that we should have discussed the disagreements years before instead of silently accumulating frustration,
and as long as we don't intend to create a code of conduct that serves all parties, we're not progressing in any way.

The old staff policy already had some relevant passages
https://wildfiregames.com/forum/index.php?/topic/2737-wildfire-games-staff-policy/

The new one should be improved in such a way that it is clear to every participant what is acceptable and tolerable and what is not:
https://wildfiregames.com/forum/index.php?/topic/24720-forum-community-guidelines-and-privacy-policy/

In particular the policy could rule out bans of team members without discussions, without a three-strike system, or without a clear deduction which posts lead to the ban and which policies these posts violate.
Instead of insulting each other, we could also try to construct a better future for ourselves.

I personally prefer a 0 permanent ban policy, and I have unbanned even the most toxic individuals on the lobby after they served their time and banned them right again termporarily when they repeated their behavior.
This gives people a chance to change no matter how often they got into conflict.

In D1754#76534, @elexis wrote:

mimo if I call your work crap without having seen it or knowning what it is about and you wouldn't like that, does that mean you just can't stand my criticism?

Whatever the facts which are subject to different interpretations

I'm not sure if you mean that you can't speak of things which you haven't witnessed or whether you're stating that insulting others can be valid criticism depending on interpretation.

mimo added a comment.Apr 28 2019, 2:55 PM

I meant that from what i witnessed, i would never have expected such a ban.

But let's go back to the original point: do you really want to discuss on a public forum what happened 10 months ago on the private team forum, with a necessary partial and biased presentation of the facts? that's just irresponsible from a project leader to lead us that way, and i hope this will not happen again.

It's better to stop it here.

Stan added a comment.Apr 28 2019, 3:27 PM

So, since you are here. What are we missing in this patch?

elexis added a comment.Apr 28 2019, 6:03 PM

do you really want to discuss on a public forum what happened 10 months ago on the private team forum, with a necessary partial and biased presentation of the facts?

The question was how the statement from the day of the bugreport on this patch in the public lobby is to be interpretted.
If bugreporter states that this diff is 'missing something', our works are 'fuckup' and further contributions to be 'crap', then that isn't inviting, nor helping me to demonstrate the banreason to be unapplicable, and if I consider the bugreporter to be intelligent and knowledgeable, then I am more productive in removing myself from the development process.

In D1754#76524, @Itms wrote:

Sorry, which 3?

You will recall https://wildfiregames.com/meetinglogs/logs/0ad-staff.2019-04-07-18.12.log.html

Itms added a comment.Apr 28 2019, 6:11 PM

Ah of course. I thought the (3) was about this patch specifically.

elexis resigned from this revision.Jun 16 2019, 10:06 PM
This revision now requires review to proceed.Jun 16 2019, 10:06 PM
elexis commandeered this revision.Jul 15 2019, 1:13 PM
elexis edited reviewers, added: Itms; removed: elexis.
elexis updated this revision to Diff 8907.Jul 15 2019, 1:48 PM

Pass JS::HandleValueArray instead of a JS::Value so that the callee can support multiple arguments.
The creation of that JS::HandleValueArray might be further abstracted with template arguments similar to the nativewrappers.

Vulcan added a comment.Jul 15 2019, 2:01 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.

source/gui/IGUIObject.h
| 167| »   vector_pObjects::iterator·begin()·{·return·m_Children.begin();·}
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.

source/gui/CGUI.h
|  27| #include·"GUIbase.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classGUITooltip{' is invalid C code. Use --std or --language to configure the language.

source/gui/GUIutil.h
|  27| 
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classGUITooltip{' is invalid C code. Use --std or --language to configure the language.

source/gui/CGUI.cpp
|1680| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollWheel

source/gui/CGUI.cpp
|1680| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollSpeed

source/gui/CGUI.cpp
|1680| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollButtons
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /zpool0/trunk/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.map(
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /zpool0/trunk/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.map(
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|  33| 				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /zpool0/trunk/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.map(
|    |  31|+		Engine.GetGUIObjectByName("tipText").caption = tipText.map(
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|  33| 				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|  34|  34| 			).join("\n\n");
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 4.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /zpool0/trunk/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.map(
|  32|    |-				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|    |  32|+			// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|  33| 				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|  34|  34| 			).join("\n\n");
|  35|  35| 			Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 4.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|  30|  30| 			Engine.GetGUIObjectByName("tipTitle").caption = tipText.shift();
|  31|  31| 			Engine.GetGUIObjectByName("tipText").caption = tipText.map(
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|    |-				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|    |  33|+			text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|  34|  34| 			).join("\n\n");
|  35|  35| 			Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|  36|  36| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|  31|  31| 			Engine.GetGUIObjectByName("tipText").caption = tipText.map(
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|  33| 				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|  34|    |-			).join("\n\n");
|    |  34|+		).join("\n\n");
|  35|  35| 			Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|  36|  36| 	}
|  37|  37| 	else
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|  33| 				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|  34|  34| 			).join("\n\n");
|  35|    |-			Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|    |  35|+		Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|  36|  36| 	}
|  37|  37| 	else
|  38|  38| 		error("Failed to find any matching tips for the loading screen.");

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

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

Harbormaster completed remote builds in B8372: Diff 8907.Jul 15 2019, 2:01 PM
elexis added inline comments.Jul 19 2019, 4:23 PM
source/simulation2/system/ReplayTurnManager.cpp
94 ↗(On Diff #8907)

ToJSVal returns a boolean, in theory it can fail, but then the JS::Value is undefined and that is passed down to the JS GUI, which even sounds like the best case scenario, since the OOS JS message should be sent even if it fails converting the hash std::string to a JS string.

(The two cases where it can return false are)

FAIL("Argument must be convertible to a string");
FAIL("JS_EncodeString failed"); // out of memory

So in case of OOM, good luck receiving that JS message anyways, in case of failed conversion. Also not necessarily convinced that one needs to add the undefined catch for OOM or failed string conversion in the JS GUI. (Adding code to supporting catching OOS without hash?)

elexis added a comment.Jul 19 2019, 4:40 PM

Changing to paramData for consistency as mentioned by Itms on IRC today.

source/gui/IGUIObject.cpp
467 ↗(On Diff #8907)

JS::AutoValueVector paramData(cx);

488 ↗(On Diff #8907)

JS::AutoValueVector paramData(cx);

elexis updated this revision to Diff 8995.Jul 19 2019, 4:45 PM

Rename to paramData.

Vulcan added a comment.Jul 19 2019, 4:58 PM

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

Linter detected issues:
Executing section Source...

source/gui/GUIutil.h
|  27| 
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classGUITooltip{' is invalid C code. Use --std or --language to configure the language.

source/gui/GUIManager.h
|  47| class·CGUIManager
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGUIManager{' is invalid C code. Use --std or --language to configure the language.

source/gui/IGUIObject.h
| 167| »   vector_pObjects::iterator·begin()·{·return·m_Children.begin();·}
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.

source/gui/CGUI.h
|  27| #include·"GUIbase.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classGUITooltip{' is invalid C code. Use --std or --language to configure the language.

source/gui/CGUI.cpp
|1680| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollWheel

source/gui/CGUI.cpp
|1680| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollSpeed

source/gui/CGUI.cpp
|1680| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollButtons
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /zpool0/trunk/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.map(
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /zpool0/trunk/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.map(
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|  33| 				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /zpool0/trunk/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.map(
|    |  31|+		Engine.GetGUIObjectByName("tipText").caption = tipText.map(
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|  33| 				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|  34|  34| 			).join("\n\n");
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 4.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /zpool0/trunk/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.map(
|  32|    |-				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|    |  32|+			// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|  33| 				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|  34|  34| 			).join("\n\n");
|  35|  35| 			Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 4.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|  30|  30| 			Engine.GetGUIObjectByName("tipTitle").caption = tipText.shift();
|  31|  31| 			Engine.GetGUIObjectByName("tipText").caption = tipText.map(
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|    |-				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|    |  33|+			text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|  34|  34| 			).join("\n\n");
|  35|  35| 			Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|  36|  36| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|  31|  31| 			Engine.GetGUIObjectByName("tipText").caption = tipText.map(
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|  33| 				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|  34|    |-			).join("\n\n");
|    |  34|+		).join("\n\n");
|  35|  35| 			Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|  36|  36| 	}
|  37|  37| 	else
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|  33| 				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|  34|  34| 			).join("\n\n");
|  35|    |-			Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|    |  35|+		Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|  36|  36| 	}
|  37|  37| 	else
|  38|  38| 		error("Failed to find any matching tips for the loading screen.");

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

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

Harbormaster completed remote builds in B8459: Diff 8995.Jul 19 2019, 4:58 PM
Itms added a comment.Jul 19 2019, 9:48 PM

The code looks almost perfect to me (I had too many >> problems in the past, it became an obsession), and this works on SM38 and on SM45. Thanks a lot!

source/gui/IGUIObject.cpp
500 ↗(On Diff #8995)

Are we still doing the > > style thing?

Itms accepted this revision.Jul 19 2019, 9:48 PM
This revision is now accepted and ready to land.Jul 19 2019, 9:48 PM
elexis added a comment.Jul 19 2019, 10:37 PM

Thanks for the review!

binaries/data/mods/public/gui/loading/loading.js
82 ↗(On Diff #8995)

(Actually this description thing is nice, also useful for the future)

elexis added inline comments.Jul 19 2019, 10:55 PM
source/gui/IGUIObject.cpp
480 ↗(On Diff #8995)

This variant is only called once by ScriptEvent("worldclick", coords); so this function can be replaced and the function introduced here can be removed.

source/gui/IGUIObject.h
397 ↗(On Diff #8995)

(adding a comment wouldn't have been inappropriate)

elexis updated this revision to Diff 9000.Jul 19 2019, 10:59 PM

Nuke the previous Value function as it costs only 2 more lines to call the new variant.

Vulcan added a comment.Jul 19 2019, 11:11 PM

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

Linter detected issues:
Executing section Source...

source/gui/IGUIObject.h
| 167| »   vector_pObjects::iterator·begin()·{·return·m_Children.begin();·}
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.

source/gui/MiniMap.cpp
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/gui/GUIManager.h
|  47| class·CGUIManager
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGUIManager{' is invalid C code. Use --std or --language to configure the language.

source/gui/GUIutil.h
|  27| 
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classGUITooltip{' is invalid C code. Use --std or --language to configure the language.

source/gui/CGUI.h
|  27| #include·"GUIbase.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classGUITooltip{' is invalid C code. Use --std or --language to configure the language.

source/gui/CGUI.cpp
|1680| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollWheel

source/gui/CGUI.cpp
|1680| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollSpeed

source/gui/CGUI.cpp
|1680| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollButtons
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /zpool0/trunk/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.map(
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /zpool0/trunk/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.map(
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|  33| 				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /zpool0/trunk/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.map(
|    |  31|+		Engine.GetGUIObjectByName("tipText").caption = tipText.map(
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|  33| 				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|  34|  34| 			).join("\n\n");
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 4.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /zpool0/trunk/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.map(
|  32|    |-				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|    |  32|+			// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|  33| 				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|  34|  34| 			).join("\n\n");
|  35|  35| 			Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 4.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|  30|  30| 			Engine.GetGUIObjectByName("tipTitle").caption = tipText.shift();
|  31|  31| 			Engine.GetGUIObjectByName("tipText").caption = tipText.map(
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|    |-				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|    |  33|+			text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|  34|  34| 			).join("\n\n");
|  35|  35| 			Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|  36|  36| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|  31|  31| 			Engine.GetGUIObjectByName("tipText").caption = tipText.map(
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|  33| 				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|  34|    |-			).join("\n\n");
|    |  34|+		).join("\n\n");
|  35|  35| 			Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|  36|  36| 	}
|  37|  37| 	else
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/loading/loading.js
|  32|  32| 				// Translation: A bullet point used before every item of list of tips displayed on loading screen
|  33|  33| 				text => text && sprintf(translate("• %(tiptext)s"), { "tiptext": text })
|  34|  34| 			).join("\n\n");
|  35|    |-			Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|    |  35|+		Engine.GetGUIObjectByName("tipImage").sprite = "stretched:" + g_TipsImagePath + tipFile + ".png";
|  36|  36| 	}
|  37|  37| 	else
|  38|  38| 		error("Failed to find any matching tips for the loading screen.");

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

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

Harbormaster completed remote builds in B8465: Diff 9000.Jul 19 2019, 11:11 PM
Closed by commit rP22515: Pass arbitrary arguments to GUI events / SendEventToAll and remove loading… (authored by elexis). · Explain WhyJul 19 2019, 11:15 PM
This revision was automatically updated to reflect the committed changes.
elexis mentioned this in D2410: Send NetClient GUIMessages to subscribing GUIObjects instead of pulling them onTick.Nov 7 2019, 3:24 PM

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
mods/
public/
gui/
loading/
13 lines
1 line
session/
6 lines
2 lines
source/
gui/
8 lines
8 lines
3 lines
7 lines
15 lines
4 lines
16 lines
6 lines
ps/
GameSetup/
16 lines
simulation2/
system/
31 lines

Diff 9001

View Options

ps/trunk/binaries/data/mods/public/gui/loading/loading.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/loading/loading.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/session/messages.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/session/session.xml

Loading...
View Options

ps/trunk/source/gui/CGUI.h

Loading...
View Options

ps/trunk/source/gui/CGUI.cpp

Loading...
View Options

ps/trunk/source/gui/GUIManager.h

Loading...
View Options

ps/trunk/source/gui/GUIManager.cpp

Loading...
View Options

ps/trunk/source/gui/GUIutil.h

Loading...
View Options

ps/trunk/source/gui/IGUIObject.h

Loading...
View Options

ps/trunk/source/gui/IGUIObject.cpp

Loading...
View Options

ps/trunk/source/gui/MiniMap.cpp

Loading...
View Options

ps/trunk/source/ps/GameSetup/GameSetup.cpp

Loading...
View Options

ps/trunk/source/simulation2/system/ReplayTurnManager.cpp

Loading...
Wildfire Games · Phabricator