Fixes for PointType; added distanceFrom(otherPoint)

[watou]

Addition:

• distanceFrom(PointType otherPoint) returns distance in meters from this point to another point. Useful for geofencing and for other purposes in rules.

Fixes:

• toString() method would return nicely formatted string version of PointType, but the formatted version was useless from the REST API, and therefore unusable by clients. Now returns 3 decimal numbers separated by commas without rounding or other formatting. @clinique, is this OK with you?
• The String constructor would take any non-empty string and construct the object regardless of the string contents if it had no commas in the string. Following the example of the HSBType ComplexType, the string is now sanity checked in the constructor, and IllegalArgumentExceptions are thrown if the string is not in the valid "lat,long[,alt]" format.
• The valueOf method must be static, since it's expected to be called in org.openhab.core.types.TypeParser using reflection. (There is still an issue with actually using TypeParser, since it uses a list of Types and the first successful call to XXXType.valueOf determines the type returned. For example, if PointType appeared before StringType in the list (which it would have to be), then any string that happened to be of an acceptable format to PointType would "win", even if a StringType was preferred.)

[clinique]

@watou : yes, that is fine to me but are you sure I did not already put distanceFrom in the lib ? Sorry, I have headaches catching up with what I think, what I do, what I push to OH1, ESH... will check but nearly sure it already exists somewhere.
For toString(), very fine with this.

[robnielsen]

Can you specify which algorithm that is being used?

[watou]

It's based on the Haversine formula.

[robnielsen]

cool, I haven't used that one before. Might be useful to update the javadoc with this info.

[watou]

Your question prompted me to do so. Thanks for suggesting!

[watou]

@clinique I don't see a distanceFrom method in the ESH version, but I will add an issue to add it there. I did see that valueOf was fixed there already, and I am taking the toString() method from ESH back to OH1 so they are the same (optional output of altitude if it's non-zero).

[clinique]

distanceFrom is in LocationItem https://github.com/eclipse/smarthome/blob/master/bundles/core/org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/library/items/LocationItem.java

[watou]

Thank you, @clinique -- I should have looked there as well. I did not expect to find it in an item class. Is there a way of thinking about where such a method should live? My thinking was it should live in PointType, but you may have more background knowledge about why having it as a method of the item would make more sense. Also, the PointType class has a getGravity method, so it seemed to be more natural to put distanceFrom there as well. Regards, John

[clinique]

Yes, because gravity only depends on the PointType itself

[watou]

I would argue that distanceFrom also only depends on the PointType itself (specifically, this and another instance of PointType only). The implementation also relies on the WGS84_a constant (equatorial radius) as does getGravity. So I would ask @kaikreuzer and @teichsta to tell me what is a good way to think about it. Sorry for duplicating your work, @clinique, but also the process will at least educate me about what the architects' thinking is as well. Regards, John

[teichsta]

So I would ask @kaikreuzer and @teichsta to tell me what is a good way to think about it.

i would say: if the Item provides the method then it should also take another Item to calculate the distance which would then delegate the calculation to the PointType since it has the "knowledge" about the data.

So i would opt to move distanceFrom to the PointType.

What do you think @kaikreuzer?

[kaikreuzer]

Yes, I can see @watou point here and would agree that this is probably the better place.
I am not sure if anybody is using this method on the LocationItem in ESH yet - if so, moving the method would break this. But if we decide to move it, then it should be done rather sooner than later.
@clinique would you be ok with this as well or do you have any good arguments why it should stay on the item?

[clinique]

@kaikreuzer , @watou , no issue to move it. I'm sure I had a very good reason to put it there when I did it, but as I can't put the finger on it anymore, the reason might not be so good :)

[watou]

Thank you, @clinique. I changed LocationItem.distanceFrom(PointType) to LocationItem.distanceFrom(LocationItem), and it delegates to PointType.distanceFrom(PointType). Also updated the LocationItemTest but didn't add PointTypeTest as it's redundant. This is a breaking change for any OH1.7 users who might be using it, but with an easy fix. Likewise for any ESH users when a matching code change is made there. This PR will also allow any REST API clients to use PointType states against 1.8, since it's toString() method now matches the ESH version.

I can set up an ESH development environment and submit a PR there, or I could leave it someone else. Whatever you think.

[clinique]

I've got an ESH dev env - no pb to push the PR

[watou]

I think this PR is ready for 1.8, so please push a matching PR in ESH and thank you again, @clinique .

[watou]

The ESH bug I opened is 482188.

[teichsta]

@watou once the ESH PR is merged please go ahead and merge this PR as well. Thanks, Thomas E.-E.

[watou]

OK, will do. Thanks.

[kaikreuzer]

eclipse-archived/smarthome#520 has been merged. Thanks!

[teichsta]

Thanks @watou!