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

Geolocation - Listening for location changes in the foreground #3678

Closed
vividos opened this issue Dec 6, 2021 · 11 comments · Fixed by #9572
Closed

Geolocation - Listening for location changes in the foreground #3678

vividos opened this issue Dec 6, 2021 · 11 comments · Fixed by #9572
Labels
area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info fixed-in-8.0.0-preview.1.7762 Look for this fix in 8.0.0-preview.1.7762! proposal/open t/enhancement ☀️ New feature or request
Milestone

Comments

@vividos
Copy link
Contributor

vividos commented Dec 6, 2021

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

  • The user likes to locate him/herself on a map currently displayed.
  • The user likes to display the current position, along with other details such as accuracy, e.g. to send it to someone else or to further use the coordinates when they are exact enough.

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

@Eilon Eilon added the area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info label Dec 8, 2021
@vividos
Copy link
Contributor Author

vividos commented Feb 15, 2022

@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
Copy link
Member

Eilon commented Feb 15, 2022

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

@Redth Redth added this to the Future milestone Feb 16, 2022
@Redth Redth added the t/enhancement ☀️ New feature or request label Feb 16, 2022
@vividos
Copy link
Contributor Author

vividos commented Feb 21, 2022

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

@orwo1
Copy link

orwo1 commented Aug 21, 2022

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
Copy link
Contributor Author

vividos commented Aug 21, 2022

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
Copy link

orwo1 commented Aug 21, 2022

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

@jfversluis
Copy link
Member

A couple of things that I see here:

  • ListeningRequest should this be something like GeolocationListeningRequest so that we know what it's meant for if we only look at this object?
    • We also already have GeolocationRequest, can we reuse that?
  • ListeningRequest. MinimumTime without an explanation I don't know what this means or does.
  • Looking at the GeolocaterPlugin it seems to have some more options in terms of minimum distance, minimum time (I assume that is the one above), include heading, etc. would that be useful here as well?
  • That same plugin also has an event handler for when an error occurs, that seems it might be useful?

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
Copy link
Contributor Author

vividos commented Jan 16, 2023

Thanks for the analysis, all good points!

  • GeolocationListeningRequest is off course the better name, will change that
  • GeolocationRequest has similar fields, and my PR first reused the class, but the Timeout property always felt odd, and MinimumTime better conveys meaning (update location after minimum of this time span)
  • I omitted the extra properties for now as most if not all of them were iOS specific. Do you think they are worthwhile to add? https://jamesmontemagno.github.io/GeolocatorPlugin/BackgroundUpdates.html
  • error event handler: good point, that should be added; I'm not sure why I omitted it
  • namespace: yes, that should be changed in the spec, and is probably already done in the PR

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
Copy link
Member

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
Copy link
Contributor Author

vividos commented Jan 17, 2023

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
Copy link
Contributor Author

vividos commented Jan 22, 2023

@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.

@samhouts samhouts added the fixed-in-8.0.0-preview.1.7762 Look for this fix in 8.0.0-preview.1.7762! label Feb 22, 2023
@samhouts samhouts modified the milestones: Backlog, 8.0-preview1 Feb 22, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2023
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.0-preview.1.7762 Look for this fix in 8.0.0-preview.1.7762! proposal/open t/enhancement ☀️ New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants