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

feat(geolocation): add interval option in watch position #1756

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Toilal
Copy link

@Toilal Toilal commented Aug 24, 2023

This follow up #1619 and #1465

@@ -52,7 +52,7 @@ export interface GeolocationPlugin {
* @since 1.0.0
*/
watchPosition(
options: PositionOptions,
options: WatchPositionOptions,
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure I need to add a new interface here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not really, you can just add the new options to the existing interface.

.setMaxUpdateDelayMillis(timeout)
.setMinUpdateIntervalMillis(5000)
Copy link
Author

Choose a reason for hiding this comment

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

Obviously it has to be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

This probably should be made another option along with interval, and have it default to 5000.

@dallastjames dallastjames added the pr review requested Adds PR to internal issue tracker for team review label Sep 11, 2023
.setMaxUpdateDelayMillis(timeout)
.setMinUpdateIntervalMillis(5000)
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably should be made another option along with interval, and have it default to 5000.

@@ -52,7 +52,7 @@ export interface GeolocationPlugin {
* @since 1.0.0
*/
watchPosition(
options: PositionOptions,
options: WatchPositionOptions,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really, you can just add the new options to the existing interface.

@Toilal Toilal force-pushed the geolocation-interval branch 2 times, most recently from 1edf337 to 639daf3 Compare January 11, 2024 07:51
@Toilal
Copy link
Author

Toilal commented Jan 11, 2024

I have just updated this pull request.

@@ -68,7 +68,7 @@ public void sendLocation(boolean enableHighAccuracy, final LocationResultCallbac
}

@SuppressWarnings("MissingPermission")
public void requestLocationUpdates(boolean enableHighAccuracy, int timeout, final LocationResultCallback resultCallback) {
public void requestLocationUpdates(boolean enableHighAccuracy, long timeout, long interval, long minInterval, final LocationResultCallback resultCallback) {
Copy link
Author

Choose a reason for hiding this comment

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

Using long type seems to be better as LocationRequest.Builder accepts long arguments.

@Toilal Toilal force-pushed the geolocation-interval branch 2 times, most recently from 3da7c72 to 99c8958 Compare March 19, 2024 10:44
@Toilal Toilal requested a review from theproducer March 19, 2024 10:44
@Toilal
Copy link
Author

Toilal commented May 22, 2024

Can i do anything to get this merged ?

@GeryStoycheva
Copy link

Hello, will this pull request be merged soon?

@Toilal
Copy link
Author

Toilal commented Sep 23, 2024

@theproducer ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr review requested Adds PR to internal issue tracker for team review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants