[Bug 20033] Contact / ContactList Group integration

[bugzilla-daemon at freedesktop.org]

http://bugs.freedesktop.org/show_bug.cgi?id=20033


Simon McVittie <simon.mcvittie at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|telepathy at lists.freedesktop.|andrunko at gmail.com
                   |org                         |
          QAContact|                            |telepathy-
                   |                            |bugs at lists.freedesktop.org




--- Comment #1 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2009-07-23 08:55:34 PST ---
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.


-- 
Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.