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.
Details
- Reviewers
- None
- Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Trac Tickets
- #5340
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 9340 Build 15471: Vulcan Build Jenkins Build 15470: Vulcan Build (Windows) Jenkins Build 15469: arc lint + arc unit
Event Timeline
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/differential/769/
Looks good. I was wondering about those Eval as well when I was doing the line color patch.
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/770/
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 | ||
---|---|---|
55 | 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.
source/lobby/IXmppClient.h | ||
---|---|---|
55 | Shouldn't the D1668 be closed then? | |
source/lobby/LobbyScriptConversions.cpp | ||
1 ↗ | (On Diff #6971) | Usually we put *ScriptConversions files into scripting subfolder, example: simulation2/scripting/EngineScriptConversions.cpp. |
26 ↗ | (On Diff #6971) | 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 ↗ | (On Diff #6971) | 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 | ||
547 | 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. |
(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 | ||
---|---|---|
55 | Every source I checked says that RVO works well for strings, but I didn't see anything about JS::Values.
such as JSInterface_VisualReplay which returns a JS::Value instead of setting a MutableHandleValue. | |
source/lobby/LobbyScriptConversions.cpp | ||
1 ↗ | (On Diff #6971) | ah, yess |
31 ↗ | (On Diff #6971) | 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? |
Rebase following rP22891 and 2019.
Adding support for child tags would really be great to remove the JSON, but will need some invention.
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/147/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/lobby/XmppClient.h | 24| | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'namespaceStunClient{' is invalid C code. Use --std or --language to configure the language. source/lobby/glooxwrapper/glooxwrapper.h | 88| namespace·glooxwrapper | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'namespaceglooxwrapper{' is invalid C code. Use --std or --language to configure the language. source/lobby/IXmppClient.h | 24| namespace·StunClient·{ | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'namespaceStunClient{' is invalid C code. Use --std or --language to configure the language. Executing section JS... Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/657/display/redirect
Since this guarantee the properties to exist anymore (wicked client inserting prepared game), the JS code needs to account for that, meh.
Still seems better to do that in JS than hardcoding all JS properties in C++.
Each time JS side wanted to add a new property, it had to change C++ code.
Mods would be allowed to pass their own JS, without having to hijack one of the existing properties.
(The mods can already send the custom properties, they just can't be received solely due to the C++ hardcoding.)
For example maxnbp in the afore mentioned rP18534.
source/lobby/XmppClient.cpp | ||
---|---|---|
603 | Notice that the entire function could be removed if we would allow ourselves to bind JSInterface_ to XmppClient class instead of the IXmppClient interface. It would mean that people who want to implement IXmppClient with a different backend to gloox / glooxwrapper would have to provide their own interface as well. The functions would then be registered in the implementation class of the interface class, so that would be easy to forget or get out of sync as it would be removed from the interface IXmppClass. Shame, silly proxy getter. | |
source/lobby/XmppClient.h | ||
189 | Gloox->Glooxwrapper | |
source/lobby/glooxwrapper/glooxwrapper.h | ||
590 | Returning gloox tags sounds to defeat the purpose of the glooxwrapper, should wrap the tags. From gloox tag.h:
We only need the keys and I dont want to mirror / wrap the entire Attribute class now, -> attributeNames() | |
source/lobby/scripting/GlooxScriptConversions.cpp | ||
61 | TOJSVAL_VECTOR(const glooxwrapper::Tag*) (needs macro split) | |
75 | Would be shorter with ScriptInterface::SetProperty due to the implicit ToJSVal conversion. |
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/168/display/redirect
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/682/display/redirect