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.

Sign up for GitHub

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

Jump to bottom

Fix distanceFilter caching for LocationObserver #6987

Closed
wants to merge 1 commit into from
Closed

Fix distanceFilter caching for LocationObserver #6987

wants to merge 1 commit into from

Conversation

jrichardlai
Copy link
Contributor

Allow changing distanceFilter after _locationManager has been initialized.

There is 2 reasons for this PR:

  • When calling getCurrentPosition, _observerOptions is possibly not set properly ( as it is set only in startObserving), in this case it would have default values so distanceFilter will be 0 in this case ( which can be the right value but we should be more explicit about it, kCLDistanceFilterNone or RCT_DEFAULT_LOCATION_ACCURACY):
    screen shot 2016-04-14 at 8 44 09 pm
  • Another issue is that distanceFilter is cached so it can't be changed afterwards:
let options;
options = {
  enableHighAccuracy: true,
  distanceFilter: 20,
};

this.watchId = this.geolocation.watchPosition(
  () => { },
  () => { },
  options,
);

// => sets distanceFilter to 20

this.geolocation.clearWatch(this.watchId);

options = {
  enableHighAccuracy: false,
  distanceFilter: 1000,
};

this.watchId = this.geolocation.watchPosition(
  () => { },
  () => { },
  options,
);

// => distanceFilter is still 20

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @realaboo, @christopherdro and @nicklockwood to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Apr 15, 2016
@facebook-github-bot
Copy link
Contributor

@jrichardlai updated the pull request.

@jrichardlai
Copy link
Contributor Author

I added some tests not sure if that's the best way to do it. Both tests will fail before the fix.

@jrichardlai
Copy link
Contributor Author

@christopherdro any thoughts on this branch ?

@facebook-github-bot
Copy link
Contributor

@jrichardlai updated the pull request.

@christopherdro
Copy link
Contributor

@jrichardlai Looks great!

I would suggest separating this into two different PR's since it's unlikely this will get merged if tests are failing.

Can you update this PR to just include your changes to RCTLocationObserver.m?
Let's see if we can get that to pass and get merged in?

We can then follow up with your tests.

Thanks again for the PR!

@facebook-github-bot
Copy link
Contributor

@jrichardlai updated the pull request.

@jrichardlai jrichardlai force-pushed the fix-distance-filter-cache branch 2 times, most recently from dfbcade to bce30b9 Compare April 22, 2016 05:01
@facebook-github-bot
Copy link
Contributor

@jrichardlai updated the pull request.

@jrichardlai jrichardlai force-pushed the fix-distance-filter-cache branch 2 times, most recently from 26893cf to 398b5e2 Compare April 22, 2016 05:03
@facebook-github-bot
Copy link
Contributor

@jrichardlai updated the pull request.

1 similar comment
@ghost
Copy link

ghost commented May 2, 2016

@jrichardlai updated the pull request.

@jrichardlai
Copy link
Contributor Author

jrichardlai commented May 2, 2016 •
edited
Loading

@christopherdro tests don't seems to get green :(, are they intermittent or did I really broke something?

@christopherdro
Copy link
Contributor

@jrichardlai Just triggered the tests again. When's the last time you rebased this branch to master?

@jrichardlai
Copy link
Contributor Author

Weeks ago, still not green retrying again.

@ghost
Copy link

ghost commented May 3, 2016

@jrichardlai updated the pull request.

@christopherdro
Copy link
Contributor

@jrichardlai The tests aren't failing with anything related to the code in your PR. I asked when was the last time you rebased because the errors from the circle ci tests appear to be something that was addressed a week or two ago. Let's hope these pass.

@jrichardlai
Copy link
Contributor Author

