Misc fixes/improvements to apk downloading
This started as a fix to #625, but quickly turned into a mini rampage to fix and/or improve a few different things in the `DownloaderService`. Hopefully the commits are self explanatory as to what they fix, but of course feel free to ask questions if you have any.
See merge request !265
Most of the logging is purely for debugging purposes during development.
As such, it has been moved to `Utils.debugLog`. Also provided more context
in some of the descriptions, so devs reading the logs without the sourcecode
will hopefully be able to infer more about what is happening. Left the error
logging as `Log.e` as it may be more informative.
Without this, if the first download in the queue is downloaded,
the notification will persist with details of that first download
until the next download begins. With this change, the notification
is remoived immediately after cancelling the download.
There was a race condition such that when the cancel `Intent` is
received, it would queue up a message on the download thread to stop
downloading. This was followed by cancelling the notification on the
thread the `Intent` was received on. As a result of the message taking
some time to get to the other thread, it would likely cause another
progress notification to be shown after we had already removed that
notification - making it show again after being cancelled.
The code in the download thread progress handler, which updates the
notification with progress, is not perfect. It does a check before
deciding to broadcast progress and update the notification. However,
the check happens at the beginning of the group of expressions. As
such, there is likely a small change that `isActive` returns true,
then the notification is cancelled on the other thread, and finally,
the download thread updates the progress again. However this is better
than it was before where the notification didn't go away.
Previously, apks were not being removed from the queue once download
was completed, only when removed. It didn't cause any specific issues
here, but I removed downloads from the queue after completion to prevent
potential problems in the future. Now the queue is a better representation
of what is to be downloaded.
Previously, navigating back to an app which is in the queue
qould indeed grey out the "Install" button and show the text
"Downloading..." in that disabled button. However, it woulnd't show
any sort of progress. This change shows an indeterminite progress
bar with the text "Waiting to start download..." underneath.
Happy to receive input on the best terminology if that is not
desirable.
In order to do this, I had to be more specific about when
the header fragment is updated. Previously, `headerFragment.updateViews()`
would get called by the `onResumeFragments()` activity method.
This was redundant because the `onResume()` method of the fragment
also invokes `updateViews()`. Thus, that call was removed (though
we still need to obtain a reference to the fragment in
`onResumeFragments()`.
When an Activity is destroyed, the next time it is created should
reinitialize all of the UI stuff again. Thus, there is no point to
clearing up the UI state before leaving. More importantly, this was
causing a problem when navigating back and forth through activities
via the downloader service notifications:
```
E java.lang.RuntimeException: Unable to destroy activity {org.fdroid.fdroid/org.fdroid.fdroid.AppDetails}: java.lang.IllegalStateException: activity is already destroyed
...
E Caused by: java.lang.IllegalStateException: activity is already destroyed
E at android.nfc.NfcActivityManager$NfcActivityState.<init>(NfcActivityManager.java:126)
E at android.nfc.NfcActivityManager.getActivityState(NfcActivityManager.java:176)
E at android.nfc.NfcActivityManager.setNdefPushContentUri(NfcActivityManager.java:252)
E at android.nfc.NfcAdapter.setBeamPushUris(NfcAdapter.java:830)
E at org.fdroid.fdroid.NfcHelper.disableAndroidBeam(NfcHelper.java:68)
E at org.fdroid.fdroid.AppDetails$AppDetailsHeaderFragment.updateViews(AppDetails.java:1507)
E at org.fdroid.fdroid.AppDetails$AppDetailsHeaderFragment.updateViews(AppDetails.java:1490)
E at org.fdroid.fdroid.AppDetails$AppDetailsHeaderFragment.removeProgress(AppDetails.java:1473)
E at org.fdroid.fdroid.AppDetails.cleanUpFinishedDownload(AppDetails.java:442)
E at org.fdroid.fdroid.AppDetails.onDestroy(AppDetails.java:567)
E at android.app.Activity.performDestroy(Activity.java:6169)
E at android.app.Instrumentation.callActivityOnDestroy(Instrumentation.java:1141)
E at android.app.ActivityThread.performDestroyActivity(ActivityThread.java:3726)
E ... 10 more
```
This is related to `onDestroy` calling a method which ends up calling
UI related stuff that assumes the `Activity` is around to interact with.
Fixes#625, as devices pre 4.0 require such `PendingIntent`s to
be passed to the notificaitons `setContentIntent` method. As the
`DownloaderService` handles both apks and repo updates, the pending
intent will either point to the relevant `AppDetails` screen for
an apk download - or the main F-Droid activity if it is any other
sort of download (i.e. a repo update download).
As per #fdroid-dev:
+mvdan | paresh: look at what lint says
+mvdan | src/main/res/transition-v21/app_list_item_click.xml:2: The resource R.transition.app_list_item_click appears to be unused
+mvdan | are you aware of this?
paresh | mvdan, oh yes
paresh | it is useless
paresh | I think I planned on adding some transition
paresh | you can remove it safely
Translators:
Danial Behzadi Persian
ezjerry liao Traditional Chinese
Mohamad Hasan Al Banna Indonesian
Olexandr Nesterenko Ukrainian
Osoitz Basque
Paresh Chouhan Hindi
Verdulo Esperanto
Translators:
Adrià García-Alzórriz Catalan
ageru French
Alberto Moshpirit Spanish
Allan Nordhøy Norwegian Bokmål
Enol P Asturian
ezjerry liao Traditional Chinese
Green Lunar Hebrew
Kristoffer Grundström Swedish
Ldm Public French
Marcelo Santana Portuguese (Brazil)
Mladen Pejaković Serbian
naofum Japanese
Pander Dutch
Sérgio Marques Portuguese (Portugal)
Tobias Bannert German
Verdulo Esperanto
Verdulo Polish
Yaron Shahrabani Hebrew
YFdyh000 Simplified Chinese
This release allows for Android 6+ support, but we'll need to ask for
permissions at runtime too. This commit simply does one half of the work
needed to support Android 6 with all things wi-fi.
See the issue and example app commit for reference:
https://github.com/mvdan/accesspoint/issues/66284f0376b
Also add useProgard true, since minifyEnabled now refers to the new
experimental code shrinker.
I'm not removing proguard yet as we depend on it for the samsung
workaround. I also do not know how to port the rest of the config
options to the new shrinker.
New DownloaderService
This merge request is too big, but I have all this work complete, so I'm submitting it now for review. If there are bigger issues with parts, I can rebase it down to the uncontroversial bits to get things merged.
This replaces `ApkDownloader`, `AsyncDownload`, and `AsyncDownloadWrapper`, and puts the whole APK download procedure into a custom `Service` that is based on the source code for `IntentService`. It can't be a subclass of `IntentService` because it needs to be cancelable. This does not yet add back a notification for the downloading, e.g. #592. That was handled before by the Android DownloadManager stuff, and was not replaced since that was disabled.
My current implementation does not filter out duplicate requests like discussed in #601. While its possible to do, I think it'll complicate the code a lot, and I really think that should be handled elsewhere. The UI should prevent the possibility of the user being able to submit duplicate install requests. If not, even if `DownloaderService` filtered them out, it would still be a buggy UX since the user would be clicking install again or something like that even though the install was in progress.
This also moves the APK verification logic to the `Installer` side. The downloading side can check the file size to see if the whole thing is downloaded. And to be extra safe, it should not be possible to submit an APK for installation without it going through the verification procedure. So the only method for installing APKs, `Installer.install()`, is where the verification now happens. Also, the installer now always copies the APK to be installed into the safe location RE: the Cure53 audit issue. This way, the APK download and cache dirs can be merged into one, making resumable downloads and cache management easy.
ping @mvdan @pserwylo @dschuermann
more comments in the commit messages
See merge request !248
Now that we have a nice background service, let's put it to work! This
makes update APKs be automatically downloaded after the index is updated.
Then when the user goes to install the updates, they will not have to wait
for the download part.
#601https://gitlab.com/fdroid/fdroidclient/issues/601
This moves a few stray preference handling instances into Preferences, and
move the non-preference "lastUpdateCheck" state to local only to
UpdateService. This will still work with existing installs since the
String constant value is the same.
This also saves the activeDownloadUrlString per packageName. Both are
necessary so that AppDetails can accurately display the current state
of a background download. Saving this per-packageName allows there to
be multiple active downloads in the background, then when people move
around AppDetails, it'll restore the progress meter and button state
when coming back to the app that they clicked install on.
By definition, there is just one DownloaderService enforced by
Android with a single active Downloader instance enforced by the
DownloaderService. That means using a static variable maps directly
to those conditions and provides a really simple, implementation,
especially compared to what would have to happen to do it via messages
from the thread and any Activities. If this ends up blocking testing
or something, it can always be changed when someone implements those
tests.
Moving towards having the Downloader always download directly into the
cache means its really easy to support resuming downloads.
This should also work for updates and icons, since it fetches the
Content Length via HTTP first. The icon and update downloading code
needs to be adjusted to support that. For APKs, it probably makes
sense to include the file size from the index db to save the query
over the net. That would be probably more helpful on Bluetooth.
This moves the last piece of code to the DownloaderService model. Moving
the APK prep and verification to Installer.installPackage() makes it hard
to mess up and make bugs like installing from the External Storage issue
found by the Cure53 audit.
AndroidNotCompatibleException is not used for anything specific right now,
and in the process of adding verification to the start of the install
process, there will be other kinds of failures. So convert that Exception
into a general usage InstallFailedException.
No need to flood receivers with progress events since they are basically
always going to the UI, and the UI will only refresh every so often. If
the refresh rate is 50Hz, then that's every 20ms. 100ms seems to make a
smooth enough progress bar, and saves some CPU time. This becomes more
important if there are multiple downloads happening in the background, like
if we make DownloaderService support parallel downloads from different repos
