GNOME Bugzilla – Bug 766670
Nicer profile picture (avatar) selection
Last modified: 2018-02-09 12:38:40 UTC
The current UI for selecting a profile picture is a bit limited and not particularly great looking. The pictures are really small, you have to open a file chooser to select a picture, there's no way to view previously used profile pictures, or to select a profile picture from one of your online accounts. There are some new designs that seek to resolve these issues and provide a much nicer experience, using more standard GNOME 3 design patterns: https://wiki.gnome.org/Design/OS/AvatarChooser Note that this design is intended to be used by initial setup also.
I have been implementing this in a wip branch ~> https://git.gnome.org/browse/gnome-control-center/log/?h=wip/feborges/avatar-chooser
Created attachment 367708 [details] [review] user-accounts: Turn UmPhotoDialog in a full GObject For the new avatar-chooser implementation we will use Gtk+ widget composite templates.
Created attachment 367709 [details] [review] user-accounts: Introduce a UI template for UmPhotoDialog
Created attachment 367710 [details] [review] user-accounts: Introduce the new Avatar Chooser popover This is based in the Avatar Chooser specifications available at https://wiki.gnome.org/Design/OS/AvatarChooser#Tentative_Design
The current implementation does not introduce visible regressions to the old one. TODO: * Pick images from online accounts * List recently used pictures * Generate avatar automatically (with the initial letter, like gmail)
Created attachment 367714 [details] [review] user-accounts: Make the avatar entries rounded See mockups at https://wiki.gnome.org/Design/OS/AvatarChooser#Tentative_Design
Created attachment 367715 [details] avatar-chooser-screenshot Allan, should the avatar shown in the panel also be rounded? (See screenshot)
> Allan, should the avatar shown in the panel also be rounded? (See screenshot) And in carousel?
Review of attachment 367708 [details] [review]: Looks ok, just see my comment on the following patches. ::: panels/user-accounts/um-photo-dialog.c @@ +601,1 @@ + g_clear_object (&um->thumb_factory); This is not necessarily related to GObject transformations, but ok.
Review of attachment 367709 [details] [review]: Looks ok, just not sure that this patch makes much sense without the following one, but definitely easier for review. ::: panels/user-accounts/um-photo-dialog.c @@ +572,3 @@ UmPhotoDialog *um; + um = g_object_new (UM_TYPE_PHOTO_DIALOG, NULL); This should be part of the previous patch I suppose. @@ +618,3 @@ { + GtkWidgetClass *wclass = GTK_WIDGET_CLASS (klass); + GObjectClass *oclass = G_OBJECT_CLASS (klass); The alignment of variables isn't probably wanted here, but that's detail.
Review of attachment 367710 [details] [review]: Popover should be probably closed after avatar selection, but it isn't. ::: panels/user-accounts/um-photo-dialog.c @@ +45,2 @@ struct _UmPhotoDialog { + GtkPopover parent; Shouldn't this be also part of the first patch? Maybe better to merge them, but nothing crucial. @@ +459,3 @@ + um = g_object_new (UM_TYPE_PHOTO_DIALOG, + "relative-to", button, + NULL); dtto @@ +465,3 @@ /* Set up the popup */ um->popup_button = button; + Please don't...
Review of attachment 367714 [details] [review]: As per Comment 9, but please discuss it with designers. See also: ./../gnome-control-center/panels/user-accounts/um-photo-dialog.c: In function ‘create_face_widget’: ../../gnome-control-center/panels/user-accounts/um-photo-dialog.c:359:32: warning: unknown conversion type character ‘}’ in format [-Wformat=] css = g_strdup_printf ("* {\n" ^~~~~~~ ../../gnome-control-center/panels/user-accounts/um-photo-dialog.c:362:54: note: format string is defined here " border-radius: 50% }\n", uri);
I also see the following when closing after avatar change, not sure this is stricly related to this bug though: (gnome-control-center:29163): GLib-GObject-CRITICAL **: 10:18:06.144: g_object_unref: assertion 'G_IS_OBJECT (object)' failed Thread 1 "gnome-control-c" received signal SIGTRAP, Trace/breakpoint trap. _g_log_abort (breakpoint=1) at /home/oholy/gnome/glib/glib/gmessages.c:583 583 } (gdb) bt
+ Trace 238375
Created attachment 367806 [details] [review] user-accounts: Turn UmPhotoDialog in a full GObject For the new avatar-chooser implementation we will use Gtk+ widget composite templates.
Created attachment 367807 [details] [review] user-accounts: Introduce a UI template for UmPhotoDialog
Created attachment 367808 [details] [review] user-accounts: Introduce the new Avatar Chooser popover This is based in the Avatar Chooser specifications available at https://wiki.gnome.org/Design/OS/AvatarChooser#Tentative_Design
Created attachment 367809 [details] [review] user-accounts: Make the avatar entries rounded See mockups at https://wiki.gnome.org/Design/OS/AvatarChooser#Tentative_Design
Review of attachment 367808 [details] [review]: Seems that it is not applicable on master, can you please do something with it?
Created attachment 367814 [details] [review] user-accounts: Turn UmPhotoDialog in a full GObject For the new avatar-chooser implementation we will use Gtk+ widget composite templates.
Created attachment 367815 [details] [review] user-accounts: Introduce a UI template for UmPhotoDialog
Created attachment 367816 [details] [review] user-accounts: Introduce the new Avatar Chooser popover This is based in the Avatar Chooser specifications available at https://wiki.gnome.org/Design/OS/AvatarChooser#Tentative_Design
Created attachment 367817 [details] [review] user-accounts: Make the avatar entries rounded See mockups at https://wiki.gnome.org/Design/OS/AvatarChooser#Tentative_Design
Review of attachment 367814 [details] [review]: Looks ok, but please push all the three together...
Review of attachment 367815 [details] [review]: dtto
Review of attachment 367816 [details] [review]: The popover is not closed after "Take a picture...", not "Select a file...".
Review of attachment 367817 [details] [review]: Doesn't make sense to me without changes in other places...
Also would be nice to propose same changes for g-i-s.
Created attachment 367819 [details] [review] user-accounts: Turn UmPhotoDialog in a full GObject For the new avatar-chooser implementation we will use Gtk+ widget composite templates.
Created attachment 367820 [details] [review] user-accounts: Introduce a UI template for UmPhotoDialog
Created attachment 367821 [details] [review] user-accounts: Introduce the new Avatar Chooser popover This is based in the Avatar Chooser specifications available at https://wiki.gnome.org/Design/OS/AvatarChooser#Tentative_Design
Created attachment 367822 [details] [review] user-accounts: Make the avatar entries rounded See mockups at https://wiki.gnome.org/Design/OS/AvatarChooser#Tentative_Design
Created attachment 367834 [details] [review] user-accounts: Render user images with CSS We make UmUserImage a GtkBox child and use CSS to render an image on top of it. This patch makes the user images rounded.
Review of attachment 367819 [details] [review]: No need to re-upload the patches, which have not changed...
Review of attachment 367820 [details] [review]: dtto
Review of attachment 367821 [details] [review]: Looks ok, thanks. But still, I hit some segfaults, which seems for some reason related to this patch (I can't reproduce without this patch)... :-( Steps to reproduce: 1/ change avatar for some user 2/ change avatar again for the SAME user 3/ change avatar for ANOTHER user You will see the following: (gnome-control-center:20162): GLib-GObject-CRITICAL **: 09:54:24.769: g_object_unref: assertion 'G_IS_OBJECT (object)' failed (gnome-control-center:20162): AccountsService-CRITICAL **: 09:54:24.770: act_user_collate: assertion 'ACT_IS_USER (user2)' failed (gnome-control-center:20162): AccountsService-CRITICAL **: 09:54:24.770: act_user_collate: assertion 'ACT_IS_USER (user2)' failed (gnome-control-center:20162): GLib-GObject-WARNING **: 09:54:24.770: invalid uninstantiatable type '(null)' in cast to 'ActUser' (gnome-control-center:20162): AccountsService-CRITICAL **: 09:54:24.770: act_user_get_uid: assertion 'ACT_IS_USER (user)' failed (gnome-control-center:20162): AccountsService-CRITICAL **: 09:54:24.770: act_user_get_real_name: assertion 'ACT_IS_USER (user)' failed (gnome-control-center:20162): AccountsService-CRITICAL **: 09:54:24.770: act_user_get_user_name: assertion 'ACT_IS_USER (user)' failed (gnome-control-center:20162): GLib-CRITICAL **: 09:54:24.770: g_utf8_collate_key: assertion 'str != NULL' failed Segmentation fault (core dumped)
Review of attachment 367822 [details] [review]: See review of the following patch...
Review of attachment 367834 [details] [review]: I did not go thru the code, because I am not still convinced that this is the right way to go. I think we should rather crop the images themselves and make their borders transparent, so they will be shown rounded in all possible places, not only in users panel (I am pretty sure I wrote this somewhere already, but can't find it right now). With this patch applied, the default avatar is not rendered for some reason. And also see: ../../gnome-control-center/panels/user-accounts/um-user-image.c: In function ‘render_image’: ../../gnome-control-center/panels/user-accounts/um-user-image.c:42:19: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] icon_file = act_user_get_icon_file (image->priv->user); ^ ../../gnome-control-center/panels/user-accounts/um-user-image.c:54:75: warning: passing argument 1 of ‘gtk_widget_get_style_context’ from incompatible pointer type [-Wincompatible-pointer-types] gtk_style_context_add_provider (gtk_widget_get_style_context (image), ^~~~~ In file included from /opt/gnome/include/gtk-3.0/gtk/gtkapplication.h:27, from /opt/gnome/include/gtk-3.0/gtk/gtkwindow.h:33, from /opt/gnome/include/gtk-3.0/gtk/gtkdialog.h:32, from /opt/gnome/include/gtk-3.0/gtk/gtkaboutdialog.h:30, from /opt/gnome/include/gtk-3.0/gtk/gtk.h:31, from ../../gnome-control-center/panels/user-accounts/um-user-image.h:22, from ../../gnome-control-center/panels/user-accounts/um-user-image.c:19: /opt/gnome/include/gtk-3.0/gtk/gtkwidget.h:1311:19: note: expected ‘GtkWidget *’ {aka ‘struct _GtkWidget *’} but argument is of type ‘UmUserImage *’ {aka ‘struct _UmUserImage *’} GtkStyleContext * gtk_widget_get_style_context (GtkWidget *widget); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
Created attachment 367905 [details] [review] user-accounts: Introduce the new Avatar Chooser popover This is based in the Avatar Chooser specifications available at https://wiki.gnome.org/Design/OS/AvatarChooser#Tentative_Design
Created attachment 367906 [details] [review] user-accounts: Introduce the new Avatar Chooser popover This is based in the Avatar Chooser specifications available at https://wiki.gnome.org/Design/OS/AvatarChooser#Tentative_Design
Review of attachment 367906 [details] [review]: ::: panels/user-accounts/um-photo-dialog.c @@ +525,3 @@ um->user = NULL; } + um->user = g_object_ref (user); I traced it down to a decrease of reference counting for the User object on disposal. This should fix it now. Let me know if you still reproduce it.
Review of attachment 367906 [details] [review]: That's it, thanks!
Created attachment 367909 [details] [review] user-accounts: Make the avatar entries rounded
(In reply to Felipe Borges from comment #42) > Created attachment 367909 [details] [review] [review] > user-accounts: Make the avatar entries rounded Ah, worth mentioning that this depends on a constant defined in Bug 792243.
(In reply to Felipe Borges from comment #43) > (In reply to Felipe Borges from comment #42) > > Created attachment 367909 [details] [review] [review] [review] > > user-accounts: Make the avatar entries rounded > > Ah, worth mentioning that this depends on a constant defined in Bug 792243. Anyway, the latest patch fails to apply on top for me for some reason, but it still just changes the bitmap only for user panel. Sorry if it was not clear before, but I meant that we should make the bitmap circular after we choose it from whatever source (default, camera, file) and **set the cropped avatar over accountsservice**, so it will be circular on all places e.g. shell and gdm, not only in users panel... It doesn't make sense to me to make users into believing that the avatars are circular and consequently show them something different in shell, or gdm... (Imagine that you will take a photo and in the corner is your naked friend, you will crop it to circle and everything looks ok in Users panel, but then the naked friend is shown in the corner of your avatar on login screen... I am just kidding, but that's exactly why I don't like this change.) However, I am not sure that e.g. gdm is ready for such change at this point. So I would push just the new set of faces and new popover without the circular shapes before freeze... Btw that change still requires an update for crop dialog. Please discuss the circular avatars with Allan if you think that I am wrong...
(In reply to Ondrej Holy from comment #44) > However, I am not sure that e.g. gdm is ready for such change at this point. > So I would push just the new set of faces and new popover without the > circular shapes before freeze... > > Btw that change still requires an update for crop dialog. > > Please discuss the circular avatars with Allan if you think that I am > wrong... Ok. To be fair lets leave the rounding discussion for the next cycle then, since it involves other components. We are too tight in the release schedule to push these changes now. I will go forward with the popover as it is, with rectangular images.
Comment on attachment 367909 [details] [review] user-accounts: Make the avatar entries rounded See Bug 793188.
Attachment 367819 [details] pushed as 3fb1c1d - user-accounts: Turn UmPhotoDialog in a full GObject Attachment 367820 [details] pushed as 169a0b3 - user-accounts: Introduce a UI template for UmPhotoDialog Attachment 367906 [details] pushed as 028a06f - user-accounts: Introduce the new Avatar Chooser popover
(In reply to Ondrej Holy from comment #44) ... > > Ah, worth mentioning that this depends on a constant defined in Bug 792243. > > Anyway, the latest patch fails to apply on top for me for some reason, but > it still just changes the bitmap only for user panel. Sorry if it was not > clear before, but I meant that we should make the bitmap circular after we > choose it from whatever source (default, camera, file) and **set the cropped > avatar over accountsservice**, so it will be circular on all places e.g. > shell and gdm, not only in users panel... ... I'm not entirely sure about this. It would certainly help with consistency, but: 1. A square border is often placed around avatar images. We ought to check whether how round images will look. 2. What if an app wants to show a square image for some reason? Is it right for us to enforce the shape of the avatar image? 3. What happens if we decide that we no longer want round images in the future?
The avatar chooser mockup looks great, but do you think that is ok to show circle and save rectangle (i.e. include something that is not visible)? I agree with showing just circular images in the user's panel (in the carousel and next to the name) even though that source images are squares. But if square images will be saved in accountsservice, then it should be obvious, at least from the avatar chooser (and when cropping), that the images are squares and not circles... that's my two cents.