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

[TIMOB-9434] [TIMOB-8672] Fix handling of trueHeading in compass events #2373

Merged
merged 7 commits into from Jun 16, 2012

Conversation

arthurevans
Copy link
Contributor

Note: This is not a doc PR, it's a code PR so I don't have to add some ugly workarounds in the doc. Should probably be reviewed by Opie as the resident Android Geo expert.

Two related fixes for compass on Android. I apologize for including two bugs, but they're related and the same test code serves them both. Test code is in TIMOB-9434.

https://jira.appcelerator.org/browse/TIMOB-9434
https://jira.appcelerator.org/browse/TIMOB-8672

if (provider != null) {
Location location = tiLocation.locationManager.getLastKnownLocation(provider);
if (location != null) {
if (geomagneticField == null || location.getTime() > geomagneticFieldLocation.getTime()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

No null checking for geomagneticFieldLocation

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a concern here I think regarding the integrity of the location data. At which point is the location data provided so old that it is worse to use it in order to report data versus simply reporting that we are unable to report data? I would argue that it is better to say we are unable to report data rather than use a location fix that may be days old.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I think somewhere between 1 hour and 8 hours would be a good threshold for freshness. In that time you could hop a plane or drive far enough to significantly alter the declination. And if the app can't get a fix once an hour, either the fix is unavailable, or they aren't trying very hard.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why we shouldn't have a much tighter threshold for freshness - say less than 10 minutes. If the location is older than that, we could register one for the purpose of getting a fresh fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, missed this comment on my earlier update. I didn't know how expensive it was to create a new GeomagneticField, so I didn't want to overdo it. Once every 10 minutes is good by me. Around here, the declination correction only changes about a 1/10th of a degree for each 60 miles. It changes a lot faster inside the Arctic circle, but IMHO, anyone who uses an Android app for navigating inside the Arctic circle is asking for trouble.

When you say, "We could register one..." Do you mean adding a location request in TiCompass? I think you might need to hold my hand for that one ;-).

@arthurevans
Copy link
Contributor Author

Updated the threshold to 10 minutes. If nothing else, it should make testing less of a pain. Will try to catch you tomorrow to talk about whether I need to kick off a location request.

@rusticphilosopher
Copy link
Contributor

Spoke with Arthur and he is going to update the logging and handling of stale locations.

@rusticphilosopher
Copy link
Contributor

Code reviewed and functional test passed. Accepted

rusticphilosopher pushed a commit that referenced this pull request Jun 16, 2012
[TIMOB-9434] [TIMOB-8672] Fix handling of trueHeading in compass events
@rusticphilosopher rusticphilosopher merged commit 253bf6e into tidev:master Jun 16, 2012
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.

None yet

2 participants