Merge lp:~pete-woods/unity-notifications/handle-client-death into lp:unity-notifications

[Pete Woods]

Handle client death by cleaning up notifications

Handle client death by cleaning up notifications

[Charles Kerr (charlesk)]

Looks good overall. Inline are one or two optional suggestions, plus one NF for how NotificationModel::removeAllNotificationsForClient() handles its loops.

review: Needs Fixing

[lp:~pete-woods/unity-notifications/handle-client-death]

228. By Pete Woods on 2015-06-16

Fix method argument order on GetServerInformation

229. By Pete Woods on 2015-06-16

Pass back full notification information to tests

230. By Pete Woods on 2015-06-16

Remove diff noise

[Charles Kerr (charlesk)]

Nice work!

review: Approve

[lp:~pete-woods/unity-notifications/handle-client-death]

232. By Pete Woods on 2015-06-23

Fix parallel build issues

234. By Pete Woods on 2015-06-23

Actually pass in notification server

235. By Pete Woods on 2015-06-23

Notifications are now owned by the client that opened them

236. By Pete Woods on 2015-06-23

Handle local notifications

237. By Pete Woods on 2015-06-23

Handle client death

=== modified file 'include/NotificationModel.h'
--- include/NotificationModel.h	2015-06-23 15:49:07 +0000
+++ include/NotificationModel.h	2015-06-23 15:49:07 +0000
@@ -50,6 +50,7 @@
     QSharedPointer<Notification> getNotification(const QString &summary) const;
     QSharedPointer<Notification> getDisplayedNotification(int index) const;
     bool hasNotification(NotificationID id) const;
+    QList<QSharedPointer<Notification>> removeAllNotificationsForClient(const QString& clientId);
 
     // getRaw() is only meant to be used from QML, since QML cannot handle
     // QSharedPointers... on C++-side only use getNotification()
@@ -87,7 +88,7 @@
     void insertSnap(const QSharedPointer<Notification> &n);
     void insertExtSnap(const QSharedPointer<Notification> &n);
     void insertToVisible(const QSharedPointer<Notification> &n, int location=-1);
-    void deleteFromVisible(int loc);
+    QSharedPointer<Notification> deleteFromVisible(int loc);
     void deleteFirst();
     int findFirst(const Notification::Type type) const;
     int countShowing(const Notification::Type type) const;

=== modified file 'include/NotificationServer.h'
--- include/NotificationServer.h	2015-06-23 15:49:07 +0000
+++ include/NotificationServer.h	2015-06-23 15:49:07 +0000
@@ -45,6 +45,7 @@
 #include <QDBusVariant>
 #include <QDBusConnection>
 #include <QDBusContext>
+#include <QDBusServiceWatcher>
 
 class NotificationModel;
 class Notification;
@@ -70,6 +71,9 @@
     void onDataChanged(unsigned int id);
     void onCompleted(unsigned int id);
 
+private Q_SLOTS:
+    void serviceUnregistered(const QString &service);
+
 Q_SIGNALS:
     void NotificationClosed(unsigned int id, unsigned int reason);
     void ActionInvoked(unsigned int id, const QString &action_key);
@@ -78,11 +82,13 @@
 private:
     void incrementCounter();
     QString messageSender();
+    bool isCmdLine();
 
     QSharedPointer<Notification> buildNotification(NotificationID id, const QVariantMap &hints);
     NotificationModel &model;
     unsigned int idCounter;
     QDBusConnection m_connection;
+    QDBusServiceWatcher m_watcher;
 
 };
 

=== modified file 'src/NotificationModel.cpp'
--- src/NotificationModel.cpp	2015-06-23 15:49:07 +0000
+++ src/NotificationModel.cpp	2015-06-23 15:49:07 +0000
@@ -220,6 +220,54 @@
     return !(getNotification(id).isNull());
 }
 
