qtui: Fix quit sequence and lifetime issues

Make use of the Singleton mixin for Client and GraphicalUi. Fix
ownership and explicitly construct and destroy the pseudo singletons.
Remove some static init methods in favor of initializing in the
constructor where possible. We still need delayed initialization
of the main UI however, so GraphicalUi::init() has to stay.

Simply calling QCoreApplication::quit() doesn't clean up properly,
because it immediately stops event processing, which means that
any deferred deletions won't be processed anymore.

Since we perform some important and asynchronous tasks during
shutdown, such as saving settings and properly unregistering from
IRC, we really want proper cleanup, though.

Fix this by relying on Quassel::quit() instead, and registering
appropriate quit handlers that shut down the client in orderly
and controlled fashion. Ensure that all open windows and dialogs
are closed prior to MainWin's destruction.

Core shutdown warrants a more complex approach, which will be
implemented in a follow-up commit.

diff --git a/src/client/client.cpp b/src/client/client.cpp
index 87fa59f..b1e2f11 100644 (file)
--- a/src/client/client.cpp
+++ b/src/client/client.cpp
@@ -55,47 +55,12 @@
 #include <stdio.h>
 #include <stdlib.h>
 
-QPointer<Client> Client::instanceptr = 0;
-
-/*** Initialization/destruction ***/
-
-bool Client::instanceExists()
-{
-    return instanceptr;
-}
-
-
-Client *Client::instance()
-{
-    if (!instanceptr)
-        instanceptr = new Client();
-    return instanceptr;
-}
-
-
-void Client::destroy()
-{
-    if (instanceptr) {
-        delete instanceptr->mainUi();
-        instanceptr->deleteLater();
-        instanceptr = 0;
-    }
-}
-
-
-void Client::init(AbstractUi *ui)
-{
-    instance()->_mainUi = ui;
-    instance()->init();
-}
-
-
-Client::Client(QObject *parent)
-    : QObject(parent),
+Client::Client(std::unique_ptr<AbstractUi> ui, QObject *parent)
+    : QObject(parent), Singleton<Client>(this),
     _signalProxy(new SignalProxy(SignalProxy::Client, this)),
-    _mainUi(0),
-    _networkModel(0),
-    _bufferModel(0),
+    _mainUi(std::move(ui)),
+    _networkModel(new NetworkModel(this)),
+    _bufferModel(new BufferModel(_networkModel)),
     _bufferSyncer(0),
     _aliasManager(0),
     _backlogManager(new ClientBacklogManager(this)),
@@ -104,39 +69,28 @@ Client::Client(QObject *parent)
     _coreInfo(new CoreInfo(this)),
     _dccConfig(0),
     _ircListHelper(new ClientIrcListHelper(this)),
-    _inputHandler(0),
+    _inputHandler(new ClientUserInputHandler(this)),
     _networkConfig(0),
     _ignoreListManager(0),
     _highlightRuleManager(0),
     _transferManager(0),
     _transferModel(new TransferModel(this)),
