After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 729740 - Add Maps feature and use it in Facebook provider
Add Maps feature and use it in Facebook provider
Status: RESOLVED FIXED
Product: gnome-online-accounts
Classification: Core
Component: general
3.13.x
Other All
: Normal enhancement
: ---
Assigned To: GNOME Online Accounts maintainer(s)
GNOME Online Accounts maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2014-05-07 18:21 UTC by Damián Nohales
Modified: 2014-05-19 12:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add Maps feature and use it in Facebook provider (6.76 KB, patch)
2014-05-07 18:21 UTC, Damián Nohales
needs-work Details | Review
Add Maps feature and use it in Facebook provider - v2 (5.52 KB, patch)
2014-05-13 15:14 UTC, Damián Nohales
needs-work Details | Review
facebook: Add support for maps (9.27 KB, patch)
2014-05-19 12:42 UTC, Debarshi Ray
committed Details | Review

Description Damián Nohales 2014-05-07 18:21:37 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
Comment 1 Debarshi Ray 2014-05-09 12:53:57 UTC
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.
Comment 2 Damián Nohales 2014-05-09 18:56:06 UTC
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!
Comment 3 Debarshi Ray 2014-05-13 08:32:40 UTC
(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
Comment 4 Damián Nohales 2014-05-13 15:14:50 UTC
Created attachment 276460 [details] [review]
Add Maps feature and use it in Facebook provider - v2
Comment 5 Debarshi Ray 2014-05-14 09:43:36 UTC
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.
Comment 6 Damián Nohales 2014-05-14 14:09:32 UTC
(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 ;)
Comment 7 Debarshi Ray 2014-05-19 12:40:06 UTC
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.
Comment 8 Debarshi Ray 2014-05-19 12:42:55 UTC
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.
Comment 9 Debarshi Ray 2014-05-19 12:44:14 UTC
Thanks for working on this, Damián. I am looking forward to the integration between Maps and Facebook.