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

Fixes #875: Add accuracy to LocationData (when available) #876

Merged
merged 5 commits into from
Aug 11, 2023

Conversation

tyler-jewell
Copy link
Contributor

Fixes #875

@docs-page
Copy link

docs-page bot commented Aug 5, 2023

To view this pull requests documentation preview, visit the following URL:

docs.page/lyokone/flutterlocation~876

Documentation is deployed and generated using docs.page.

Copy link
Collaborator

@bartekpacia bartekpacia left a comment

Choose a reason for hiding this comment

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

Thank you, looks good 🎉 just one small comment

@@ -172,7 +172,7 @@ class LocationData {
final double accuracy; // Estimated horizontal accuracy of this location, radial, in meters
final double altitude; // In meters above the WGS 84 reference ellipsoid
final double speed; // In meters/second
final double speedAccuracy; // In meters/second, always 0 on iOS
final double speedAccuracy; // In meters/second, always 0 on iOS, web
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you missed something here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to docs here: https://developer.mozilla.org/en-US/docs/Web/API/GeolocationCoordinates, speedAccuracy is not available on web so it will always be zero.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think if we change to this though, we should just change the documentation to state that all coordinate parameters are 0 if missing (this could be a dangerous way to handle this though).

Edge cases that this can cause some unintended consequences:

  • Speed is actually 0 (user is not moving). Developer would have a hard time knowing if it was missing or truly zero.
  • Same goes for heading?

Maybe we should make these Null if missing to avoid this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I think using null for missing values is a good idea. Are you up to contributing this as well? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, sign me up. We will keep this PR open and I will make these changes?

Copy link
Contributor Author

@tyler-jewell tyler-jewell Aug 8, 2023

Choose a reason for hiding this comment

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

@bartekpacia maybe a dumb question, but what is the best practice after forking the repo to test the changes with the example folder in the location package? I would like to run the example code and have it use the changes to the location_web package, but it is using the pub.dev location_web package.

For now, I am changing the pubspec to use local, just wondering if there is a better way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Melos solves this problem. Run melos bootstrap in this project's root directory and it'll locally override ("link together") all the packages.

Copy link
Collaborator

@bartekpacia bartekpacia left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution, this looks good.

Is it ready to be merged?

packages/location/README.md Outdated Show resolved Hide resolved
@tyler-jewell
Copy link
Contributor Author

Thank you for the contribution, this looks good.

Is it ready to be merged?

Yes!

@bartekpacia bartekpacia merged commit 3bcb77a into Lyokone:master Aug 11, 2023
3 checks passed
@bartekpacia
Copy link
Collaborator

bartekpacia commented Aug 11, 2023

Thanks, released in v4.2.0 😎😎😎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add accuracy to LocationData (when available)
2 participants