[android]: Update GoogleFusedLocationProvider to use the latest API #2023

rtsisyk · 2022-02-05T07:54:36Z

rtsisyk commented

2022-02-05 07:54:36 +00:00

No description provided.

vng (Migrated from github.com) reviewed 2022-02-05 07:54:36 +00:00

vng (Migrated from github.com) reviewed 2022-02-05 10:17:33 +00:00

android/flavors/gms-enabled/com/mapswithme/maps/location/GoogleFusedLocationProvider.java

					
				@ -70,0 +76,4 @@

				    LocationSettingsRequest.Builder builder = new LocationSettingsRequest.Builder();

				    builder.addLocationRequest(locationRequest);

				    final LocationSettingsRequest locationSettingsRequest = builder.build();

vng (Migrated from github.com) commented

2022-02-05 10:08:10 +00:00

mFusedLocationClient
mSettingsClient

Они не могут быть null и потом все сыплется ? Типа нет в системе этих google сервисов/провайдеров.

mFusedLocationClient mSettingsClient Они не могут быть null и потом все сыплется ? Типа нет в системе этих google сервисов/провайдеров.

android/flavors/gms-enabled/com/mapswithme/maps/location/GoogleFusedLocationProvider.java

vng (Migrated from github.com) commented

2022-02-05 10:17:26 +00:00

Хм, чот странная логика в LocationHelper.INSTANCE .. Вызвать onLocationUpdated и потом следом еще надо вызвать notifyLocationUpdated :)

biodranik (Migrated from github.com) requested changes 2022-02-05 12:15:35 +00:00

android/flavors/gms-enabled/com/mapswithme/maps/location/GoogleFusedLocationProvider.java

					
				@ -34,3 +45,3 @@

				  @NonNull

				  private final GoogleLocationListener mListener;

				  private final GoogleLocationCallback mCallback;

biodranik (Migrated from github.com) commented

2022-02-05 11:50:34 +00:00

Returns the most recent location available in this result, or null if no locations are available.

