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 626130 - Add linking UI for Individuals
Add linking UI for Individuals
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: Meta Contacts
2.31.x
Other Linux
: Normal normal
: ---
Assigned To: empathy-maint
Depends on: 626441
Blocks: 460647 626544
 
 
Reported: 2010-08-05 17:10 UTC by Philip Withnall
Modified: 2010-08-11 16:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Only enable row reordering in EmpathyIndividualView if dragging is enabled (1.85 KB, patch)
2010-08-05 17:12 UTC, Philip Withnall
none Details | Review
Add EmpathyPersonaStore and EmpathyPersonaView (58.22 KB, patch)
2010-08-05 17:12 UTC, Philip Withnall
reviewed Details | Review
Add EmpathyIndividualWidget (15.10 KB, patch)
2010-08-09 17:36 UTC, Philip Withnall
reviewed Details | Review
Add EmpathyIndividualLinker (23.59 KB, patch)
2010-08-09 17:36 UTC, Philip Withnall
reviewed Details | Review
Allow linking personas through EmpathyIndividualManager (2.26 KB, patch)
2010-08-09 17:37 UTC, Philip Withnall
reviewed Details | Review
Add EmpathyLinkingDialog (10.68 KB, patch)
2010-08-09 17:37 UTC, Philip Withnall
reviewed Details | Review
Squashed diff of the persona-store-rebase1 branch (updated) (59.95 KB, patch)
2010-08-10 10:42 UTC, Philip Withnall
committed Details | Review
Squashed diff of the linking-dialogue-rebase1 branch (updated) (51.91 KB, patch)
2010-08-10 10:43 UTC, Philip Withnall
committed Details | Review

Description Philip Withnall 2010-08-05 17:10:23 UTC
We're aiming for something similar to this mockup, although a few things have changed since then:

http://people.collabora.co.uk/~treitter/empathy-mockups/Empathy-contact-linking-4.png

This first branch lays some of the groundwork. The first commit fixes EmpathyIndividualView to only enable dragging if the DRAG feature is enabled. The second adds EmpathyPersonaStore and EmpathyPersonaView classes, which allow the Personas inside a given Individual to be displayed.

http://git.collabora.co.uk/?p=user/pwith/empathy;a=shortlog;h=refs/heads/persona-store
Comment 1 Philip Withnall 2010-08-05 17:12:10 UTC
Created attachment 167202 [details] [review]
Only enable row reordering in EmpathyIndividualView if dragging is enabled
Comment 2 Philip Withnall 2010-08-05 17:12:35 UTC
Created attachment 167203 [details] [review]
Add EmpathyPersonaStore and EmpathyPersonaView
Comment 3 Guillaume Desmottes 2010-08-09 14:03:05 UTC
Review of attachment 167203 [details] [review]:

Does the branch pass "make check" ?

Seems there are lot of code duplication in the store. Can't we share some? Maybe note by subclassing but at least with an helper file?

::: libempathy-gtk/empathy-persona-store.c
@@ +56,3 @@
+/* Time in seconds after connecting which we wait before active users are
+ * enabled */
+#define ACTIVE_USER_WAIT_TO_ENABLE_TIME 5

Do we really need this feature here? I guess we don't care for the linking UI.

@@ +63,3 @@
+{
+  FolksIndividual *individual;
+  GHashTable *personas; /* Persona -> GtkTreeRowReference */

Please document if those are owned or borrowed.

@@ +137,3 @@
+          "The FolksIndividual whose Personas should be listed by the store.",
+          FOLKS_TYPE_INDIVIDUAL,
+          G_PARAM_READWRITE));

We use to add "G_PARAM_READWRITE" in new code as well.

