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 678875 - empathy-nautilus-sendto should use RosterView
empathy-nautilus-sendto should use RosterView
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: File Transfer
unspecified
Other Linux
: Normal normal
: ---
Assigned To: empathy-maint
empathy-maint
Depends on:
Blocks:
 
 
Reported: 2012-06-26 12:41 UTC by Laurent Contzen
Modified: 2012-06-28 09:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
empathy-nautilus-sendto now uses a RosterView instead of EmpathyContactChooser (7.33 KB, patch)
2012-06-26 12:41 UTC, Laurent Contzen
reviewed Details | Review
Added new function to get selected individual (1.60 KB, patch)
2012-06-26 14:12 UTC, Laurent Contzen
reviewed Details | Review
Added new function to get selected individual (1.60 KB, patch)
2012-06-27 08:17 UTC, Laurent Contzen
reviewed Details | Review
Added new function to get selected individual (1.60 KB, patch)
2012-06-27 08:38 UTC, Laurent Contzen
accepted-commit_now Details | Review
Added new function to get selected individual (1.51 KB, patch)
2012-06-27 09:24 UTC, Laurent Contzen
committed Details | Review
empathy-nautilus-sendto now uses a RosterView instead of EmpathyContactChooser (6.04 KB, patch)
2012-06-27 10:12 UTC, Laurent Contzen
reviewed Details | Review
empathy-nautilus-sendto now uses a RosterView instead of EmpathyContactChooser (4.15 KB, patch)
2012-06-27 12:03 UTC, Laurent Contzen
reviewed Details | Review
empathy-nautilus-sendto now uses a RosterView instead of EmpathyContactChooser (3.62 KB, patch)
2012-06-27 14:09 UTC, Laurent Contzen
committed Details | Review

Description Laurent Contzen 2012-06-26 12:41:17 UTC
Empathy-nautilus-sendto should use RosterView instead of EmpathyContactChooser
Comment 1 Laurent Contzen 2012-06-26 12:41:57 UTC
Created attachment 217281 [details] [review]
empathy-nautilus-sendto now uses a RosterView instead of EmpathyContactChooser
Comment 2 Guillaume Desmottes 2012-06-26 12:56:40 UTC
Review of attachment 217281 [details] [review]:

::: libempathy-gtk/empathy-roster-view.h
@@ +78,3 @@
     guint event_id);
 
+EmpathyRosterContact * empathy_roster_view_get_activated_contact (EmpathyRosterView *self);

The activated contact is the last one which has been activated when we double clicked on it. What we want is the currently selected one (simple click). You can get it using  egg_list_box_get_selected_child().

Also, The RosterContact is more of an implementation detail of the view; we shouldn't expose it. What the user really care is the individual so this 
should be something like:

FolksIndividual * ..._get_selected_individual (...);

::: nautilus-sendto-plugin/empathy-nautilus-sendto.c
@@ +188,3 @@
+  gtk_scrolled_window_set_policy (GTK_SCROLLED_WINDOW (scrolled),
+      GTK_POLICY_NEVER, GTK_POLICY_AUTOMATIC);
+  scrolled = gtk_scrolled_window_new (NULL, NULL);

Filtering individuals is a pretty common case for the view widget so we should have helper API here. Something like:

static gooblean
my_very_own_filtering_function (EmapthyRosterView *view,
    FolksIndividual *individual,
    gpointer user_data)
{
    return @individual-is-cool-enough;
}

empathy_roster_view_set_ individual_filter_func (view, my_very_own_filtering_function, self);


If user specifies such function, the view should use it instead of empathy_roster_contact_is_online() in contact_should_be_displayed().

@@ +200,3 @@
+  gtk_widget_show (box);
+
+      EMPATHY_LIVE_SEARCH (search));

Use a #define for the KEY (saffer from typos).
Comment 3 Laurent Contzen 2012-06-26 14:12:05 UTC
Created attachment 217295 [details] [review]
Added new function to get selected individual
Comment 4 Guillaume Desmottes 2012-06-26 14:22:12 UTC
Review of attachment 217295 [details] [review]:

::: libempathy-gtk/empathy-roster-view.c
@@ +1573,3 @@
+{
+  GtkWidget *child;
+  child = egg_list_box_get_selected_child(EGG_LIST_BOX (self));

We usually let a blank line after the var declaration.
missing space before ().

@@ +1575,3 @@
+  child = egg_list_box_get_selected_child(EGG_LIST_BOX (self));
+
+  if (EMPATHY_IS_ROSTER_CONTACT (child))

This is real nip-ticking, but we the usual function flow is;

if (shit1)
  return NULL;

if (shit2)
  return NULL;

return good-thing;

so ideally this should be the other way around.
Comment 5 Laurent Contzen 2012-06-27 08:17:57 UTC
Created attachment 217347 [details] [review]
Added new function to get selected individual
Comment 6 Guillaume Desmottes 2012-06-27 08:35:15 UTC
Review of attachment 217347 [details] [review]:

::: libempathy-gtk/empathy-roster-view.c
@@ +1579,3 @@
+    return NULL;
+
+

_CONTACT (child));  > missing space.


