ionic-team / capacitor-plugins

Official plugins for Capacitor ⚡️
510 stars 577 forks source link

Geolocation watchPosition on Android seems to ignore timeout position option #1465

Open basvdijk opened 1 year ago

basvdijk commented 1 year ago

Bug Report

Plugin(s)

Geolocation

Capacitor Version

💊   Capacitor Doctor  💊

Latest Dependencies:

  @capacitor/cli: 4.7.0
  @capacitor/core: 4.7.0
  @capacitor/android: 4.7.0
  @capacitor/ios: 4.7.0

Installed Dependencies:

  @capacitor/cli: 4.6.3
  @capacitor/core: 4.6.3
  @capacitor/android: 4.6.3
  @capacitor/ios: 4.6.3

[success] iOS looking great! 👌
[success] Android looking great! 👌

Platform(s)

Android

Current Behavior

 GeoLocationWatchId = await Geolocation.watchPosition(
    { enableHighAccuracy: true, timeout: 2000, maximumAge: 2500 },

The code above works fine on iOS. Every 2000ms I get a position. On Android however there is lots of time between updates. It looks like it takes the 10 second default instead of the provided 2000ms

Expected Behavior

To receive a new location every 2000ms

Code Reproduction

 GeoLocationWatchId = await Geolocation.watchPosition(
    { enableHighAccuracy: true, timeout: 2000, maximumAge: 2500 },

Repo with screenshot at: https://github.com/basvdijk/capacitor-geolocation-android-timeout

Other Technical Details

I've tested this while driving with 1 iOS device, and 2 Android devices running:

In the code I saw this line: https://github.com/ionic-team/capacitor-plugins/blob/main/geolocation/android/src/main/java/com/capacitorjs/plugins/geolocation/Geolocation.java#L92

                LocationRequest locationRequest = LocationRequest
                    .create()
                    .setMaxWaitTime(timeout)
                    .setInterval(10000)
                    .setFastestInterval(5000)
                    .setPriority(priority);

Could it be that the timeout and interval value is hardcoded and 2000 is not allowed? I need sometimes an interval of 1000ms for GPS navigation

Side note: According to https://developers.google.com/android/reference/com/google/android/gms/location/LocationRequest#setFastestInterval(long) the setFastestInterval method is deprecated.

Additional Context

n/a

Ionitron commented 1 year ago

This issue may need more information before it can be addressed. In particular, it will need a reliable Code Reproduction that demonstrates the issue.

Please see the Contributing Guide for how to create a Code Reproduction.

Thanks! Ionitron 💙

basvdijk commented 1 year ago

added repo with minimal code reproduction at https://github.com/basvdijk/capacitor-geolocation-android-timeout

basvdijk commented 1 year ago

@jcesarmobile I guess the "needs reproduction" label could be removed?

Toilal commented 1 year ago

Please see this pull request to fix the issue : https://github.com/ionic-team/capacitor-plugins/pull/1619

basvdijk commented 1 year ago

Any update on this? Tested with the latest version and the bug is still there.

Toilal commented 1 year ago

I'm not sure maintainers actually reads issues and pull requests sadly :(

bozhidarc commented 1 year ago

@jcesarmobile, is there something we can do for merging the @Toilal PR? It's hanging without replay for 3 months, already ...

basvdijk commented 1 year ago

@bozhidarc I ended up with adding postinstall npm script which basically patches this branch's fix onto the geolocation package

bozhidarc commented 1 year ago

Thanks, this is always a solution, but I prefer if can not to do it :)

dallastjames commented 1 year ago

Hey all, just copying my comment from the PR to here so it's visible.

I think there's a misunderstanding of the timeout option and how it's expected to be used. The timeout option is described as the maximum wait time in milliseconds for location updates and is based on the web geolocation API. This timeout is a setting for how long you're willing to wait for a location update before giving up and assuming no response. It's not intended to control the frequency of updates received. That is strictly an Android only behavior that you can control, neither the web nor iOS allow you to specify the frequency of updates.

We could potentially expose this as a new option added to the watch options interface that's explicitly for Android, but this would be something new added to the API instead of changing the existing behavior of the timeout option.

basvdijk commented 1 year ago

@dallastjames I don't see the relation with the "web geolocation API" in this PR.

If we look at this line in the current Android code, it says:

new LocationRequest.Builder(10000)

According to the Android location documentation

Creates a new Builder with the given interval. See setIntervalMillis(long) for more information on the interval.

If we visit this setIntervalMillis section:

Sets the request interval

So in the current code there is a hardcoded interval of 10 seconds between requesting the OS for a location update.

The PR at https://github.com/ionic-team/capacitor-plugins/pull/1619/files makes this dynamic.

If you don't think this PR is a good solution I am happy to hear how to get faster updates when building a navigation app. In iOS there is no problem and the updates are fast enough.

In my opinion this is a bug and not a feature request since the interval is hardcoded.

Toilal commented 1 year ago

@dallastjames I don't see the relation with the "web geolocation API" in this PR.

The relation is that Capacitor Geolocation plugin implements the "web geolocation API", so the timeout parameter must match spec from this API, but the PR I have proposed use the timeout parameter for another purpose (refresh rate).

basvdijk commented 1 year ago

@Toilal thanks for explaining. I just hope it gets sorted out that we as developers can use almost realtime location updates. Since it looks like with the current code it is not possible. At least I have not seen a solution of anyone being able to do so.

dallastjames commented 1 year ago

@basvdijk apologies if I was unclear about that, but yes, as Toilal better clarified, the plugin is based on the web API and the timeout parameter already has an intended function which that PR would be causing a breaking change by changing in the way that it did.

I'll definitely keep an eye out for the PR that Toilal said they'd open implementing a new parameter for this control on Android though!

basvdijk commented 1 year ago

@dallastjames Ok, but then do you have a suggestion how I can get the position every second on Android via this plugin?