[android] Streamline LocationHelper

vng (Migrated from github.com) reviewed 2022-11-27 18:37:20 +00:00

drape_frontend/my_position_controller.cpp

vng (Migrated from github.com) commented

2022-11-27 18:37:19 +00:00

Это чему-то мешает? Вроде логично при таймауте выключать позицию

rtsisyk reviewed 2022-11-28 08:39:20 +00:00

drape_frontend/my_position_controller.cpp

rtsisyk commented

2022-11-28 08:39:20 +00:00

Author

Owner

Попросили как раз не выключать и искать далее, мол gps не успевает прочухаться. Оно работает в Androiid сейчас, просто индикация была сломана. В iOS мне кажется, что отключается.

biodranik (Migrated from github.com) reviewed 2022-11-28 13:32:47 +00:00

biodranik (Migrated from github.com) left a comment

Изменений очень много, сложно оценить, как оно будет работать, и где всплывут проблемы. Надо тестировать. Соберёшь, пожалуйста, beta версию отсюда? Хотя бы для нас сначала, приватную.

android/flavors/gms-enabled/app/organicmaps/location/GoogleFusedLocationProvider.java

biodranik (Migrated from github.com) commented

2022-11-28 09:57:34 +00:00

Почему выбран именно 1 час, а не, например, 5 минут? У совершенно очень очевидно сейчас на экране, что позиция старая. Пользователь может некоторое время смотреть на какое-то совсем не релевантное место на карте, вместо честного ожидания.

android/src/app/organicmaps/MwmActivity.java

biodranik (Migrated from github.com) commented

2022-11-27 17:52:51 +00:00

@vng разве у нас нет такого колбэка? Ведь именно из ядра должно прилетать, что роут завершён.

android/src/app/organicmaps/location/AndroidNativeProvider.java

biodranik (Migrated from github.com) commented

2022-11-27 17:56:13 +00:00

У нас эта логика сработает без перезапуска приложения, если юзер в ОМ отключит GP Services?

android/src/app/organicmaps/location/AndroidNativeProvider.java

biodranik (Migrated from github.com) commented

2022-11-28 10:00:37 +00:00

Только мне такой подход с асинхронными колбэками кажется странным? Других вариантов нет?

android/src/app/organicmaps/location/LocationHelper.java

biodranik (Migrated from github.com) commented

2022-11-28 13:31:19 +00:00

Почему не start? Кажется неправильным вызывать stop() тогда, когда ничего не было запущено.

android/src/app/organicmaps/location/LocationHelper.java

biodranik (Migrated from github.com) commented

2022-11-28 13:30:12 +00:00

Может лучше вынести всё, что требует activity, туда? Сваливание в всего "кучу" в helper не выглядит хорошей идеей, особенно на фоне Android Auto и Track Recorder.

drape_frontend/my_position_controller.cpp

biodranik (Migrated from github.com) commented

2022-11-28 09:56:13 +00:00

Можно подробнее про "попросили искать дальше"? Что именно будет происходить сейчас? После 30 секундного таймаута дальше будет бесконечно крутиться радар и не гаснуть экран, пока пользователь явно не нажмёт кнопку с радаром, чтобы остановить поиск позиции?

rtsisyk reviewed 2022-11-28 19:21:08 +00:00

android/src/app/organicmaps/location/LocationHelper.java

rtsisyk commented

2022-11-28 19:21:08 +00:00

Author

Owner

Да, сделаю if (!isActive()) start(); здесь. Немного перезаложился без необходимости.

rtsisyk reviewed 2022-11-28 19:22:18 +00:00

android/src/app/organicmaps/location/LocationHelper.java

rtsisyk commented

2022-11-28 19:22:17 +00:00

Author

Owner

Я могу вынести всё это напрямую в MwmActivity исходя из соображений, что это единственный UI юзер этого хелпера. Может быть, тогда мы сможем его рано или поздно убить совсем.

Будем делать так? Код MwmActivity станет еще больше, но в целом поддержка будет проще.

Я могу вынести всё это напрямую в MwmActivity исходя из соображений, что это единственный UI юзер этого хелпера. Может быть, тогда мы сможем его рано или поздно убить совсем. Будем делать так? Код MwmActivity станет еще больше, но в целом поддержка будет проще.

rtsisyk reviewed 2022-11-28 19:33:22 +00:00

android/src/app/organicmaps/location/AndroidNativeProvider.java

rtsisyk commented

2022-11-28 19:33:22 +00:00