+QList<QSharedPointer<Notification>> NotificationModel::removeAllNotificationsForClient(const QString& clientId)
+{
+    QList<QSharedPointer<Notification>> notifications;
+
+    for(int i=0; i<p->ephemeralQueue.size();) {
+        if(p->ephemeralQueue[i]->getClientId() == clientId) {
+            notifications << p->ephemeralQueue.takeAt(i);
+            Q_EMIT queueSizeChanged(queued());
+        } else {
+            ++i;
+        }
+    }
+
+    for(int i=0; i<p->snapQueue.size();) {
+        if(p->snapQueue[i]->getClientId() == clientId) {
+            notifications << p->snapQueue.takeAt(i);
+            Q_EMIT queueSizeChanged(queued());
+        } else {
+            ++i;
+        }
+    }
+
+    for(int i=0; i<p->interactiveQueue.size();) {
+        if(p->interactiveQueue[i]->getClientId() == clientId) {
+            notifications << p->interactiveQueue.takeAt(i);
+            Q_EMIT queueSizeChanged(queued());
+        } else {
+            ++i;
+        }
+    }
+
+    bool needToTimeout = false;
+    for(int i=0; i<p->displayedNotifications.size();) {
+        if(p->displayedNotifications[i]->getClientId() == clientId) {
+            notifications << deleteFromVisible(i);
+            needToTimeout = true;
+        } else {
+            ++i;
+        }
+    }
+
+    if (needToTimeout) {
+        timeout(); // Simulate a timeout so visual state is updated.
+    }
+
+    return notifications;
+}
+
 void NotificationModel::removeNotification(const NotificationID id) {
     for(int i=0; i<p->ephemeralQueue.size(); i++) {
         if(p->ephemeralQueue[i]->getID() == id) {
@@ -261,13 +309,14 @@
     deleteFromVisible(0);
 }
 
-void NotificationModel::deleteFromVisible(int loc) {
+QSharedPointer<Notification> NotificationModel::deleteFromVisible(int loc) {
     QModelIndex deletePoint = QModelIndex();
     beginRemoveRows(deletePoint, loc, loc);
     QSharedPointer<Notification> n = p->displayedNotifications[loc];
     p->displayTimes.erase(p->displayTimes.find(n->getID()));
-    p->displayedNotifications.erase(p->displayedNotifications.begin() + loc);
+    auto notification = p->displayedNotifications.takeAt(loc);
     endRemoveRows();
+    return notification;
 }
 
 void NotificationModel::timeout() {

=== modified file 'src/NotificationServer.cpp'
--- src/NotificationServer.cpp	2015-06-23 15:49:07 +0000
+++ src/NotificationServer.cpp	2015-06-23 15:49:07 +0000
@@ -41,6 +41,10 @@
     // Memory managed by Qt
     new NotificationsAdaptor(this);
 
+    m_watcher.setConnection(m_connection);
+    m_watcher.setWatchMode(QDBusServiceWatcher::WatchForUnregistration);
+    connect(&m_watcher, &QDBusServiceWatcher::serviceUnregistered, this, &NotificationServer::serviceUnregistered);
+
     connect(this, SIGNAL(dataChanged(unsigned int)), &m, SLOT(onDataChanged(unsigned int)));
 
     if(!m_connection.registerObject(DBUS_PATH, this)) {
@@ -149,9 +153,11 @@
 
 void NotificationServer::incrementCounter() {
     idCounter++;
-    if(idCounter == 0) // Spec forbids zero as return value.
+    // Spec forbids zero as return value.
+    if(idCounter == 0) {
         idCounter = 1;
 }
+}
 
 QString NotificationServer::messageSender() {
     QString sender(LOCAL_OWNER);
@@ -161,6 +167,24 @@
         return sender;
 }
 
+bool NotificationServer::isCmdLine() {
+    if (!calledFromDBus()) {
+        return false;
+    }
+    QString sender = message().service();
+    uint pid = connection().interface()->servicePid(sender);
+    QString path = QDir(QString("/proc/%1/exe").arg(pid)).canonicalPath();
+    return (path == "/usr/bin/notify-send");
+}
+
+void NotificationServer::serviceUnregistered(const QString &clientId) {
+    m_watcher.removeWatchedService(clientId);
+    auto notifications = model.removeAllNotificationsForClient(clientId);
+    for (auto notification: notifications) {
+        Q_EMIT NotificationClosed(notification->getID(), 1);
+    }
+}
+
 unsigned int NotificationServer::Notify(const QString &app_name, uint replaces_id,
                            const QString &app_icon, const QString &summary,
                            const QString &body, const QStringList &actions,
@@ -268,6 +292,10 @@
     notification->setClientId(clientId);
 
     if (newNotification) {
+        // Don't clean up after the command line client closes
+        if (!isCmdLine()) {
+            m_watcher.addWatchedService(clientId);
+        }
         model.insertNotification(notification);
     } else {
         model.notificationUpdated(currentId);

=== modified file 'test/notificationservertest.cpp'
--- test/notificationservertest.cpp	2015-06-23 15:49:07 +0000
+++ test/notificationservertest.cpp	2015-06-23 15:49:07 +0000
@@ -18,7 +18,7 @@
 
     QSharedPointer<QDBusConnection> secondConnection;
 
-    QSharedPointer<OrgFreedesktopNotificationsInterface> notificationsInterfaceSecond;
+    QSharedPointer<OrgFreedesktopNotificationsInterface> secondNotificationsInterface;
 
 private Q_SLOTS:
 
@@ -48,7 +48,7 @@
                             dbus->sessionBus() + "_second")));
 
     // Create a second connection to the server over a different dbus connection
-    notificationsInterfaceSecond.reset(
+    secondNotificationsInterface.reset(
                 new OrgFreedesktopNotificationsInterface(
                         DBUS_SERVICE_NAME,
                         DBUS_PATH,
@@ -56,7 +56,7 @@
 }
 
 void cleanup() {
-    notificationsInterfaceSecond.reset();
+    secondNotificationsInterface.reset();
 
     secondConnection.reset();
 
@@ -84,7 +84,7 @@
 }
 
 int notifySecond(const QString& name, int replacesId = 0, bool secondInterface = false) {
-    return _notify(notificationsInterfaceSecond, name, replacesId);
+    return _notify(secondNotificationsInterface, name, replacesId);
 }
 
 void expectNotification(const NotificationDataList& notifications, int index, int id, const QString& name) {
@@ -100,7 +100,7 @@
         );
 }
 
-void close(QSharedPointer<OrgFreedesktopNotificationsInterface> interface, int id) {
+void _close(QSharedPointer<OrgFreedesktopNotificationsInterface> interface, int id) {
     auto reply = interface->CloseNotification(id);
     reply.waitForFinished();
     if (reply.isError()) {
@@ -109,11 +109,11 @@
 }
 
 void closeFirst(int id) {
-    close(notificationsInterface, id);
+    _close(notificationsInterface, id);
 }
 
 void closeSecond(int id) {
-    close(notificationsInterfaceSecond, id);
+    _close(secondNotificationsInterface, id);
 }
 
 void testNotify() {
@@ -221,11 +221,78 @@
     uint id1 = notifySecond("1");
     QCOMPARE(id1, uint(1));
 
+    qDebug() << "====== EXPECTED ERRORS BELOW ======";
+
     // Try and update notification from another client
     QVERIFY_EXCEPTION_THROWN(notifyFirst("2", id1), std::domain_error);
 
     // Try and close notification from another client
     QVERIFY_EXCEPTION_THROWN(closeFirst(id1), std::domain_error);
+
+    qDebug() << "====== EXPECTED ERRORS ABOVE ======";
+}
+
+void testNotifyOneClientDies() {
+    QSignalSpy closedSpy(notificationsInterface.data(), SIGNAL(NotificationClosed(uint, uint)));
+
+    uint id1 = notifySecond("1");
+    QCOMPARE(id1, uint(1));
+
+    uint id2 = notifySecond("2");
+    QCOMPARE(id2, uint(2));
+
+    uint id3 = notifySecond("3");
+    QCOMPARE(id3, uint(3));
+
+    uint id4 = notifySecond("4");
+    QCOMPARE(id4, uint(4));
+
+    // Kill off the client without cleaning up
+    QString connectionName = secondConnection->name();
+    secondNotificationsInterface.reset();
+    secondConnection.reset();
+    QDBusConnection::disconnectFromBus(connectionName);
+
+    QVERIFY2(closedSpy.wait(), "We should receive a closed notification");
+
+    // We shouldn't have any notifications left behind
+    {
+        NotificationDataList notifications = notificationsInterface->GetNotifications("my app");
+        QCOMPARE(notifications.size(), 0);
+    }
+}
+
+void testNotifyTwoClientsOneDies() {
+    QSignalSpy closedSpy(notificationsInterface.data(), SIGNAL(NotificationClosed(uint, uint)));
+
+    uint id1 = notifyFirst("1");
+    QCOMPARE(id1, uint(1));
+
+    uint id2 = notifySecond("2");
+    QCOMPARE(id2, uint(2));
+
+    uint id3 = notifyFirst("3");
+    QCOMPARE(id3, uint(3));
+
+    uint id4 = notifySecond("4");
+    QCOMPARE(id4, uint(4));
+
+    // Kill off the client without cleaning up
+    QString connectionName = secondConnection->name();
+    secondNotificationsInterface.reset();
+    secondConnection.reset();
+    QDBusConnection::disconnectFromBus(connectionName);
+
+    QVERIFY2(closedSpy.wait(), "We should receive a closed notification");
+
+    // We should only have notifications from the first client left behind
+    {
+        NotificationDataList notifications = notificationsInterface->GetNotifications("my app");
+        QCOMPARE(notifications.size(), 2);
+
+        expectNotification(notifications, 0, id1, "1");
+        expectNotification(notifications, 1, id3, "3");
+    }
 }
 
 void testGetCapabilities() {