GNOME Bugzilla – Bug 521747
Use Cheese to set avatar
Last modified: 2011-05-26 07:28:11 UTC
It would be great to be able to use Cheese to take a picture and use it as avatar as Mac OSX does. I don't know how this could be done from a GUI and code point of view so any suggestion is welcome.
In my opinion: 1) Add a button in empathy's user info dialog that launch cheese if clicked. 2) In cheese we should be able to select a picture and set it as avatar, for that cheese can use libempathy(-gtk) like that: on_set_as_avatar (gchar *image_data, gsize image_size, gchar *image_mime_type) { EmpathyContactFactory *factory; GList *accounts, *l; factory = empathy_contact_factory_new (); accounts = mc_accounts_list (); for (l = accounts; l; l = l->next) { empathy_contact_factory_set_avatar (factory, l->data, image_data, image_size, image_mime_type); } g_object_unref (account); } Or cheese could popup a dialog with a EmpathyAccountChooser (widget derivated from GtkComboBox) to let the user select the account on which he wants that avatar.
i like the idea of having a dbus interface for cheese, which more applications can call, e.g. empathy, gdm, gnome-about-me..
Having a D-Bus interface would solve the problem of Empathy being able to check whether Cheese is installed: it can just call ListActivatableNames().
There's now a CheeseChooserWidget in libcheese-gtk (part of cheese 2.29.90), which could be used for this. Although it might be better in the long run to try to use the login avatar, as exposed in the accounts-dialogue[1]. [1]: http://git.gnome.org/browse/accounts-dialog/ http://blogs.fedoraproject.org/wp/mclasen/2010/01/15/old-promises/ http://www.hadess.net/2010/01/user-accounts-dialogue.html
I added support for setting the avatar from camera to Empathy through libcheese. I added a button "Camera Picture" to the file selection dialog. Pressing that button opens the Cheese-camera-chooser dialog. Some screenshots: 1. personal info before setting avatar from cheese: http://swarm.cs.pub.ro/~raluca/empathy/Empathy-Personal-Information-dialog.png 2. clicking on the image opens the file chooser dialog (with a new "Camera Picture" button): http://swarm.cs.pub.ro/~raluca/empathy/Empathy-Select-Your-Avatar-Image-dialog.png 3. clicking the "Camera Picture" dialog opens the Cheese-camera-chooser dialog: http://swarm.cs.pub.ro/~raluca/empathy/Empathy-Take-a-photo-dialog.png 4. after taking a picture/cropping it and clicking "Select" the avatar is updated: http://swarm.cs.pub.ro/~raluca/empathy/Empathy-Personal-Information-after-taking-new-avatar.png Source code: https://github.com/raluca-elena/empathy-cheese I will attack a patch.
Created attachment 186919 [details] [review] empathy avatar chooser using libcheese This is for review at the moment. The solution is sub-optimal: I create an avatar by saving a pixbuf in a file and create the avatar from that file. Is there a way to create the avatar directly from a pixbuf?
Review of attachment 186919 [details] [review]: Please work in a branch instead of master. Can you please squash your commits, the current history doesn't really make sense (we use code before depending on it). I think 2 patches are enough: - Depend on cheese - Use it This dialog looks weird http://swarm.cs.pub.ro/~raluca/empathy/Empathy-Take-a-photo-dialog.png It should have a proper title and decoration. The "Take a photo"'s button position is strange as well. I think libcheese should have more API to pass it the minimal and maximum size of the avatar (we get them from TpAvatarRequirements), so the camera widget wouldn't allow to crop a too small/big image. ::: libempathy-gtk/Makefile.am @@ +17,3 @@ $(DISABLE_DEPRECATED) +if BUILD_CHEESE No need to check, if Empathy isn't build with Cheese support, this var will be empty. See how we are handling this with WEBKIT_CFLAGS for example. ::: libempathy-gtk/empathy-avatar-chooser.c @@ +71,3 @@ + * "Camera Picture" button. Any positive value would be sufficient. + */ +#define EMPATHY_FILE_CHOOSER_CAMERA_PICTURE 10 I'd name it EMPATHY_AVATAR_CHOOSER_RESPONSE_WEBCAM or something. Please re-define the other response type as well. _FILE, _NO_IMAGE, etc. @@ +905,3 @@ + GdkPixbuf *pb; + /* TODO: decide on a propper place to this image */ + This is wrong, we should either add API to libcheese to give us the raw data, or add API to EmpathyAvatarChooser to get a GdkPixbuf directly. @@ +907,3 @@ + const char *filename = "/tmp/empathy-avatar.jpg"; + +static void Why not use cheese_avatar_chooser_get_picture()? @@ +916,3 @@ + if (response != GTK_RESPONSE_DELETE_EVENT && + response != GTK_RESPONSE_NONE) + if (response == GTK_RESPONSE_ACCEPT) { Why not just call gtk_widget_destroy()? @@ +968,3 @@ + else if (response == EMPATHY_FILE_CHOOSER_CAMERA_PICTURE) { + /* This corresponds to "Camera Picture" */ + choose_avatar_from_webcam(widget, chooser); our coding style is choose_avatar_from_webcam (widget, chooser); "make check" would detect such problem.
Created attachment 187383 [details] [review] libempathy-gtk: add dependency on libcheese libempathy-gtk: add dependency on libcheese
Created attachment 187384 [details] [review] libempathy-gtk: avatar chooser from cheese libempathy-gtk: avatar chooser from cheese Uses gmemoryoutputstream to write the pixbuf to a data format suitable for Empathy.
Review of attachment 187383 [details] [review]: ::: configure.ac @@ +471,3 @@ + +if test x"$with_cheese" != x"no" ; then + PKG_CHECK_MODULES(CHEESE, gstreamer-0.10 cheese-gtk >= 2.91.91.1, [have_cheese=yes], [have_cheese=no]) We already check for gstreamer; no need to do it twice. Please define a CHEESE_REQUIRED at the begining of the file. ::: libempathy-gtk/Makefile.am @@ +17,3 @@ $(DISABLE_DEPRECATED) +if BUILD_CHEESE As said before: "No need to check, if Empathy isn't build with Cheese support, this var will be empty. See how we are handling this with WEBKIT_CFLAGS for example."
Review of attachment 187384 [details] [review]: ::: libempathy-gtk/empathy-avatar-chooser.c @@ +751,3 @@ + data_stream = g_memory_output_stream_get_data (G_MEMORY_OUTPUT_STREAM(stream)); + data_size = g_memory_output_stream_get_size (G_MEMORY_OUTPUT_STREAM(stream)); + isn't load_data leaked?. Also, you don't respect coding style.
Review of attachment 187384 [details] [review]: ::: libempathy-gtk/empathy-avatar-chooser.c @@ +753,3 @@ + load_data = g_memdup(data_stream, data_size); + g_object_unref (stream); + stream = g_memory_output_stream_new(NULL, 0, g_realloc, g_free); You could save some work by creating the EmpathyAvatar with the existing pixbuf and then call avatar_chooser_set_image().
Review of attachment 187384 [details] [review]: ::: libempathy-gtk/empathy-avatar-chooser.c @@ +741,3 @@ + GOutputStream *stream; + + GError *error = NULL; You don't even that actually. All you need is gdk_pixbuf_save_to_buffer() create an EmpathyAvatar. Pass it to avatar_chooser_set_image().
Created attachment 187598 [details] [review] libempathy-gtk: avatar chooser from cheese
Review of attachment 187598 [details] [review]: Looks better. Some tweaks here and there and we'll be good. ::: libempathy-gtk/empathy-avatar-chooser.c @@ +740,3 @@ + EmpathyAvatar *avatar = NULL; + + gchar *mime = g_strdup ("image/png"); How can you be sure that the file is a png? This doesn't seem to be guarantee in libcheese's API doc. @@ +741,3 @@ + + if (!gdk_pixbuf_save_to_buffer (pb, &buf, &size, "png", NULL, NULL)) { + gchar *mime = g_strdup ("image/png"); You should pass an error and display the error message. @@ +925,3 @@ + + cheese_chooser = CHEESE_AVATAR_CHOOSER (dialog); +static void Looking at its code, cheese_avatar_chooser_get_picture() seems to return a new pixbuf, so we should unref it to not leak it. @@ +1016,2 @@ _("No Image"), GTK_RESPONSE_NO, You didn't change the response of the other actions.
Created attachment 187661 [details] [review] libempathy-gtk: avatar chooser from cheese
Review of attachment 187661 [details] [review]: What happen to the configure and Makefile change? ::: libempathy-gtk/empathy-avatar-chooser.c @@ +735,3 @@ + GdkPixbuf *pb) +{ + gchar *mime = g_strdup ("image/png"); what's the point of this? It's leaked btw. @@ +747,3 @@ + return; + } + EmpathyAvatar *avatar = NULL; Just pass "image/png" directly.
Also, please stop ignoring some of my comments: (In reply to comment #15) > ::: libempathy-gtk/empathy-avatar-chooser.c > @@ +740,3 @@ > + EmpathyAvatar *avatar = NULL; > + > + gchar *mime = g_strdup ("image/png"); > > How can you be sure that the file is a png? This doesn't seem to be guarantee > in libcheese's API doc. > @@ +1016,2 @@ > _("No Image"), > GTK_RESPONSE_NO, > > You didn't change the response of the other actions.
Hi, > Also, please stop ignoring some of my comments: I didn't ignore any of your comments. > (In reply to comment #15) >> ::: libempathy-gtk/empathy-avatar-chooser.c >> @@ +740,3 @@ >> + EmpathyAvatar *avatar = NULL; >> + >> + gchar *mime = g_strdup ("image/png"); >> >> How can you be sure that the file is a png? This doesn't seem to be guarantee >> in libcheese's API doc. Cheese gives me a pixbuf that has not in any specific format( png or else) that's why I choose a format for it. http://developer.gnome.org/gdk-pixbuf/2.23/gdk-pixbuf-File-saving.html#gdk-pixbuf-save-to-buffer Saves pixbuf to a new buffer in format type, which is currently "jpeg", "png", "tiff", "ico" or "bmp". http://developer.gnome.org/gdk-pixbuf/2.23/gdk-pixbuf-The-GdkPixbuf-Structure.html The GdkPixbuf structure contains information that describes an image in memory. >> @@ +1016,2 @@ >> _("No Image"), >> GTK_RESPONSE_NO, >> >> You didn't change the response of the other actions. I also responded to this one. The responses that you want me to change are standard ones. In the previous response to your request I also asked you how would you like them to be changed.
> > (In reply to comment #15) > >> ::: libempathy-gtk/empathy-avatar-chooser.c > >> @@ +740,3 @@ > >> + EmpathyAvatar *avatar = NULL; > >> + > >> + gchar *mime = g_strdup ("image/png"); > >> > >> How can you be sure that the file is a png? This doesn't seem to be guarantee > >> in libcheese's API doc. > > Cheese gives me a pixbuf that has not in any specific format( png or > else) that's why I choose a format for it. > > http://developer.gnome.org/gdk-pixbuf/2.23/gdk-pixbuf-File-saving.html#gdk-pixbuf-save-to-buffer > Saves pixbuf to a new buffer in format type, which is currently > "jpeg", "png", "tiff", "ico" or "bmp". Oh I see, sorry I misunderstood the API. > >> @@ +1016,2 @@ > >> _("No Image"), > >> GTK_RESPONSE_NO, > >> > >> You didn't change the response of the other actions. > > I also responded to this one. The responses that you want me to change > are standard ones. In the previous response to your request I also > asked you how would you like them to be changed. If we define at least one response type, I prefer to redefine all of them (making the code clearer). Se we whould have: EMPATHY_AVATAR_CHOOSER_RESPONSE_FILE EMPATHY_AVATAR_CHOOSER_RESPONSE_NO_IMAGE EMPATHY_AVATAR_CHOOSER_RESPONSE_WEBCAM etc. Their value doesn't mind much.
PNG is a bad choice for pictures, jpg compress them much better and with best quality, AFAIK. PNG is better in the case of icons and drawings, especially for transparency, but not in real-life pictures. Actually the most important is to keep the original compression format, because conversion would degrade the quality even more. I suggest adding an API in libcheese to give the raw data coming from the webcam, or at least give the format used by the pixbuf they give.
I choose the format PNG because from a webcam we already have a bad quality so I think that we shouldn't compress more the image. Avatars usually have reduced resolution and not so good quality. That's why I considered PNG, because it doesn't affect the quality even more. Cheese gives me a pixbuf, that's the data type, why should they extend the API? Even if that would happen all the operations in cheese-avatar-chooser are done on pixbufs so if I wanted to make another operations on the image I would still need pixbufs. http://developer.gnome.org/gdk-pixbuf/2.23/gdk-pixbuf-The-GdkPixbuf-Structure.html The GdkPixbuf structure contains information that describes an image in memory. Cheese gives me a pixbuf that has not in any specific format( png or else) that's why I choose a format for it. http://developer.gnome.org/gdk-pixbuf/2.23/gdk-pixbuf-File-saving.html#gdk-pixbuf-save-to-buffer Saves pixbuf to a new buffer in format type, which is currently "jpeg", "png", "tiff", "ico" or "bmp". What format would you prefer?
Created attachment 188343 [details] [review] avatar-chooser-from-cheese
Review of attachment 188343 [details] [review]: Once again, what happened to the configure/Makefile patches? ::: libempathy-gtk/empathy-avatar-chooser.c @@ +739,3 @@ + GdkPixbuf *pb) +{ + gchar *mime = g_strdup ("image/png"); mime is leaked. @@ +751,3 @@ + return; + } + EmpathyAvatar *avatar = NULL; Why not just pass "image/png" directly here? @@ +937,3 @@ + if (response != GTK_RESPONSE_DELETE_EVENT && + response != GTK_RESPONSE_NONE) + EmpathyAvatarChooser *chooser) Why are you using an idle?
>> Once again, what happened to the configure/Makefile patches? it is already there for some time now.Check here: http://bugzilla-attachments.gnome.org/attachment.cgi?id=187383 > ::: libempathy-gtk/empathy-avatar-chooser.c > @@ +739,3 @@ > + GdkPixbuf *pb) > +{ > + gchar *mime = g_strdup ("image/png"); > > mime is leaked. No it's not. Look at the code already written.In the same source at avatar_chooser_set_image_from_data gchar *mime_type = NULL; pixbuf = empathy_pixbuf_from_data_and_mime (data, size, &mime_type); avatar = empathy_avatar_new ((guchar *) data, size, mime_type, NULL); It is ok without free(mime_type),mime_type is given to empathy_avatar_new, it will be handled by it and will be freed also by someone else later. > > @@ +751,3 @@ > + return; > + } > + EmpathyAvatar *avatar = NULL; > > Why not just pass "image/png" directly here? Because it crashes. Somebody frees it later. > @@ +937,3 @@ > + if (response != GTK_RESPONSE_DELETE_EVENT && > + response != GTK_RESPONSE_NONE) > + EmpathyAvatarChooser *chooser) > > Why are you using an idle? This is what the code in the user accounts did which was written by Bastien Nocera who wrote the cheese avatar code. I'm not sure what the implications are. Please look with attention at the links I send you because I am a new in this and every time you send me a review that something is wrong I have to check all over again to be sure that what I coded is correct although I always double-check it before committing the patches. Also thank you for all the reviews till now.
(In reply to comment #25) > >> Once again, what happened to the configure/Makefile patches? > > it is already there for some time now.Check here: > http://bugzilla-attachments.gnome.org/attachment.cgi?id=187383 Right but you didn't fix this comment: """ +if BUILD_CHEESE No need to check, if Empathy isn't build with Cheese support, this var will be empty. See how we are handling this with WEBKIT_CFLAGS for example. """ > > ::: libempathy-gtk/empathy-avatar-chooser.c > > @@ +739,3 @@ > > + GdkPixbuf *pb) > > +{ > > + gchar *mime = g_strdup ("image/png"); > > > > mime is leaked. > No it's not. > Look at the code already written.In the same source at > avatar_chooser_set_image_from_data > gchar *mime_type = NULL; > pixbuf = empathy_pixbuf_from_data_and_mime (data, size, &mime_type); > avatar = empathy_avatar_new ((guchar *) data, size, mime_type, NULL); > > > It is ok without free(mime_type),mime_type is given to empathy_avatar_new, it > will be handled by it and will be freed also by someone else later. God this code is aweful, we should really clean that up. But yeah you're right, can you please add a comment /* dup the string as empathy_avatar_new() steals ownership of the it */ so it's clearer? I opened bug #650939 about the clean up but let's merge your patch first. > > @@ +937,3 @@ > > + if (response != GTK_RESPONSE_DELETE_EVENT && > > + response != GTK_RESPONSE_NONE) > > + EmpathyAvatarChooser *chooser) > > > > Why are you using an idle? > > > > This is what the code in the user accounts did which was written by > Bastien Nocera who wrote the cheese avatar code. I'm not sure what the > implications are. Does it work if you destroy the widget right away? > Please look with attention at the links I send you because I am a new in this > and every time you send me a review that something is wrong I have to check all > over again to be sure that what I coded is correct although I always > double-check it before committing the patches. Also thank you for all the > reviews till now. Don't worry, that's the best way to learn. Don't be afraid to ask if you don't understand some of my comments. Also, keep in mind that I generally just review patches; as I don't know all the Empathy code by heart this can lead to some error (like with empathy_avatar_new() above) so if you think I'm wrong just tell me. Anyway, it's getting pretty good, I'm sure we'll be able to merge this soon.
Created attachment 188579 [details] [review] libempathy-gtk-add-dependency-on-libcheese
Created attachment 188580 [details] [review] libempathy-gtk-avatar-chooser-from-cheese
Does it work if you destroy the widget right away? No it doesn't. If I destroy it the callback remains with no standing ground. It is like using the methods of an object that I already destroyed.
Review of attachment 188580 [details] [review]: Looks good! Please provide a fixed version of the first patch and that's good to go. :) There are still a bunch of things I'd like to see improved but that can be done post-merge, I'll open bugs for those.
Created attachment 188612 [details] [review] libempathy-gtk-add-dependency-on-libcheese
Merged to master! Thanks a lot for your work on this. I'll open bugs for the remaining bits to improve. commit baeea6e11b647b788693ce6e16cccc18cd0b8402 Author: Raluca Elena Podiuc <ralucaelena1985@gmail.com> Date: Sun Apr 10 22:19:14 2011 +0200 libempathy-gtk: avatar chooser from cheese (#521747) pixbuf saved into buffer as png -> set avatar from buffer commit 87118ba7e4c1a07765ccb762b9ccd603b5facc7b Author: Raluca Elena Podiuc <ralucaelena1985@gmail.com> Date: Mon Apr 11 21:34:01 2011 +0200 libempathy-gtk: add dependency on libcheese 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.
I opened bug #651115 and bug #651116 can you please work on those now that the initial code has landed?