Author

Owner

Да, это однозначная самая кривая часть этой реализации данной подсистемы. Проблема в том, что FusedLocationProvider имеет полностью асинхронный дизайн API инициализации. То есть мы даже не можем синхронно дождаться начала подписки на GPS и можем получить ошибку когда-нибудь позже, асинхронно. Прямо сейчас на практике одна из ошибок приходит вообще синхронно. Это приводит к тому, что start() не успевает завершиться, но мы уже прыгаем в другой метод и пытаемся там что-то делать. Но и закладываться на синхронность нельзя, потому что API асинхронное и поведение под капотом может поменяться незаметно.

В AndroidNativeLocationProvider мы пародируем это поведение в целях унификации. Там есть один кейс, когда мы прям в процессе инициализации детектим, что локация тю-тю и продолжать нет смысла. При этом есть абсолютно такой же асинхронный кейс, когда юзер тупо отключает какой-нибудь провайдер в настройках системы. Пока выглядит так, что обрабатывать всё асинхронно будет более надежно.

Здесь бы неплохо зашел message-passing based подход на акторах, но это требует времени для поиска работоспособного фреймворка. Не могу дать какой-то разумный estimate на этот дизайн.

Да, это однозначная самая кривая часть этой реализации данной подсистемы. Проблема в том, что FusedLocationProvider имеет полностью асинхронный дизайн API инициализации. То есть мы даже не можем синхронно дождаться начала подписки на GPS и можем получить ошибку когда-нибудь позже, асинхронно. Прямо сейчас на практике одна из ошибок приходит вообще синхронно. Это приводит к тому, что start() не успевает завершиться, но мы уже прыгаем в другой метод и пытаемся там что-то делать. Но и закладываться на синхронность нельзя, потому что API асинхронное и поведение под капотом может поменяться незаметно. В AndroidNativeLocationProvider мы пародируем это поведение в целях унификации. Там есть один кейс, когда мы прям в процессе инициализации детектим, что локация тю-тю и продолжать нет смысла. При этом есть абсолютно такой же асинхронный кейс, когда юзер тупо отключает какой-нибудь провайдер в настройках системы. Пока выглядит так, что обрабатывать всё асинхронно будет более надежно. Здесь бы неплохо зашел message-passing based подход на акторах, но это требует времени для поиска работоспособного фреймворка. Не могу дать какой-то разумный estimate на этот дизайн.

rtsisyk reviewed 2022-11-28 19:36:06 +00:00

android/flavors/gms-enabled/app/organicmaps/location/GoogleFusedLocationProvider.java

rtsisyk commented

2022-11-28 19:36:05 +00:00

Author

Owner

