Merge remote-tracking branch 'mapsme/master' into vng-merge #332

rtsisyk · 2021-04-24T11:51:26Z

vng commented

2021-04-24 11:51:26 +00:00

(Migrated from github.com)

organicmaps/organicmaps#350

https://git.omaps.dev/organicmaps/organicmaps/issues/350

rtsisyk reviewed 2021-04-24 11:51:26 +00:00

biodranik (Migrated from github.com) reviewed 2021-04-24 12:17:48 +00:00

biodranik (Migrated from github.com) left a comment

Как-то сильно сложно всё. Хрен разберёшься, как оно будет в целом работать.

android/AndroidManifest.xml

					
				@ -25,6 +25,7 @@

				  <uses-permission android:name="android.permission.ACCESS_COARSE_LOCATION"/>

				  <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION"/>

				  <uses-permission android:name="android.permission.INTERNET"/>

				  <uses-permission android:name="android.permission.DOWNLOAD_WITHOUT_NOTIFICATION"/>

biodranik (Migrated from github.com) commented

2021-04-24 11:57:29 +00:00

Закомменти пока, посмотрим, может оно не нужно.

android/jni/com/mapswithme/maps/DownloaderAdapter.cpp

					
				@ -0,0 +37,4 @@

				{

				  CHECK_THREAD_CHECKER(m_threadChecker, ());

				  g_completionHandlers.clear();

biodranik (Migrated from github.com) commented

2021-04-24 12:02:45 +00:00

Не освобождается m_downloadManager

android/src/com/mapswithme/maps/downloader/MapDownloadCompletedProcessor.java

					
				@ -0,0 +70,4 @@

				    return copyFile(context, downloadedFileUri, dstPath);

				  }

				  private static boolean copyFile(@NonNull Context context, @NonNull Uri from, @NonNull String to)

biodranik (Migrated from github.com) commented

2021-04-24 12:09:24 +00:00

Похоже на велосипед. Разве нет готового метода для копирования? @rtsisyk

platform/downloader_utils.cpp

					
				@ -33,0 +46,4 @@

				  int64_t dataVersion = 0;

				  if (!strings::to_int64(urlComponents[1], dataVersion))

				    return false;

biodranik (Migrated from github.com) commented

2021-04-24 12:11:48 +00:00

Это может не работать у нас, урлы меняли вроде.

storage/CMakeLists.txt

					
				@ -61,3 +64,4 @@

				    background_downloading/downloader_queue.hpp

				  )

				else()

				  append(

biodranik (Migrated from github.com) commented

2021-04-24 12:13:56 +00:00

Под ios тоже ниже используется.

vng (Migrated from github.com) reviewed 2021-04-24 12:23:01 +00:00

android/AndroidManifest.xml

					
				@ -25,6 +25,7 @@

				  <uses-permission android:name="android.permission.ACCESS_COARSE_LOCATION"/>

				  <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION"/>

				  <uses-permission android:name="android.permission.INTERNET"/>

				  <uses-permission android:name="android.permission.DOWNLOAD_WITHOUT_NOTIFICATION"/>

vng (Migrated from github.com) commented

2021-04-24 12:23:00 +00:00

Я уверен, все это и писалось ради этого бубна

vng (Migrated from github.com) reviewed 2021-04-24 12:23:45 +00:00

platform/downloader_utils.cpp

					
				@ -33,0 +46,4 @@

				  int64_t dataVersion = 0;

				  if (!strings::to_int64(urlComponents[1], dataVersion))

				    return false;

vng (Migrated from github.com) commented

2021-04-24 12:23:45 +00:00

Я и обновил в соответствии с нашими правилами.

vng (Migrated from github.com) reviewed 2021-04-24 16:57:52 +00:00

android/src/com/mapswithme/maps/downloader/MapDownloadCompletedProcessor.java

					
				@ -0,0 +70,4 @@

				    return copyFile(context, downloadedFileUri, dstPath);

				  }

				  private static boolean copyFile(@NonNull Context context, @NonNull Uri from, @NonNull String to)

vng (Migrated from github.com) commented

2021-04-24 16:57:52 +00:00

Тут фишка в том, что копирование идет из Uri, который в данном контексте будет не файл на диске, а что-то типа content://downloaded/52

vng (Migrated from github.com) reviewed 2021-04-24 16:59:48 +00:00

