Android - Essentials Geolocation | Added support for MslAltitudeMeters #27579
Conversation
…ion for altitude
|
Hey there @bricefriha! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
jsuarezruiz
added
platform/android 🤖
area-essentials
| @@ -24,7 +24,11 @@ internal static Location ToLocation(this AndroidLocation location) => | |||
| { | |||
| Latitude = location.Latitude, | |||
| Longitude = location.Longitude, | |||
| #if ANDROID34_0_OR_GREATER | |||
| Altitude = location.HasMslAltitude ? location.MslAltitudeMeters : location.HasAltitude ? location.Altitude : default(double?), | |||
There was a problem hiding this comment.
@jfversluis
(sorry in advance, if you're not the right person to ping here)
Is it ok if we set MslAltitudeMeters as the default? or would it be better to have another option for it?
The reason why I went for this method is to avoid having to add this for all platforms
There was a problem hiding this comment.
Hm good question. This is a breaking change because if people rely on location.Altitude they will now be forced to use location.MslAltitudeMeters and there is no way to opt out.
Can those values be different? Is there a way we can add to make it more optional?
There was a problem hiding this comment.
According to @cschwarz 's comment on #27554, it should be a default since it offers a more consistent result compared to other platforms. Which checks out, since Apple uses MSL Altitude in their API too
I would personally go for it. We could also add a LegacyAltitude (I'm bad at naming things) property that would represent Location.Altitude.
There was a problem hiding this comment.
I wonder if we should just add a new property? We could even add 2 new properties an be more explicit:
EllipsoidalAltitudeMeanSeaLevelAltitude
Then we obsolete Altitude and keep it returning what it did. Dows Windows have 2 different APIs?
Also saw this:
- https://developer.android.com/reference/android/location/altitude/AltitudeConverter
- https://developer.apple.com/documentation/corelocation/cllocation/ellipsoidalaltitude?language=objc
- https://developer.android.com/reference/kotlin/androidx/core/location/LocationManagerCompat
- https://developer.android.com/reference/kotlin/androidx/core/location/LocationCompat
Description of Change
The goal of this change is to add support for MslAltitudeMeters.
We are setting MslAltitudeMeters as the default value for
Location.Altitude. If MslAltitude is unavailable or Android version is lower than 34, we just set the regular Altitude.Issues Fixed
Fixes #27554