@christopherdro boo they are still failing, they were green on my first commit 3 weeks ago ( which was rebased so not here anymore :( )

@christopherdro
Copy link
Contributor

@jrichardlai I thought you had re based again from the previous comment. If the last time you rebased was over 3 weeks ago, please do so again now and lets see if they pass.

@jrichardlai
Fix distanceFilter caching for LocationObserver
84c9154 
Allow changing distanceFilter after _locationManager has been initialized.
@jrichardlai jrichardlai force-pushed the fix-distance-filter-cache branch from 6fa600c to 84c9154 Compare May 4, 2016 15:39
@ghost
Copy link

ghost commented May 4, 2016

@jrichardlai updated the pull request.

@jrichardlai
Copy link
Contributor Author

@christopherdro oh wow it seem to got green this time 👍

@christopherdro
Copy link
Contributor

@facebook-github-bot shipit

@ghost ghost added the Import Started This pull request has been imported. This does not imply the PR has been approved. label May 4, 2016
@ghost
Copy link

ghost commented May 4, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review.

@ghost ghost closed this in 2310494 May 4, 2016
ptmt pushed a commit to ptmt/react-native that referenced this pull request May 9, 2016
@jrichardlai @ptmt
Fix distanceFilter caching for LocationObserver
9e19e05 
Summary:
Allow changing distanceFilter after _locationManager has been initialized.

There is 2 reasons for this PR:
- When calling `getCurrentPosition`, `_observerOptions` is possibly not set properly ( as it is set only in `startObserving`), in this case it would have default values so `distanceFilter` will be 0 in this case ( which can be the right value but we should be more explicit about it, `kCLDistanceFilterNone` or `RCT_DEFAULT_LOCATION_ACCURACY`):
<img width="961" alt="screen shot 2016-04-14 at 8 44 09 pm" src="https://cloud.githubusercontent.com/assets/159813/14551465/6aa8791a-0288-11e6-9c98-1687357f8c2a.png">
- Another issue is that `distanceFilter` is cached so it can't be changed afterwards:

```javascript
let options;
options = {
  enableHighAccuracy: true,
  distanceFilter: 20,
};

this.watchId = this.geolocation.watchPosition(
  () => { },
  () => { },
  options,
);

// => sets distanceFilter to 20

this.geolocation.clearWatch(this.watchId);

options = {
  enableHighAccuracy:
Closes facebook#6987

Differential Revision: D3258956

fb-gh-sync-id: 00a1d1b29d732a54cdc30e20a7a9a2de3dd9b725
fbshipit-source-id: 00a1d1b29d732a54cdc30e20a7a9a2de3dd9b725
@jrichardlai jrichardlai deleted the fix-distance-filter-cache branch May 19, 2016 06:02
@christopherdro christopherdro mentioned this pull request Jun 2, 2016
Closed
zebulgar pushed a commit to nightingale/react-native that referenced this pull request Jun 18, 2016
@jrichardlai @zebulgar
Fix distanceFilter caching for LocationObserver
1a99bbd 
Summary:
Allow changing distanceFilter after _locationManager has been initialized.

There is 2 reasons for this PR:
- When calling `getCurrentPosition`, `_observerOptions` is possibly not set properly ( as it is set only in `startObserving`), in this case it would have default values so `distanceFilter` will be 0 in this case ( which can be the right value but we should be more explicit about it, `kCLDistanceFilterNone` or `RCT_DEFAULT_LOCATION_ACCURACY`):
<img width="961" alt="screen shot 2016-04-14 at 8 44 09 pm" src="https://cloud.githubusercontent.com/assets/159813/14551465/6aa8791a-0288-11e6-9c98-1687357f8c2a.png">
- Another issue is that `distanceFilter` is cached so it can't be changed afterwards:

```javascript
let options;
options = {
  enableHighAccuracy: true,
  distanceFilter: 20,
};

this.watchId = this.geolocation.watchPosition(
  () => { },
  () => { },
  options,
);

// => sets distanceFilter to 20

this.geolocation.clearWatch(this.watchId);

options = {
  enableHighAccuracy:
Closes facebook#6987

Differential Revision: D3258956

fb-gh-sync-id: 00a1d1b29d732a54cdc30e20a7a9a2de3dd9b725
fbshipit-source-id: 00a1d1b29d732a54cdc30e20a7a9a2de3dd9b725
bubblesunyum pushed a commit to iodine/react-native that referenced this pull request Aug 23, 2016
@jrichardlai @bubblesunyum
Fix distanceFilter caching for LocationObserver
472adf3 
Summary:
Allow changing distanceFilter after _locationManager has been initialized.

There is 2 reasons for this PR:
- When calling `getCurrentPosition`, `_observerOptions` is possibly not set properly ( as it is set only in `startObserving`), in this case it would have default values so `distanceFilter` will be 0 in this case ( which can be the right value but we should be more explicit about it, `kCLDistanceFilterNone` or `RCT_DEFAULT_LOCATION_ACCURACY`):
<img width="961" alt="screen shot 2016-04-14 at 8 44 09 pm" src="https://cloud.githubusercontent.com/assets/159813/14551465/6aa8791a-0288-11e6-9c98-1687357f8c2a.png">
- Another issue is that `distanceFilter` is cached so it can't be changed afterwards:

```javascript
let options;
options = {
  enableHighAccuracy: true,
  distanceFilter: 20,
};

this.watchId = this.geolocation.watchPosition(
  () => { },
  () => { },
  options,
);

// => sets distanceFilter to 20

this.geolocation.clearWatch(this.watchId);

options = {
  enableHighAccuracy:
Closes facebook#6987

Differential Revision: D3258956

fb-gh-sync-id: 00a1d1b29d732a54cdc30e20a7a9a2de3dd9b725
fbshipit-source-id: 00a1d1b29d732a54cdc30e20a7a9a2de3dd9b725
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jrichardlai @facebook-github-bot @christopherdro