changes from code review suggestions

This commit is contained in:
Heather Anderson 2021-11-14 20:45:20 -08:00 committed by ksuprynowicz
parent 70fbe76341
commit bd04554150
11 changed files with 38 additions and 42 deletions

View file

@ -318,7 +318,7 @@ void JSConsole::setScriptManager(const ScriptManagerPointer& scriptManager) {
if (_scriptManager == scriptManager && scriptManager != nullptr) { if (_scriptManager == scriptManager && scriptManager != nullptr) {
return; return;
} }
if (scriptManager != nullptr) { if (_scriptManager != nullptr) {
disconnect(_scriptManager.get(), nullptr, this, nullptr); disconnect(_scriptManager.get(), nullptr, this, nullptr);
_scriptManager.reset(); _scriptManager.reset();
} }

View file

@ -24,12 +24,12 @@ TestingDialog::TestingDialog(QWidget* parent) :
_console->setFixedHeight(TESTING_CONSOLE_HEIGHT); _console->setFixedHeight(TESTING_CONSOLE_HEIGHT);
_manager = DependencyManager::get<ScriptEngines>()->loadScript(qApp->applicationDirPath() + testRunnerRelativePath); _scriptManager = DependencyManager::get<ScriptEngines>()->loadScript(qApp->applicationDirPath() + testRunnerRelativePath);
_console->setScriptManager(_manager); _console->setScriptManager(_scriptManager);
connect(_manager.get(), &ScriptManager::finished, this, &TestingDialog::onTestingFinished); connect(_scriptManager.get(), &ScriptManager::finished, this, &TestingDialog::onTestingFinished);
} }
void TestingDialog::onTestingFinished(const QString& scriptPath) { void TestingDialog::onTestingFinished(const QString& scriptPath) {
_manager.reset(); _scriptManager.reset();
_console->setScriptManager(); _console->setScriptManager();
} }

View file

@ -33,7 +33,7 @@ public:
private: private:
std::unique_ptr<JSConsole> _console; std::unique_ptr<JSConsole> _console;
ScriptManagerPointer _manager; ScriptManagerPointer _scriptManager;
}; };
#endif #endif

View file

