Merge branch 'roster-consistency'

Reviewed-by: Andre Magalhaes (andrunko) <andre.magalhaes@collabora.co.uk>

5 files changed, 197 insertions, 38 deletions

diff --git a/TelepathyQt4/contact-manager-internal.h b/TelepathyQt4/contact-manager-internal.h
index 9a9281e9..0205bf05 100644
--- a/TelepathyQt4/contact-manager-internal.h
+++ b/TelepathyQt4/contact-manager-internal.h
@@ -41,6 +41,21 @@ private Q_SLOTS:
     void onChannelClosed(Tp::PendingOperation *);
 };
 
+class TELEPATHY_QT4_NO_EXPORT RosterModifyFinishOp : public PendingOperation
+{
+    Q_OBJECT
+
+public:
+    RosterModifyFinishOp(const ConnectionPtr &conn);
+    ~RosterModifyFinishOp() {};
+
+    void setError(const QString &errorName, const QString &errorMessage);
+    void finish();
+
+private:
+    QString errorName, errorMessage;
+};
+
 } // Tp
 
 #endif
diff --git a/TelepathyQt4/contact-manager.cpp b/TelepathyQt4/contact-manager.cpp
index fb5c4b62..f8ecc4c5 100644
--- a/TelepathyQt4/contact-manager.cpp
+++ b/TelepathyQt4/contact-manager.cpp
@@ -65,6 +65,9 @@ struct TELEPATHY_QT4_NO_EXPORT ContactManager::Private
     void processContactListGroupsCreated();
     void processContactListGroupRenamed();
     void processContactListGroupsRemoved();
+    void processFinishedModify();
+
+    PendingOperation *queuedFinishVoid(const QDBusPendingCall &call);
 
     Contacts allKnownContactsFallback() const;
     void computeKnownContactsChangesFallback(const Contacts &added,
@@ -125,6 +128,9 @@ struct TELEPATHY_QT4_NO_EXPORT ContactManager::Private
     QQueue<QStringList> contactListGroupsRemovedQueue;
     bool processingContactListChanges;
 
+    QMap<Tp::PendingOperation * /* actual */, Tp::RosterModifyFinishOp *> returnedModifyOps;
+    QQueue<RosterModifyFinishOp *> modifyFinishQueue;
+
     // old roster API
     QMap<uint, ContactListChannel> contactListChannels;
     ChannelPtr subscribeChannel;
@@ -376,6 +382,36 @@ void ContactManager::Private::processContactListGroupsRemoved()
     processContactListChanges();
 }
 
