Bug 27669

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).

Other people's locations
========================

> +    if (!mPriv->requestedFeatures.contains(FeatureLocation)) {

Is requestedFeatures really the right member there?

> -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?

--------------------------------

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.

> 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).

> +    // 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.

> +PendingOperation *Connection::selfSelfLocationAccessControl(

I think you mean setSelf, not selfSelf

> +        warning().nospace() << "Getting channels failed with " <<
> +            reply.error().name() << ":" << reply.error().message();

"Getting location properties failed", surely...

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.

(In reply to comment #1)
> the bug I'm about to file

... is Bug #27752.

(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.

(In reply to comment #1)
> Please separate this into two branches

Or three branches. That works too :-)

Basic codegen
=============

review+ for andrunko/location.

Other people's locations
========================

> > +    if (!mPriv->requestedFeatures.contains(FeatureLocation)) {
> 
> Is requestedFeatures really the right member there?

I still think this is wrong.

> > -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 still think this is wrong, and I'd be inclined to define and use a Tp::ContactLocationPtr (or you could turn ContactLocation into a value type with a refcounted private struct, like QString, if you prefer).

(In reply to comment #4)
> (In reply to comment #1)
> > Please separate this into two branches
> 
> Or three branches. That works too :-)
> 
> Basic codegen
> =============
> 
> review+ for andrunko/location.
> 
> Other people's locations
> ========================
> 
> > > +    if (!mPriv->requestedFeatures.contains(FeatureLocation)) {
> > 
> > Is requestedFeatures really the right member there?
> 
> I still think this is wrong.
No, it is not

> 
> > > -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 still think this is wrong, and I'd be inclined to define and use a
> Tp::ContactLocationPtr (or you could turn ContactLocation into a value type
> with a refcounted private struct, like QString, if you prefer).
I wouldn't go this route. It's normal in Qt do this also as returning a QObject* for example. We also decided to go this route for ContactCapabilities so I believe we should follow what we already have.

Merged location and others-location with minor changes. It will be in next release 0.3.2

The own-location will be merged later when the spec is updated. Leaving bug open until we merge own-location support

Nothing to review here right now, then. Thanks!

Looks like we lost own-location code. The mentioned branch is not available and the code does not exists in master.

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

Any comments? Andre, it would be really awesome if you'll push the code somewhere.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-qt/issues/4.