@ -53,7 +53,7 @@ void staticEntityScriptInitializer(ScriptManager* manager) {
auto entityScriptingInterface = DependencyManager::get<EntityScriptingInterface>(); auto entityScriptingInterface = DependencyManager::get<EntityScriptingInterface>();
entityScriptingInterface->init(); 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); registerMetaTypes(scriptEngine);
@ -71,7 +71,7 @@ void staticEntityScriptInitializer(ScriptManager* manager) {
// so... yay lambdas everywhere to get the sender // so... yay lambdas everywhere to get the sender
manager->connect( manager->connect(
manager, &ScriptManager::attachDefaultEventHandlers, entityScriptingInterface.data(), manager, &ScriptManager::attachDefaultEventHandlers, entityScriptingInterface.data(),
[ifacePtr, manager] { ifacePtr->attachDefaultEventHandlers(manager); }, [interfacePtr, manager] { interfacePtr->attachDefaultEventHandlers(manager); },
Qt::DirectConnection); Qt::DirectConnection);
manager->connect(manager, &ScriptManager::releaseEntityPacketSenderMessages, entityScriptingInterface.data(), manager->connect(manager, &ScriptManager::releaseEntityPacketSenderMessages, entityScriptingInterface.data(),
&EntityScriptingInterface::releaseEntityPacketSenderMessages, Qt::DirectConnection); &EntityScriptingInterface::releaseEntityPacketSenderMessages, Qt::DirectConnection);
@ -99,7 +99,7 @@ EntityScriptingInterface::EntityScriptingInterface(bool bidOnSimulationOwnership
void EntityScriptingInterface::releaseEntityPacketSenderMessages(bool wait) { void EntityScriptingInterface::releaseEntityPacketSenderMessages(bool wait) {
EntityEditPacketSender* entityPacketSender = getEntityPacketSender(); EntityEditPacketSender* entityPacketSender = getEntityPacketSender();
if (entityPacketSender->serversExist()) { if (entityPacketSender && entityPacketSender->serversExist()) {
// release the queue of edit entity messages. // release the queue of edit entity messages.
entityPacketSender->releaseQueuedMessages(); entityPacketSender->releaseQueuedMessages();
@ -1513,17 +1513,17 @@ bool EntityPropertyMetadataRequest::script(EntityItemID entityID, const ScriptVa
using LocalScriptStatusRequest = QFutureWatcher<QVariant>; using LocalScriptStatusRequest = QFutureWatcher<QVariant>;
LocalScriptStatusRequest* request = new LocalScriptStatusRequest; LocalScriptStatusRequest* request = new LocalScriptStatusRequest;
QObject::connect(request, &LocalScriptStatusRequest::finished, _manager, [=]() mutable { QObject::connect(request, &LocalScriptStatusRequest::finished, _scriptManager, [=]() mutable {
auto details = request->result().toMap(); auto details = request->result().toMap();
ScriptValue err, result; ScriptValue err, result;
if (details.contains("isError")) { if (details.contains("isError")) {
if (!details.contains("message")) { if (!details.contains("message")) {
details["message"] = details["errorInfo"]; details["message"] = details["errorInfo"];
} }
err = _manager->engine()->makeError(_manager->engine()->toScriptValue(details)); err = _scriptManager->engine()->makeError(_scriptManager->engine()->toScriptValue(details));
} else { } else {
details["success"] = true; details["success"] = true;
result = _manager->engine()->toScriptValue(details); result = _scriptManager->engine()->toScriptValue(details);
} }
callScopedHandlerObject(handler, err, result); callScopedHandlerObject(handler, err, result);
request->deleteLater(); request->deleteLater();
@ -1546,9 +1546,9 @@ bool EntityPropertyMetadataRequest::script(EntityItemID entityID, const ScriptVa
bool EntityPropertyMetadataRequest::serverScripts(EntityItemID entityID, const ScriptValue& handler) { bool EntityPropertyMetadataRequest::serverScripts(EntityItemID entityID, const ScriptValue& handler) {
auto client = DependencyManager::get<EntityScriptClient>(); auto client = DependencyManager::get<EntityScriptClient>();
auto request = client->createScriptStatusRequest(entityID); auto request = client->createScriptStatusRequest(entityID);
QPointer<ScriptManager> manager = _manager; QPointer<ScriptManager> manager = _scriptManager;
QObject::connect(request, &GetScriptStatusRequest::finished, _manager, [=](GetScriptStatusRequest* request) mutable { QObject::connect(request, &GetScriptStatusRequest::finished, _scriptManager, [=](GetScriptStatusRequest* request) mutable {
auto manager = _manager; auto manager = _scriptManager;
if (!manager) { if (!manager) {
qCDebug(entities) << __FUNCTION__ << " -- engine destroyed while inflight" << entityID; qCDebug(entities) << __FUNCTION__ << " -- engine destroyed while inflight" << entityID;
return; return;

View file

@ -56,11 +56,11 @@ extern const QString NOT_GRABBABLE_USER_DATA;
// problems with their own Entity scripts. // problems with their own Entity scripts.
class EntityPropertyMetadataRequest { class EntityPropertyMetadataRequest {
public: public:
EntityPropertyMetadataRequest(ScriptManager* manager) : _manager(manager == nullptr ? nullptr : manager){}; EntityPropertyMetadataRequest(ScriptManager* manager) : _scriptManager(manager){};
bool script(EntityItemID entityID, const ScriptValue& handler); bool script(EntityItemID entityID, const ScriptValue& handler);
bool serverScripts(EntityItemID entityID, const ScriptValue& handler); bool serverScripts(EntityItemID entityID, const ScriptValue& handler);
private: private:
QPointer<ScriptManager> _manager; QPointer<ScriptManager> _scriptManager;
}; };
/*@jsdoc /*@jsdoc

View file

@ -61,7 +61,7 @@ public:
//ExcludeChildObjects = 0x0001, // The script object will not expose child objects as properties. //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. 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. 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. //ExcludeDeleteLater = 0x0010, // The script object will not expose the QObject::deleteLater() slot.
ExcludeSlots = 0x0020, // The script object will not expose the QObject's slots. 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. 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.

View file

@ -11,12 +11,12 @@
#include "Scriptable.h" #include "Scriptable.h"
static thread_local ScriptContext* ScriptContextStore; static thread_local ScriptContext* scriptContextStore;
ScriptContext* Scriptable::context() { ScriptContext* Scriptable::context() {
return ScriptContextStore; return scriptContextStore;
} }
void Scriptable::setContext(ScriptContext* context) { void Scriptable::setContext(ScriptContext* context) {
ScriptContextStore = context; scriptContextStore = context;
} }

View file

@ -37,26 +37,22 @@ public:
ScriptEnginePointer Scriptable::engine() { ScriptEnginePointer Scriptable::engine() {
ScriptContext* scriptContext = context(); ScriptContext* scriptContext = context();
if (!scriptContext) return nullptr; return scriptContext ? scriptContext->engine() : nullptr;
return scriptContext->engine();
} }
ScriptValue Scriptable::thisObject() { ScriptValue Scriptable::thisObject() {
ScriptContext* scriptContext = context(); ScriptContext* scriptContext = context();
if (!scriptContext) return ScriptValue(); return scriptContext ? scriptContext->thisObject() : ScriptValue();
return scriptContext->thisObject();
} }
int Scriptable::argumentCount() { int Scriptable::argumentCount() {
ScriptContext* scriptContext = context(); ScriptContext* scriptContext = context();
if (!scriptContext) return 0; return scriptContext ? scriptContext->argumentCount() : 0;
return scriptContext->argumentCount();
} }
ScriptValue Scriptable::argument(int index) { ScriptValue Scriptable::argument(int index) {
ScriptContext* scriptContext = context(); ScriptContext* scriptContext = context();
if (!scriptContext) return ScriptValue(); return scriptContext ? scriptContext->argument(index) : ScriptValue();
return scriptContext->argument(index);
} }
#endif // hifi_Scriptable_h #endif // hifi_Scriptable_h

View file

@ -38,7 +38,7 @@ public: // QScriptEngineAgent implementation
virtual void functionExit(qint64 scriptId, const QScriptValue& returnValue) override; virtual void functionExit(qint64 scriptId, const QScriptValue& returnValue) override;
private: // storage private: // storage
bool _contextActive = false; bool _contextActive{ false };
QList<ScriptContextQtPointer> _contextStack; QList<ScriptContextQtPointer> _contextStack;
ScriptContextQtPointer _currContext; ScriptContextQtPointer _currContext;
ScriptEngineQtScript* _engine; ScriptEngineQtScript* _engine;

View file

@ -229,11 +229,11 @@ bool ScriptEngineQtScript::raiseException(const QScriptValue& exception) {
// we have an active context / JS stack frame so throw the exception per usual // we have an active context / JS stack frame so throw the exception per usual
QScriptEngine::currentContext()->throwValue(makeError(exception)); QScriptEngine::currentContext()->throwValue(makeError(exception));
return true; return true;
} else if(_manager) { } else if (_scriptManager) {
// we are within a pure C++ stack frame (ie: being called directly by other C++ code) // 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 // in this case no context information is available so just emit the exception for reporting
QScriptValue thrown = makeError(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))));
} }
return false; return false;
} }
@ -242,8 +242,8 @@ bool ScriptEngineQtScript::maybeEmitUncaughtException(const QString& debugHint)
if (!IS_THREADSAFE_INVOCATION(thread(), __FUNCTION__)) { if (!IS_THREADSAFE_INVOCATION(thread(), __FUNCTION__)) {
return false; return false;
} }
if (!isEvaluating() && hasUncaughtException() && _manager) { if (!isEvaluating() && hasUncaughtException() && _scriptManager) {
emit _manager->unhandledException(cloneUncaughtException(debugHint)); emit _scriptManager->unhandledException(cloneUncaughtException(debugHint));
clearExceptions(); clearExceptions();
return true; return true;
} }
@ -322,21 +322,21 @@ static void ScriptValueFromQScriptValue(const QScriptValue& src, ScriptValue& de
ScriptEngineQtScript::ScriptEngineQtScript(ScriptManager* scriptManager) : ScriptEngineQtScript::ScriptEngineQtScript(ScriptManager* scriptManager) :
QScriptEngine(), QScriptEngine(),
_manager(scriptManager), _scriptManager(scriptManager),
_arrayBufferClass(new ArrayBufferClass(this)) _arrayBufferClass(new ArrayBufferClass(this))
{ {
qScriptRegisterMetaType(this, ScriptValueToQScriptValue, ScriptValueFromQScriptValue); qScriptRegisterMetaType(this, ScriptValueToQScriptValue, ScriptValueFromQScriptValue);
if (_manager) { if (_scriptManager) {
connect(this, &QScriptEngine::signalHandlerException, this, [this](const QScriptValue& exception) { connect(this, &QScriptEngine::signalHandlerException, this, [this](const QScriptValue& exception) {
if (hasUncaughtException()) { if (hasUncaughtException()) {
// the engine's uncaughtException() seems to produce much better stack traces here // the engine's uncaughtException() seems to produce much better stack traces here
emit _manager->unhandledException(cloneUncaughtException("signalHandlerException")); emit _scriptManager->unhandledException(cloneUncaughtException("signalHandlerException"));
clearExceptions(); clearExceptions();
} else { } else {
// ... but may not always be available -- so if needed we fallback to the passed exception // ... but may not always be available -- so if needed we fallback to the passed exception
QScriptValue thrown = makeError(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); }, Qt::DirectConnection);
moveToThread(scriptManager->thread()); moveToThread(scriptManager->thread());
@ -649,7 +649,7 @@ ScriptValue ScriptEngineQtScript::evaluateInClosure(const ScriptValue& _closure,
} }
ScriptValue ScriptEngineQtScript::evaluate(const QString& sourceCode, const QString& fileName) { ScriptValue ScriptEngineQtScript::evaluate(const QString& sourceCode, const QString& fileName) {
if (_manager && _manager->isStopped()) { if (_scriptManager && _scriptManager->isStopped()) {
return undefinedValue(); // bail early return undefinedValue(); // bail early
} }
@ -692,7 +692,7 @@ ScriptValue ScriptEngineQtScript::evaluate(const QString& sourceCode, const QStr
} }
Q_INVOKABLE ScriptValue ScriptEngineQtScript::evaluate(const ScriptProgramPointer& program) { Q_INVOKABLE ScriptValue ScriptEngineQtScript::evaluate(const ScriptProgramPointer& program) {
if (_manager && _manager->isStopped()) { if (_scriptManager && _scriptManager->isStopped()) {
return undefinedValue(); // bail early return undefinedValue(); // bail early
} }
@ -751,7 +751,7 @@ ScriptValue ScriptEngineQtScript::globalObject() const {
} }
ScriptManager* ScriptEngineQtScript::manager() const { ScriptManager* ScriptEngineQtScript::manager() const {
return _manager; return _scriptManager;
} }
ScriptValue ScriptEngineQtScript::newArray(uint length) { ScriptValue ScriptEngineQtScript::newArray(uint length) {

View file

@ -163,7 +163,7 @@ protected:
const QScriptEngine::ValueOwnership& ownership = QScriptEngine::AutoOwnership); const QScriptEngine::ValueOwnership& ownership = QScriptEngine::AutoOwnership);
protected: protected:
QPointer<ScriptManager> _manager; QPointer<ScriptManager> _scriptManager;
int _nextCustomType = 0; int _nextCustomType = 0;
ScriptValue _nullValue; ScriptValue _nullValue;