GNOME Bugzilla – Bug 707569
Reimplement the Flickr source using Grilo
Last modified: 2014-02-18 10:16:01 UTC
The Flickr integration for gnome-photos uses Grilo for the protocol / API implementation. We should reimplement the Background panel's BgFlickrSource using the same stack so that the same code paths are used by both gnome-control-center and gnome-photos, and we reduce the dependency footprint.
The Flickr source was supposed to show "related" sources, eg. best/most recent pictures from friends. So this would require us only showing the flickr source when the user has Flickr setup in GOA, and not in other circumstances.
(In reply to comment #1) > So this would require us only showing the flickr source > when the user has Flickr setup in GOA, and not in other circumstances. Yes, that is the idea.
Created attachment 254265 [details] [review] background: Remove uses of libsocialweb
Created attachment 254266 [details] [review] background: Don't access possibly invalid object
Created attachment 254267 [details] [review] background: Implement the Flickr source using Grilo
Review of attachment 254265 [details] [review]: Does this actually compile and work (eg. not crash)? If so, that's fine to commit, but I'd rather the cc-background-chooser-dialog.c ifdef removals were changes to "#if 0" so that the UI isn't actually enabled.
Review of attachment 254266 [details] [review]: This needs to be committed to master.
Review of attachment 254267 [details] [review]: ::: panels/background/bg-flickr-source.h @@ +71,3 @@ GType bg_flickr_source_get_type (void) G_GNUC_CONST; +BgFlickrSource *bg_flickr_source_new (GList *accounts); I would make this fallible, using GInitable... ::: panels/background/cc-background-chooser-dialog.c @@ +296,3 @@ priv->colors_source = bg_colors_source_new (); + + if (client != NULL) ...And do all this GOA work inside the flickr source.
Review of attachment 254265 [details] [review]: Marking as after freeze, I doubt this will go in for 3.10 though you can try once the code itself is sorted.
(In reply to comment #6) > Review of attachment 254265 [details] [review]: > > Does this actually compile and work (eg. not crash)? Yes, it works. > If so, that's fine to commit, but I'd rather the cc-background-chooser-dialog.c > ifdef removals were changes to "#if 0" so that the UI isn't actually enabled. I kept this as a separate commit mainly for the purposes of the review. We can squash it.
(In reply to comment #9) > Review of attachment 254265 [details] [review]: > > Marking as after freeze, I doubt this will go in for 3.10 though you can try > once the code itself is sorted. Allan specifically wanted it last week after we realized that gnome-photos is not yet ready for prime time (no import from camera, no search) and we need one other core module to leverage GOA's Flickr support in some way.
Created attachment 254479 [details] [review] background: Remove uses of libsocialweb Ue #if 0 instead of removing the #ifdefs.
Created attachment 254481 [details] [review] background: Implement the Flickr source using Grilo Make BgFlickrSource creation fallible using GInitable and move the GOA accounts checking code inside it.
Created attachment 254490 [details] [review] background: Remove uses of libsocialweb Fix conflicts caused by bug 707602
Review of attachment 254481 [details] [review]: Showing only a limited number of pictures isn't great. Ideally we would load more on demand as the user scrolls, but for now I guess we need need to somehow let the user know that we are capping the amount of pictures. And perhaps let the user open up gnome-photos or something? But if we were going to open gnome-photos in the first place this whole Flickr source wouldn't be needed... I think this needs some more design input. Unfortunately, that will likely push this feature out of 3.10 :-( ::: panels/background/bg-flickr-source.c @@ +75,3 @@ + g_warning ("Failed to load image: %s", error->message); + goto out; + } If we were cancelled we also want to goto out @@ +89,3 @@ + "shading", G_DESKTOP_BACKGROUND_SHADING_SOLID, + "source_url", url, + NULL); Don't you also have to set "flags", CC_BACKGROUND_ITEM_HAS_URI | CC_BACKGROUND_ITEM_HAS_SHADING, ? @@ +122,3 @@ + g_free (path); + goto out; + } If we were cancelled we also want to goto out @@ +206,3 @@ + } + + if (priv->count == 60) This should at least be a #define. @@ +325,3 @@ + error = NULL; + if (!grl_registry_load_plugin_by_id (registry, "grl-flickr", &error)) + g_error ("%s", error->message); g_error() isn't acceptable IMO. I think a better approach would be to record that we couldn't initialize the grilo plugin here and then on _inititable_init() error out yielding an invalid BgFlickrSource instance which then allows us to skip adding the Flickr radio button to the UI. ::: panels/background/bg-flickr-source.h @@ +25,3 @@ #define _BG_FLICKR_SOURCE_H +#include <glib.h> Not needed ::: panels/background/cc-background-chooser-dialog.c @@ +284,3 @@ priv->pictures_source = bg_pictures_source_new (); priv->colors_source = bg_colors_source_new (); + priv->flickr_source = bg_flickr_source_new (NULL); This needs to be cleared in dispose() @@ +337,3 @@ g_object_set_data (G_OBJECT (button), "source", priv->colors_source); + if (priv->flickr_source != NULL) It can't ever be NULL, right? It might be invalid though, if you agree with my suggestion above
Review of attachment 254490 [details] [review]: Ok, consider it a-c-n if/when the other patch is accepted
After talking to the Allan and others in #gnome-design, the current plan is: - to not have a separate view for Flickr because we do not discriminate among local and cloud sources - only show the N most recent photos from Flickr
Created attachment 268807 [details] [review] background: Remove uses of libsocialweb and BgFlickrSource
Created attachment 268936 [details] [review] background: Remove redundant and unused parameter from add_single_file
Created attachment 268937 [details] [review] background: Minor clean up
Created attachment 268939 [details] [review] background: Add support for Flickr pictures
Review of attachment 268807 [details] [review]: Looks good.
Review of attachment 268936 [details] [review]: Not quite, it's only needed for "xml" wallpapers, not for all local/native files. I would always set it instead.
Review of attachment 268937 [details] [review]: Fine.
Review of attachment 268939 [details] [review]: I'd prefer the grilo specific bits be moved in a separate file, which could have a simpler API as well. For example, an "available" property on that separate object that will trigger adding/removing the remote photos would be good. Send a signal in that object with the title/url of the item when the account is added/removed (toggled on/off). ::: panels/background/bg-pictures-source.c @@ +58,3 @@ + + GList *accounts; + guint count; count of what? @@ +341,3 @@ + } + + /* since we were not cancelled, we can now cast user_data That's not needed. @@ +424,2 @@ item = cc_background_item_new (uri); + flags |= CC_BACKGROUND_ITEM_HAS_SHADING; Why remove the shading flag and still set it below? @@ +959,3 @@ + caps = grl_source_get_caps (source, GRL_OP_BROWSE); + options = grl_operation_options_new (caps); + grl_operation_options_set_flags (options, GRL_RESOLVE_FAST_ONLY); Add a filter to only get images, and set the number of items at 50 on the search options.
Created attachment 269011 [details] [review] background: Remove redundant and unused parameter from add_single_file
Review of attachment 269011 [details] [review]: Looks good.
(In reply to comment #25) > Review of attachment 268939 [details] [review]: > For example, an "available" property on that separate object that will trigger > adding/removing the remote photos would be good. > Send a signal in that object with the title/url of the item when the account is > added/removed (toggled on/off). We can ignore the possibility of the account being toggled while the chooser-dialog is displayed because it is impossible to use the Online Accounts and Background panel simultaneously. > @@ +424,2 @@ > item = cc_background_item_new (uri); > + flags |= CC_BACKGROUND_ITEM_HAS_SHADING; > > Why remove the shading flag and still set it below? |= would set the flag, not unset it. Right?
Review of attachment 269011 [details] [review]: I made a mistake. ::: panels/background/bg-pictures-source.c @@ +336,3 @@ + needs_download = FALSE; + source_uri = g_strdup (uri); + flags |= CC_BACKGROUND_ITEM_HAS_URI; This will break the build. The flags variable leaked in while rebasing from the subsequent patch. @@ +351,3 @@ "placement", G_DESKTOP_BACKGROUND_STYLE_ZOOM, "modified", mtime, + "needs-download", needs_download, This can just be !is_native.
Created attachment 269015 [details] [review] background: Remove redundant and unused parameter from add_single_file This is better.
Review of attachment 269015 [details] [review]: Looks good
Created attachment 269025 [details] [review] background: Add support for Flickr pictures
*** Bug 636869 has been marked as a duplicate of this bug. ***
Created attachment 269367 [details] [review] background: Add CcBackgroundGriloMiner
Created attachment 269368 [details] [review] background: Add support for Flickr pictures
Review of attachment 269367 [details] [review]: Rest looks fine. Just make sure it doesn't blow up if there's a Flickr account configured and the grl-flickr plugin isn't there. ::: panels/background/cc-background-grilo-miner.c @@ +297,3 @@ + G_TYPE_FROM_CLASS (klass), + G_SIGNAL_RUN_LAST, + 0, /* class_offset */ You can remove those comments. @@ +323,3 @@ + +void +cc_background_grilo_miner_start (CcBackgroundGriloMiner *self) What happens when I call start and the grl-flickr plugin couldn't be loaded?
Review of attachment 269368 [details] [review]: ::: configure.ac @@ +123,3 @@ + gdk-pixbuf-2.0 >= $GDKPIXBUF_REQUIRED_VERSION + goa-1.0 >= $GOA_REQUIRED_VERSION + grilo-0.2 >= $GRILO_REQUIRED_VERSION) You'll need to move the configure.ac changes to the other patch (or the Makefile.am changes to this one) so that the build doesn't break in between the 2 (which might be useful for debugging). ::: panels/background/bg-pictures-source.c @@ +825,3 @@ + file = g_file_new_for_uri (uri); + g_object_set_data_full (G_OBJECT (file), "grl-media", g_object_ref (media), g_object_unref); + g_file_query_info_async (file, Why do that? The remote HTTP server is unlikely to give us much useful information, and it's a round-trip to the server to find that out.
(In reply to comment #36) > Review of attachment 269367 [details] [review]: > > Rest looks fine. Just make sure it doesn't blow up if there's a Flickr account > configured and the grl-flickr plugin isn't there. Yes, I checked. > ::: panels/background/cc-background-grilo-miner.c > @@ +297,3 @@ > + G_TYPE_FROM_CLASS (klass), > + G_SIGNAL_RUN_LAST, > + 0, /* class_offset */ > > You can remove those comments. Ok. > @@ +323,3 @@ > + > +void > +cc_background_grilo_miner_start (CcBackgroundGriloMiner *self) > > What happens when I call start and the grl-flickr plugin couldn't be loaded? You get a WARNING on the console that the plugin could not be loaded and query_online_source will never be called leading to no media-found signal being emitted.
Created attachment 269455 [details] [review] background: Add CcBackgroundGriloMiner Moved the configure.ac changes from the other patch to this one.
(In reply to comment #37) > Review of attachment 269368 [details] [review]: > > ::: configure.ac > @@ +123,3 @@ > + gdk-pixbuf-2.0 >= $GDKPIXBUF_REQUIRED_VERSION > + goa-1.0 >= $GOA_REQUIRED_VERSION > + grilo-0.2 >= $GRILO_REQUIRED_VERSION) > > You'll need to move the configure.ac changes to the other patch (or the > Makefile.am changes to this one) so that the build doesn't break in between the > 2 (which might be useful for debugging). Done. I moved the configure.ac hunk to the other patch. > ::: panels/background/bg-pictures-source.c > @@ +825,3 @@ > + file = g_file_new_for_uri (uri); > + g_object_set_data_full (G_OBJECT (file), "grl-media", g_object_ref (media), > g_object_unref); > + g_file_query_info_async (file, > > Why do that? The remote HTTP server is unlikely to give us much useful > information, and it's a round-trip to the server to find that out. I was doing it to get the mime type because grl_media_get_mime was coming out as NULL, but you are right. We can work around it using g_content_type_guess on the URI.
(In reply to comment #40) > (In reply to comment #37) <snip> > > Why do that? The remote HTTP server is unlikely to give us much useful > > information, and it's a round-trip to the server to find that out. > > I was doing it to get the mime type because grl_media_get_mime was coming out > as NULL, but you are right. We can work around it using g_content_type_guess on > the URI. Or fix the grilo plugin to set the mime-type?
Created attachment 269515 [details] [review] background: Add support for Flickr pictures
Created attachment 269516 [details] [review] background: Add support for Flickr pictures
Review of attachment 269515 [details] [review]: ::: panels/background/bg-pictures-source.c @@ +530,3 @@ + /* grl_media_get_mime does not work with the Flickr plugin */ + uri = g_file_get_uri (file); + content_type = g_content_type_guess (uri, NULL, 0, NULL); Looks good apart from that.
Created attachment 269522 [details] [review] background: Add support for Flickr pictures Changed it to use grl_media_get_mime on the assumption that we will have a Grilo release with the feature.