Skip to content
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.

Sign up for GitHub

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

Jump to bottom

qtcollider: QcSignalSpy prevent creating QVariant<QVariant> #5216

Merged
merged 1 commit into from
Nov 1, 2020
Merged

qtcollider: QcSignalSpy prevent creating QVariant<QVariant> #5216

merged 1 commit into from
Nov 1, 2020

Conversation

elgiano
Copy link
Contributor

@elgiano elgiano commented Oct 25, 2020

Purpose and Motivation

I encountered a problem while working on #4883 : when sclang calls WebView::runJavascript, it would fail to get back return values. I digged into it and this is how it works:

  • A Function (the callback) is connected to a Qt signal (the onCalled signal of QcCallback)
  • A QcSignalSpy is connected to the signal
  • When the signal fires, QcSignalSpy gets its arguments, and converts them to a QList to be passed to QtCollider::runLang and from there back to sclang. Since QcSignalSpy receives arguments as a (void**) and their types as a separate argument, it wraps each of them into a QVariant and pushes it to the list.

The problem is that the callback for runJavascript returns a QVariant already, that when wrapped becomes a QVariant, which QtCollider::runLang doesn't know how to translate into an object sclang can use.

So, in this PR, I'm just checking in QcSignalSpy if an argument is already a QVariant, preventing it then to being made into a QVariant

Types of changes

  • Bug fix
  • Code is tested
  • All tests are passing
  • Updated documentation
  • This PR is ready for review

@elgiano elgiano force-pushed the topic/QCallbackQVariant branch from 10c9bef to e95df8e Compare October 25, 2020 12:15
@elgiano elgiano added comp: sclang sclang C++ implementation (primitives, etc.). for changes to class lib use "comp: class library" qt5 labels Oct 25, 2020
mossheim
mossheim suggested changes Oct 31, 2020
View reviewed changes
Copy link
Contributor

@mossheim mossheim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! would you mind also undoing the change you made in #4883 to get around this?

QtCollider/QcSignalSpy.h
args << QVariant(type, argData[i + 1]);
if (type == QMetaType::QVariant) {
// avoid creating a QVariant<QVariant>
args << QVariant(type, argData[i + 1]).value<QVariant>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is exactly the same, just fewer steps :)

Suggested change
args << QVariant(type, argData[i + 1]).value<QVariant>();
args << argData[i + 1]);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also what I would think, but then it complains that QVariant(void*) is a deleted function.

QcSignalSpy.h:96:40: error: use of deleted function ‘QVariant::QVariant(void*)’
   96 |                     args << argData[i+1];

/usr/include/qt/QtCore/qvariant.h:499:12: note: declared here
  499 |     inline QVariant(void *) = delete;

Does this mean that Qt explicitly forbids "casting" a (void*) to QVariant?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh i missed that was the type of argData (void**). yeah, this is fine the way it is, i think this would also work:

Suggested change
args << QVariant(type, argData[i + 1]).value<QVariant>();
args << *static_cast<const QVariant*>(argData[i + 1]);

but that's just a hunch.

inline QVariant(void *) = delete; is a constructor for QVariant, when marked with "delete" it means the compiler will forbid calling it.

@elgiano
Copy link
Contributor Author

elgiano commented Oct 31, 2020 •
edited
Loading

thanks for looking at this @brianlheim!
About undoing the changes: #4883 is now in 3.11, while this PR is against develop. Shall I rebase this against 3.11 or wait until 3.11 is merged into develop? Oh, I see it already happened, I'll undo the changes now :)

And with this PR we still can't pass a QMap back to sclang. Thinking about the use-case with javascript, I would suggest that the user serializes a map into a JSON string, which can then be deserialized in sclang. Just thought we could add this to WebView::-runJavascript documentation, and then we are good to go for me.

@elgiano elgiano force-pushed the topic/QCallbackQVariant branch from e95df8e to fb29fda Compare October 31, 2020 17:38
@mossheim
Copy link
Contributor

And with this PR we still can't pass a QMap back to sclang. Thinking about the use-case with javascript, I would suggest that the user serializes a map into a JSON string, which can then be deserialized in sclang. Just thought we could add this to WebView::-runJavascript documentation, and then we are good to go for me.

ok.. by "JSON string" do you just mean a string that holds a JSON object? this was a bug before and after #4883 right? if so, don't try to fix it here.

@elgiano
Copy link
Contributor Author

elgiano commented Oct 31, 2020

Before #4883 return values from javascript were completely broken (IIRC): we couldn't even return strings.
With #4883 we would convert any return type to string and always return strings.
After this PR, we can return all javascript types (I could be wrong, maybe I forgot some), except Objects, which translate to QMaps, which we can't translate for sclang yet.

I wouldn't fix this in any way here, definitely needs more thinking, for now I would just update documentation about it.

@mossheim
Copy link
Contributor

ok, sounds good!

mossheim
mossheim approved these changes Nov 1, 2020
View reviewed changes
Copy link
Contributor

@mossheim mossheim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@mossheim mossheim merged commit d808a45 into supercollider:develop Nov 1, 2020
@elgiano elgiano deleted the topic/QCallbackQVariant branch November 1, 2020 16:29
@elgiano elgiano mentioned this pull request Nov 1, 2020
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mossheim mossheim mossheim approved these changes

Assignees
No one assigned
Labels
comp: sclang sclang C++ implementation (primitives, etc.). for changes to class lib use "comp: class library" qt5
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@elgiano @mossheim