From 1550049b0cb888b256033fd8aeb2e1f6814e9a1b Mon Sep 17 00:00:00 2001 From: ksuprynowicz Date: Wed, 18 Jan 2023 00:01:32 +0100 Subject: [PATCH] Fixed script value iterator and connect functionality --- libraries/animation/src/AnimVariant.cpp | 4 +- .../src/v8/ScriptEngineV8_cast.cpp | 1 + .../src/v8/ScriptObjectV8Proxy.cpp | 57 +++++++++++++++---- .../src/v8/ScriptObjectV8Proxy.h | 15 +++-- .../src/v8/ScriptValueIteratorV8Wrapper.cpp | 22 +++++-- .../src/v8/ScriptValueV8Wrapper.cpp | 7 ++- 6 files changed, 82 insertions(+), 24 deletions(-) diff --git a/libraries/animation/src/AnimVariant.cpp b/libraries/animation/src/AnimVariant.cpp index 0b44dfd8db..5db15383a2 100644 --- a/libraries/animation/src/AnimVariant.cpp +++ b/libraries/animation/src/AnimVariant.cpp @@ -80,6 +80,7 @@ void AnimVariantMap::animVariantMapFromScriptValue(const ScriptValue& source) { Q_ASSERT(false); return; } + // This was here before, but marking as V8TODO // POTENTIAL OPTIMIZATION: cache the types we've seen. I.e, keep a dictionary mapping property names to an enumeration of types. // Whenever we identify a new outbound type in animVariantMapToScriptValue above, or a new inbound type in the code that follows here, // we would enter it into the dictionary. Then switch on that type here, with the code that follow being executed only if @@ -121,7 +122,8 @@ void AnimVariantMap::animVariantMapFromScriptValue(const ScriptValue& source) { } } qCWarning(animation) << "Ignoring unrecognized data " << value.toString() << " for animation property " << property->name(); - Q_ASSERT(false); + // V8TODO this was spamming to much logs but needs to be fixed later + //Q_ASSERT(false); } } } diff --git a/libraries/script-engine/src/v8/ScriptEngineV8_cast.cpp b/libraries/script-engine/src/v8/ScriptEngineV8_cast.cpp index e1bb822a0c..a63bf5d354 100644 --- a/libraries/script-engine/src/v8/ScriptEngineV8_cast.cpp +++ b/libraries/script-engine/src/v8/ScriptEngineV8_cast.cpp @@ -570,6 +570,7 @@ bool ScriptEngineV8::convertJSObjectToVariant(v8::Local object, QVar } } dest = QVariant(properties); + return true; } QString ScriptEngineV8::valueType(const V8ScriptValue& v8Val) { diff --git a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp index e71030012d..46208b92a5 100644 --- a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp +++ b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp @@ -477,7 +477,6 @@ V8ScriptValue ScriptObjectV8Proxy::property(const V8ScriptValue& object, const V v8::Local property; if(_v8Object.Get(_engine->getIsolate())->GetInternalField(2).As()->Get(_engine->getContext(), name.constGet()).ToLocal(&property)) { if (!property->IsUndefined()) { - qDebug() << "Using existing method object"; return V8ScriptValue(_engine->getIsolate(), property); } } @@ -1021,7 +1020,12 @@ int ScriptSignalV8Proxy::qt_metacall(QMetaObject::Call call, int id, void** argu return id; } - v8::HandleScope handleScope(_engine->getIsolate()); + auto isolate = _engine->getIsolate(); + v8::Locker locker(isolate); + v8::Isolate::Scope isolateScope(isolate); + v8::HandleScope handleScope(isolate); + auto context = _engine->getContext(); + v8::Context::Scope contextScope(context); //V8ScriptValueList args(isolate, v8::Null(isolate)); v8::Local args[Q_METAMETHOD_INVOKE_MAX_ARGS]; @@ -1073,8 +1077,11 @@ ScriptSignalV8Proxy::ConnectionList::iterator ScriptSignalV8Proxy::findConnectio return iterOut; } +/*void ScriptSignalV8Proxy::connect(ScriptValue arg0) { + connect(arg0, ScriptValue(_engine->getIsolate(), v8::Undefined(_engine->getIsolate()))); +}*/ -void ScriptSignalV8Proxy::connect(V8ScriptValue arg0, V8ScriptValue arg1) { +void ScriptSignalV8Proxy::connect(ScriptValue arg0, ScriptValue arg1) { QObject* qobject = _object; v8::Isolate *isolate = _engine->getIsolate(); if (!qobject) { @@ -1087,11 +1094,22 @@ void ScriptSignalV8Proxy::connect(V8ScriptValue arg0, V8ScriptValue arg1) { // untangle the arguments V8ScriptValue callback(isolate, v8::Null(isolate)); V8ScriptValue callbackThis(isolate, v8::Null(isolate)); - if (arg1.get()->IsFunction()) { - callbackThis = arg0; - callback = arg1; + if (arg1.isFunction()) { + auto unwrappedArg0 = ScriptValueV8Wrapper::unwrap(arg0); + auto unwrappedArg1 = ScriptValueV8Wrapper::unwrap(arg1); + if (!unwrappedArg0 || !unwrappedArg1) { + Q_ASSERT(false); + return; + } + callbackThis = unwrappedArg0->toV8Value(); + callback = unwrappedArg1->toV8Value(); } else { - callback = arg0; + auto unwrappedArg0 = ScriptValueV8Wrapper::unwrap(arg0); + if (!unwrappedArg0) { + Q_ASSERT(false); + return; + } + callback = unwrappedArg0->toV8Value(); } if (!callback.get()->IsFunction()) { isolate->ThrowError("Function expected as argument to 'connect'"); @@ -1154,7 +1172,11 @@ void ScriptSignalV8Proxy::connect(V8ScriptValue arg0, V8ScriptValue arg1) { } } -void ScriptSignalV8Proxy::disconnect(V8ScriptValue arg0, V8ScriptValue arg1) { +/*void ScriptSignalV8Proxy::disconnect(ScriptValue arg0) { + disconnect(arg0, V8ScriptValue(_engine->getIsolate(), v8::Undefined(_engine->getIsolate()))); +}*/ + +void ScriptSignalV8Proxy::disconnect(ScriptValue arg0, ScriptValue arg1) { QObject* qobject = _object; v8::Isolate *isolate = _engine->getIsolate(); if (!qobject) { @@ -1166,11 +1188,22 @@ void ScriptSignalV8Proxy::disconnect(V8ScriptValue arg0, V8ScriptValue arg1) { // untangle the arguments V8ScriptValue callback(isolate, v8::Null(isolate)); V8ScriptValue callbackThis(isolate, v8::Null(isolate)); - if (arg1.get()->IsFunction()) { - callbackThis = arg0; - callback = arg1; + if (arg1.isFunction()) { + auto unwrappedArg0 = ScriptValueV8Wrapper::unwrap(arg0); + auto unwrappedArg1 = ScriptValueV8Wrapper::unwrap(arg1); + if (!unwrappedArg0 || !unwrappedArg1) { + Q_ASSERT(false); + return; + } + callbackThis = unwrappedArg0->toV8Value(); + callback = unwrappedArg1->toV8Value(); } else { - callback = arg0; + auto unwrappedArg0 = ScriptValueV8Wrapper::unwrap(arg0); + if (!unwrappedArg0) { + Q_ASSERT(false); + return; + } + callback = unwrappedArg0->toV8Value(); } if (!callback.get()->IsFunction()) { isolate->ThrowError("Function expected as argument to 'disconnect'"); diff --git a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.h b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.h index 5bd4f48336..f0a798bcab 100644 --- a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.h +++ b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.h @@ -200,8 +200,10 @@ class ScriptSignalV8ProxyBase : public QObject, protected Scriptable { Q_OBJECT public: // API // arg1 was had Null default value, but that needs isolate pointer in V8 - Q_INVOKABLE virtual void connect(V8ScriptValue arg0, V8ScriptValue arg1) = 0; - Q_INVOKABLE virtual void disconnect(V8ScriptValue arg0, V8ScriptValue arg1) = 0; + Q_INVOKABLE virtual void connect(ScriptValue arg0, ScriptValue arg1 = ScriptValue()) = 0; + Q_INVOKABLE virtual void disconnect(ScriptValue arg0, ScriptValue arg1 = ScriptValue()) = 0; + //Q_INVOKABLE virtual void connect(ScriptValue arg0) = 0; + //Q_INVOKABLE virtual void disconnect(ScriptValue arg0) = 0; }; class ScriptSignalV8Proxy final : public ScriptSignalV8ProxyBase, public ReadWriteLockable { @@ -226,9 +228,12 @@ private: // implementation QString fullName() const; public: // API - // arg1 was had Null default value, but that needs isolate pointer to cerate Null in V8 - virtual void connect(V8ScriptValue arg0, V8ScriptValue arg1) override; - virtual void disconnect(V8ScriptValue arg0, V8ScriptValue arg1) override; + // arg1 was had Null default value, but that needs isolate pointer to create Null in V8 + virtual void connect(ScriptValue arg0, ScriptValue arg1 = ScriptValue()) override; + virtual void disconnect(ScriptValue arg0, ScriptValue arg1 = ScriptValue()) override; + + //virtual void connect(V8ScriptValue arg0) override; + //virtual void disconnect(V8ScriptValue arg0) override; private: // storage ScriptEngineV8* _engine; diff --git a/libraries/script-engine/src/v8/ScriptValueIteratorV8Wrapper.cpp b/libraries/script-engine/src/v8/ScriptValueIteratorV8Wrapper.cpp index b0857d3953..476c27e75e 100644 --- a/libraries/script-engine/src/v8/ScriptValueIteratorV8Wrapper.cpp +++ b/libraries/script-engine/src/v8/ScriptValueIteratorV8Wrapper.cpp @@ -15,15 +15,19 @@ V8ScriptValueIterator::V8ScriptValueIterator(ScriptEngineV8* engine, v8::Local object) : _engine(engine) { auto isolate = _engine->getIsolate(); + v8::Locker locker(isolate); + v8::Isolate::Scope isolateScope(isolate); v8::HandleScope handleScope(isolate); + // V8TODO Is this necessary? _context.Reset(isolate, _engine->getContext()); auto context = _context.Get(isolate); + v8::Context::Scope contextScope(context); Q_ASSERT(object->IsObject()); v8::Local v8Object = v8::Local::Cast(object); _object.Reset(isolate, v8Object); _propertyNames.Reset(isolate, v8Object->GetOwnPropertyNames(context).ToLocalChecked()); _length = _propertyNames.Get(isolate)->Length(); - _currentIndex = 0; + _currentIndex = -1; } bool V8ScriptValueIterator::hasNext() const { @@ -31,29 +35,37 @@ bool V8ScriptValueIterator::hasNext() const { } QString V8ScriptValueIterator::name() const { + Q_ASSERT(_currentIndex >= 0); auto isolate = _engine->getIsolate(); + v8::Locker locker(isolate); + v8::Isolate::Scope isolateScope(isolate); v8::HandleScope handleScope(isolate); auto context = _context.Get(isolate); + v8::Context::Scope contextScope(context); v8::Local propertyName; - if (!_propertyNames.Get(isolate)->Get(context, _length).ToLocal(&propertyName)) { + if (!_propertyNames.Get(isolate)->Get(context, _currentIndex).ToLocal(&propertyName)) { Q_ASSERT(false); } return QString(*v8::String::Utf8Value(isolate, propertyName)); } void V8ScriptValueIterator::next() { - if (_length < _currentIndex - 1) { - _length++; + if (_currentIndex < _length - 1) { + _currentIndex++; } } V8ScriptValue V8ScriptValueIterator::value() { + Q_ASSERT(_currentIndex >= 0); auto isolate = _engine->getIsolate(); + v8::Locker locker(isolate); + v8::Isolate::Scope isolateScope(isolate); v8::HandleScope handleScope(isolate); auto context = _context.Get(isolate); + v8::Context::Scope contextScope(context); v8::Local v8Value; v8::Local propertyName; - if (!_propertyNames.Get(isolate)->Get(context, _length).ToLocal(&propertyName)) { + if (!_propertyNames.Get(isolate)->Get(context, _currentIndex).ToLocal(&propertyName)) { Q_ASSERT(false); } if (!_object.Get(isolate)->Get(context, propertyName->ToString(context).ToLocalChecked()).ToLocal(&v8Value)) { diff --git a/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp b/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp index 6ca8409aa2..b2bfdfb5cf 100644 --- a/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp +++ b/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp @@ -202,7 +202,12 @@ ScriptEnginePointer ScriptValueV8Wrapper::engine() const { } ScriptValueIteratorPointer ScriptValueV8Wrapper::newIterator() const { - return std::make_shared(_engine, _value); + v8::Locker locker(_engine->getIsolate()); + v8::Isolate::Scope isolateScope(_engine->getIsolate()); + v8::HandleScope handleScope(_engine->getIsolate()); + v8::Context::Scope contextScope(_engine->getContext()); + ScriptValueIteratorPointer iterator = std::make_shared(_engine, _value); + return iterator; } bool ScriptValueV8Wrapper::hasProperty(const QString& name) const {