From 310654831c6f663e5745a6f66f7642ebeb3dac9e Mon Sep 17 00:00:00 2001 From: ZappoMan Date: Tue, 24 Feb 2015 17:34:19 -0800 Subject: [PATCH] clean up comments remove dead code --- interface/src/Application.cpp | 6 ++--- .../src/EntityTreeRenderer.cpp | 14 ++++++------ libraries/script-engine/src/ScriptEngine.cpp | 22 +++++++++++++++---- libraries/script-engine/src/ScriptEngine.h | 1 - 4 files changed, 28 insertions(+), 15 deletions(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 390b42eaa7..d3161ebf51 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -3541,6 +3541,9 @@ void Application::registerScriptEngineWithApplicationServices(ScriptEngine* scri scriptEngine->registerGlobalObject("MIDI", &MIDIManager::getInstance()); #endif + // TODO: Consider moving some of this functionality into the ScriptEngine class instead. It seems wrong that this + // work is being done in the Application class when really these dependencies are more related to the ScriptEngine's + // implementation QThread* workerThread = new QThread(this); QString scriptEngineName = QString("Script Thread:") + scriptEngine->getFilename(); workerThread->setObjectName(scriptEngineName); @@ -3552,9 +3555,6 @@ void Application::registerScriptEngineWithApplicationServices(ScriptEngine* scri connect(scriptEngine, SIGNAL(doneRunning()), scriptEngine, SLOT(deleteLater())); connect(workerThread, SIGNAL(finished()), workerThread, SLOT(deleteLater())); - // when the application is about to quit, stop our script engine so it unwinds properly - //connect(this, SIGNAL(aboutToQuit()), scriptEngine, SLOT(stop())); - auto nodeList = DependencyManager::get(); connect(nodeList.data(), &NodeList::nodeKilled, scriptEngine, &ScriptEngine::nodeKilled); diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.cpp b/libraries/entities-renderer/src/EntityTreeRenderer.cpp index 8648cea285..70e1054c39 100644 --- a/libraries/entities-renderer/src/EntityTreeRenderer.cpp +++ b/libraries/entities-renderer/src/EntityTreeRenderer.cpp @@ -59,14 +59,14 @@ EntityTreeRenderer::EntityTreeRenderer(bool wantScripts, AbstractViewStateInterf } EntityTreeRenderer::~EntityTreeRenderer() { - // NOTE: we don't need to delete _entitiesScriptEngine because it's owned by the application and gets cleaned up - // automatically but we do need to delete our sandbox script engine. + // NOTE: we don't need to delete _entitiesScriptEngine because it is registered with the application and has a + // signal tied to call it's deleteLater on doneRunning if (_sandboxScriptEngine) { - // NOTE: is it possible this is a problem? I think that we hook the script engine object up to a deleteLater() - // call inside of registerScriptEngineWithApplicationServices() but do we not call that for _sandboxScriptEngine??? - // this _sandboxScriptEngine implementation is confusing and potentially error prone because it's not a full fledged - // ScriptEngine that has been fully connected. We did this so that scripts that were ill-formed could be evaluated - // but not execute against the application. + // TODO: consider reworking how _sandboxScriptEngine is managed. It's treated differently than _entitiesScriptEngine + // because we don't call registerScriptEngineWithApplicationServices() for it. This implementation is confusing and + // potentially error prone because it's not a full fledged ScriptEngine that has been fully connected to the + // application. We did this so that scripts that were ill-formed could be evaluated but not execute against the + // application services. But this means it's shutdown behavior is different from other ScriptEngines delete _sandboxScriptEngine; _sandboxScriptEngine = NULL; } diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index d090fc0ac6..b41331450b 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -135,16 +135,31 @@ void ScriptEngine::stopAllScripts(QObject* application) { // complete. After that we can handle the stop process appropriately if (scriptEngine->evaluatePending()) { while (scriptEngine->evaluatePending()) { + + // This event loop allows any started, but not yet finished evaluate() calls to complete + // we need to let these complete so that we can be guaranteed that the script engine isn't + // in a partially setup state, which can confuse our shutdown unwinding. QEventLoop loop; QObject::connect(scriptEngine, &ScriptEngine::evaluationFinished, &loop, &QEventLoop::quit); loop.exec(); } } + // We disconnect any script engine signals from the application because we don't want to do any + // extra stopScript/loadScript processing that the Application normally does when scripts start + // and stop. We can safely short circuit this because we know we're in the "quitting" process scriptEngine->disconnect(application); + + // Calling stop on the script engine will set it's internal _isFinished state to true, and result + // in the ScriptEngine gracefully ending it's run() method. scriptEngine->stop(); + // We need to wait for the engine to be done running before we proceed, because we don't + // want any of the scripts final "scriptEnding()" or pending "update()" methods from accessing + // any application state after we leave this stopAllScripts() method scriptEngine->waitTillDoneRunning(); + + // If the script is stopped, we can remove it from our set i.remove(); } } @@ -156,10 +171,11 @@ void ScriptEngine::stopAllScripts(QObject* application) { void ScriptEngine::waitTillDoneRunning() { QString scriptName = getFilename(); + // If the script never started running or finished running before we got here, we don't need to wait for it if (_isRunning) { - _doneRunningThisScript = false; - _isWaitingForDoneRunning = true; + _doneRunningThisScript = false; // NOTE: this is static, we serialize our waiting for scripts to finish + // What can we do here??? // we will be calling this on the main Application thread, inside of stopAllScripts() // we want the application thread to continue to process events, because the script will @@ -178,8 +194,6 @@ void ScriptEngine::waitTillDoneRunning() { break; } } - - _isWaitingForDoneRunning = false; } } diff --git a/libraries/script-engine/src/ScriptEngine.h b/libraries/script-engine/src/ScriptEngine.h index 51cacd5e58..154fdb88e3 100644 --- a/libraries/script-engine/src/ScriptEngine.h +++ b/libraries/script-engine/src/ScriptEngine.h @@ -136,7 +136,6 @@ protected: bool _isFinished; bool _isRunning; int _evaluatesPending = 0; - bool _isWaitingForDoneRunning = false; bool _isInitialized; bool _isAvatar; QTimer* _avatarIdentityTimer;