(Mass changing milestones; please search for this phrase to delete the bug spam.)

e-d-s provides EContactGeo, so we can easily support this now.

Any progress on this? Having location API on FolksIndividual would make things easier in Empathy.

(In reply to comment #3)
> Any progress on this? Having location API on FolksIndividual would make things
> easier in Empathy.

Nope, none to speak of.

Timezone data would be good too (see: http://blogs.gnome.org/wjjt/2012/05/11/maps-and-clocks-and-contact-locations/).

Just to note: in the EDS backend, this would be implemented using the GEO vCard property.

Created attachment 235511 [details] [review]
infrastructure extensions + EDS

Review of attachment 235511 [details] [review]:

A good start. I think the Location API needs some more discussion.

::: backends/eds/lib/edsf-persona-store.vala
@@ +36,3 @@
+ * and typecase into E.ContactGeo -
+ * this is reasonably safe, because the struct is part of the
+ * EDS ABI and unlikely to change.

Is it possible to fix the EDS bindings? If so, can we have a bug number referenced here please?

s/typecase/typecast/

@@ +41,3 @@
+{
+        public double latitude;
+        public double longitude;

These should use 2-space indenting.

@@ +2247,3 @@
+      else
+        {
+          MyContactGeo? geo = new MyContactGeo();

Missing space before ‘()’.

@@ +2250,3 @@
+          geo.latitude = location.latitude;
+          geo.longitude = location.longitude;
+          contact.set (ContactField.GEO, (E.ContactGeo)geo);

Missing space after the typecast.

::: backends/eds/lib/edsf-persona.vala
@@ +2112,3 @@
+          if (this._location == null ||
+              this._location.latitude != _location.latitude ||
+              this._location.longitude != _location.longitude)

This (latitude != latitude || …) logic should be wrapped up in a Location.equal() method.

@@ +2117,3 @@
+                {
+                  this._location.latitude = _location.latitude;
+                  this._location.longitude = _location.longitude;

What’s the purpose of this? Why not create a new Location?

::: folks/individual.vala
@@ +2328,3 @@
+        }, (a, b) =>
+        {
+          /* TODO (?): pick the "better" location information. For now, pick more or less randomly. */

This should be implemented before the patch is committed.

@@ +2339,3 @@
+            }
+
+          if (new_location != this.location)

Does this comparison make sense? At the moment, I think it’s just a pointer comparison, so a notification will almost always be emitted, even if the coordinates are unchanged.

Perhaps the Location class should gain a ‘accuracy’ field which gives a radius around the latitude/longitude which contains the contact. A new ‘equal’ method on the Location class could then compare locations, returning true if their error circles overlapped?

::: folks/location-details.vala
@@ +22,3 @@
+
+/**
+ * A location. Typically latidue and longitude will

s/latidue/latitude/

@@ +32,3 @@
+{
+  /**
+   * The latitude.

These two fields need a ‘@since’ line each.

@@ +40,3 @@
+ public double longitude;
+
+ public Location(double latitude, double longitude)

This method needs documentation. Also missing a space before the opening ‘(’.

@@ +58,3 @@
+{
+  /**
+   * The current location of the contact.

Please add something stating what a null location value means.

@@ +65,3 @@
+
+  /**
+   * Change the contact's location.

Might want to rephrase this so it doesn’t sound like calling the method will teleport the person to somewhere new. :-P
Perhaps “Update the contact’s currently advertised location.”?

@@ +74,3 @@
+   * @param location the contact's location
+   * @throws PropertyError if setting the location failed
+   * @since 0.6.2

0.6.2? :-)

@@ +80,3 @@
+      /* Default implementation. */
+      throw new PropertyError.NOT_WRITEABLE (
+          _("Location is not writeable on this contact."));

This file needs listing in POTFILES.in because of the translatable string.

Created attachment 236576 [details] [review]
revised solution, without EDS ContactGeo workaround

(In reply to comment #8)
> ::: backends/eds/lib/edsf-persona-store.vala
> @@ +36,3 @@
> + * and typecase into E.ContactGeo -
> + * this is reasonably safe, because the struct is part of the
> + * EDS ABI and unlikely to change.
> 
> Is it possible to fix the EDS bindings? If so, can we have a bug number
> referenced here please?

I submitted a fix which was included in EDS master. As you depend on that in 0.9.x already, I removed the MyContactGeo workaround.

> @@ +2117,3 @@
> +                {
> +                  this._location.latitude = _location.latitude;
> +                  this._location.longitude = _location.longitude;
> 
> What’s the purpose of this? Why not create a new Location?

Performance and memory fragmentation. I need to write the two values either way, so why should I allocate a completely new instance? I've added comment in the code to explain that.

> ::: folks/individual.vala
> @@ +2328,3 @@
> +        }, (a, b) =>
> +        {
> +          /* TODO (?): pick the "better" location information. For now, pick
> more or less randomly. */
> 
> This should be implemented before the patch is committed.

As discussed before, this is a bit out of scope for me ATM. I hope that's okay.

One would have to look at other sources for location to determine how folks should choose the latest value, and how often it should update.

> @@ +2339,3 @@
> +            }
> +
> +          if (new_location != this.location)
> 
> Does this comparison make sense? At the moment, I think it’s just a pointer
> comparison, so a notification will almost always be emitted, even if the
> coordinates are unchanged.

The comparison detects when a location gets added or removed. I've also added a check for the actual value changes.

> Perhaps the Location class should gain a ‘accuracy’ field which gives a radius
> around the latitude/longitude which contains the contact. A new ‘equal’ method
> on the Location class could then compare locations, returning true if their
> error circles overlapped?

This is part of the questions which still need to be determined. Is it worth updating the location if the contact only moved a few centimeters? I don't know.

At the moment, the only source of location information is EDS, and there every update is assumed to be intentional and thus applied to the persona and individual.

> @@ +32,3 @@
> +{
> +  /**
> +   * The latitude.
> 
> These two fields need a ‘@since’ line each.

Really? The whole class is new and has a "@since UNRELEASED" remark (in the new revision of the patch). To avoid chatter, I've not added that remark to members.

> @@ +65,3 @@
> +
> +  /**
> +   * Change the contact's location.
> 
> Might want to rephrase this so it doesn’t sound like calling the method will
> teleport the person to somewhere new. :-P

Heh, wouldn't that be neat? :-)

> Perhaps “Update the contact’s currently advertised location.”?

Done. As in the constructor, I mentioned that null means "removing location".

Review of attachment 236576 [details] [review]:

Lots of minor changes. Sorry to be picky.

> This is part of the questions which still need to be determined. Is it worth
> updating the location if the contact only moved a few centimeters? I don't
> know.
>
> At the moment, the only source of location information is EDS, and there every
> update is assumed to be intentional and thus applied to the persona and
> individual.

I think a reasonable policy would be for backends to determine whether incoming location changes are intentional or not (so for EDS, all changes are intentional, even if they’re only a few cm; for a backend which uses location from GPS, only larger changes should be considered intentional), and for the backends themselves to only update a persona’s location on _intentional_ location changes. Individual should then update its location whenever any of its personas’ locations change (so no intentional/unintentional update policy is applied in the Individual code). It would be good to get this documented in the code, e.g. in individual.vala’s _update_location().

::: backends/eds/lib/edsf-persona.vala
@@ +2138,3 @@
+        {
+          if (this._location == null ||
+              !this._location.equal_coordinates(_geo.latitude, _geo.longitude))

Missing space before ‘(’.

Would be useful to add a comment explaining that we assume every change to GEO is intentional, and thus always notify of it. (As opposed to, e.g., in a backend where location comes from some noisy source like GPS, where the backend would have to filter out unintentional location changes.)

@@ +2149,3 @@
+              else
+                {
+                  this._location = new Location(_geo.latitude, _geo.longitude);

Missing space before ‘(’.

::: folks/individual.vala
@@ +2329,3 @@
+        }, (a, b) =>
+        {
+          /* TODO (?): pick the "better" location information. For now, pick more or less randomly. */

Hmm. I guess this is OK to leave for now, but please add a reference to this bug report.

@@ +2340,3 @@
+            }
+
+          if (new_location != this.location /* adding or removing a location? */ ||

This will produce spurious change notifications in the case the pointers are different but latitude and longitude are unchanged. How about:
   (new_location == null) != (this._location == null)

::: folks/location-details.vala
@@ +32,3 @@
+{
+  /**
+   * The latitude.

As mentioned on IRC, it’s useful to be explicit about the ‘@since’ entries on *every* piece of API documentation.

@@ +41,3 @@
+
+ /**
+  * Constructs a new instance with the given coordinates.

Missing ‘@param latitude …’ and ‘@param longitude …’.

@@ +50,3 @@
+
+ /**
+  * Strict comparison (= floats must be exactly the same).

That isn’t very clear to me. How about “Compare this location to another by geographical position, returning ``true`` iff the latitude and longitude match exactly.”.

Also needs a ‘@param other …’ line and a @return line.

@@ +52,3 @@
+  * Strict comparison (= floats must be exactly the same).
+  */
+ public bool equal(Location other)

Missing space before ‘(’.

@@ +59,3 @@
+
+ /**
+  * Strict comparison (= floats must be exactly the same).

Again, this documentation isn’t very clear to me and is missing @param lines and a @return line.

Also, the above three documentation comments have indentation problems (need to be indented further by 1 space).

@@ +61,3 @@
+  * Strict comparison (= floats must be exactly the same).
+  */
+  public bool equal_coordinates(double latitude, double longitude)

Missing space before ‘(’.

@@ +80,3 @@
+  /**
+   * The current location of the contact. Null if not
+   * set.

How about “Null if the contact’s current location isn’t known, or they’re keeping it private.”.

Created attachment 236728 [details] [review]
revised comments

(In reply to comment #10)
> Lots of minor changes. Sorry to be picky.

No problem. It's worth getting this right.

I'm not a fan of documenting every single parameter and return value just for the sake of it. It's a slippery slope which leads to useless comments which just repeat the parameter name without adding further information. Anyway, I've added it to the updated patch.

> > This is part of the questions which still need to be determined. Is it worth
> > updating the location if the contact only moved a few centimeters? I don't
> > know.
> >
> > At the moment, the only source of location information is EDS, and there every
> > update is assumed to be intentional and thus applied to the persona and
> > individual.
> 
> I think a reasonable policy would be for backends to determine whether incoming
> location changes are intentional or not (so for EDS, all changes are
> intentional, even if they’re only a few cm; for a backend which uses location
> from GPS, only larger changes should be considered intentional), and for the
> backends themselves to only update a persona’s location on _intentional_
> location changes. Individual should then update its location whenever any of
> its personas’ locations change (so no intentional/unintentional update policy
> is applied in the Individual code). It would be good to get this documented in
> the code, e.g. in individual.vala’s _update_location().

I decided to put that into LocationDetails instead, because knowing how folks behaves is relevant for users of it.

Review of attachment 236728 [details] [review]:

Looks good. Please fix the few tiny issues below and commit directly to master. Thanks!

> I'm not a fan of documenting every single parameter and return value just for
> the sake of it. It's a slippery slope which leads to useless comments which
> just repeat the parameter name without adding further information. Anyway, I've
> added it to the updated patch.

I know what you mean. I guess the argument for documenting every single parameter and return value is that it means warnings and errors from valadoc are useful; and you won’t miss warnings about missing documentation for _important_ parameters. Thanks for adding it.

> I decided to put that into LocationDetails instead, because knowing how folks
> behaves is relevant for users of it.

Good idea.

::: folks/individual.vala
@@ +581,3 @@
     }
 
+  private Location _location = null;

This needs to have nullable type ‘Location?’ rather than just ‘Location’.

@@ +1860,3 @@
       persona.notify["local-ids"].connect
           (this._notify_local_ids_cb);
+      persona.notify["location"].connect (this._notify_location_cb);

Need to add an equivalent disconnect() in _disconnect_from_persona().

@@ +2341,3 @@
+
+          if ((new_location == null) != (this.location == null) /* adding or removing a location? */ ||
+              new_location != null && !new_location.equal(this.location) /* different value? */)

Another missing space before the ’(’.

Comment on attachment 236728 [details] [review]
revised comments

Committed including the fixes for the latest comment.