GNOME Bugzilla – Bug 729837
Add Foursquare support
Last modified: 2014-12-18 13:14:44 UTC
Hello, As my GSoC project [1] needs Foursquare API support to be accomplished, one of the task would be to add the Foursquare provider to GOA. Thanks! [1] https://wiki.gnome.org/Outreach/SummerOfCode/2014/Projects/DamianNohales_MapsFoursquareFacebook
Created attachment 276201 [details] [review] DON'T APPLY: Add Foursquare support v1 This add Foursquare support, I've tested the provider and it seems to be working, but it should not be applied since there is no default Foursquare Client ID because there is no GNOME official Foursquare app yet, also, the images should be changed to don't be so ugly :) . So, this patch is only for code review purpose. Thanks!
Any update on this? The check-in integration in GNOME Maps that includes Foursquare Check-In support is happening at #731113. Thanks!
Review of attachment 276201 [details] [review]: Looks good, Damián. For some reason Git thinks that the patch is corrupt and asks "Did you hand edit your patch?", which is why I could not actually try it out. Maybe try creating a Git branch somewhere? ::: configure.ac @@ +333,3 @@ + [Enable Foursquare provider])], + [], + [enable_foursquare=yes]) Lets keep enable_foursquare=no until bug 731113 is finished. @@ +340,3 @@ + []) +if test "$with_foursquare_client_id" = ""; then + with_foursquare_client_id="" Eventually we will need a default client ID. Do you want to own it? Or do the Maps maintainers want to own it? Or should I do it? Can there be multiple co-owners of it? Whoever ends up owning it should fill in https://wiki.gnome.org/Projects/GnomeOnlineAccounts/Providers ::: po/POTFILES.in @@ +16,3 @@ src/goabackend/goaowncloudprovider.c src/goabackend/goapocketprovider.c +src/goabackend/goafoursquareprovider.c This should move up so that the file is sorted alphabetically. ::: src/goabackend/goafoursquareprovider.c @@ +1,3 @@ +/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */ +/* + * Copyright (C) 2014 Red Hat, Inc. Are you sure that the copyright belongs to Red Hat? You wrote it, so you should have the copyright. @@ +125,3 @@ +{ + return 1; +} You don't need this when the provider is new because the default implementation will set it to 0, which is good enough. You will need this later if you change the scopes used by the provider. @@ +337,3 @@ + g_free (id); + return ret; +} Do you really need to implement is_deny_node? If Foursquare is sending OAuth2 compliant error messages, then we should not need it any more. See bug 709570 ::: src/goabackend/goafoursquareprovider.h @@ +1,3 @@ +/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */ +/* + * Copyright (C) 2014 Red Hat, Inc. Same thing about the copyright notice. ::: src/goabackend/goaprovider.c @@ +788,3 @@ +#ifdef GOA_FOURSQUARE_ENABLED + type = GOA_TYPE_FOURSQUARE_PROVIDER; +#endif This part has changed a bit in current master. Now you need to add it to the ordered_builtins_map array.
(In reply to comment #3) > Review of attachment 276201 [details] [review]: > > Looks good, Damián. For some reason Git thinks that the patch is corrupt and > asks "Did you hand edit your patch?", which is why I could not actually try it > out. Maybe try creating a Git branch somewhere? That's my fault, I actually edited the patch by hand to delete the the client_id from configure.ac. I didn't know about the details of diff format so I carelessly edited the patch, sorry about that. > > ::: configure.ac > @@ +333,3 @@ > + [Enable Foursquare provider])], > + [], > + [enable_foursquare=yes]) > > Lets keep enable_foursquare=no until bug 731113 is finished. > > @@ +340,3 @@ > + []) > +if test "$with_foursquare_client_id" = ""; then > + with_foursquare_client_id="" > > Eventually we will need a default client ID. Do you want to own it? Or do the > Maps maintainers want to own it? Or should I do it? Can there be multiple > co-owners of it? Whoever ends up owning it should fill in > https://wiki.gnome.org/Projects/GnomeOnlineAccounts/Providers No problem, I'm going to create the app. > > ::: po/POTFILES.in > @@ +16,3 @@ > src/goabackend/goaowncloudprovider.c > src/goabackend/goapocketprovider.c > +src/goabackend/goafoursquareprovider.c > > This should move up so that the file is sorted alphabetically. > > ::: src/goabackend/goafoursquareprovider.c > @@ +1,3 @@ > +/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */ > +/* > + * Copyright (C) 2014 Red Hat, Inc. > > Are you sure that the copyright belongs to Red Hat? You wrote it, so you should > have the copyright. > > @@ +125,3 @@ > +{ > + return 1; > +} > > You don't need this when the provider is new because the default implementation > will set it to 0, which is good enough. You will need this later if you change > the scopes used by the provider. > > @@ +337,3 @@ > + g_free (id); > + return ret; > +} > > Do you really need to implement is_deny_node? If Foursquare is sending OAuth2 > compliant error messages, then we should not need it any more. See bug 709570 > > ::: src/goabackend/goafoursquareprovider.h > @@ +1,3 @@ > +/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */ > +/* > + * Copyright (C) 2014 Red Hat, Inc. > > Same thing about the copyright notice. > > ::: src/goabackend/goaprovider.c > @@ +788,3 @@ > +#ifdef GOA_FOURSQUARE_ENABLED > + type = GOA_TYPE_FOURSQUARE_PROVIDER; > +#endif > > This part has changed a bit in current master. Now you need to add it to the > ordered_builtins_map array. All right, I'll make the changes in the following days. Thanks for the review!
(In reply to comment #3) > Review of attachment 276201 [details] [review]: > > ::: configure.ac > @@ +333,3 @@ > + [Enable Foursquare provider])], > + [], > + [enable_foursquare=yes]) > > Lets keep enable_foursquare=no until bug 731113 is finished. > > @@ +340,3 @@ > + []) > +if test "$with_foursquare_client_id" = ""; then > + with_foursquare_client_id="" > > Eventually we will need a default client ID. Do you want to own it? Or do the > Maps maintainers want to own it? Or should I do it? Can there be multiple > co-owners of it? Whoever ends up owning it should fill in > https://wiki.gnome.org/Projects/GnomeOnlineAccounts/Providers Done. client ID: NNPSDPQKEZRSPSMWQOQ3EXQKET5TMZ0KQTQ4KUU0WHKLO0YG.
Created attachment 278362 [details] [review] Add Foursquare support - v2 Plus the corrections pointed in the review, this version also adds the gtk-doc sections for the GoaFoursquareProvider.
Review of attachment 278362 [details] [review]: Thanks for the new patch, Damián. Mostly looks good. A few comments below, and it would be nice to add https://developer.foursquare.com/overview/auth to README. ::: gnome-online-accounts.doap @@ +16,3 @@ providers for Google, ownCloud, Facebook, Flickr, Windows Live, + Pocket, Foursquare, Microsoft Exchange, IMAP/SMTP, Jabber, SIP and + Kerberos. Nice. I like your attention to detail. :) ::: src/goabackend/goafoursquareprovider.c @@ +136,3 @@ + +gboolean +get_use_mobile_browser (GoaOAuth2Provider *provider) You forgot the static. :) @@ +139,3 @@ +{ + /* Foursquare needs to identify the browser as mobile to + * provide a better interface for GOA */ This comment is stating the obvious because the documentation for goa_oauth2_provider_get_use_mobile_browser already says what it is for. I would remove it. More importantly we don't we use display=touch as documented [1] instead of this hack? I don't know what is the difference between touch and wap, but touch sounds correct and works. [1] https://developer.foursquare.com/overview/auth @@ +163,3 @@ + JsonParser *parser; + JsonObject *json_object; + JsonObject *json_contact_object; Unused variable. @@ +189,3 @@ + if (!rest_proxy_call_sync (call, error)) + { + printf("1\n%d\n%s\n", rest_proxy_call_get_status_code (call), rest_proxy_call_get_status_message (call)); These debug statements can be removed, right? @@ +194,3 @@ + if (rest_proxy_call_get_status_code (call) != 200) + { + printf("2\n"); Ditto. @@ +210,3 @@ + &identity_error)) + { + printf("3\n"); Ditto. @@ +222,3 @@ + } + + /* Get response.user member */ These comments are also stating the obvious in my opinion. @@ +297,3 @@ + g_free (presentation_identity); + if (parser != NULL) + g_object_unref (parser); Use g_clear_object. I know that we use if (...) g_object_unref in parts of the code, but those were written before g_clear_object came into existence and we should slowly migrate them. @@ +328,3 @@ + + g_object_get (element, "type", &element_type, NULL); + if (g_strcmp0 (element_type, "text") != 0) It should be "email", not "text". ;-) @@ +359,3 @@ + gboolean ret = FALSE; + + account = NULL; Put this where you defined 'account', since that is what you have done with the other variables. Another one of those cases where the coding style is a bit mixed up in the code base.
Created attachment 278398 [details] [review] Add Foursquare support - v3 I fixed all the minor issues. We need to decide on the 'display=touch vs faking a mobile UA' issue. I slightly prefer using what is documented by the web service.
(In reply to comment #5) Hey, Zeeshan, > Done. client ID: NNPSDPQKEZRSPSMWQOQ3EXQKET5TMZ0KQTQ4KUU0WHKLO0YG. Would it be possible to create a GNOME Foundation account on Foursquare? Currently it prominently shows your name against GNOME in the user's settings (see attachment). It might be better to have some thing that is a little less personal. It looks to me that Foursquare actually encourages creating non-personal accounts for these cases after reading the first paragraph in https://developer.foursquare.com/overview/auth I know that some providers frown upon non-human accounts (eg., Facebook), but if it is possible in this case it would be nice to do so.
Created attachment 278399 [details] Foursquare GNOME screenshot
> @@ +139,3 @@ > +{ > + /* Foursquare needs to identify the browser as mobile to > + * provide a better interface for GOA */ > > This comment is stating the obvious because the documentation for > goa_oauth2_provider_get_use_mobile_browser already says what it is for. I would > remove it. > > More importantly we don't we use display=touch as documented [1] instead of > this hack? I don't know what is the difference between touch and wap, but touch > sounds correct and works. > > [1] https://developer.foursquare.com/overview/auth > Ah, I think I missed that part of the documentation (or maybe read and forgotten :( ). Yes, display=touch has the same effect than using the user-agent hack. As a side note, the documentation does not recommend to force an specific display unless we are using "webpopup" option. I really don't know why. I've tried "webpopup" too, it looks better (without the brand header) and works too, but the documentation says that will open a popup for us, redirects the parent window and then close the popup (so, it works and looks better, but does not seem correct to our case and perhaps only coincidentally works) In conclusión, I prefer to use the display=touch option, it seems safer. > @@ +163,3 @@ > + JsonParser *parser; > + JsonObject *json_object; > + JsonObject *json_contact_object; > > Unused variable. > > @@ +189,3 @@ > + if (!rest_proxy_call_sync (call, error)) > + { > + printf("1\n%d\n%s\n", rest_proxy_call_get_status_code (call), > rest_proxy_call_get_status_message (call)); > > These debug statements can be removed, right? > > @@ +194,3 @@ > + if (rest_proxy_call_get_status_code (call) != 200) > + { > + printf("2\n"); > > Ditto. > > @@ +210,3 @@ > + &identity_error)) > + { > + printf("3\n"); > > Ditto. > Oh my God, what a shame! :( > @@ +222,3 @@ > + } > + > + /* Get response.user member */ > > These comments are also stating the obvious in my opinion. > > @@ +297,3 @@ > + g_free (presentation_identity); > + if (parser != NULL) > + g_object_unref (parser); > > Use g_clear_object. I know that we use if (...) g_object_unref in parts of the > code, but those were written before g_clear_object came into existence and we > should slowly migrate them. > > @@ +328,3 @@ > + > + g_object_get (element, "type", &element_type, NULL); > + if (g_strcmp0 (element_type, "text") != 0) > > It should be "email", not "text". ;-) > > @@ +359,3 @@ > + gboolean ret = FALSE; > + > + account = NULL; > > Put this where you defined 'account', since that is what you have done with the > other variables. Another one of those cases where the coding style is a bit > mixed up in the code base.
(In reply to comment #9) > (In reply to comment #5) > > Hey, Zeeshan, > > > Done. client ID: NNPSDPQKEZRSPSMWQOQ3EXQKET5TMZ0KQTQ4KUU0WHKLO0YG. > > Would it be possible to create a GNOME Foundation account on Foursquare? > Currently it prominently shows your name against GNOME in the user's settings > (see attachment). It might be better to have some thing that is a little less > personal. It looks to me that Foursquare actually encourages creating > non-personal accounts for these cases after reading the first paragraph in > https://developer.foursquare.com/overview/auth > > I know that some providers frown upon non-human accounts (eg., Facebook), but > if it is possible in this case it would be nice to do so. A GNOME member should create a page in Foursquare [1] with a proper foundation e-mail address. [1] http://business.foursquare.com/brands/
(In reply to comment #9) > (In reply to comment #5) > > Hey, Zeeshan, > > > Done. client ID: NNPSDPQKEZRSPSMWQOQ3EXQKET5TMZ0KQTQ4KUU0WHKLO0YG. > > Would it be possible to create a GNOME Foundation account on Foursquare? > Currently it prominently shows your name against GNOME in the user's settings > (see attachment). It might be better to have some thing that is a little less > personal. It looks to me that Foursquare actually encourages creating > non-personal accounts for these cases after reading the first paragraph in > https://developer.foursquare.com/overview/auth > > I know that some providers frown upon non-human accounts (eg., Facebook), but > if it is possible in this case it would be nice to do so. I need gnome twitter account password from aday for this and I keep forgetting to get it from him when I meet. :( I'll try to remember next I meet him but I don't its something that should block this feature. In the absence of any reminders in past month or so I totally forgot this is blocking and now we are already in feature freeeze. :( Could you get an exception for this and get this in?
(In reply to comment #13) > (In reply to comment #9) > > (In reply to comment #5) > > > > Hey, Zeeshan, > > > > > Done. client ID: NNPSDPQKEZRSPSMWQOQ3EXQKET5TMZ0KQTQ4KUU0WHKLO0YG. > > > > Would it be possible to create a GNOME Foundation account on Foursquare? > > Currently it prominently shows your name against GNOME in the user's settings > > (see attachment). It might be better to have some thing that is a little less > > personal. It looks to me that Foursquare actually encourages creating > > non-personal accounts for these cases after reading the first paragraph in > > https://developer.foursquare.com/overview/auth > > > > I know that some providers frown upon non-human accounts (eg., Facebook), but > > if it is possible in this case it would be nice to do so. > > I need gnome twitter account password from aday for this and I keep forgetting > to get it from him when I meet. :( I'll try to remember next I meet him but I > don't its something that should block this feature. To be clear, I meant that for now we can live with my name appearing. And since that will annoy people, I and Allan will be forced to get this done sooner than later. :)
(In reply to comment #13) > (In reply to comment #9) > > (In reply to comment #5) > > > > Hey, Zeeshan, > > > > > Done. client ID: NNPSDPQKEZRSPSMWQOQ3EXQKET5TMZ0KQTQ4KUU0WHKLO0YG. > > > > Would it be possible to create a GNOME Foundation account on Foursquare? > > Currently it prominently shows your name against GNOME in the user's settings > > (see attachment). It might be better to have some thing that is a little less > > personal. It looks to me that Foursquare actually encourages creating > > non-personal accounts for these cases after reading the first paragraph in > > https://developer.foursquare.com/overview/auth Finally done. :) client ID: MBNU2NES5HASNDQJ25YPFGG2UGRZHPI3IYTNJGE0KIWT2HCF
(In reply to comment #15) > Finally done. :) > > client ID: MBNU2NES5HASNDQJ25YPFGG2UGRZHPI3IYTNJGE0KIWT2HCF Awesome! Thanks, Zeeshan. The Maps's check-in patches were merged today, so I need just one thing, because right now is throwing errors. Please check the app settings and set the "Redirect URI(s)" field to "https://localhost/". Please ping me when you are done so I can try it.
(In reply to comment #16) > (In reply to comment #15) > > Finally done. :) > > > > client ID: MBNU2NES5HASNDQJ25YPFGG2UGRZHPI3IYTNJGE0KIWT2HCF > > Awesome! Thanks, Zeeshan. > > The Maps's check-in patches were merged today, so I need just one thing, > because right now is throwing errors. Please check the app settings and set the > "Redirect URI(s)" field to "https://localhost/". > > Please ping me when you are done so I can try it. Done!
Created attachment 292661 [details] [review] Add Foursquare support
So, I think this is ready to be merged. Zeeshan, just a few more comments for the app: - Change the app website to https://www.gnome.org/ instead of https://download.gnome.org/. The second one is not very user friendly and the user can see the link in every check-in he/she make, so is quite accessible. - Add icon and logos, a section "Icons and Images" is in the sidebar when editing the app.
(In reply to comment #19) > So, I think this is ready to be merged. > > Zeeshan, just a few more comments for the app: > > - Change the app website to https://www.gnome.org/ instead of > https://download.gnome.org/. The second one is not very user friendly and the > user can see the link in every check-in he/she make, so is quite accessible. Well, its suppposed to be a 'Download link' so thats why I set it to that but yeah if its going to be presented to users each time they check in, former is better. > - Add icon and logos, a section "Icons and Images" is in the sidebar when > editing the app. I have set all the icons/logos now.
(In reply to comment #20) > (In reply to comment #19) > > So, I think this is ready to be merged. > > > > Zeeshan, just a few more comments for the app: > > > > - Change the app website to https://www.gnome.org/ instead of > > https://download.gnome.org/. The second one is not very user friendly and the > > user can see the link in every check-in he/she make, so is quite accessible. > > Well, its suppposed to be a 'Download link' so thats why I set it to that but > yeah if its going to be presented to users each time they check in, former is > better. Yeah, that's actually a confusing field name. > > > - Add icon and logos, a section "Icons and Images" is in the sidebar when > > editing the app. > > I have set all the icons/logos now. The logo should be squared (width = height), if not, the icon is shown wrong [1][2]. Maybe you could add some transparent borders to the left and right side, and center the image horizontally. [1] http://i.imgur.com/CFeN063.png [2] http://i.imgur.com/1a5X7Q2.png
(In reply to comment #21) > (In reply to comment #20) > > (In reply to comment #19) > > > So, I think this is ready to be merged. > > > > > > Zeeshan, just a few more comments for the app: > > > > > > - Change the app website to https://www.gnome.org/ instead of > > > https://download.gnome.org/. The second one is not very user friendly and the > > > user can see the link in every check-in he/she make, so is quite accessible. > > > > Well, its suppposed to be a 'Download link' so thats why I set it to that but > > yeah if its going to be presented to users each time they check in, former is > > better. > > Yeah, that's actually a confusing field name. > > > > > > - Add icon and logos, a section "Icons and Images" is in the sidebar when > > > editing the app. > > > > I have set all the icons/logos now. > > The logo should be squared (width = height), if not, the icon is shown wrong > [1][2]. Maybe you could add some transparent borders to the left and right > side, and center the image horizontally. I just set the icon file you sent me. It looked right in the preview at least.
Review of attachment 292661 [details] [review]: ::: configure.ac @@ +363,3 @@ + []) +if test "$with_foursquare_client_id" = ""; then + with_foursquare_client_id="NNPSDPQKEZRSPSMWQOQ3EXQKET5TMZ0KQTQ4KUU0WHKLO0YG" This doesn't look like the new client ID that Zeeshan created. :) ::: src/goabackend/goafoursquareprovider.c @@ +344,3 @@ + connection, + just_added, + error)) The alignment is off. @@ +388,3 @@ +get_use_mobile_browser (GoaOAuth2Provider *provider) +{ + return TRUE; The mobile variant of the page is causing webkitgtk-2.4.7 to crash on my F21 system, while the non-mobile variant works. I guess, it doesn't crash for you, right? I will file a WebKit bug and see if it can be fixed.
Created attachment 292964 [details] [review] Add Foursquare I fixed the client ID and the alignment problem. I have kept the mobile variant of the page. If the WebKit bug is not resolved in time we can always switch to the non-mobile page later.
Thanks for all your work on this, Damián & Zeeshan. This will be in the 3.15.3 release.
Review of attachment 292661 [details] [review]: ::: configure.ac @@ +363,3 @@ + []) +if test "$with_foursquare_client_id" = ""; then + with_foursquare_client_id="NNPSDPQKEZRSPSMWQOQ3EXQKET5TMZ0KQTQ4KUU0WHKLO0YG" Damn!! I forgot to commit!! I tested it with the Zeeshan app, I promise :) ::: src/goabackend/goafoursquareprovider.c @@ +388,3 @@ +get_use_mobile_browser (GoaOAuth2Provider *provider) +{ + return TRUE; Hmmm... I also have webkitgtk-2.4.7 in my ArchLinux system. Actually, I got a segmentation fault on the second time I select Foursquare to add an account, to clarify, the steps are: - Open G-C-C in online-accounts panel - Click add and then Foursquare - Cancel (close the window) - Click add and then Foursquare again - Seg fault Maybe a problem with GOA on how deal with WebKit?