[android] Simplify location code #2087

rtsisyk · 2022-02-12T04:38:47Z

rtsisyk commented

2022-02-12 04:38:47 +00:00

Remove unnecessary abstractions and duplicated code
Fix "Ask every time" location for all cases
Fix "Continue detecting your current location" - "No"
Improve error handling for both providers
Fix logical problems with isActive()/start()/stop()
Remove dangling @Nullable members from LocationProviders
Remove circular dependency between LocationProvider and LocationHelper
Remove annoying stack trace from the log

Signed-off-by: Roman Tsisyk roman@tsisyk.com

Notes for reviewers

Please take a look at BaseLocationProvider.java, AndroidNativeLocationProvider.java and GoogleFusedLocationProvider.java top to bottom.

- Remove unnecessary abstractions and duplicated code - Fix "Ask every time" location for all cases - Fix "Continue detecting your current location" - "No" - Improve error handling for both providers - Fix logical problems with isActive()/start()/stop() - Remove dangling @Nullable members from LocationProviders - Remove circular dependency between LocationProvider and LocationHelper - Remove annoying stack trace from the log Signed-off-by: Roman Tsisyk <roman@tsisyk.com> ## Notes for reviewers Please take a look at BaseLocationProvider.java, AndroidNativeLocationProvider.java and GoogleFusedLocationProvider.java top to bottom.

pastk reviewed 2022-02-12 04:38:47 +00:00

biodranik (Migrated from github.com) approved these changes 2022-02-12 08:46:04 +00:00

biodranik (Migrated from github.com) left a comment

Надо тестить, включая fdroid сборку.

android/src/com/mapswithme/maps/location/LocationHelper.java

biodranik (Migrated from github.com) commented

2022-02-12 08:42:25 +00:00

Коряво.

android/src/com/mapswithme/util/LocationUtils.java

biodranik (Migrated from github.com) commented

2022-02-12 08:40:21 +00:00

Приятнее, когда в конце предложений стоят точки.

android/src/com/mapswithme/util/LocationUtils.java

biodranik (Migrated from github.com) commented

2022-02-12 08:39:56 +00:00

Нам критически важно видеть в логах, какие есть провайдеры.

vng (Migrated from github.com) requested changes 2022-02-12 10:15:16 +00:00

android/src/com/mapswithme/maps/location/BaseLocationProvider.java

					
				@ -14,3 +12,1 @@

				  private boolean mActive;

				  @NonNull

				  LocationFixChecker getLocationFixChecker()

				  static protected final Logger LOGGER = LoggerFactory.INSTANCE.getLogger(LoggerFactory.Type.LOCATION);

vng (Migrated from github.com) commented

2022-02-12 10:09:19 +00:00

nit: You can leave static logger instance in Base class.

android/src/com/mapswithme/util/LocationUtils.java

vng (Migrated from github.com) commented

2022-02-12 10:11:25 +00:00

nit: Rename getDiff with LocationUtils.getTimeDiff, because it also may be getLengthDiff, getSpeedDiff ...

android/src/com/mapswithme/util/LocationUtils.java

					
				@ -101,0 +132,4 @@

				    return newLocation.getAccuracy() < lastAccuracy;

				  }

vng (Migrated from github.com) commented

2022-02-12 10:14:41 +00:00

You added this function as a replacement of LocationFixChecker, but where is it called from?

android/src/com/mapswithme/util/LocationUtils.java

vng (Migrated from github.com) commented

2022-02-12 10:15:09 +00:00

You added this function as a replacement of LocationFixChecker, but where is it called from?

rtsisyk reviewed 2022-02-12 11:23:38 +00:00

android/src/com/mapswithme/util/LocationUtils.java

rtsisyk commented

2022-02-12 11:23:38 +00:00

В логах пишет Request Android native provider 'xxx' to get locations at this interval = 5000 ms для каждого активного провайдера.

organicmaps/organicmaps#2087/files

В логах пишет `Request Android native provider 'xxx' to get locations at this interval = 5000 ms` для каждого активного провайдера. https://git.omaps.dev/organicmaps/organicmaps/pulls/2087/files#diff-8d5ad1b4db802a27fbe9bd55c954aa421f225be89973810cbc2613aded33ee8aR84

rtsisyk reviewed 2022-02-12 11:24:47 +00:00

android/src/com/mapswithme/maps/location/LocationHelper.java

rtsisyk commented

2022-02-12 11:24:47 +00:00

Это общая проблема паттерна с static enum. По этой же методологии сделаны почти все 10 с чем то хелперов. Можем попробовать отдельно переделать их все, но патч будет очень большой.

rtsisyk reviewed 2022-02-12 11:29:19 +00:00

android/src/com/mapswithme/util/LocationUtils.java

					
				@ -101,0 +132,4 @@

				    return newLocation.getAccuracy() < lastAccuracy;

				  }

rtsisyk commented

2022-02-12 11:29:19 +00:00

Not a replacement, just moved the same code from LocaitonFixChecker to LocationUtils. Both isAccuracySatisfied and isLocationBetterThanLast functions are called from LocationHelper.onLocationChanged().

organicmaps/organicmaps#2087/files

Not a replacement, just moved the same code from LocaitonFixChecker to LocationUtils. Both `isAccuracySatisfied` and `isLocationBetterThanLast` functions are called from `LocationHelper.onLocationChanged()`. https://git.omaps.dev/organicmaps/organicmaps/pulls/2087/files#diff-92baea9cd78cc337841445fde116fcd6b7e9dc6968d9cd56e855cbbf8a32b420R301-R330

biodranik (Migrated from github.com) reviewed 2022-02-12 11:29:30 +00:00

android/src/com/mapswithme/util/LocationUtils.java

biodranik (Migrated from github.com) commented

2022-02-12 11:29:30 +00:00

Нам нужен список всех доступных на устройстве провайдеров. Он сейчас печатается, если отправлять логи из аппы?

rtsisyk reviewed 2022-02-12 11:29:57 +00:00

android/src/com/mapswithme/util/LocationUtils.java

rtsisyk commented

2022-02-12 11:29:57 +00:00

Please see a comment regarding isAccuracySatisfied() above.

rtsisyk reviewed 2022-02-12 11:31:49 +00:00

android/src/com/mapswithme/maps/location/BaseLocationProvider.java

					
				@ -14,3 +12,1 @@

				  private boolean mActive;

				  @NonNull

				  LocationFixChecker getLocationFixChecker()

				  static protected final Logger LOGGER = LoggerFactory.INSTANCE.getLogger(LoggerFactory.Type.LOCATION);

rtsisyk commented

2022-02-12 11:31:49 +00:00

Done.

rtsisyk reviewed 2022-02-12 11:32:03 +00:00

android/src/com/mapswithme/util/LocationUtils.java

rtsisyk commented

2022-02-12 11:32:02 +00:00

Done.

rtsisyk reviewed 2022-02-12 12:17:20 +00:00

android/src/com/mapswithme/util/LocationUtils.java

rtsisyk commented

2022-02-12 12:17:20 +00:00

Мы подписываемся на все доступные.

vng (Migrated from github.com) reviewed 2022-02-12 12:26:00 +00:00

android/src/com/mapswithme/maps/location/LocationHelper.java

vng (Migrated from github.com) commented

2022-02-12 12:26:00 +00:00

nit: По моему этот startInternal() лог obsolete. Или добавить важное вниз, после if (!PermissionsUtils.isLocationGranted(mContext)), в лог mLogger.i(TAG, "start(): interval = " + mInterval);

vng (Migrated from github.com) reviewed 2022-02-12 12:26:34 +00:00

android/src/com/mapswithme/util/LocationUtils.java

					
				@ -101,0 +132,4 @@

				    return newLocation.getAccuracy() < lastAccuracy;

				  }

vng (Migrated from github.com) commented

2022-02-12 12:26:34 +00:00

Ok, увидел.

vng (Migrated from github.com) approved these changes 2022-02-12 12:27:29 +00:00

rtsisyk reviewed 2022-02-12 13:31:13 +00:00

android/src/com/mapswithme/maps/location/LocationHelper.java

rtsisyk commented

2022-02-12 13:31:12 +00:00

Объединил с логгированием визу.

biodranik (Migrated from github.com) reviewed 2022-02-12 15:59:24 +00:00

android/src/com/mapswithme/util/LocationUtils.java

biodranik (Migrated from github.com) commented

2022-02-12 15:59:24 +00:00

В заголовке логов, как раньше, пишется список всех доступных на устройстве провайдеров?

This repo is archived. You cannot comment on pull requests.

No reviewers

pastk

biodranik

vng

No labels

Android Automotive (AAOS)

API

AppGallery

AppStore

Battery and Performance

External Map Datasets

F-Droid

Fonts

Frequently User Reported

No milestone

No project

No assignees

3 participants

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: organicmaps/organicmaps-tmp#2087

Rows
Columns