@@ +457,3 @@
+add_persona_and_connect (EmpathyPersonaStore *self, FolksPersona *persona)
+{
+  /* We don't want any non-Telepathy personas */

rly? How can I merge a, say, evolution persona with a TP one then?
Comment 4 Philip Withnall 2010-08-09 15:16:01 UTC
I've addressed most of these comments locally, but I'll wait until the other issues are resolved before pushing an updated branch.

(In reply to comment #3)
> Review of attachment 167203 [details] [review]:
> 
> Does the branch pass "make check" ?

I had to make one style fix. :-(

> Seems there are lot of code duplication in the store. Can't we share some?
> Maybe note by subclassing but at least with an helper file?

I can't see much code which could be shared. Perhaps the active user code could be shared, but if we're removing that, it obviously can't.

There's also the possibility that both the store and view could be moved to a libfolks-gtk sometime, though I don't know when.

> ::: libempathy-gtk/empathy-persona-store.c
> @@ +56,3 @@
> +/* Time in seconds after connecting which we wait before active users are
> + * enabled */
> +#define ACTIVE_USER_WAIT_TO_ENABLE_TIME 5
> 
> Do we really need this feature here? I guess we don't care for the linking UI.

I kept the code so that it could be used for highlighting personas which have been added to the Individual, but forgot to get round to actually using it. Does that sound like a good idea, or should I just rip the code out?

> @@ +63,3 @@
> +{
> +  FolksIndividual *individual;
> +  GHashTable *personas; /* Persona -> GtkTreeRowReference */
> 
> Please document if those are owned or borrowed.

Done.

> @@ +137,3 @@
> +          "The FolksIndividual whose Personas should be listed by the store.",
> +          FOLKS_TYPE_INDIVIDUAL,
> +          G_PARAM_READWRITE));
> 
> We use to add "G_PARAM_READWRITE" in new code as well.

*G_PARAM_STATIC_STRINGS. Done.

> @@ +457,3 @@
> +add_persona_and_connect (EmpathyPersonaStore *self, FolksPersona *persona)
> +{
> +  /* We don't want any non-Telepathy personas */
> 
> rly? How can I merge a, say, evolution persona with a TP one then?

You'd use the list of Personas from the FolksIndividual which is having its personas listed by the EmpathyPersonaStore. The EmpathyPersonaStore should only list the Personas which Empathy can understand which are, at the moment, just TpfPersonas.
Comment 5 Philip Withnall 2010-08-09 17:35:16 UTC
This branch is based on the persona-store branch, but each commit is fairly self-contained, so can be reviewed in parallel with the persona-store branch: http://git.collabora.co.uk/?p=user/pwith/empathy;a=shortlog;h=refs/heads/linking-dialogue
Comment 6 Philip Withnall 2010-08-09 17:36:09 UTC
Created attachment 167439 [details] [review]
Add EmpathyIndividualWidget
Comment 7 Philip Withnall 2010-08-09 17:36:43 UTC
Created attachment 167440 [details] [review]
Add EmpathyIndividualLinker
Comment 8 Philip Withnall 2010-08-09 17:37:09 UTC
Created attachment 167441 [details] [review]
Allow linking personas through EmpathyIndividualManager
Comment 9 Philip Withnall 2010-08-09 17:37:29 UTC
Created attachment 167442 [details] [review]
Add EmpathyLinkingDialog
Comment 10 Guillaume Desmottes 2010-08-10 07:43:33 UTC
(In reply to comment #4)
> There's also the possibility that both the store and view could be moved to a
> libfolks-gtk sometime, though I don't know when.

Right, that should probably move at some point.

> > ::: libempathy-gtk/empathy-persona-store.c
> > @@ +56,3 @@
> > +/* Time in seconds after connecting which we wait before active users are
> > + * enabled */
> > +#define ACTIVE_USER_WAIT_TO_ENABLE_TIME 5
> > 
> > Do we really need this feature here? I guess we don't care for the linking UI.
> 
> I kept the code so that it could be used for highlighting personas which have
> been added to the Individual, but forgot to get round to actually using it.
> Does that sound like a good idea, or should I just rip the code out?

Maybe yeah. Keep it for now, we'll see how it goes once it's used.
Comment 11 Guillaume Desmottes 2010-08-10 07:49:26 UTC
Review of attachment 167439 [details] [review]:

::: libempathy-gtk/empathy-individual-widget.c
@@ +290,3 @@
+
+const gchar *
+empathy_individual_widget_get_alias (EmpathyIndividualWidget *self)

When is this used? Shouldn't we iter over persona and return the first alias which is different from the ID.
Why don't we have a similar function for, say, avatar?
Comment 12 Guillaume Desmottes 2010-08-10 08:13:22 UTC
Review of attachment 167440 [details] [review]:

::: libempathy-gtk/empathy-individual-linker.c
@@ +67,3 @@
+  FolksIndividual *start_individual;
+  FolksIndividual *new_individual;
+  GHashTable *changed_individuals; /* Individual -> bool */

Document if the object is owned or borowed. That's good practice when you are storing objects in any data structure.
Also document the semantic of the hash table.

@@ +130,3 @@
+  self->priv = priv;
+
+  priv->changed_individuals = g_hash_table_new (g_direct_hash, g_direct_equal);

you can pass NULL, NULL as a small optimisation (see g_hash_table_new's doc).

@@ +136,3 @@
+
+static void
+get_property (GObject *object, guint param_id, GValue *value, GParamSpec *pspec)

one line per arg.

@@ +154,3 @@
+
+static void
+set_property (GObject *object, guint param_id, const GValue *value,

here too

@@ +202,3 @@
+  GtkWidget *child;
+
+  requisition->width = gtk_container_get_border_width (GTK_CONTAINER (widget)) * 2;

those lines are > 80 chars

@@ +246,3 @@
+static void
+contact_toggle_cell_data_func (GtkTreeViewColumn *tree_column,
+    GtkCellRenderer *cell, GtkTreeModel *tree_model, GtkTreeIter *iter,

one line per arg (check all the functions of this file as most of them are wrong in this regard).

@@ +343,3 @@
+      priv->changed_individuals, individual));
+
+  if (individual_added == TRUE)

no need to check == TRUE

::: libempathy-gtk/empathy-individual-linker.h
@@ +57,3 @@
+GtkWidget *empathy_individual_linker_new (FolksIndividual *start_individual);
+
+FolksIndividual *empathy_individual_linker_get_start_individual (

FolksIndividual * empathy_individual_linker_get_start_individual

@@ +60,3 @@
+    EmpathyIndividualLinker *self);
+void empathy_individual_linker_set_start_individual (
+    EmpathyIndividualLinker *self, FolksIndividual *individual);

one line per arg.

@@ +62,3 @@
+    EmpathyIndividualLinker *self, FolksIndividual *individual);
+
+GList *empathy_individual_linker_get_linked_personas (

GList * empathy_...
Comment 13 Guillaume Desmottes 2010-08-10 08:15:16 UTC
Review of attachment 167441 [details] [review]:

::: libempathy/empathy-individual-manager.c
@@ +546,3 @@
+      &error);
+
+  if (error != NULL)

The usual pattern is:

if (!badger_finish (..., &error))
{
display error;
free error;
}
Comment 14 Guillaume Desmottes 2010-08-10 08:22:27 UTC
Review of attachment 167442 [details] [review]:

::: libempathy-gtk/empathy-linking-dialog.c
@@ +67,3 @@
+
+  /* Reset and present the dialogue if it already exists */
+  if (linking_dialog)

We tend to use "if (ptr != NULL)" rather than "if (ptr)"

@@ +101,3 @@
+
+  /* Linker widget */
+  linker = empathy_individual_linker_new (individual);

Isn't that ref leaked? You should use g_object_set_data_full()

::: libempathy-gtk/empathy-linking-dialog.h
@@ +27,3 @@
+
+G_BEGIN_DECLS
+

I think we should have a EmpathyLinkingDialog object inheriting from GtkDialog.
Then API would be something like:

GtkWidget * empathy_linking_dialog_show (FolksIndividual *, GtkWindow *parent);
Comment 15 Philip Withnall 2010-08-10 10:38:58 UTC
Updated branches: http://git.collabora.co.uk/?p=user/pwith/empathy;a=shortlog;h=refs/heads/persona-store-rebase1
http://git.collabora.co.uk/?p=user/pwith/empathy;a=shortlog;h=refs/heads/linking-dialogue-rebase1

(In reply to comment #11)
> Review of attachment 167439 [details] [review]:
> 
> ::: libempathy-gtk/empathy-individual-widget.c
> @@ +290,3 @@
> +
> +const gchar *
> +empathy_individual_widget_get_alias (EmpathyIndividualWidget *self)
> 
> When is this used? Shouldn't we iter over persona and return the first alias
> which is different from the ID.
> Why don't we have a similar function for, say, avatar?

It wasn't used, it was a leftover from the EmpathyContactWidget code. I've removed it, along with some other unused code, since when API like that is needed in EmpathyIndividualWidget, it'll be sufficiently different to just be written from scratch.

Code to "iter over persona and return the first alias" belongs (and is) in libfolks.

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

All done.

(In reply to comment #13)
> Review of attachment 167441 [details] [review]:
> 
> ::: libempathy/empathy-individual-manager.c
> @@ +546,3 @@
> +      &error);
> +
> +  if (error != NULL)
> 
> The usual pattern is:
> 
> if (!badger_finish (..., &error))
> {
> display error;
> free error;
> }

folks_individual_aggregator_link_personas_finish() doesn't return a success boolean, since it's autogenerated by Vala, and Vala believes that such booleans are extraneous; just use the GError (as we have done) instead.

(In reply to comment #14)
> Review of attachment 167442 [details] [review]:
> 
> ::: libempathy-gtk/empathy-linking-dialog.c
> @@ +67,3 @@
> +
> +  /* Reset and present the dialogue if it already exists */
> +  if (linking_dialog)
> 
> We tend to use "if (ptr != NULL)" rather than "if (ptr)"

Done.

> @@ +101,3 @@
> +
> +  /* Linker widget */
> +  linker = empathy_individual_linker_new (individual);
> 
> Isn't that ref leaked? You should use g_object_set_data_full()

No, since it was a child widget of the dialogue, so it would be destroyed when the dialogue was. I've followed your suggestion below and reimplemented EmpathyLinkingDialog as a GtkDialog subclass, though, so this doesn't matter now.

> ::: libempathy-gtk/empathy-linking-dialog.h
> @@ +27,3 @@
> +
> +G_BEGIN_DECLS
> +
> 
> I think we should have a EmpathyLinkingDialog object inheriting from GtkDialog.
> Then API would be something like:
> 
> GtkWidget * empathy_linking_dialog_show (FolksIndividual *, GtkWindow *parent);

Done.
Comment 16 Philip Withnall 2010-08-10 10:42:16 UTC
Created attachment 167493 [details] [review]
Squashed diff of the persona-store-rebase1 branch (updated)
Comment 17 Philip Withnall 2010-08-10 10:43:55 UTC
Created attachment 167494 [details] [review]
Squashed diff of the linking-dialogue-rebase1 branch (updated)
Comment 18 Philip Withnall 2010-08-10 16:13:31 UTC
I just realised that the "Link" menu entry should actually be "Link…". I'll change this in the next branch iteration/when it's committed.
Comment 19 Guillaume Desmottes 2010-08-11 09:17:06 UTC
Review of attachment 167493 [details] [review]:

Feel free to merge this one once you have fixed the style issue (assuming it doesn't depend on unreleased folks changes).

::: libempathy-gtk/empathy-persona-store.c
@@ +104,3 @@
+    GtkTreePath *path, GtkTreeIter *iter, EmpathyPersonaStore *self);
+static GdkPixbuf *get_persona_status_icon (EmpathyPersonaStore *self,
+    FolksPersona *persona);

I'm sure we could re-order most of these functions to avoid all those prototypes (don't mind changing it that now but that's good to keep in mind when writing new code).

@@ +237,3 @@
+
+static void
+get_property (GObject *object, guint param_id, GValue *value, GParamSpec *pspec)

one arg per line (most of the functions in this file are wrong).

::: libempathy-gtk/empathy-persona-view.c
@@ +82,3 @@
+
+static gboolean
+filter_visible_func (GtkTreeModel *model, GtkTreeIter *iter,

Same comment regarding style.
Comment 20 Guillaume Desmottes 2010-08-11 10:15:29 UTC
Review of attachment 167494 [details] [review]:

::: libempathy-gtk/empathy-individual-linker.c
@@ +73,3 @@
+  /* Stores the Individuals whose Personas have been added to the
+   * new_individual */
+  GHashTable *changed_individuals; /* unowned Individual -> bool */

I'd prefer "Borrowed from badger" assuming badger is the one owning the ref. (makes debugging easier when tracking nasty ref bugs).

@@ +83,3 @@
+    GParamSpec *pspec);
+static void size_request (GtkWidget *widget, GtkRequisition *requisition);
+static void size_allocate (GtkWidget *widget, GtkAllocation *allocation);

Why not move the _class_init below and so get rid of these prototypes?

::: libempathy-gtk/empathy-individual-widget.c
@@ +74,3 @@
+    const GValue *value,
+    GParamSpec *pspec);
+static void dispose (GObject *object);

Same comment regarding _class_init

::: libempathy/empathy-individual-manager.c
@@ +566,3 @@
+  DEBUG ("Linking %u personas", g_list_length (personas));
+
+  folks_individual_aggregator_link_personas (priv->aggregator, personas,

This will requier a folks release and bump the dep in configure.ac (just a reminder so we don't forget when merging the branch).
Comment 21 Philip Withnall 2010-08-11 16:41:33 UTC
commit 99c59741441d9b10df00b2c63f5a119de9cfc5f2
Author: Philip Withnall <philip.withnall@collabora.co.uk>
Date:   Wed Aug 11 17:39:54 2010 +0100

    Fix linking menu entry mnemonic
    
    Closes: bgo#626130

 libempathy-gtk/empathy-individual-menu.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

commit dd8b8fb6b503194aecec14b4f5f9186c253a46c6
Author: Philip Withnall <philip.withnall@collabora.co.uk>
Date:   Thu Aug 5 17:28:16 2010 +0100

    Add EmpathyLinkingDialog
    
    A dialogue which uses EmpathyIndividualLinker to allow linking of Individuals,
    accessible by a "Link" entry in the contacts' context menu.

 libempathy-gtk/Makefile.am               |    2 +
 libempathy-gtk/empathy-individual-menu.c |   39 +++++++-
 libempathy-gtk/empathy-individual-menu.h |    4 +-
 libempathy-gtk/empathy-linking-dialog.c  |  175 ++++++++++++++++++++++++++++++
 libempathy-gtk/empathy-linking-dialog.h  |   61 +++++++++++
 po/POTFILES.in                           |    1 +
 6 files changed, 280 insertions(+), 2 deletions(-)

commit 9d23b85fc16e7bc4b83af2850fdd2e96909b09d9
Author: Philip Withnall <philip.withnall@collabora.co.uk>
Date:   Mon Aug 9 11:25:20 2010 +0100

    Allow linking personas through EmpathyIndividualManager
    
    Wrap the FolksIndividualAggregator persona linking API in
    EmpathyIndividualManager with some basic error reporting (it isn't expected
    that linking will fail).

 libempathy/empathy-individual-manager.c |   34 +++++++++++++++++++++++++++++++
 libempathy/empathy-individual-manager.h |    3 ++
 2 files changed, 37 insertions(+), 0 deletions(-)

commit 4b7d482ea72ad143c4ed2c7b778581ed669723e3
Author: Philip Withnall <philip.withnall@collabora.co.uk>
Date:   Thu Aug 5 17:27:24 2010 +0100

    Add EmpathyIndividualLinker
    
    This is a widget to allow selection of Individuals to link together to form
    linked Individuals.

 libempathy-gtk/Makefile.am                 |    2 +
 libempathy-gtk/empathy-individual-linker.c |  609 ++++++++++++++++++++++++++++
 libempathy-gtk/empathy-individual-linker.h |   70 ++++
 po/POTFILES.in                             |    1 +
 4 files changed, 682 insertions(+), 0 deletions(-)

commit 62318cf781b2d94dc8bc4d5f084e12e6a269f354
Author: Philip Withnall <philip.withnall@collabora.co.uk>
Date:   Thu Aug 5 17:27:44 2010 +0100

    Add EmpathyIndividualWidget
    
    This displays details for a single Individual, in much the same way that
    EmpathyContactWidget displays the details of a single Persona.

 libempathy-gtk/Makefile.am                 |    2 +
 libempathy-gtk/empathy-individual-widget.c |  257 ++++++++++++++++++++++++++++
 libempathy-gtk/empathy-individual-widget.h |  106 ++++++++++++
 3 files changed, 365 insertions(+), 0 deletions(-)

commit 9acc3aabf75d159a0c7b6bb729ccfd9d8daff865
Author: Philip Withnall <philip.withnall@collabora.co.uk>
Date:   Wed Aug 4 16:49:11 2010 +0100

    Add EmpathyPersonaStore and EmpathyPersonaView
    
    Based on stripped-down versions of EmpathyContactListStore and
    EmpathyContactListView, these allow listing of all the Personas for a given
    Individual.

 libempathy-gtk/Makefile.am             |    4 +
 libempathy-gtk/empathy-persona-store.c | 1168 ++++++++++++++++++++++++++++++++
 libempathy-gtk/empathy-persona-store.h |  107 +++
 libempathy-gtk/empathy-persona-view.c  |  574 ++++++++++++++++
 libempathy-gtk/empathy-persona-view.h  |   74 ++
 5 files changed, 1927 insertions(+), 0 deletions(-)

commit 7322c5352d6b719da9f632c68fa48ae1ea81977f
Author: Philip Withnall <philip.withnall@collabora.co.uk>
Date:   Wed Aug 11 17:29:47 2010 +0100

    Bump libfolks requirement to 0.1.13 for the linking API changes

 configure.ac |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

commit f84b2a0c688bf42002ddb93ca941af166c846687
Author: Philip Withnall <philip.withnall@collabora.co.uk>
Date:   Thu Aug 5 17:14:06 2010 +0100

    Only enable row reordering in EmpathyIndividualView if dragging is enabled

 libempathy-gtk/empathy-individual-view.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)