GNOME Bugzilla – Bug 697565
Using GOA in Flickr plugin
Last modified: 2013-05-28 15:10:30 UTC
Hey, some support for GOA in flickr plugin would be nice, wouldn't be? It definetily would, so here it is. I added two optional features (during compilation): --enable-flickr-goa-forced and --enable-flickr-goa. The forced thing means that plugin will take configuration only from GOA and any other configuration passed is ignored. (So with no GOA flickr account plugin initiation fails) The --enable-flickr-goa option behaves the old way (takes programmer's configuration and goes through it) but if boolean "flickr-enable-goa" is found in configuration and it's TRUE, then it tries to take configuration from GOA either. So using this option the only thing needed to allow plugin search in GOA is write this line into configuration: grl_config_set_boolean (config, "flickr-enable-goa", TRUE); This option was marked default in configure.ac When creating this patch I used source codes after patching to OAuth patch (https://bugzilla.gnome.org/show_bug.cgi?id=697175) because it searches for OAuth keys in GOA.
Created attachment 240956 [details] [review] patch for grilo-test-ui
Created attachment 240957 [details] [review] The patch for flickr plugin Sry, had some uploading troubles. Here's the patch
*** Bug 697564 has been marked as a duplicate of this bug. ***
I'll take a look during this week. Thanks for the patches!
Review of attachment 240957 [details] [review]: ::: configure.ac @@ +134,3 @@ PKG_CHECK_MODULES(OAUTH, oauth, HAVE_OAUTH=yes, HAVE_OAUTH=no) +PKG_CHECK_MODULES(GOA, goa-1.0, HAVE_GOA=yes, HAVE_GOA=no) GOA supports Flickr since 3.7.1. So it would be a good idea to check for the version. Something like this should work: PKG_CHECK_MODULES(GOA, [goa-1.0 >= 3.7.1], HAVE_GOA=yes, HAVE_GOA=no)
Created attachment 241567 [details] [review] Previous patch merged with proposed changes. Here's merged version of the patch as Debarshi proposed along with some aesthetical changes (no newlines in GRL_DEBUG, some line wraps, etc.). Also FLICKR_GOA_FORCED is now really forced (the goa configs is not concatenated to the original configs, but the original configs are replaced by the goa configs).
Created attachment 242125 [details] [review] The patch rebased against master Here's the same pach as the 241567 rebased against master
Review of attachment 242125 [details] [review]: In general, I find quite complex all the building options to enable GOA. And if you think in the future where other plugins could use GOA too (for instance, YouTube), it makes even more a mess all the options. I would reduce the options to just one: "--enable-goa". If user doesn't specify it, then it will detect if GOA is present; if so then support for GOA will be included. Else, not. And if the user specifies "--enable-goa" then it will raise an error if GOA is not present. Thus, Flickr will build with GOA support if GOA is enabled. And the same will happen for other plugins supporting GOA in the future. Now, that's about the building part. About how to pass the configurations. Either with GOA or without GOA, I would allow applications to pass the api-key, api-secret, api-token and api-token-secret. 1) If api-key and api-secret are passed, the plugin would use them to create the "public" Flickr source. 2) If api-token and api-token-secret are passed, the plugin would use them to create the "personal" Flickr source. 3) Besides that, if GOA is active, plugin will get all the personal accounts for Flickr and create the proper Flickr sources. Additional, if application passes no config to Flickr plugin, then I would use the GOA values to create the "public" Flickr source, and the "personal" sources. So summing up, the idea is that applications can use the Flickr plugin either if GOA is available or not. But, at the same time, if it's available, then it will get for free the personal Flickr content. ::: src/flickr/grl-flickr.c @@ +852,3 @@ +GList * +grl_flickr_get_goa_multiple_config (GrlPlugin *plugin) +{ Better let's move this function to the "utilities" section, and let's keep here the Grilo API functions. This makes easier to locate the functions when reading the code.
> And if > you think in the future where other plugins could use GOA too (for instance, > YouTube), it makes even more a mess all the options. Because of that I split this new patch into two parts: > I would reduce the options to just one: "--enable-goa". If user doesn't specify > it, then it will detect if GOA is present; if so then support for GOA will be > included. Else, not. And if the user specifies "--enable-goa" then it will > raise an error if GOA is not present. = the first part of the patch > 3) Besides that, if GOA is active, plugin will get all the personal accounts > for Flickr and create the proper Flickr sources. > > Additional, if application passes no config to Flickr plugin, then I would use > the GOA values to create the "public" Flickr source, and the "personal" > sources. = second part of the patch
Created attachment 242796 [details] [review] This patch adds --enable-goa option
Created attachment 242797 [details] [review] Include GOA into the flickr plugin.
Review of attachment 242797 [details] [review]: ::: configure.ac @@ +514,2 @@ AC_SUBST(DEPS_FLICKR_CFLAGS) +DEPS_FLICKR_LIBS="$DEPS_LIBS $XML_LIBS $GRLNET_LIBS $OAUTH_LIBS $GOA_LIBS" $GOA_LIBS and GOA_CFLAGS must be added only when GOA is enabled ::: src/flickr/flickr-oauth.h @@ +36,3 @@ +/* these are used to create public source when no configuration is given */ +#define DEFAULT_CONSUMER_KEY "ed00ad7e0869897506e23c0d18e34d01" +#define DEFAULT_CONSUMER_SECRET "ebd556dd187188b1" We shouldn't provide this keys. User should provide them, or use GOA instead (see comment in grl-flickr.c) ::: src/flickr/grl-flickr.c @@ +150,3 @@ bind_textdomain_codeset (GETTEXT_PACKAGE, "UTF-8"); + if (configs == NULL) { Reviewing the rest of the function, I think this should be moved after the "while (configs) { }" loop, and then create the public source from GOA only if GOA is active and "public_source_created == FALSE" @@ +160,3 @@ + + grl_config_set_api_key (public, DEFAULT_CONSUMER_KEY); + grl_config_set_api_secret (public, DEFAULT_CONSUMER_SECRET); Can't we get this two values from GOA directly? In grl_flickr_get_goa_multiple_config() function actually you are obtaining them.
(In reply to comment #12) > Review of attachment 242797 [details] [review]: > > ::: configure.ac > @@ +514,2 @@ > AC_SUBST(DEPS_FLICKR_CFLAGS) > +DEPS_FLICKR_LIBS="$DEPS_LIBS $XML_LIBS $GRLNET_LIBS $OAUTH_LIBS $GOA_LIBS" > > $GOA_LIBS and GOA_CFLAGS must be added only when GOA is enabled Initialise them to "" and use them instead, it's easier than making this conditional.
Created attachment 243204 [details] [review] Use GOA in the flickr plugin. (In reply to comment #13) > (In reply to comment #12) > > Review of attachment 242797 [details] [review] [details]: > > > > ::: configure.ac > > @@ +514,2 @@ > > AC_SUBST(DEPS_FLICKR_CFLAGS) > > +DEPS_FLICKR_LIBS="$DEPS_LIBS $XML_LIBS $GRLNET_LIBS $OAUTH_LIBS $GOA_LIBS" > > > > $GOA_LIBS and GOA_CFLAGS must be added only when GOA is enabled > > Initialise them to "" and use them instead, it's easier than making this > conditional. I used conditions, because if I would overwrote them, than other plugins couldn't use them. > + grl_config_set_api_key (public, DEFAULT_CONSUMER_KEY); > + grl_config_set_api_secret (public, DEFAULT_CONSUMER_SECRET); > > Can't we get this two values from GOA directly? In > grl_flickr_get_goa_multiple_config() function actually you are obtaining them. I modified grl_flickr_get_goa_multiple_config(), now it takes another parameter, which is boolean an if it's TRUE, than it creates even configuration for "public" flickr.
Created attachment 243205 [details] [review] Use GOA in the flickr plugin Removed useless #include "flickr-oauth.h" in grl-flickr.c
(In reply to comment #14) > I used conditions, because if I would overwrote them, than other plugins > couldn't use them. > The would be something like: PKG_CHECK(GOA, ...) if HAVE_GOA == No GOA_CFLAGS="" GOA_LIBS="" endif Of course, using the syntax for configure.ac :) Anyway, I'm fine with the conditional too.
(In reply to comment #14) > I used conditions, because if I would overwrote them, than other plugins > couldn't use them. > It would be something like: PKG_CHECK(GOA, ...) if HAVE_GOA == No GOA_CFLAGS="" GOA_LIBS="" endif Of course, using the syntax for configure.ac :) Anyway, I'm fine with the conditional too.
commit 2b97310957ded00aa98c84779e6572e577a926fa Author: Marek Chalupa <mchalupa@redhat.com> Date: Fri May 3 17:35:26 2013 +0200 flickr: use GOA to get user's accounts When compiled with --enable-goa, flickr will try to find additional configuration in GOA. When GOA is enabled the behavoiur is following: If no cofig is passed to the plugin -> create public source and personal sources from GOA (if at least one personal source is present) Otherwise use given config and then create personal sources from GOA too. https://bugzilla.gnome.org/show_bug.cgi?id=697565 configure.ac | 8 ++++++++ src/flickr/grl-flickr.c | 156 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 155 insertions(+), 9 deletions(-) commit 3929f808c6c51a136b1fcb0a7626975b27a1ce26 Author: Marek Chalupa <mchalupa@redhat.com> Date: Mon Apr 29 15:04:30 2013 +0200 build: add option --enable-goa When this option is on, macro GOA_ENABLED is defined to 1 if GOA is present in the system. Otherwise error message is raised. Default: auto https://bugzilla.gnome.org/show_bug.cgi?id=697565 configure.ac | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)