Bug 68658

(In reply to comment #0)
> each application of geoclue requires about 100 lines of code just to obtain
> the the longitude and latitude data, which can not be okey!
> 
> Therefore please provide an additional simplified api to get the data
> easily, like corresponding to:
> 
> var monitor = new GClue.LocationMonitor ();
> monitor.location_changed.connect ((location) => {
>   double lat = location.latitude;
>   double lon = location.longitude;
>   // do something with it...
> });

I'm always all for simplification but how would this monitor deal with authentication. The idea behind requiring each app to get its own client and start it is that geoclue asks the user or check against a whitelist if app should be given access to location information. Also start call is needed because in future app will be able to tell geoclue what precision of geolocation they need so geoclue only tries to provide the requested precision.

(In reply to comment #1)
> (In reply to comment #0)
> > each application of geoclue requires about 100 lines of code just to obtain
> > the the longitude and latitude data, which can not be okey!
> > 
> > Therefore please provide an additional simplified api to get the data
> > easily, like corresponding to:
> > 
> > var monitor = new GClue.LocationMonitor ();
> > monitor.location_changed.connect ((location) => {
> >   double lat = location.latitude;
> >   double lon = location.longitude;
> >   // do something with it...
> > });
> 
> I'm always all for simplification but how would this monitor deal with
> authentication. The idea behind requiring each app to get its own client and
> start it is that geoclue asks the user or check against a whitelist if app
> should be given access to location information. Also start call is needed
> because in future app will be able to tell geoclue what precision of
> geolocation they need so geoclue only tries to provide the requested
> precision.
the same way you would deal it with the dbus call, but just inside a location monitor class

yeah, right, I forgot to add "monitor.start ();"

(In reply to comment #2)
> (In reply to comment #1)
> > (In reply to comment #0)
> > > each application of geoclue requires about 100 lines of code just to obtain
> > > the the longitude and latitude data, which can not be okey!
> > > 
> > > Therefore please provide an additional simplified api to get the data
> > > easily, like corresponding to:
> > > 
> > > var monitor = new GClue.LocationMonitor ();
> > > monitor.location_changed.connect ((location) => {
> > >   double lat = location.latitude;
> > >   double lon = location.longitude;
> > >   // do something with it...
> > > });
> > 
> > I'm always all for simplification but how would this monitor deal with
> > authentication. The idea behind requiring each app to get its own client and
> > start it is that geoclue asks the user or check against a whitelist if app
> > should be given access to location information. Also start call is needed
> > because in future app will be able to tell geoclue what precision of
> > geolocation they need so geoclue only tries to provide the requested
> > precision.
> the same way you would deal it with the dbus call, but just inside a
> location monitor class
> 
> yeah, right, I forgot to add "monitor.start ();"

So it would seem that your idea of simplification means merging both client and location objects together? I don't think killing OO concept for a bit of simplification is worth it.

Is it really 100 lines in vala btw? I'd imagine it be much simpler with yield syntax etc.

As I said on https://bugzilla.gnome.org/show_bug.cgi?id=706627#c11 there is currently way too much code to write just to track the location.

As long as the helper code (LocationMonitor or something) is in the app process that shouldn't change anything for authentication checking.

The precision isn't an issue either, we could just pass it as an argument to the monitor constructor.

I wrote a simple helper for Empathy:
http://cgit.collabora.com/git/user/cassidy/empathy/log/?h=geoclue-helper

Why couldn't we integrate something like that in geoclue2 itself?

Ok, Since all app developers are asking for this, so how about we provide a client library with an API like this (sorry for Vala syntax but hard to denote OO API in C):

public class GClue.Location {
    public double latitude { get; }
    public double longitude { get; }
    public double accuracy { get; }
    public string description { get; }
}

public class GClue.Client {
    public GClue.Location location { get; }

    public signal void location_updated (GClue.Location old,
                                         GClue.Location new);

    public async Client () throw GLib.Error;
    // Create the client and also start it
    public async Client.started () throw GLib.Error;
    public async void start () throw GLib.Error;
}

Here is how the simple Vala app would look like:

public void main (string args[]) {
    try {
        var client = yield new Client.started ();

        g_print ("You seem to be within %f meters of %f.%f\n",
                  client.location.accuracy,
                  client.location.latitude,
                  client.location.longitude);
    } catch (GLib.Error e) {
        g_print ("Failed to retrieve your location from Geoclue: %s\n", e.message);
    }
}

Would that be simple enough for apps?

Yeah that's exactly what I'd need. I'd be happy to change my helper's API to match this one and contribute it.

(In reply to comment #7)
> Yeah that's exactly what I'd need. I'd be happy to change my helper's API to
> match this one and contribute it.

Cool! Please do. I'm hoping that Bastien had only objection to providing the generated dbus wrapper as it wasn't very useful to apps (it really wasn't).

(In reply to comment #6)
> Ok, Since all app developers are asking for this, so how about we provide a
> client library with an API like this (sorry for Vala syntax but hard to
> denote OO API in C):

We should probably pick another name than GClueClient as it would conflict with the object generated by gdbus-codegen.
Same for GClueLocation.

Also, shouldn't we have API to set the distance threshold?

(In reply to comment #6)
> Ok, Since all app developers are asking for this, so how about we provide a
> client library with an API like this (sorry for Vala syntax but hard to
> denote OO API in C):
> 
> public class GClue.Location {
>     public double latitude { get; }
>     public double longitude { get; }
>     public double accuracy { get; }
>     public string description { get; }
> }
> 
> public class GClue.Client {
>     public GClue.Location location { get; }
> 
>     public signal void location_updated (GClue.Location old,
>                                          GClue.Location new);
> 
>     public async Client () throw GLib.Error;
>     // Create the client and also start it
>     public async Client.started () throw GLib.Error;
>     public async void start () throw GLib.Error;
> }
> 
> Here is how the simple Vala app would look like:
> 
> public void main (string args[]) {
>     try {
>         var client = yield new Client.started ();
> 
>         g_print ("You seem to be within %f meters of %f.%f\n",
>                   client.location.accuracy,
>                   client.location.latitude,
>                   client.location.longitude);
>     } catch (GLib.Error e) {
>         g_print ("Failed to retrieve your location from Geoclue: %s\n",
> e.message);
>     }
> }
> 
> Would that be simple enough for apps?

Are you thinking of changing the dbus interface?

I do not think, that is simple. It should be simple as possible but not simpler, like Einstein has said once. So, I think only one class with a location property should be enough. One can have an additional signal or one could just connect to the notify property signal to monitor the changes and if necessary with a start() function to start monitoring, if there is no other possibility to start the service, but I believe, that there is.

(In reply to comment #9)
> (In reply to comment #6)
> > Ok, Since all app developers are asking for this, so how about we provide a
> > client library with an API like this (sorry for Vala syntax but hard to
> > denote OO API in C):
> 
> We should probably pick another name than GClueClient as it would conflict
> with the object generated by gdbus-codegen.
> Same for GClueLocation.

I don't think we need to use gdbus-codegen here, it doesn't help much as you can see from changes to where-am-i.c when we ditched the generated library: http://cgit.freedesktop.org/geoclue/commit/?id=609e8293f19be189b05ff4df505cae9b0aac0eb4

(In reply to comment #10)
> Also, shouldn't we have API to set the distance threshold?

Sure thing. I remembered it last night when going to sleep. :)

(In reply to comment #11)
> (In reply to comment #6)
> > Ok, Since all app developers are asking for this, so how about we provide a
> > client library with an API like this (sorry for Vala syntax but hard to
> > denote OO API in C):
> > 
> > public class GClue.Location {
> >     public double latitude { get; }
> >     public double longitude { get; }
> >     public double accuracy { get; }
> >     public string description { get; }
> > }
> > 
> > public class GClue.Client {
> >     public GClue.Location location { get; }
> > 
> >     public signal void location_updated (GClue.Location old,
> >                                          GClue.Location new);
> > 
> >     public async Client () throw GLib.Error;
> >     // Create the client and also start it
> >     public async Client.started () throw GLib.Error;
> >     public async void start () throw GLib.Error;
> > }
> > 
> > Here is how the simple Vala app would look like:
> > 
> > public void main (string args[]) {
> >     try {
> >         var client = yield new Client.started ();
> > 
> >         g_print ("You seem to be within %f meters of %f.%f\n",
> >                   client.location.accuracy,
> >                   client.location.latitude,
> >                   client.location.longitude);
> >     } catch (GLib.Error e) {
> >         g_print ("Failed to retrieve your location from Geoclue: %s\n",
> > e.message);
> >     }
> > }
> > 
> > Would that be simple enough for apps?
> 
> Are you thinking of changing the dbus interface?
> 
> I do not think, that is simple. It should be simple as possible but not
> simpler, like Einstein has said once. So, I think only one class with a
> location property should be enough. One can have an additional signal or one
> could just connect to the notify property signal to monitor the changes and
> if necessary with a start() function to start monitoring, if there is no
> other possibility to start the service, but I believe, that there is.

Kalev had a similar thought:

<kalev> 1) if there's going to be a simpler client api, maybe it turns out it's also possible to simplify the DBus API?
<kalev> 2) the client library should be LGPL
<zeenix> 2) sure thing!
<zeenix> 1) I thought of that but then we'll need a low-level api and a high-level api
<zeenix> and I'd try to reduce API as much as possible
<zeenix> so if you want simple api, you use the lib. If you want more low-level one, use dbus directly
<kalev> what kind of apps would even need the low level API?
<kalev> if the there's a difficulty making the permissions work with simple api, that's a good enough reason for me
<kalev> but otherwise, I can't see why anyone would want anything beyond the location info
<kalev> I am in love with the systemd dbus apis, so amazingly simple
<kalev> org.freedesktop.timedate1: http://paste.fedoraproject.org/36385/37806527
<zeenix> kalev: there is a process involved that we must retain
<zeenix> apps need to get their own client
<zeenix> for that, they need to make a call to something
<zeenix> and that something is the manager
<zeenix> also from dbus you don't actually get a Location instance but rather path to it
<zeenix> so you got three objects and you need to create proxies for them
<zeenix> and w/ dbus its better to do that async
<zeenix> hence the dbus API can't be made as easy as the library 
<zeenix> anyways, as long as there is an easy way, i really dont see a problem

I think it could probably be slightly simpler but:
- Start/Stop are required. We cannot be tied to the lifetime of the app, as it might only need the current location for a very short period of time (eg. one page in the web browser) and we don't want to eat battery just because the web browser is long running
- We need one client per, well, client. We don't want to leak information from one client's request to another client.
- I'm not really happy with the location object, and that could certainly be made easier. It's supposed to be an exact match to GeocodeLocation in the client code. I don't know whether the D-Bus XML allows for constants, but in this case we could have a single "(ddds)" variant where we use Location objects, and add helper code in geocode-glib to handle that variant (geocode_location_new_for_variant()?). We also need to make sure that the location changes are handled atomically (if the Location property changes, you don't want to receive a signal for the longitude, then for the latitude).

To sum up, make locations easier to handle, make client creation easier.

Here is the helper code I wrote for Empathy in order to have this merged for 3.10. Feel free to re-use some of it if you want to: https://git.gnome.org/browse/empathy/tree/libempathy-gtk/empathy-geoclue-helper.c

(In reply to comment #16)
> Here is the helper code I wrote for Empathy in order to have this merged for
> 3.10. Feel free to re-use some of it if you want to:
> https://git.gnome.org/browse/empathy/tree/libempathy-gtk/empathy-geoclue-
> helper.c

I was more like expecting patches to geolcue. :(

I finally got around to fixing this. \o/ The added library not only provide gdbus-codegen generated API but also some convenience API that makes things really easy.

API docs: http://www.freedesktop.org/software/geoclue/docs/libgeoclue/

Reverse log of relevant patches:

commit: 96f815b059a5f3326ec60b02c0c491739a669816

    dbus: Put each interface in it's own file
    
    This probably makes sense anyway but it will be useful when we add a
    convenience library in a following patch so that code for each interface
    could be generated in a separate module to be able to have separate
    chapters for generated API in the docs.
    
commit: 1004038ab17ea59b6813c922e4cd7e2d480e4791

    build: Rename libgeoclue.la to libgeoclue-internal.la
    
    We are about to add a new public library with the name libgeoclue so
    let's rename this internal intermediate library to avoid any
    confusions/conflicts.
    
commit: f6d386e123c02ef506c52ecbc7cf501c7ac82c53

    public-api: Relicense to LGPLv2
    
    In a following patch, we'll add a library and install this existing
    public API along with it. It'll be easier if all public API is under the
    same licence and besides all public API should be LGPL anyway.

commit: 3ec8eb50b66f54f09304aeac1930b592996524fd

    Add convenience library
    
    Currently this is just gdbus-codegen generated code.
    
commit: 7d1289e 7d1289e2147a0bde59f21720c29afadf46032ce9

    lib: Add helper API for easy client creation
    
    Add API that makes creation of new clients slightly easier by hiding
    communication with Manager object.
    
commit: af336ae4682c6f698474c73a4c19037cdb1cb13d

    lib: Add helper API for quickly fetching location
    
    Add API that makes it dead simple to fetch the location.

commit: c9ef691d1756b17d3456f230818aba642c521b98

    demo: Adapt where-am-i to use convenience API
    
    This reduces the code by 50%, thus nicely demonstrating how easy the new
    API makes it to get location.
    
commit: 6bf3a136ff0babd34648c51658d9fd6cdaf280dc

    lib,build: Build introspection for library
    
commit: 2bf812b3a4ec76e32d49e47e2158aad186b55c8c

    lib: Generate docs