Bug 20033

Comments on andrunko's branch, apart from some that I already told him on IRC.

+    if (group.isEmpty() || group.trimmed().isEmpty()) {
+        return new PendingFailure(this, TELEPATHY_ERROR_INVALID_ARGUMENT,
+                "Group must be a non-empty UTF-8 string");
+    }

This is wrong - nothing says that CMs don't support groups with the empty name, or groups whose name is entirely whitespace. Don't do this check - if the CM doesn't like your group name, it is the CM's responsibility to fail.

+    // TODO Check here. Spec states that in order to create an empty group
+    //      RequestHandles with HandleType 'HandleTypeGroup' and the UTF-8 name
+    //      of the group should be called, but this just does not work.
+    //      Using CreateChannel does the job, so let's use it.
+    /*
+    return connection()->requestHandles(HandleTypeGroup,
+            QStringList() << group);
+    */

You are right that this is both insufficient and unnecessary. The old way to make a group was RequestHandles() followed by RequestChannel(), but CreateChannel() does that for us.

+/**
+ * Attempt to add an user-defined contact list group named \a group.
+ *
+ * On some protocols (e.g. XMPP) empty groups are not represented on the server,
+ * so disconnecting from the server and reconnecting might cause empty groups to
+ * vanish.
+ *
+ * \param group Group name.
+ * \return A pending operation which will return when an attempt has been made
+ *         to add an user-defined contact list group.
+ * \sa groupAdded(), groupAddContacts()
+ */
+PendingOperation *ContactManager::addGroup(const QString &group)

+    return connection()->createChannel(request);

I think this function should use ensureChannel, so it succeeds if the group already exists.

Do we actually need API to create a group at all, or should the C++ API to create groups just be "add a contact to the desired group"?

+/**
+ * Attempt to remove an user-defined contact list group named \a group.
+ *
+ * User-defined contact list groups may only be deleted if the group is
+ * already empty.

If the user has explicitly called a method called removeGroup(), wouldn't it be friendlier to remove all the members and then close the channel?

(The restriction on deleting non-empty groups via the D-Bus API is to stop naive clients, like MC 4, accidentally deleting all your groups because they don't know where to dispatch them... it's not desirable here.)

-void ContactManager::setContactListChannels(
+void ContactManager::setContactListsChannels(

Revert this, please. The old version is correct English.

+/**
+ * Return a list of user-defined contact list groups the contact belongs.
+ *
+ * This method requires Connection::FeatureRosterGroups to be enabled.

Can we have a feature on Contact objects, that implicitly enables that Connection feature the first time it is enable on any Contact?

I think a setGroups() method would be good to have - in the current D-Bus API it would become an inefficient series of calls to group{Add,Remove}Contacts, but in a future (more Contact-centric) D-Bus API, it would be a single D-Bus call.

+void TestConnRosterGroups::initTestCase()
...
+    g_set_prgname("conn-basics");

No. Hope that helps. :-P

What's the test coverage in lcov like? I find it hard to believe that this test covers all the new functionality, although what's there looks reasonable.

The obligatory en_BR -> en_GB pass through the docs:

I don't know whether it's better to call things "contact groups" or "contact list groups"; I vaguely lean towards "contact groups". It may be worth adding a brief explanation of the concept of a contact [list] group in one function header, and having a \sa to it from every other mention. In particular, mention that in most protocols, contact groups behave like tags, so a contact can be in 0, 1 or many groups.

Perhaps we need some support for checking the Only_One_Group flag (<http://telepathy.freedesktop.org/spec/org.freedesktop.Telepathy.Channel.Interface.Group.html#org.freedesktop.Telepathy.Channel.Interface.Group.Channel_Group_Flags>), but tbh that can wait.

+ * Return a list of user-defined contact list groups names.

"contact [list] groups' names"

+ * Return a list of contacts on a given user-defined contact list group
+ * named \a group.

Return the contacts in the user-defined contact [list] group named \a group

+ * Return a list of user-defined contact list groups the contact belongs.

Return the names of the user-defined contact [list] groups to which the contact belongs

Fixed upstream, it will be in next release 0.1.9