Предложите любое значение для этой константы. Могу поставить 5 минут. Самое главное, что я убираю принудительный запрос getLastLocation() из кода ниже, т.к. считаю это причиной возникновения протухшей локации и весьма доставшего меня бага "Внуково" (organicmaps/organicmaps#2034). Пока наблюдения показывают, что при наличии рабочего GPS локация из subscription приходит моментально и смысла запрашивать всякий мусор из getLastLocation() нет никакго.

Предложите любое значение для этой константы. Могу поставить 5 минут. Самое главное, что я убираю принудительный запрос getLastLocation() из кода ниже, т.к. считаю это причиной возникновения протухшей локации и весьма доставшего меня бага "Внуково" (https://git.omaps.dev/organicmaps/organicmaps/issues/2034). Пока наблюдения показывают, что при наличии рабочего GPS локация из subscription приходит моментально и смысла запрашивать всякий мусор из getLastLocation() нет никакго.

rtsisyk reviewed 2022-11-28 19:37:02 +00:00

android/src/app/organicmaps/location/AndroidNativeProvider.java

rtsisyk commented

2022-11-28 19:37:01 +00:00

Author

Owner

Нет, это для обработки отключения в системе. Оно срабатывает при отключении wifi + mobile data. Fused просто отваливается моментально.

rtsisyk reviewed 2022-11-28 19:37:36 +00:00

android/src/app/organicmaps/MwmActivity.java

rtsisyk commented

2022-11-28 19:37:36 +00:00

Author

Owner

По идее там вообще в ядре можно всё разрулить. Я пока не решаю эту проблему. Просто перенес код из LocationHelper в MwmActivity без изменения семантики. Стало выглядеть лучше.

rtsisyk reviewed 2022-11-28 19:40:12 +00:00

drape_frontend/my_position_controller.cpp

rtsisyk commented

2022-11-28 19:40:11 +00:00

Author

Owner

Что именно будет происходить сейчас? После 30 секундного таймаута дальше будет бесконечно крутиться радар и не гаснуть экран, пока пользователь явно не нажмёт кнопку с радаром, чтобы остановить поиск позиции?

Будет бесконечно крутиться радар и искаться локация. Нажатие на кнопку с радаром у нас не ведет к отключению локации и лишь переключает между режимами. Таким образом, локация будет искаться вечно или пока прилага не уйдет в фоне. Это именно то, на что жаловались. Есть или смысл показывать диалог каждые 30 секунд? Не знаю. Я могу оставить эту строчку и вставить moveToNextMode() перед показом диалога. Тогда core будет думать, что локация перезапустилась и диалог вылезет еще раз через 30 секунд. Хотя текущая логика isErrorDialogAnnyoing работает именно на скрытие этого диалога совсем. Я лишь выпилил дублирующийся флажок из Android части.

> Что именно будет происходить сейчас? После 30 секундного таймаута дальше будет бесконечно крутиться радар и не гаснуть экран, пока пользователь явно не нажмёт кнопку с радаром, чтобы остановить поиск позиции? Будет бесконечно крутиться радар и искаться локация. Нажатие на кнопку с радаром у нас не ведет к отключению локации и лишь переключает между режимами. Таким образом, локация будет искаться вечно или пока прилага не уйдет в фоне. Это именно то, на что жаловались. Есть или смысл показывать диалог каждые 30 секунд? Не знаю. Я могу оставить эту строчку и вставить moveToNextMode() перед показом диалога. Тогда core будет думать, что локация перезапустилась и диалог вылезет еще раз через 30 секунд. Хотя текущая логика isErrorDialogAnnyoing работает именно на скрытие этого диалога совсем. Я лишь выпилил дублирующийся флажок из Android части.

biodranik (Migrated from github.com) reviewed 2022-11-28 22:11:03 +00:00

android/src/app/organicmaps/location/AndroidNativeProvider.java

biodranik (Migrated from github.com) commented

2022-11-28 22:11:03 +00:00

Никаких новых фреймворков тащить не нужно, пожалуйста.
Если есть несколько чётких исключений, когда прилетает на том же потоке сразу, лучше явно их откомментить и именно в этих случаях вызвать отложенный асинхронный вызов, вместо того, чтобы весь модуль писать асинхронно, даже там, где это явно не нужно. Такой подход упростит в будущем выпиливание ненужного, поддержку и рефакторинг.
Зачем нам синхронно дожидаться начала подписки на GPS? Отправили запрос в систему, ждём события, пока оно там продуплится. Что тут не так?
Про "детектим в процессе инициализации, что локация тютю" — это можно вынести отдельной проверкой ДО запуска процесса инициализации?

1. Никаких новых фреймворков тащить не нужно, пожалуйста. 2. Если есть несколько чётких исключений, когда прилетает на том же потоке сразу, лучше явно их откомментить и именно в этих случаях вызвать отложенный асинхронный вызов, вместо того, чтобы весь модуль писать асинхронно, даже там, где это явно не нужно. Такой подход упростит в будущем выпиливание ненужного, поддержку и рефакторинг. 3. Зачем нам синхронно дожидаться начала подписки на GPS? Отправили запрос в систему, ждём события, пока оно там продуплится. Что тут не так? 4. Про "детектим в процессе инициализации, что локация тютю" — это можно вынести отдельной проверкой ДО запуска процесса инициализации?

biodranik (Migrated from github.com) reviewed 2022-11-28 22:14:06 +00:00

android/flavors/gms-enabled/app/organicmaps/location/GoogleFusedLocationProvider.java

biodranik (Migrated from github.com) commented

2022-11-28 22:14:05 +00:00

Главный кейз, который должен работать с last location — это когда после работы с ОМ, ОМ сворачивается, интернет отключается и девайс блокируется. И после этого, через несколько минут, снова открывается ОМ. Что будет сейчас в таком случае? (важно чтобы в фоне не было других приложений с активной локацией).

Главный кейз, который _должен_ работать с last location — это когда после работы с ОМ, ОМ сворачивается, интернет отключается и девайс блокируется. И после этого, через несколько минут, снова открывается ОМ. Что будет сейчас в таком случае? (важно чтобы в фоне не было других приложений с активной локацией).

biodranik (Migrated from github.com) reviewed 2022-11-28 22:15:13 +00:00

android/src/app/organicmaps/location/AndroidNativeProvider.java

biodranik (Migrated from github.com) commented

2022-11-28 22:15:13 +00:00

Этот комментарий бы да в код. И другие некоторые комментарии тоже. Чем больше, тем лучше, чтобы потом не забыть важные детали.

biodranik (Migrated from github.com) reviewed 2022-11-28 22:20:28 +00:00

drape_frontend/my_position_controller.cpp

biodranik (Migrated from github.com) commented

2022-11-28 22:20:27 +00:00

Т.е. предлагается забрать у пользователей возможность перестать палить батарею, не выходя из приложения? И теперь единственный способ остановить поиск локации в ОМ будет запретить локацию в системе (сразу для всех приложений)? Мне не кажется это хорошей идеей.
Надо либо оставлять текущее поведение, либо добавлять явное отключение поиска позиции (переход в no follow no position) по тычку в крутящийся радар, как описано в #3450

Т.е. предлагается забрать у пользователей возможность _перестать_ палить батарею, не выходя из приложения? И теперь единственный способ остановить поиск локации в ОМ будет запретить локацию в системе (сразу для всех приложений)? Мне не кажется это хорошей идеей. Надо либо оставлять текущее поведение, либо добавлять явное отключение поиска позиции (переход в no follow no position) по тычку в крутящийся радар, как описано в #3450

vng (Migrated from github.com) reviewed 2022-11-29 07:00:43 +00:00

drape_frontend/my_position_controller.cpp

vng (Migrated from github.com) commented

2022-11-29 07:00:43 +00:00

Так или иначе, если все же решишь убрать таймаут, то оставь там коммент мол так и так - это больше не надо. Поможет потом вообще вычистить эту логику с таймерами.

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

android/src/app/organicmaps/MwmActivity.java

vng (Migrated from github.com) commented

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

Я сам натыкался на это и после беглого просмотра что-то там не сходится. В пределах этого PR вполне достаточно коммента Ромы.

vng (Migrated from github.com) reviewed 2022-11-29 07:06:14 +00:00

android/src/app/organicmaps/location/LocationHelper.java

vng (Migrated from github.com) commented

2022-11-29 07:06:14 +00:00

Но ведь другие активити (Download resources, Search) тоже работают с позицией.

rtsisyk reviewed 2022-12-04 07:28:52 +00:00

android/src/app/organicmaps/MwmActivity.java

rtsisyk commented

2022-12-04 07:28:52 +00:00

Author

Owner

Давайте пока не трогать это место, PR и так уже очень и очень большой.

rtsisyk reviewed 2022-12-04 07:29:41 +00:00

drape_frontend/my_position_controller.cpp

rtsisyk commented

2022-12-04 07:29:41 +00:00

Author

Owner

Т.е. предлагается забрать у пользователей возможность перестать палить батарею, не выходя из приложения?

Отключается по нажатию кнопки Stop.

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

rtsisyk reviewed 2022-12-04 07:44:34 +00:00

android/flavors/gms-enabled/app/organicmaps/location/GoogleFusedLocationProvider.java

rtsisyk commented

2022-12-04 07:44:33 +00:00

Author

Owner

Локация начнет искаться заново с холодного старта. Данный патч не добавляет поддержку локации в фоновом режиме и никак не меняет это поведение.

rtsisyk reviewed 2022-12-04 07:48:15 +00:00

android/src/app/organicmaps/location/AndroidNativeProvider.java

rtsisyk commented

2022-12-04 07:48:14 +00:00

Author

Owner

В этом патче ничего этого нет.
Не очень понял что именно предлагается поменять на текущем коде.
Не очень понял что именно предлагается поменять на текущем коде.
"Проверка" имеет асинхронное API 2d2d7a4a70/android/flavors/gms-enabled/app/organicmaps/location/GoogleFusedLocationProvider.java (L85-L119), но при этом иногда (не всегда) вызов callback может происходить в блокирующем режиме. В данном патче я всего прогоняю onLocationDisabled через event loop чтобы всегда было одинаковое неблокирующее поведение. Мы запускаем локацию, она тупит какое-то время и асинхронно фейлится.

1. В этом патче ничего этого нет. 2. Не очень понял что именно предлагается поменять на текущем коде. 3. Не очень понял что именно предлагается поменять на текущем коде. 4. "Проверка" имеет асинхронное API https://github.com/organicmaps/organicmaps/blob/2d2d7a4a705e74520aac43f5c1b634dca7453feb/android/flavors/gms-enabled/app/organicmaps/location/GoogleFusedLocationProvider.java#L85-L119, но при этом иногда (не всегда) вызов callback может происходить в блокирующем режиме. В данном патче я всего прогоняю onLocationDisabled через event loop чтобы всегда было одинаковое неблокирующее поведение. Мы запускаем локацию, она тупит какое-то время и асинхронно фейлится.

rtsisyk reviewed 2022-12-04 07:48:59 +00:00

android/src/app/organicmaps/location/AndroidNativeProvider.java

rtsisyk commented

2022-12-04 07:48:59 +00:00

Author

Owner

По коду и так очевидно и понятно, что это системная проверка и с нашими настройками никак не связана. Наши настройки отключают GoogleFusedLocationProvider, а не системный Fused. Системный fused отключать не надо.

rtsisyk reviewed 2022-12-04 07:50:49 +00:00

android/src/app/organicmaps/location/LocationHelper.java

rtsisyk commented

2022-12-04 07:50:49 +00:00

Author

Owner

Но ведь другие активити (Download resources, Search) тоже работают с позицией.

По факту Activity в LocationHelper нужна только для показа диалогов с ошибками. В случае Splash там стоит специальная проверка на пермишены сверху. Я в целом думал там сделать attach()/deattach() чтобы можно было бы обрабатывать не только пермишены, но и отключенные сервисы локации. Наверное, пусть пока этот LocationHelper останется с его attach()/deattach() семантикой и ничего инлайнить в MwmActivity не будем.

> Но ведь другие активити (Download resources, Search) тоже работают с позицией. По факту Activity в LocationHelper нужна только для показа диалогов с ошибками. В случае Splash там стоит специальная проверка на пермишены сверху. Я в целом думал там сделать attach()/deattach() чтобы можно было бы обрабатывать не только пермишены, но и отключенные сервисы локации. Наверное, пусть пока этот LocationHelper останется с его attach()/deattach() семантикой и ничего инлайнить в MwmActivity не будем.

rtsisyk reviewed 2022-12-04 08:05:09 +00:00

drape_frontend/my_position_controller.cpp

rtsisyk commented

2022-12-04 08:05:08 +00:00

Author

Owner

Логика по продолжению поиска после 30 секунд была добавлена ранее по запросу @biodranik. Данный патч не добавляет никакие новые режимы в кнопку локации. Поведение диалога также не меняется. Меняется лишь реализация (удаляются ненужные флажки) и фиксится проблема с неправильной индикацией кнопки. Без изменения данной строчки кода всё также будет работать, просто юзер будет видеть перечёркнутую иконку, хотя по факту локация будет продолжать искаться. Мне, конечно, было бы сильно проще просто увеличить там таймаут до условных 5 минут или вообще выпилить этот диалог к черту за отсутствием необходимости.

Вообщем, какие-нибудь actionable комментарии приветствуются, а пока я тут ничего больше делать не буду.

Логика по продолжению поиска после 30 секунд была добавлена ранее по запросу @biodranik. Данный патч не добавляет никакие новые режимы в кнопку локации. Поведение диалога также не меняется. Меняется лишь реализация (удаляются ненужные флажки) и фиксится проблема с неправильной индикацией кнопки. Без изменения данной строчки кода всё также будет работать, просто юзер будет видеть перечёркнутую иконку, хотя по факту локация будет продолжать искаться. Мне, конечно, было бы сильно проще просто увеличить там таймаут до условных 5 минут или вообще выпилить этот диалог к черту за отсутствием необходимости. Вообщем, какие-нибудь actionable комментарии приветствуются, а пока я тут ничего больше делать не буду.

biodranik (Migrated from github.com) reviewed 2022-12-04 09:05:16 +00:00

drape_frontend/my_position_controller.cpp

biodranik (Migrated from github.com) commented

2022-12-04 09:05:16 +00:00

Я не до конца понимаю, какое сейчас будет поведение. Можешь описать подробно, что увидит пользователь в ответ на свои действия?

biodranik (Migrated from github.com) approved these changes 2022-12-04 14:33:32 +00:00

biodranik (Migrated from github.com) left a comment

No more comments. Need to beta test it.

vng (Migrated from github.com) approved these changes 2022-12-04 15:55:01 +00:00

Rows
Columns

[android] Streamline LocationHelper #3944