dotnet / maui

.NET MAUI is the .NET Multi-platform App UI, a framework for building native device applications spanning mobile, tablet, and desktop.
https://dot.net/maui
MIT License
22.27k stars 1.76k forks source link

Geolocation - Listening for location changes in the foreground #3678

Closed vividos closed 1 year ago

vividos commented 2 years ago

Description

Xamarin Essentials as well as MAUI Essentials have the Geolocation API that allows to get the current (or last known) location. James Montemagno's Geolocation plugin also has additional API calls to start/stop listening for location updates. In my apps (both hobby projects and professional work) I use these APIs for several use cases, which might benefit having the API directly in MAUI Essentials. Note that the feature request is just asking for foreground location changes, as I'm aware that background location updates are much more complicated to implement.

Note that the Geolocation foreground listener feature was previously proposed in Xamarin.Essentials, but didn't make the cut in 1.7.0. See:

Public API Changes

public interface IGeolocation
{
    public bool IsListeningForeground { get; }
    public Task<bool> StartListeningForegroundAsync(GeolocationListeningRequest request);
    public void StopListeningForeground();
    public event EventHandler<LocationEventArgs> LocationChanged;
    public event EventHandler<GeolocationErrorEventArgs> LocationError;
}
public static class Geolocation
{
    public static bool IsListeningForeground { get; }
    public static Task<bool> StartListeningForegroundAsync(GeolocationListeningRequest request);
    public static void StopListeningForeground();
    public static event EventHandler<LocationEventArgs> LocationChanged;
    public static event EventHandler<GeolocationErrorEventArgs> LocationError;
}
public class LocationEventArgs : EventArgs
{
    public Location Location { get; }
    public LocationEventArgs(Location location);
}
public enum GeolocationError
{
    PositionUnavailable,
    Unauthorized,
}
public class GeolocationErrorEventArgs : EventArgs
{
    public GeolocationError Error { get; }
    public GeolocationErrorEventArgs(GeolocationError error);
}
public class GeolocationListeningRequest
{
    public GeolocationListeningRequest();
    public GeolocationListeningRequest(GeolocationAccuracy accuracy);
    public GeolocationListeningRequest(GeolocationAccuracy accuracy, TimeSpan minimumTime);
    public TimeSpan MinimumTime { get; set; } = TimeSpan.FromSeconds(1);
    public GeolocationAccuracy DesiredAccuracy { get; set; } = GeolocationAccuracy.Default;
}

(all classes live in the namespace Microsoft.Maui.Devices.Sensors)

Intended Use-Case

Note that all of these scenarios happen in the foreground. Background location requests are out of scope for this feature request.

vividos commented 2 years ago

@Eilon I'd like to get a discussion about this new feature to get started. How should I proceed, who should I ping for this? I can also prepare a PR if you like.

Eilon commented 2 years ago

Tagging @Redth and @mattleibow who know more about the Essentials library.

vividos commented 2 years ago

I guess I'm waiting for #4634 to be merged and propose a new API?

orwo1 commented 2 years ago

Is this something that was resolved? I don't see MAUI Essentials currently has those, so I guess not.... I am using Xam.Plugin.Geolocator: https://github.com/jamesmontemagno/GeolocatorPlugin in my XF project, but it's probably not going to be ported to MAUI. Is there an alternative?

vividos commented 2 years ago

I'm also using the GeolocatorPlugin at the moment, and that's why I wrote this issue, but no one from the Essentials or MAUI team ever responded.

orwo1 commented 2 years ago

@jamesmontemagno Any idea if this is something that will be supported in MAUI?

jfversluis commented 1 year ago

A couple of things that I see here:

You mention that it goes to the Essentials namespace which doesn't exist anymore so probably the right place now will be Microsoft.Maui.Devices.Sensors which you probably already have in the PR you set up. Also, I didn't check in your PR but just to be sure: we dropped one variation of the Location object so that we're aligned also with the Map component, but I'm assuming that you're using the right one for that.

Just thinking out loud for a bit here. If we think ahead a bit about maybe someday a background location listening functionality will be added. Can we still get away with all of this? If we can I'd love to not duplicate any of this but just be able to use it for that as well as much as we can and is useful.

Thanks for putting this together @vividos!

vividos commented 1 year ago

Thanks for the analysis, all good points!

About the background location updates: I think this is harder to do, and James' plugin makes some efforts, but my use case was only foreground listening. I think Shiny does that, but there are some more steps to set this up: https://shinylib.net/mobile/gps/index.html?tabs=android When you think Essentials should eventually also get background location updates, we should probably think about renaming now. I wanted to make clear with method names that it's only about foreground listening.

mattleibow commented 1 year ago

Very nice PR! Pretty much all looks good except the naming that Gerald pointed out.

But also a point in the future and background. I think it is fine to have StartForeground and StartBackground as that is often separate things.

They both can probably use the same event as well.

My question is can they run simultaneously? I am thinking if we have both, the IsListening might be ambiguous... Maybe we have a StartForeground and then have a MoveToBackground concept (that could just stop foreground and start background). But if we want to support simultaneously background and foreground then the boolean may need renaming.

vividos commented 1 year ago

Also good point, Matt! I think the property should for now be IsListeningForeground, to make the intent clearer. I'm still not sure if Essentials should implement background listening at all and better let Shiny implement that.

vividos commented 1 year ago

@jfversluis I updated the API specification in the initial post with all the points that you and Matt brought up. I also found out that StopListeningForegroundAsync() can a actually be synchronous, as no implementation calls async APIs. I will update the PR with the changes to the API now.