Issue 915: Stop/start Location manager based on listeners registered. #999

soumyashisPR · 2020-08-23T09:43:23Z

Issue: react-native-mapbox-gl https://github.com/react-native-mapbox-gl/maps/issues/915
This issue was caused by UserLocation component as it was stopping the locationManager, which can be shared by other UserLocation instances.

Fix:
'UserLocation' should only add/remove its listener on 'locationManager' service and never start or stop it. It the responsibility of the locationManager to start or stop itself depending on the number of listeners registered.

Removed code to start/stop locationManager from UserLocation component.
Added code to start/stop the LocationManager within LocationManager component itself, based on number of listeners registered at the moment.

EDIT ferdicus:
resolves #915

Maps issue 915: Stop/start Location manager based on listeners registered.

soumyashisPR · 2020-08-26T08:55:51Z

@mfazekas Would you be able to have a look at the PR?
Thanks.

mfazekas

👍 Thanks, nice work.
It would be great if you could summarise the issues fixed with the changes.

There are also some rename, which is not backward compatible. If the new names are better i'm all for it, but we must keep old versions working and showing a deprecation alert.

docs/UserLocation.md

javascript/components/UserLocation.js

mfazekas · 2020-08-27T06:12:59Z

javascript/modules/location/locationManager.js

    if (!this._listeners.includes(listener)) {
      this._listeners.push(listener);
-
-      if (this._lastKnownLocation) {


why was this removed?!

Change reverted.
Reason for removing this, I was trying to keep addListener() and the method call listener() independent, but didn't work out.

@mfazekas Please let me know if any more changes are required with this PR.
Thanks.

ferdicus

Thanks for tackling this, just added some comments.

Besides fixing failing tests it would be great if you would kindly add additional tests covering your changes.

🙇

__tests__/components/UserLocation.test.js

__tests__/modules/location/locationManager.test.js

javascript/components/UserLocation.js

ferdicus · 2020-09-11T10:17:18Z

@soumyashisPR, did you find the time to check the review comments?

Thanks 🙇

soumyashisPR · 2020-09-12T15:40:18Z

Hey @ferdicus , have addressed the review comments. Let me know if there are further changes required.
Currently stuck with some failing unit tests and unable to figure out the issue there.
Would be great if I could get some help / guidance.
Thanks

ferdicus · 2020-09-18T08:48:27Z

javascript/components/UserLocation.js

-      if (running) {
-        locationManager.start();
-
-        const location = await locationManager.getLastKnownLocation();


I missed this the last time around, I think you can remove getLastKnownLocation then from locationManager as it's not being used anymore?

Hi, this is actually being used in setLocationManager
if (running) { locationManager.addListener(this._onLocationUpdate); const location = await locationManager.getLastKnownLocation(); this._onLocationUpdate(location); } else { locationManager.removeListener(this._onLocationUpdate); }

I mean... yeah, now it does after you reverted it again :)

ferdicus · 2020-09-18T08:56:34Z

Hey @ferdicus , have addressed the review comments. Let me know if there are further changes required.
Currently stuck with some failing unit tests and unable to figure out the issue there.
Would be great if I could get some help / guidance.
Thanks

Thanks @soumyashisPR, can you please address all comments. 🙇

Also, what seems to be the issue with the unit tests?
Could you be more specific?

Thanks again

systemlevel · 2020-09-22T01:01:51Z

Is this PR going to create an issue for us who don't want the Location Manager running at all?

For example, we are unmounting the <UserLocation /> when we don't want the location manager to run at all when the app is in the background, inactive, or active.

soumyashisPR · 2020-09-22T05:11:12Z

Is this PR going to create an issue for us who don't want the Location Manager running at all?

For example, we are unmounting the <UserLocation /> when we don't want the location manager to run at all when the app is in the background, inactive, or active.

Hey @systemlevel, according to the PR, unmounting <UserLocation /> will stop it from listening to the locationManager, it won't stop the location manager, unless it was the only <UserLocation />listening to the location manager, in which case locationManger would stop.
Same applies for when <UserLocation /> is mounted, it will start listening to locationManager, and location manager is started if not already started.

soumyashisPR · 2020-09-22T05:31:01Z

Hey @ferdicus , have addressed the review comments. Let me know if there are further changes required.
Currently stuck with some failing unit tests and unable to figure out the issue there.
Would be great if I could get some help / guidance.
Thanks

Thanks @soumyashisPR, can you please address all comments. 🙇

Also, what seems to be the issue with the unit tests?
Could you be more specific?

Thanks again

Nevermind, the issue with unit tests is fixed.

Thanks.

mfazekas · 2020-10-01T05:03:34Z

__tests__/modules/location/locationManager.test.js

@@ -80,7 +81,7 @@ describe('LocationManager', () => {
        expect(locationManager._listeners).toStrictEqual([myListener]);
      });

-      test('does not readd same listener', () => {
+      test('does not read same listener', () => {


I think 're-add' make more sense here

Suggested change

test('does not read same listener', () => {

test('does not readd same listener', () => {

ferdicus

Thanks @soumyashisPR for this PR and sorry it took a while :)

Lets get this out the door 🚪

…rnmapbox#999) * Fixed doc descriptions for Camera * Fixed documentation for Camera * Maps: location manager start/stop listening fix * Remove commented lines * Reaname boolean fields for UserLocation * fix unit tests * Fix unit test cases for location manager * Fix userlocation test * Add modified docs * PR review comment addressed * PR review comment addressed * PR review commit - reverted renames * Fix unit test * typo fix for read to re-add

soumyashisPR and others added 11 commits July 2, 2020 11:46

Fixed doc descriptions for Camera

54c500e

Fixed documentation for Camera

2a19415

Merge branch 'master' of https://github.com/miqdigital/maps

8af090c

Maps: location manager start/stop listening fix

5cf5ee9

Remove commented lines

f2550ae

Reaname boolean fields for UserLocation

ccf3c1c

Merge pull request #11 from soumyashisPR/maps-issue-915-ss

b61268b

Maps issue 915: Stop/start Location manager based on listeners registered.

fix unit tests

ed46202

Fix unit test cases for location manager

5491ca3

Fix userlocation test

b11ac63

Add modified docs

6da74f7

mfazekas requested changes Aug 27, 2020

View reviewed changes

ferdicus reviewed Aug 28, 2020

View reviewed changes

__tests__/components/UserLocation.test.js Outdated Show resolved Hide resolved

__tests__/modules/location/locationManager.test.js Show resolved Hide resolved

javascript/components/UserLocation.js Outdated Show resolved Hide resolved

soumyashisPR added 2 commits September 12, 2020 20:32

PR review comment addressed

41f282b

PR review comment addressed

0df8663

soumyashisPR mentioned this pull request Sep 12, 2020

UserLocation stops update when an other UserLocation is unmounted #915

Closed

ferdicus reviewed Sep 18, 2020

View reviewed changes

PR review commit - reverted renames

ac280cc

soumyashisPR requested review from mfazekas and ferdicus September 22, 2020 05:42

Fix unit test

3c469a8

mfazekas approved these changes Oct 1, 2020

View reviewed changes

typo fix for read to re-add

1d398f0

ferdicus approved these changes Oct 2, 2020

View reviewed changes

ferdicus merged commit 9404ec4 into rnmapbox:master Oct 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 915: Stop/start Location manager based on listeners registered. #999

Issue 915: Stop/start Location manager based on listeners registered. #999

soumyashisPR commented Aug 23, 2020 •

edited by ferdicus

Loading

soumyashisPR commented Aug 26, 2020

mfazekas left a comment

mfazekas Aug 27, 2020

soumyashisPR Sep 29, 2020

soumyashisPR Oct 1, 2020

ferdicus left a comment

ferdicus commented Sep 11, 2020

soumyashisPR commented Sep 12, 2020

ferdicus Sep 18, 2020

soumyashisPR Sep 22, 2020 •

edited

Loading

ferdicus Sep 25, 2020

ferdicus commented Sep 18, 2020

systemlevel commented Sep 22, 2020 •

edited

Loading

soumyashisPR commented Sep 22, 2020 •

edited

Loading

soumyashisPR commented Sep 22, 2020

mfazekas Oct 1, 2020 •

edited

Loading

soumyashisPR Oct 1, 2020

ferdicus left a comment

	test('does not read same listener', () => {
	test('does not readd same listener', () => {

Issue 915: Stop/start Location manager based on listeners registered. #999

Issue 915: Stop/start Location manager based on listeners registered. #999

Conversation

soumyashisPR commented Aug 23, 2020 • edited by ferdicus Loading

soumyashisPR commented Aug 26, 2020

mfazekas left a comment

Choose a reason for hiding this comment

mfazekas Aug 27, 2020

Choose a reason for hiding this comment

soumyashisPR Sep 29, 2020

Choose a reason for hiding this comment

soumyashisPR Oct 1, 2020

Choose a reason for hiding this comment

ferdicus left a comment

Choose a reason for hiding this comment

ferdicus commented Sep 11, 2020

soumyashisPR commented Sep 12, 2020

ferdicus Sep 18, 2020

Choose a reason for hiding this comment

soumyashisPR Sep 22, 2020 • edited Loading

Choose a reason for hiding this comment

ferdicus Sep 25, 2020

Choose a reason for hiding this comment

ferdicus commented Sep 18, 2020

systemlevel commented Sep 22, 2020 • edited Loading

soumyashisPR commented Sep 22, 2020 • edited Loading

soumyashisPR commented Sep 22, 2020

mfazekas Oct 1, 2020 • edited Loading

Choose a reason for hiding this comment

soumyashisPR Oct 1, 2020

Choose a reason for hiding this comment

ferdicus left a comment

Choose a reason for hiding this comment

soumyashisPR commented Aug 23, 2020 •

edited by ferdicus

Loading

soumyashisPR Sep 22, 2020 •

edited

Loading

systemlevel commented Sep 22, 2020 •

edited

Loading

soumyashisPR commented Sep 22, 2020 •

edited

Loading

mfazekas Oct 1, 2020 •

edited

Loading