Looks good otherwise
Comment 7 Laurent Contzen 2012-06-27 08:38:55 UTC
Created attachment 217349 [details] [review]
Added new function to get selected individual
Comment 8 Guillaume Desmottes 2012-06-27 09:08:00 UTC
Review of attachment 217349 [details] [review]:

Looks good, I'll merge thanks!
Comment 9 Laurent Contzen 2012-06-27 09:24:47 UTC
Created attachment 217360 [details] [review]
Added new function to get selected individual

New patch based on master
Comment 10 Guillaume Desmottes 2012-06-27 09:35:15 UTC
Comment on attachment 217360 [details] [review]
Added new function to get selected individual

Attachment 217360 [details] pushed as 81a65a9 - Added new function to get selected individual
Comment 11 Laurent Contzen 2012-06-27 10:12:38 UTC
Created attachment 217381 [details] [review]
empathy-nautilus-sendto now uses a RosterView instead of EmpathyContactChooser
Comment 12 Guillaume Desmottes 2012-06-27 10:28:40 UTC
Review of attachment 217381 [details] [review]:

::: libempathy-gtk/empathy-roster-view.h
@@ +5,3 @@
 #include <libempathy-gtk/egg-list-box/egg-list-box.h>
 #include <libempathy-gtk/empathy-live-search.h>
+#include <libempathy-gtk/empathy-roster-contact.h>

We don't need this here; just include it in the plugin file.

::: nautilus-sendto-plugin/empathy-nautilus-sendto.c
@@ +38,3 @@
 #include <libempathy-gtk/empathy-ui-utils.h>
+#include <libempathy-gtk/empathy-roster-view.h>
+#include <libempathy-gtk/empathy-roster-group.h>

do we really need roster-group?

@@ +131,3 @@
+  if (EMPATHY_IS_ROSTER_CONTACT (child))
+    return filter_contact (self, EMPATHY_ROSTER_CONTACT (child));
+    }

You could use ignore groups completely and return FALSE if whatever is passed to the filtering function which is not a contact.

@@ +137,3 @@
+
+static void
+

We don't need this signal here. Double clicking on a contact should be a no op.

@@ +148,3 @@
+
+static gboolean
+  EmpathyRosterView *self = user_data;

You wouldn't use a tooltip here.

@@ +176,3 @@
+      G_CALLBACK (individual_tooltip_cb), NULL);
+
+  gtk_widget_set_has_tooltip (roster_view, TRUE);

Nope, no tooltip.

@@ +178,3 @@
+  gtk_widget_set_has_tooltip (roster_view, TRUE);
+
+  empathy_roster_view_show_groups (EMPATHY_ROSTER_VIEW (roster_view), TRUE);

I wouldn't use groups; the old UI doesn't.
Comment 13 Laurent Contzen 2012-06-27 12:03:37 UTC
Created attachment 217391 [details] [review]
empathy-nautilus-sendto now uses a RosterView instead of EmpathyContactChooser
Comment 14 Guillaume Desmottes 2012-06-27 12:48:22 UTC
Review of attachment 217391 [details] [review]:

Live search doesn't work but I suspect you won't be able to fix it without refactoring the view filtering code so I'll remove it for now.

::: nautilus-sendto-plugin/empathy-nautilus-sendto.c
@@ +116,3 @@
+    return FALSE;
+
+  EmpathyRosterView *self = user_data;

You can inline filter_contact() here; it's only used here right?
Comment 15 Laurent Contzen 2012-06-27 14:09:48 UTC
Created attachment 217408 [details] [review]
empathy-nautilus-sendto now uses a RosterView instead of EmpathyContactChooser
Comment 16 Guillaume Desmottes 2012-06-27 14:55:56 UTC
Review of attachment 217408 [details] [review]:

Looks good, I'll merge. thanks!
Comment 17 Guillaume Desmottes 2012-06-27 14:58:32 UTC
Attachment 217408 [details] pushed as 539cd8f - empathy-nautilus-sendto now uses a RosterView instead of EmpathyContactChooser
Comment 18 Bastien Nocera 2012-06-27 15:10:08 UTC
nautilus-sendto master could do with something like that too, see bug 626553 (instead of being restricted to IM, the nautilus-sendto plugin would also filter based on e-mail address, through folks).

Let me know if you're interested in working on that.
Comment 19 Guillaume Desmottes 2012-06-28 09:25:46 UTC
(In reply to comment #18)
> nautilus-sendto master could do with something like that too, see bug 626553
> (instead of being restricted to IM, the nautilus-sendto plugin would also
> filter based on e-mail address, through folks).
> 
> Let me know if you're interested in working on that.

We definitely are, that may fit nicely in Laurent's SoC project. But ideally I'd like first to have a complete design approved by the design team. From our conversation yesterday, some bits weren't that clear, like how sending through email would work.