[Platform][Android][iOS] Upload file in background. #12074
Conversation
mesozoic-drones
force-pushed
the
master-platform-add-dummy-uploadfile
branch
2 times, most recently
from
5018d31
to
92880eb
Compare
mesozoic-drones
changed the title
mesozoic-drones
force-pushed
the
master-platform-add-dummy-uploadfile
branch
from
92880eb
to
b78b373
Compare
|
Нельзя ли использовать интерфейс HttpUploader с измененной реализацией на платформах? Разделять реализации при помощи инстанцирования разных реализаций одного интерфейса, либо через какой-то спец флажок, переключающий режимы, либо через перегруженный конструктор. |
|
Сейчас у нас очень много похожих классов для решения задачи передачи данных HttpRequest, HttpClient, HttpThread. Не хотелось бы чтобы тоже самое получилось и с HttpUploader. Хотелось бы чтобы каким-то образом реализации (или хотя бы интерфейсы) были переиспользованы. |
mesozoic-drones
force-pushed
the
master-platform-add-dummy-uploadfile
branch
from
67e535a
to
82f95a8
Compare
|
@bykoianko @alexzatsepin PTAL |
|
Поясните п-та, в чем идея теперь. Как данный интерфейс позволяет выгружать файлы? |
На стороне ядра (например, из ArchivalReporter) создается экземпляр HttpUploader и для него через соответствующие сеттеры задаются url, путь к файлу и http-хедеры. Затем вызывается метод UploadInBackground(). Он возвращает строковый id таска, который будет в бэграунде отправлять файл на стороне платформы. Отправка осуществляется при доступном вай-фае/по событию включения вай-фая. Применительно к ArhcivalReporter: После отправки файла платформа вызывает статический метод класса ArchivalReporter, передает в него id таска. ArhcivalReporter поднимает соответсвие id - файл, и пробует удалить этот файл. |
platform/http_uploader.hpp
Outdated
| /// @TODO Add implementation. | ||
| /// \note Uses only |m_url|, |m_headers| and |m_filePath|. | ||
| /// \returns task id. | ||
| std::string UploadInBackground() const { return "dummyTaskId"; } |
There was a problem hiding this comment.
Обсудили голосом. Решили вынести метод по выгрузке файла в бэкграунде в отдельный интерфейс/класс. Кода под формированию данных для запроса (method,url, param, headers и т.д) вынести также в отдельный класс и включить его в текущий http_uploader и в новый созданный интерфейс для переиспользования.
mesozoic-drones
changed the title
|
JTANDROID |
|
JTIOS |
| #include "platform/payload.hpp" | ||
| namespace platform | ||
| { | ||
| class HttpUploaderBackground |
There was a problem hiding this comment.
HttpUploaderBackground перименуй, пожалуйста, в HttpBackgroundUploader или FileBackgroundUploader или просто BackgroundUploader
There was a problem hiding this comment.
HttpBackgroundUploader.
There was a problem hiding this comment.
Почему HttpBackgroundUploader лучше чем HttpUploaderBackground? я считаю, что предыдущий вариант был лучше. Лучше потому, что на мой взгляд так легче искать файлы и классы. легче искать потому что при сортировке они будут рядом и в случае поиска файла достаточно написать httpupload и будут предолжены оба варианта отправщиков
There was a problem hiding this comment.
Я руководствовался только лишь грамматикой английского. Правильно с точки зрения английского background uploader - бэкграунд загрузчик/ или загрузчик работающий в бэкграунде. background в данном случае это прилагательное для uploader. Uploader background - это неверно грамматически. UploaderInBackground это еще норм.
There was a problem hiding this comment.
Ну при именовании мы зачастую пропускаем предлоги, которые не влияют на читабельность и семантику класса. Чтобы не удлинять лишний раз названия. Также как и не указываем артикли, иначе у нас получилось бы UploaderInTheBackground
There was a problem hiding this comment.
В общем, если удобнее вернуть, как было, я не буду возражать. Но мое мнение, грамматика в любых случаях должна быть правильной (если не пишем предлоги, формировать значит предложение надо без использования предлогов, как я и предложил). Но возникающие неудобства при работе с файлами, поиском, тоже понимаю, поэтому не возражаю к откатке к предыдущему названию.
There was a problem hiding this comment.
Холиварить вместо работы про названия классов, переменных, функций, нэймспейсов можно бесконечно. Но не нужно. Переименовываю в изначальный вариант, тему закрываем.
mesozoic-drones
force-pushed
the
master-platform-add-dummy-uploadfile
branch
from
eaec42a
to
11bbef9
Compare
|
не лучше ли было бы если геттеры и сеттеры были бы у класса payload, а у классов отправителей было бы только по методу setpayload? |
HttpPayload - это структура, для нее get/set вообще не нужен. |
|
JTLINUX |
|
По мне по сути ок. Сейчас напишу мелки замечая. |
|
На Linux HttpUploader не реализован? Заглушка просто? |
Ну мы договорились сделать их в HttpUploader, чтобы не вносить изменения в bookmarks, user, cloud и относящиеся к ним iOS/jni связки. Цель PR - подготовить почву для логирования трэков и сделать это компромиссно-удобно для платформ. |
Да, кастомной реализации нет. |
mesozoic-drones
force-pushed
the
master-platform-add-dummy-uploadfile
branch
from
c352e75
to
0260b63
Compare
Это косметические, мелкие изменения, в jni/ios изменения все равно пришлось внести. Вместо вызова сеттеров аплоадера были бы вызваны сеттеры структуры. |
|
Например, потому что мы договорились о том, что у HttpBackgroundUploader будет намеренно отсутствовать часть сеттеров:
|
|
да, сделай, пожалуйста, сеттеры. |
mesozoic-drones
force-pushed
the
master-platform-add-dummy-uploadfile
branch
from
0260b63
to
c3cc8a9
Compare
|
JTANDROID |
1 similar comment
|
JTANDROID |
mesozoic-drones
force-pushed
the
master-platform-add-dummy-uploadfile
branch
from
8e0cf09
to
fa90d54
Compare
|
JTALL |
|
|
||
| std::string m_method; | ||
| std::string m_url; | ||
| std::map<std::string, std::string> m_params; |
There was a problem hiding this comment.
Опционально. Было бы здорово переименовать в multipartParams
There was a problem hiding this comment.
Они используются как обычные http-параметры, не только для мультипарт-аплоада.
Например
request.SetParam("locale", languages::GetCurrentOrig());
There was a problem hiding this comment.
а, ну ок, просто после вчерашнего разговора у меня было ощущение, что они специфические.
|
А в чем сложность использовать поля структуры напрямую вместо вызовов set/get? На сложность рефакторинга HttpClient это не должно повлиять. |
Например был метод SetParam, пользователь вызывал его с какими-то параметрами и ему не надо было знать как они там устроены, а теперь нужно знать тип поля, хранящие эти параметры и соответствующим образом добавлять. Или, если мы хотим помимо наших прокинутых хедеров выставляеть всегда еще какие-то, например хедер Accept-Encoding, то в методе SetHeaders мы всегда инсертим в контейнер хедеров, тем самым не затираем дефолтные хедеры, а если дать юзеру контейнер, то он может через оператор = добавить новые и затрет дефолтные. Ну, в общем, для целей инкапсуляции. |
platform/http_uploader.hpp
Outdated
|
|
||
| HttpUploader() = delete; | ||
| explicit HttpUploader(HttpPayload const & payload) : m_payload(payload) {} | ||
| HttpPayload GetPayload() const { return m_payload; } |
|
на сложность рефакторинга это не повлияет, просто нужно будет добавить геттеры-сеттеры при рефакторинге |
mesozoic-drones
force-pushed
the
master-platform-add-dummy-uploadfile
branch
from
fa90d54
to
e3ddc9f
Compare
|
JTALL |
HttpPayload, содержащей поля для отправки данных по HTTP.HttpUploadна структуруHttpPayload, добавление геттеров для полей структуры.HttpBackgroundUploaderс методом-заглушкойUpload()и полемHttpPayload. МетодUpload()должен быть реализован на платформах для отправки файла по HTTP по событию подключения wi-fi, в том числе из бэкграунда.