Page MenuHomeWildfire Games

Lobby ScriptConversion to remove stanza property name hardcoding and duplication
Changes PlannedPublic

Authored by elexis on Nov 9 2018, 4:53 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Trac Tickets
#5340
Summary

Whenever the JS lobby developers wanted to show a new property in the gamelist stanza, the XmppClient.cpp file had to be updated with a hardcoded list of property names.
But these properties can be read out directly, there is no reason to hardcode them.
After the removal of hardcoded property names, the functions are identical, and the remains are a ScriptConversion function as known from GuiScriptConversions.cpp, EngineScriptConversions.cpp, ScriptConversions.cpp etc.
The "push" and Eval calls are quite ugly too, see #5340.

Test Plan

Make sure that the JS side doesn't rely on all properties being given.
If the stanza must contain all property names, that should be ensured in some place that isn't code-logic.

Diff Detail

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

Event Timeline

elexis created this revision.Nov 9 2018, 4:53 PM
Vulcan added a subscriber: Vulcan.Nov 9 2018, 4:56 PM

Build failure - The Moirai have given mortals hearts that can endure.

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

elexis updated this revision to Diff 6971.Nov 9 2018, 5:00 PM
elexis marked an inline comment as done.

Missing new file

Stan added a subscriber: Stan.Nov 9 2018, 5:33 PM

Looks good. I was wondering about those Eval as well when I was doing the line color patch.

Stan added a reviewer: Restricted Owners Package.Nov 9 2018, 5:35 PM
Vulcan added a comment.Nov 9 2018, 5:44 PM

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

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

elexis marked 2 inline comments as done.Wed, Nov 14, 12:26 PM

Notice all of this and the converse conversion work very well this way as long as we only work with attributes.

But we should probably use children instead of injecting JSON for the players and mods of the gamelist stanza, refs
rP18534 anymore. (https://trac.wildfiregames.com/ticket/3476#comment:11, irc 2016-06-15, 2016-06-16, 2016-09-02)
So perhaps the generic JS<->Stanza conversion approach must be dumped, as gloox Tag children (named) are not really the same as array items (no name).
XpartaMuPP also needs changes to work with children.

source/lobby/IXmppClient.h
57

refs D1668. If RVO doesnt work here, it should be changed for the other JSI too

This is what a generic JS<->gloox:Tag converison would look like, revealing the array problem:

template<> bool ScriptInterface::FromJSVal<glooxwrapper::Tag>(JSContext* cx, JS::HandleValue value, glooxwrapper::Tag& tag)
{
	JSAutoRequest rq(cx);

	if (!value.isObject())
		FAIL("the given value is not an object!");

	JS::RootedObject object(cx, &value.toObject());
	JS::AutoIdArray properties(cx, JS_Enumerate(cx, object));
	if (!properties)
		FAIL("the properties of the given object could not be enumerated!");

	for (size_t i = 0; i < properties.length(); ++i)
	{
		JS::RootedId propertyID(cx, properties[i]);
		JS::RootedValue propertyIDValue(cx);

		if (!JS_IdToValue(cx, propertyID, &propertyIDValue))
			FAIL("a property ID of the given object could not be converted to a value!");

		std::string propertyName;

		// TODO: numeric properties
		if (JS_IsArrayObject(cx, object))
			propertyName = "item";// + propertyName;
		else if (!ScriptInterface::FromJSVal(cx, propertyIDValue, propertyName))
			FAIL("a property ID value of the given object could not be converted to a string!");

		debug_printf("prop %s\n", propertyName.c_str());

		JS::RootedValue propertyValue(cx);
		if (!JS_GetPropertyById(cx, object, propertyID, &propertyValue))
			FAIL("a property value could not be read!");

		switch (JS_TypeOfValue(cx, propertyValue))
		{
		case JSTYPE_NUMBER: // fall through
		case JSTYPE_STRING:
		{
			std::string propertyValueString;
			if (!ScriptInterface::FromJSVal(cx, propertyValue, propertyValueString))
				FAIL("a property string could not be read!");

			if (JS_IsArrayObject(cx, object))
			{
				glooxwrapper::Tag* childTag = glooxwrapper::Tag::allocate("item");
				if (!ScriptInterface::FromJSVal(cx, propertyValue, *childTag))
					FAIL("a property that is a string could not be added as an array item!");

				tag.addChild(childTag);
			}
			else
				tag.addAttribute(glooxwrapper::string(propertyName), glooxwrapper::string(propertyValueString));

			break;
		}
		case JSTYPE_OBJECT:
		{
			glooxwrapper::Tag* childTag = glooxwrapper::Tag::allocate("item");
			if (!ScriptInterface::FromJSVal(cx, propertyValue, *childTag))
				FAIL("a property that is an object could not be read!");

			tag.addChild(childTag);
			break;
		}
		case JSTYPE_BOOLEAN:
			FAIL("TODO boolean");
		case JSTYPE_VOID:
			FAIL("TODO undefined");
		case JSTYPE_NULL:
			FAIL("TODO null");
		case JSTYPE_FUNCTION:
			FAIL("a property value is a function!");
		}
	}

	return true;
}

It's also intended by SleekXMPP that stanzas are parsed specifically. One needs to specify in advance which child elements (arrays) can be specified before they can be read.

vladislavbelov added inline comments.
source/lobby/IXmppClient.h
57

Shouldn't the D1668 be closed then?

source/lobby/LobbyScriptConversions.cpp
1

Usually we put *ScriptConversions files into scripting subfolder, example: simulation2/scripting/EngineScriptConversions.cpp.

26

The pretty long line, >120 symbols. Would it be better to format it a little bit? Like this:

template<>
void ScriptInterface::ToJSVal<GlooxTagList>(
	JSContext* cx, JS::MutableHandleValue ret, const GlooxTagList& tagList)
31

Wrong type here, JS_SetElement accepts uint32_t: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_SetElement. So the type have to be u32.

source/lobby/XmppClient.cpp
508

u32.

source/lobby/XmppClient.h
30

If using is allowed, we need to use using instead of typedef.

source/lobby/scripting/JSInterface_Lobby.cpp
190

Here and below: operator −> has a higher priority than the *, so parenthesis are not needed. But it's not a problem I think.

elexis planned changes to this revision.Thu, Nov 15, 11:57 AM
elexis marked 5 inline comments as done.

(To repeat) The current patch and the patch posted in the comments conceptually cannot work with child elements, while child elements should be used to remove the JSON encoded arrays in stanzas.

source/lobby/IXmppClient.h
57

Every source I checked says that RVO works well for strings, but I didn't see anything about JS::Values.

it should be changed for the other JSI too

such as JSInterface_VisualReplay which returns a JS::Value instead of setting a MutableHandleValue.

source/lobby/LobbyScriptConversions.cpp
1

ah, yess

31

yes

source/lobby/XmppClient.h
30

using supports templates whereas typedefdoesn't, or where is the difference? And then using is preferred because every typedef can become using, so the entire codebase uses less vocabulary if every typedef becomes using, or what is the reasoning?