From bd045541505d2a4d10581090ddbbb319e330566c Mon Sep 17 00:00:00 2001 From: Heather Anderson Date: Sun, 14 Nov 2021 20:45:20 -0800 Subject: [PATCH] changes from code review suggestions --- interface/src/ui/JSConsole.cpp | 2 +- interface/src/ui/TestingDialog.cpp | 8 +++---- interface/src/ui/TestingDialog.h | 2 +- .../entities/src/EntityScriptingInterface.cpp | 18 +++++++-------- .../entities/src/EntityScriptingInterface.h | 4 ++-- libraries/script-engine/src/ScriptEngine.h | 2 +- libraries/script-engine/src/Scriptable.cpp | 6 ++--- libraries/script-engine/src/Scriptable.h | 12 ++++------ .../src/qtscript/ScriptContextQtAgent.h | 2 +- .../src/qtscript/ScriptEngineQtScript.cpp | 22 +++++++++---------- .../src/qtscript/ScriptEngineQtScript.h | 2 +- 11 files changed, 38 insertions(+), 42 deletions(-) diff --git a/interface/src/ui/JSConsole.cpp b/interface/src/ui/JSConsole.cpp index 77b8af22e2..89b20918d7 100644 --- a/interface/src/ui/JSConsole.cpp +++ b/interface/src/ui/JSConsole.cpp @@ -318,7 +318,7 @@ void JSConsole::setScriptManager(const ScriptManagerPointer& scriptManager) { if (_scriptManager == scriptManager && scriptManager != nullptr) { return; } - if (scriptManager != nullptr) { + if (_scriptManager != nullptr) { disconnect(_scriptManager.get(), nullptr, this, nullptr); _scriptManager.reset(); } diff --git a/interface/src/ui/TestingDialog.cpp b/interface/src/ui/TestingDialog.cpp index 49653c032f..efaf49fc1a 100644 --- a/interface/src/ui/TestingDialog.cpp +++ b/interface/src/ui/TestingDialog.cpp @@ -24,12 +24,12 @@ TestingDialog::TestingDialog(QWidget* parent) : _console->setFixedHeight(TESTING_CONSOLE_HEIGHT); - _manager = DependencyManager::get()->loadScript(qApp->applicationDirPath() + testRunnerRelativePath); - _console->setScriptManager(_manager); - connect(_manager.get(), &ScriptManager::finished, this, &TestingDialog::onTestingFinished); + _scriptManager = DependencyManager::get()->loadScript(qApp->applicationDirPath() + testRunnerRelativePath); + _console->setScriptManager(_scriptManager); + connect(_scriptManager.get(), &ScriptManager::finished, this, &TestingDialog::onTestingFinished); } void TestingDialog::onTestingFinished(const QString& scriptPath) { - _manager.reset(); + _scriptManager.reset(); _console->setScriptManager(); } diff --git a/interface/src/ui/TestingDialog.h b/interface/src/ui/TestingDialog.h index 2197737325..7707213b03 100644 --- a/interface/src/ui/TestingDialog.h +++ b/interface/src/ui/TestingDialog.h @@ -33,7 +33,7 @@ public: private: std::unique_ptr _console; - ScriptManagerPointer _manager; + ScriptManagerPointer _scriptManager; }; #endif diff --git a/libraries/entities/src/EntityScriptingInterface.cpp b/libraries/entities/src/EntityScriptingInterface.cpp index 8ed610dc69..6efb4f4671 100644 --- a/libraries/entities/src/EntityScriptingInterface.cpp +++ b/libraries/entities/src/EntityScriptingInterface.cpp @@ -53,7 +53,7 @@ void staticEntityScriptInitializer(ScriptManager* manager) { auto entityScriptingInterface = DependencyManager::get(); entityScriptingInterface->init(); - auto ifacePtr = entityScriptingInterface.data(); // using this when we don't want to leak a reference + auto interfacePtr = entityScriptingInterface.data(); // using this when we don't want to leak a reference registerMetaTypes(scriptEngine); @@ -71,7 +71,7 @@ void staticEntityScriptInitializer(ScriptManager* manager) { // so... yay lambdas everywhere to get the sender manager->connect( manager, &ScriptManager::attachDefaultEventHandlers, entityScriptingInterface.data(), - [ifacePtr, manager] { ifacePtr->attachDefaultEventHandlers(manager); }, + [interfacePtr, manager] { interfacePtr->attachDefaultEventHandlers(manager); }, Qt::DirectConnection); manager->connect(manager, &ScriptManager::releaseEntityPacketSenderMessages, entityScriptingInterface.data(), &EntityScriptingInterface::releaseEntityPacketSenderMessages, Qt::DirectConnection); @@ -99,7 +99,7 @@ EntityScriptingInterface::EntityScriptingInterface(bool bidOnSimulationOwnership void EntityScriptingInterface::releaseEntityPacketSenderMessages(bool wait) { EntityEditPacketSender* entityPacketSender = getEntityPacketSender(); - if (entityPacketSender->serversExist()) { + if (entityPacketSender && entityPacketSender->serversExist()) { // release the queue of edit entity messages. entityPacketSender->releaseQueuedMessages(); @@ -1513,17 +1513,17 @@ bool EntityPropertyMetadataRequest::script(EntityItemID entityID, const ScriptVa using LocalScriptStatusRequest = QFutureWatcher; LocalScriptStatusRequest* request = new LocalScriptStatusRequest; - QObject::connect(request, &LocalScriptStatusRequest::finished, _manager, [=]() mutable { + QObject::connect(request, &LocalScriptStatusRequest::finished, _scriptManager, [=]() mutable { auto details = request->result().toMap(); ScriptValue err, result; if (details.contains("isError")) { if (!details.contains("message")) { details["message"] = details["errorInfo"]; } - err = _manager->engine()->makeError(_manager->engine()->toScriptValue(details)); + err = _scriptManager->engine()->makeError(_scriptManager->engine()->toScriptValue(details)); } else { details["success"] = true; - result = _manager->engine()->toScriptValue(details); + result = _scriptManager->engine()->toScriptValue(details); } callScopedHandlerObject(handler, err, result); request->deleteLater(); @@ -1546,9 +1546,9 @@ bool EntityPropertyMetadataRequest::script(EntityItemID entityID, const ScriptVa bool EntityPropertyMetadataRequest::serverScripts(EntityItemID entityID, const ScriptValue& handler) { auto client = DependencyManager::get(); auto request = client->createScriptStatusRequest(entityID); - QPointer manager = _manager; - QObject::connect(request, &GetScriptStatusRequest::finished, _manager, [=](GetScriptStatusRequest* request) mutable { - auto manager = _manager; + QPointer manager = _scriptManager; + QObject::connect(request, &GetScriptStatusRequest::finished, _scriptManager, [=](GetScriptStatusRequest* request) mutable { + auto manager = _scriptManager; if (!manager) { qCDebug(entities) << __FUNCTION__ << " -- engine destroyed while inflight" << entityID; return; diff --git a/libraries/entities/src/EntityScriptingInterface.h b/libraries/entities/src/EntityScriptingInterface.h index bbcaa6f8c7..730feba981 100644 --- a/libraries/entities/src/EntityScriptingInterface.h +++ b/libraries/entities/src/EntityScriptingInterface.h @@ -56,11 +56,11 @@ extern const QString NOT_GRABBABLE_USER_DATA; // problems with their own Entity scripts. class EntityPropertyMetadataRequest { public: - EntityPropertyMetadataRequest(ScriptManager* manager) : _manager(manager == nullptr ? nullptr : manager){}; + EntityPropertyMetadataRequest(ScriptManager* manager) : _scriptManager(manager){}; bool script(EntityItemID entityID, const ScriptValue& handler); bool serverScripts(EntityItemID entityID, const ScriptValue& handler); private: - QPointer _manager; + QPointer _scriptManager; }; /*@jsdoc diff --git a/libraries/script-engine/src/ScriptEngine.h b/libraries/script-engine/src/ScriptEngine.h index c69a0af8c3..d611e37559 100644 --- a/libraries/script-engine/src/ScriptEngine.h +++ b/libraries/script-engine/src/ScriptEngine.h @@ -61,7 +61,7 @@ public: //ExcludeChildObjects = 0x0001, // The script object will not expose child objects as properties. ExcludeSuperClassMethods = 0x0002, // The script object will not expose signals and slots inherited from the superclass. ExcludeSuperClassProperties = 0x0004, // The script object will not expose properties inherited from the superclass. - ExcludeSuperClassContents = 0x0006, // Shorthand form for ExcludeSuperClassMethods | ExcludeSuperClassProperties + ExcludeSuperClassContents = ExcludeSuperClassMethods | ExcludeSuperClassProperties, //ExcludeDeleteLater = 0x0010, // The script object will not expose the QObject::deleteLater() slot. ExcludeSlots = 0x0020, // The script object will not expose the QObject's slots. AutoCreateDynamicProperties = 0x0100, // Properties that don't already exist in the QObject will be created as dynamic properties of that object, rather than as properties of the script object. diff --git a/libraries/script-engine/src/Scriptable.cpp b/libraries/script-engine/src/Scriptable.cpp index 1ba432b061..07860ce6df 100644 --- a/libraries/script-engine/src/Scriptable.cpp +++ b/libraries/script-engine/src/Scriptable.cpp @@ -11,12 +11,12 @@ #include "Scriptable.h" -static thread_local ScriptContext* ScriptContextStore; +static thread_local ScriptContext* scriptContextStore; ScriptContext* Scriptable::context() { - return ScriptContextStore; + return scriptContextStore; } void Scriptable::setContext(ScriptContext* context) { - ScriptContextStore = context; + scriptContextStore = context; } diff --git a/libraries/script-engine/src/Scriptable.h b/libraries/script-engine/src/Scriptable.h index 45669a111c..d2ca04ac69 100644 --- a/libraries/script-engine/src/Scriptable.h +++ b/libraries/script-engine/src/Scriptable.h @@ -37,26 +37,22 @@ public: ScriptEnginePointer Scriptable::engine() { ScriptContext* scriptContext = context(); - if (!scriptContext) return nullptr; - return scriptContext->engine(); + return scriptContext ? scriptContext->engine() : nullptr; } ScriptValue Scriptable::thisObject() { ScriptContext* scriptContext = context(); - if (!scriptContext) return ScriptValue(); - return scriptContext->thisObject(); + return scriptContext ? scriptContext->thisObject() : ScriptValue(); } int Scriptable::argumentCount() { ScriptContext* scriptContext = context(); - if (!scriptContext) return 0; - return scriptContext->argumentCount(); + return scriptContext ? scriptContext->argumentCount() : 0; } ScriptValue Scriptable::argument(int index) { ScriptContext* scriptContext = context(); - if (!scriptContext) return ScriptValue(); - return scriptContext->argument(index); + return scriptContext ? scriptContext->argument(index) : ScriptValue(); } #endif // hifi_Scriptable_h diff --git a/libraries/script-engine/src/qtscript/ScriptContextQtAgent.h b/libraries/script-engine/src/qtscript/ScriptContextQtAgent.h index f23d82dc2b..0869e5a50f 100644 --- a/libraries/script-engine/src/qtscript/ScriptContextQtAgent.h +++ b/libraries/script-engine/src/qtscript/ScriptContextQtAgent.h @@ -38,7 +38,7 @@ public: // QScriptEngineAgent implementation virtual void functionExit(qint64 scriptId, const QScriptValue& returnValue) override; private: // storage - bool _contextActive = false; + bool _contextActive{ false }; QList _contextStack; ScriptContextQtPointer _currContext; ScriptEngineQtScript* _engine; diff --git a/libraries/script-engine/src/qtscript/ScriptEngineQtScript.cpp b/libraries/script-engine/src/qtscript/ScriptEngineQtScript.cpp index ae76b2f627..c2c25f9bb8 100644 --- a/libraries/script-engine/src/qtscript/ScriptEngineQtScript.cpp +++ b/libraries/script-engine/src/qtscript/ScriptEngineQtScript.cpp @@ -229,11 +229,11 @@ bool ScriptEngineQtScript::raiseException(const QScriptValue& exception) { // we have an active context / JS stack frame so throw the exception per usual QScriptEngine::currentContext()->throwValue(makeError(exception)); return true; - } else if(_manager) { + } else if (_scriptManager) { // we are within a pure C++ stack frame (ie: being called directly by other C++ code) // in this case no context information is available so just emit the exception for reporting QScriptValue thrown = makeError(exception); - emit _manager->unhandledException(ScriptValue(new ScriptValueQtWrapper(this, std::move(thrown)))); + emit _scriptManager->unhandledException(ScriptValue(new ScriptValueQtWrapper(this, std::move(thrown)))); } return false; } @@ -242,8 +242,8 @@ bool ScriptEngineQtScript::maybeEmitUncaughtException(const QString& debugHint) if (!IS_THREADSAFE_INVOCATION(thread(), __FUNCTION__)) { return false; } - if (!isEvaluating() && hasUncaughtException() && _manager) { - emit _manager->unhandledException(cloneUncaughtException(debugHint)); + if (!isEvaluating() && hasUncaughtException() && _scriptManager) { + emit _scriptManager->unhandledException(cloneUncaughtException(debugHint)); clearExceptions(); return true; } @@ -322,21 +322,21 @@ static void ScriptValueFromQScriptValue(const QScriptValue& src, ScriptValue& de ScriptEngineQtScript::ScriptEngineQtScript(ScriptManager* scriptManager) : QScriptEngine(), - _manager(scriptManager), + _scriptManager(scriptManager), _arrayBufferClass(new ArrayBufferClass(this)) { qScriptRegisterMetaType(this, ScriptValueToQScriptValue, ScriptValueFromQScriptValue); - if (_manager) { + if (_scriptManager) { connect(this, &QScriptEngine::signalHandlerException, this, [this](const QScriptValue& exception) { if (hasUncaughtException()) { // the engine's uncaughtException() seems to produce much better stack traces here - emit _manager->unhandledException(cloneUncaughtException("signalHandlerException")); + emit _scriptManager->unhandledException(cloneUncaughtException("signalHandlerException")); clearExceptions(); } else { // ... but may not always be available -- so if needed we fallback to the passed exception QScriptValue thrown = makeError(exception); - emit _manager->unhandledException(ScriptValue(new ScriptValueQtWrapper(this, std::move(thrown)))); + emit _scriptManager->unhandledException(ScriptValue(new ScriptValueQtWrapper(this, std::move(thrown)))); } }, Qt::DirectConnection); moveToThread(scriptManager->thread()); @@ -649,7 +649,7 @@ ScriptValue ScriptEngineQtScript::evaluateInClosure(const ScriptValue& _closure, } ScriptValue ScriptEngineQtScript::evaluate(const QString& sourceCode, const QString& fileName) { - if (_manager && _manager->isStopped()) { + if (_scriptManager && _scriptManager->isStopped()) { return undefinedValue(); // bail early } @@ -692,7 +692,7 @@ ScriptValue ScriptEngineQtScript::evaluate(const QString& sourceCode, const QStr } Q_INVOKABLE ScriptValue ScriptEngineQtScript::evaluate(const ScriptProgramPointer& program) { - if (_manager && _manager->isStopped()) { + if (_scriptManager && _scriptManager->isStopped()) { return undefinedValue(); // bail early } @@ -751,7 +751,7 @@ ScriptValue ScriptEngineQtScript::globalObject() const { } ScriptManager* ScriptEngineQtScript::manager() const { - return _manager; + return _scriptManager; } ScriptValue ScriptEngineQtScript::newArray(uint length) { diff --git a/libraries/script-engine/src/qtscript/ScriptEngineQtScript.h b/libraries/script-engine/src/qtscript/ScriptEngineQtScript.h index 3d99f0a39c..9d1e54d1b3 100644 --- a/libraries/script-engine/src/qtscript/ScriptEngineQtScript.h +++ b/libraries/script-engine/src/qtscript/ScriptEngineQtScript.h @@ -163,7 +163,7 @@ protected: const QScriptEngine::ValueOwnership& ownership = QScriptEngine::AutoOwnership); protected: - QPointer _manager; + QPointer _scriptManager; int _nextCustomType = 0; ScriptValue _nullValue;