This repository has been archived by the owner on Feb 6, 2024. It is now read-only.
Feature/kak/add geofencing#44 #73
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maurizi
approved these changes
Jun 5, 2018
There was a problem hiding this comment.
This is looking great, and working well! I tested both the production mode by going to a nearby destination as well as the debug mode in the emulator and they both worked as expected 🎉
I left a few small notes, but they're fairly small - fell free to merge this w/o another round of review.
|
|
||
| // Sanity check the data before starting the worker | ||
| if (latitudes.length == 0 || latitudes.length != longitudes.length || | ||
| latitudes.length != labels.length) { |
There was a problem hiding this comment.
Should we check names.length as well?
| } else if (haveExistingGeofence) { | ||
| Log.e(LOG_LABEL, "Removing geofence from event list"); | ||
| RemoveGeofenceWorker.removeOneGeofence(String.valueOf(eventInfo.getAttraction().getId())); | ||
| } |
There was a problem hiding this comment.
This same code pattern pattern shows up in several activities. Consider pulling it out into a helper method that takes AttractionInfo and haveExistingGeofence parameters.
| // https://developer.android.com/training/location/geofencing | ||
| // TODO: geofences should be re-registered on PROVIDERS_CHANGED | ||
| // but implicit system broadcast cannot read DB in background to find | ||
| // what to fence. |
There was a problem hiding this comment.
Reminder to create an issue for any follow-up here.
maurizi
reviewed
Jun 5, 2018
| @@ -65,8 +65,6 @@ protected void saveCallResult(@NonNull DestinationQueryResponse response) { | |||
| item.setDestination(null); | |||
| } | |||
| item.setTimestamp(timestamp); | |||
| // TODO: Remove | |||
| if (item.getDestination() != null && item.getDestination() == 20) { continue; } | |||
Listen to broadcasts to add geofences or handle geofence transitions.
Add method that does not require Activity context to get last known location. Not needed by any background workers yet.
Set up dependency injection for reading from database to find geofences to add when setting them up again after reboot.
Start listening to geofence on flag set in UI.
Use dwell in production.
Send simple notification on geofence entry or exit.
Open details on notification tap. Include place name in notification message.
Add worker to remove geofences after user unselects "want to go" for a place, and also to re-register geofences on exit.
Ensure background task gets released after worker started.
Intent callbacks for geofences may return places not added with the registered callback intent, so its data must come from the database on handling the event.
flibbertigibbet
force-pushed
the
feature/kak/add-geofencing#44
branch
from
c9f463e to
ca4a9ba
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Add geofences when user flags a place or an event with an associated destination as "want to go". Remove geofences when flag changes to something else. Add geofences back after reboot. Send a notification when user enters a geofenced area. Notification links in to the detail page for the place or event.
Demo
Notes
Device location settings must be set to "high accuracy" for geofence events to fire.
Testing
Closes #44
Closes #45