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

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME'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.gnome.org/GNOME/folks/issues/5.