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

Location constructor silently cuts off latitude and longitude values #19389

Closed
janusw opened this issue Dec 13, 2023 · 4 comments · Fixed by #19459
Closed

Location constructor silently cuts off latitude and longitude values #19389

janusw opened this issue Dec 13, 2023 · 4 comments · Fixed by #19459
Labels
area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info fixed-in-8.0.10 fixed-in-8.0.14 fixed-in-8.0.40 fixed-in-9.0.0-preview.2.10247 fixed-in-9.0.0-preview.2.10293 migration-compatibility Xamarin.Forms to .NET MAUI Migration, Upgrade Assistant, Try-Convert platform/macOS 🍏 macOS / Mac Catalyst t/bug Something isn't working
Milestone

Comments

@janusw
Copy link
Contributor

janusw commented Dec 13, 2023

Description

When constructing a Microsoft.Maui.Devices.Sensors.Location from a latitude/longitude pair, the constructor silently seems to cut off the values, if they are outside of the expected range:

  • latitude values that are larger than 90 degrees are cut down to 90, values smaller than -90 are cut to -90
  • longitude values that are larger than 180 degrees are cut down to 180, values smaller than -180 are cut to -180

This did not occur with Xamarin.Essentials, and IMHO it is not a good way to deal with this situation. Better options would be:

  1. Throw an ArgumentOutOfRangeException or ...
  2. ... use a modulus / remainder operation to project the values into the allowed range. In particular this would make sense for the longitude (but not so much for the latitude, I guess).

In any case, the behavior and the allowed values should be documented.

Steps to Reproduce

Execute the following test case:

using System.Diagnostics;
using Microsoft.Maui.Devices.Sensors;

var loc = new Location(0, 200);
Debug.WriteLine($"{loc.Latitude}, {loc.Longitude}");

Expected outcome: 0, -160 (or throw an ArgumentOutOfRangeException)

Actual outcome: 0, 180

Link to public reproduction project repository

No response

Version with bug

8.0.3

Is this a regression from previous behavior?

Yes, this used to work in Xamarin.Forms

Last version that worked well

Unknown/Other

Affected platforms

macOS

Affected platform versions

No response

Did you find any workaround?

No response

Relevant log output

No response

@janusw janusw added the t/bug Something isn't working label Dec 13, 2023
@janusw
Copy link
Contributor Author

janusw commented Dec 13, 2023

This is the relevant code:

public Location(double latitude, double longitude)
{
Latitude = Math.Min(Math.Max(latitude, -90.0), 90.0);
Longitude = Math.Min(Math.Max(longitude, -180.0), 180.0);
Timestamp = DateTimeOffset.UtcNow;
}

In Xamarin it looked like this (no limiting/cutting of values there):

https://github.com/xamarin/Essentials/blob/6ec2124ec4354a0631de2c233a3d29123015f6df/Xamarin.Essentials/Types/Location.shared.cs#L28-L33

@janusw
Copy link
Contributor Author

janusw commented Dec 13, 2023

Additional inconsistency: The other Location constructors behave differently and do not cut the input values. All of them should behave in the same way (by calling each other if non-trivial checks or modifications are involved).

public Location(double latitude, double longitude, DateTimeOffset timestamp)
{
Latitude = latitude;
Longitude = longitude;
Timestamp = timestamp;
}

public Location(double latitude, double longitude, double altitude)
{
Latitude = latitude;
Longitude = longitude;
Altitude = altitude;
Timestamp = DateTimeOffset.UtcNow;
}

public Location(Location point)
{
if (point == null)
throw new ArgumentNullException(nameof(point));
Latitude = point.Latitude;
Longitude = point.Longitude;

@janusw
Copy link
Contributor Author

janusw commented Dec 13, 2023

This is the relevant code:

... and it was modified in this (huge) commit: 4f3075b (by @rmarinho et al.)

@jsuarezruiz jsuarezruiz added the area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info label Dec 13, 2023
@PureWeen PureWeen added the migration-compatibility Xamarin.Forms to .NET MAUI Migration, Upgrade Assistant, Try-Convert label Dec 13, 2023
@PureWeen PureWeen added this to the Backlog milestone Dec 13, 2023
@ghost
Copy link

ghost commented Dec 13, 2023

We've added this issue to our backlog, and we will work to address it as time and resources allow. If you have any additional information or questions about this issue, please leave a comment. For additional info about issue management, please read our Triage Process.

janusw added a commit to janusw/maui that referenced this issue Dec 16, 2023
…ssue dotnet#19389)

* throw an exception if latitude is out of range
* longitude is a cyclic variable, so we project it into a defined range
* make all constructors behave the same way
  (and document the behaviour)
@samhouts samhouts added the platform/macOS 🍏 macOS / Mac Catalyst label Feb 1, 2024
rmarinho pushed a commit that referenced this issue Feb 12, 2024
…ssue #19389) (#19459)

* throw an exception if latitude is out of range
* longitude is a cyclic variable, so we project it into a defined range
* make all constructors behave the same way
  (and document the behaviour)
@github-actions github-actions bot locked and limited conversation to collaborators Mar 15, 2024
@samhouts samhouts modified the milestones: Backlog, .NET 8 SR3 Apr 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info fixed-in-8.0.10 fixed-in-8.0.14 fixed-in-8.0.40 fixed-in-9.0.0-preview.2.10247 fixed-in-9.0.0-preview.2.10293 migration-compatibility Xamarin.Forms to .NET MAUI Migration, Upgrade Assistant, Try-Convert platform/macOS 🍏 macOS / Mac Catalyst t/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants