From 0353940d9ebe721cb17e51f1ea1fb7d494bde917 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Tue, 2 May 2017 17:42:23 -0400 Subject: [PATCH] decouple initialization from storage in Table.getTablet() --- .../src/TabletScriptingInterface.cpp | 71 +++++++++++-------- .../src/TabletScriptingInterface.h | 8 +-- 2 files changed, 47 insertions(+), 32 deletions(-) diff --git a/libraries/script-engine/src/TabletScriptingInterface.cpp b/libraries/script-engine/src/TabletScriptingInterface.cpp index d4eeecc82e..aa9840b226 100644 --- a/libraries/script-engine/src/TabletScriptingInterface.cpp +++ b/libraries/script-engine/src/TabletScriptingInterface.cpp @@ -21,6 +21,8 @@ #include #include "SoundEffect.h" +const QString SYSTEM_TOOLBAR = "com.highfidelity.interface.toolbar.system"; +const QString SYSTEM_TABLET = "com.highfidelity.interface.tablet.system"; QScriptValue tabletToScriptValue(QScriptEngine* engine, TabletProxy* const &in) { return engine->newQObject(in, QScriptEngine::QtOwnership, QScriptEngine::ExcludeDeleteLater | QScriptEngine::ExcludeChildObjects); @@ -35,11 +37,11 @@ TabletScriptingInterface::TabletScriptingInterface() { } QObject* TabletScriptingInterface::getSystemToolbarProxy() { - const QString SYSTEM_TOOLBAR = "com.highfidelity.interface.toolbar.system"; Qt::ConnectionType connectionType = Qt::AutoConnection; if (QThread::currentThread() != _toolbarScriptingInterface->thread()) { connectionType = Qt::BlockingQueuedConnection; } + QObject* toolbarProxy = nullptr; bool hasResult = QMetaObject::invokeMethod(_toolbarScriptingInterface, "getToolbar", connectionType, Q_RETURN_ARG(QObject*, toolbarProxy), Q_ARG(QString, SYSTEM_TOOLBAR)); if (hasResult) { @@ -51,28 +53,38 @@ QObject* TabletScriptingInterface::getSystemToolbarProxy() { } TabletProxy* TabletScriptingInterface::getTablet(const QString& tabletId) { + TabletProxy* tabletProxy = nullptr; + { + // the only thing guarded should be map mutation + // this avoids a deadlock with the Main thread + // from Qt::BlockingQueuedEvent invocations later in the call-tree + std::lock_guard guard(_mapMutex); - std::lock_guard guard(_mutex); - - // look up tabletId in the map. - auto iter = _tabletProxies.find(tabletId); - if (iter != _tabletProxies.end()) { - // tablet already exists, just return it. - return iter->second; - } else { - // allocate a new tablet, add it to the map then return it. - auto tabletProxy = new TabletProxy(tabletId); - tabletProxy->setParent(this); - _tabletProxies[tabletId] = tabletProxy; - tabletProxy->setToolbarMode(_toolbarMode); - return tabletProxy; + auto iter = _tabletProxies.find(tabletId); + if (iter != _tabletProxies.end()) { + // tablet already exists + return iter->second; + } else { + // tablet must be created + tabletProxy = new TabletProxy(this, tabletId); + _tabletProxies[tabletId] = tabletProxy; + } } + + assert(tabletProxy); + // initialize new tablet + tabletProxy->setToolbarMode(_toolbarMode); + return tabletProxy; } void TabletScriptingInterface::setToolbarMode(bool toolbarMode) { - std::lock_guard guard(_mutex); - - _toolbarMode = toolbarMode; + { + // the only thing guarded should be _toolbarMode + // this avoids a deadlock with the Main thread + // from Qt::BlockingQueuedEvent invocations later in the call-tree + std::lock_guard guard(_mapMutex); + _toolbarMode = toolbarMode; + } for (auto& iter : _tabletProxies) { iter.second->setToolbarMode(toolbarMode); @@ -89,8 +101,9 @@ void TabletScriptingInterface::setQmlTabletRoot(QString tabletId, QQuickItem* qm } QQuickWindow* TabletScriptingInterface::getTabletWindow() { - TabletProxy* tablet = qobject_cast(getTablet("com.highfidelity.interface.tablet.system")); + TabletProxy* tablet = qobject_cast(getTablet(SYSTEM_TABLET)); QObject* qmlSurface = tablet->getTabletSurface(); + OffscreenQmlSurface* surface = dynamic_cast(qmlSurface); if (!surface) { @@ -156,7 +169,7 @@ void TabletScriptingInterface::processTabletEvents(QObject* object, const QKeyEv void TabletScriptingInterface::processEvent(const QKeyEvent* event) { - TabletProxy* tablet = qobject_cast(getTablet("com.highfidelity.interface.tablet.system")); + TabletProxy* tablet = qobject_cast(getTablet(SYSTEM_TABLET)); QObject* qmlTablet = tablet->getQmlTablet(); QObject* qmlMenu = tablet->getQmlMenu(); @@ -185,10 +198,12 @@ class TabletRootWindow : public QmlWindowClass { virtual QString qmlSource() const override { return "hifi/tablet/WindowRoot.qml"; } }; -TabletProxy::TabletProxy(QString name) : _name(name) { +TabletProxy::TabletProxy(QObject* parent, QString name) : QObject(parent), _name(name) { } void TabletProxy::setToolbarMode(bool toolbarMode) { + std::lock_guard guard(_tabletMutex); + if (toolbarMode == _toolbarMode) { return; } @@ -288,7 +303,7 @@ bool TabletProxy::isPathLoaded(QVariant path) { return path.toString() == _currentPathLoaded.toString(); } void TabletProxy::setQmlTabletRoot(QQuickItem* qmlTabletRoot, QObject* qmlOffscreenSurface) { - std::lock_guard guard(_mutex); + std::lock_guard guard(_tabletMutex); _qmlOffscreenSurface = qmlOffscreenSurface; _qmlTabletRoot = qmlTabletRoot; if (_qmlTabletRoot && _qmlOffscreenSurface) { @@ -519,7 +534,7 @@ void TabletProxy::gotoWebScreen(const QString& url, const QString& injectedJavaS QObject* TabletProxy::addButton(const QVariant& properties) { auto tabletButtonProxy = QSharedPointer(new TabletButtonProxy(properties.toMap())); - std::lock_guard guard(_mutex); + std::lock_guard guard(_tabletMutex); _tabletButtonProxies.push_back(tabletButtonProxy); if (!_toolbarMode && _qmlTabletRoot) { auto tablet = getQmlTablet(); @@ -555,7 +570,7 @@ bool TabletProxy::onHomeScreen() { } void TabletProxy::removeButton(QObject* tabletButtonProxy) { - std::lock_guard guard(_mutex); + std::lock_guard guard(_tabletMutex); auto tablet = getQmlTablet(); if (!tablet) { @@ -743,12 +758,12 @@ TabletButtonProxy::TabletButtonProxy(const QVariantMap& properties) : } void TabletButtonProxy::setQmlButton(QQuickItem* qmlButton) { - std::lock_guard guard(_mutex); + std::lock_guard guard(_buttonMutex); _qmlButton = qmlButton; } void TabletButtonProxy::setToolbarButtonProxy(QObject* toolbarButtonProxy) { - std::lock_guard guard(_mutex); + std::lock_guard guard(_buttonMutex); _toolbarButtonProxy = toolbarButtonProxy; if (_toolbarButtonProxy) { QObject::connect(_toolbarButtonProxy, SIGNAL(clicked()), this, SLOT(clickedSlot())); @@ -756,12 +771,12 @@ void TabletButtonProxy::setToolbarButtonProxy(QObject* toolbarButtonProxy) { } QVariantMap TabletButtonProxy::getProperties() const { - std::lock_guard guard(_mutex); + std::lock_guard guard(_buttonMutex); return _properties; } void TabletButtonProxy::editProperties(QVariantMap properties) { - std::lock_guard guard(_mutex); + std::lock_guard guard(_buttonMutex); QVariantMap::const_iterator iter = properties.constBegin(); while (iter != properties.constEnd()) { diff --git a/libraries/script-engine/src/TabletScriptingInterface.h b/libraries/script-engine/src/TabletScriptingInterface.h index aff14128db..85c1fdaf9a 100644 --- a/libraries/script-engine/src/TabletScriptingInterface.h +++ b/libraries/script-engine/src/TabletScriptingInterface.h @@ -71,7 +71,7 @@ private: void processTabletEvents(QObject* object, const QKeyEvent* event); protected: - std::mutex _mutex; + std::mutex _mapMutex; std::map _tabletProxies; QObject* _toolbarScriptingInterface { nullptr }; bool _toolbarMode { false }; @@ -90,7 +90,7 @@ class TabletProxy : public QObject { Q_PROPERTY(bool landscape READ getLandscape WRITE setLandscape) Q_PROPERTY(bool tabletShown MEMBER _tabletShown NOTIFY tabletShownChanged) public: - TabletProxy(QString name); + TabletProxy(QObject* parent, QString name); void setQmlTabletRoot(QQuickItem* qmlTabletRoot, QObject* qmlOffscreenSurface); @@ -248,7 +248,7 @@ protected: QVariant _initialPath { "" }; QVariant _currentPathLoaded { "" }; QString _name; - std::mutex _mutex; + std::mutex _tabletMutex; std::vector> _tabletButtonProxies; QQuickItem* _qmlTabletRoot { nullptr }; QObject* _qmlOffscreenSurface { nullptr }; @@ -309,7 +309,7 @@ signals: protected: QUuid _uuid; int _stableOrder; - mutable std::mutex _mutex; + mutable std::mutex _buttonMutex; QQuickItem* _qmlButton { nullptr }; QObject* _toolbarButtonProxy { nullptr }; QVariantMap _properties;