GNOME Bugzilla – Bug 678875
empathy-nautilus-sendto should use RosterView
Last modified: 2012-06-28 09:25:46 UTC
Empathy-nautilus-sendto should use RosterView instead of EmpathyContactChooser
Created attachment 217281 [details] [review] empathy-nautilus-sendto now uses a RosterView instead of EmpathyContactChooser
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).
Created attachment 217295 [details] [review] Added new function to get selected individual
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.
Created attachment 217347 [details] [review] Added new function to get selected individual
Review of attachment 217347 [details] [review]: ::: libempathy-gtk/empathy-roster-view.c @@ +1579,3 @@ + return NULL; + + _CONTACT (child)); > missing space. Looks good otherwise
Created attachment 217349 [details] [review] Added new function to get selected individual
Review of attachment 217349 [details] [review]: Looks good, I'll merge thanks!
Created attachment 217360 [details] [review] Added new function to get selected individual New patch based on master
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
Created attachment 217381 [details] [review] empathy-nautilus-sendto now uses a RosterView instead of EmpathyContactChooser
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.
Created attachment 217391 [details] [review] empathy-nautilus-sendto now uses a RosterView instead of EmpathyContactChooser
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?
Created attachment 217408 [details] [review] empathy-nautilus-sendto now uses a RosterView instead of EmpathyContactChooser
Review of attachment 217408 [details] [review]: Looks good, I'll merge. thanks!
Attachment 217408 [details] pushed as 539cd8f - empathy-nautilus-sendto now uses a RosterView instead of EmpathyContactChooser
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.
(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.