Created attachment 265940 [details] [review]
add pixbuf filter

Created attachment 265944 [details] [review]
do not generate avatars for dirs

To fix following warning:

(gnome-control-center:24581): GnomeDesktop-WARNING **: Error reading from file:///home/oholy19/Desktop: Error reading from file descriptor: Is a directory

Created attachment 265946 [details] [review]
do not generate previews for dirs

Created attachment 265949 [details] [review]
fix showing arrow

ALso fixed:

um-photo-dialog.c: In function 'popup_button_draw':
um-photo-dialog.c:540:9: warning: 'gtk_widget_get_state' is deprecated (declared at /opt/gnome/include/gtk-3.0/gtk/gtkwidget.h:678): Use 'gtk_widget_get_state_flags' instead [-Wdeprecated-declarations]
         if (gtk_widget_get_state (gtk_bin_get_child (GTK_BIN (widget))) != GTK_STATE_PRELIGHT &&

Review of attachment 265946 [details] [review]:

I still see warnings while browsing in the dialogue:
(gnome-control-center:11660): GnomeDesktop-WARNING **: Error reading from file:///home/hadess/Pictures/Webcam: Error reading from file descriptor: Is a directory

Why don't you filter out those errors?
if (g_error_matches(error, G_IO_ERROR, G_IO_ERROR_IS_DIRECTORY))
  ...

Review of attachment 265940 [details] [review]:

That looks fine.

Review of attachment 265949 [details] [review]:

Looks fine.

(In reply to comment #5)
> Review of attachment 265946 [details] [review]:
> 
> I still see warnings while browsing in the dialogue:
> (gnome-control-center:11660): GnomeDesktop-WARNING **: Error reading from
> file:///home/hadess/Pictures/Webcam: Error reading from file descriptor: Is a
> directory
> 
> Why don't you filter out those errors?
> if (g_error_matches(error, G_IO_ERROR, G_IO_ERROR_IS_DIRECTORY))
>   ...

...because the warnings comes from:

+ Trace 233041

Should I suppress the warnings in the _gdk_pixbuf_new_from_uri_at_scal when G_IO_ERROR_IS_DIRECTORY?

I think you shouldn't show any errors when browsing, just disable the OK button when there's an error (it's an unreadable file, or a directory, or not really an image, etc.)

Created attachment 266466 [details]
arrow screenshot

(In reply to comment #7)
> Review of attachment 265949 [details] [review]:
> 
> Looks fine.

Not sure I fixed it right, because don't know correct behavioral... Please look at the screenshot. Shouldn't be the black arrow visible only when popup with avatars is shown?

Created attachment 266471 [details] [review]
remove broken code to draw arrow

#control-center:
aday	oholy, what's the issue here exactly? why do we need the arrow?
oholy	aday: there is code to draw the arrow which produces warning and not working, so I try to fix it, but not sure about correct behavioral...
oholy	aday: but if we don't want it, we can just drop it :-)
aday	oholy, that might be best. i don't think we really need it

(In reply to comment #9)
> I think you shouldn't show any errors when browsing, just disable the OK button
> when there's an error (it's an unreadable file, or a directory, or not really
> an image, etc.)

Actually this is already done:
gtk_dialog_set_response_sensitive (GTK_DIALOG (chooser), GTK_RESPONSE_ACCEPT, (pixbuf != NULL));

However those warnings comes from gnome_desktop_thumbnail_factory_generate_thumbnail when generating pixbuf...

But reported bug is about something slightly different I've understand recently:
1/ open file chooser
2/ select image -> preview is generated and OK button is sensitive
3/ type wrong location using location bar (ctrl+l) and press enter, it fails

However there is also opposite problem:
1/ open file chooser
2/ don't select image -> preview isn't generated and OK button is unsensitive
3/ type correct location using location bar (ctrl+l) and press enter, nothing happens

Is it there any clean way to validate location when focus out/activate?

e.g. Eye of gnome has OK button sensitive constantly and shows error dialog when wrong file selected...

Comment on attachment 265940 [details] [review]
add pixbuf filter

commit 8d5d712e2c5f5327a987c7214a6ea9a60cefdc01

Comment on attachment 266471 [details] [review]
remove broken code to draw arrow

this is obsolete and also irrelevant for this bug

Comment on attachment 266466 [details]
arrow screenshot

dtto

Created attachment 279110 [details] [review]
show error dialog for wrong location

Show error dialog and don't try to crop the file if wrong location is set by the location bar. It is workaround when user type wrong location by hand. See Comment 13.

Review of attachment 265946 [details] [review]:

That seems to work.

Review of attachment 279110 [details] [review]:

That doesn't seem very useful, as the file chooser will be dismissed, and it's unclear whether we set a non-working file as the new avatar, or need to try again.

We should:
- make the filechooser unsensitive
- process the response (and try to load the pixbuf)
- if loading the pixbuf fails, make the filechooser sensitive again and show that warning on top of it

That way, the user has a chance of correcting that problem straight away.

Thanks for reviews. The first patch from Bug 547988 is the reason why it is so broken now, however the second patch from the same bug allow us to fix it completely I hope... :-)

Comment on attachment 265946 [details] [review]
do not generate previews for dirs

commit 924872ba0cae7643d1b739f6b5f545ebbee117b1

Created attachment 281134 [details] [review]
fix response sensitivity

Preview has to be generated after default handler of "selection-changed"
signal, otherwise dialog response sensitivity is rewritten (Bug 547988).
Preview also has to be generated on "selection-changed" signal to reflect
all changes (Bug 660877).

So, what about this solution?

Review of attachment 281134 [details] [review]:

Looks good otherwise.

::: panels/user-accounts/um-photo-dialog.c
@@ +236,3 @@
+         * Preview also has to be generated on "selection-changed" signal to reflect
+         * all changes (Bug 660877).
+         */

Close the comment on the same line as the text.

Comment on attachment 281134 [details] [review]
fix response sensitivity

commit 4f2e590cf597385ea3bdee2d3231c3c80c769f2b