android/flavors/gms-enabled/com/mapswithme/maps/location/GoogleFusedLocationProvider.java

					
				@ -39,3 +49,3 @@

				                              @NonNull Context context)

				  GoogleFusedLocationProvider(@NonNull LocationFixChecker locationFixChecker, @NonNull Context context)

				  {

				    super(locationFixChecker);

biodranik (Migrated from github.com) commented

2022-02-05 11:51:49 +00:00

private void onLocationChanged(@NonNull Location location)

android/flavors/gms-enabled/com/mapswithme/maps/location/GoogleFusedLocationProvider.java

biodranik (Migrated from github.com) commented

2022-02-05 12:12:09 +00:00

Хоть какой грубый новый локейшн лучше никакого или очень старого.

android/flavors/gms-enabled/com/mapswithme/maps/location/GoogleFusedLocationProvider.java

biodranik (Migrated from github.com) commented

2022-02-05 12:15:20 +00:00

Разве для fused провайдера нужна эта проверка? Он сам должен внутри миксовать и всегда выдавать лучшие позиции (или худшие по одному параметру но лучшие по другому). Я бы убрал этот фильтр.

vng (Migrated from github.com) reviewed 2022-02-05 14:10:38 +00:00

android/flavors/gms-enabled/com/mapswithme/maps/location/GoogleFusedLocationProvider.java

vng (Migrated from github.com) commented

2022-02-05 14:10:38 +00:00

Я бы в этом PR не менял логику фильтрации которая была до этого. Лучше потом отдельны PR. Иначе когда посыпется, будет непонятно из-за чего ... Пусть mLocationFixChecker логика остается как есть тут.

rtsisyk reviewed 2022-02-06 07:08:44 +00:00

android/flavors/gms-enabled/com/mapswithme/maps/location/GoogleFusedLocationProvider.java

					
				@ -34,3 +45,3 @@

				  @NonNull

				  private final GoogleLocationListener mListener;

				  private final GoogleLocationCallback mCallback;

rtsisyk commented

2022-02-06 07:08:44 +00:00

LocationResult.getLastLocation() is declared as NonNull.

  @NonNull
  public Location getLastLocation() {
    int var1 = this.zzb.size();
    return var1 == 0 ? null : (Location)this.zzb.get(var1 + -1);
  }

For mFusedLocationClient.getLastLocation() this check exists:

    mFusedLocationClient.getLastLocation().addOnSuccessListener(location -> {
      if (location == null)
        return;

LocationResult.getLastLocation() is declared as NonNull. ``` @NonNull public Location getLastLocation() { int var1 = this.zzb.size(); return var1 == 0 ? null : (Location)this.zzb.get(var1 + -1); } ``` For `mFusedLocationClient.getLastLocation()` this check exists: ``` mFusedLocationClient.getLastLocation().addOnSuccessListener(location -> { if (location == null) return; ```

rtsisyk reviewed 2022-02-06 07:09:23 +00:00

android/flavors/gms-enabled/com/mapswithme/maps/location/GoogleFusedLocationProvider.java

rtsisyk commented

2022-02-06 07:09:23 +00:00

Это старый код перенесенный во время рефакторинга.

rtsisyk reviewed 2022-02-06 07:10:15 +00:00

android/flavors/gms-enabled/com/mapswithme/maps/location/GoogleFusedLocationProvider.java

rtsisyk commented

2022-02-06 07:10:15 +00:00

mLocationFixChecker перенесен из старого кода как есть без изменений. В целом данный код выглядит логичным и я не вижу смысла его трогать.

rtsisyk reviewed 2022-02-06 07:10:59 +00:00

android/flavors/gms-enabled/com/mapswithme/maps/location/GoogleFusedLocationProvider.java

					
				@ -70,0 +76,4 @@

				    LocationSettingsRequest.Builder builder = new LocationSettingsRequest.Builder();

				    builder.addLocationRequest(locationRequest);

				    final LocationSettingsRequest locationSettingsRequest = builder.build();

rtsisyk commented

2022-02-06 07:10:58 +00:00

  @NonNull
  public static FusedLocationProviderClient getFusedLocationProviderClient(@NonNull Context var0) {

  @NonNull
  public static SettingsClient getSettingsClient(@NonNull Activity var0) {

``` @NonNull public static FusedLocationProviderClient getFusedLocationProviderClient(@NonNull Context var0) { ``` ``` @NonNull public static SettingsClient getSettingsClient(@NonNull Activity var0) { ```

rtsisyk reviewed 2022-02-06 07:13:14 +00:00

android/flavors/gms-enabled/com/mapswithme/maps/location/GoogleFusedLocationProvider.java

rtsisyk commented

2022-02-06 07:13:13 +00:00

  public void onLocationUpdated(@NonNull Location location)
  {
    mSavedLocation = location;
    mMyPosition = null;
    mSavedLocationTime = System.currentTimeMillis();
  }

  void notifyLocationUpdated()
  {
    if (mSavedLocation == null)
    {

Да, весь этот LocationHelper выглядит очень странно. Нагромождение лишних методов и классов. Но в этом патче я там вообще ничего не трогал и оставил старую логику. Любой рефакторинг здесь надо делать аккуратно чтобы не сломать что-либо.

``` public void onLocationUpdated(@NonNull Location location) { mSavedLocation = location; mMyPosition = null; mSavedLocationTime = System.currentTimeMillis(); } ``` ``` void notifyLocationUpdated() { if (mSavedLocation == null) { ``` Да, весь этот LocationHelper выглядит очень странно. Нагромождение лишних методов и классов. Но в этом патче я там вообще ничего не трогал и оставил старую логику. Любой рефакторинг здесь надо делать аккуратно чтобы не сломать что-либо.

rtsisyk reviewed 2022-02-06 07:14:19 +00:00

android/flavors/gms-enabled/com/mapswithme/maps/location/GoogleFusedLocationProvider.java

					
				@ -39,3 +49,3 @@

				                              @NonNull Context context)

				  GoogleFusedLocationProvider(@NonNull LocationFixChecker locationFixChecker, @NonNull Context context)

				  {

				    super(locationFixChecker);

rtsisyk commented

2022-02-06 07:14:19 +00:00

See my answer above: organicmaps/organicmaps#2023

See my answer above: https://git.omaps.dev/organicmaps/organicmaps/pulls/2023#discussion_r800137883

biodranik (Migrated from github.com) reviewed 2022-02-06 07:39:10 +00:00

android/flavors/gms-enabled/com/mapswithme/maps/location/GoogleFusedLocationProvider.java

					
				@ -34,3 +45,3 @@

				  @NonNull

				  private final GoogleLocationListener mListener;

				  private final GoogleLocationCallback mCallback;

biodranik (Migrated from github.com) commented

2022-02-06 07:39:10 +00:00

Documentation says, that it may return null. It should crash on the very first cold start.
In your first code sample it returns null. Where did you get it?
How is the second code sample relevant?

Please fix.

1. Documentation [says](https://developers.google.com/android/reference/com/google/android/gms/location/LocationResult?hl=en#getLastLocation()), that it may return null. It should crash on the very first cold start. 2. In your first code sample it returns null. Where did you get it? 3. How is the second code sample relevant? Please fix.

biodranik (Migrated from github.com) reviewed 2022-02-06 07:48:22 +00:00

android/flavors/gms-enabled/com/mapswithme/maps/location/GoogleFusedLocationProvider.java

biodranik (Migrated from github.com) commented

2022-02-06 07:48:22 +00:00

Вот и давай сделаем аккуратный рефакторинг. Надо решать проблему.

biodranik (Migrated from github.com) reviewed 2022-02-06 07:48:54 +00:00

android/flavors/gms-enabled/com/mapswithme/maps/location/GoogleFusedLocationProvider.java

biodranik (Migrated from github.com) commented

2022-02-06 07:48:54 +00:00

Уже очевидно, что старая логика не работает. Пользователи жалуются. Не вижу смысла оставлять старый код.

vng commented

2022-02-06 08:42:18 +00:00

(Migrated from github.com)

Я предлагаю закатить бету сначала с этими изменениями и собрать фидбек работает ли на том же Pixel 6.
Потом уже коцать рефакторинг фильтрации и прочее.

Я предлагаю закатить бету сначала с этими изменениями и собрать фидбек работает ли на том же Pixel 6. Потом уже коцать рефакторинг фильтрации и прочее.

biodranik commented

2022-02-06 08:45:10 +00:00

(Migrated from github.com)

Эти правки вообще не релевантны проблеме пикселя. Там у людей чистый оффлайн, без всяких плей сервисов. И у нас не работает. Начинает работать только после османда.

vng commented

2022-02-06 09:05:48 +00:00

(Migrated from github.com)

Ок, есть же issues которые релевантны или близки к этому фиксу? Проверить сначала там.

biodranik commented

2022-02-06 09:11:48 +00:00

(Migrated from github.com)

Ок, есть же issues которые релевантны или близки к этому фиксу? Проверить сначала там.

Нет. На google play services никто не жаловался, просто мы решили обновить старый код в рамках акции "починим локейшн везде раз и навсегда".

> Ок, есть же issues которые релевантны или близки к этому фиксу? Проверить сначала там. Нет. На google play services никто не жаловался, просто мы решили обновить старый код в рамках акции "починим локейшн везде раз и навсегда".

rtsisyk reviewed 2022-02-06 10:42:25 +00:00

android/flavors/gms-enabled/com/mapswithme/maps/location/GoogleFusedLocationProvider.java

rtsisyk commented

2022-02-06 10:42:25 +00:00

Данный делает ровно то, что написано в заголовке - переводит код на использование актуальной версии Google Location Services API вместо deprecated API. Логика по выбору наиболее точной локации в данном патче не меняется.

Данный делает ровно то, что написано в заголовке - переводит код на использование актуальной версии [Google Location Services API](https://developer.android.com/reference/android/location/package-summary.html) вместо deprecated API. Логика по выбору наиболее точной локации в данном патче не меняется.

rtsisyk reviewed 2022-02-06 10:47:08 +00:00

android/flavors/gms-enabled/com/mapswithme/maps/location/GoogleFusedLocationProvider.java

					
				@ -34,3 +45,3 @@

				  @NonNull

				  private final GoogleLocationListener mListener;

				  private final GoogleLocationCallback mCallback;

rtsisyk commented

2022-02-06 10:47:08 +00:00

It looks like a bug in their SDK and/or documentation. I added an extra check for null. Thanks.

rtsisyk commented

2022-02-06 10:48:46 +00:00

Да, целью этого патча было обновление на новое API. Хуже от этого стать точно не должно. Для других проблем созданы другие PR.

biodranik (Migrated from github.com) approved these changes 2022-02-06 11:26:55 +00:00

android/flavors/gms-enabled/com/mapswithme/maps/location/GoogleFusedLocationProvider.java

					
				@ -37,3 +48,2 @@

				  GoogleFusedLocationProvider(@NonNull LocationFixChecker locationFixChecker,

				                              @NonNull Context context)

				  GoogleFusedLocationProvider(@NonNull LocationFixChecker locationFixChecker, @NonNull Context context)

biodranik (Migrated from github.com) commented

2022-02-06 11:22:26 +00:00

Logging it will help us with debugging.

rtsisyk reviewed 2022-02-06 16:31:40 +00:00

android/flavors/gms-enabled/com/mapswithme/maps/location/GoogleFusedLocationProvider.java

					
				@ -37,3 +48,2 @@

				  GoogleFusedLocationProvider(@NonNull LocationFixChecker locationFixChecker,

				                              @NonNull Context context)

				  GoogleFusedLocationProvider(@NonNull LocationFixChecker locationFixChecker, @NonNull Context context)