GNOME Bugzilla – Bug 626130
Add linking UI for Individuals
Last modified: 2010-08-11 16:41:33 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
Created attachment 167202 [details] [review] Only enable row reordering in EmpathyIndividualView if dragging is enabled
Created attachment 167203 [details] [review] Add EmpathyPersonaStore and EmpathyPersonaView
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?
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.
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
Created attachment 167439 [details] [review] Add EmpathyIndividualWidget
Created attachment 167440 [details] [review] Add EmpathyIndividualLinker
Created attachment 167441 [details] [review] Allow linking personas through EmpathyIndividualManager
Created attachment 167442 [details] [review] Add EmpathyLinkingDialog
(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.
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?
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_...
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; }
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);
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.
Created attachment 167493 [details] [review] Squashed diff of the persona-store-rebase1 branch (updated)
Created attachment 167494 [details] [review] Squashed diff of the linking-dialogue-rebase1 branch (updated)
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.
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.
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).
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(-)