Skip to content

[Essentials] Use mean sea level altitude on Android API 34+#35097

Open
KitKeen wants to merge 5 commits intodotnet:mainfrom
KitKeen:feature/geolocation-msl-altitude
Open

[Essentials] Use mean sea level altitude on Android API 34+#35097
KitKeen wants to merge 5 commits intodotnet:mainfrom
KitKeen:feature/geolocation-msl-altitude

Conversation

@KitKeen
Copy link
Copy Markdown

@KitKeen KitKeen commented Apr 22, 2026

Fixes #27554.

Android API level 34 introduced
Location.getMslAltitudeMeters(),
which reports altitude measured against the geoid (mean sea level).
The existing implementation always reads the WGS84 ellipsoidal
altitude, which is reported inconsistently with iOS and Windows where
altitude is typically expressed against the geoid. As the issue notes,
this forces consumers to apply manual geoid corrections to get
consistent results across platforms.

This change updates LocationExtensions.android.cs to:

  1. Prefer MslAltitudeMeters when the device runs Android 34+ and
    HasMslAltitude is true, reporting
    AltitudeReferenceSystem.Geoid.
  2. Fall back to the existing Altitude / Ellipsoid path on older
    devices or when MSL altitude is not available.
  3. Report AltitudeReferenceSystem.Unspecified when no altitude is
    available at all (previously this case was reported as
    Ellipsoid even though Altitude was null).

The MSL preference is opaque to callers — Location.Altitude keeps
the same shape and semantics; consumers that care about the reference
system can inspect AltitudeReferenceSystem as before.

Issues Fixed

Fixes #27554

Tested

  • Code change is guarded by OperatingSystem.IsAndroidVersionAtLeast(34)
    so older devices are unaffected.
  • Device validation for Android 34+ devices to confirm MslAltitudeMeters
    is surfaced will rely on existing Geolocation_Tests.cs device tests
    exercised on CI.

Android API level 34 introduced Location.getMslAltitudeMeters(), which
provides altitude measured against the geoid (mean sea level). The
existing implementation always reads the WGS84 ellipsoidal altitude,
which is reported inconsistently with iOS/Windows where altitude is
typically expressed against the geoid.

Prefer the MSL altitude when the device exposes it (API 34+ and
HasMslAltitude is true) and report AltitudeReferenceSystem.Geoid in
that case. Fall back to the existing WGS84 path on older devices.

Also reports AltitudeReferenceSystem.Unspecified when no altitude is
available, instead of hard-coding Ellipsoid for a null altitude.

Fixes dotnet#27554
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 22, 2026

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35097

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35097"

@dotnet-policy-service dotnet-policy-service Bot added the community ✨ Community Contribution label Apr 22, 2026
@kubaflo
Copy link
Copy Markdown
Contributor

kubaflo commented Apr 23, 2026

🟠 .NET MAUI Code Review — Discussion Needed

👋 @KitKeen — new code review results are available. Please review the latest session below.

🟠 Review Session — Discussion Needed63444de · [Essentials] Use mean sea level altitude on Android API 34+ · 2026-04-23 10:10 UTC

Code Review — PR #35097

Independent Assessment

What this changes: Refactors ToLocation(AndroidLocation) to delegate altitude selection to a new GetAltitude helper. On Android API 34+ with HasMslAltitude == true, returns MslAltitudeMeters paired with AltitudeReferenceSystem.Geoid; otherwise falls back to the existing Altitude/Ellipsoid; otherwise returns (null, Unspecified).

Inferred motivation: Bring Android altitude semantics in line with iOS (always Geoid) and Windows (passes WinRT's own reference through), so cross-platform consumers no longer need a manual geoid correction on Android. Also corrects an incoherent prior state where Altitude == null was paired with AltitudeReferenceSystem.Ellipsoid.

Reconciliation with PR Narrative

The PR description matches the implementation. One inaccuracy: it claims existing Geolocation_Tests.cs device tests will exercise the new branch, but those tests only assert Latitude/Longitude/Accuracy — they never look at Altitude or AltitudeReferenceSystem, so the new path is effectively untested.

Findings

⚠️ Warning — VerticalAccuracy is sourced from the ellipsoidal accuracy even when Altitude is MSL (flagged by both reviewing models — Claude Sonnet 4.6 and GPT-5.4)

src/Essentials/src/Types/LocationExtensions.android.cs:30-32 still reads VerticalAccuracyMeters (API 26+), which Android documents as the uncertainty of getAltitude() (ellipsoidal). On API 34+ the framework also exposes HasMslAltitudeAccuracy / MslAltitudeAccuracyMeters as the matching uncertainty for getMslAltitudeMeters().

After this PR, an API-34+ caller can receive Altitude (MSL/geoid) paired with VerticalAccuracy from a different reference system. In practice the two values are usually close on consumer GPS chipsets, so the user-visible impact is small, but it is a semantic inconsistency that is trivial to fix in the same helper. Suggested shape:

VerticalAccuracy =
    OperatingSystem.IsAndroidVersionAtLeast(34) && location.HasMslAltitudeAccuracy
        ? location.MslAltitudeAccuracyMeters
        : OperatingSystem.IsAndroidVersionAtLeast(26) && location.HasVerticalAccuracy
            ? location.VerticalAccuracyMeters
            : null,

Ideally the MSL-vs-ellipsoid choice for altitude and accuracy should be made together in GetAltitude so the two values can never diverge.

💡 Suggestion — Add coverage for the new altitude paths

The three branches in GetAltitude (MSL → Geoid, ellipsoid → Ellipsoid, none → Unspecified) are not exercised by any test. Existing device tests under src/Essentials/test/DeviceTests/Tests/Geolocation_Tests.cs don't assert on Altitude or AltitudeReferenceSystem. Even a lightweight device test that constructs a synthetic Android.Locations.Location and asserts the resulting AltitudeReferenceSystem would be enough to lock in the new behavior and prevent regressions on the Ellipsoid → Unspecified change.

💡 Suggestion — Behavior change on the no-altitude path

Pre-existing behavior returned (null, Ellipsoid) when HasAltitude was false; the new code returns (null, Unspecified). This is a strict improvement (the prior pairing was incoherent and the Windows path already uses Unspecified for the same case). Just calling it out so it lands in release notes — any consumer that conditioned on AltitudeReferenceSystem == Ellipsoid as a sentinel for "Android" will now see Unspecified when there is no fix.

Devil's Advocate

  • API 34 guard correctness: The OperatingSystem.IsAndroidVersionAtLeast(34) && location.HasMslAltitude pattern matches the existing HasVerticalAccuracy (API 26) and Mock (API 31) usages in the same file, and CA1416 will be satisfied via short-circuit. No platform analyzer suppression is needed. ✅
  • Blast radius of Ellipsoid → Unspecified: No internal MAUI consumer in this repo depends on the old sentinel; the only AltitudeReferenceSystem use outside the conversion is GeolocationViewModel.ToString(). External consumers' behavior is unverifiable, but the previous state was indefensible.
  • Cross-platform parity: This actually improves Android↔iOS alignment, which is the issue's stated goal.

Verdict: NEEDS_DISCUSSION (lean LGTM)

Confidence: medium-high
Summary: Direction is correct and the API gating is sound. Two reviewing models independently flagged the VerticalAccuracy/MSL mismatch — Sonnet rated it a non-blocking warning, GPT rated it a blocker. I'd suggest either addressing it in this PR (small change, same helper) or filing a follow-up issue and merging. The missing test coverage is consistent with the rest of the geolocation code, but adding even one assertion on AltitudeReferenceSystem would substantially de-risk this change.


Multi-model review: independently performed by Claude Sonnet 4.6 and GPT-5.4. Both models reached the same primary finding (VerticalAccuracy/MSL mismatch) with different severity calibrations.


@MauiBot MauiBot added s/agent-changes-requested AI agent recommends changes - found a better alternative or issues s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review) labels Apr 23, 2026
@dotnet dotnet deleted a comment from MauiBot Apr 23, 2026
KitKeen added 2 commits April 24, 2026 01:01
Address review feedback on dotnet#35097. Previously VerticalAccuracy was
always read from Location.VerticalAccuracyMeters (ellipsoidal
accuracy), regardless of whether Altitude came from the MSL/geoid or
WGS84/ellipsoid path. On Android API 34+ with MSL altitude, this
produced an altitude and a vertical accuracy in different reference
systems.

Select altitude and its accuracy together in GetAltitude so they can
never diverge:

- MSL path (API 34+ && HasMslAltitude): altitude from
  MslAltitudeMeters, accuracy from MslAltitudeAccuracyMeters when
  HasMslAltitudeAccuracy is true, else null.
- Ellipsoid path (HasAltitude): altitude from Altitude, accuracy from
  VerticalAccuracyMeters when API 26+ and HasVerticalAccuracy.
- No altitude: (null, Unspecified, null).

Add device tests covering all three GetAltitude branches and the
MSL-without-MSL-accuracy case.
The existing TestLocation_EllipsoidalAltitude_UsesEllipsoidReferenceSystem
test happened to exercise the fallback because MslAltitudeMeters was never
set, but that was implicit. This adds a dedicated test that pins the
invariant: on API 34+ where HasMslAltitude is false (e.g. the device did
not report a reference-corrected altitude), ToLocation falls back to the
ellipsoidal altitude and tags it Ellipsoid — not Geoid, not Unspecified.

This is the second half of the IsAndroidVersionAtLeast(34) && HasMslAltitude
guard added in the previous commit; without an explicit test, a refactor
that dropped the HasMslAltitude check would still pass the suite.
@MauiBot MauiBot added the s/agent-fix-pr-picked AI could not beat the PR fix - PR is the best among all candidates label Apr 24, 2026
@dotnet dotnet deleted a comment from MauiBot Apr 24, 2026
Copy link
Copy Markdown
Contributor

@kubaflo kubaflo left a comment

Choose a reason for hiding this comment

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

Could you please review ai's suggestions?

Each test now runs at least one assertion on any API level before the
API 34+ specific branch, so pre-34 runs exercise the fallback path
(Ellipsoid or Unspecified) instead of returning early without asserting.
This keeps the gate test meaningful on devices below API 34 and catches
regressions in either code path.
Copy link
Copy Markdown
Collaborator

@MauiBot MauiBot left a comment

Choose a reason for hiding this comment

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

🤖 Automated review — alternative fix proposed

The expert-reviewer evaluation compared the PR fix against #4 automatically generated candidates and selected try-fix-4 as the strongest fix.

Why: try-fix-4 wins because it combines the PR's correct GetAltitude helper with two critical additions the PR omits: (1) a one-line fix in the Location(Location point) copy constructor that was missing AltitudeReferenceSystem = point.AltitudeReferenceSystem — without this, Geocoding.GetPlacemarksAsync silently resets the reference system on Android 34+; (2) a regression test (LocationCopyConstructor_CopiesAltitudeReferenceSystem) that protects against that bug recurring. It also removes the byte-for-byte duplicate test.

Please consider applying the candidate diff below (or use it as guidance). Once you push an update, this workflow will re-trigger and re-evaluate.

Candidate diff (`try-fix-4`)
diff --git a/src/Essentials/src/Types/Location.shared.cs b/src/Essentials/src/Types/Location.shared.cs
index 892cf582d4..cf5bb4caec 100644
--- a/src/Essentials/src/Types/Location.shared.cs
+++ b/src/Essentials/src/Types/Location.shared.cs
@@ -109,6 +109,7 @@ namespace Microsoft.Maui.Devices.Sensors
 			Speed = point.Speed;
 			Course = point.Course;
 			IsFromMockProvider = point.IsFromMockProvider;
+			AltitudeReferenceSystem = point.AltitudeReferenceSystem;
 		}
 
 		/// <summary>
diff --git a/src/Essentials/src/Types/LocationExtensions.android.cs b/src/Essentials/src/Types/LocationExtensions.android.cs
index 7c3d664b94..ceb72cc9a9 100644
--- a/src/Essentials/src/Types/LocationExtensions.android.cs
+++ b/src/Essentials/src/Types/LocationExtensions.android.cs
@@ -19,18 +19,18 @@ namespace Microsoft.Maui.Devices.Sensors
 		internal static IEnumerable<Location> ToLocations(this IEnumerable<AndroidAddress> addresses) =>
 			addresses?.Select(a => a.ToLocation());
 
-		internal static Location ToLocation(this AndroidLocation location) =>
-			new Location
+		internal static Location ToLocation(this AndroidLocation location)
+		{
+			var (altitude, altitudeReference, verticalAccuracy) = GetAltitude(location);
+
+			return new Location
 			{
 				Latitude = location.Latitude,
 				Longitude = location.Longitude,
-				Altitude = location.HasAltitude ? location.Altitude : default(double?),
+				Altitude = altitude,
 				Timestamp = location.GetTimestamp().ToUniversalTime(),
 				Accuracy = location.HasAccuracy ? location.Accuracy : default(float?),
-				VerticalAccuracy =
-					OperatingSystem.IsAndroidVersionAtLeast(26) && location.HasVerticalAccuracy
-						? location.VerticalAccuracyMeters
-						: null,
+				VerticalAccuracy = verticalAccuracy,
 				ReducedAccuracy = false,
 				Course = location.HasBearing ? location.Bearing : default(double?),
 				Speed = location.HasSpeed ? location.Speed : default(double?),
@@ -40,8 +40,35 @@ namespace Microsoft.Maui.Devices.Sensors
 #pragma warning disable CS0618 // Type or member is obsolete
 						: location.IsFromMockProvider,
 #pragma warning restore CS0618 // Type or member is obsolete
-				AltitudeReferenceSystem = AltitudeReferenceSystem.Ellipsoid
+				AltitudeReferenceSystem = altitudeReference
 			};
+		}
+
+		// Prefer mean sea level altitude (Android API 34+) when available so altitude
+		// values are consistent across platforms without manual geoid correction.
+		// Altitude and its accuracy are selected together so they always come from
+		// the same reference system (MSL/geoid vs WGS84/ellipsoid).
+		static (double? Altitude, AltitudeReferenceSystem Reference, double? VerticalAccuracy) GetAltitude(AndroidLocation location)
+		{
+			if (OperatingSystem.IsAndroidVersionAtLeast(34) && location.HasMslAltitude)
+			{
+				double? mslAccuracy = location.HasMslAltitudeAccuracy
+					? location.MslAltitudeAccuracyMeters
+					: null;
+				return (location.MslAltitudeMeters, AltitudeReferenceSystem.Geoid, mslAccuracy);
+			}
+
+			if (location.HasAltitude)
+			{
+				double? ellipsoidAccuracy =
+					OperatingSystem.IsAndroidVersionAtLeast(26) && location.HasVerticalAccuracy
+						? location.VerticalAccuracyMeters
+						: null;
+				return (location.Altitude, AltitudeReferenceSystem.Ellipsoid, ellipsoidAccuracy);
+			}
+
+			return (null, AltitudeReferenceSystem.Unspecified, null);
+		}
 
 		static readonly DateTime epoch = new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc);
 
diff --git a/src/Essentials/test/DeviceTests/Tests/Android/Geolocation_Tests.cs b/src/Essentials/test/DeviceTests/Tests/Android/Geolocation_Tests.cs
index 9742e83aad..f07e37b7e8 100644
--- a/src/Essentials/test/DeviceTests/Tests/Android/Geolocation_Tests.cs
+++ b/src/Essentials/test/DeviceTests/Tests/Android/Geolocation_Tests.cs
@@ -42,33 +42,6 @@ namespace Microsoft.Maui.Essentials.DeviceTests.Shared
 				Assert.Null(location.VerticalAccuracy);
 	}
 
-	[Fact]
-	public void ToLocation_HasAltitudeButNoMslAltitude_UsesEllipsoid()
-	{
-		// On every API level, a location that reports an ellipsoidal altitude but no
-		// MSL altitude must resolve to Ellipsoid. On pre-34 devices that is the only
-		// code path; on API 34+ devices this exercises the HasMslAltitude == false
-		// fallback branch. Either way this assertion runs, so the test cannot silently
-		// pass if the fallback regresses.
-		var androidLocation = new AndroidLocation("test")
-		{
-			Altitude = 123.45,
-		};
-
-		if (OperatingSystem.IsAndroidVersionAtLeast(26))
-			androidLocation.VerticalAccuracyMeters = 5.0f;
-
-		var location = androidLocation.ToLocation();
-
-		Assert.Equal(123.45, location.Altitude);
-		Assert.Equal(AltitudeReferenceSystem.Ellipsoid, location.AltitudeReferenceSystem);
-
-		if (OperatingSystem.IsAndroidVersionAtLeast(26))
-			Assert.Equal(5.0, location.VerticalAccuracy);
-		else
-			Assert.Null(location.VerticalAccuracy);
-	}
-
 	[Fact]
 	public void ToLocation_MslAltitude_UsesGeoidReferenceSystem()
 	{
@@ -130,5 +103,30 @@ namespace Microsoft.Maui.Essentials.DeviceTests.Shared
 			Assert.Equal(AltitudeReferenceSystem.Geoid, location.AltitudeReferenceSystem);
 			Assert.Null(location.VerticalAccuracy);
 		}
+
+	[Fact]
+	public void LocationCopyConstructor_CopiesAltitudeReferenceSystem()
+	{
+		var originalLocation = new Location
+		{
+			Latitude = 37.7749,
+			Longitude = -122.4194,
+			Altitude = 100.0,
+			AltitudeReferenceSystem = AltitudeReferenceSystem.Geoid,
+			VerticalAccuracy = 2.5
+		};
+
+		var copiedLocation = new Location(originalLocation);
+
+		Assert.Equal(originalLocation.Latitude, copiedLocation.Latitude);
+		Assert.Equal(originalLocation.Longitude, copiedLocation.Longitude);
+		Assert.Equal(originalLocation.Altitude, copiedLocation.Altitude);
+		Assert.Equal(AltitudeReferenceSystem.Geoid, copiedLocation.AltitudeReferenceSystem);
+		Assert.Equal(originalLocation.VerticalAccuracy, copiedLocation.VerticalAccuracy);
+	}
 	}
 }

@MauiBot MauiBot added s/agent-fix-win AI found a better alternative fix than the PR and removed s/agent-fix-pr-picked AI could not beat the PR fix - PR is the best among all candidates labels Apr 30, 2026
Copy link
Copy Markdown
Contributor

@kubaflo kubaflo left a comment

Choose a reason for hiding this comment

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

Could you please try the ai's suggestions?

@dotnet dotnet deleted a comment from MauiBot May 1, 2026
@kubaflo kubaflo dismissed MauiBot’s stale review May 1, 2026 09:39

Resetting for re-review

MauiBot
MauiBot previously requested changes May 1, 2026
Copy link
Copy Markdown
Collaborator

@MauiBot MauiBot left a comment

Choose a reason for hiding this comment

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

🤖 Automated review — alternative fix proposed

The expert-reviewer evaluation compared the PR fix against #1 automatically generated candidates and selected try-fix-1 as the strongest fix.

Why: try-fix-1 (inline set-defaults-then-override) is functionally equivalent to the PR's GetAltitude() helper but inlined into ToLocation() with plain local variables. It is the simplest approach, passes all 5 device tests, and avoids any helper-method linker sensitivity that may have contributed to the gate failure on the PR's version.

Please consider applying the candidate diff below (or use it as guidance). Once you push an update, this workflow will re-trigger and re-evaluate.

Candidate diff (`try-fix-1`)
diff --git a/src/Essentials/src/Types/LocationExtensions.android.cs b/src/Essentials/src/Types/LocationExtensions.android.cs
index 7c3d664b94..370bf854c6 100644
--- a/src/Essentials/src/Types/LocationExtensions.android.cs
+++ b/src/Essentials/src/Types/LocationExtensions.android.cs
@@ -19,18 +19,40 @@ namespace Microsoft.Maui.Devices.Sensors
 		internal static IEnumerable<Location> ToLocations(this IEnumerable<AndroidAddress> addresses) =>
 			addresses?.Select(a => a.ToLocation());
 
-		internal static Location ToLocation(this AndroidLocation location) =>
-			new Location
+		internal static Location ToLocation(this AndroidLocation location)
+		{
+			// Default to no altitude with Unspecified reference; override below
+			// when the platform reports a usable value.
+			double? altitude = null;
+			AltitudeReferenceSystem altitudeRef = AltitudeReferenceSystem.Unspecified;
+			double? verticalAccuracy = null;
+
+			// Prefer mean-sea-level altitude (API 34+) for cross-platform consistency.
+			if (OperatingSystem.IsAndroidVersionAtLeast(34) && location.HasMslAltitude)
+			{
+				altitude = location.MslAltitudeMeters;
+				altitudeRef = AltitudeReferenceSystem.Geoid;
+				verticalAccuracy = location.HasMslAltitudeAccuracy
+					? location.MslAltitudeAccuracyMeters
+					: null;
+			}
+			else if (location.HasAltitude)
+			{
+				altitude = location.Altitude;
+				altitudeRef = AltitudeReferenceSystem.Ellipsoid;
+				verticalAccuracy = OperatingSystem.IsAndroidVersionAtLeast(26) && location.HasVerticalAccuracy
+					? location.VerticalAccuracyMeters
+					: null;
+			}
+
+			return new Location
 			{
 				Latitude = location.Latitude,
 				Longitude = location.Longitude,
-				Altitude = location.HasAltitude ? location.Altitude : default(double?),
+				Altitude = altitude,
 				Timestamp = location.GetTimestamp().ToUniversalTime(),
 				Accuracy = location.HasAccuracy ? location.Accuracy : default(float?),
-				VerticalAccuracy =
-					OperatingSystem.IsAndroidVersionAtLeast(26) && location.HasVerticalAccuracy
-						? location.VerticalAccuracyMeters
-						: null,
+				VerticalAccuracy = verticalAccuracy,
 				ReducedAccuracy = false,
 				Course = location.HasBearing ? location.Bearing : default(double?),
 				Speed = location.HasSpeed ? location.Speed : default(double?),
@@ -40,8 +62,9 @@ namespace Microsoft.Maui.Devices.Sensors
 #pragma warning disable CS0618 // Type or member is obsolete
 						: location.IsFromMockProvider,
 #pragma warning restore CS0618 // Type or member is obsolete
-				AltitudeReferenceSystem = AltitudeReferenceSystem.Ellipsoid
+				AltitudeReferenceSystem = altitudeRef
 			};
+		}
 
 		static readonly DateTime epoch = new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc);

@MauiBot MauiBot added s/agent-fix-pr-picked AI could not beat the PR fix - PR is the best among all candidates and removed s/agent-fix-win AI found a better alternative fix than the PR labels May 1, 2026
@dotnet dotnet deleted a comment from MauiBot May 3, 2026
Copy link
Copy Markdown
Collaborator

@MauiBot MauiBot left a comment

Choose a reason for hiding this comment

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

Expert Review — 2 findings

See inline comments for details.

@MauiBot MauiBot added s/agent-fix-win AI found a better alternative fix than the PR and removed s/agent-fix-pr-picked AI could not beat the PR fix - PR is the best among all candidates labels May 3, 2026
@dotnet dotnet deleted a comment from MauiBot May 3, 2026
@dotnet dotnet deleted a comment from MauiBot May 3, 2026
@dotnet dotnet deleted a comment from MauiBot May 3, 2026
@kubaflo kubaflo dismissed MauiBot’s stale review May 3, 2026 12:45

Resetting for re-review

Add missing AltitudeReferenceSystem assignment in the Location copy
constructor and add a unit test to cover the regression.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@KitKeen
Copy link
Copy Markdown
Author

KitKeen commented May 3, 2026

Addressed the copy constructor issue- added AltitudeReferenceSystem to the copy constructor and added a unit test to cover the regression.

MauiBot
MauiBot previously requested changes May 3, 2026
Copy link
Copy Markdown
Collaborator

@MauiBot MauiBot left a comment

Choose a reason for hiding this comment

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

🤖 Automated review — alternative fix proposed

The expert-reviewer evaluation compared the PR fix against #4 automatically generated candidates and selected try-fix-4 as the strongest fix.

Why: try-fix-4 passes all 272 device tests and uses the same GetAltitude tuple-helper pattern as the PR. It replaces the byte-for-byte duplicate test with an explicit MSL-precedence regression test that directly validates the API 34+ behaviour when both altitude systems are present. The PR gate failure is an XHarness infrastructure issue (exit code 82), not a code correctness failure.

Please consider applying the candidate diff below (or use it as guidance). Once you push an update, this workflow will re-trigger and re-evaluate.

Candidate diff (`try-fix-4`)
diff --git a/src/Essentials/src/Types/LocationExtensions.android.cs b/src/Essentials/src/Types/LocationExtensions.android.cs
index 7c3d664b94..d342f057cb 100644
--- a/src/Essentials/src/Types/LocationExtensions.android.cs
+++ b/src/Essentials/src/Types/LocationExtensions.android.cs
@@ -19,18 +19,18 @@ namespace Microsoft.Maui.Devices.Sensors
 		internal static IEnumerable<Location> ToLocations(this IEnumerable<AndroidAddress> addresses) =>
 			addresses?.Select(a => a.ToLocation());
 
-		internal static Location ToLocation(this AndroidLocation location) =>
-			new Location
+		internal static Location ToLocation(this AndroidLocation location)
+		{
+			var (altitude, altitudeReference, verticalAccuracy) = GetAltitude(location);
+
+			return new Location
 			{
 				Latitude = location.Latitude,
 				Longitude = location.Longitude,
-				Altitude = location.HasAltitude ? location.Altitude : default(double?),
+				Altitude = altitude,
 				Timestamp = location.GetTimestamp().ToUniversalTime(),
 				Accuracy = location.HasAccuracy ? location.Accuracy : default(float?),
-				VerticalAccuracy =
-					OperatingSystem.IsAndroidVersionAtLeast(26) && location.HasVerticalAccuracy
-						? location.VerticalAccuracyMeters
-						: null,
+				VerticalAccuracy = verticalAccuracy,
 				ReducedAccuracy = false,
 				Course = location.HasBearing ? location.Bearing : default(double?),
 				Speed = location.HasSpeed ? location.Speed : default(double?),
@@ -40,8 +40,34 @@ namespace Microsoft.Maui.Devices.Sensors
 #pragma warning disable CS0618 // Type or member is obsolete
 						: location.IsFromMockProvider,
 #pragma warning restore CS0618 // Type or member is obsolete
-				AltitudeReferenceSystem = AltitudeReferenceSystem.Ellipsoid
+				AltitudeReferenceSystem = altitudeReference
 			};
+		}
+
+		// Prefer mean sea level altitude (Android API 34+) when available so Android
+		// matches the geoid-based altitude reported by iOS and Windows. Altitude and
+		// accuracy are selected together so they always come from the same reference system.
+		static (double? Altitude, AltitudeReferenceSystem Reference, double? VerticalAccuracy) GetAltitude(AndroidLocation location)
+		{
+			if (OperatingSystem.IsAndroidVersionAtLeast(34) && location.HasMslAltitude)
+			{
+				double? mslAccuracy = location.HasMslAltitudeAccuracy
+					? location.MslAltitudeAccuracyMeters
+					: null;
+				return (location.MslAltitudeMeters, AltitudeReferenceSystem.Geoid, mslAccuracy);
+			}
+
+			if (location.HasAltitude)
+			{
+				double? ellipsoidAccuracy =
+					OperatingSystem.IsAndroidVersionAtLeast(26) && location.HasVerticalAccuracy
+						? location.VerticalAccuracyMeters
+						: null;
+				return (location.Altitude, AltitudeReferenceSystem.Ellipsoid, ellipsoidAccuracy);
+			}
+
+			return (null, AltitudeReferenceSystem.Unspecified, null);
+		}
 
 		static readonly DateTime epoch = new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc);
 
diff --git a/src/Essentials/test/DeviceTests/Tests/Android/Geolocation_Tests.cs b/src/Essentials/test/DeviceTests/Tests/Android/Geolocation_Tests.cs
index 9742e83aad..2042d07b1c 100644
--- a/src/Essentials/test/DeviceTests/Tests/Android/Geolocation_Tests.cs
+++ b/src/Essentials/test/DeviceTests/Tests/Android/Geolocation_Tests.cs
@@ -43,13 +43,8 @@ namespace Microsoft.Maui.Essentials.DeviceTests.Shared
 		}
 
 		[Fact]
