From 6ce10bd2f00bd7eb4e39902907d3be74837829e8 Mon Sep 17 00:00:00 2001 From: Vasily Date: Thu, 10 Jan 2019 19:21:47 +0100 Subject: [PATCH] Change OverlayController from singleton pattern to regular RAII The singleton pattern was poorly implemented and didn't provide the benefits usually associated with the singleton pattern, with all the negatives. Refer to this github issue: https://github.com/OpenVR-Advanced-Settings/OpenVR-AdvancedSettings/issues/29 --- src/main.cpp | 14 ++-------- src/overlaycontroller.cpp | 59 +++++++++++++++++++++++---------------- src/overlaycontroller.h | 19 +------------ 3 files changed, 39 insertions(+), 53 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index fe968c0..5c72d6b 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -408,16 +408,8 @@ int main( int argc, char* argv[] ) // The OverlayController handles the majority of the application specfic // things. It contains all the other tabs. - // It is created using a singleton pattern that destructs the old - // instance whenever createInstance is called. This is obviously not - // ideal since it can lead to dangling points and use after free. - // The constructor only sets two member vars called desktopMode and - // noSound, the rest of the initialization is done in Init. It is - // unknown why this is done. - advsettings::OverlayController* controller - = advsettings::OverlayController::createInstance( desktopMode, - noSound ); - controller->Init( &qmlEngine ); + advsettings::OverlayController controller( + desktopMode, noSound, qmlEngine ); QQmlComponent component( &qmlEngine, QUrl::fromLocalFile( "res/qml/mainwidget.qml" ) ); @@ -428,7 +420,7 @@ int main( int argc, char* argv[] ) << std::endl; } auto quickObj = component.create(); - controller->SetWidget( + controller.SetWidget( qobject_cast( quickObj ), advsettings::OverlayController::applicationDisplayName, advsettings::OverlayController::applicationKey ); diff --git a/src/overlaycontroller.cpp b/src/overlaycontroller.cpp index fac5885..b69663f 100644 --- a/src/overlaycontroller.cpp +++ b/src/overlaycontroller.cpp @@ -24,18 +24,14 @@ // application namespace namespace advsettings { -std::unique_ptr OverlayController::singleton; - constexpr const char* OverlayController::applicationVersionString; QSettings* OverlayController::_appSettings = nullptr; -OverlayController::~OverlayController() -{ - Shutdown(); -} - -void OverlayController::Init( QQmlEngine* qmlEngine ) +OverlayController::OverlayController( bool desktopMode, + bool noSound, + QQmlEngine& qmlEngine ) + : QObject(), desktopMode( desktopMode ), noSound( noSound ) { // Loading the OpenVR Runtime auto initError = vr::VRInitError_None; @@ -237,29 +233,39 @@ void OverlayController::Init( QQmlEngine* qmlEngine ) accessibilityTabController.initStage1(); // Set qml context - qmlEngine->rootContext()->setContextProperty( "applicationVersion", - getVersionString() ); - qmlEngine->rootContext()->setContextProperty( "vrRuntimePath", - getVRRuntimePathUrl() ); + qmlEngine.rootContext()->setContextProperty( "applicationVersion", + getVersionString() ); + qmlEngine.rootContext()->setContextProperty( "vrRuntimePath", + getVRRuntimePathUrl() ); - // Register qml singletons + // Pretty disgusting trick to allow qmlRegisterSingletonType to continue + // working with the lambdas that were already there. The callback function + // in qmlRegisterSingletonType won't work with any lambdas that capture the + // environment. The alternative to making a static pointer to this was + // rewriting all QML to not be singletons, which should probably be done + // whenever possible. + static OverlayController* const objectAddress = this; qmlRegisterSingletonType( "matzman666.advsettings", 1, 0, "OverlayController", []( QQmlEngine*, QJSEngine* ) { - QObject* obj = getInstance(); + QObject* obj = objectAddress; QQmlEngine::setObjectOwnership( obj, QQmlEngine::CppOwnership ); return obj; } ); + // It is unknown if it it intended for the generic in + // qmlRegisterSingletonType to be in all the + // remaining function calls, or if it's just a copy paste accident that + // happens to work. qmlRegisterSingletonType( "matzman666.advsettings", 1, 0, "SteamVRTabController", []( QQmlEngine*, QJSEngine* ) { - QObject* obj = &getInstance()->steamVRTabController; + QObject* obj = &( objectAddress->steamVRTabController ); QQmlEngine::setObjectOwnership( obj, QQmlEngine::CppOwnership ); return obj; } ); @@ -269,7 +275,7 @@ void OverlayController::Init( QQmlEngine* qmlEngine ) 0, "ChaperoneTabController", []( QQmlEngine*, QJSEngine* ) { - QObject* obj = &getInstance()->chaperoneTabController; + QObject* obj = &( objectAddress->chaperoneTabController ); QQmlEngine::setObjectOwnership( obj, QQmlEngine::CppOwnership ); return obj; } ); @@ -279,7 +285,7 @@ void OverlayController::Init( QQmlEngine* qmlEngine ) 0, "MoveCenterTabController", []( QQmlEngine*, QJSEngine* ) { - QObject* obj = &getInstance()->moveCenterTabController; + QObject* obj = &( objectAddress->moveCenterTabController ); QQmlEngine::setObjectOwnership( obj, QQmlEngine::CppOwnership ); return obj; } ); @@ -289,7 +295,7 @@ void OverlayController::Init( QQmlEngine* qmlEngine ) 0, "FixFloorTabController", []( QQmlEngine*, QJSEngine* ) { - QObject* obj = &getInstance()->fixFloorTabController; + QObject* obj = &( objectAddress->fixFloorTabController ); QQmlEngine::setObjectOwnership( obj, QQmlEngine::CppOwnership ); return obj; } ); @@ -299,7 +305,7 @@ void OverlayController::Init( QQmlEngine* qmlEngine ) 0, "AudioTabController", []( QQmlEngine*, QJSEngine* ) { - QObject* obj = &getInstance()->audioTabController; + QObject* obj = &( objectAddress->audioTabController ); QQmlEngine::setObjectOwnership( obj, QQmlEngine::CppOwnership ); return obj; } ); @@ -309,7 +315,7 @@ void OverlayController::Init( QQmlEngine* qmlEngine ) 0, "StatisticsTabController", []( QQmlEngine*, QJSEngine* ) { - QObject* obj = &getInstance()->statisticsTabController; + QObject* obj = &( objectAddress->statisticsTabController ); QQmlEngine::setObjectOwnership( obj, QQmlEngine::CppOwnership ); return obj; } ); @@ -319,7 +325,7 @@ void OverlayController::Init( QQmlEngine* qmlEngine ) 0, "SettingsTabController", []( QQmlEngine*, QJSEngine* ) { - QObject* obj = &getInstance()->settingsTabController; + QObject* obj = &( objectAddress->settingsTabController ); QQmlEngine::setObjectOwnership( obj, QQmlEngine::CppOwnership ); return obj; } ); @@ -329,7 +335,7 @@ void OverlayController::Init( QQmlEngine* qmlEngine ) 0, "ReviveTabController", []( QQmlEngine*, QJSEngine* ) { - QObject* obj = &getInstance()->reviveTabController; + QObject* obj = &( objectAddress->reviveTabController ); QQmlEngine::setObjectOwnership( obj, QQmlEngine::CppOwnership ); return obj; } ); @@ -339,7 +345,7 @@ void OverlayController::Init( QQmlEngine* qmlEngine ) 0, "UtilitiesTabController", []( QQmlEngine*, QJSEngine* ) { - QObject* obj = &getInstance()->utilitiesTabController; + QObject* obj = &( objectAddress->utilitiesTabController ); QQmlEngine::setObjectOwnership( obj, QQmlEngine::CppOwnership ); return obj; } ); @@ -349,12 +355,17 @@ void OverlayController::Init( QQmlEngine* qmlEngine ) 0, "AccessibilityTabController", []( QQmlEngine*, QJSEngine* ) { - QObject* obj = &getInstance()->accessibilityTabController; + QObject* obj = &( objectAddress->accessibilityTabController ); QQmlEngine::setObjectOwnership( obj, QQmlEngine::CppOwnership ); return obj; } ); } +OverlayController::~OverlayController() +{ + Shutdown(); +} + void OverlayController::Shutdown() { if ( m_pPumpEventsTimer ) diff --git a/src/overlaycontroller.h b/src/overlaycontroller.h index 0f57b0c..36280cb 100644 --- a/src/overlaycontroller.h +++ b/src/overlaycontroller.h @@ -94,16 +94,12 @@ public: // I know it's an ugly hack to make them public to enable external AccessibilityTabController accessibilityTabController; private: - OverlayController( bool desktopMode, bool noSound ) - : QObject(), desktopMode( desktopMode ), noSound( noSound ) - { - } QPoint getMousePositionForEvent( vr::VREvent_Mouse_t mouse ); public: + OverlayController( bool desktopMode, bool noSound, QQmlEngine& qmlEngine ); virtual ~OverlayController(); - void Init( QQmlEngine* qmlEngine ); void Shutdown(); bool isDashboardVisible() @@ -174,21 +170,8 @@ signals: private: static QSettings* _appSettings; - static std::unique_ptr singleton; public: - static OverlayController* getInstance() - { - return singleton.get(); - } - - static OverlayController* createInstance( bool desktopMode, bool noSound ) - { - singleton.reset( - new advsettings::OverlayController( desktopMode, noSound ) ); - return singleton.get(); - } - static QSettings* appSettings() { return _appSettings;