Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Android - Essentials Geolocation | Added support for MslAltitudeMeters #27579

Draft
wants to merge 5 commits into
base: net10.0
Choose a base branch
from

Conversation

bricefriha
Copy link

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

Copy link
Contributor

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.

@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Feb 5, 2025
@jsuarezruiz jsuarezruiz added platform/android 🤖 area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info labels Feb 5, 2025
@bricefriha bricefriha marked this pull request as ready for review February 6, 2025 05:42
@bricefriha bricefriha requested a review from a team as a code owner February 6, 2025 05:42
@jfversluis jfversluis self-assigned this Feb 6, 2025
Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should just add a new property? We could even add 2 new properties an be more explicit:

  • EllipsoidalAltitude
  • MeanSeaLevelAltitude

Then we obsolete Altitude and keep it returning what it did. Dows Windows have 2 different APIs?

Also saw this:

Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should target .net10 branch

@bricefriha bricefriha changed the base branch from main to net10.0 February 14, 2025 11:31
@bricefriha
Copy link
Author

bricefriha commented Feb 14, 2025

I wonder if we should just add a new property? We could even add 2 new properties an be more explicit:

  • EllipsoidalAltitude
  • MeanSeaLevelAltitude
    Then we obsolete Altitude and keep it returning what it did.

I agree, but my concern is that we need these for all platforms, which would be a bit redundant for iOS as Altitude is MeanSeaLevelAltitude there.

Also saw this:

thanks, that's interesting; we could play around with that perhaps

@bricefriha bricefriha marked this pull request as draft February 19, 2025 10:40
@bricefriha
Copy link
Author

I converted it back to draft so I can make the changes @mattleibow mentioned

@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info community ✨ Community Contribution platform/android 🤖
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for MslAltitudeMeters in Essentials Geolocation on Android
6 participants