Skip to content
This repository has been archived by the owner on Apr 23, 2023. It is now read-only.

[WIP] Rebase of continued Geofence implementation (see #86, #88) #91

Merged
merged 7 commits into from
Sep 8, 2016

Conversation

friesenkiwi
Copy link
Contributor

Overview

Rebase of #88, continuing to implement Geofencing support, also the API level bump is reverted

Proposed Changes

  • bump gradle versions
    
  • add dcendents dependency
    
  • implement adding Geofences by List in builder
    
  • implement adding Geofences by List
    
  • implement Geofence removal
    
  • add GeofencingEvent scaffold
    

@ecgreb
Copy link
Collaborator

ecgreb commented Sep 6, 2016

@friesenkiwi The rebase looks good :)

Test coverage just needs to be added for the new methods implemented in the geofence API and then we should be good to merge. @sarahlensing posted a few suggestions on #88 for how to approach this.

Also I noticed Circle CI is not building your branch even though the pull request is now pointing at master... we'll look into this issue on our end.

@friesenkiwi
Copy link
Contributor Author

friesenkiwi commented Sep 6, 2016

OK, thanks for the review. So it's (on my end):

I'll have to see, when I have time to work on it, probably on the weekend.

@ecgreb
Copy link
Collaborator

ecgreb commented Sep 6, 2016

If you can add some basic unit tests for creating multiple geofences and removing a geofence I would be happy to merge what we have here so far :)

Device testing and other APIs (transition types, initial trigger, etc.) can come later in follow-up pull requests.

@friesenkiwi
Copy link
Contributor Author

I added the testcases @sarahlensing suggested over at #88, added one more (looks a little bit complicated, but there's no way, to make it simpler, or is there?) and included some checkstyle fixes.
How to handle those conflicts?

ecgreb and others added 7 commits September 8, 2016 11:26
bump API level,
bump gradle versions,
add dcendents dependency
implement adding Geofences by List
implement Geofence removal
Add GeofencingEvent scaffold
add RequiresPermission annotations
fix circular method call/hidden field
Execute checkstyle rules and resepect some linter hints
@friesenkiwi
Copy link
Contributor Author

OK, never mind, I am starting to get the hang of rebasing and force-pushing :D

@ecgreb
Copy link
Collaborator

ecgreb commented Sep 8, 2016

Thanks @friesenkiwi!

I'm going to merge this now and create new issues for any outstanding TODO items related to geofences.

@ecgreb ecgreb merged commit 3af25f7 into lostzen:master Sep 8, 2016
@ecgreb ecgreb removed the in progress label Sep 8, 2016
@friesenkiwi
Copy link
Contributor Author

Thank you! :)

@ecgreb
Copy link
Collaborator

ecgreb commented Sep 8, 2016

@sarahlensing @friesenkiwi I created issues for the following outstanding features related to geofences:

Out of this list I would say we definitely want to flesh out (and return) a GeofencingEvent when an alert is triggered. Currently only the boolean extra created by the LocationManager is returned.

Also we should definitely have support for GEOFENCE_TRANSITION_ENTER and GEOFENCE_TRANSITION_EXIT transitions since we get these "for free" from the LocationManager.

Things that I would consider nice-to-haves but optional at this time are support for GEOFENCE_TRANSITION_DWELL and loitering delay. Also setting the initial trigger.

Finally, I'm not even sure we can implement anything useful for notification responsiveness as long as we are relying on the LocationManager proximity alert implementation as this would require access to the underlying location listeners.

ecgreb pushed a commit that referenced this pull request Sep 8, 2016
* Change checkstyle rules to allow for current version (see checkstyle/checkstyle#473 (comment))
Add some information to the contributing guidelines

* [WIP] Rebase of continued Geofence implementation (see #86, #88) (#91)

* Adds GeofencingRequest class

* Fix checkstyle error

* Changes in build environment:
bump API level,
bump gradle versions,
add dcendents dependency

* implement adding Geofences by List in builder
implement adding Geofences by List
implement Geofence removal
Add GeofencingEvent scaffold

* fix missing duration default
add RequiresPermission annotations
fix circular method call/hidden field

* revert targetSDK back to 23 for robolectric compatibility

* Add Test cases
Execute checkstyle rules and resepect some linter hints

* Include GitHub remarks

* Change checkstyle rules to allow for current version (see checkstyle/checkstyle#473 (comment))
Add some information to the contributing guidelines

* Include GitHub remarks
@ecgreb ecgreb mentioned this pull request Sep 8, 2016
@friesenkiwi friesenkiwi deleted the upstream-w-geofence branch September 10, 2016 14:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants