GNOME Bugzilla – Bug 708943
shows "No Pictures found" and then shows pictures
Last modified: 2014-02-03 15:02:52 UTC
When I first open the Pictures selection for Background I get a label that no pictures were found and then it is replaced with pictures. If we aren't sure then we should show a spinner or something I guess.
Wait for bug 709243 to be sorted out to avoid lots of conflicts.
Created attachment 256369 [details] [review] background: Warn if the pictures or cache directory are missing
Created attachment 256370 [details] [review] background: Notify when the sources are not busy anymore
Created attachment 256371 [details] [review] background: Use the same method to set the initial view and toggle
Created attachment 256372 [details] [review] background: Remove unused variable
Created attachment 256373 [details] [review] background: Set the size request only on the parent
Created attachment 256374 [details] [review] background: Use a spinner to indicate a busy source
Created attachment 256378 [details] [review] background: Add a delay to the sources for testing This is only for testing! I couldn't find a better way to test than to add a 10 second delay to the sources. I tried ionice(1) and while it did slow down the file operations, it did not work reliably on all invocations of the application.
Created attachment 256380 [details] Screenshot of the spinner
Review of attachment 256369 [details] [review]: It shouldn't warn when the cache or pictures directories are missing. If pictures is missing, it should use the fallback ($HOME), and if the cache directory is missing, it should just create it.
Review of attachment 256370 [details] [review]: I think it's fine populating the list store on the fly, though we could wait until a couple of items are processed so it's not completely empty. Furthermore, it would be good to be able to start/stop the processing of certains "tabs"/sources. That way, you'd process the visible tab in priority. That would require actually profiling the dialog... ::: panels/background/bg-pictures-source.c @@ +140,3 @@ +possibly_unset_busy (BgPicturesSource *bg_source) +{ + if (bg_source->priv->pending_enumerate_children > 0) You'll need to use atomic operations, as those could be increased and decreased from threads. I would make the decision inside bg-source.[ch], rather than doing so inside each of the sources.
Review of attachment 256371 [details] [review]: That looks fine.
Review of attachment 256372 [details] [review]: What's 79ec684fa4912a1e5bff17ce11b036cef4a298aa ? Another commit in your patchset?
Review of attachment 256373 [details] [review]: ::: panels/background/cc-background-chooser-dialog.c @@ +295,3 @@ gtk_grid_set_column_spacing (GTK_GRID (grid), 0); gtk_container_add (GTK_CONTAINER (vbox), grid); + gtk_widget_set_size_request (grid, 860, 550); Note that this is too big for smaller screens (there's a bug opened about that, make a note in the commit or the code that it doesn't fix the problem).
Review of attachment 256374 [details] [review]: The spinner should be inside the icon view, making it look like the list is populating.
Review of attachment 256378 [details] [review]: Those are fine for testing, but I don't think they represent reality. I'd really like to see profiling information instead. Add a few sleep() when processing the images so each one takes a long time to process, as on a slow machine?
(In reply to comment #13) > Review of attachment 256372 [details] [review]: > > What's 79ec684fa4912a1e5bff17ce11b036cef4a298aa ? Another commit in your > patchset? commit 79ec684fa4912a1e5bff17ce11b036cef4a298aa Author: William Jon McCann <jmccann@redhat.com> Date: Tue May 22 11:34:20 2012 -0400 background: New background panel design Implement a new design for the wallpaper selection. https://bugzilla.gnome.org/show_bug.cgi?id=676539
Created attachment 256404 [details] [review] background: Be robust against missing directories, and warn otherwise
Comment on attachment 256371 [details] [review] background: Use the same method to set the initial view and toggle Thanks for the review.
Review of attachment 256404 [details] [review]: ::: panels/background/bg-pictures-source.c @@ +612,2 @@ pictures_path = g_get_user_special_dir (G_USER_DIRECTORY_PICTURES); + if (!g_file_test (pictures_path, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR)) Do a g_mkdir_with_parents of the pictures_path instead.
Created attachment 256453 [details] [review] background: Be robust against missing directories, and warn otherwise
Review of attachment 256453 [details] [review]: Ay.
Comment on attachment 256373 [details] [review] background: Set the size request only on the parent Pushed after editing the commit message as suggested.
(In reply to comment #11) > Review of attachment 256370 [details] [review]: > > I think it's fine populating the list store on the fly, though we could wait > until a couple of items are processed so it's not completely empty. I changed it so that it is busy till the first item is added. > ::: panels/background/bg-pictures-source.c > @@ +140,3 @@ > +possibly_unset_busy (BgPicturesSource *bg_source) > +{ > + if (bg_source->priv->pending_enumerate_children > 0) > > You'll need to use atomic operations, as those could be increased and decreased > from threads. We are not using threads in BgPicturesSource. > I would make the decision inside bg-source.[ch], rather than doing so inside > each of the sources. I tried doing that, initially, by checking the number of items in the store from BgSource. However it broke down if the source really is empty. In that case it is tricky to distinguish whether the store is empty because we are still trying to fill it up, or because there is nothing to put into it. Doing it in each of the children wasn't a bad alternative because I don't expect us to have a large number of sources (FWIW, the designers want to put Flickr items in BgPicturesSource), and it is only the pictures source that is doing something fancy. BgColorsSource is never going to be busy and tracking BgWallpapersSource was trivial.
Created attachment 256473 [details] [review] background: Notify when the sources are not busy anymore
Created attachment 256476 [details] perf data from test-chooser-dialog
(In reply to comment #15) > Review of attachment 256374 [details] [review]: > > The spinner should be inside the icon view, making it look like the list is > populating. FWIW, I chose to use the same pattern that is used in some of the apps. Maybe we want to change the apps too? Or not?
Created attachment 256481 [details] [review] background: Serialize thumbnail creation for pictures
Review of attachment 256481 [details] [review]: ::: panels/background/bg-pictures-source.c @@ +397,3 @@ + return; + + data = g_queue_peek_head (bg_source->priv->thumb_queue); You don't want to be using peek here, you should pop the item, and use it. Free it when the operation has finished.
Created attachment 266531 [details] [review] background: Consolidate exit path
Created attachment 266532 [details] [review] background: Serialize thumbnail creation for pictures
(In reply to comment #29) > Review of attachment 256481 [details] [review]: > > ::: panels/background/bg-pictures-source.c > @@ +397,3 @@ > + return; > + > + data = g_queue_peek_head (bg_source->priv->thumb_queue); > > You don't want to be using peek here, you should pop the item, and use it. Free > it when the operation has finished. Fixed.
Review of attachment 266531 [details] [review]: Really don't see the point of this patch. ::: panels/background/bg-pictures-source.c @@ +189,2 @@ out: + g_clear_error (&error); That's not consolidating, the error is only set in a single branch, where you already had a g_error_free(). @@ +213,3 @@ } + goto out; Here you'd handle the error free'ing in branch. @@ +230,3 @@ + out: + g_clear_error (&error); + g_clear_object (&stream); The stream is already non-NULL here.
Review of attachment 266532 [details] [review]: As I mentioned earlier, I'm not too happy about any of the "performance" patches, as there's absolutely no performance data to compare it to. I doubt that blocking on decoding pictures in a single thread will make things appear faster on screen. Go grab those very very high-resolution images at: http://sourceforge.net/projects/jpeg/files/jpeg2000_images/jpeg2000_images/ and see whether a single-threaded image decoding is a good idea :)
(In reply to comment #34) > Review of attachment 266532 [details] [review]: > Really don't see the point of this patch. You would, if you saw it in the context of the 2nd patch.
(In reply to comment #35) > (In reply to comment #34) > > Review of attachment 266532 [details] [review] [details]: > > > Really don't see the point of this patch. > > You would, if you saw it in the context of the 2nd patch. Right, in which case you need to mention in the commit log that the changes are necessary for when you add more code. Even then, I'd just merge both patches.
Created attachment 267045 [details] [review] background: Add a placeholder icon before creating thumbnails As discussed in #control-center in GIMPNet last week.
Created attachment 267047 [details] [review] background: Add a placeholder icon before creating thumbnails Do not crash if the icon_info came out as NULL.
Created attachment 267050 [details] [review] background: Add a placeholder icon before creating thumbnails Silly typo in the commit message.
Review of attachment 267050 [details] [review]: Can you attach a screenshot and check with the designers whether that looks ok? ::: panels/background/bg-pictures-source.c @@ +355,3 @@ + -1); + + g_object_set_data_full (G_OBJECT (item), You can't do that. iter are only valid until the next operation done on the iter. You need to use tree row references (gtk_tree_row_reference_new ()...).
Created attachment 267057 [details] [review] background: Add a placeholder icon before creating thumbnails Also check before using the row_ref -- it can be NULL. We were not doing that in remove_placeholder.
Created attachment 267058 [details] Screenshot of the placeholder icons
Review of attachment 267057 [details] [review]: Looks good.
Comment on attachment 267057 [details] [review] background: Add a placeholder icon before creating thumbnails Thanks for the review.
The original problem of "no pictures" coming and going should now be fixed. If we want to make further performance improvements in future, we will use a separate bug.
Comment on attachment 266532 [details] [review] background: Serialize thumbnail creation for pictures We will explore this in future if we want to make further performance improvements.