spix icon indicating copy to clipboard operation
spix copied to clipboard

add the ability to invoke QML functions

Open prototypicalpro opened this issue 2 years ago • 5 comments

This PR adds the ability to invoke arbitrary QML methods for testing purposes. For example:

TextArea {
  id: textArea
  function test(arg) {
    ...
    return {'myvalue': [3, true, 7.0]}
  }
}
s = xmlrpc.client.ServerProxy('http://localhost:9000')
# invoke a built-in method to change the selection area
s.invokeMethod('window/textArea', 'select', [200, 300])
# invoke a custom method
print(s.invokeMethod('window/textArea', 'test', ['myarg'])) # prints "{'myvalue': [3, True, 7.0]}"

I'm posting this PR as a draft to get more feedback on my approach before I continue. More specifically, I have written a lot of code devoted to massaging types (Value -> QVariant -> parameter type and vice versa) because QMetaObject::invokeMethod and QMetaMethod::invoke both require the parameter types to exactly match the QML interface. For instance when attempting to invoke TextArea.select(int, int):

QMetaObject::invoke(textArea, "select", Q_ARG(int, 100), Q_ARG(int, 200)); // ok
QMetaObject::invoke(textArea, "select", Q_ARG(QVariant, QVariant::fromValue(100)), Q_ARG(QVariant, QVariant::fromValue(200))); // fails
QMetaObject::invoke(textArea, "select" Q_ARG(QJSValue, ...), Q_ARG(QJSValue, ...)); // also fails

Because of this restriction my code must first search for a matching method interface, perform the numerous conversion steps to put the arguments in, and do the conversion again in reverse with the return value. I have implemented this and it works but is rather cumbersome: I'd appreciate ideas on how to improve this process. One lead I've investigated is finding a way to invoke QML functions JavaScript-style (with no conversions or type checks) using the QJSValue or QJSEngine API, but thus far I haven't found anything spix could use.

TODO before this PR is merges (in addition to any feedback):

  • [x] Qt5 support (currently only tested on Qt6 and there are some major differences with the meta typing systems).
  • [x] Tests
  • [x] Docs

prototypicalpro avatar Apr 13 '22 00:04 prototypicalpro

Thanks for the detailed review!

I know it's an additional step, but I would prefer if everything from anyrpc was first converted to a std::variant and then, in the Scene/Qt/* classes, to a Qt object.

Sure, that makes sense to me. I initially stuck to anyrpc::Value since std::variant requires C++17, but if you'd prefer the abstraction layer and are good with bumping the C++ version I'd be happy to make that change. QVariant also has some integrations with std::variant so that should remove some conversion logic.

I found one SO-post where a QVariant is used for the arguments and return type of invokeMethod, this would avoid the need for the special variant/conversion struct you have. Only some freestanding functions for converting a QVariant would be needed.

I did look into this, and as you mentioned this method works only for functions that don't have type annotations:

function test(arg) {} // QVariant works
function test(arg: int) {} // QVariant doesn't work
function test(arg): int {} // QVariant doesn't work

This isn't really a problem for user-defined functions, but it becomes annoying when dealing with Qt-defined functions since almost all of them have type annotations: TextArea for example has type annotations for every method. A user-defined proxy method would fix this issue, but personally I'd rather not define a proxy method every time I want to call a Qt-defined QML function. I'll take a look at implementing the return type as std::variant and maybe it'll be good enough until Qt updates invokeMethod.

I think as a first step it would help to move as much as possible of the "invokeMethod" related code into a separate file. This would then provide a clean API that QtItem can use to call methods without having to take care of the type conversion business. At least this would move the "cumbersome" stuff into a separate part of the library.

For sure, I'll tackle this on my next pass.

prototypicalpro avatar Apr 19 '22 20:04 prototypicalpro

Sure, that makes sense to me. I initially stuck to anyrpc::Value since std::variant requires C++17, but if you'd prefer the abstraction layer and are good with bumping the C++ version I'd be happy to make that change. QVariant also has some integrations with std::variant so that should remove some conversion logic.

@prototypicalpro I think bumping the C++ version to 17 is fine 👍

I did look into this, and as you mentioned this method works only for functions that don't have type annotations:

Too bad. I was hoping there would already be some magic in place to map any type to the QVariant 😞

Thanks for looking into this!

faaxm avatar Apr 23 '22 21:04 faaxm

I implemented an intermediary variant (spix::Variant) to preserve the abstraction layer between anyrpc, spix, and Qt: spix::Variant is a union of all the basic anyrpc types, including a map of itself and a list of itself. I was hoping that the conversions between the variant types would be a bit more elegant, but it turns out std::visit only supports std::variant and not classes inheriting from std::variant (at least until recently), so it looks like we're stuck with switch statements for now :pensive:.

In addition to the above:

  • Moves some logic out of QtItem::invokeMethod. Replaced the return type tagged union with an std::variant.
  • Refactored AnyRPCUtils: now split into AnyRPCUtils (type conversion stuff) and AnyRPCFunction (anyrpc registration stuff). Refactored callAndAssignAnyRpcResult to use if constexpr instead of SFINAE.
  • Updates QMetaType stuff to be compatible with Qt5.
  • Bumped to C++17.
  • Added a log line when spix starts: "Spix server is enabled. Only use this in a safe environment." to take after the QML debugging warning (I can make this it's own PR if you want).

I think concept-wise this PR is good pending review. I'll work on adding docs, tests, and an example next week.

Edit: returning objects in Qt5 doesn't appear to be working, so I'll have to check that out as well.

prototypicalpro avatar Apr 26 '22 20:04 prototypicalpro

I added tests and some text in the README, and with that I think this PR is good to go!

prototypicalpro avatar May 17 '22 19:05 prototypicalpro

Sorry for the delay... I have lots of stuff going on right now, but I will get to this PR eventually 😕

faaxm avatar Jun 11 '22 09:06 faaxm