[Bug 28199] port Gabble to TpBaseContactList

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=28199

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|patch                       |

--- Comment #8 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-08-26 05:54:17 PDT ---
(In reply to comment #7)
> I've done a first review (patch by patch) of this. Keep in mind that I'm not a
> Gabble roster expert (I never really hacked on it) so I would have probably
> missed subtil issues in its logic.
> 
> _gabble_roster_item_free() should free item->publish_request

Fixed, and reviewed on IRC. I've merged the fixes approved so far into
contact-list and kept the rest in contact-list-fixes.

> http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=commitdiff;h=a07e273d4694040ab4ed287c97bb8a99fcf46b90
> I like the idea. Would be good to have somthing similar in glib. Maybe using
> another result object implementing the GAsyncResult interface so we wouldn't
> have to abuse the res pointer.

Yes, but I'd like to to defer this. I think landing ContactList before it
bit-rots any further is more important than GAsyncResult infrastructure :-)

> +  /* list of GSimpleAsyncResult */
> +  GSList *results;
> I'd say if the result objects are owned or borrowed

It's a bit weird: it's "borrowed from itself" (they ref themselves for as long
as they have a nonzero counter). I've changed that to an explicit reference,
and also fixed a leak of the GSList links themselves.

> http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=commitdiff;h=6e66629b4958990d197c15e03020f7c958f2fe25
> why not keep both variants and so test the old API as well as you did with
> other tests?
> Same for other tests

Done for roster/removed-from-rp-subscribe (which increases it to 8 sub-tests).
I don't think I deleted any other old-test variants.

roster/authorize.py is new and doesn't exercise the old API; I could test the
old API as well, I suppose.

> http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=commitdiff;h=a132acd51f6bf7d9c1cec5c801704f5305fdcf03
> if the change didn't stick, shouldn't conn.Contacts.GetContactAttributes()
> return the "people starting with A" group as well?

tbd

> I try to use "pyflakes" when adding new tests. For example pyflakes
> tests/twisted/roster/edit-before-roster.py returns a bunch of trivial warnings
> that could easily be fixed.
> Same for tests/twisted/roster/authorize.py

tbd

> Why not use your shiny tp_clear_pointer() ? :)

It didn't exist when I initially wrote this code in mid June; I did grep for
g_object_unref to add some calls to tp_clear_object, but good places for
tp_clear_pointer are harder to spot automatically. I've added some.

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