Follow-up from "Use new index and database library"
The following discussions from !1135 (merged) should be addressed:
-
@eighthave started a discussion: (+2 comments) Why are these commented out?
-
@eighthave started a discussion: (+2 comments) These should be constants in the canonical classes, e.g. in _org.fdroid.index.v1.IndexV1.kt:
public data class IndexV1( internal const val SIGNED_FILE_NAME = "index-v1.jar"; internal const val DATA_FILE_NAME = "index-v1.json";
And in org.fdroid.index.v2.IndexV2.kt under
Entry
:internal const val DATA_FILE_NAME = "index-v2.json";
And in org.fdroid.index.v2.IndexV2.kt under
Entry
:public data class Entry( internal const val SIGNED_FILE_NAME = "entry.jar"; internal const val DATA_FILE_NAME = "entry.json";
index-v1.unsigned.jar is only used here, so it can stay here IMHO.
-
@eighthave started a discussion: (+4 comments) IIRC, the original test was testing two things:
- The conversion of the v0 index.xml truncated permissions to full, canonical permission names
- Permissions parsing when a given permission was specified more than once in the manifest
Clearly it makes sense to remove the first case since there is no more index.xml support. But i think we want to keep the second aspect of this test. I'm pretty sure that the Android build tools treat the collection of
<uses-permission>
as a list, and will just assemble a single list of them in the final APK. That means if the app dev specifies a permission twice, it will appear in the manifest twice. And even more painful, the dev could specify<uses-permission>
multiple times but with differentmaxSdkVersion
. Or in<uses-permission>
and in<uses-permission-sdk23>
. I'm not sure if this new test case is the right place to test this any more, but I do think that we need that kind of test somewhere.I manually converted the permissions list in the old extendedPerms.xml to this format, here's the diff:
diff --git a/app/src/androidTest/java/org/fdroid/fdroid/installer/ApkVerifierTest.java b/app/src/androidTest/java/org/fdroid/fdroid/installer/ApkVerifierTest.java index b32c4e4b6..72d90d771 100644 --- a/app/src/androidTest/java/org/fdroid/fdroid/installer/ApkVerifierTest.java +++ b/app/src/androidTest/java/org/fdroid/fdroid/installer/ApkVerifierTest.java @@ -273,22 +273,27 @@ public class ApkVerifierTest { Apk apk = new Apk(); apk.packageName = "urzip.at.or.at.urzip"; ArrayList<PermissionV2> perms = new ArrayList<>(); - perms.add(new PermissionV2("android.permission.READ_EXTERNAL_STORAGE", 18)); + perms.add(new PermissionV2("android.permission.READ_EXTERNAL_STORAGE", null)); perms.add(new PermissionV2("android.permission.WRITE_SYNC_SETTINGS", null)); perms.add(new PermissionV2("android.permission.ACCESS_NETWORK_STATE", null)); - perms.add(new PermissionV2("android.permission.WRITE_EXTERNAL_STORAGE", 18)); + perms.add(new PermissionV2("android.permission.WRITE_EXTERNAL_STORAGE", null)); perms.add(new PermissionV2("android.permission.WRITE_CONTACTS", null)); perms.add(new PermissionV2("android.permission.ACCESS_WIFI_STATE", null)); perms.add(new PermissionV2("android.permission.REQUEST_IGNORE_BATTERY_OPTIMIZATIONS", null)); perms.add(new PermissionV2("android.permission.WRITE_CALENDAR", null)); perms.add(new PermissionV2("android.permission.READ_CONTACTS", null)); perms.add(new PermissionV2("android.permission.READ_SYNC_SETTINGS", null)); - perms.add(new PermissionV2("android.permission.MANAGE_ACCOUNTS", 22)); + perms.add(new PermissionV2("android.permission.MANAGE_ACCOUNTS", null)); perms.add(new PermissionV2("android.permission.INTERNET", null)); - perms.add(new PermissionV2("android.permission.AUTHENTICATE_ACCOUNTS", 22)); - perms.add(new PermissionV2("android.permission.GET_ACCOUNTS", 22)); + perms.add(new PermissionV2("android.permission.AUTHENTICATE_ACCOUNTS", null)); + perms.add(new PermissionV2("android.permission.GET_ACCOUNTS", null)); perms.add(new PermissionV2("android.permission.READ_CALENDAR", null)); perms.add(new PermissionV2("android.permission.READ_SYNC_STATS", null)); + perms.add(new PermissionV2("android.permission.GET_ACCOUNTS", 22)); + perms.add(new PermissionV2("android.permission.READ_EXTERNAL_STORAGE", 18)); + perms.add(new PermissionV2("android.permission.WRITE_EXTERNAL_STORAGE", 18)); + perms.add(new PermissionV2("android.permission.AUTHENTICATE_ACCOUNTS", 22)); + perms.add(new PermissionV2("android.permission.MANAGE_ACCOUNTS", 22)); apk.setRequestedPermissions(perms, 0); ArrayList<PermissionV2> perms23 = new ArrayList<>(); perms23.add(new PermissionV2("android.permission.CAMERA", null));
-
@uniqx started a discussion: (+2 comments) I found this issue which is repoducible on my Fairphone4 running CalyxOS 4.3.0 (Android 13, api level 33)
After clicking the uninstall button, and the app got uninstalled. The uninstall (and open) button is still visible. If I hit it again the app crashed. Expected behavior would be to hide the uninstall button and display the install button instead.
Here are crash logs where I reproduced it with several different apps:
logcat_11-17-2022_23-20-39.txt
-
@eighthave started a discussion: Seems to me that this should never happen, so this should crash the app ASAP so that the dev realizes that they've broken something. Like convert the Log message to a
IllegalStateException
. -
@eighthave started a discussion: Just to confirm: it is my understanding that the new data classes only contain variables that are set from the index file, so this kind of test is no longer necessary. Are there any instance variables that should never be set from the index?
-
@eighthave started a discussion: Yes, this is still needed since it resets the database after the device's OS was upgraded. All sorts of things could have changed, so it is much easier to just reset the database then maintain a list of the relevant changes for each OS version.
-
@eighthave started a discussion: I found no place that adds
EXTRA_REPO_ID
to theIntent
. It seems it is not wired up. It looks like below you might have intended the swap repo to be fixed at0
. -
@eighthave started a discussion: Are you sure this can be removed? Does
db.getRepositoryDao().getRepositories()
not return any swap repos that the user is connected to? If the regular index update happens while swap is active, there could be two processes trying to update the swap repo at the same time. Or the generally schedule repo update could try to connect to the last swap setup. I suppose if the current code can guarantee to clean up the old swap repos, it would be OK to remove this. I'm not sure that is even possible since there is not a clear single to differentiate temporary network difficulties vs the user is done swapping. -
@eighthave started a discussion: Doesn't the
repoId
need to be an actual value from the database? This42L
seems to be a static value for the tests, but this is used for the live swapping.