-    _messageModel(0),
-    _messageProcessor(0),
+    _messageModel(_mainUi->createMessageModel(this)),
+    _messageProcessor(_mainUi->createMessageProcessor(this)),
     _coreAccountModel(new CoreAccountModel(this)),
     _coreConnection(new CoreConnection(this)),
     _connected(false)
 {
-    _signalProxy->synchronize(_ircListHelper);
-}
-
-
-Client::~Client()
-{
-    disconnectFromCore();
-}
-
-
-void Client::init()
-{
-    _networkModel = new NetworkModel(this);
+    //connect(mainUi(), SIGNAL(connectToCore(const QVariantMap &)), this, SLOT(connectToCore(const QVariantMap &)));
+    connect(mainUi(), SIGNAL(disconnectFromCore()), this, SLOT(disconnectFromCore()));
+    connect(this, SIGNAL(connected()), mainUi(), SLOT(connectedToCore()));
+    connect(this, SIGNAL(disconnected()), mainUi(), SLOT(disconnectedFromCore()));
 
-    connect(this, SIGNAL(networkRemoved(NetworkId)),
-        _networkModel, SLOT(networkRemoved(NetworkId)));
+    connect(this, SIGNAL(networkRemoved(NetworkId)), _networkModel, SLOT(networkRemoved(NetworkId)));
+    connect(this, SIGNAL(networkRemoved(NetworkId)), _messageProcessor, SLOT(networkRemoved(NetworkId)));
 
-    _bufferModel = new BufferModel(_networkModel);
-    _messageModel = mainUi()->createMessageModel(this);
-    _messageProcessor = mainUi()->createMessageProcessor(this);
-    _inputHandler = new ClientUserInputHandler(this);
+    connect(backlogManager(), SIGNAL(messagesReceived(BufferId, int)), _messageModel, SLOT(messagesReceived(BufferId, int)));
+    connect(coreConnection(), SIGNAL(stateChanged(CoreConnection::ConnectionState)), SLOT(connectionStateChanged(CoreConnection::ConnectionState)));
 
     SignalProxy *p = signalProxy();
 
@@ -163,34 +117,24 @@ void Client::init()
     p->attachSignal(this, SIGNAL(requestKickClient(int)), SIGNAL(kickClient(int)));
     p->attachSlot(SIGNAL(disconnectFromCore()), this, SLOT(disconnectFromCore()));
 
-    //connect(mainUi(), SIGNAL(connectToCore(const QVariantMap &)), this, SLOT(connectToCore(const QVariantMap &)));
-    connect(mainUi(), SIGNAL(disconnectFromCore()), this, SLOT(disconnectFromCore()));
-    connect(this, SIGNAL(connected()), mainUi(), SLOT(connectedToCore()));
-    connect(this, SIGNAL(disconnected()), mainUi(), SLOT(disconnectedFromCore()));
-
-    // Listen to network removed events
-    connect(this, SIGNAL(networkRemoved(NetworkId)),
-        _messageProcessor, SLOT(networkRemoved(NetworkId)));
-
-    // attach backlog manager
     p->synchronize(backlogManager());
-    connect(backlogManager(), SIGNAL(messagesReceived(BufferId, int)), _messageModel, SLOT(messagesReceived(BufferId, int)));
-
-    coreAccountModel()->load();
-
-    // Attach CoreInfo
     p->synchronize(coreInfo());
+    p->synchronize(_ircListHelper);
 
-    connect(coreConnection(), SIGNAL(stateChanged(CoreConnection::ConnectionState)), SLOT(connectionStateChanged(CoreConnection::ConnectionState)));
+    coreAccountModel()->load();
     coreConnection()->init();
 }
 
 
-/*** public static methods ***/
+Client::~Client()
+{
+    disconnectFromCore();
+}
+
 
 AbstractUi *Client::mainUi()
 {
-    return instance()->_mainUi;
+    return instance()->_mainUi.get();
 }
 
 

diff --git a/src/client/client.h b/src/client/client.h
index 00c9c97..ef37ad5 100644 (file)
--- a/src/client/client.h
+++ b/src/client/client.h
@@ -20,6 +20,8 @@
 
 #pragma once
 
+#include <memory>
+
 #include <QList>
 #include <QPointer>
 
@@ -29,6 +31,7 @@
 #include "coreconnection.h"
 #include "highlightrulemanager.h"
 #include "quassel.h"
+#include "singleton.h"
 #include "types.h"
 
 class Message;
@@ -63,7 +66,7 @@ class TransferModel;
 
 struct NetworkInfo;
 
-class Client : public QObject
+class Client : public QObject, public Singleton<Client>
 {
     Q_OBJECT
 
@@ -73,10 +76,9 @@ public:
         RemoteCore
     };
 