-		public void ToLocation_HasAltitudeButNoMslAltitude_UsesEllipsoid()
+		public void ToLocation_MslAltitudeTakesPrecedenceOverEllipsoidalAltitude()
 		{
-			// On every API level, a location that reports an ellipsoidal altitude but no
-			// MSL altitude must resolve to Ellipsoid. On pre-34 devices that is the only
-			// code path; on API 34+ devices this exercises the HasMslAltitude == false
-			// fallback branch. Either way this assertion runs, so the test cannot silently
-			// pass if the fallback regresses.
 			var androidLocation = new AndroidLocation("test")
 			{
 				Altitude = 123.45,
@@ -58,49 +53,59 @@ namespace Microsoft.Maui.Essentials.DeviceTests.Shared
 			if (OperatingSystem.IsAndroidVersionAtLeast(26))
 				androidLocation.VerticalAccuracyMeters = 5.0f;
 
-			var location = androidLocation.ToLocation();
-
-			Assert.Equal(123.45, location.Altitude);
-			Assert.Equal(AltitudeReferenceSystem.Ellipsoid, location.AltitudeReferenceSystem);
-
+			var baseline = androidLocation.ToLocation();
+			Assert.Equal(123.45, baseline.Altitude);
+			Assert.Equal(AltitudeReferenceSystem.Ellipsoid, baseline.AltitudeReferenceSystem);
 			if (OperatingSystem.IsAndroidVersionAtLeast(26))
-				Assert.Equal(5.0, location.VerticalAccuracy);
+				Assert.Equal(5.0, baseline.VerticalAccuracy);
 			else
-				Assert.Null(location.VerticalAccuracy);
+				Assert.Null(baseline.VerticalAccuracy);
+
+			if (!OperatingSystem.IsAndroidVersionAtLeast(34))
+				return;
+
+			// When both altitude systems are present, API 34+ must prefer MSL/geoid so
+			// Android matches iOS and Windows without manual geoid correction.
+			androidLocation.MslAltitudeMeters = 100.0;
+			androidLocation.MslAltitudeAccuracyMeters = 2.5f;
+
+			var location = androidLocation.ToLocation();
+
+			Assert.Equal(100.0, location.Altitude);
+			Assert.Equal(AltitudeReferenceSystem.Geoid, location.AltitudeReferenceSystem);
+			Assert.Equal(2.5, location.VerticalAccuracy);
 		}
 
 		[Fact]
 		public void ToLocation_MslAltitude_UsesGeoidReferenceSystem()
 		{
-			var androidLocation = new AndroidLocation("test")
+			var baselineLocation = new AndroidLocation("test")
 			{
 				Altitude = 123.45,
 			};
 
-			// Baseline: without MSL altitude set, we must get Ellipsoid on any API level.
-			// This guarantees an assertion runs even on pre-34 devices so the test cannot
-			// silently pass if the API-34 branch is accidentally taken or the fallback
-			// regresses.
-			var baseline = androidLocation.ToLocation();
+			if (OperatingSystem.IsAndroidVersionAtLeast(26))
+				baselineLocation.VerticalAccuracyMeters = 5.0f;
+
+			var baseline = baselineLocation.ToLocation();
 			Assert.Equal(123.45, baseline.Altitude);
 			Assert.Equal(AltitudeReferenceSystem.Ellipsoid, baseline.AltitudeReferenceSystem);
+			if (OperatingSystem.IsAndroidVersionAtLeast(26))
+				Assert.Equal(5.0, baseline.VerticalAccuracy);
+			else
+				Assert.Null(baseline.VerticalAccuracy);
 
-			// The remaining assertions exercise the API 34+ MSL path. MslAltitudeMeters is
-			// only meaningful on API 34+, so below that we stop after validating the
-			// fallback above.
 			if (!OperatingSystem.IsAndroidVersionAtLeast(34))
 				return;
 
+			var androidLocation = new AndroidLocation("test");
 			androidLocation.MslAltitudeMeters = 100.0;
 			androidLocation.MslAltitudeAccuracyMeters = 2.5f;
-			androidLocation.VerticalAccuracyMeters = 5.0f; // ellipsoidal accuracy, must NOT be surfaced
 
 			var location = androidLocation.ToLocation();
 
 			Assert.Equal(100.0, location.Altitude);
 			Assert.Equal(AltitudeReferenceSystem.Geoid, location.AltitudeReferenceSystem);
-			// VerticalAccuracy must be paired with the chosen altitude reference system,
-			// so the MSL accuracy is used rather than the ellipsoidal one.
 			Assert.Equal(2.5, location.VerticalAccuracy);
 		}

Copy link
Copy Markdown
Contributor

@kubaflo kubaflo left a comment

Choose a reason for hiding this comment

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

Could you please review the ai's suggestions?

@dotnet dotnet deleted a comment from MauiBot May 5, 2026
@kubaflo kubaflo dismissed MauiBot’s stale review May 5, 2026 18:42

Resetting for re-review

@MauiBot
Copy link
Copy Markdown
Collaborator

MauiBot commented May 5, 2026

🤖 AI Summary

👋 @KitKeen — new AI review results are available. Please review the latest session below.

📊 Review Session93c354b · Fix copy constructor not preserving AltitudeReferenceSystem · 2026-05-05 20:32 UTC
🚦 Gate — Test Before & After Fix

Gate Result: ❌ FAILED

Platform: ANDROID · Base: main · Merge base: 1463c4c5

🩺 Fix does not pass the tests — every test still fails after applying the fix. The PR's change does not resolve the failure(s).

Test Without Fix (expect FAIL) With Fix (expect PASS)
📱 Geolocation_Tests (ToLocation_NoAltitude_UsesUnspecifiedReferenceSystem, ToLocation_EllipsoidalAltitude_UsesEllipsoidReferenceSystem, ToLocation_HasAltitudeButNoMslAltitude_UsesEllipsoid, ToLocation_MslAltitude_UsesGeoidReferenceSystem, LocationCopyConstructor_PreservesAltitudeReferenceSystem, ToLocation_MslAltitudeWithoutMslAccuracy_ReportsNullVerticalAccuracy) Category=Android Geolocation ✅ FAIL — 1227s ❌ FAIL — 349s
🔴 Without fix — 📱 Geolocation_Tests (ToLocation_NoAltitude_UsesUnspecifiedReferenceSystem, ToLocation_EllipsoidalAltitude_UsesEllipsoidReferenceSystem, ToLocation_HasAltitudeButNoMslAltitude_UsesEllipsoid, ToLocation_MslAltitude_UsesGeoidReferenceSystem, LocationCopyConstructor_PreservesAltitudeReferenceSystem, ToLocation_MslAltitudeWithoutMslAccuracy_ReportsNullVerticalAccuracy): FAIL ✅ · 1227s

(truncated to last 15,000 chars)

t.Encoding.dll -> System.Text.Encoding.dll.so
  [40/129] Xamarin.AndroidX.Lifecycle.Common.Jvm.dll -> Xamarin.AndroidX.Lifecycle.Common.Jvm.dll.so
  [113/129] System.Text.Encodings.Web.dll -> System.Text.Encodings.Web.dll.so
  [41/129] Xamarin.AndroidX.Lifecycle.LiveData.Core.dll -> Xamarin.AndroidX.Lifecycle.LiveData.Core.dll.so
  [42/129] Xamarin.AndroidX.Lifecycle.ViewModel.Android.dll -> Xamarin.AndroidX.Lifecycle.ViewModel.Android.dll.so
  [43/129] Xamarin.AndroidX.Lifecycle.ViewModelSavedState.Android.dll -> Xamarin.AndroidX.Lifecycle.ViewModelSavedState.Android.dll.so
  [114/129] System.Text.RegularExpressions.dll -> System.Text.RegularExpressions.dll.so
  [44/129] Xamarin.AndroidX.Loader.dll -> Xamarin.AndroidX.Loader.dll.so
  [115/129] System.Threading.Tasks.Parallel.dll -> System.Threading.Tasks.Parallel.dll.so
  [45/129] Xamarin.AndroidX.Navigation.Common.Android.dll -> Xamarin.AndroidX.Navigation.Common.Android.dll.so
  [116/129] System.Threading.Tasks.dll -> System.Threading.Tasks.dll.so
  [117/129] System.Text.Json.dll -> System.Text.Json.dll.so
  [46/129] Xamarin.AndroidX.Navigation.Fragment.dll -> Xamarin.AndroidX.Navigation.Fragment.dll.so
  [118/129] System.Threading.Thread.dll -> System.Threading.Thread.dll.so
  [119/129] System.Threading.ThreadPool.dll -> System.Threading.ThreadPool.dll.so
  [120/129] System.Threading.dll -> System.Threading.dll.so
  [47/129] Xamarin.AndroidX.Navigation.Runtime.Android.dll -> Xamarin.AndroidX.Navigation.Runtime.Android.dll.so
  [121/129] System.Xml.Linq.dll -> System.Xml.Linq.dll.so
  [122/129] System.Xml.ReaderWriter.dll -> System.Xml.ReaderWriter.dll.so
  [123/129] System.Xml.XDocument.dll -> System.Xml.XDocument.dll.so
  [48/129] Xamarin.AndroidX.Navigation.UI.dll -> Xamarin.AndroidX.Navigation.UI.dll.so
  [124/129] System.dll -> System.dll.so
  [125/129] netstandard.dll -> netstandard.dll.so
  [49/129] Xamarin.AndroidX.RecyclerView.dll -> Xamarin.AndroidX.RecyclerView.dll.so
  [126/129] Mono.Android.Runtime.dll -> Mono.Android.Runtime.dll.so
  [50/129] Xamarin.AndroidX.SavedState.SavedState.Android.dll -> Xamarin.AndroidX.SavedState.SavedState.Android.dll.so
  [127/129] Java.Interop.dll -> Java.Interop.dll.so
  [51/129] Xamarin.AndroidX.Security.SecurityCrypto.dll -> Xamarin.AndroidX.Security.SecurityCrypto.dll.so
  [52/129] Xamarin.AndroidX.SwipeRefreshLayout.dll -> Xamarin.AndroidX.SwipeRefreshLayout.dll.so
  [53/129] Xamarin.AndroidX.ViewPager.dll -> Xamarin.AndroidX.ViewPager.dll.so
  [54/129] Xamarin.AndroidX.ViewPager2.dll -> Xamarin.AndroidX.ViewPager2.dll.so
  [55/129] Xamarin.Google.Android.Material.dll -> Xamarin.Google.Android.Material.dll.so
  [128/129] Mono.Android.dll -> Mono.Android.dll.so
  [56/129] Xamarin.Google.Crypto.Tink.Android.dll -> Xamarin.Google.Crypto.Tink.Android.dll.so
  [57/129] Xamarin.Kotlin.StdLib.dll -> Xamarin.Kotlin.StdLib.dll.so
  [58/129] Xamarin.KotlinX.Coroutines.Core.Jvm.dll -> Xamarin.KotlinX.Coroutines.Core.Jvm.dll.so
  [59/129] Xamarin.KotlinX.Serialization.Core.Jvm.dll -> Xamarin.KotlinX.Serialization.Core.Jvm.dll.so
  [60/129] xunit.abstractions.dll -> xunit.abstractions.dll.so
  [61/129] xunit.assert.dll -> xunit.assert.dll.so
  [62/129] xunit.core.dll -> xunit.core.dll.so
  [63/129] xunit.execution.dotnet.dll -> xunit.execution.dotnet.dll.so
  [64/129] xunit.runner.utility.netcoreapp10.dll -> xunit.runner.utility.netcoreapp10.dll.so
  [65/129] System.Collections.Concurrent.dll -> System.Collections.Concurrent.dll.so
  [66/129] System.Collections.Immutable.dll -> System.Collections.Immutable.dll.so
  [67/129] System.Collections.NonGeneric.dll -> System.Collections.NonGeneric.dll.so
  [68/129] System.Collections.Specialized.dll -> System.Collections.Specialized.dll.so
  [69/129] System.Collections.dll -> System.Collections.dll.so
  [70/129] System.ComponentModel.Primitives.dll -> System.ComponentModel.Primitives.dll.so
  [71/129] System.ComponentModel.TypeConverter.dll -> System.ComponentModel.TypeConverter.dll.so
  [72/129] System.ComponentModel.dll -> System.ComponentModel.dll.so
  [73/129] System.Console.dll -> System.Console.dll.so
  [74/129] System.Diagnostics.Debug.dll -> System.Diagnostics.Debug.dll.so
  [75/129] System.Diagnostics.DiagnosticSource.dll -> System.Diagnostics.DiagnosticSource.dll.so
  [76/129] System.Diagnostics.Process.dll -> System.Diagnostics.Process.dll.so
  [77/129] System.Diagnostics.Tools.dll -> System.Diagnostics.Tools.dll.so
  [78/129] System.Diagnostics.TraceSource.dll -> System.Diagnostics.TraceSource.dll.so
  [129/129] System.Private.CoreLib.dll -> System.Private.CoreLib.dll.so
  [79/129] System.Diagnostics.Tracing.dll -> System.Diagnostics.Tracing.dll.so
  [80/129] System.Drawing.Primitives.dll -> System.Drawing.Primitives.dll.so
  [81/129] System.Drawing.dll -> System.Drawing.dll.so
  [82/129] System.Formats.Asn1.dll -> System.Formats.Asn1.dll.so
  [83/129] System.Globalization.dll -> System.Globalization.dll.so
  [84/129] System.IO.Compression.Brotli.dll -> System.IO.Compression.Brotli.dll.so
  [85/129] System.IO.Compression.dll -> System.IO.Compression.dll.so
  [86/129] System.IO.FileSystem.dll -> System.IO.FileSystem.dll.so
  [87/129] System.IO.Pipelines.dll -> System.IO.Pipelines.dll.so
  [88/129] System.IO.dll -> System.IO.dll.so
  [89/129] System.Linq.Expressions.dll -> System.Linq.Expressions.dll.so
  [90/129] System.Linq.dll -> System.Linq.dll.so
  [91/129] System.Memory.dll -> System.Memory.dll.so
  [92/129] System.Net.Http.dll -> System.Net.Http.dll.so
  [93/129] System.Net.NameResolution.dll -> System.Net.NameResolution.dll.so
  [94/129] System.Net.Primitives.dll -> System.Net.Primitives.dll.so
  [95/129] System.Net.Requests.dll -> System.Net.Requests.dll.so
  [96/129] System.Net.Sockets.dll -> System.Net.Sockets.dll.so
  [97/129] System.Numerics.Vectors.dll -> System.Numerics.Vectors.dll.so
  [98/129] System.ObjectModel.dll -> System.ObjectModel.dll.so
  [99/129] System.Private.Uri.dll -> System.Private.Uri.dll.so
  [100/129] System.Private.Xml.Linq.dll -> System.Private.Xml.Linq.dll.so
  [101/129] System.Private.Xml.dll -> System.Private.Xml.dll.so
  [102/129] System.Reflection.Extensions.dll -> System.Reflection.Extensions.dll.so
  [103/129] System.Reflection.TypeExtensions.dll -> System.Reflection.TypeExtensions.dll.so
  [104/129] System.Reflection.dll -> System.Reflection.dll.so
  [105/129] System.Runtime.Extensions.dll -> System.Runtime.Extensions.dll.so
  [106/129] System.Runtime.InteropServices.RuntimeInformation.dll -> System.Runtime.InteropServices.RuntimeInformation.dll.so
  [107/129] System.Runtime.InteropServices.dll -> System.Runtime.InteropServices.dll.so
  [108/129] System.Runtime.Loader.dll -> System.Runtime.Loader.dll.so
  [109/129] System.Runtime.Numerics.dll -> System.Runtime.Numerics.dll.so
  [110/129] System.Runtime.dll -> System.Runtime.dll.so
  [111/129] System.Security.Cryptography.dll -> System.Security.Cryptography.dll.so
  [112/129] System.Text.Encoding.dll -> System.Text.Encoding.dll.so
  [113/129] System.Text.Encodings.Web.dll -> System.Text.Encodings.Web.dll.so
  [114/129] System.Text.Json.dll -> System.Text.Json.dll.so
  [115/129] System.Text.RegularExpressions.dll -> System.Text.RegularExpressions.dll.so
  [116/129] System.Threading.Tasks.Parallel.dll -> System.Threading.Tasks.Parallel.dll.so
  [117/129] System.Threading.Tasks.dll -> System.Threading.Tasks.dll.so
  [118/129] System.Threading.Thread.dll -> System.Threading.Thread.dll.so
  [119/129] System.Threading.ThreadPool.dll -> System.Threading.ThreadPool.dll.so
  [120/129] System.Threading.dll -> System.Threading.dll.so
  [121/129] System.Xml.Linq.dll -> System.Xml.Linq.dll.so
  [122/129] System.Xml.ReaderWriter.dll -> System.Xml.ReaderWriter.dll.so
  [123/129] System.Xml.XDocument.dll -> System.Xml.XDocument.dll.so
  [124/129] System.dll -> System.dll.so
  [125/129] netstandard.dll -> netstandard.dll.so
  [126/129] Java.Interop.dll -> Java.Interop.dll.so
  [127/129] Mono.Android.Runtime.dll -> Mono.Android.Runtime.dll.so
  [128/129] Mono.Android.dll -> Mono.Android.dll.so
  [129/129] System.Private.CoreLib.dll -> System.Private.CoreLib.dll.so

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:10:09.79
[11.0.0-prerelease.26107.1+bfbac237157e59cdbd19334325b2af80bd6e9828] XHarness command issued: android test --app /home/vsts/work/1/s/artifacts/bin/Essentials.DeviceTests/Release/net10.0-android/com.microsoft.maui.essentials.devicetests-Signed.apk --package-name com.microsoft.maui.essentials.devicetests --device-id emulator-5554 -o artifacts/log --timeout 01:00:00 -v --arg TestFilter=Category=Android Geolocation
�[40m�[37mdbug�[39m�[22m�[49m: ADBRunner using ADB.exe supplied from /home/vsts/.nuget/packages/microsoft.dotnet.xharness.cli/11.0.0-prerelease.26107.1/tools/net10.0/any/../../../runtimes/any/native/adb/linux/adb
�[40m�[37mdbug�[39m�[22m�[49m: Full resolved path:'/home/vsts/.nuget/packages/microsoft.dotnet.xharness.cli/11.0.0-prerelease.26107.1/runtimes/any/native/adb/linux/adb'
�[40m�[32minfo�[39m�[22m�[49m: Will attempt to find device supporting architectures: 'arm64-v8a', 'x86_64'
�[40m�[37mdbug�[39m�[22m�[49m: Executing command: '/home/vsts/.nuget/packages/microsoft.dotnet.xharness.cli/11.0.0-prerelease.26107.1/runtimes/any/native/adb/linux/adb start-server'
�[40m�[37mdbug�[39m�[22m�[49m: 
�[40m�[32minfo�[39m�[22m�[49m: Finding attached devices/emulators...
�[40m�[37mdbug�[39m�[22m�[49m: Executing command: '/home/vsts/.nuget/packages/microsoft.dotnet.xharness.cli/11.0.0-prerelease.26107.1/runtimes/any/native/adb/linux/adb devices -l'
�[40m�[37mdbug�[39m�[22m�[49m: Found 1 possible devices
�[40m�[37mdbug�[39m�[22m�[49m: Evaluating output line for device serial: emulator-5554          device product:sdk_gphone_x86_64 model:sdk_gphone_x86_64 device:generic_x86_64_arm64 transport_id:1
�[40m�[37mdbug�[39m�[22m�[49m: Executing command: '/home/vsts/.nuget/packages/microsoft.dotnet.xharness.cli/11.0.0-prerelease.26107.1/runtimes/any/native/adb/linux/adb -s emulator-5554 shell getprop ro.product.cpu.abilist'
�[40m�[37mdbug�[39m�[22m�[49m: Found 1 possible devices. Using 'emulator-5554'
�[40m�[32minfo�[39m�[22m�[49m: Active Android device set to serial 'emulator-5554'
�[40m�[37mdbug�[39m�[22m�[49m: Executing command: '/home/vsts/.nuget/packages/microsoft.dotnet.xharness.cli/11.0.0-prerelease.26107.1/runtimes/any/native/adb/linux/adb -s emulator-5554 -s emulator-5554 shell getprop ro.product.cpu.abi'
�[40m�[37mdbug�[39m�[22m�[49m: Executing command: '/home/vsts/.nuget/packages/microsoft.dotnet.xharness.cli/11.0.0-prerelease.26107.1/runtimes/any/native/adb/linux/adb -s emulator-5554 -s emulator-5554 shell getprop ro.build.version.sdk'
�[40m�[32minfo�[39m�[22m�[49m: Waiting for device to be available (max 5 minutes)
�[40m�[37mdbug�[39m�[22m�[49m: Executing command: '/home/vsts/.nuget/packages/microsoft.dotnet.xharness.cli/11.0.0-prerelease.26107.1/runtimes/any/native/adb/linux/adb -s emulator-5554 wait-for-device'
�[40m�[37mdbug�[39m�[22m�[49m: Executing command: '/home/vsts/.nuget/packages/microsoft.dotnet.xharness.cli/11.0.0-prerelease.26107.1/runtimes/any/native/adb/linux/adb -s emulator-5554 -s emulator-5554 shell getprop sys.boot_completed'
�[40m�[37mdbug�[39m�[22m�[49m: sys.boot_completed = '1'
�[40m�[37mdbug�[39m�[22m�[49m: Waited 0 seconds for device boot completion
�[40m�[37mdbug�[39m�[22m�[49m: Working with emulator-5554 (API 30)
�[40m�[37mdbug�[39m�[22m�[49m: Check current adb install and/or package verification settings
�[40m�[37mdbug�[39m�[22m�[49m: Executing command: '/home/vsts/.nuget/packages/microsoft.dotnet.xharness.cli/11.0.0-prerelease.26107.1/runtimes/any/native/adb/linux/adb -s emulator-5554 shell settings get global verifier_verify_adb_installs'
�[40m�[37mdbug�[39m�[22m�[49m: verifier_verify_adb_installs = 0
�[40m�[37mdbug�[39m�[22m�[49m: Executing command: '/home/vsts/.nuget/packages/microsoft.dotnet.xharness.cli/11.0.0-prerelease.26107.1/runtimes/any/native/adb/linux/adb -s emulator-5554 shell settings get global package_verifier_enable'
�[40m�[37mdbug�[39m�[22m�[49m: package_verifier_enable = 
�[40m�[1m�[33mwarn�[39m�[22m�[49m: Installing debug apks on a device might be rejected with INSTALL_FAILED_VERIFICATION_FAILURE. Make sure to set 'package_verifier_enable' to '0'
�[40m�[32minfo�[39m�[22m�[49m: Attempting to remove apk 'com.microsoft.maui.essentials.devicetests'..
�[40m�[37mdbug�[39m�[22m�[49m: Executing command: '/home/vsts/.nuget/packages/microsoft.dotnet.xharness.cli/11.0.0-prerelease.26107.1/runtimes/any/native/adb/linux/adb -s emulator-5554 uninstall com.microsoft.maui.essentials.devicetests'
�[41m�[30mfail�[39m�[22m�[49m: Waiting for command timed out: execution may be compromised
�[41m�[30mfail�[39m�[22m�[49m: Error: Exit code: -2
      Std out:
      
      
      
�[40m�[32minfo�[39m�[22m�[49m: Attempting to install /home/vsts/work/1/s/artifacts/bin/Essentials.DeviceTests/Release/net10.0-android/com.microsoft.maui.essentials.devicetests-Signed.apk
�[40m�[37mdbug�[39m�[22m�[49m: Executing command: '/home/vsts/.nuget/packages/microsoft.dotnet.xharness.cli/11.0.0-prerelease.26107.1/runtimes/any/native/adb/linux/adb -s emulator-5554 install /home/vsts/work/1/s/artifacts/bin/Essentials.DeviceTests/Release/net10.0-android/com.microsoft.maui.essentials.devicetests-Signed.apk'
�[41m�[30mfail�[39m�[22m�[49m: Error:
      Exit code: 1
      Std out:
      Serving...
      Performing Incremental Install
      cmd: Failure calling service package: Broken pipe (32)
      Performing Streamed Install
      
      
      Std err:
      All files should be loaded. Notifying the device.
      adb: failed to install /home/vsts/work/1/s/artifacts/bin/Essentials.DeviceTests/Release/net10.0-android/com.microsoft.maui.essentials.devicetests-Signed.apk: cmd: Can't find service: package
      
      
      
�[41m�[1m�[37mcrit�[39m�[22m�[49m: Install failure: Test command cannot continue
�[40m�[32minfo�[39m�[22m�[49m: Attempting to remove apk 'com.microsoft.maui.essentials.devicetests'..
�[40m�[37mdbug�[39m�[22m�[49m: Executing command: '/home/vsts/.nuget/packages/microsoft.dotnet.xharness.cli/11.0.0-prerelease.26107.1/runtimes/any/native/adb/linux/adb -s emulator-5554 uninstall com.microsoft.maui.essentials.devicetests'
�[41m�[30mfail�[39m�[22m�[49m: Error: Exit code: 20
      Std out:
      
      
      Std err:
      cmd: Can't find service: package
      
      
      
�[40m�[32minfo�[39m�[22m�[49m: Attempting to remove apk 'com.microsoft.maui.essentials.devicetests'..
�[40m�[37mdbug�[39m�[22m�[49m: Executing command: '/home/vsts/.nuget/packages/microsoft.dotnet.xharness.cli/11.0.0-prerelease.26107.1/runtimes/any/native/adb/linux/adb -s emulator-5554 uninstall com.microsoft.maui.essentials.devicetests'
�[41m�[30mfail�[39m�[22m�[49m: Error: Exit code: 20
      Std out:
      
      
      Std err:
      cmd: Can't find service: package
      
      
      
XHarness exit code: 78 (PACKAGE_INSTALLATION_FAILURE)
  Tests completed with exit code: 78

🟢 With fix — 📱 Geolocation_Tests (ToLocation_NoAltitude_UsesUnspecifiedReferenceSystem, ToLocation_EllipsoidalAltitude_UsesEllipsoidReferenceSystem, ToLocation_HasAltitudeButNoMslAltitude_UsesEllipsoid, ToLocation_MslAltitude_UsesGeoidReferenceSystem, LocationCopyConstructor_PreservesAltitudeReferenceSystem, ToLocation_MslAltitudeWithoutMslAccuracy_ReportsNullVerticalAccuracy): FAIL ❌ · 349s

(truncated to last 15,000 chars)

05 12:22:04.788 11987 16256 W PhBaseOp: 	at dxzg.f(:com.google.android.gms@261631026@26.16.31 (150800-900800821):16)
      05-05 12:22:04.788 11987 16256 W PhBaseOp: 	at csew.fB(:com.google.android.gms@261631026@26.16.31 (150800-900800821):1)
      05-05 12:22:04.788 11987 16256 W PhBaseOp: 	at csfh.run(:com.google.android.gms@261631026@26.16.31 (150800-900800821):121)
      05-05 12:22:04.788 11987 16256 W PhBaseOp: 	at fttg.run(:com.google.android.gms@261631026@26.16.31 (150800-900800821):23)
      05-05 12:22:04.788 11987 16256 W PhBaseOp: 	at bfzu.c(:com.google.android.gms@261631026@26.16.31 (150800-900800821):50)
      05-05 12:22:04.788 11987 16256 W PhBaseOp: 	at bfzu.run(:com.google.android.gms@261631026@26.16.31 (150800-900800821):66)
      05-05 12:22:04.788 11987 16256 W PhBaseOp: 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
      05-05 12:22:04.788 11987 16256 W PhBaseOp: 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
      05-05 12:22:04.788 11987 16256 W PhBaseOp: 	at bgfl.run(:com.google.android.gms@261631026@26.16.31 (150800-900800821):8)
      05-05 12:22:04.788 11987 16256 W PhBaseOp: 	at java.lang.Thread.run(Thread.java:923)
      05-05 12:22:04.792 12609 15676 W gle.android.gm: Verification of void hwkv.b(hwhy) took 130.372ms (1273.27 bytecodes/s) (5200B approximate peak alloc)
      05-05 12:22:04.798 11987 16280 I SystemMemoryMap: (REDACTED) [activityrecognition] serialization complete (extras/values=%.2f/%.2fkb)
      05-05 12:22:04.812 11987 16256 W AsyncOperation: operation=CommitToConfigurationOperationCall, opStatusCode=29538 [CONTEXT service_id=51 ]
      05-05 12:22:04.812 11987 16256 W AsyncOperation: OperationException[Status{statusCode=Stale snapshot for com.google.android.apps.wellbeing.device(change count changed - expected 5  but was 4), resolution=null}]
      05-05 12:22:04.812 11987 16256 W AsyncOperation: 	at dxzg.h(:com.google.android.gms@261631026@26.16.31 (150800-900800821):64)
      05-05 12:22:04.812 11987 16256 W AsyncOperation: 	at dxzg.f(:com.google.android.gms@261631026@26.16.31 (150800-900800821):16)
      05-05 12:22:04.812 11987 16256 W AsyncOperation: 	at csew.fB(:com.google.android.gms@261631026@26.16.31 (150800-900800821):1)
      05-05 12:22:04.812 11987 16256 W AsyncOperation: 	at csfh.run(:com.google.android.gms@261631026@26.16.31 (150800-900800821):121)
      05-05 12:22:04.812 11987 16256 W AsyncOperation: 	at fttg.run(:com.google.android.gms@261631026@26.16.31 (150800-900800821):23)
      05-05 12:22:04.812 11987 16256 W AsyncOperation: 	at bfzu.c(:com.google.android.gms@261631026@26.16.31 (150800-900800821):50)
      05-05 12:22:04.812 11987 16256 W AsyncOperation: 	at bfzu.run(:com.google.android.gms@261631026@26.16.31 (150800-900800821):66)
      05-05 12:22:04.812 11987 16256 W AsyncOperation: 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
      05-05 12:22:04.812 11987 16256 W AsyncOperation: 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
      05-05 12:22:04.812 11987 16256 W AsyncOperation: 	at bgfl.run(:com.google.android.gms@261631026@26.16.31 (150800-900800821):8)
      05-05 12:22:04.812 11987 16256 W AsyncOperation: 	at java.lang.Thread.run(Thread.java:923)
      05-05 12:22:04.820 12609 16260 I ChimeraConfigurator: Checking 1 listeners. All downloaded: false
      05-05 12:22:04.829 16209 16224 W DynamiteModule: Local module descriptor class for com.google.android.gms.googlecertificates not found.
      05-05 12:22:04.861 16209 16224 W .apps.wellbein: ClassLoaderContext classpath size mismatch. expected=14, found=0 (DLC[];PCL[base.apk*3555418091:base.apk!classes2.dex*3336391460:base.apk!classes3.dex*3958618713:base.apk!classes4.dex*3022934062:base.apk!classes5.dex*2476764232:base.apk!classes6.dex*1612679893:base.apk!classes7.dex*1823849344:base.apk!classes8.dex*466855726:base.apk!classes9.dex*4227236518:base.apk!classes10.dex*169475094:base.apk!classes11.dex*2053099836:base.apk!classes12.dex*95922838:base.apk!classes13.dex*3324838914:base.apk!classes14.dex*2736438263]{PCL[/system/framework/org.apache.http.legacy.jar*1452144318]#PCL[/system/framework/com.android.location.provider.jar*4177849200]#PCL[/system/framework/com.android.media.remotedisplay.jar*1128910120]#PCL[/system_ext/framework/androidx.window.sidecar.jar*709954415]} | DLC[];PCL[])
      05-05 12:22:04.863 16209 16224 I DynamiteModule: Considering local module com.google.android.gms.googlecertificates:0 and remote module com.google.android.gms.googlecertificates:7
      05-05 12:22:04.863 16209 16224 I DynamiteModule: Selected remote version of com.google.android.gms.googlecertificates, version >= 7
      05-05 12:22:04.866 16209 16224 W System  : ClassLoader referenced unknown path: 
      05-05 12:22:04.902 12326 12821 I Finsky  : [166] SCH: Scheduling phonesky job Id: 26-1414141414, CT: 1778008538037, Constraints: [{ L: 43199988, D: 44099988, C: CHARGING_NONE, I: IDLE_NONE, N: NET_NONE, B: BATTERY_ANY }]
      05-05 12:22:04.925 12326 12637 I Finsky  : [141] SCH: Scheduling 1 system job(s)
      05-05 12:22:04.925 12326 12637 I Finsky  : [141] SCH: Scheduling system job Id: 9307, L: 42813100, D: 43713100, C: false, I: false, N: 0
      05-05 12:22:04.928 12326 16283 I Finsky  : [235] SCH: job service finished with id 9300.
      05-05 12:22:04.932 16209 16224 W .apps.wellbein: ClassLoaderContext classpath size mismatch. expected=14, found=2 (DLC[];PCL[base.apk*3555418091:base.apk!classes2.dex*3336391460:base.apk!classes3.dex*3958618713:base.apk!classes4.dex*3022934062:base.apk!classes5.dex*2476764232:base.apk!classes6.dex*1612679893:base.apk!classes7.dex*1823849344:base.apk!classes8.dex*466855726:base.apk!classes9.dex*4227236518:base.apk!classes10.dex*169475094:base.apk!classes11.dex*2053099836:base.apk!classes12.dex*95922838:base.apk!classes13.dex*3324838914:base.apk!classes14.dex*2736438263]{PCL[/system/framework/org.apache.http.legacy.jar*1452144318]#PCL[/system/framework/com.android.location.provider.jar*4177849200]#PCL[/system/framework/com.android.media.remotedisplay.jar*1128910120]#PCL[/system_ext/framework/androidx.window.sidecar.jar*709954415]} | DLC[];PCL[/product/priv-app/WellbeingPrebuilt/WellbeingPrebuilt.apk*3105014714:/product/priv-app/WellbeingPrebuilt/WellbeingPrebuilt.apk!classes2.dex*659704715]{PCL[/system/framework/android.test.base.jar*315452938]#PCL[/system/framework/org.apache.http.legacy.jar*1452144318]})
      05-05 12:22:04.932 16209 16224 W .apps.wellbein: Found duplicate classes, falling back to extracting from APK : /data/app/~~kkSGM7GxDm1qKV1RGeojPA==/com.google.android.gms-1YSasfZqvqngMMGQ2T91Ow==/split_GoogleCertificates_installtime.apk
      05-05 12:22:04.932 16209 16224 W .apps.wellbein: NOTE: This wastes RAM and hurts startup performance.
      05-05 12:22:04.932 16209 16224 W .apps.wellbein: Found duplicated class when checking oat files: 'Landroid/support/annotation/Keep;' in /data/app/~~kkSGM7GxDm1qKV1RGeojPA==/com.google.android.gms-1YSasfZqvqngMMGQ2T91Ow==/split_GoogleCertificates_installtime.apk!classes2.dex and /product/priv-app/WellbeingPrebuilt/WellbeingPrebuilt.apk
      05-05 12:22:04.996 12326 12637 I Finsky  : [141] Not an active Cubes user. Ignoring package install/uninstall event.
      05-05 12:22:05.003 11987 11991 I .gms.persisten: Background young concurrent copying GC freed 53922(2595KB) AllocSpace objects, 12(368KB) LOS objects, 5% free, 40MB/43MB, paused 24us total 351.392ms
      05-05 12:22:05.290 12609 13636 I cn_CronetUrlRequestContext: destroyNativeStreamLocked org.chromium.net.impl.CronetBidirectionalStream@988c7f5
      05-05 12:22:05.292 12609 15676 W feks    : Primes not initialized, returning default (no-op) Primes instance which will ignore all calls. Please call Primes.initialize(...) before using any Primes API.
      05-05 12:22:05.468 12609 16134 I Icing   : IndexChimeraService.getServiceInterface callingPackage=com.google.android.gms componentName=AppsCorpus serviceId=36
      05-05 12:22:05.505 11373 12210 W PackageManager: Cannot suspend package "com.android.vending": required for package verification
      05-05 12:22:05.506 11373 12210 W PackageManager: Cannot suspend the platform package: android
      05-05 12:22:05.506 11373 12210 W PackageManager: Cannot suspend package "com.google.android.permissioncontroller": required for permissions management
      05-05 12:22:05.507 11373 12210 W PackageManager: Cannot suspend package "com.android.dialer": is the default dialer
      05-05 12:22:05.507 11373 12210 W PackageManager: Cannot suspend package "com.google.android.packageinstaller": required for package installation
      05-05 12:22:05.508 11373 12210 W PackageManager: Cannot suspend package "com.google.android.apps.nexuslauncher": contains the active launcher
      05-05 12:22:03.300 11373 11373 W Binder:11373_12: type=1400 audit(0.0:428): avc: denied { getopt } for scontext=u:r:system_server:s0 tcontext=u:r:shell:s0 tclass=unix_stream_socket permissive=0
      05-05 12:22:05.588 12609 15676 I Icing   : Usage reports ok 0, Failed Usage reports 0, indexed 0, rejected 0
      05-05 12:22:05.592 12609 16134 I Icing   : IndexChimeraService.getServiceInterface callingPackage=com.google.android.gms componentName=null serviceId=30
      05-05 12:22:05.626 12609 12617 I gle.android.gm: Background concurrent copying GC freed 257554(10MB) AllocSpace objects, 36(12MB) LOS objects, 49% free, 23MB/46MB, paused 36us total 894.923ms
      05-05 12:22:05.665   288  1140 I bt      : system/bt/vendor_libs/test_vendor_lib/model/devices/scripted_beacon.cc:113 set_state: Events file not opened yet, for event: 2
      05-05 12:22:05.672   288  1140 I bt      : system/bt/vendor_libs/test_vendor_lib/model/devices/scripted_beacon.cc:113 set_state: Events file not opened yet, for event: 3
      05-05 12:22:03.300 11373 11373 W Binder:11373_12: type=1400 audit(0.0:429): avc: denied { getopt } for scontext=u:r:system_server:s0 tcontext=u:r:shell:s0 tclass=unix_stream_socket permissive=0
      05-05 12:22:05.983 12609 15676 I Icing   : Usage reports ok 0, Failed Usage reports 0, indexed 0, rejected 0
      05-05 12:22:05.993 12609 15676 I Icing   : Indexing com.google.android.gms-apps from com.google.android.gms
      05-05 12:22:06.136 11987 13383 I Nearby  : (REDACTED) is blocked device type %s, isAuto=%s, isIot=%s, isLatchsky=%s, isChinaWearable=%s, isTv=%s, isWearable=%s (supportWearOs=%s), isChromeOsDevice=%s (supportChromeOs=%s)
      05-05 12:22:06.140 12609 15676 I Icing   : Indexing done com.google.android.gms-apps
      05-05 12:22:06.145 11987 13383 I NearbyDiscovery: (REDACTED) DiscoveryController, showNotification, itemSize=%s
      05-05 12:22:06.145 11987 13383 I NearbyDiscovery: (REDACTED) Show notifications: %d total, no changes since last shown, no-op.
      05-05 12:22:06.223 16301 16301 D AndroidRuntime: >>>>>> START com.android.internal.os.RuntimeInit uid 2000 <<<<<<
      05-05 12:22:06.270 16301 16301 E libc    : Access denied finding property "persist.device_config.runtime_native_boot.profilebootclasspath"
      05-05 12:22:06.270 16301 16301 E libc    : Access denied finding property "persist.device_config.runtime_native_boot.enable_apex_image"
      05-05 12:22:06.270 16301 16301 I AndroidRuntime: Using default boot image
      05-05 12:22:06.271 16301 16301 E libc    : Access denied finding property "persist.device_config.runtime_native_boot.disable_lock_profiling"
      05-05 12:22:06.271 16301 16301 I AndroidRuntime: Leaving lock profiling enabled
      05-05 12:22:06.271 16301 16301 E libc    : Access denied finding property "persist.device_config.runtime_native_boot.enable_generational_cc"
      05-05 12:22:06.256 16301 16301 W app_process: type=1400 audit(0.0:430): avc: denied { read } for name="u:object_r:device_config_runtime_native_boot_prop:s0" dev="tmpfs" ino=7421 scontext=u:r:shell:s0 tcontext=u:object_r:device_config_runtime_native_boot_prop:s0 tclass=file permissive=0
      05-05 12:22:06.508 16301 16301 D app_process: Time zone APEX ICU file found: /apex/com.android.tzdata/etc/icu/icu_tzdata.dat
      05-05 12:22:06.508 16301 16301 D app_process: I18n APEX ICU file found: /apex/com.android.i18n/etc/icu/icudt66l.dat
      05-05 12:22:06.534 16301 16301 W app_process: Unexpected CPU variant for X86 using defaults: x86_64
      05-05 12:22:06.539 16301 16301 I app_process: The ClassLoaderContext is a special shared library.
      05-05 12:22:06.268 16301 16301 W app_process: type=1400 audit(0.0:431): avc: denied { read } for name="u:object_r:device_config_runtime_native_boot_prop:s0" dev="tmpfs" ino=7421 scontext=u:r:shell:s0 tcontext=u:object_r:device_config_runtime_native_boot_prop:s0 tclass=file permissive=0
      05-05 12:22:06.574 16301 16301 W app_process: JNI RegisterNativeMethods: attempt to register 0 native methods for android.media.AudioAttributes
      05-05 12:22:06.617 16301 16301 D AndroidRuntime: Calling main entry com.android.commands.am.Am
      05-05 12:22:06.649 16301 16301 D AndroidRuntime: Shutting down VM
      05-05 12:22:06.268 16301 16301 W app_process: type=1400 audit(0.0:432): avc: denied { read } for name="u:object_r:device_config_runtime_native_boot_prop:s0" dev="tmpfs" ino=7421 scontext=u:r:shell:s0 tcontext=u:object_r:device_config_runtime_native_boot_prop:s0 tclass=file permissive=0
      05-05 12:22:06.268 16301 16301 W app_process: type=1400 audit(0.0:433): avc: denied { read } for name="u:object_r:device_config_runtime_native_boot_prop:s0" dev="tmpfs" ino=7421 scontext=u:r:shell:s0 tcontext=u:object_r:device_config_runtime_native_boot_prop:s0 tclass=file permissive=0
      05-05 12:22:07.129   433   433 E netmgr  : qemu_pipe_open_ns:62: Could not connect to the 'pipe:qemud:network' service: Invalid argument
      05-05 12:22:07.129   433   433 E netmgr  : Failed to open QEMU pipe 'qemud:network': Invalid argument
      05-05 12:22:07.157 12609 16272 I GmsDebugLogger: [56] Pay.optional_261631100800_3.apk
      05-05 12:22:07.166 12609 16272 I DynamicModuleDownloader: Using base module file stored externally.
      05-05 12:22:07.224   453   453 E wifi_forwarder: qemu_pipe_open_ns:62: Could not connect to the 'pipe:qemud:wififorward' service: Invalid argument
      05-05 12:22:07.224   453   453 E wifi_forwarder: RemoteConnection failed to initialize: RemoteConnection failed to open pipe
      
�[40m�[32minfo�[39m�[22m�[49m: Attempting to remove apk 'com.microsoft.maui.essentials.devicetests'..
�[40m�[37mdbug�[39m�[22m�[49m: Executing command: '/home/vsts/.nuget/packages/microsoft.dotnet.xharness.cli/11.0.0-prerelease.26107.1/runtimes/any/native/adb/linux/adb -s emulator-5554 uninstall com.microsoft.maui.essentials.devicetests'
�[40m�[32minfo�[39m�[22m�[49m: Successfully uninstalled com.microsoft.maui.essentials.devicetests
XHarness exit code: 82 (RETURN_CODE_NOT_SET)
  Tests completed with exit code: 82

⚠️ Failure Details

  • Geolocation_Tests (ToLocation_NoAltitude_UsesUnspecifiedReferenceSystem, ToLocation_EllipsoidalAltitude_UsesEllipsoidReferenceSystem, ToLocation_HasAltitudeButNoMslAltitude_UsesEllipsoid, ToLocation_MslAltitude_UsesGeoidReferenceSystem, LocationCopyConstructor_PreservesAltitudeReferenceSystem, ToLocation_MslAltitudeWithoutMslAccuracy_ReportsNullVerticalAccuracy) FAILED with fix (should pass)
📁 Fix files reverted (3 files)
  • eng/pipelines/ci-copilot.yml
  • src/Essentials/src/Types/Location.shared.cs
  • src/Essentials/src/Types/LocationExtensions.android.cs

🧪 UI Tests — Category Detection

Detected UI test categories: Essentials


🔍 Regression Cross-Reference

🔍 Regression Cross-Reference

🟢 No regression risks detected. No labeled bug-fix PRs in the last 6 months touched the modified files.


🔍 Pre-Flight — Context & Validation

Issue: #27554 - Add support for MslAltitudeMeters in Essentials Geolocation on Android
PR: #35097 - [Essentials] Use mean sea level altitude on Android API 34+
Platforms Affected: Android (API 34+ new behavior; pre-34 fallback preserved; all platforms via copy constructor fix)
Files Changed: 2 implementation, 1 test

Key Findings

  • PR adds GetAltitude() helper in LocationExtensions.android.cs that prefers MSL altitude on API 34+ (paired correctly with MslAltitudeAccuracyMeters) and falls back to ellipsoidal altitude on older devices
  • Copy constructor Location(Location point) was missing AltitudeReferenceSystem propagation — fixed in Location.shared.cs:106
  • Previously, HasAltitude == false returned (null, Ellipsoid) — semantically incoherent; now correctly returns (null, Unspecified)
  • New test file adds 6 Android device tests covering all branches of GetAltitude() plus the copy constructor fix
  • Prior code review (session 63444de) flagged VerticalAccuracy/MSL mismatch — fully addressed in current PR: GetAltitude() pairs altitude and accuracy from the same reference system
  • Two tests (ToLocation_EllipsoidalAltitude_UsesEllipsoidReferenceSystem and ToLocation_HasAltitudeButNoMslAltitude_UsesEllipsoid) are functionally identical — both exercise the same code paths

Code Review Summary

Verdict: LGTM
Confidence: high
Errors: 0 | Warnings: 0 | Suggestions: 3

Key code review findings:

  • 💡 src/Essentials/test/DeviceTests/Tests/Android/Geolocation_Tests.cs:46 — Duplicate test: ToLocation_HasAltitudeButNoMslAltitude_UsesEllipsoid is byte-for-byte identical to ToLocation_EllipsoidalAltitude_UsesEllipsoidReferenceSystem; one should be removed
  • 💡 src/Essentials/src/Types/LocationExtensions.android.cs:70 — Cross-platform divergence: iOS always returns AltitudeReferenceSystem.Geoid even when no altitude is available; Android now correctly returns Unspecified; worth tracking a follow-up iOS fix
  • 💡 src/Essentials/test/DeviceTests/Tests/Android/Geolocation_Tests.cs:108LocationCopyConstructor_PreservesAltitudeReferenceSystem tests shared Location.shared.cs code but lives in Android-only test folder; should be in src/Essentials/test/UnitTests/ for all-platform coverage

Fix Candidates

# Source Approach Test Result Files Changed Notes
PR PR #35097 Add GetAltitude() helper to prefer MSL on API 34+, fix copy constructor ❌ FAILED (Gate) LocationExtensions.android.cs, Location.shared.cs, new test file Gate failed — see gate/content.md

🔬 Code Review — Deep Analysis

Code Review — PR #35097

Independent Assessment

What this changes:

  1. Location.shared.cs — The copy constructor Location(Location point) was missing AltitudeReferenceSystem = point.AltitudeReferenceSystem. The property is now propagated correctly during copy (line 106).
  2. LocationExtensions.android.csToLocation(AndroidLocation) is refactored from an expression-body to a block body. A new private GetAltitude() helper encapsulates altitude selection:
    • API 34+ with HasMslAltitude: returns (MslAltitudeMeters, Geoid, MslAltitudeAccuracyMeters)
    • Otherwise with HasAltitude: returns (Altitude, Ellipsoid, VerticalAccuracyMeters@API26+)
    • No altitude: returns (null, Unspecified, null)
  3. Geolocation_Tests.cs — New Android device-test file with 6 tests covering all branches of GetAltitude and the copy-constructor fix.

Inferred motivation:

  • The copy constructor bug silently dropped AltitudeReferenceSystem during Placemark copies (see Placemark.shared.cs:30).
  • On Android, the prior implementation always returned AltitudeReferenceSystem.Ellipsoid even when HasAltitude was false and Altitude was null — a semantically incoherent state. More importantly, it never exposed the geoid-corrected (MSL) altitude introduced in Android API 34, forcing cross-platform consumers to apply manual geoid corrections to reconcile Android values with iOS and Windows.

Is the approach sound?
Yes. The GetAltitude helper is clean and ensures altitude value and vertical accuracy are always sourced from the same reference system — the key invariant. The approach is minimal and consistent with the existing API-level gating style used elsewhere in the same file (IsAndroidVersionAtLeast(26) for VerticalAccuracy, IsAndroidVersionAtLeast(31) for Mock).


Reconciliation with PR Narrative

Author claims: Fixes #27554 — prefer MslAltitudeMeters on API 34+, fall back to ellipsoidal altitude, report Unspecified when no altitude. Change is opaque to callers. The existing Geolocation_Tests.cs device tests will exercise the new path on CI.

Agreement: Direction, motivation, and implementation all match. The VerticalAccuracy/MSL mismatch flagged in the previous review (session 63444de) has been fully addressed — GetAltitude now returns altitude and vertical accuracy together, ensuring they always come from the same reference system.

Disagreement — minor: The claim that existing Geolocation_Tests.cs tests cover the new paths was never accurate (those tests don't assert on Altitude or AltitudeReferenceSystem). A new test file was added to address this, though two of the six tests are functionally identical (see Findings).

Author update (2026-05-03): Copy-constructor fix and test added. Confirmed in comment.


Findings

💡 Suggestion — Duplicate test: ToLocation_HasAltitudeButNoMslAltitude_UsesEllipsoid

src/Essentials/test/DeviceTests/Tests/Android/Geolocation_Tests.cs:46–70

ToLocation_HasAltitudeButNoMslAltitude_UsesEllipsoid (line 46) and ToLocation_EllipsoidalAltitude_UsesEllipsoidReferenceSystem (line 24) have byte-for-byte identical setup and assertions. Neither sets MslAltitudeMeters, so on API 34+ both have HasMslAltitude == false and exercise the same fallback branch. The comment in the second test claims it "exercises the HasMslAltitude == false fallback branch" on API 34+ — but the first test exercises that exact same branch. One of the two should be removed.

💡 Suggestion — Cross-platform AltitudeReferenceSystem divergence when no altitude

src/Essentials/src/Types/LocationExtensions.android.cs:70

After this PR:

  • iOS (LocationExtensions.ios.tvos.watchos.macos.cs:45): always returns AltitudeReferenceSystem = Geoid, even when Altitude == null (the CLLocation.VerticalAccuracy < 0 path).
  • Android (new): returns AltitudeReferenceSystem = Unspecified when no altitude.

A cross-platform consumer using AltitudeReferenceSystem != Unspecified to detect altitude availability will see platform-divergent behavior. The Android behavior is semantically correct (Unspecified is the right value when Altitude is null), but the PR's stated goal is to improve Android/iOS alignment, and for the no-altitude case it now diverges from iOS. Consider a follow-up to fix the iOS case (Unspecified when VerticalAccuracy < 0), or at minimum document the divergence in the PR description.

💡 Suggestion — Cross-platform copy-constructor test lives in Android-only folder

src/Essentials/test/DeviceTests/Tests/Android/Geolocation_Tests.cs:107–122

LocationCopyConstructor_PreservesAltitudeReferenceSystem tests Location(Location point) in Location.shared.cs, which is compiled for all platforms. Placing it under Tests/Android/ means it only runs when Android device-test CI is scheduled. There is no existing Location_Tests.cs in src/Essentials/test/UnitTests/, but adding this test there (as a plain xUnit unit test with no Android dependency) would give it platform-agnostic, always-on coverage and prevent the copy-constructor regression from slipping through on a non-Android run.


Devil's Advocate

API guard correctness: The OperatingSystem.IsAndroidVersionAtLeast(34) && location.HasMslAltitude pattern is identical in style to the existing IsAndroidVersionAtLeast(26) and IsAndroidVersionAtLeast(31) guards in the same file. CA1416 is satisfied by short-circuit evaluation. No [SupportedOSPlatform] annotation is needed. ✅

Blast radius of Ellipsoid → Unspecified: No internal MAUI consumer branches on the old value. AltitudeReferenceSystem is only read in GeolocationViewModel.cs for display. External consumers could theoretically condition on Ellipsoid as a sentinel for "Android-no-altitude", but the previous state was indefensible (null altitude paired with a non-null reference system). ✅

float → double implicit cast for MslAltitudeAccuracyMeters: Consistent with how VerticalAccuracyMeters (also float) was already handled in the pre-existing code. Safe widening. ✅

Could a device return HasMslAltitude == true but MslAltitudeMeters == 0.0? The Android docs say getMslAltitudeMeters() must be set via setMslAltitudeMeters(float) before hasMslAltitude() returns true, so HasMslAltitude is reliable as a gate. ✅

Does the existing API 26 VerticalAccuracy still apply on the ellipsoidal fallback path? Yes — the HasAltitude branch retains the exact same IsAndroidVersionAtLeast(26) && HasVerticalAccuracy guard from the pre-PR code, so no regression there. ✅


Verdict: LGTM

Confidence: high

Summary: The implementation is correct, well-structured, and addresses a real cross-platform inconsistency. The previous review's primary concern (VerticalAccuracy/MSL mismatch) has been fully resolved in the updated code. All three findings are suggestions — none are blockers. The duplicate test is cosmetic; the iOS divergence and test-placement issues are reasonable follow-up work. The API gating is sound and consistent with the existing patterns in the file. Ready for human approval.


🔧 Fix — Analysis & Comparison

Fix Candidates

# Source Approach Test Result Files Changed Notes
1 try-fix Inline local variables — no helper method, if/else-if/else branching in ToLocation() directly ⚠️ BLOCKED (no device) 3 files Avoids tuple allocation; clean but less structured
2 try-fix PR's GetAltitude helper (private) + moved copy constructor test to UnitTests + Timestamp bug fix ⚠️ BLOCKED (no device) 4 files (new UnitTests/LocationTests.cs) Best test architecture; also fixes pre-existing Timestamp copy bug
3 try-fix Switch expression / declarative tuple selection in ToLocation() ⚠️ BLOCKED (no device; build only) 3 files Declarative style; build-only "Pass" — not a real device test pass
4 try-fix Named AltitudeInfo readonly struct with Resolve() factory ⚠️ BLOCKED (no device) 4 files (new UnitTests/Location_Tests.cs) Strongest encapsulation; unit tests pass
PR PR #35097 GetAltitude() tuple helper + copy constructor fix ❌ FAILED (Gate) 3 files Original PR; gate failed (see gate/content.md)

Cross-Pollination

Model Round New Ideas? Details
claude-sonnet-4.6 2 No "NO NEW IDEAS — the PR's ValueTuple helper is the right call. out-parameter style is the only structural variant not tried but is strictly worse."

Exhausted: Yes
Selected Fix: pr-plus-reviewer (PR fix with code review suggestions applied) — reason: All try-fix candidates were blocked due to no Android device, so empirical validation was unavailable. The PR's core implementation is correct; Attempt 2 (claude-sonnet-4.6) shows the best incremental improvements (private modifier, dedup test, move copy constructor test to UnitTests), which together constitute the pr-plus-reviewer candidate.


📋 Report — Final Recommendation

⚠️ Final Recommendation: REQUEST CHANGES

Phase Status

Phase Status Notes
Pre-Flight ✅ COMPLETE Issue #27554, 3 files changed, prior review found VerticalAccuracy/MSL mismatch (now fixed)
Code Review LGTM (high) 0 errors, 0 warnings, 3 suggestions
Gate ❌ FAILED android — tests did NOT behave as expected (see gate/content.md)
Try-Fix ✅ COMPLETE (4 attempts) 0 passing, 4 blocked (no Android device/emulator), 1 build-only "Pass" (Attempt 3 — not a real device run)
Expert Review ✅ COMPLETE pr-plus-reviewer candidate produced with reviewer feedback applied
Report ✅ COMPLETE

Candidate Comparison

Candidate Approach Test Result Score
pr GetAltitude helper, copy constructor fix ❌ Gate FAILED 2nd
pr-plus-reviewer PR + private modifier + remove duplicate test + move copy constructor test to UnitTests ⚠️ Not directly tested (gate failed for base PR) 1st (best)
try-fix-1 Inline local vars (no helper) ⚠️ BLOCKED 3rd
try-fix-2 GetAltitude helper + test architecture + Timestamp fix ⚠️ BLOCKED 3rd
try-fix-3 Switch expression (declarative tuple) ⚠️ Build-only "Pass" (no device) 3rd
try-fix-4 AltitudeInfo readonly struct ⚠️ BLOCKED 3rd

Code Review Impact on Try-Fix

The code review found 3 💡 suggestions (no ❌ errors). These directly shaped the try-fix exploration:

  • All 4 models independently recognized the duplicate test issue and addressed it in their approaches
  • Attempt 2 (claude-sonnet-4.6) specifically addressed the copy constructor test placement by creating LocationTests.cs in UnitTests
  • Attempt 4 applied all 3 reviewer suggestions plus a named struct approach
  • No model found a fundamentally different implementation — consensus is that the PR's core approach (paired altitude+accuracy selection with API gating) is correct

Summary

PR #35097 adds MSL altitude support on Android API 34+ with a clean GetAltitude() helper. The core implementation is sound and correct. The gate failed because the tests include guards (if (!OperatingSystem.IsAndroidVersionAtLeast(34)) return;) that silently skip the key assertions on pre-34 test devices, so the gate cannot verify the tests fail without the fix. Three code-quality suggestions from the expert review should be applied: (1) add private modifier to GetAltitude(), (2) remove the byte-for-byte duplicate test ToLocation_HasAltitudeButNoMslAltitude_UsesEllipsoid, (3) move LocationCopyConstructor_PreservesAltitudeReferenceSystem to a platform-agnostic unit test project.

Root Cause

Android API 34 introduced getMslAltitudeMeters() which returns geoid-corrected altitude, matching iOS/Windows behavior. The prior implementation always returned AltitudeReferenceSystem.Ellipsoid with WGS84 ellipsoidal altitude, requiring consumers to apply a manual geoid correction (~0-50m) to reconcile Android readings with other platforms. Additionally, the Location copy constructor was silently dropping AltitudeReferenceSystem during copies.

Fix Quality

The PR's fix is architecturally sound: it correctly pairs altitude and vertical accuracy from the same reference system (a design invariant), uses the established OperatingSystem.IsAndroidVersionAtLeast(N) API gating pattern, and is backward-compatible (pre-34 devices unchanged). The behavioral change from (null, Ellipsoid)(null, Unspecified) when no altitude is available is semantically correct but is a subtle breaking change for consumers that used Ellipsoid as an Android sentinel. The pr-plus-reviewer candidate (PR + 3 code quality fixes) is the best result: it addresses all reviewer-identified issues with no regressions.


Copy link
Copy Markdown
Collaborator

@MauiBot MauiBot left a comment

Choose a reason for hiding this comment

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

Expert Review — 6 findings

See inline comments for details.

else
Assert.Null(location.VerticalAccuracy);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[major] Regression Prevention — Duplicate testToLocation_EllipsoidalAltitude_UsesEllipsoidReferenceSystem (this test) and ToLocation_HasAltitudeButNoMslAltitude_UsesEllipsoid (lines 64–87) are functionally identical: both create new AndroidLocation("test") { Altitude = 123.45 }, optionally set VerticalAccuracyMeters, and assert AltitudeReferenceSystem.Ellipsoid. On API 34+, neither test sets MslAltitudeMeters, so HasMslAltitude is false in both — the same fallback branch is hit. The long comment in the third test asserting it uniquely exercises the API-34 fallback branch is incorrect. Remove ToLocation_HasAltitudeButNoMslAltitude_UsesEllipsoid; a test named 'HasAltitudeButNoMslAltitude' that is semantically indistinguishable from 'EllipsoidalAltitude' will silently pass even if the fallback branch is accidentally removed.

var location = androidLocation.ToLocation();

Assert.Equal(123.45, location.Altitude);
Assert.Equal(AltitudeReferenceSystem.Ellipsoid, location.AltitudeReferenceSystem);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[major] Regression Prevention — Duplicate test (see finding at line 44)ToLocation_HasAltitudeButNoMslAltitude_UsesEllipsoid is byte-for-byte equivalent to the preceding ToLocation_EllipsoidalAltitude_UsesEllipsoidReferenceSystem. Both tests create an AndroidLocation with only Altitude = 123.45, leave MslAltitudeMeters unset, and assert Ellipsoid. The comment 'On every API level … exercises the HasMslAltitude == false fallback branch' is true of both tests. Remove this test.

AltitudeReferenceSystem = AltitudeReferenceSystem.Geoid,
VerticalAccuracy = 2.5
};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[moderate] Regression Prevention — Wrong test project for shared-code testLocationCopyConstructor_PreservesAltitudeReferenceSystem exercises only the shared Location(Location point) constructor in Location.shared.cs; no Android APIs are used. Placing it in Tests/Android/ means it only runs on Android device-test CI legs and will not catch a copy-constructor regression on an iOS or Windows run. Move it to a new src/Essentials/test/UnitTests/Location_Tests.cs so it runs on every platform without requiring a device.

return (location.Altitude, AltitudeReferenceSystem.Ellipsoid, ellipsoidAccuracy);
}

return (null, AltitudeReferenceSystem.Unspecified, null);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[moderate] Cross-Platform Consistency — AltitudeReferenceSystem diverges from iOS when no altitude is available — iOS (LocationExtensions.ios.tvos.watchos.macos.cs line 45) always sets AltitudeReferenceSystem = AltitudeReferenceSystem.Geoid even when Altitude is null (when VerticalAccuracy < 0). After this PR, Android returns Unspecified when HasAltitude is false. This is semantically more correct — there is no meaningful reference system when there is no altitude value — but it creates a cross-platform inconsistency: a consumer checking location.AltitudeReferenceSystem != AltitudeReferenceSystem.Unspecified as a proxy for altitude availability will get different results on iOS vs Android. The PR description frames the goal as cross-platform consistency; the no-altitude case now diverges. Recommend either: (a) open a follow-up to fix iOS to return Unspecified when altitude is unavailable, or (b) add an XML doc comment to AltitudeReferenceSystem.Unspecified documenting when each platform emits it.

// values are consistent across platforms without manual geoid correction.
// Altitude and its accuracy are selected together so they always come from
// the same reference system (MSL/geoid vs WGS84/ellipsoid).
static (double? Altitude, AltitudeReferenceSystem Reference, double? VerticalAccuracy) GetAltitude(AndroidLocation location)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[minor] Logic and Correctness — Missing explicit private modifier on GetAltitude — The method has no access modifier (C# defaults to private inside a class). Explicitly writing private static would match the style of other private helpers and makes intent immediately clear without readers needing to recall the default.

@@ -103,6 +103,7 @@ public Location(Location point)
Longitude = point.Longitude;
Timestamp = DateTime.UtcNow;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[minor] Logic and Correctness — Copy constructor resets Timestamp instead of copying itTimestamp = DateTime.UtcNow discards the source location's timestamp. This is a pre-existing behaviour but the PR modifies the copy constructor, making this a good opportunity to either fix the copy (Timestamp = point.Timestamp) or add an explicit XML doc comment explaining why the time is intentionally reset. As written, a caller wishing to clone a Location and preserve its recorded timestamp cannot use this constructor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community ✨ Community Contribution s/agent-changes-requested AI agent recommends changes - found a better alternative or issues s/agent-fix-win AI found a better alternative fix than the PR s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for MslAltitudeMeters in Essentials Geolocation on Android

3 participants