New download manager #6048
New download manager #6048
Conversation
|
This work is a looong overdue attempt to tackle the sync issues, but I believe that this require a bit better foundations than the current uploader/downloader infrastructure we have in place. Since this area is very complex and drowning in accumulated legacy issues, I don't think we can do any sensible work there without better building materials first. I identified contacts import/export area as being relatively simple and suitable to test the new approach. @tobiasKaminsky @AndyScherzinger Please take a look and let me know what you think. |
|
Unit test failed, but no output was generated. Maybe a preliminary stage failed. |
|
I don't quite understand all the new concepts/structures like live data but the code/structure look good to me 👍 |
ezaquarii
force-pushed
the
ezaquarii/new-download-manager
branch
2 times, most recently
from
a724833 to
8bf48f8
Compare
|
@AndyScherzinger @tobiasKaminsky Partial rewrite to eliminate some issues I found when trying to use it in contacts import. I have updated the changes description as well: #6048 (comment) |
ezaquarii
force-pushed
the
ezaquarii/new-download-manager
branch
4 times, most recently
from
6718a38 to
2bd5cf7
Compare
|
Unit test failed, but no output was generated. Maybe a preliminary stage failed. |
ezaquarii
force-pushed
the
ezaquarii/new-download-manager
branch
from
2bd5cf7 to
3570a7d
Compare
|
@tobiasKaminsky @AndyScherzinger ready for review |
|
Unit test failed, but no output was generated. Maybe a preliminary stage failed. |
|
IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/14061-IT |
|
Getting rid of broadcast-based progress updates is required to solve issues similar to this one: #4205 |
ezaquarii
force-pushed
the
ezaquarii/new-download-manager
branch
from
3570a7d to
70ab37d
Compare
|
Unit test failed, but no output was generated. Maybe a preliminary stage failed. |
|
IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/14063-IT |
AndyScherzinger
left a comment
AndyScherzinger
left a comment
There was a problem hiding this comment.
Needs a rebase, other than that I'd say let's give a go :)
| val op = DownloadFileOperation( | ||
| request.user.toPlatformAccount(), | ||
| request.file, | ||
| OCFileListFragment.DOWNLOAD_SEND, |
There was a problem hiding this comment.
This and the next 2 lines is only needed in one special case (FDA:2255), but it should not be hardcoded.
There was a problem hiding this comment.
@tobiasKaminsky
The thing is that DownloadFileOperation does not use those values for anything.
It seems to me that it just carries them around so FileDownloader can action finished operation.
There is nothing wrong with that approach, but since new downloader implementation does not support sending downloaded files anywhere at this moment, we don't need those values.
I'm also pretty sure that we won't use them in the future, because new implementation splits request, download state and download code into separate entitites and it doesn't have access to DownloadFileOperation instance, becuase download mechanism is hidden behind opaque function/lambda interface.
At first glance, information about post-download actions should be carried by Request and actioned in DownloaderService when downloader signals completion or failure.
However, it's pretty pointless to put those hardcoded values here, so I'll add an overloaded constructor for this case.
There was a problem hiding this comment.
@tobiasKaminsky Let me know if you'd like to migrate more use cases to the downloader as part of this PR - in such case, we'll need to add this functionality. If this is not the case, I think we can skip it. I'm confident that the design is ready to support this feature with minimal effort when we decide migrate preview functionality to the new API.
| String downloadedRemotePath = download.getRequest().getFile().getRemotePath(); | ||
| FileDataStorageManager storageManager = new FileDataStorageManager(user.toPlatformAccount(), | ||
| activity.getContentResolver()); | ||
| ocFile = storageManager.getFileByPath(downloadedRemotePath); |
There was a problem hiding this comment.
With the new approach, is this roundtrip via FDSM needed, as we already have the OCFile?
There was a problem hiding this comment.
OCFile in download status is part of the Request, so it's not updated.
DownloadStatus does not carry result OCFile, but this can be changed.
There was a problem hiding this comment.
@tobiasKaminsky Downloaded OCFile is now exposed in the download status.
tobiasKaminsky
force-pushed
the
ezaquarii/new-download-manager
branch
from
70ab37d to
a3cba24
Compare
|
This is just amazing! This is a giant step forward! |
|
@ezaquarii unit tests with TestEtmViewModel fail… |
ezaquarii
force-pushed
the
ezaquarii/new-download-manager
branch
2 times, most recently
from
71f398f to
aba9554
Compare
|
@tobiasKaminsky test should be ok now |
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/14970.apk |
|
master-Screenshot test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/14970-Screenshot-master |
|
stable-Screenshot test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/14970-Screenshot-stable |
Codacy396Lint
SpotBugs (new)
SpotBugs (master)
Lint increased! |
ezaquarii
force-pushed
the
ezaquarii/new-download-manager
branch
from
aba9554 to
d20e246
Compare
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/14986.apk |
Codacy396Lint
SpotBugs (new)
SpotBugs (master)
Lint increased! |
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/14990.apk |
Codacy396Lint
SpotBugs (new)
SpotBugs (master)
Lint increased! |
Signed-off-by: Chris Narkiewicz <hello@ezaquarii.com>
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
tobiasKaminsky
force-pushed
the
ezaquarii/new-download-manager
branch
from
d20e246 to
5a9d578
Compare
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/15024.apk |
Codacy396Lint
SpotBugs (new)
SpotBugs (master)
|
New download service is a complete rewrite of a legacy
FileDownloaderservice.Sync code uses old solution - this PR does not replace it as it would be too much work and risk to swallow.
Small comparison of old vs new downloader design:
IntentsLiveData<T>- not used in the service anymoreLiveData<T>is an observable value of typeTthat integrates with Android life-cycle mess. It's a relatively new API introduced by Google inandroidxthat is meant to cut the chores of lifecycle management. It works pretty well and the only issue I have with it is that unit testing is a bit more involving.Update: I removed dependency on
LiveData<T>- it was a bad idea o have it there.LocalConnection<S>andLocalBinder<S>LocalConnectionandLocalBinderare generic implementations of platformServiceConnectionandBinderthat everybody re-implements over and over again in the same form. It also provies a good dumping ground for typed service interaction API in general, so we can address #5866 for services this way. We already used this approach in PlayerServiceConnection.kt (extracted from legacy Java inline implementation).Apart from that, we need a connection object as a facade for
Service, becauseServiceinstance is created asynchonously and might (will) not be available when UI wants to interact with it.DownloaderinterfaceI managed to extract public downloader API and calls can be nicely delegated through all layers -> connector -> binder -> user downloader processor. All layers use the same interface.
DownloaderConnectionThis component expsoses very vanilla API to the underlying Android
Servicemess, hiding all asynchronous issues behind a boring interface.Updates are delivered using vanilla listener API (
addListener()/removeListener(), so everydeveloper should be familiar with them. API solves problematic missed updates issue caused by asynchronous start/binding races, so no more any need for messaging bus pattern to deliver updates reliably.
If we'd like to expose
LiveData<T>interface to the UI, this is the place to do it.DownloaderServiceThis is a proposed replacement for
FileDownloaderservice. Apart from cleaner code, it willkeep per-user
Downloaderinstance and automatically terminate when all downloads are finished.AsyncRunnerAsyncRunneris a replacement forAsyncTaskthat uses generic Java concurrency API and does not require any subclassing (tasks are just lambdas or functions so can be tested easily, contrary toAsyncTaskbeing virtually untestable piece of global state). Also,AsyncTaskAPI is officially deprecated by Google in favour of `java.util.concurrent' APIs.This PR extends the API by adding progress and task cancelation, as those will be needed to provide adequate UX.
MockK
MockKis aMockito, but in Kotlin. I introduced it because I findMockitounable to mock Kotlin classes on Dalvik (it works intest, but not inandroidTest). There is some weird issue with bytecode generator and I'm still investigating it.MockKhas some nice properties, like support for co-routines, but the community behind it is smaller.Mockitois more mature, but the android support seems to still be rather poorly maintained. I do not commit myself to use it - I needed something to move forward with unit tests.DexOpener
Mocking final classes Android below P (API <= 27) is not possible, which makes testing very troublesome. Android 28+ has out-of-the-box support for mocking, but our CI runs tests on older versions.
DexOpener is installed by instrumentation test runner and removes final attribute, enabling mocking of Kotlin classes.
ContactListFragment- proof of concept useContacts imports has been migrated to a new API. To test it:
Contacts will be downloaded using proposed downloader service.
MockUserSadly, using mockito to mock
Userinterface will blow up when actually trying to parcel it into Intent (mock does no contain properParcelableimplementation, obviously).This litle helper provides a true implementation mock to be used in connected tests.
EtmDownloaderFragmentand test modeETM page to test the downloader. Any download request can be scheduled with a
testflagtrue- this triggers "fake" download task that justThread.sleep()and pushes some progress. One can use it for logic testing or UI work.NotificationsManagerDownloader service must be switched to foreground and that requires a notifcation. I ported some notification code that is smeared in
FileDownloaderand placed it behind an interface.Notifications requires some more work, as the UX is not great - it contains too much redundant text and not enough interesting information. Also, new downloader architecture allows us potentially to show multiple downloads, download transfer rate, number of pending files, etc, etc... We can do more.