fdo bug #27792 is a blocker for this bug. Once it's fixed, I'll need to make some minor changes to libfolks for it to work against the mainline telepathy-glib Vala bindings (which are automatically generated based on gobject-introspection).

At that point, I'll make a release of libfolks and migrate it over to gnome.org. Then this Empathy branch will only depend upon released, mainline branches.

Created attachment 165408 [details] [review]
attaching a diff for easier review

Review of attachment 165408 [details] [review]:

I just reviewed the existing files for now. I'll review the new ones separately.

Does the branch pass "make check" and "make distcheck" ?

::: libempathy-gtk/empathy-contact-widget.c
@@ +583,3 @@
+          information->contact);
+
+      if (individual)

we use if (ptr != NULL) for non boolean types.

@@ +586,2 @@
         {
+          folks_groups_change_group (FOLKS_GROUPS (individual), group, !enabled);

If I understand folks correctly, the 3rd arg is "is_member", so why are you passing !enabled instead of enabled?

::: libempathy-gtk/empathy-ui-utils.c
@@ +230,3 @@
+	TpConnectionPresenceType presence;
+
+	folks_presence = folks_individual_get_presence_type (individual);

Any reason to redefine presence types instead of reusing the TP ones?

@@ +529,3 @@
+
+static void
+pixbuf_avatar_from_individual_closure_free (

Please add a _new function as well, it makes code clearer.

@@ +563,3 @@
+	loader = gdk_pixbuf_loader_new ();
+
+	/* XXX: this seems a bit racy, but apparently works well enough */

Why is it racy?

@@ +588,3 @@
+	g_free (data);
+	if (loader != NULL)
+		g_object_unref (loader);

You can use tp_clear_object()

@@ +593,3 @@
+
+void
+empathy_pixbuf_avatar_from_individual_scaled_async (FolksIndividual                     *individual,

this line is too long.
Also, wouldn't be cleaner to use the _async/_finish pattern?

@@ +602,3 @@
+	PixbufAvatarFromIndividualClosure *closure;
+
+	if (!FOLKS_IS_INDIVIDUAL (individual)) {

I'd use a g_return_if_fail, if we pass something invalid, we loose.

::: libempathy/empathy-utils.c
@@ +572,3 @@
+
+gboolean
+empathy_folks_individual_contains_contact (FolksIndividual *individual)

A small comment explaining the function would be good. This is true for all these utilty functions actually.

@@ +583,3 @@
+      TpfPersona *persona = l->data;
+
+      if (TPF_IS_PERSONA (persona))

Any reason why this would be FALSE?

@@ +587,3 @@
+          TpContact *contact = tpf_persona_get_contact (persona);
+
+          if (TP_IS_CONTACT (contact))

I don't understand this. Why this would be FALSE? Shouldn't it check for NULL either?

@@ +611,3 @@
+      TpfPersona *persona = l->data;
+
+      if (TPF_IS_PERSONA (persona))

same question here.

@@ +640,3 @@
+
+      if (c == contact)
+        individual = g_object_ref (i);

This function return a new ref while empathy_contact_from_folks_individual() doesn't. We should stay coherent.

::: src/empathy-main-window.c
@@ +222,3 @@
 
+	g_object_unref (individual);
+	if (contact)

!= NULL

@@ +356,3 @@
 	g_object_unref (contact);
+OUT:
+	if (individual)

same here

@@ +793,2 @@
 	/* Get string from index */
+		type = empathy_individual_store_sort_get_type ();

alignement is wrong.

Review of attachment 165408 [details] [review]:

empathy-individual-dialogs
=================

::: libempathy-gtk/empathy-individual-dialogs.c
@@ +1,3 @@
+/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */
+/*
+ * Copyright (C) 2007-2008 Collabora Ltd.

Copyright should be 2010

@@ +41,3 @@
+/*
+ *  New contact dialog
+ */

empathy-contact-dialog uses to do more things. What happens to this code?

@@ +97,3 @@
+  GtkWidget *contact_widget;
+
+  g_return_if_fail (individual == NULL || FOLKS_IS_INDIVIDUAL (individual));

shouldn't we test the opposite?

@@ +109,3 @@
+  gtk_dialog_set_has_separator (GTK_DIALOG (dialog), FALSE);
+  gtk_window_set_resizable (GTK_WINDOW (dialog), FALSE);
+  gtk_window_set_title (GTK_WINDOW (dialog), _("New Individual"));

Do we really want to expose the notion of "individual" to the user? That seems like a folks implementation detail to me.

@@ +125,3 @@
+
+  /* Contact info widget */
+  if (FOLKS_IS_INDIVIDUAL (individual))

Why this would be FALSE? We should check that at the beginning of the function.

Review of attachment 165408 [details] [review]:

empathy-individual-menu
================

::: libempathy-gtk/empathy-individual-menu.c
@@ +233,3 @@
+
+out:
+  if (contact != NULL)

you can use tp_clear_object

@@ +247,3 @@
+  contact = empathy_contact_from_folks_individual (individual);
+
+  g_return_if_fail (EMPATHY_IS_CONTACT (contact));

checking != NULL is enough.

@@ +472,3 @@
+}
+
+/* FIXME  we should check if the individual supports vnc stream tube */

We are doing that actually. This FIXME is obsolete.

Review of attachment 165408 [details] [review]:

empathy-individual-store
===============

::: libempathy-gtk/empathy-individual-store.c
@@ +130,3 @@
+      EMPATHY_INDIVIDUAL_STORE_COL_CAN_AUDIO_CALL,
+      folks_individual_get_capabilities (individual) &
+      FOLKS_CAPABILITIES_FLAGS_AUDIO,

Is that really something we want to expose in folks? Those capabilities were just helper we used in Empathy but I think we should be more generic in folks and expose capabilities as TpCapabilies.

@@ +316,3 @@
+
+  iters = individual_store_find_contact (self, individual);
+  if (!iters)

== NULL

@@ +364,3 @@
+  if (EMP_STR_EMPTY (folks_individual_get_alias (individual)) ||
+      (!priv->show_offline && !folks_individual_is_online (individual)))
+    {

I usually don't set { } for early return.

@@ +380,3 @@
+      connection);
+
+  if (!groups)

== NULL

@@ +570,3 @@
+  iters = individual_store_find_contact (self, individual);
+  if (!iters)
+    {

no need to put {} for such small block.

@@ +1211,3 @@
+  ret_val = g_utf8_collate (folks_individual_get_id (individual_a),
+      folks_individual_get_id (individual_b));
+

The old code was also sorting on the protocol and account ID.

@@ +1273,3 @@
+  g_free (name_b);
+
+  if (individual_a)

Use tp_clear_object

@@ +1791,3 @@
+  for (l = personas, contact_count = 0; l; l = l->next)
+    {
+      if (TPF_IS_PERSONA (l->data))

Why this would be FALSE?

@@ +1794,3 @@
+        contact_count++;
+
+      if (contact_count > 1)

Can't we use g_list_length directly?

@@ +1798,3 @@
+    }
+
+  show_protocols_here = priv->show_protocols && (contact_count == 1);

please add () around the expression.

Review of attachment 165408 [details] [review]:

empathy-individual-view
===============

::: libempathy-gtk/empathy-individual-view.c
@@ +159,3 @@
+  personas = folks_individual_get_personas (individual);
+  for (l = personas; l; l = l->next)
+    {

You should declare dup_str here to avoid freeing twice the same string.

@@ +240,3 @@
+  EmpathyIndividualViewPriv *priv = GET_PRIV (view);
+
+  if (priv->tooltip_widget != NULL)

use tp_clear_object

@@ +276,3 @@
+  /* Don't show the tooltip if there's already a popup menu */
+  if (gtk_menu_get_for_attach_widget (GTK_WIDGET (view)) != NULL)
+    {

I'd remove all these { }

Review of attachment 165408 [details] [review]:

empathy-individual-manager
==================

::: libempathy/empathy-individual-manager.c
@@ +61,3 @@
+    EmpathyIndividualManager *self)
+{
+  g_signal_emit_by_name (self, "groups-changed", individual, group,

Use g_signal_emit instead

@@ +81,3 @@
+{
+  GList *l;
+

Shouldn't we store/ref the individuals objects here? If not, the reference policy should be documented.

@@ +90,3 @@
+    }
+
+  /* TODO: don't hard-code the reason or message */

How can we fix that?

@@ +122,3 @@
+  EmpathyIndividualManagerPriv *priv = GET_PRIV (object);
+
+  if (priv->contact_manager != NULL)

this should be in _dispose.
Also, use tp_clear_object

@@ +297,3 @@
+  g_object_ref (contact);
+
+  DEBUG (G_STRLOC ": adding individual from contact %s (%s)",

Why G_STRLOC? DEBUG does it for us.

@@ +303,3 @@
+  store_id = tp_proxy_get_object_path (TP_PROXY (account));
+
+  details = g_hash_table_new (g_str_hash, g_str_equal);

How is define this hash exactly? Shouldn't it be a string => GValue for more flexibility?

@@ +330,3 @@
+  priv = GET_PRIV (self);
+
+  DEBUG (G_STRLOC ": removing individual %s (%s)",

G_STRLOC?

@@ +338,3 @@
+
+static void
+remove_group_cb (const gchar *id, FolksIndividual *individual,

one arg per line please

(In reply to comment #3)
> Review of attachment 165408 [details] [review]:
> 
> I just reviewed the existing files for now. I'll review the new ones
> separately.
> 
> Does the branch pass "make check" and "make distcheck" ?
> 
> ::: libempathy-gtk/empathy-contact-widget.c
> @@ +583,3 @@
> +          information->contact);
> +
> +      if (individual)
> 
> we use if (ptr != NULL) for non boolean types.
> 
> @@ +586,2 @@
>          {
> +          folks_groups_change_group (FOLKS_GROUPS (individual), group,
> !enabled);
> 
> If I understand folks correctly, the 3rd arg is "is_member", so why are you
> passing !enabled instead of enabled?
> 
> ::: libempathy-gtk/empathy-ui-utils.c
> @@ +230,3 @@
> +    TpConnectionPresenceType presence;
> +
> +    folks_presence = folks_individual_get_presence_type (individual);
> 
> Any reason to redefine presence types instead of reusing the TP ones?
> 
> @@ +529,3 @@
> +
> +static void
> +pixbuf_avatar_from_individual_closure_free (
> 
> Please add a _new function as well, it makes code clearer.
> 
> @@ +563,3 @@
> +    loader = gdk_pixbuf_loader_new ();
> +
> +    /* XXX: this seems a bit racy, but apparently works well enough */
> 
> Why is it racy?
> 
> @@ +588,3 @@
> +    g_free (data);
> +    if (loader != NULL)
> +        g_object_unref (loader);
> 
> You can use tp_clear_object()
> 
> @@ +593,3 @@
> +
> +void
> +empathy_pixbuf_avatar_from_individual_scaled_async (FolksIndividual           
>          *individual,
> 
> this line is too long.
> Also, wouldn't be cleaner to use the _async/_finish pattern?
> 
(In reply to comment #3)
> Review of attachment 165408 [details] [review]:
> 
> I just reviewed the existing files for now. I'll review the new ones
> separately.
> 
> Does the branch pass "make check" and "make distcheck" ?

Sorry, there were some trivial violations of the machine-checked coding style, but they're fixed now, and both run successfully.

> @@ +586,2 @@
>          {
> +          folks_groups_change_group (FOLKS_GROUPS (individual), group,
> !enabled);
> 
> If I understand folks correctly, the 3rd arg is "is_member", so why are you
> passing !enabled instead of enabled?

"enabled" is set to the previous state, and we're toggling it here. But that confused me at first too, so I've made a trivial commit to rename it "was_enabled".

> ::: libempathy-gtk/empathy-ui-utils.c
> @@ +230,3 @@
> +    TpConnectionPresenceType presence;
> +
> +    folks_presence = folks_individual_get_presence_type (individual);
> 
> Any reason to redefine presence types instead of reusing the TP ones?

libfolks doesn't technically depend upon Telepathy, so I didn't want to forward that symbol directly. So, eg, if libfolks only found an e-d-s backend available, folks_individual_get_presence_type () wouldn't need to awkwardly return TP_CONNECTION_PRESENCE_TYPE_UNSET.

> @@ +563,3 @@
> +    loader = gdk_pixbuf_loader_new ();
> +
> +    /* XXX: this seems a bit racy, but apparently works well enough */
> 
> Why is it racy?

Just glancing at it, there seems to be a race between the "size-prepared" signal handler and the call to avatar_pixbuf_from_loader(). But, of course, "size-prepared" must be emitted before gdk_pixbuf_loader_write() completes, so it shouldn't be a problem. I've cut the comment.

> @@ +583,3 @@
> +      TpfPersona *persona = l->data;
> +
> +      if (TPF_IS_PERSONA (persona))
> 
> Any reason why this would be FALSE?

Yes, if we have a non-Telepathy backend, Individuals can contain FolksPersonas that aren't TpfPersonas. (TpfPersona derives from FolksPersona and folks_individual_get_personas() returns a GList of FolksPersonas).

> @@ +587,3 @@
> +          TpContact *contact = tpf_persona_get_contact (persona);
> +
> +          if (TP_IS_CONTACT (contact))
> 
> I don't understand this. Why this would be FALSE? Shouldn't it check for NULL
> either?

This was a little excessive. I've simplified the entire block to just

      if (TPF_IS_PERSONA (l->data))
        return (tpf_persona_get_contact (TPF_PERSONA (l->data)) != NULL);

> @@ +640,3 @@
> +
> +      if (c == contact)
> +        individual = g_object_ref (i);
> 
> This function return a new ref while empathy_contact_from_folks_individual()
> doesn't. We should stay coherent.

They actually both reference the return value (empathy_contact_from_folks_individual() returns the value of empathy_contact_dup_from_tp_contact ()).

But there is an inconsistency in the way they didn't indicate by their names that the return value is already ref'd, so I've added "_dup" to them. This helped uncover a few places where I forgot to unref, as I updated the names.


I've fixed all the other points you've brought up (they're committed in the order you mentioned them in your original comment).

(In reply to comment #4)
> Review of attachment 165408 [details] [review]:
> 
> empathy-individual-dialogs
> =================
> 
> ::: libempathy-gtk/empathy-individual-dialogs.c
> @@ +1,3 @@
> +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */
> +/*
> + * Copyright (C) 2007-2008 Collabora Ltd.
> 
> Copyright should be 2010

Fixed.

> @@ +41,3 @@
> +/*
> + *  New contact dialog
> + */
> 
> empathy-contact-dialog uses to do more things. What happens to this code?

It's still there, and we still use it. I just created empathy-individual-dialogs.c as a separate file in the same way that I separated the Individual-based store and view. We'll certainly convert some of the contact-dialogs to individual-dialogs, though, eg, empathy_subscription_dialog_show() still makes sense to be on a contact-basis.

Maybe I should move the content of individual-dialogs into contact-dialogs, and convert the remaining functions in-place as necessary?

> @@ +97,3 @@
> +  GtkWidget *contact_widget;
> +
> +  g_return_if_fail (individual == NULL || FOLKS_IS_INDIVIDUAL (individual));
> 
> shouldn't we test the opposite?

No - just as with empathy_new_contact_dialog_show_with_contact(), it's OK to provide a NULL individual. In both cases, a NULL contact will be passed on to empathy_contact_widget_new(), which means it just doesn't get pre-populated.

> @@ +109,3 @@
> +  gtk_dialog_set_has_separator (GTK_DIALOG (dialog), FALSE);
> +  gtk_window_set_resizable (GTK_WINDOW (dialog), FALSE);
> +  gtk_window_set_title (GTK_WINDOW (dialog), _("New Individual"));
> 
> Do we really want to expose the notion of "individual" to the user? That seems
> like a folks implementation detail to me.

Not really. "Individual" is an awkward word in English. I'd certainly like some vocabulary to make a distinction between "contacts" and "meta-contacts" (which is also an awkward expression to include in a UI).

Personally, I'd prefer "contact" and "person". The only reason FolksIndividual isn't FolksPerson is because we've got a separate FolksPersona class (which is basically the equivalent of EmpathyContact).

Either way, we should decide on the vocabulary by the time we visibly support meta-contacts in the UI (after we merge this branch and start work on contact linking).

Until then, we'll just stick with "contact". Fixed.

> @@ +125,3 @@
> +
> +  /* Contact info widget */
> +  if (FOLKS_IS_INDIVIDUAL (individual))
> 
> Why this would be FALSE? We should check that at the beginning of the function.

It's a little bit of over-kill. I've changed it to "if (individual != NULL)". One of those checks needs to be there, so we don't try to derive its contact if a NULL individual was passed into the original call.

(In reply to comment #5)
> Review of attachment 165408 [details] [review]:
> 
> empathy-individual-menu
> ================

All fixed.

(In reply to comment #6)
> Review of attachment 165408 [details] [review]:
> 
> empathy-individual-store
> ===============
> 
> ::: libempathy-gtk/empathy-individual-store.c
> @@ +130,3 @@
> +      EMPATHY_INDIVIDUAL_STORE_COL_CAN_AUDIO_CALL,
> +      folks_individual_get_capabilities (individual) &
> +      FOLKS_CAPABILITIES_FLAGS_AUDIO,
> 
> Is that really something we want to expose in folks? Those capabilities were
> just helper we used in Empathy but I think we should be more generic in folks
> and expose capabilities as TpCapabilies.

I think this is the right abstraction to have in libfolks. The capabilities we have defined now probably only will be used for Individuals that contain at least one TpfPersona (TpContact). I can't really think of any non-Telepathy backends that would need these specific capabilities. So we could push this enum down to TpfPersona for now.

However, as soon as we do want to add more flags (eg, WRITABLE), we'll want an enum like this (which isn't limited to the Telepathy backend). Also, these capabilities apply to individuals, not just personas. So if we'd like to filter the list of individuals to only individuals who can video chat, then prompt the user to select a specific video-capable contact within a specific individual to start the video chat with, we'd need this high-level enum.

Eg, we'd want this:

List<Folks.Persona> Folks.Individual.get_capable_personas (Folks.CapabilitiesFlags.AUDIO);

not this:

List<Folks.Persona> Folks.Individual.get_capable_personas (Tpf.CapabilitiesFlags.AUDIO);

(Tpf, the telepathy-backend library depends upon Folks, not the other way around).

> @@ +570,3 @@
> +  iters = individual_store_find_contact (self, individual);
> +  if (!iters)
> +    {
> 
> no need to put {} for such small block.

Fixed this and others.

> @@ +1791,3 @@
> +  for (l = personas, contact_count = 0; l; l = l->next)
> +    {
> +      if (TPF_IS_PERSONA (l->data))
> 
> Why this would be FALSE?

Same explanation as in comment #9

> @@ +1794,3 @@
> +        contact_count++;
> +
> +      if (contact_count > 1)
> 
> Can't we use g_list_length directly?

No, because some members may not be TpfPersonas


All others fixed.

(In reply to comment #7)
> Review of attachment 165408 [details] [review]:
> 
> empathy-individual-view
> ===============
> 
> ::: libempathy-gtk/empathy-individual-view.c
> @@ +159,3 @@
> +  personas = folks_individual_get_personas (individual);
> +  for (l = personas; l; l = l->next)
> +    {
> 
> You should declare dup_str here to avoid freeing twice the same string.

Nice catch; I also moved in a couple other variables that weren't used outside of the loop.

> @@ +240,3 @@
> +  EmpathyIndividualViewPriv *priv = GET_PRIV (view);
> +
> +  if (priv->tooltip_widget != NULL)
> 
> use tp_clear_object

Fixed.

> @@ +276,3 @@
> +  /* Don't show the tooltip if there's already a popup menu */
> +  if (gtk_menu_get_for_attach_widget (GTK_WIDGET (view)) != NULL)
> +    {
> 
> I'd remove all these { }

Fixed in an earlier commit.

(In reply to comment #8)
> Review of attachment 165408 [details] [review]:
> 
> empathy-individual-manager
> ==================

> @@ +81,3 @@
> +{
> +  GList *l;
> +
> 
> Shouldn't we store/ref the individuals objects here? If not, the reference
> policy should be documented.

I've now documented this. It's basically the same as EmpathyContactManager - storage is left to the source (in this case, priv->aggregator), so the manager just relays signals and functions from the other client code.

> @@ +90,3 @@
> +    }
> +
> +  /* TODO: don't hard-code the reason or message */
> 
> How can we fix that?

I'll need to add the message and reason to the Folks API. I left it out because it seemed unimportant, but they're useful for IRC. I'll fix this in a later update.

> @@ +122,3 @@
> +  EmpathyIndividualManagerPriv *priv = GET_PRIV (object);
> +
> +  if (priv->contact_manager != NULL)
> 
> this should be in _dispose.
> Also, use tp_clear_object

Fixed both and also made it chain up to the parent class's dispose().

> @@ +303,3 @@
> +  store_id = tp_proxy_get_object_path (TP_PROXY (account));
> +
> +  details = g_hash_table_new (g_str_hash, g_str_equal);
> 
> How is define this hash exactly? Shouldn't it be a string => GValue for more
> flexibility?

Yeah, it's just string => string right now. I'll fix this in a later update.


All the others are fixed.

(In reply to comment #12)
> > ::: libempathy-gtk/empathy-individual-store.c
> > @@ +130,3 @@
> > +      EMPATHY_INDIVIDUAL_STORE_COL_CAN_AUDIO_CALL,
> > +      folks_individual_get_capabilities (individual) &
> > +      FOLKS_CAPABILITIES_FLAGS_AUDIO,
> > 
> > Is that really something we want to expose in folks? Those capabilities were
> > just helper we used in Empathy but I think we should be more generic in folks
> > and expose capabilities as TpCapabilies.
> 
> I think this is the right abstraction to have in libfolks. The capabilities we
> have defined now probably only will be used for Individuals that contain at
> least one TpfPersona (TpContact). I can't really think of any non-Telepathy
> backends that would need these specific capabilities. So we could push this
> enum down to TpfPersona for now.

Agreed, this kind of capabilities will probably only be used when there is a Telepathy backend involved.
Which is why I think we shouldn't over simplify them.

What are exactly these capabilities? Mixing audio/video (which is a capability describing an action that you can do with the contact) with WRITABLE (which is more an informatiion about the underlying backend of the individual) seems weird to me.

Also, what exactly mean having the audio or video capability? Empathy knows it because it's the one making the channel request to make the call. But if you expose it in your API you'll have to precisely document what that mean in term of channel request.

Furthermore, there is more things we'd like to be able to do when filtering individuals:
- contacts that I can invite to this room
- contacts with who I can establish a VNC stream tube
- contacts which can join a Muji call
...

(In reply to comment #15)
> > I think this is the right abstraction to have in libfolks. The capabilities we
> > have defined now probably only will be used for Individuals that contain at
> > least one TpfPersona (TpContact). I can't really think of any non-Telepathy
> > backends that would need these specific capabilities. So we could push this
> > enum down to TpfPersona for now.
> 
> Agreed, this kind of capabilities will probably only be used when there is a
> Telepathy backend involved.
> Which is why I think we shouldn't over simplify them.
> 
> What are exactly these capabilities? Mixing audio/video (which is a capability
> describing an action that you can do with the contact) with WRITABLE (which is
> more an informatiion about the underlying backend of the individual) seems
> weird to me.
> 
> Also, what exactly mean having the audio or video capability? Empathy knows it
> because it's the one making the channel request to make the call. But if you
> expose it in your API you'll have to precisely document what that mean in term
> of channel request.
> 
> Furthermore, there is more things we'd like to be able to do when filtering
> individuals:
> - contacts that I can invite to this room
> - contacts with who I can establish a VNC stream tube
> - contacts which can join a Muji call
> ...

So, we need to do some more thinking about the Capabilities interface in libfolks. I still think it makes sense to do at a high level (so Individuals implement the interface), but we'll at least be much more descriptive (eg, AUDIO, like we have now, isn't clear enough, as you said).

Till then, I've cut the Capabilities interface from libfolks and libfolks-telepathy, and pushed a change to this Empathy branch to just use the EmpathyContact capabilities directly.

(In reply to comment #14)
> > @@ +90,3 @@
> > +    }
> > +
> > +  /* TODO: don't hard-code the reason or message */
> > 
> > How can we fix that?
> 
> I'll need to add the message and reason to the Folks API. I left it out because
> it seemed unimportant, but they're useful for IRC. I'll fix this in a later
> update.

Fixed in a new commit.

(In reply to comment #14)
> > @@ +303,3 @@
> > +  store_id = tp_proxy_get_object_path (TP_PROXY (account));
> > +
> > +  details = g_hash_table_new (g_str_hash, g_str_equal);
> > 
> > How is define this hash exactly? Shouldn't it be a string => GValue for more
> > flexibility?
> 
> Yeah, it's just string => string right now. I'll fix this in a later update.

I've fixed this as well, so now it's an a{sv}.

This should fix the last of your original concerns.

Created attachment 165792 [details] [review]
new diff

Review of attachment 165792 [details] [review]:

Review of the new commits.

::: libempathy-gtk/empathy-ui-utils.c
@@ +628,3 @@
+	result = g_simple_async_result_new (G_OBJECT (individual),
+			callback, user_data,
+			empathy_pixbuf_avatar_from_individual_scaled_finish);

convention is to pass the _async function as source tag.

::: libempathy/empathy-individual-manager.c
@@ +315,3 @@
+  g_value_init (&value, G_TYPE_STRING);
+  g_value_set_string (&value, empathy_contact_get_id (contact));
+  g_hash_table_insert (details, "contact", &value);

tp_asv_new () is your friend :)

::: po/POTFILES.in
@@ +67,3 @@
 [type: gettext/glade]src/empathy-accounts-dialog.ui
 src/empathy-auto-salut-account-helper.c
+src/empathy-av.c

I've done that as well when releasing (sorry didn't realize than 'make check' was broken in master) so you may want to rebase and fix conflicts.
Actually I think you can just drop 559f8388598e6544fbbd99c447f35d112fb802fa and  573e408989eaf386c653a9e1799d96b7471034b0

In the future, please record those kind of fixes separately and get them merged to master right away.

Review of attachment 165792 [details] [review]:

::: configure.ac
@@ +144,3 @@
 [
+   folks >= $FOLKS_REQUIRED
+   folks-telepathy >= $FOLKS_REQUIRED

dependencies check has been simplified in master so this is going to conflict.

(In reply to comment #20)
> Review of attachment 165792 [details] [review]:

Fixed the suggestions.

> I've done that as well when releasing (sorry didn't realize than 'make check'
> was broken in master) so you may want to rebase and fix conflicts.
> Actually I think you can just drop 559f8388598e6544fbbd99c447f35d112fb802fa and
>  573e408989eaf386c653a9e1799d96b7471034b0
> 
> In the future, please record those kind of fixes separately and get them merged
> to master right away.

Yeah, sorry about that. I've removed those two commits in the new branch.

(In reply to comment #21)
> Review of attachment 165792 [details] [review]:
> 
> ::: configure.ac
> @@ +144,3 @@
>  [
> +   folks >= $FOLKS_REQUIRED
> +   folks-telepathy >= $FOLKS_REQUIRED
> 
> dependencies check has been simplified in master so this is going to conflict.

Fixed.

The newly-rebased branch folks-integ-rebase2 should be all caught-up with your review points:

http://git.collabora.co.uk/?p=user/treitter/empathy.git;a=shortlog;h=refs/heads/folks-integ-rebase2

I'm ok to merge it as soon as folks doesn't depend on unreleased things (but that's fine now right?) and we have a Debian package so people can continue to easily build Empathy.

Also, please open bugs for remaining issues (there are few FIXME is the code iirc).

I've finally managed to build this branch. Here are my observations after some testing:

** WARNING **: failed to ensure channel: Handle type not supported by this connection manager
** WARNING **: failed to ensure channel: 'stored' is not one of the valid handles

a warning is overkill, CM are not forced to implement those


Contacts with an unknown presences are now always displayed while there were only displayed when the "show offline contacts" was enabled before.


I got this crash (I tried to join a XMPP muc which failed, not sure if it's related)

** WARNING **: tpf-persona.vala:232: group invalidated: Channel was closed
aborting...

+ Trace 222870

Merged.