Bug 20033

Summary: Contact / ContactList Group integration
Product: Telepathy Reporter: Simon McVittie <smcv>
Component: tp-qtAssignee: Andre Moreira Magalhaes <andrunko>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: enhancement    
Priority: medium Keywords: patch
Version: unspecified   
Hardware: Other   
OS: All   
URL: http://git.collabora.co.uk/?p=user/andrunko/telepathy-qt4.git;a=shortlog;h=refs/heads/roster-groups
Whiteboard:
i915 platform: i915 features:
Bug Depends on: 20032    
Bug Blocks:    

Description Simon McVittie 2009-02-10 04:06:28 UTC
- Make Connection watch for NewChannel with channel type Group if the
  ConnectionFeature member FeatureGroups is enabled (or perhaps it's a
  ContactFeature?), and signal the results
  to its Contact objects
- Implement Contact::groups()
- Implement ContactManager::allGroups()
- Implement ContactManager::groupMembers()
Comment 1 Simon McVittie 2009-07-23 08:55:34 UTC
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.
Comment 2 Simon McVittie 2009-07-23 08:59:36 UTC
+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.
Comment 3 Simon McVittie 2009-07-23 09:19:03 UTC
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
Comment 4 Andre Moreira Magalhaes 2009-07-23 15:13:22 UTC
Fixed upstream, it will be in next release 0.1.9

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.