android/jni/com/mapswithme/maps/DownloaderAdapter.cpp

					
				@ -0,0 +37,4 @@

				{

				  CHECK_THREAD_CHECKER(m_threadChecker, ());

				  g_completionHandlers.clear();

vng (Migrated from github.com) commented

2021-04-24 16:59:48 +00:00

То что он std::shared_ptr<jobject>, не делает release автоматом?

То что он ```std::shared_ptr<jobject>```, не делает release автоматом?

biodranik (Migrated from github.com) reviewed 2021-04-24 17:08:06 +00:00

android/jni/com/mapswithme/maps/DownloaderAdapter.cpp

					
				@ -0,0 +37,4 @@

				{

				  CHECK_THREAD_CHECKER(m_threadChecker, ());

				  g_completionHandlers.clear();

biodranik (Migrated from github.com) commented

2021-04-24 17:08:06 +00:00

Да, я не прав. Там в делетере shared ptr всё чистится.

biodranik (Migrated from github.com) reviewed 2021-04-24 17:10:24 +00:00

android/AndroidManifest.xml

					
				@ -25,6 +25,7 @@

				  <uses-permission android:name="android.permission.ACCESS_COARSE_LOCATION"/>

				  <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION"/>

				  <uses-permission android:name="android.permission.INTERNET"/>

				  <uses-permission android:name="android.permission.DOWNLOAD_WITHOUT_NOTIFICATION"/>

biodranik (Migrated from github.com) commented

2021-04-24 17:10:24 +00:00

Этот бубен про не оповещать в юае что файлы загрузились или зафейлились. А нам проще и лучше явно оповещать, чем лишний стрёмный пермишн. Если цель ПРа была скрыть оповещения, то оно нам не надо )

vng (Migrated from github.com) reviewed 2021-04-24 22:24:25 +00:00

android/src/com/mapswithme/maps/background/AbstractLogBroadcastReceiver.java

vng (Migrated from github.com) commented

2021-04-24 22:24:24 +00:00

@rtsisyk Тут скорее всего можно заменить на MwmBroadcastReceiver

rtsisyk reviewed 2021-04-25 11:27:20 +00:00

android/src/com/mapswithme/maps/downloader/MapDownloadCompletedProcessor.java

					
				@ -0,0 +70,4 @@

				    return copyFile(context, downloadedFileUri, dstPath);

				  }

				  private static boolean copyFile(@NonNull Context context, @NonNull Uri from, @NonNull String to)

rtsisyk commented

2021-04-25 11:27:20 +00:00

Тут надо или в плюсовом коде качать или в ведроидном, а не два параллельных загрузчика разводить.

rtsisyk reviewed 2021-04-25 11:28:29 +00:00

android/src/com/mapswithme/maps/background/AbstractLogBroadcastReceiver.java

rtsisyk commented

2021-04-25 11:28:29 +00:00

AbstractLogBroadcastReceiver делает тоже самое, что MwmBroadcastReceiver. Предлагаю просто дропнуть AbstractLogBroadcastReceiver.

rtsisyk reviewed 2021-04-25 11:28:54 +00:00

android/src/com/mapswithme/maps/background/SystemDownloadCompletedReceiver.java

rtsisyk commented

2021-04-25 11:28:54 +00:00

Вот здесь тупо заменить на MwmBroadcastReceiver

rtsisyk reviewed 2021-04-25 11:29:18 +00:00

android/src/com/mapswithme/maps/background/SystemDownloadCompletedService.java

rtsisyk commented

2021-04-25 11:29:18 +00:00

А здесь на MwmJobIntentService

rtsisyk reviewed 2021-04-25 11:30:53 +00:00

rtsisyk left a comment

Количество нового года непропорционально решаемой задаче.

android/src/com/mapswithme/maps/downloader/MapDownloadCompletedProcessor.java

					
				@ -0,0 +21,4 @@

				import java.io.InputStream;

				import java.io.OutputStream;

				public class MapDownloadCompletedProcessor

rtsisyk commented

2021-04-25 11:29:53 +00:00

Зачем так много классов на такую простую задачу?

vng (Migrated from github.com) reviewed 2021-04-25 12:38:42 +00:00

android/src/com/mapswithme/maps/downloader/MapDownloadCompletedProcessor.java

					
				@ -0,0 +21,4 @@

				import java.io.InputStream;

				import java.io.OutputStream;

				public class MapDownloadCompletedProcessor

vng (Migrated from github.com) commented

2021-04-25 12:38:41 +00:00

Ну я сделал merge из mapsme. Можно/нужно это брать как Merge commit, потом уже причесывать как сами посчитаем. Важно это взять как merge, чтобы в будущем также легко затягивать возможные фиксы оттуда.

biodranik (Migrated from github.com) reviewed 2021-04-25 15:25:13 +00:00

android/src/com/mapswithme/maps/downloader/MapDownloadCompletedProcessor.java

					
				@ -0,0 +21,4 @@

				import java.io.InputStream;

				import java.io.OutputStream;

				public class MapDownloadCompletedProcessor

