Bug 29077

Summary:	refactor the roster, moving logic out of the channels into the roster itself
Product:	Telepathy	Reporter:	Simon McVittie <smcv>
Component:	haze	Assignee:	Simon McVittie <smcv>
Status:	RESOLVED FIXED	QA Contact:	Telepathy bugs list <telepathy-bugs>
Severity:	normal
Priority:	medium	Keywords:	patch
Version:	git master
Hardware:	Other
OS:	All
URL:	http://git.collabora.co.uk/?p=user/smcv/haze-smcv.git;a=shortlog;h=refs/heads/contact-list
Whiteboard:	review+ with trivia
i915 platform:		i915 features:
Bug Depends on:	29076
Bug Blocks:

Description Simon McVittie 2010-07-15 08:25:24 UTC

Haze's contact lists, like Gabble's, have some logic in the channel itself. All of this logic needs to move to the channel manager so it can become a TpBaseContactList.

This branch also adds tests for the contact lists' current behaviour, with good coverage.

http://git.collabora.co.uk/?p=user/smcv/haze-smcv.git;a=shortlog;h=refs/heads/contact-list

Comment 1 Will Thompson 2010-09-22 06:42:36 UTC

Test deleting contact lists via Close()

Is this change merged back to the corresponding test in Gabble?

+    bus.get_object(conn.bus_name, '/').Ping()

sync_dbus?

haze_contact_list_add_to_group() uses:

+    PurpleGroup *group = purple_group_new (group_name);

which is actually kosher, because _new() returns an existing group if possible. It threw me a bit, though. Guess I've been away too long. Any chance of a comment explaining that this is correct?

Otherwise looks fine.

Comment 2 Simon McVittie 2010-09-22 08:16:10 UTC

(In reply to comment #1)
> Test deleting contact lists via Close()
> 
> Is this change merged back to the corresponding test in Gabble?

There's no directly corresponding test, so, no; libpurple's contact list handling differs sufficiently that the tests would have to differ anyway. My TpBaseContactList branch for Gabble does add a test for deleting non-empty groups.

> +    bus.get_object(conn.bus_name, '/').Ping()
> 
> sync_dbus?

Obvious change applied.

> haze_contact_list_add_to_group() uses:
> 
> +    PurpleGroup *group = purple_group_new (group_name);
> 
> which is actually kosher, because _new() returns an existing group if possible.
> It threw me a bit, though. Guess I've been away too long. Any chance of a
> comment explaining that this is correct?

    /* This is correct, despite the naming: it returns a borrowed reference
     * to an existing group (if possible) or to a new group (otherwise). */
    PurpleGroup *group = purple_group_new (group_name);

Comment 3 Will Thompson 2010-09-22 08:59:41 UTC

looks fine, ship it.

Comment 4 Simon McVittie 2010-09-22 09:53:00 UTC

Fixed in git for 0.5.0

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.