Enable PMD java-basic
This is the fixes necessary to enable PMD's `java-basic` ruleset. I think there will be a few rules in there that will largely be annoying, so we'll need to ultimately decide whether to use `// NOPMD` or just specify the rules we want from `java-basic`.
See merge request !280
This should fix the PMD error:
"Check the value returned by the skip() method of an InputStream to see if
the requested number of bytes has been skipped."
Installer manager fixes
Builds on the recently merged `InstallManagerService` to fix a few minor UX bugs:
* Cancellation of pending downloads now removes notifications.
* No longer shows "Downloading Downloading {AppName}" in notification, just "Downloading {AppName}"
* If cached file is same size but corrupted, remove it and then continue with the process of downloading + installing
See merge request !281
The check was set up to only cancel when the `AppDetails` for that app
was shown. This is the correct behaviour for the 'complete' event, but
not the cancel. The cancel event should always result in the relevant
notification being removed.
If we are capable of bailing earlier rather than later, then we should. This way,
if a hash doesn't match, the file will be removed and a new download will begin,
as expected. The alternative is to let the installer catch the unmatching hashes.
By then though, it is too late to really do anything meaningfull and it becomes
more difficult to recover in a way that the user would expect.
Due to the earlier refactoring of `getNotificationTitle()` (or something like that)
to `getAppName()`, it was still returning `getString(downloading_apk, appName)` instead
of just the app name.
InstallManagerService
This provides an over-arching `Service` for managing the whole install process, from checking the cache, downloading files, handling the notification. Ultimately, it should probably also handle starting and tracking progress of the final installation steps.
Note: this does undo some of the `Notification` handling stuff, putting it back to one notification per APK. I did that to get that part working OK for the short term, giving us time to figure out what the whole picture should look like. I think @pserwylo has it pretty well sketched out in #592. But I have no strong feelings about the notification stuff for 0.100, so I'm happy to shape this MR accordingly, provided its only a little work.
See merge request !278
I wrestled with this a bunch, it seems quite difficult to make the Cancel
button on the notification responsive. This collection of minor changes
made it more reliable, but its still kind of flaky. I think the problem
might be related to the fact that it is creating a whole new Notification
instance, with the accompanying Intent and PendingIntent instances, for
every single download progress update.
closes#652https://gitlab.com/fdroid/fdroidclient/issues/652
If an APK was queued to download but had not started downloading yet, it
was not able to be fully canceled because ACTION_INTERRUPTED was not sent.
That meant that the UI never got updated, even though the APK was removed
from the queue.
#652https://gitlab.com/fdroid/fdroidclient/issues/652
This logic is pretty basic for now, it'll have to be expanded a lot to
support the different UX between priv and non-priv installs. For example,
priv updates will be able to happen entirely in the background. Those will
then require leaving a notification to tell the user that the app was
updated so nothing can transparently install updates without the user
knowing. When the user is an active part of each install, like the
non-priv experience requires, then keeping the "app installed" notification
feels like just extra noise.
The Event class is no longer needed once there is specific ProgressListener
instances for each type of progress update. The sourceUrl serves as the
unique ID, like with DownloaderService and InstallManagerService.
fixes#633https://gitlab.com/fdroid/fdroidclient/issues/633
This allows the install process to have consistent data, even if the index
database changes while an APK is making its way through this process.
This also provides a set of packageNames to be easily queried.
Standardizing on Apk as the internal data type means that most of the data
that is needed for the whole lifecycle of a given APK going through this
process will be available regardless of the database changes. Once App
instances are also included, then all of the data should be available
separately from the database. This is important to support parallel
operation. The index could be updated and an app could disappear while an
APK of that app is being downloaded. In that case, it should not show
blank notifications.
Also, in AppDetail, the Apk instance is completely loaded from the db, so
there should not be any nulls on the essential bits like packageName and
download URL.
This keeps the IDs standard throughout the code: either urlString when it
should be a String, or urlString.hashCode() when it should be an int. It
also follows the naming convention in DownloaderService helper methods,
e.g. getIntentFilter(). "create" to me doesn't necessarily mean also "get"
Using @NonNull or @Nullable is fine when it is actually useful, like the
compiler can catch errors, but it also adds a lot of noise when reading the
code. For example, @NonNull here will just make people avoid thinking.
Context can never be null anywhere in Android, that's a given throughout
the Android API. And in this code, urlString is the unique ID used
throughout the process, so if its ever null, nothing works.
This keeps DownloaderService tightly focused on downloading, and makes it a
lot easier to manage Notifications since InstallManagerService's lifecycle
lasts as long as the Notifications, unlike DownloaderService.
DownloaderService is structured to be as simple as possible, and as tightly
matched to the downloading lifecycle as possible, with a single queue for
all requests to avoid downloads competing for bandwidth. This does not
represent the possibilities of the whole install process. For example,
downloading can happen in parallel with checking the cache, and if an APK
is fully cached, there is no need for it to go through the DownloaderService
queue.
This also lays the groundwork towards simplifying DownloaderService even
more, by moving the Notification handling to InstallManagerService. That
will provide a single place to manage all aspects of the Notifications that
has a lifecycle that is longer than the Notifications, unlike an Activity
or DownloaderService.
Fixes#624.
The `AppDetails` activity was not correctly asking for the active
download url string when being resumed. This change recalculates the
value when being resumed now.
Added assertions that both apkName and repoAddress need to be populated
in order to call `getUrl()`. Also verified that this is the case for all
usages of this method, which it should be. All `Apk` objects which currently
have `getUrl()` called on them are loaded using the `ApkProvider.Helper.findById()`
method without specifying which columns to load (which defaults to all).
Addresses a bug found in MR !273 whereby removing `stopForeground` results
in a persistent "Downloading ..." notification even though it was cancelled.
In the process of doing this, it also addresses / Fixes#621 by ensuring
that all downloads of apks are done in a foreground service, regardless
of the preference used for foreground updater service updating.
DownloaderService fixes
This aims to simplify the `DownloaderService` a bunch to make it easier to understand. It also fixes some bugs with the download queue and related progress reports.
This is groundwork for the upcoming `InstallManagerService` which will manage the whole process of installing an app, from checking the cache, to downloading, to running the install process.
See merge request !273
DownloaderServiceTest is just a stub of a test that doesn't really test
anything yet. It is now causing a NullPointerException, so its a problem:
java.lang.NullPointerException
at org.fdroid.fdroid.net.DownloaderService.notifyDownloadComplete(DownloaderService.java:313)
at org.fdroid.fdroid.net.DownloaderService.handleIntent(DownloaderService.java:287)
at org.fdroid.fdroid.net.DownloaderService$ServiceHandler.handleMessage(DownloaderService.java:104)
at android.os.Handler.dispatchMessage(Handler.java:99)
at android.os.Looper.loop(Looper.java:137)
at android.os.HandlerThread.run(HandlerThread.java:60)
This method provides the exact same results as the underlying method it
uses, intent.getStringExtra()
This was added in 0163d6efa6013181c2e6554760e5fa6e67a6daf9
There is already a method for reproducibly generating an int based on
a the contents of a String, thanks to @pserwylo for finding it! So
urlString.hashCode() is used as the ID for Notifications and the
DownloaderService queue (msg.what). This entirely replaces the
QUEUE_WHATS hack. Both requestCode and msg.what just need to be
unique int, and that value can always be generated by the urlString.
This also fixes a bug preventing removing correct URL from Downloader queue.
b66810944fec802aa119c0e5ec8b7875930a2c22 made a change that breaks removing
the correct item from the queue. The `what` value must be fetched based on
urlString and fed to serviceHandler.removeMessages(what). The commit made
it use the `what` value from the last item that was queued. Those will
often be different things.
This also removes all stopForeground(true) calls except for the one in
onDestroy(). The nature of an IntentService, which DownloaderService
is basically a copy of, is to quit running once it is no longer
processing Intents. That is also the time to remove the notification.
Before, these were not being reliably canceled before the final COMPLETE or
INTERRUPTED notification went out. This moves closing the progress Timer
to a finally block after the Timer is setup, which should hopefully
guarantee the progress reports are always stopped.
This prevents any more progress reports from being sent, in case there is
one pending anywhere. downloaderProgressListener needs to be volatile to
ensure that the two threads are seeing the same values.
This was an omission on my part when putting together the DownloaderService
in !248
Having the notification as its own Service is overkill and really only
serves to increase complexity. The notification stuff should not take much
time or resources at all.
use latest android testing support setup
With 2.0 of Android Studio and gradle pluging coming out shortly, I wanted to start trying to use the improved testing setup. So this is a place to track that stuff until it stabilizes.
See merge request !252
Test downloads with actual files, not dynamically generated things.
Testing with the progress reports is really hard with multiple URLs, so
just test progress with a single URL for now, and multiple URLs can still
be tested without the progress check.
fixes#650https://gitlab.com/fdroid/fdroidclient/issues/650
This is the common pattern I've seen in travis-ci builds. It should
speed things up a little bit since the adb connection process will
happen in parallel with waiting for the screen lock to be dismissed.
Two NPE fixes, cleanup
@eighthave I couldn't find why `setExecutable()` isn't used anymore. The fact that it is now unused code begs the question: why don't we need to set the APK download folder as executable anymore?
See merge request !279
Mirror the check already being done in the other fragment. As reported
via ACRA:
ANDROID_VERSION=4.4.4
APP_VERSION_NAME=0.100-alpha6
STACK_TRACE=java.lang.NullPointerException
at org.fdroid.fdroid.AppDetails$AppDetailsHeaderFragment.updateViews(AppDetails.java:1524)
at org.fdroid.fdroid.AppDetails$AppDetailsHeaderFragment.updateViews(AppDetails.java:1519)
at org.fdroid.fdroid.AppDetails.refreshHeader(AppDetails.java:624)
at org.fdroid.fdroid.AppDetails.onAppChanged(AppDetails.java:549)
at org.fdroid.fdroid.AppDetails.access$000(AppDetails.java:100)
at org.fdroid.fdroid.AppDetails$AppObserver.onChange(AppDetails.java:141)
at android.database.ContentObserver$NotificationRunnable.run(ContentObserver.java:180)
at android.os.Handler.handleCallback(Handler.java:733)
at android.os.Handler.dispatchMessage(Handler.java:95)
at android.os.Looper.loop(Looper.java:136)
at android.app.ActivityThread.main(ActivityThread.java:5146)
at java.lang.reflect.Method.invokeNative(Native Method)
at java.lang.reflect.Method.invoke(Method.java:515)
at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:732)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:566)
at dalvik.system.NativeStart.main(Native Method)
This was very probably introduced with the new CleanCacheService stuff.
Reported via ACRA:
ANDROID_VERSION=6.0.1
APP_VERSION_NAME=0.100-alpha6
STACK_TRACE=java.lang.NullPointerException: Attempt to get length of null array
at org.fdroid.fdroid.Utils.clearOldFiles(Utils.java:349)
at org.fdroid.fdroid.Utils.clearOldFiles(Utils.java:351)
at org.fdroid.fdroid.Utils.clearOldFiles(Utils.java:351)
at org.fdroid.fdroid.CleanCacheService.onHandleIntent(CleanCacheService.java:54)
at android.app.IntentService$ServiceHandler.handleMessage(IntentService.java:66)
at android.os.Handler.dispatchMessage(Handler.java:102)
at android.os.Looper.loop(Looper.java:148)
at android.os.HandlerThread.run(HandlerThread.java:61)
There is nothing to clear if the directory could not be obtained for
some reason, so this seems like a reasonable fix. Anything is better
than a crash anyway.