+void ContactManager::Private::processFinishedModify()
+{
+    RosterModifyFinishOp *op = modifyFinishQueue.dequeue();
+    // Only continue processing changes (and thus, emitting change signals) when the op has signaled
+    // finish (it'll only do this after we've returned to the mainloop)
+    connect(op,
+            SIGNAL(finished(Tp::PendingOperation*)),
+            parent,
+            SLOT(onModifyFinishSignaled()));
+    op->finish();
+}
+
+PendingOperation *ContactManager::Private::queuedFinishVoid(const QDBusPendingCall &call)
+{
+    PendingOperation *actual = new PendingVoid(call, parent->connection());
+    connect(actual,
+            SIGNAL(finished(Tp::PendingOperation*)),
+            parent,
+            SLOT(onModifyFinished(Tp::PendingOperation*)));
+    RosterModifyFinishOp *toReturn = new RosterModifyFinishOp(parent->connection());
+    returnedModifyOps.insert(actual, toReturn);
+    return toReturn;
+}
+
+void ContactManager::onModifyFinishSignaled()
+{
+    mPriv->processingContactListChanges = false;
+    mPriv->processContactListChanges();
+}
+
 Contacts ContactManager::Private::allKnownContactsFallback() const
 {
     Contacts contacts;
@@ -814,7 +850,7 @@ PendingOperation *ContactManager::addGroup(const QString &group)
     Client::ConnectionInterfaceContactGroupsInterface *iface =
         connection()->interface<Client::ConnectionInterfaceContactGroupsInterface>();
     Q_ASSERT(iface);
-    return new PendingVoid(iface->AddToGroup(group, UIntList()), connection());
+    return mPriv->queuedFinishVoid(iface->AddToGroup(group, UIntList()));
 }
 
 /**
@@ -859,7 +895,7 @@ PendingOperation *ContactManager::removeGroup(const QString &group)
     Client::ConnectionInterfaceContactGroupsInterface *iface =
         connection()->interface<Client::ConnectionInterfaceContactGroupsInterface>();
     Q_ASSERT(iface);
-    return new PendingVoid(iface->RemoveGroup(group), connection());
+    return mPriv->queuedFinishVoid(iface->RemoveGroup(group));
 }
 
 /**
@@ -938,7 +974,7 @@ PendingOperation *ContactManager::addContactsToGroup(const QString &group,
     Client::ConnectionInterfaceContactGroupsInterface *iface =
         connection()->interface<Client::ConnectionInterfaceContactGroupsInterface>();
     Q_ASSERT(iface);
-    return new PendingVoid(iface->AddToGroup(group, handles), connection());
+    return mPriv->queuedFinishVoid(iface->AddToGroup(group, handles));
 }
 
 /**
@@ -983,7 +1019,7 @@ PendingOperation *ContactManager::removeContactsFromGroup(const QString &group,
     Client::ConnectionInterfaceContactGroupsInterface *iface =
         connection()->interface<Client::ConnectionInterfaceContactGroupsInterface>();
     Q_ASSERT(iface);
-    return new PendingVoid(iface->RemoveFromGroup(group, handles), connection());
+    return mPriv->queuedFinishVoid(iface->RemoveFromGroup(group, handles));
 }
 
 /**
@@ -1088,7 +1124,7 @@ PendingOperation *ContactManager::requestPresenceSubscription(
     Client::ConnectionInterfaceContactListInterface *iface =
         connection()->interface<Client::ConnectionInterfaceContactListInterface>();
     Q_ASSERT(iface);
-    return new PendingVoid(iface->RequestSubscription(handles, message), connection());
+    return mPriv->queuedFinishVoid(iface->RequestSubscription(handles, message));
 }
 
 /**
@@ -1226,7 +1262,8 @@ PendingOperation *ContactManager::removePresenceSubscription(
     Client::ConnectionInterfaceContactListInterface *iface =
         connection()->interface<Client::ConnectionInterfaceContactListInterface>();
     Q_ASSERT(iface);
-    return new PendingVoid(iface->Unsubscribe(handles), connection());
+
+    return mPriv->queuedFinishVoid(iface->Unsubscribe(handles));
 }
 
 /**
@@ -1318,7 +1355,7 @@ PendingOperation *ContactManager::authorizePresencePublication(
     Client::ConnectionInterfaceContactListInterface *iface =
         connection()->interface<Client::ConnectionInterfaceContactListInterface>();
     Q_ASSERT(iface);
-    return new PendingVoid(iface->AuthorizePublication(handles), connection());
+    return mPriv->queuedFinishVoid(iface->AuthorizePublication(handles));
 }
 
 /**
@@ -1443,7 +1480,7 @@ PendingOperation *ContactManager::removePresencePublication(
     Client::ConnectionInterfaceContactListInterface *iface =
         connection()->interface<Client::ConnectionInterfaceContactListInterface>();
     Q_ASSERT(iface);
-    return new PendingVoid(iface->Unpublish(handles), connection());
+    return mPriv->queuedFinishVoid(iface->Unpublish(handles));
 }
 
 /**
@@ -1482,7 +1519,7 @@ PendingOperation *ContactManager::removeContacts(
     Client::ConnectionInterfaceContactListInterface *iface =
         connection()->interface<Client::ConnectionInterfaceContactListInterface>();
     Q_ASSERT(iface);
-    return new PendingVoid(iface->RemoveContacts(handles), connection());
+    return mPriv->queuedFinishVoid(iface->RemoveContacts(handles));
 }
 
 /**
@@ -1935,6 +1972,22 @@ void ContactManager::onContactListGroupsRemoved(const QStringList &names)
     mPriv->processContactListChanges();
 }
 
+void ContactManager::onModifyFinished(Tp::PendingOperation *op)
+{
+    RosterModifyFinishOp *returned = mPriv->returnedModifyOps.take(op);
+
+    // Finished twice, or we didn't add the returned op at all?
+    Q_ASSERT(returned);
+
+    if (op->isError()) {
+        returned->setError(op->errorName(), op->errorMessage());
+    }
+
+    mPriv->modifyFinishQueue.enqueue(returned);
+    mPriv->contactListChangesQueue.enqueue(&Private::processFinishedModify);
+    mPriv->processContactListChanges();
+}
+
 void ContactManager::onStoredChannelMembersChangedFallback(
         const Contacts &groupMembersAdded,
         const Contacts &groupLocalPendingMembersAdded,
@@ -2432,6 +2485,31 @@ void PendingContactManagerRemoveContactListGroup::onChannelClosed(PendingOperati
     }
 }
 
+RosterModifyFinishOp::RosterModifyFinishOp(const ConnectionPtr &conn)
+    : PendingOperation(conn)
+{
+}
+
+void RosterModifyFinishOp::setError(const QString &errorName, const QString &errorMessage)
+{
+    Q_ASSERT(this->errorName.isEmpty());
+    Q_ASSERT(this->errorMessage.isEmpty());
+
+    Q_ASSERT(!errorName.isEmpty());
+
+    this->errorName = errorName;
+    this->errorMessage = errorMessage;
+}
+
+void RosterModifyFinishOp::finish()
+{
+    if (errorName.isEmpty()) {
+        setFinished();
+    } else {
+        setFinishedWithError(errorName, errorMessage);
+    }
+}
+
 void ContactManager::connectNotify(const char *signalName)
 {
     if (qstrcmp(signalName, SIGNAL(presencePublicationRequested(Tp::Contacts,Tp::Channel::GroupMemberChangeDetails))) == 0) {
diff --git a/TelepathyQt4/contact-manager.h b/TelepathyQt4/contact-manager.h
index 75d3cb63..2ec05257 100644
--- a/TelepathyQt4/contact-manager.h
+++ b/TelepathyQt4/contact-manager.h
@@ -156,6 +156,9 @@ private Q_SLOTS:
     void onContactListGroupRenamed(const QString &oldName, const QString &newName);
     void onContactListGroupsRemoved(const QStringList &names);
 
+    void onModifyFinished(Tp::PendingOperation *op);
+    void onModifyFinishSignaled();
+
     void onStoredChannelMembersChangedFallback(
         const Tp::Contacts &groupMembersAdded,
         const Tp::Contacts &groupLocalPendingMembersAdded,
diff --git a/tests/dbus/conn-roster-groups.cpp b/tests/dbus/conn-roster-groups.cpp
index e8527967..caa5cafa 100644
--- a/tests/dbus/conn-roster-groups.cpp
+++ b/tests/dbus/conn-roster-groups.cpp
@@ -34,6 +34,9 @@ public:
           mContactsAddedToGroup(0), mContactsRemovedFromGroup(0)
     { }
 
+private:
+    void causeCongestion(const ConnectionPtr &conn, const ContactPtr &contact);
+
 protected Q_SLOTS:
     void onGroupAdded(const QString &group);
     void onGroupRemoved(const QString &group);
@@ -65,29 +68,54 @@ private:
     bool mConnInvalidated;
 };
 
+void TestConnRosterGroups::causeCongestion(const ConnectionPtr &conn, const ContactPtr &contact)
+{
+    // Cause some congestion in the roster events queue so we can check that it doesn't cause
+    // inconsistent event reordering
+    for (int i = 0; i < 5; i++) {
+        QString name = QString(QLatin1String("Rush%1")).arg(i);
+        conn->contactManager()->addGroup(name);
+        conn->contactManager()->addContactsToGroup(name, QList<ContactPtr>() << contact);
+        contact->requestPresenceSubscription();
+        contact->removePresenceSubscription();
+        conn->contactManager()->removeGroup(name);
+    }
+}
+
 void TestConnRosterGroups::onGroupAdded(const QString &group)
 {
+    if (group.startsWith(QLatin1String("Rush"))) {
+        return;
+    }
+
     mGroupAdded = group;
-    mLoop->exit(0);
 }
 
 void TestConnRosterGroups::onGroupRemoved(const QString &group)
 {
+    if (group.startsWith(QLatin1String("Rush"))) {
+        return;
+    }
+
     mGroupRemoved = group;
-    mLoop->exit(0);
 }
 
-
 void TestConnRosterGroups::onContactAddedToGroup(const QString &group)
 {
+    if (group.startsWith(QLatin1String("Rush"))) {
+        return;
+    }
+
     mContactsAddedToGroup++;
-    mLoop->exit(0);
 }
 
 void TestConnRosterGroups::onContactRemovedFromGroup(const QString &group)
 {
+    if (group.startsWith(QLatin1String("Rush"))) {
+        return;
+    }
+
     mContactsRemovedFromGroup++;
-    mLoop->exit(0);
 }
 
 void TestConnRosterGroups::expectConnInvalidated()
@@ -162,7 +190,8 @@ void TestConnRosterGroups::testRosterGroups()
     QCOMPARE(mConn->isReady(), true);
     QCOMPARE(mConn->status(), ConnectionStatusConnected);
 
-    Features features = Features() << Connection::FeatureRoster << Connection::FeatureRosterGroups;
+    Features features = Features() << Connection::FeatureRoster << Connection::FeatureRosterGroups
+        << Connection::FeatureSelfContact;
     QVERIFY(connect(mConn->becomeReady(features),
             SIGNAL(finished(Tp::PendingOperation*)),
             SLOT(expectSuccessfulCall(Tp::PendingOperation*))));
@@ -182,17 +211,20 @@ void TestConnRosterGroups::testRosterGroups()
     QString group(QLatin1String("foo"));
     QVERIFY(contactManager->groupContacts(group).isEmpty());
 
+    causeCongestion(mConn, mConn->selfContact());
+
     // add group foo
     QVERIFY(connect(contactManager.data(),
                     SIGNAL(groupAdded(const QString&)),
                     SLOT(onGroupAdded(const QString&))));
+
+    causeCongestion(mConn, mConn->selfContact());
+
     QVERIFY(connect(contactManager->addGroup(group),
                     SIGNAL(finished(Tp::PendingOperation*)),
                     SLOT(expectSuccessfulCall(Tp::PendingOperation*))));
     QCOMPARE(mLoop->exec(), 0);
-    while (mGroupAdded.isEmpty()) {
-        QCOMPARE(mLoop->exec(), 0);
-    }
+    QVERIFY(!mGroupAdded.isEmpty());
     QCOMPARE(mGroupAdded, group);
 
     expectedGroups << group;
@@ -201,6 +233,8 @@ void TestConnRosterGroups::testRosterGroups()
     groups.sort();
     QCOMPARE(groups, expectedGroups);
 
+    causeCongestion(mConn, mConn->selfContact());
+
     // add Montreal contacts to group foo
     Contacts contacts = contactManager->groupContacts(QLatin1String("Montreal"));
     Q_FOREACH (const ContactPtr &contact, contacts) {
@@ -208,17 +242,20 @@ void TestConnRosterGroups::testRosterGroups()
                         SIGNAL(addedToGroup(const QString&)),
                         SLOT(onContactAddedToGroup(const QString&))));
     }
+
+    causeCongestion(mConn, mConn->selfContact());
+
     QVERIFY(connect(contactManager->addContactsToGroup(group, contacts.toList()),
                     SIGNAL(finished(Tp::PendingOperation*)),
                     SLOT(expectSuccessfulCall(Tp::PendingOperation*))));
     QCOMPARE(mLoop->exec(), 0);
-    while (mContactsAddedToGroup != contacts.size()) {
-        QCOMPARE(mLoop->exec(), 0);
-    }
+    QCOMPARE(mContactsAddedToGroup, contacts.size());
     Q_FOREACH (const ContactPtr &contact, contacts) {
         QVERIFY(contact->groups().contains(group));
     }
 
+    causeCongestion(mConn, mConn->selfContact());
+
     // remove all contacts from group foo
     contacts = contactManager->groupContacts(group);
     Q_FOREACH (const ContactPtr &contact, contacts) {
@@ -226,28 +263,32 @@ void TestConnRosterGroups::testRosterGroups()
                         SIGNAL(removedFromGroup(const QString&)),
                         SLOT(onContactRemovedFromGroup(const QString&))));
     }
+
+    causeCongestion(mConn, mConn->selfContact());
+
     QVERIFY(connect(contactManager->removeContactsFromGroup(group, contacts.toList()),
                     SIGNAL(finished(Tp::PendingOperation*)),
                     SLOT(expectSuccessfulCall(Tp::PendingOperation*))));
     QCOMPARE(mLoop->exec(), 0);
-    while (mContactsRemovedFromGroup != contacts.size()) {
-        QCOMPARE(mLoop->exec(), 0);
-    }
+    QCOMPARE(mContactsRemovedFromGroup, contacts.size());
     Q_FOREACH (const ContactPtr &contact, contacts) {
         QVERIFY(!contact->groups().contains(group));
     }
 
-    // add group foo
+    causeCongestion(mConn, mConn->selfContact());
+
+    // remove group foo
     QVERIFY(connect(contactManager.data(),
                     SIGNAL(groupRemoved(const QString&)),
                     SLOT(onGroupRemoved(const QString&))));
+
+    causeCongestion(mConn, mConn->selfContact());
+
     QVERIFY(connect(contactManager->removeGroup(group),
                     SIGNAL(finished(Tp::PendingOperation*)),
                     SLOT(expectSuccessfulCall(Tp::PendingOperation*))));
     QCOMPARE(mLoop->exec(), 0);
-    while (mGroupRemoved.isEmpty()) {
-        QCOMPARE(mLoop->exec(), 0);
-    }
+    QVERIFY(!mGroupRemoved.isEmpty());
     QCOMPARE(mGroupRemoved, group);
 
     expectedGroups.removeOne(group);
@@ -351,6 +392,8 @@ void TestConnRosterGroups::testNotADeathTrap()
     QCOMPARE(mLoop->exec(), 0);
     QCOMPARE(mConn->isReady(features), true);
 
+    causeCongestion(mConn, mContact);
+
     // The roster functions should work now
     QVERIFY(connect(mConn->contactManager()->requestPresenceSubscription(
                     QList<ContactPtr>() << mContact,
@@ -361,11 +404,7 @@ void TestConnRosterGroups::testNotADeathTrap()
 
     QVERIFY(mContact->subscriptionState() != Contact::PresenceStateNo);
 
-    // Bah... The test CM fails to cancel its "accept auth request" synthesized event even if we
-    // cancel the subscription request, and that event may screw up the rest of the test. So, wait
-    // for the event here.
-    while (mContact->subscriptionState() != Contact::PresenceStateYes)
-        mLoop->processEvents();
+    causeCongestion(mConn, mContact);
 
     QVERIFY(connect(mConn->contactManager()->removePresenceSubscription(
                     QList<ContactPtr>() << mContact,
@@ -376,6 +415,8 @@ void TestConnRosterGroups::testNotADeathTrap()
 
     QCOMPARE(mContact->subscriptionState(), Contact::PresenceStateNo);
 
+    causeCongestion(mConn, mContact);
+
     QVERIFY(connect(mConn->contactManager()->authorizePresencePublication(
                     QList<ContactPtr>() << mContact,
                     QLatin1String("Please don't fail")),
@@ -383,6 +424,8 @@ void TestConnRosterGroups::testNotADeathTrap()
                 SLOT(expectSuccessfulCall(Tp::PendingOperation*))));
     QCOMPARE(mLoop->exec(), 0);
 
+    causeCongestion(mConn, mContact);
+
     QVERIFY(connect(mConn->contactManager()->removePresencePublication(
                     QList<ContactPtr>() << mContact,
                     QLatin1String("Please don't fail")),
@@ -440,6 +483,8 @@ void TestConnRosterGroups::testNotADeathTrap()
                 SLOT(expectSuccessfulCall(Tp::PendingOperation*))));
     QCOMPARE(mLoop->exec(), 0);
 
+    causeCongestion(mConn, mContact);
+
     QVERIFY(connect(mConn->contactManager()->requestPresenceSubscription(
                     QList<ContactPtr>() << mContact,
                     QLatin1String("Please don't fail")),
@@ -449,11 +494,7 @@ void TestConnRosterGroups::testNotADeathTrap()
 
     QVERIFY(mContact->subscriptionState() != Contact::PresenceStateNo);
 
-    // Bah... The test CM fails to cancel its "accept auth request" synthesized event even if we
-    // cancel the subscription request, and that event may screw up the rest of the test. So, wait
-    // for the event here.
-    while (mContact->subscriptionState() != Contact::PresenceStateYes)
-        mLoop->processEvents();
+    causeCongestion(mConn, mContact);
 
     QVERIFY(connect(mConn->contactManager()->removePresenceSubscription(
                     QList<ContactPtr>() << mContact,
@@ -478,28 +519,46 @@ void TestConnRosterGroups::testNotADeathTrap()
                 SLOT(expectSuccessfulCall(Tp::PendingOperation*))));
     QCOMPARE(mLoop->exec(), 0);
 
+    causeCongestion(mConn, mContact);
+
     QVERIFY(connect(mConn->contactManager()->addGroup(QLatin1String("My successful entourage")),
                 SIGNAL(finished(Tp::PendingOperation*)),
                 SLOT(expectSuccessfulCall(Tp::PendingOperation*))));
     QCOMPARE(mLoop->exec(), 0);
 
+    QVERIFY(mConn->contactManager()->allKnownGroups().contains(QLatin1String("My successful entourage")));
+
+    causeCongestion(mConn, mContact);
+
     QVERIFY(connect(mConn->contactManager()->addContactsToGroup(QLatin1String("My successful entourage"),
                     QList<ContactPtr>() << mContact),
                 SIGNAL(finished(Tp::PendingOperation*)),
                 SLOT(expectSuccessfulCall(Tp::PendingOperation*))));
     QCOMPARE(mLoop->exec(), 0);
 
+    QVERIFY(mConn->contactManager()->
+        groupContacts(QLatin1String("My successful entourage")).contains(mContact));
+
+    causeCongestion(mConn, mContact);
+
     QVERIFY(connect(mConn->contactManager()->removeContactsFromGroup(QLatin1String("My successful entourage"),
                     QList<ContactPtr>() << mContact),
                 SIGNAL(finished(Tp::PendingOperation*)),
                 SLOT(expectSuccessfulCall(Tp::PendingOperation*))));
     QCOMPARE(mLoop->exec(), 0);
 
+    QVERIFY(!mConn->contactManager()->
+        groupContacts(QLatin1String("My successful entourage")).contains(mContact));
+
+    causeCongestion(mConn, mContact);
+
     QVERIFY(connect(mConn->contactManager()->removeGroup(QLatin1String("My successful entourage")),
                 SIGNAL(finished(Tp::PendingOperation*)),
                 SLOT(expectSuccessfulCall(Tp::PendingOperation*))));
     QCOMPARE(mLoop->exec(), 0);
 
+    QVERIFY(!mConn->contactManager()->allKnownGroups().contains(QLatin1String("My successful entourage")));
+
     // Now, invalidate the connection by disconnecting it
     QVERIFY(connect(mConn.data(),
                 SIGNAL(invalidated(Tp::DBusProxy *,
diff --git a/tests/lib/glib/contactlist2/contact-list.c b/tests/lib/glib/contactlist2/contact-list.c
index fec9afe7..bdefc149 100644
--- a/tests/lib/glib/contactlist2/contact-list.c
+++ b/tests/lib/glib/contactlist2/contact-list.c
@@ -883,6 +883,10 @@ receive_authorized (gpointer p)
   if (d->subscribe)
     return FALSE;
 
+  /* DITTO, if our subscription request was cancelled in the meantime */
+  if (!d->subscribe_requested)
+    return FALSE;
+
   d->subscribe_requested = FALSE;
   d->subscribe_rejected = FALSE;
   d->subscribe = TRUE;