GNOME Bugzilla – Bug 611972
Support marking contacts as favorites
Last modified: 2010-10-02 09:06:19 UTC
It would be useful to be able to mark contacts as favorites (which would push them to the top of the contact list). This is especially useful with very large contact lists (eg, almost any one containing a Facebook account). I've created a few patches that does this, based on favorites support in telepathy-logger. The UI is fairly basic -- every contact has a star next to it, and clicking on it toggles it on or off, sorting the contact higher or lower in the list. The git repo is here: http://git.collabora.co.uk/?p=user/treitter/empathy.git;a=shortlog;h=refs/heads/favorites-using-logger Danielle, could you please review this, make changes as necessary, and apply to mainline? A few minor points/questions: * contacts in groups don't leave their group when they become a favorite; it would probably be be best to add and remove an extra copy of such a contact when their favorite status toggles * we might want to s/favourite/favorite/, for consistency with the rest of the code
(In reply to comment #0) > The git repo is here: > > http://git.collabora.co.uk/?p=user/treitter/empathy.git;a=shortlog;h=refs/heads/favorites-using-logger I had a really quick look over this branch and have a few comments, but what I really don't understand is the top commit, c3a5c8a48ed. You say "rebase upon the logger", but it doesn't look like a rebase at all. Or, did you actually add Logger.xml to the source at that point? Very confused. > * we might want to s/favourite/favorite/, for consistency with the rest of the > code I say the other way round, unsurprisingly. "Most" of Telepathy is en_GB (the spec for example), so let's keep it that way. And now that I've had a closer look at the branch, I see you're using "favourite" in your Logger spec XML. I especially think this should be en_GB now, otherwise you're just going to have to change it on smcv's demands later.
GNOME convention is to use American English for translated strings. As the Telepathy spec is British English, I'd say to use British everywhere except for translated strings.
Code generation fails because of a type bug. I fixed it in: http://git.collabora.co.uk/?p=user/cassidy/empathy;a=commitdiff;h=0c225291cf4dd325783b3eac855e912824574c51 We should use the standard "emblem-favorite" icon rather than our own empathy-favorite image which has been removed in master. I'm really not convinced by the UI. Adding one column in the contact list treeview is really ugly and a bit of a shame as we've done some work to reduce the number of things in this treeview. What about adding a checkbox in the contact menu? I pushed a patch fixing coding style issues and will continue to work on this branch.
I rebased this branch on top of master (to fix conflicts since the s/moblin/MeeGo renaming) and added some more patches: http://git.collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/favorites-using-logger-611972 Any reason to display favorites contacts on top of their group rather than creating a special group on top of the contact list?
(In reply to comment #3) > I'm really not convinced by the UI. Adding one column in the contact list > treeview is really ugly and a bit of a shame as we've done some work to reduce > the number of things in this treeview. > What about adding a checkbox in the contact menu? I've done that in my branch.
Created attachment 155628 [details] [review] squashed diff
Review of attachment 155628 [details] [review]: Looks mostly fine. ::: configure.ac @@ +468,3 @@ + PKG_CHECK_MODULES(TELEPATHY_LOGGER, + [ + libtelepathy-logger "telepathy-logger" ::: libempathy-gtk/empathy-contact-list-store.c @@ +944,3 @@ + } + + g_list_foreach (iters, (GFunc) gtk_tree_iter_free, NULL); Can't these be free'd during iteration, save the extra loop. ::: libempathy-gtk/empathy-images.h @@ +43,3 @@ #define EMPATHY_IMAGE_DOCUMENT_SEND "document-send" +#define EMPATHY_IMAGE_FAVOURITE "empathy-starred" +#define EMPATHY_IMAGE_UNFAVOURITE "empathy-unstarred" Weren't these removed?
So having used this for a bit, I'm kinda wondering what it's doing in Empathy at all. It seems completely orthogonal to contact groups in Empathy. Maybe we want to rethink this UI completely?
(In reply to comment #7) > Review of attachment 155628 [details] [review]: > > Looks mostly fine. > > ::: configure.ac > @@ +468,3 @@ > + PKG_CHECK_MODULES(TELEPATHY_LOGGER, > + [ > + libtelepathy-logger > > "telepathy-logger" fixed. > ::: libempathy-gtk/empathy-contact-list-store.c > @@ +944,3 @@ > + } > + > + g_list_foreach (iters, (GFunc) gtk_tree_iter_free, NULL); > > Can't these be free'd during iteration, save the extra loop. I removed this function. > ::: libempathy-gtk/empathy-images.h > @@ +43,3 @@ > #define EMPATHY_IMAGE_DOCUMENT_SEND "document-send" > +#define EMPATHY_IMAGE_FAVOURITE "empathy-starred" > +#define EMPATHY_IMAGE_UNFAVOURITE "empathy-unstarred" > > Weren't these removed? removed.
I did the following changes in my branch http://git.collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/favorites-using-logger-611972 - Contacts which are not in any group are now in fake group "Ungrouped" displayed at the bottom of the contact list. - Favorites contacts are in a fake group "Favorite People" displayed on the top of the contact list with an icon - Fake groups can't be removed I still have to: - Add an favorite option in the contact edit dialog. - Make fake group easier to distinguish; not sure how? Setting a different background in the group title maybe? - DnD to the favorite group should add the contact as a favorite.
Created attachment 155699 [details] [review] squashed diff
Review of attachment 155699 [details] [review]: Ok, this looks pretty good. This UI works much better as well. Didn't notice any major issues. Somewhat related to this branch, now that we have 'Fake Groups', should People Nearby become a Fake Group? Since it's not a real contact group. It could be '[Avahi logo] People Nearby'. ::: configure.ac @@ +485,3 @@ +AM_CONDITIONAL(HAVE_FAVOURITE_CONTACTS, test "x$have_telepathy_logger" = "xyes") +AC_SUBST(FAVOURITE_CONTACTS_CFLAGS) +AC_SUBST(FAVOURITE_CONTACTS_LIBS) Why are these getting exported, there aren't any new libs here. ::: libempathy-gtk/Makefile.am @@ +12,3 @@ $(ENCHANT_CFLAGS) \ $(LIBCHAMPLAIN_CFLAGS) \ + $(FAVOURITE_CONTACTS_CFLAGS) \ Similarly this. @@ +137,3 @@ $(ENCHANT_LIBS) \ $(LIBCHAMPLAIN_LIBS) \ + $(FAVOURITE_CONTACTS_LIBS) \ and this. ::: libempathy-gtk/empathy-contact-list-store.h @@ +69,3 @@ +#define EMPATHY_CONTACT_LIST_STORE_UNGROUPED _("Ungrouped") +#define EMPATHY_CONTACT_LIST_STORE_FAVORITE _("Favorites People") Doesn't make sense. Either 'Favorites' or 'Favorite People'. ::: libempathy-gtk/empathy-contact-list-view.c @@ +896,3 @@ + + if (!is_group) + goto out;; Are the double ;; intentional? @@ +899,3 @@ + + if (tp_strdiff (name, EMPATHY_CONTACT_LIST_STORE_FAVORITE)) + goto out;; Here too. ::: libempathy/Makefile.am @@ +11,3 @@ $(TPL_CFLAGS) \ $(LIBEMPATHY_CFLAGS) \ + $(FAVOURITE_CONTACTS_CFLAGS) \ Is this required? @@ +121,3 @@ $(top_builddir)/extensions/libemp-extensions.la \ $(LIBEMPATHY_LIBS) \ + $(FAVOURITE_CONTACTS_LIBS) \ And this?
Review of attachment 155699 [details] [review]: ::: configure.ac @@ +485,3 @@ +AM_CONDITIONAL(HAVE_FAVOURITE_CONTACTS, test "x$have_telepathy_logger" = "xyes") +AC_SUBST(FAVOURITE_CONTACTS_CFLAGS) +AC_SUBST(FAVOURITE_CONTACTS_LIBS) Good point; I missed that while reviewing Travis's branch. Good catch! ::: libempathy-gtk/empathy-contact-list-store.h @@ +69,3 @@ +#define EMPATHY_CONTACT_LIST_STORE_UNGROUPED _("Ungrouped") +#define EMPATHY_CONTACT_LIST_STORE_FAVORITE _("Favorites People") Changed to 'Favorite People' (as we already have 'People Nearby') ::: libempathy-gtk/empathy-contact-list-view.c @@ +896,3 @@ + + if (!is_group) + goto out;; removed.
(In reply to comment #12) > Review of attachment 155699 [details] [review]: > > Somewhat related to this branch, now that we have 'Fake Groups', should People > Nearby become a Fake Group? Since it's not a real contact group. It could be > '[Avahi logo] People Nearby'. Good idea; but that will be for 2.31 as that's a change in existing UI. I opened bug #612390
(In reply to comment #10) > I still have to: > - Add an favorite option in the contact edit dialog. done. > - Make fake group easier to distinguish; not sure how? Setting a different > background in the group title maybe? I'm still not sure about this one. Any suggestion? > - DnD to the favorite group should add the contact as a favorite. done.
I added DnD support.
+ /* Fake groups can't be modified. We allow to drag to the favorite fake + * group tough so user can mark contact as favorite using DnD */ 'dragging' and 'though' and 'a favourite'. Unless you really meant that you want it to be tough. Otherwise looks fine. (In reply to comment #15) > > - Make fake group easier to distinguish; not sure how? Setting a different > > background in the group title maybe? > > I'm still not sure about this one. Any suggestion? Not really, no.
(In reply to comment #17) > + /* Fake groups can't be modified. We allow to drag to the favorite > fake > + * group tough so user can mark contact as favorite using DnD */ > > 'dragging' and 'though' and 'a favourite'. Unless you really meant that you > want it to be tough. This comment has been removed in a latter patch.
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
*** Bug 523087 has been marked as a duplicate of this bug. ***