After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 623714 - Rebase Empathy upon libfolks
Rebase Empathy upon libfolks
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: Contact List
2.29.x
Other Linux
: Normal enhancement
: ---
Assigned To: empathy-maint
Depends on:
Blocks: 460647
 
 
Reported: 2010-07-06 22:42 UTC by Travis Reitter
Modified: 2011-08-29 10:12 UTC
See Also:
GNOME target: ---
GNOME version: 2.29/2.30


Attachments
attaching a diff for easier review (201.49 KB, patch)
2010-07-07 08:56 UTC, Guillaume Desmottes
reviewed Details | Review
new diff (208.47 KB, patch)
2010-07-13 11:47 UTC, Guillaume Desmottes
none Details | Review

Description Travis Reitter 2010-07-06 22:42:41 UTC
As a first step toward real metacontacts in Empathy, we should rebase it upon libfolks.

I've created a branch that aims to be feature-identical to mainline Empathy:

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

This branch is a few months in the making, so I've rebased it upon the latest mainline. So, it should hopefully be easy to review (though there may be some artifacts from the rebasing). We'll add new libfolks-based features (such as contact linking) after this branch is merged to mainline.

In order to build this branch, you will need libfolks:

http://git.collabora.co.uk/?p=user/treitter/folks.git;a=summary

Libfolks currently depends upon this special branch of telepathy-glib:

http://git.collabora.co.uk/?p=user/treitter/telepathy-glib.git;a=shortlog;h=refs/heads/vala-bindings-rebase1

However, this last requirement will go away as soon as fdo bug #27792 is fixed (which I hope to finish this week).

Known bugs with the Empathy branch:

* a new contact added by the Add Contact dialog shows up by its UID instead of alias. Opening and closing the Edit Information dialog for that contact or restarting Empathy restores the proper alias
  * similarly for a contact added by the "Subscribe request" dialog

* a new contact added by the Add Contact dialog is not (visibly) added to the groups selected in the dialog. Again, this can be worked around by re-adding the contact in the Edit Information dialog or restarting Empathy.
  * similarly for a contact added by the "Subscribe request" dialog
  
These bugs are all symptoms of not immediately matching up incoming TpfPersonas with EmpathyContacts, but we should be able to add a short-term work-around for this. In the long term, FolksIndividual/TpfPersona make EmpathyContacts (and a handful of other classes) unncessary.
Comment 1 Travis Reitter 2010-07-06 23:02:35 UTC
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.
Comment 2 Guillaume Desmottes 2010-07-07 08:56:02 UTC
Created attachment 165408 [details] [review]
attaching a diff for easier review
Comment 3 Guillaume Desmottes 2010-07-07 09:47:35 UTC
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.
Comment 4 Guillaume Desmottes 2010-07-07 10:20:54 UTC
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.
Comment 5 Guillaume Desmottes 2010-07-07 10:33:50 UTC
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.
Comment 6 Guillaume Desmottes 2010-07-07 11:57:31 UTC
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.
Comment 7 Guillaume Desmottes 2010-07-07 12:48:34 UTC
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 { }
Comment 8 Guillaume Desmottes 2010-07-08 07:18:36 UTC
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
Comment 9 Travis Reitter 2010-07-08 19:00:00 UTC
(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).
Comment 10 Travis Reitter 2010-07-09 20:37:42 UTC
(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.
Comment 11 Travis Reitter 2010-07-09 20:56:31 UTC
(In reply to comment #5)
> Review of attachment 165408 [details] [review]:
> 
> empathy-individual-menu
> ================

All fixed.
Comment 12 Travis Reitter 2010-07-10 00:09:37 UTC
(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.
Comment 13 Travis Reitter 2010-07-10 00:43:36 UTC
(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.
Comment 14 Travis Reitter 2010-07-10 01:29:21 UTC
(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.
Comment 15 Guillaume Desmottes 2010-07-12 09:07:09 UTC
(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
...
Comment 16 Travis Reitter 2010-07-12 18:17:53 UTC
(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.
Comment 17 Travis Reitter 2010-07-13 00:13:21 UTC
(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.
Comment 18 Travis Reitter 2010-07-13 06:33:49 UTC
(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.
Comment 19 Guillaume Desmottes 2010-07-13 11:47:24 UTC
Created attachment 165792 [details] [review]
new diff
Comment 20 Guillaume Desmottes 2010-07-13 12:01:25 UTC
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.
Comment 21 Guillaume Desmottes 2010-07-13 12:21:38 UTC
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.
Comment 22 Travis Reitter 2010-07-14 18:00:08 UTC
(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
Comment 23 Guillaume Desmottes 2010-07-15 08:15:53 UTC
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).
Comment 24 Guillaume Desmottes 2010-07-16 10:09:02 UTC
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...

  • #0 g_logv
    at gmessages.c line 544
  • #1 g_log
    at gmessages.c line 568
  • #2 _lambda17_
    at tpf-persona.c line 265
  • #3 __lambda17__tpf_persona_store_group_removed
    at tpf-persona.c line 272
  • #4 g_closure_invoke
    at gclosure.c line 766
  • #5 signal_emit_unlocked_R
  • #6 g_signal_emit_valist
    at gsignal.c line 2983
  • #7 g_signal_emit_by_name
    at gsignal.c line 3077
  • #8 tpf_persona_store_channel_invalidated_cb
    at tpf-persona-store.c line 1236
  • #9 _tpf_persona_store_channel_invalidated_cb_tp_proxy_invalidated
    at tpf-persona-store.c line 1056
  • #10 g_closure_invoke
    at gclosure.c line 766
  • #11 signal_emit_unlocked_R
    at gsignal.c line 3252
  • #12 g_signal_emit_valist
    at gsignal.c line 2983
  • #13 g_signal_emit
    at gsignal.c line 3040
  • #14 tp_proxy_emit_invalidated
    at proxy.c line 491
  • #15 tp_channel_closed_cb
    at channel.c line 1148
  • #16 _tp_cli_channel_invoke_callback_for_closed
    at _gen/tp-cli-channel-body.h line 27
  • #17 tp_proxy_signal_invocation_run
    at proxy-signals.c line 266
  • #18 g_idle_dispatch
    at gmain.c line 4148
  • #19 g_main_dispatch
    at gmain.c line 2043
  • #20 g_main_context_dispatch
    at gmain.c line 2596
  • #21 g_main_context_iterate
    at gmain.c line 2674
  • #22 g_main_loop_run
    at gmain.c line 2882
  • #23 IA__gtk_main
    at gtkmain.c line 1237
  • #24 main
    at empathy.c line 600

Comment 25 Travis Reitter 2010-07-21 15:01:39 UTC
Merged.