InstalledAppProviderService

I worked a bunch to get the swap code to represent the lifecycle of things better. This includes making SwapService stay alive as long as anything related to swap is running. SwapService is then becomes the one thing that stores all the state of swap, then the state does not need to be stored in SwapWorkflowActivity or any of the swap views.

I would like to include this in 0.100, but only if y'all think it won't delay the release. These changes are pretty much entirely contained in the swap stuff. There is some changes to App, but those are in the App(Context context, PackageManager pm, String packageName), which is the constructor that is only used for the swap stuff. It also touches RepoXMLHandler but that change only adds public to the IndexReceiver interface.

Milestone changed to 0.101

Added 34 commits:

• c3672406...f631d168 - 22 commits from branch fdroid:master
• e361d8b5 - SuppressLint("ParcelCreator") on MockApplicationInfo
• 8944d8c6 - when parsing APKs for the local repo, correctly set maxSdkVersion
• facdf85a - only parse <uses-sdk> once when looking for min/max SDK version
• 9622032a - after downloading is complete, update notification to "Tap to install"
• a5ecc2c0 - simplify local repo XML writing and remove dead code
• b6601511 - remove "Try to install" from swap UI
• 5fbce661 - new LoadAppsCacheService to load swap index.jar when swap starts
• 3fe7c2cf - mark as compatible when App/Apk instances are from installed apps
• f011291a - new CacheSwapAppsService for caching parsed apps to be swapped
• 9c37dc34 - swap: skip writing index.xml, output straight to index.jar
• a55441f8 - SwapService should be running as long as anything swap is active
• e957dabd - only use swap header image on large screens

Added 12 commits:

• a9fcfcf2 - SuppressLint("ParcelCreator") on MockApplicationInfo
• f27e2607 - when parsing APKs for the local repo, correctly set maxSdkVersion
• 7a146e67 - only parse <uses-sdk> once when looking for min/max SDK version
• b5d6cba5 - after downloading is complete, update notification to "Tap to install"
• efb47dd5 - simplify local repo XML writing and remove dead code
• 0852bda8 - remove "Try to install" from swap UI
• 257f4ed2 - new LoadAppsCacheService to load swap index.jar when swap starts
• 7c7903aa - mark as compatible when App/Apk instances are from installed apps
• a606134d - new CacheSwapAppsService for caching parsed apps to be swapped
• 394a0b3b - swap: skip writing index.xml, output straight to index.jar
• a90f9b33 - SwapService should be running as long as anything swap is active
• b936aaa0 - only use swap header image on large screens

Changed title: WIP: swap stability → WIP: InstalledAppProviderService

This has evolved into reworking the InstalledAppProvider stuff to handle caching for swap. That required reworking InstalledAppCacheUpdater with an IntentService to make things event based, and hopefully remove any thread sync issues.

Its annoying that gitlab seems to have lost the previous discussion on this MR... I wonder if there is anything we can do about that. Github is handling this issue well these days.

Discussions are never lost - they're just not listed anymore. If you know who commented, you can always dig up the threads by looking through people's activity history, where all their comments are listed.

some discussion about InstalledAppProvider https://botbot.me/freenode/fdroid-dev/2016-05-24/

mentioned in merge request !313 (merged)

Added 30 commits:

• b936aaa0...1bfd3425 - 21 commits from branch fdroid:master
• 4fac170f - simplify local repo XML writing and remove dead code
• 28940559 - swap: skip writing index.xml, output straight to index.jar
• 5834735a - new CacheSwapAppsService for caching parsed apps to be swapped
• df8371f7 - SwapService should be running as long as anything swap is active
• 3cf6e8a9 - Utils.getPackageUri() for creating Uris from packageNames
• db985b49 - InstalledAppProviderService to replace InstalledAppCacheUpdater
• 3b242ec7 - rate limit InstallApp install/delete notifications to 500ms
• ae085577 - InstalledAppProvider: store APK hash and last update time
• 8a23a835 - WIP: InstalledAppProviderServiceTest

mentioned in issue #509

Unmarked this merge request as a Work In Progress

Added 37 commits:

• 8a23a835...9c1b9176 - 27 commits from branch fdroid:master
• 74335e24 - simplify local repo XML writing and remove dead code
• 33330766 - swap: skip writing index.xml, output straight to index.jar
• 0151391e - new CacheSwapAppsService for caching parsed apps to be swapped
• 6d798f00 - SwapService should be running as long as anything swap is active
• c7435af1 - Utils.getPackageUri() for creating Uris from packageNames
• 4cc59142 - InstalledAppProviderService to replace InstalledAppCacheUpdater
• 48651385 - rate limit InstallApp install/delete notifications to 1000ms
• de8c0c10 - InstalledAppProvider: store APK hash and last update time
• e21588b0 - do not update InstalledAppProvider if already current
• 3e0df497 - fix crash if an APK has a short embedded description

@pserwylo this is ready to merged

Cool, I'll take a proper look at it today.

Other than my one comment above and the broken tests, this looks great to me. I've got a few style things to mention, but they shouldn't stop us from merging this stuff in. I'll post a WIP MR with proposed additions that I jotted down while CR-ing, based on this branch.

Added 17 commits:

• 3e0df497...de238f3f - 7 commits from branch fdroid:master
• 3ec206b1 - simplify local repo XML writing and remove dead code
• 944d355e - swap: skip writing index.xml, output straight to index.jar
• 335be87c - new CacheSwapAppsService for caching parsed apps to be swapped
• ae3ea853 - SwapService should be running as long as anything swap is active
• 677db72b - Utils.getPackageUri() for creating Uris from packageNames
• d734e584 - InstalledAppProviderService to replace InstalledAppCacheUpdater
• 906a2641 - rate limit InstallApp install/delete notifications to 1000ms
• dc9f841c - InstalledAppProvider: store APK hash and last update time
• 7c114cab - do not update InstalledAppProvider if already current
• 8c48749a - fix crash if an APK has a short embedded description

ok, I fixed the tests on android-17 but I can't reproduce all the failures on android-10, the tests now work for me in my android-10 emulator and device. So I rebased on master to see if that was at all related. We'll see. Here are the changes to my commits:

diff --git a/app/src/androidTest/java/org/fdroid/fdroid/data/InstalledAppProviderTest.java b/app/src/androidTest/java/org/fd
index a7a2b4a..56259b0 100644
--- a/app/src/androidTest/java/org/fdroid/fdroid/data/InstalledAppProviderTest.java
+++ b/app/src/androidTest/java/org/fdroid/fdroid/data/InstalledAppProviderTest.java
@@ -156,6 +156,9 @@ public class InstalledAppProviderTest extends FDroidProviderTest<InstalledAppPro
         values.put(InstalledAppProvider.DataColumns.VERSION_CODE, versionCode);
         values.put(InstalledAppProvider.DataColumns.VERSION_NAME, versionNumber);
         values.put(InstalledAppProvider.DataColumns.SIGNATURE, "");
+        values.put(InstalledAppProvider.DataColumns.LAST_UPDATE_TIME, System.currentTimeMillis());
+        values.put(InstalledAppProvider.DataColumns.HASH_TYPE, "sha256");
+        values.put(InstalledAppProvider.DataColumns.HASH, "cafecafecafecafecafecafecafecafecafecafecafecafecafecafecafecafe
         return values;
     }
 
diff --git a/app/src/main/java/org/fdroid/fdroid/data/InstalledAppProviderService.java b/app/src/main/java/org/fdroid/fdroid
index 10744c1..ca6a44c 100644
--- a/app/src/main/java/org/fdroid/fdroid/data/InstalledAppProviderService.java
+++ b/app/src/main/java/org/fdroid/fdroid/data/InstalledAppProviderService.java
@@ -144,7 +144,7 @@ public class InstalledAppProviderService extends IntentService {
             if (ACTION_INSERT.equals(action)) {
                 insertOrUpdateAppInDb(this, packageName, (PackageInfo) intent.getParcelableExtra(EXTRA_PACKAGE_INFO), false
             } else if (ACTION_UPDATE.equals(action)) {
-                insertOrUpdateAppInDb(this, packageName, (PackageInfo) intent.getParcelableExtra(EXTRA_PACKAGE_INFO), false
+                insertOrUpdateAppInDb(this, packageName, (PackageInfo) intent.getParcelableExtra(EXTRA_PACKAGE_INFO), true)
             } else if (ACTION_DELETE.equals(action)) {
                 deleteAppFromDb(this, packageName);
             }

Added 3 commits:

• 90467bf8 - InstalledAppProvider: store APK hash and last update time
• 748352e5 - do not update InstalledAppProvider if already current
• cf4fedbe - fix crash if an APK has a short embedded description

hmm, ok, so the tests are flaky, as in they don't always fail when they should. I was using InstalledAppProvider.update() which just throws a UnsupportedOperationException, but the tests weren't always failing. It seems that the current provider tests only really work on android-10. So I reworked 90467bf8 and 748352e5 to remove the pointless update logic and just switched it to insert() when there is an update. I also added a test to check the new "last update time" field.

This android-10 build still fails with " android.database.sqlite.SQLiteConstraintException: error code 19: constraint failed" I can't reproduce that on either my android-10 emulator or device, so I'm going to chalk that up to android-10 emulator flakiness.

I think now its actually ready to merge, and the tests even pass :)

Status changed to closed

Status changed to reopened

Whoops, brain fart because I closed the issue instead of merging. Fixed now.

One other comment regarding this:

I'm quite confident that this is fine in this instance (hence my successfull CR) however as a matter of principle, we should not change the order of these statements. This is a history of things which need to be done to get somebody from A -> Z where A is an arbitrary point in the past. By removing certain parts, it may be prone to introducing bugs. The main thing I checked for while CR'ing this was that nothing from this line up until the new line 301 tried to ALTER the installedApp table. If they had off (which they were entitled to) then we would have had crashes when people upgraded.

Additionally, these are the type of bugs which are hard to pick up during testing, because we don't test upgrading from arbitrary DBVersions through to the current one.

yeah, I should have said more about that change, I suppose. Since it DROPs then CREATEs, I figured that order wouldn't matter.

@pserwylo don't know what you did with the merge there, but you managed to do the merge commit without it closing this.

@mvdan: good question. Right now for me it says:

Merge in progress… This merge request is in the process of being merged, during which time it is locked and cannot be closed.

But the merge commit is in master.