Instead of including the etag in the HTTP GET request and letting the
server set the Response Code depending on whether the etag machines, this
makes the client first issue a HEAD request, which is uses to get the etag
and the file size. We need to do the HEAD beforehand anyway to get the
file size for resumable downloads, and this approach prevents the server
from using the etag as a form of tracking cookie:
http://lucb1e.com/rp/cookielesscookies/closes#562
This caused problems when users then tried to action the pending
install, where it would no longer have enough information to install the
app. Although it may be technically possible to keep enough information
around in memory to make the app installable, but it is not worth the
effort. If a user intentionally disables a repo, we should no longer be
responsible for keeping information about its apps around.
Fixes#995.
There are some ACRA reports with this IllegalStateException getting hit.
It used to be that it was only ever because we forgot to request the
correct fields from the database. However now I'm not sure that this is
the only source. Perhaps it is also possible in response to parcelling
apk instances, or maybe something else? Either way, this should provide
further info about whether the apk doesn't belong to a repo for some
reason.
There are two methods which allow callers to choose which fields to
return. These were originally added for performance, so you only ask for
what you need. However empirically the performance gain doesn't mean
anything, because it is dwarfed by the query that was just executed.
However, it does open the code up to bugs because we forget to ask for
the right fields. So now it just returns all fields when querying for
apks.
While investigating #1086 which was about swap being busted, I
discovered that we recently introduced a worse bug when working with
multi sig stuff. The swap process, when installing an app (or even when
listening for if a user started installing - before they even did
anything), would ask for an apk from any repo. This is wrong, because we
should only ask for the apks from the swap repo when presented with a
swap dialog.
By fixing this so that it asks for a specific apk, this may also
fix the issue in #1086, because that was about us not asking for enough
info from the database for each Apk which was returned. Now we just
return all columns, because the performance overhead should be minimal,
but it prevents this class of bugs, where we didn't fully populate
our value object. However, I'm not confident that it is fixed, because I
was unable to reproduce it due to the other crash fixed in this change.
Relevant crash:
```
java.lang.NullPointerException: Attempt to invoke virtual method 'java.lang.String org.fdroid.fdroid.data.Apk.getUrl()' on a null object reference
at org.fdroid.fdroid.views.swap.SwapAppsView$AppListAdapter$ViewHolder.setApp(SwapAppsView.java:311)
at org.fdroid.fdroid.views.swap.SwapAppsView$AppListAdapter.bindView(SwapAppsView.java:422)
at org.fdroid.fdroid.views.swap.SwapAppsView$AppListAdapter.newView(SwapAppsView.java:414)
at android.support.v4.widget.CursorAdapter.getView(CursorAdapter.java:269)
at android.widget.AbsListView.obtainView(AbsListView.java:2349)
at android.widget.ListView.makeAndAddView(ListView.java:1864)
at android.widget.ListView.fillDown(ListView.java:698)
...
```
Each app insert required asking the database for the ID of each
category an app is in. Given the categories don't change (ever)
but are only appended to, we can cache the results in a static
Java variable for increased performance.
This reduced the "repo persister" logic for me from 50 seconds
for main F-Droid and 100 seconds for Archive, down to 15 seconds
and 30 seconds respectively.
Now that we need only "insert" new apps rather than"
* Identify if an app exists
* If so, update
* If not, insert
There is much less code required for all of this stuff.
When preparing a temp database to write to, don't copy all apps/apks.
Instead, only copy those _not_ belonging to the repo we are updating.
In an ideal world, we'd not even need to copy them, but we need
their IDs to be in the temp database so that we don't accidentally
use the same auto-generated ID as the main database.
This also means that we can drop the check for "does this app exist,
and hence should we UPDATE it instead of INSERTing it?" and always
just insert it.
Then, when copying the temp table back to disk, first delete all
apps/apks _belonging to the repo being updated_. Then, copy back the
apks/apps we found in the repo. This again improves performance because
we no longer need to bopy back and forth data which we know
wont change (as evidenced by the fact it belongs to a differen trepo).
I don't think this was possible earlier before we did the work to
support repo priorities properly. That is because we had a single app
which was serviced by several repositories. Now, we have multiple
entries in the `fdroid_app` table, for each repo which supports
that app.
RepoUpdater.prepareRepoDetailsForSaving() was broken here because the Repo
properties were being set before calling it, and then the Repo instance was
passed to it for comparison. So the comparison was always saying the value
was unchanged. In IndexV1Updater, the flow doesn't need those checks.
This also fixes the bug where added repos never had their name/description/
icon/etc show up in ManageRepos and RepoDetails.
@cde found this bug working on mirror support, thanks!
related to #35closes#1016
The install process automatically sanitizes filenames to avoid exploits
that put attack code in the filename. Media files are also installed using
this logic, so the installed check needs to use sanitized file names to be
accurate.
This will be more important as people work with media, since it is quite
common to use spaces in filenames generated by humans. Media files will
not be built by fdroid, so most likely, they will have human-generated
names.
* This installer is invoked when for non-apk/media files, and
copies them to an appropriate folder on the sdcard.
* We also introduce a FileInstallerActivity to ask for storage
permissions at runtime, as needed by Android 6.0 and above,
and handle the install/uninstall process.
* A toast is shown with the install path after installation.
TODO:
* Manage Installed Apps screen doesn't show media files.
* For non apk files, the signature column would be NULL always,
and in SQLite NULL = NULL is false, but NULL IS NULL is true.
See http://www.sqlite.org/lang_expr.html Operators
If an app is downloaded into our cache, but an app with the same hash is
already installed, don't bother notifying people about it.
Extracted the logic for finding the path to an apk on disk (given a
PackageInfo object) to also be used by AppUpdateStatusSerice.
Change logging so that if we discard a downloaded file we Log.i instead
of Utils.debugLog. This is so that when debugging a release build we can
see what is happening, because this specific problem was easier to debug
with release builds (rather than setting up a custom fdroid repo).
Previously, it was only done on repo update.
Now it is done whenever an app is installed or unisntalled. The query to
update the suggested version for each app is quite slow when run at the
end of a repo update. However in this change, we are limiting the query
to only update a single app, which means that performance should not be
a problem.
This is a conservative fix. If we wanted to really highlight the
feature graphic functionality and reward upstream devs for keeping
metadata up to date, then we could also take apps which were recently
updated, and prioritise them over new apps if they have a feature
graphic.
Fixes#938.
Also fixed display of feature graphic in main screen by getting full
path to image, not the relative path (e.g. "en-US/featureGraphic.png").
This creates a hard dependence between `RepoUpdater` and
`UpdateService`. However this could be trivially extracted by moving the
helper methods from `UpdateService` to `RepoUpdater`, and making the
broadcasts more "repo updater" oriented. That would also require
changing the broadcasts which `UpdateService` listens for.
Reuses the "commiting" message to indicate how many apps have been
processed so far.
Refactors existing progress handling between `RepoUpdater` and
`UpdateService` to use `LocalBroadcastManager` in preference to
`ProgressListener`. Still needs to use `ProgressListener` to talk
between `RepoUpdater` and the `Downloader` +
`ProgressBufferedInputStream`.
The only change that is related to something more important than
notifications is the fact that now `IndexV1Updater` makes use of the
`indexUrl`. To do so, because it is final, the base class constructor
delegates to `getIndexUrl()` which is overriden by the v1 updater.
This is required because we want to differentiate between broadcasts
coming from different repo update processes.
Fixes#1054.
This was setup to work correctly, but for two problems:
* The `cursor.close()` in `CategoriesViewBinder` stops the cursor from
being requeried when required.
* The `AppProvider` was not notifying correctly after deleting apps
belonging to a repo.
Fixes#1028.
Moved methods around so the class is more coherent when reading from top
to bottom.
Added some comments.
Formatted lines to be under 120 chars. No longer suppress line length checkstyle messages.
This breaks out subclasses for each specific type of app list item,
allowing for code reuse, but also letting the specific business logic
belonging to each different app list item to be separate.
This is particularly helpful in the following situation:
* In the search results, it is great to be able to render "App
downloaded, ready to install" in the same manner as the update tab.
* In the installed app list, this is not desired. Indeed, the status
text which should be shown should reference the currently installed
version and whether the user has ignored any updates.
By separating the AppListItemController into subclasses, it reduced the
need to handle several different types of text view (e.g.
"installedStatus", "status", "ignoredStatus", "downloadReady"), and
replace them all with a "status" and "secondaryStatus" TextView. What is
displayed in status and secondaryStatus is up to the individual
subclasses of AppListItemController.
Previously, there were different pieces of business logic, invoked at
different times, which would touch subsets of the UI.
This change rips that out, and replaces it with a single place where the
UI is setup. This can always be called safely, and it will render the
correct data for the current state of the app (e.g. downloading, waiting
for install, etc).
The AppListItemState class is a dumb object which keeps track of what is
supposed to be displayed in the UI. The AppListItemController now
creates a different AppListItemState depending on what state the list
item is in. This AppListItemState is then used to bind the values of
each UI widget.
All of the binding code is now in the single `resetView()` method, but
all of the business logic for what the view should look like is
separated into different `getViewState*()` methods.
This separation should make it easier to make sense of the UI code, and
hopefully should be testable should somebody choose to write tests for
it in the future.
