From: Manuel Nickschas <sputnick@quassel-irc.org>
Date: Sat, 29 Sep 2018 09:52:12 +0000 (+0200)
Subject: qtui: Fix quit sequence and lifetime issues
X-Git-Tag: 0.13-rc2~11
X-Git-Url: https://git.quassel-irc.org/?p=quassel.git;a=commitdiff_plain;h=72473527f99cbe68dcfcb4ca17f828bd3775bba7

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 87fa59f8..b1e2f119 100644
--- 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 00c9c974..ef37ad53 100644
--- 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 801fab8b..8d122f39 100644
--- 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 9dc26949..505bb614 100644
--- 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 f6e75a24..c9695cc8 100644
--- 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 56f4f93b..6064312d 100644
--- 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 d335d078..a6c48c35 100644
--- 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 f85d2252..af9ddff3 100644
--- 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 47e3a140..03f887b4 100644
--- 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 508bea52..a5e68e1b 100644
--- 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 fb4a2c7f..3f65505a 100644
--- 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 b7e96bf3..3797d084 100644
--- 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;