-    static bool instanceExists();
-    static Client *instance();
-    static void destroy();
-    static void init(AbstractUi *);
+    Client(std::unique_ptr<AbstractUi>, QObject *parent = nullptr);
+    ~Client() override;
+
     static AbstractUi *mainUi();
 
     static QList<NetworkId> networkIds();
@@ -292,10 +294,6 @@ private slots:
     void sendBufferedUserInput();
 
 private:
-    Client(QObject *parent = 0);
-    virtual ~Client();
-    void init();
-
     void requestInitialBacklog();
 
     /**
@@ -311,10 +309,8 @@ private:
 
     static void addNetwork(Network *);
 
-    static QPointer<Client> instanceptr;
-
     SignalProxy *_signalProxy;
-    AbstractUi *_mainUi;
+    std::unique_ptr<AbstractUi> _mainUi;
     NetworkModel *_networkModel;
     BufferModel *_bufferModel;
     BufferSyncer *_bufferSyncer;

diff --git a/src/qtui/mainwin.cpp b/src/qtui/mainwin.cpp
index 801fab8..8d122f3 100644 (file)
--- a/src/qtui/mainwin.cpp
+++ b/src/qtui/mainwin.cpp
@@ -305,21 +305,17 @@ void MainWin::init()
     // restore locked state of docks
     QtUi::actionCollection("General")->action("LockLayout")->setChecked(s.value("LockLayout", false).toBool());
 
-    QTimer::singleShot(0, this, SLOT(doAutoConnect()));
-}
-
-
-MainWin::~MainWin()
-{
-}
+    Quassel::registerQuitHandler([this]() {
+        QtUiSettings s;
+        saveStateToSettings(s);
+        saveLayout();
+        // Close all open dialogs and the MainWin, so we can safely kill the Client instance afterwards
+        // Note: This does not quit the application, as quitOnLastWindowClosed is set to false.
+        //       We rely on another quit handler to be registered that actually quits the application.
+        qApp->closeAllWindows();
+    });
 
-
-void MainWin::quit()
-{
-    QtUiSettings s;
-    saveStateToSettings(s);
-    saveLayout();
-    QApplication::quit();
+    QTimer::singleShot(0, this, SLOT(doAutoConnect()));
 }
 
 
@@ -416,7 +412,7 @@ void MainWin::setupActions()
     //
     // See https://doc.qt.io/qt-5/qkeysequence.html
     coll->addAction("Quit", new Action(icon::get("application-exit"), tr("&Quit"), coll,
-            this, SLOT(quit()), Qt::CTRL + Qt::Key_Q));
+            Quassel::instance(), SLOT(quit()), Qt::CTRL + Qt::Key_Q));
 
     // View
     coll->addAction("ConfigureBufferViews", new Action(tr("&Configure Chat Lists..."), coll,
@@ -1731,7 +1727,7 @@ void MainWin::closeEvent(QCloseEvent *event)
     else if(!_aboutToQuit) {
         _aboutToQuit = true;
         event->accept();
-        quit();
+        Quassel::instance()->quit();
     }
     else {
         event->ignore();

diff --git a/src/qtui/mainwin.h b/src/qtui/mainwin.h
index 9dc2694..505bb61 100644 (file)
--- a/src/qtui/mainwin.h
+++ b/src/qtui/mainwin.h
@@ -68,7 +68,6 @@ class MainWin
 
 public:
     MainWin(QWidget *parent = 0);
-    virtual ~MainWin();
 
     void init();
 
@@ -102,9 +101,6 @@ public slots:
 
     void onExitRequested(const QString &reason);
 
-    //! Quit application
-    void quit();
-
 protected:
     void closeEvent(QCloseEvent *event);
     void moveEvent(QMoveEvent *event);

diff --git a/src/qtui/monoapplication.cpp b/src/qtui/monoapplication.cpp
index f6e75a2..c9695cc 100644 (file)
--- a/src/qtui/monoapplication.cpp
+++ b/src/qtui/monoapplication.cpp
@@ -53,12 +53,25 @@ void MonolithicApplication::init()
 }
 
 
-MonolithicApplication::~MonolithicApplication()
+Quassel::QuitHandler MonolithicApplication::quitHandler()
 {
-    // Client needs to be destroyed first
-    Client::destroy();
-    _coreThread.quit();
-    _coreThread.wait();
+    return [this]() {
+        connect(_client.get(), SIGNAL(destroyed()), this, SLOT(onClientDestroyed()));
+        _client.release()->deleteLater();
+    };
+}
+
+
+void MonolithicApplication::onClientDestroyed()
+{
+    if (_core) {
+        connect(_core, SIGNAL(destroyed()), QCoreApplication::instance(), SLOT(quit()));
+        _coreThread.quit();
+        _coreThread.wait();
+    }
+    else {
+        QCoreApplication::quit();
+    }
 }
 
 

diff --git a/src/qtui/monoapplication.h b/src/qtui/monoapplication.h
index 56f4f93..6064312 100644 (file)
--- a/src/qtui/monoapplication.h
+++ b/src/qtui/monoapplication.h
@@ -34,15 +34,18 @@ class MonolithicApplication : public QtUiApplication
 
 public:
     MonolithicApplication(int &, char **);
-    ~MonolithicApplication() override;
 
     void init() override;
 
+protected:
+    Quassel::QuitHandler quitHandler() override;
+
 signals:
     void connectInternalPeer(QPointer<InternalPeer> peer);
 
 private slots:
     void onConnectionRequest(QPointer<InternalPeer> peer);
+    void onClientDestroyed();
 
 private:
     void startInternalCore();

diff --git a/src/qtui/qtui.cpp b/src/qtui/qtui.cpp
index d335d07..a6c48c3 100644 (file)
--- a/src/qtui/qtui.cpp
+++ b/src/qtui/qtui.cpp
@@ -40,23 +40,13 @@
 #include "types.h"
 #include "util.h"
 
-MainWin *QtUi::_mainWin = nullptr;
 QList<AbstractNotificationBackend *> QtUi::_notificationBackends;
 QList<AbstractNotificationBackend::Notification> QtUi::_notifications;
 
-namespace {
-
-QtUi *_instance{nullptr};
-
-}
-
 
 QtUi *QtUi::instance()
 {
-    if (!_instance) {
-        _instance = new QtUi();
-    }
-    return _instance;
+    return static_cast<QtUi*>(GraphicalUi::instance());
 }
 
 
@@ -64,44 +54,40 @@ QtUi::QtUi()
     : GraphicalUi()
     , _systemIconTheme{QIcon::themeName()}
 {
+    QtUiSettings uiSettings;
+    Quassel::loadTranslation(uiSettings.value("Locale", QLocale::system()).value<QLocale>());
+
     if (Quassel::isOptionSet("icontheme")) {
         _systemIconTheme = Quassel::optionValue("icontheme");
         QIcon::setThemeName(_systemIconTheme);
     }
-
-    QtUiSettings uiSettings;
-    Quassel::loadTranslation(uiSettings.value("Locale", QLocale::system()).value<QLocale>());
-
     setupIconTheme();
-
     QApplication::setWindowIcon(icon::get("quassel"));
 
-    setContextMenuActionProvider(new ContextMenuActionProvider(this));
-    setToolBarActionProvider(new ToolBarActionProvider(this));
-
     setUiStyle(new QtUiStyle(this));
-    _mainWin = new MainWin();
-
-    setMainWidget(_mainWin);
-
-    connect(_mainWin, SIGNAL(connectToCore(const QVariantMap &)), this, SIGNAL(connectToCore(const QVariantMap &)));
-    connect(_mainWin, SIGNAL(disconnectFromCore()), this, SIGNAL(disconnectFromCore()));
-    connect(Client::instance(), SIGNAL(bufferMarkedAsRead(BufferId)), SLOT(closeNotifications(BufferId)));
 }
 
 
 QtUi::~QtUi()
 {
     unregisterAllNotificationBackends();
-    delete _mainWin;
-    _mainWin = nullptr;
-    _instance = nullptr;
 }
 
 
 void QtUi::init()
 {
+    setContextMenuActionProvider(new ContextMenuActionProvider(this));
+    setToolBarActionProvider(new ToolBarActionProvider(this));
+
+    _mainWin.reset(new MainWin());  // TODO C++14: std::make_unique
+    setMainWidget(_mainWin.get());
+
+    connect(_mainWin.get(), SIGNAL(connectToCore(const QVariantMap &)), this, SIGNAL(connectToCore(const QVariantMap &)));
+    connect(_mainWin.get(), SIGNAL(disconnectFromCore()), this, SIGNAL(disconnectFromCore()));
+    connect(Client::instance(), SIGNAL(bufferMarkedAsRead(BufferId)), SLOT(closeNotifications(BufferId)));
+
     _mainWin->init();
+
     QtUiSettings uiSettings;
     uiSettings.initAndNotify("UseSystemTrayIcon", this, SLOT(useSystemTrayChanged(QVariant)), true);
 

diff --git a/src/qtui/qtui.h b/src/qtui/qtui.h
index f85d225..af9ddff 100644 (file)
--- a/src/qtui/qtui.h
+++ b/src/qtui/qtui.h
@@ -49,7 +49,8 @@ class QtUi : public GraphicalUi
     Q_OBJECT
 
 public:
-    ~QtUi();
+    QtUi();
+    ~QtUi() override;
 
     MessageModel *createMessageModel(QObject *parent) override;
     AbstractMessageProcessor *createMessageProcessor(QObject *parent) override;
@@ -122,18 +123,17 @@ private slots:
     void useSystemTrayChanged(const QVariant &);
 
 private:
-    QtUi();
-
     /**
      * Sets up icon theme handling.
      */
     void setupIconTheme();
 
 private:
-    static MainWin *_mainWin;
     static QList<AbstractNotificationBackend *> _notificationBackends;
     static QList<AbstractNotificationBackend::Notification> _notifications;
 
+    std::unique_ptr<MainWin> _mainWin;
+
     QString _systemIconTheme;
 
 #if QT_VERSION >= 0x050000
@@ -144,4 +144,4 @@ private:
 };
 
 QtUiStyle *QtUi::style() { return qobject_cast<QtUiStyle *>(uiStyle()); }
-MainWin *QtUi::mainWindow() { return _mainWin; }
+MainWin *QtUi::mainWindow() { return instance()->_mainWin.get(); }

diff --git a/src/qtui/qtuiapplication.cpp b/src/qtui/qtuiapplication.cpp
index 47e3a14..03f887b 100644 (file)
--- a/src/qtui/qtuiapplication.cpp
+++ b/src/qtui/qtuiapplication.cpp
@@ -29,7 +29,6 @@
 #endif
 
 #include "chatviewsettings.h"
-#include "client.h"
 #include "cliparser.h"
 #include "mainwin.h"
 #include "qtui.h"
@@ -110,28 +109,32 @@ void QtUiApplication::init()
         throw ExitException{EXIT_FAILURE, tr("Could not load or upgrade client settings!")};
     }
 