biodranik (Migrated from github.com) commented

2021-04-25 15:25:13 +00:00

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

vng (Migrated from github.com) reviewed 2021-04-26 06:57:54 +00:00

android/src/com/mapswithme/maps/downloader/MapDownloadCompletedProcessor.java

					
				@ -0,0 +21,4 @@

				import java.io.InputStream;

				import java.io.OutputStream;

				public class MapDownloadCompletedProcessor

vng (Migrated from github.com) commented

2021-04-26 06:57:54 +00:00

Я довел его до ума и изменения включил в merge commit чтобы он был рабочим. Сверху буду докидывать тогда review.

vng (Migrated from github.com) reviewed 2021-04-26 07:54:05 +00:00

android/AndroidManifest.xml

					
				@ -25,6 +25,7 @@

				  <uses-permission android:name="android.permission.ACCESS_COARSE_LOCATION"/>

				  <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION"/>

				  <uses-permission android:name="android.permission.INTERNET"/>

				  <uses-permission android:name="android.permission.DOWNLOAD_WITHOUT_NOTIFICATION"/>

vng (Migrated from github.com) commented

2021-04-26 07:54:05 +00:00

Валится без него в DownloadManager

biodranik (Migrated from github.com) reviewed 2021-04-26 11:53:57 +00:00

android/AndroidManifest.xml

					
				@ -25,6 +25,7 @@

				  <uses-permission android:name="android.permission.ACCESS_COARSE_LOCATION"/>

				  <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION"/>

				  <uses-permission android:name="android.permission.INTERNET"/>

				  <uses-permission android:name="android.permission.DOWNLOAD_WITHOUT_NOTIFICATION"/>

biodranik (Migrated from github.com) commented

2021-04-26 11:53:57 +00:00

Странно, а на каком вызове? Может можно как-то отображать в юае, но не крешить? @rtsisyk подскажешь?

biodranik (Migrated from github.com) reviewed 2021-05-01 12:35:26 +00:00

android/AndroidManifest.xml

					
				@ -25,6 +25,7 @@

				  <uses-permission android:name="android.permission.ACCESS_COARSE_LOCATION"/>

				  <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION"/>

				  <uses-permission android:name="android.permission.INTERNET"/>

				  <uses-permission android:name="android.permission.DOWNLOAD_WITHOUT_NOTIFICATION"/>

biodranik (Migrated from github.com) commented

2021-05-01 12:35:25 +00:00

Не хотелось бы релизить с новым пермишном без необходимости. Получается странно, выпилили всё, а возможность втихаря в фоне что-то подгружать осталась.

biodranik (Migrated from github.com) approved these changes 2021-05-01 19:57:02 +00:00

biodranik (Migrated from github.com) left a comment

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

LGTM если оно работает на ведре как и задумывалось, и если @rtsisyk не против.

Тут вроде есть полезные коммиты не по даунлоадеру. Может их отдельно втянуть, а потом одним большим коммитом весь загрузчик? LGTM если оно работает на ведре как и задумывалось, и если @rtsisyk не против.

vng commented

2021-05-02 08:11:09 +00:00

(Migrated from github.com)

@biodranik Только один малюсенький не про download.
organicmaps/organicmaps#332/commits/7fac9e211289d5c89e20a7ca81326d87b36eb819
Бросилось в глаза когда обсуждали с тобой буст.

@biodranik Только один малюсенький не про download. https://git.omaps.dev/organicmaps/organicmaps/pulls/332/commits/7fac9e211289d5c89e20a7ca81326d87b36eb819 Бросилось в глаза когда обсуждали с тобой буст.

biodranik commented

2021-05-02 10:31:20 +00:00

(Migrated from github.com)

Тут ещё меняются файлы transit, pinger, routing. Это разве напрямую к загрузчику относится?

vng commented

2021-05-02 14:18:01 +00:00

(Migrated from github.com)

pinger к загрузчику. Эти изменения шли скопом как Merge is mapsme, и я не хочу тут разделять, чтобы не сломать логику - чтобы потом следующий Merge определился нормально.

biodranik commented

2021-05-02 15:33:18 +00:00

(Migrated from github.com)

А я бы вмержил отдельно то, что хорошо само по себе. Может и пингер тоже кстати, а потом бы мержил загрузчик, если что будет легче откатывать.

vng commented

2021-05-02 15:45:08 +00:00

(Migrated from github.com)

Лишний пессимизм. Там еще фиксы загрузчика, которые уже просто так неотделимы. Мы на бОльших реквестах так не беспокоились, как тут, где весь код на ладони. Давайте забирать как я создал, если боязно, давайте мержить как "Create merge commit".

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

No reviewers

rtsisyk

biodranik