[Bug 27669] Implement Location interface support

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=27669

Andre Moreira Magalhaes <andrunko at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|http://git.collabora.co.uk/ |
                   |?p=user/andrunko/telepathy- |
                   |qt4.git;a=shortlog;h=refs/h |
                   |eads/location               |
  Status Whiteboard|review-                     |
           Keywords|                            |patch

--- Comment #3 from Andre Moreira Magalhaes <andrunko at gmail.com> 2010-04-20 14:11:19 PDT ---
(In reply to comment #1)
> Please separate this into two branches, one for other contacts' locations
> (which is quite easy) and one for setting our own location (which is much
> harder, and could benefit from some spec work).
> 
Split the branch into 3, the location branch that contains the basic stuff
(autogen) needed by own-location and others-location branches.
Please review per branch so we can merge one even if the other is still
pending.

http://git.collabora.co.uk/?p=user/andrunko/telepathy-qt4.git;a=shortlog;h=refs/heads/location
http://git.collabora.co.uk/?p=user/andrunko/telepathy-qt4.git;a=shortlog;h=refs/heads/others-location
http://git.collabora.co.uk/?p=user/andrunko/telepathy-qt4.git;a=shortlog;h=refs/heads/own-location

> Other people's locations
> ========================
> 
> > +    if (!mPriv->requestedFeatures.contains(FeatureLocation)) {
>
> Is requestedFeatures really the right member there?
Yep, we are checking if the user requested the FeatureLocation to be used.

> 
> > -QVariantMap Contact::location() const
> > +ContactLocation *Contact::location() const
> 
> This seems to return a pointer that's only valid as long as the Contact is. Is
> that appropriate? It should at least be documented.
> 
> It should probably either be a const pointer that's documented to have the same
> validity as the Contact, or a refcounted Tp::ContactLocationPtr?
I did the same as for ContactCapabilities. Other way we would have to have copy
constructor ... in ContactLocation which is not desirable.


> --------------------------------
> 
> Our own location
> ================
> 
> > + * This is useful if the user wants to obscure their exact location
> > + * by reducing the precision or accuracy, instead of relying on the
> > + * connection manager to do so.
> 
> This isn't true - you call setSelfLocation regardless of whether you want to
> fuzz your location or not. How about this?
> 
>  * Set the user's location, to be advertised to other contacts.
>  *
>  * The connection manager will advertise this as precisely as
>  * possible, so if you want to tell contacts an obscured or
>  * reduced-precision version of the user's location, you must
>  * reduce the precision yourself.
>  *
>  * Clients that interact with more than one connection should advertise the
> same
>  * reduced-accuracy location to all of them, so that contacts cannot obtain an
>  * undesirably accurate location by assuming that random errors have been added
>  * and averaging the locations advertised on multiple connections.
Fixed

> > Also the name FeatureSelfLocation may be misleading as there is no info about
> > the current user self location retrieved, but only the access control.
> > Suggestions are welcome.
> 
> Perhaps FeatureLocationSetting? When we've enhanced the spec to have a CanSet
> flag (see the spec bug I'm about to file), this feature could fail to prepare
> on Google Talk accounts (which don't have PEP, so Gabble can't advertise
> locations on them).
Changed to FeatureLocatinSetting.

> > +    // TODO How to know when location access control changed?
> 
> You're right, there should be a change notification signal in tp-spec for this
> (I'll file a bug). In the meantime, telepathy-qt4 should respond to Set() by
> changing the cached value.
Not done yet, waiting for spec changes

> > +PendingOperation *Connection::selfSelfLocationAccessControl(
> 
> I think you mean setSelf, not selfSelf
Fixed :D

> > +        warning().nospace() << "Getting channels failed with " <<
> > +            reply.error().name() << ":" << reply.error().message();
> 
> "Getting location properties failed", surely...
Fixed :D

> I'm not sure that having separate setters for the access control type and the
> location is the right model; since geolocation is such sensitive information,
> we should perhaps set the access control and the location at the same time?
> 
> In particular, the first SetLocation on a Connection is specified to set the
> access control to an empty whitelist if possible (though I don't know whether
> Gabble actually allows this); if you want anyone to actually see your location,
> you'll need to set an access control model.
> 
> There should also be a flag for "can set location", for XMPP connections that
> don't have PEP (notably Google Talk). I'll include that in the bug.
> 
> I think it'd probably be fair to make telepathy-qt4 only support setting
> Location (with high-level API at least) if the connection is new enough to have
> the enhanced API from the bug I'm about to file.

I will wait till the changes to spec are in place in order to finish
own-location. In the meantime review others-location and location branches in
my repo.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.