-    Client::init(QtUi::instance());
+    _client.reset(new Client(std::unique_ptr<QtUi>(new QtUi())));  // TODO C++14: std::make_unique
 
     // Init UI only after the event loop has started
     // TODO Qt5: Make this a lambda
     QTimer::singleShot(0, this, SLOT(initUi()));
-
-    Quassel::registerQuitHandler([]() {
-        QtUi::mainWindow()->quit();
-    });
 }
 
 
-QtUiApplication::~QtUiApplication()
+void QtUiApplication::initUi()
 {
-    Client::destroy();
+    QtUi::instance()->init();
+
+    // Needs to happen after UI init, so the MainWin quit handler is registered first
+    Quassel::registerQuitHandler(quitHandler());
+
+    resumeSessionIfPossible();
 }
 
 
-void QtUiApplication::initUi()
+Quassel::QuitHandler QtUiApplication::quitHandler()
 {
-    QtUi::instance()->init();
-    resumeSessionIfPossible();
+    // Wait until the Client instance is destroyed before quitting the event loop
+    return [this]() {
+        connect(_client.get(), SIGNAL(destroyed()), QCoreApplication::instance(), SLOT(quit()));
+        _client.release()->deleteLater();
+    };
 }
 
 

diff --git a/src/qtui/qtuiapplication.h b/src/qtui/qtuiapplication.h
index 508bea5..a5e68e1 100644 (file)
--- a/src/qtui/qtuiapplication.h
+++ b/src/qtui/qtuiapplication.h
@@ -20,6 +20,8 @@
 
 #pragma once
 
