[Bug 28200] TpContactListManager (or perhaps TpBaseContactList)

[bugzilla-daemon at freedesktop.org]

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

--- Comment #4 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-05-31 07:42:26 PDT ---
(In reply to comment #2)
> Review to f56db201f69bc10768fc365128025a21157acf04 (included):
> 
> +  _tp_dynamic_handle_repo_set_normalization_data ((TpHandleRepoIface *) obj,
> +      NULL, NULL);
> Maybe add a comment saying that's to call the GDestroyNotify on the existing
> data?

That's not why I call it: the existing data is a borrowed cyclic ref to the
contact list manager, so that GROUP handles can be normalized by calling its
virtual method.

When the contact list manager becomes unusable (either when disposed, or when
the parent TpBaseConnection disconnects), that dangling pointer needs to be
released; the normalization function should no longer be called anyway, but if
it is, it'll fall back to just passing group names through as-is.

I've added brief comments.

(In this code I'm trying very hard to avoid the problems Gabble has, with
assumptions about objects' lifetimes all over the place.)

> there are 2 FIXME:
> +      /* FIXME: borrowed reference */

The channels assumed that they wouldn't survive longer than the connection and
their manager. This is in fact true, but I think it's better style to not have
these borrowed references. I've added logic to have circular references for the
lifetime of the channels, which are cleaned up when the channels are closed.

> I heard than passing NULL as hash_func and key_equal_func but is more
> optimized.

Fixed.

(In reply to comment #3)
> Didn't see anything obvioulsy wrong in the latest commits. The design and
> implementation is pretty clear; nice work!
> Did you already ported a CM to it?

No, I haven't done that yet. I should port at least one to prove the design
before we merge this; I should also demonstrate (on another branch) that it can
implement the planned new D-Bus API.

> +                    self->priv->group_repo, handle));
> +
> +            }
> 
> Useless \n

Fixed

> +    /* string borrowed from priv->all_tags => itself */
> I had to check what the "itself" actually meant (the self pointer or
> something?) but it seemed obvious once I read the code. :)

I changed those comments to read "the same pointer" instead of "itself". Is
that better?

(In reply to comment #1)
> * It should probably be called TpBaseContactList

I plan to make this change, but I haven't yet, because it's a huge-but-trivial
change; I'll go through and do this when you're otherwise happy with it.

> * Everything that takes a GStrv or a const gchar * const * should be using the
> array + length pattern, but with a signed (gssize) length, with -1 meaning
> NULL-terminated - this is "the best of both worlds"

Done; this produces more natural-looking code in the example CM, so I think
it's worth the extra code in the library.

> * Some of the handle sets would probably be nicer as int sets

I don't think this is worth it. The major awkwardness of handle sets is that
you have to use tp_handle_set_peek() to iterate over them, which is no big deal
really.

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