-
Notifications
You must be signed in to change notification settings - Fork 279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix invokeMethod issues with Qt 6.5 #778
Conversation
The only better fix I can think of is to use the private |
Since Qt 6.5 there is a new variadic version of the QMetaObject::invokeMethod() method [0]. Generally this is a good idea, as this fixes the problem of being able to pass a maximum number of only 10 arguments to invokeMethod(). However for usage in GammaRay this poses a problem: When we call invokeObjectLocal() from within the inspected process, we can no longer just dump all arguments, as the variadic invokeMethod() version will then always attempt to call a version of the method with exactly 10 arguments of type GammaRay::MethodArgument [1]. The fact that the argument number is fixed (and usually wrong) is not the main problem here, as it could easily be worked around by adding 10 if branches that select the appropriate invokeMethod() with the correct arity. The more serious problem is that the argument type is always fixed to GammaRay::MethodArgument [2]. We can't easily work around this as Q_ARG() wrappers and similar shenanigans need the type at compile time, but we don't know the type at compile time. Luckily the non-variadic version of QMetaObject::invokeMethod still exists, so as a workaround we force the usage of the non-variadic version by explicitly passing the arguments as QGenericArgument. Note that this version of QMetaObject::invokeMethod is obsolete since Qt 6.5 and might be removed with Qt 7.0, in which case we need to find a better fix. As for the actual implementation in this patch, note that we need to store the MethodArgument first and can't just construct the QGenericArgument directly. Alternatively we could wrap every parameter in the function call to invokeMethod() with QGenericArgument(), but using a second array is a little bit cleaner code-wise. Also convert the QVector to a C-style array, as we always allocate and need all 10 elements anyway. Fixes KDAB#777 Fixes KDAB#761 [0] https://codereview.qt-project.org/c/qt/qtbase/+/422745 [1] see KDAB#777 (comment) [2] e.g.: QMetaObject::invokeMethod: No such method GammaRay::ToolManagerClient::availableToolsResponse(GammaRay::MethodArgument) Candidates are: availableToolsResponse(QList<GammaRay::ToolData>)
a5a17b1
to
a9365a3
Compare
Not sure if it's really better to use private API here. With this current patch we have at least the guarantee that it will continue working until at least Qt 7.0. If it changes then, |
For now yes, as the public API essentially does that for us internally |
Another thing to keep in mind (although this is unrelated to the change in this PR) is that for some strange reason with Qt 6.5 the signal
Not sure if this is a bug in Qt, because As a workaround the following diff fixes the problem in GammaRay: diff --git a/plugins/quickinspector/quickinspectorinterface.h b/plugins/quickinspector/quickinspectorinterface.h
index a1d19474b..c98af8438 100644
--- a/plugins/quickinspector/quickinspectorinterface.h
+++ b/plugins/quickinspector/quickinspectorinterface.h
@@ -86,7 +86,7 @@ public slots:
virtual void setSlowMode(bool slow) = 0;
signals:
- void features(GammaRay::QuickInspectorInterface::Features features);
+ void features(QFlags<GammaRay::QuickInspectorInterface::Feature> features);
void serverSideDecorationChanged(bool enabled);
void overlaySettings(const GammaRay::QuickDecorationsSettings &settings);
void slowModeChanged(bool slow); But perhaps this is better fixed in Qt. |
I think this is https://codereview.qt-project.org/c/qt/qtbase/+/399035 I have already put some work arounds for our property view to make this work. Perhaps we can register the alias metatype and it will work? |
There already is a It also doesn't work if I Any other ideas? |
To register an alias, I think it should be |
That unfortunately doesn't work either. :( |
It looks like this problem is not just with
Perhaps we can send some more type info, so that we can construct the type for |
A simpler and likely more robust solution for the above is to just use the real type. Aliases are just sugar anyways. |
Since https://codereview.qt-project.org/c/qt/qtbase/+/422745 there is a new variadic version of QMetaObject::invokeMethod().
However this new variadic version is not compatible with our usage in
Endpoint::invokeObjectLocal()
, as we just dump all 10 parameters at once without caring about the actual function arity.Theoretically this could be fixed by manually adding 10 if-branches for the correct arity.
However the second much bigger issue is that we don't know the type at compile time, and thus would end up hardcoding all parameter types to
MethodArgument
.Hence to fix this, we revert back to the non-variadic version by explicitly calling
QMetaObject::invokeMethod()
withQGenericArgument
as parameters.Please see the commit message for more details.
Fixes #777