+#include <memory>
+
 #ifdef HAVE_KDE4
 #  include <KApplication>
 #else
@@ -28,6 +30,7 @@
 
 #include <QSessionManager>
 
+#include "client.h"
 #include "quassel.h"
 #include "uisettings.h"
 #include "qtuisettings.h"
@@ -46,7 +49,7 @@ class QtUiApplication : public QApplication
 
 public:
     QtUiApplication(int &, char **);
-    ~QtUiApplication();
+
     virtual void init();
 
     void resumeSessionIfPossible();
@@ -60,6 +63,9 @@ public:
     void saveState(QSessionManager &manager);
 #endif
 
+protected:
+    virtual Quassel::QuitHandler quitHandler();
+
 private:
     /**
      * Migrate settings if neccessary and possible
@@ -86,6 +92,9 @@ private:
 private slots:
     void initUi();
 
+protected:
+    std::unique_ptr<Client> _client;
+
 private:
     bool _aboutToQuit{false};
 };

diff --git a/src/uisupport/graphicalui.cpp b/src/uisupport/graphicalui.cpp
index fb4a2c7..3f65505 100644 (file)
--- a/src/uisupport/graphicalui.cpp
+++ b/src/uisupport/graphicalui.cpp
@@ -43,23 +43,9 @@ ToolBarActionProvider *GraphicalUi::_toolBarActionProvider = 0;
 UiStyle *GraphicalUi::_uiStyle = 0;
 bool GraphicalUi::_onAllDesktops = false;
 
-namespace {
 
-GraphicalUi *_instance{nullptr};
-
-}
-
-
-GraphicalUi *GraphicalUi::instance() {
-    return _instance;
-}
-
-
-GraphicalUi::GraphicalUi(QObject *parent) : AbstractUi(parent)
+GraphicalUi::GraphicalUi(QObject *parent) : AbstractUi(parent), Singleton<GraphicalUi>(this)
 {
-    Q_ASSERT(!_instance);
-    _instance = this;
-
 #ifdef Q_OS_WIN
     _dwTickCount = 0;
 #endif

diff --git a/src/uisupport/graphicalui.h b/src/uisupport/graphicalui.h
index b7e96bf..3797d08 100644 (file)
--- a/src/uisupport/graphicalui.h
+++ b/src/uisupport/graphicalui.h
@@ -22,6 +22,7 @@
 #define GRAPHICALUI_H_
 
 #include "abstractui.h"
+#include "singleton.h"
 
 class ActionCollection;
 class ContextMenuActionProvider;
@@ -35,7 +36,7 @@ class UiStyle;
 #include <Carbon/Carbon.h>
 #endif
 
-class GraphicalUi : public AbstractUi
+class GraphicalUi : public AbstractUi, protected Singleton<GraphicalUi>
 {
     Q_OBJECT
 
@@ -109,8 +110,6 @@ protected slots:
     virtual void disconnectedFromCore();
 
 private:
-    static GraphicalUi *instance();
-
     static QWidget *_mainWidget;
     static QHash<QString, ActionCollection *> _actionCollections;
     static ContextMenuActionProvider *_contextMenuActionProvider;