GNOME Bugzilla – Bug 729740
Add Maps feature and use it in Facebook provider
Last modified: 2014-05-19 12:44:14 UTC
Created attachment 276095 [details] [review] Add Maps feature and use it in Facebook provider Hello, I want to propose a patch that add the Maps feature and implements the usage of that feature in the Facebook provider. This enhancement is part of a GSoC 2014 project [1] Thanks! [1] https://wiki.gnome.org/Outreach/SummerOfCode/2014/Projects/DamianNohales_MapsFoursquareFacebook
Review of attachment 276095 [details] [review]: Thanks for the patch, Damián! It looks great apart from a few minor things. ::: src/goabackend/goafacebookprovider.c @@ +142,3 @@ + "friends_status," + "friends_photos," + "publish_actions"; I can understand that you might need user_status and publish_actions. Do you really need friends_status and friends_photos? I could not find anything about them on https://developers.facebook.com/docs/facebook-login/permissions/ @@ -370,3 @@ - "ChatEnabled"); - } - This is an unrelated change. Lets do it in a separate patch. @@ -405,3 @@ - g_object_unref (chat); - if (account != NULL) - g_object_unref (account); Ditto. This is an unrelated change. Lets do it in a separate patch.
Hi Debarshi, thanks for the review. (In reply to comment #1) > Review of attachment 276095 [details] [review]: > > Thanks for the patch, Damián! It looks great apart from a few minor things. > > ::: src/goabackend/goafacebookprovider.c > @@ +142,3 @@ > + "friends_status," > + "friends_photos," > + "publish_actions"; > > I can understand that you might need user_status and publish_actions. Do you > really need friends_status and friends_photos? I could not find anything about > them on https://developers.facebook.com/docs/facebook-login/permissions/ > Maps probably will need get the friends latest check-in... but it seems that if user's friends have the user_status permission in the application, I can get that information. I'm really not sure, Facebook API is absurdly confusing in regarding permissions, I left that permissions there "just in case". Maybe would be better to take those away until I resume the research about friends check-in. > @@ -370,3 @@ > - "ChatEnabled"); > - } > - > > This is an unrelated change. Lets do it in a separate patch. > > @@ -405,3 @@ > - g_object_unref (chat); > - if (account != NULL) > - g_object_unref (account); > > Ditto. This is an unrelated change. Lets do it in a separate patch. Ok. Thanks!
(In reply to comment #2) > > ::: src/goabackend/goafacebookprovider.c > > @@ +142,3 @@ > > + "friends_status," > > + "friends_photos," > > + "publish_actions"; > > > > I can understand that you might need user_status and publish_actions. Do you > > really need friends_status and friends_photos? I could not find anything about > > them on https://developers.facebook.com/docs/facebook-login/permissions/ > > > > Maps probably will need get the friends latest check-in... but it seems that if > user's friends have the user_status permission in the application, I can get > that information. I'm really not sure, Facebook API is absurdly confusing in > regarding permissions, I left that permissions there "just in case". Maybe > would be better to take those away until I resume the research about friends > check-in. Yes, the FB API can be confusing. I had the same experience when working with libgfbgraph [1] and Facebook photos. It is good to be conservative about the scopes we use. The less we use, the more trustworthy we are in the user's eyes. For starters we can atleast remove "friends_photos", if not "friends_status", because that does not look related to gnome-maps. We can always tweak the scopes during the 3.13 cycle as you progress. [1] http://git.gnome.org/browse/libgfbgraph
Created attachment 276460 [details] [review] Add Maps feature and use it in Facebook provider - v2
By the way, Facebook just published version 2.0 of their API: https://developers.facebook.com/docs/apps/changelog We should try not to use any version 1.0 stuff in newly introduced code.
(In reply to comment #5) > By the way, Facebook just published version 2.0 of their API: > https://developers.facebook.com/docs/apps/changelog We should try not to use > any version 1.0 stuff in newly introduced code. Yes... I'm aware of that, I think that is good time to update libgfbgraph which is the library that I'm going to use, actually this library performs v1 API calls. Anyways, the two API are really similar and I'm performing check-in with no deprecated v1 API calls, which are available in v2 and are essentially the same. Migration should be planned and I'm going to try to do something for this cycle, but as always, the time has the last word ;)
Review of attachment 276460 [details] [review]: Thanks! This looks great. I forgot to tell you before, but we need to add some glue so that the documentation for the new org.gnome.OnlineAccounts.Maps interface shows up in devhelp. Other than that you should put the URL to this bug in the commit message.
Created attachment 276766 [details] [review] facebook: Add support for maps I took the liberty to add the missing glue for the documentation and fixed the commit message.
Thanks for working on this, Damián. I am looking forward to the integration between Maps and Facebook.