Refactor UpdateManager for better readability

also separate UpdateInstaller since updating apps is technically a different concern than loading updates and exposing state about them.

app/src/main/kotlin/org/fdroid/install/AppInstallManager.kt

@@ -125,13 +125,24 @@ constructor(
    */
   @UiThread
   suspend fun install(
-    appMetadata: AppMetadata,
+    appMetadata: AppMetadata?,
     version: AppVersion,
     currentVersionName: String?,
-    repo: Repository,
+    repo: Repository?,
     iconModel: Any?,
     canAskPreApprovalNow: Boolean,
   ): InstallState {
+    if (appMetadata == null || repo == null) {
+      log.error { "Can't install app without metadata for ${version.packageName}" }
+      return InstallState.Error(
+        msg = "App ${version.packageName} no longer in DB.",
+        name = version.packageName,
+        versionName = version.versionName,
+        currentVersionName = currentVersionName,
+        lastUpdated = version.added,
+        iconModel = iconModel,
+      )
+    }
     val packageName = appMetadata.packageName
     val currentState = apps.value[packageName]
     if (currentState?.showProgress == true && currentState !is InstallState.Waiting) {

app/src/main/kotlin/org/fdroid/updates/UpdateInstaller.kt

@@ -0,0 +1,113 @@
+package org.fdroid.updates
+
+import android.content.Context
+import androidx.core.os.LocaleListCompat
+import dagger.hilt.android.qualifiers.ApplicationContext
+import javax.inject.Inject
+import javax.inject.Singleton
+import kotlin.math.min
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.currentCoroutineContext
+import kotlinx.coroutines.ensureActive
+import kotlinx.coroutines.joinAll
+import kotlinx.coroutines.launch
+import kotlinx.coroutines.sync.Semaphore
+import kotlinx.coroutines.sync.withPermit
+import org.fdroid.LocaleChooser.getBestLocale
+import org.fdroid.database.AppVersion
+import org.fdroid.database.FDroidDatabase
+import org.fdroid.index.RepoManager
+import org.fdroid.install.AppInstallManager
+import org.fdroid.ui.apps.AppUpdateItem
+import org.fdroid.utils.IoDispatcher
+
+/**
+ * Encapsulates installation orchestration for app updates.
+ *
+ * Responsibilities:
+ * * decide pre-approval behavior for batch updates
+ * * update other apps in parallel with bounded concurrency
+ * * defer updating the client app itself until the end
+ */
+@Singleton
+class UpdateInstaller
+@Inject
+constructor(
+  @param:ApplicationContext private val context: Context,
+  private val db: FDroidDatabase,
+  private val repoManager: RepoManager,
+  private val appInstallManager: AppInstallManager,
+  @param:IoDispatcher private val coroutineScope: CoroutineScope,
+) {
+  companion object {
+    private const val UNKNOWN_APP_NAME = "Unknown"
+    private const val MAX_CONCURRENT_UPDATES = 8
+  }
+
+  /**
+   * Applies all provided updates.
+   *
+   * @param appsToUpdate the update items to install
+   * @param canAskPreApprovalNow whether pre-approval may be requested now
+   */
+  suspend fun updateAll(appsToUpdate: List<AppUpdateItem>, canAskPreApprovalNow: Boolean) {
+    if (appsToUpdate.isEmpty()) return
+    val canRequestPreApproval = canAskPreApprovalNow && appsToUpdate.size == 1
+
+    // separate the update for our own app (if present) from the rest,
+    // so we can defer it until the end and do all other updates in parallel first
+    val (ownAppList, otherApps) = appsToUpdate.partition { it.packageName == context.packageName }
+    // set our own app to "waiting for install" state, so the UI can reflect that immediately
+    val ownApp = ownAppList.firstOrNull()
+    if (ownApp != null) setOwnAppWaitingState(ownApp)
+
+    // Update all non-self apps first, then our own package at the end.
+    updateAppsInParallel(otherApps, canRequestPreApproval)
+
+    ownApp?.let { update -> updateApp(update, canRequestPreApproval) }
+  }
+
+  private suspend fun updateAppsInParallel(
+    apps: List<AppUpdateItem>,
+    canRequestPreApproval: Boolean,
+  ) {
+    val concurrencyLimit = min(Runtime.getRuntime().availableProcessors(), MAX_CONCURRENT_UPDATES)
+    val semaphore = Semaphore(concurrencyLimit)
+
+    apps
+      .map { update ->
+        coroutineScope.launch {
+          semaphore.withPermit {
+            currentCoroutineContext().ensureActive()
+            updateApp(update, canRequestPreApproval)
+          }
+        }
+      }
+      .joinAll()
+
+    currentCoroutineContext().ensureActive()
+  }
+
+  private fun setOwnAppWaitingState(update: AppUpdateItem) {
+    val app = db.getAppDao().getApp(update.repoId, update.packageName)
+    appInstallManager.setWaitingState(
+      packageName = update.packageName,
+      name = app?.metadata?.name.getBestLocale(LocaleListCompat.getDefault()) ?: UNKNOWN_APP_NAME,
+      versionName = update.update.versionName,
+      currentVersionName = update.installedVersionName,
+      lastUpdated = update.update.added,
+    )
+  }
+
+  private suspend fun updateApp(update: AppUpdateItem, canAskPreApprovalNow: Boolean) {
+    val app = db.getAppDao().getApp(update.repoId, update.packageName)
+    appInstallManager.install(
+      appMetadata = app?.metadata,
+      version = update.update as AppVersion,
+      currentVersionName = update.installedVersionName,
+      repo = repoManager.getRepository(update.repoId),
+      iconModel = update.iconModel,
+      canAskPreApprovalNow = canAskPreApprovalNow,
+    )
+  }
+}

app/src/main/kotlin/org/fdroid/updates/UpdateMappers.kt

@@ -0,0 +1,83 @@
+package org.fdroid.updates
+
+import androidx.core.os.LocaleListCompat
+import io.ktor.client.engine.ProxyConfig
+import org.fdroid.database.AvailableAppWithIssue
+import org.fdroid.database.UnavailableAppWithIssue
+import org.fdroid.database.UpdatableApp
+import org.fdroid.download.DownloadRequest
+import org.fdroid.download.PackageName
+import org.fdroid.download.getImageModel
+import org.fdroid.index.RepoManager
+import org.fdroid.ui.apps.AppUpdateItem
+import org.fdroid.ui.apps.AppWithIssueItem
+
+private const val UNKNOWN_APP_NAME = "Unknown app"
+private const val UNAVAILABLE_APP_LAST_UPDATED = -1L
+
+/** Transforms a [UpdatableApp] from the database into an [AppUpdateItem] for the UI layer. */
+internal fun UpdatableApp.toAppUpdateItem(
+  localeList: LocaleListCompat,
+  proxyConfig: ProxyConfig?,
+  repoManager: RepoManager,
+): AppUpdateItem {
+  val iconDownloadRequest =
+    repoManager.getRepository(repoId)?.let { repo ->
+      getIcon(localeList)?.getImageModel(repo, proxyConfig)
+    } as? DownloadRequest
+
+  return AppUpdateItem(
+    repoId = repoId,
+    packageName = packageName,
+    name = name ?: UNKNOWN_APP_NAME,
+    installedVersionName = installedVersionName,
+    update = update,
+    whatsNew = update.getWhatsNew(localeList)?.trim(),
+    iconModel = PackageName(packageName, iconDownloadRequest),
+  )
+}
+
+/** Transforms an [AvailableAppWithIssue] into an [AppWithIssueItem] for the UI layer. */
+internal fun AvailableAppWithIssue.toAppWithIssueItem(
+  localeList: LocaleListCompat,
+  proxyConfig: ProxyConfig?,
+  repoManager: RepoManager,
+): AppWithIssueItem {
+  val iconDownloadRequest =
+    repoManager.getRepository(app.repoId)?.let { repo ->
+      app.getIcon(localeList)?.getImageModel(repo, proxyConfig)
+    } as? DownloadRequest
+
+  return AppWithIssueItem(
+    packageName = app.packageName,
+    name = app.getName(localeList) ?: UNKNOWN_APP_NAME,
+    installedVersionName = installVersionName,
+    installedVersionCode = installVersionCode,
+    issue = issue,
+    lastUpdated = app.lastUpdated,
+    iconModel = PackageName(app.packageName, iconDownloadRequest),
+  )
+}
+
+/** Transforms an [UnavailableAppWithIssue] into an [AppWithIssueItem] for the UI layer. */
+internal fun UnavailableAppWithIssue.toAppWithIssueItem(): AppWithIssueItem {
+  return AppWithIssueItem(
+    packageName = packageName,
+    name = name.toString(),
+    installedVersionName = installVersionName,
+    installedVersionCode = installVersionCode,
+    issue = issue,
+    lastUpdated = UNAVAILABLE_APP_LAST_UPDATED,
+    iconModel = PackageName(packageName, null),
+  )
+}
+
+/** Transforms an [AppUpdateItem] into an [AppUpdate] for notification purposes. */
+internal fun AppUpdateItem.toAppUpdate(): AppUpdate {
+  return AppUpdate(
+    packageName = packageName,
+    name = name,
+    currentVersionName = installedVersionName,
+    updateVersionName = update.versionName,
+  )
+}

app/src/main/kotlin/org/fdroid/updates/UpdatesManager.kt

@@ -4,33 +4,23 @@ import android.content.Context
 import android.content.pm.PackageInfo
 import androidx.core.os.LocaleListCompat
 import dagger.hilt.android.qualifiers.ApplicationContext
+import io.ktor.client.engine.ProxyConfig
 import javax.inject.Inject
 import javax.inject.Singleton
-import kotlin.math.min
 import kotlinx.coroutines.CoroutineScope
-import kotlinx.coroutines.currentCoroutineContext
-import kotlinx.coroutines.ensureActive
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.asStateFlow
-import kotlinx.coroutines.flow.first
+import kotlinx.coroutines.flow.firstOrNull
 import kotlinx.coroutines.flow.map
-import kotlinx.coroutines.joinAll
 import kotlinx.coroutines.launch
-import kotlinx.coroutines.sync.Semaphore
-import kotlinx.coroutines.sync.withPermit
 import mu.KotlinLogging
-import org.fdroid.LocaleChooser.getBestLocale
 import org.fdroid.NotificationManager
-import org.fdroid.database.AppVersion
+import org.fdroid.database.AppWithIssue
 import org.fdroid.database.AvailableAppWithIssue
 import org.fdroid.database.DbAppChecker
-import org.fdroid.database.FDroidDatabase
 import org.fdroid.database.UnavailableAppWithIssue
-import org.fdroid.download.DownloadRequest
-import org.fdroid.download.PackageName
-import org.fdroid.download.getImageModel
+import org.fdroid.database.UpdatableApp
 import org.fdroid.index.RepoManager
-import org.fdroid.install.AppInstallManager
 import org.fdroid.install.InstalledAppsCache
 import org.fdroid.repo.RepoUpdateWorker
 import org.fdroid.settings.SettingsManager
@@ -43,13 +33,12 @@ class UpdatesManager
 @Inject
 constructor(
   @param:ApplicationContext private val context: Context,
-  private val db: FDroidDatabase,
   private val dbAppChecker: DbAppChecker,
   private val settingsManager: SettingsManager,
   private val repoManager: RepoManager,
-  private val appInstallManager: AppInstallManager,
   private val installedAppsCache: InstalledAppsCache,
   private val notificationManager: NotificationManager,
+  private val updateInstaller: UpdateInstaller,
   @param:IoDispatcher private val coroutineScope: CoroutineScope,
 ) {
   private val log = KotlinLogging.logger {}
@@ -80,26 +69,16 @@ constructor(
     }
 
   val notificationStates: UpdateNotificationState
-    get() =
-      UpdateNotificationState(
-        updates =
-          updates.value?.map { update ->
-            AppUpdate(
-              packageName = update.packageName,
-              name = update.name,
-              currentVersionName = update.installedVersionName,
-              updateVersionName = update.update.versionName,
-            )
-          } ?: emptyList()
-      )
+    get() = UpdateNotificationState(updates = updates.value.orEmpty().map { it.toAppUpdate() })
 
   init {
     coroutineScope.launch {
-      // refresh updates whenever installed apps change
+      // Auto-refresh updates when installed apps change.
       installedAppsCache.installedApps.collect { loadUpdates(it) }
     }
   }
 
+  /** Loads available updates and app issues for the given [packageInfoMap]. */
   fun loadUpdates(
     packageInfoMap: Map<String, PackageInfo> = installedAppsCache.installedApps.value
   ) =
@@ -109,122 +88,63 @@ constructor(
       try {
         log.info { "Checking for updates (${packageInfoMap.size} apps)..." }
         val proxyConfig = settingsManager.proxyConfig
-        val apps = dbAppChecker.getApps(packageInfoMap = packageInfoMap)
-        val updates =
-          apps.updates.map { update ->
-            val iconModel =
-              repoManager.getRepository(update.repoId)?.let { repo ->
-                update.getIcon(localeList)?.getImageModel(repo, proxyConfig)
-              } as? DownloadRequest
-            AppUpdateItem(
-              repoId = update.repoId,
-              packageName = update.packageName,
-              name = update.name ?: "Unknown app",
-              installedVersionName = update.installedVersionName,
-              update = update.update,
-              whatsNew = update.update.getWhatsNew(localeList)?.trim(),
-              iconModel = PackageName(update.packageName, iconModel),
-            )
-          }
-        _updates.value = updates
-        _numUpdates.value = updates.size
-        // update 'update available' notification, if it is currently showing
-        if (notificationManager.isAppUpdatesAvailableNotificationShowing) {
-          if (updates.isEmpty()) notificationManager.cancelAppUpdatesAvailableNotification()
-          else notificationManager.showAppUpdatesAvailableNotification(notificationStates)
-        }
-
-        val issueItems =
-          apps.issues.mapNotNull { app ->
-            if (app.packageName in settingsManager.ignoredAppIssues) return@mapNotNull null
-            when (app) {
-              is AvailableAppWithIssue ->
-                AppWithIssueItem(
-                  packageName = app.app.packageName,
-                  name = app.app.getName(localeList) ?: "Unknown app",
-                  installedVersionName = app.installVersionName,
-                  installedVersionCode = app.installVersionCode,
-                  issue = app.issue,
-                  lastUpdated = app.app.lastUpdated,
-                  iconModel =
-                    PackageName(
-                      packageName = app.app.packageName,
-                      iconDownloadRequest =
-                        repoManager.getRepository(app.app.repoId)?.let {
-                          app.app.getIcon(localeList)?.getImageModel(it, proxyConfig)
-                        } as? DownloadRequest,
-                    ),
-                )
-              is UnavailableAppWithIssue ->
-                AppWithIssueItem(
-                  packageName = app.packageName,
-                  name = app.name.toString(),
-                  installedVersionName = app.installVersionName,
-                  installedVersionCode = app.installVersionCode,
-                  issue = app.issue,
-                  lastUpdated = -1,
-                  iconModel = PackageName(app.packageName, null),
-                )
-            }
-          }
-        // don't flag issues too early when we are still at first start
-        if (!settingsManager.isFirstStart) _appsWithIssues.value = issueItems
+        val updateCheckResult = dbAppChecker.getApps(packageInfoMap = packageInfoMap)
+        processAvailableUpdates(updateCheckResult.updates, localeList, proxyConfig)
+        processAppIssues(updateCheckResult.issues, localeList, proxyConfig)
       } catch (e: Exception) {
-        log.error(e) { "Error loading updates: " }
-        return@launch
+        log.error(e) { "Error loading updates" }
       }
     }
 
-  suspend fun updateAll(canAskPreApprovalNow: Boolean) {
-    val appsToUpdate = updates.value ?: updates.first() ?: return
-    // we could do more in-depth checks regarding pre-approval, but this also works
-    val preApprovalNow = canAskPreApprovalNow && appsToUpdate.size == 1
-    val concurrencyLimit = min(Runtime.getRuntime().availableProcessors(), 8)
-    val semaphore = Semaphore(concurrencyLimit)
-    // remember our own app, if it is to be updated as well
-    val updateLast = appsToUpdate.find { it.packageName == context.packageName }
-    appsToUpdate
-      .mapNotNull { update ->
-        // don't update our own app just yet
-        if (update.packageName == context.packageName) {
-          // set app to update last to Starting as well, so it doesn't seem stuck
-          val app =
-            db.getAppDao().getApp(update.repoId, update.packageName) ?: return@mapNotNull null
-          appInstallManager.setWaitingState(
-            packageName = update.packageName,
-            name = app.metadata.name.getBestLocale(LocaleListCompat.getDefault()) ?: "Unknown",
-            versionName = update.update.versionName,
-            currentVersionName = update.installedVersionName,
-            lastUpdated = update.update.added,
-          )
-          return@mapNotNull null
-        }
-        currentCoroutineContext().ensureActive()
-        // launch a new co-routine for each app to update
-        coroutineScope.launch {
-          // suspend here until we get a permit from the semaphore (there's free workers)
-          semaphore.withPermit {
-            currentCoroutineContext().ensureActive()
-            updateApp(update, preApprovalNow)
-          }
-        }
-      }
-      .joinAll()
-    currentCoroutineContext().ensureActive()
-    // now it is time to update our own app
-    updateLast?.let { updateApp(it, preApprovalNow) }
+  private fun processAvailableUpdates(
+    updates: List<UpdatableApp>,
+    localeList: LocaleListCompat,
+    proxyConfig: ProxyConfig?,
+  ) {
+    val updateItems =
+      updates.map { update -> update.toAppUpdateItem(localeList, proxyConfig, repoManager) }
+    _updates.value = updateItems
+    _numUpdates.value = updateItems.size
+    updateNotificationIfShowing(updateItems)
   }
 
-  private suspend fun updateApp(update: AppUpdateItem, canAskPreApprovalNow: Boolean) {
-    val app = db.getAppDao().getApp(update.repoId, update.packageName) ?: return
-    appInstallManager.install(
-      appMetadata = app.metadata,
-      // we know this is true, because we set this above in loadUpdates()
-      version = update.update as AppVersion,
-      currentVersionName = update.installedVersionName,
-      repo = repoManager.getRepository(update.repoId) ?: return,
-      iconModel = update.iconModel,
-      canAskPreApprovalNow = canAskPreApprovalNow,
-    )
+  private fun updateNotificationIfShowing(updates: List<AppUpdateItem>) {
+    if (!notificationManager.isAppUpdatesAvailableNotificationShowing) return
+
+    when {
+      updates.isEmpty() -> notificationManager.cancelAppUpdatesAvailableNotification()
+      else -> notificationManager.showAppUpdatesAvailableNotification(notificationStates)
+    }
+  }
+
+  private fun processAppIssues(
+    issues: List<AppWithIssue>,
+    localeList: LocaleListCompat,
+    proxyConfig: ProxyConfig?,
+  ) {
+    // Don't flag issues too early when we are still at first start
+    if (settingsManager.isFirstStart) return
+
+    _appsWithIssues.value =
+      issues
+        .filterNot { it.packageName in settingsManager.ignoredAppIssues }
+        .map { issue ->
+          when (issue) {
+            is AvailableAppWithIssue ->
+              issue.toAppWithIssueItem(localeList, proxyConfig, repoManager)
+            is UnavailableAppWithIssue -> issue.toAppWithIssueItem()
+          }
+        }
+  }
+
+  /**
+   * Apply all available updates. If [canAskPreApprovalNow] is true, the user can be asked for
+   * pre-approval for the update if needed. This should only be true if the user explicitly
+   * triggered the update process, e.g. by clicking "Update all" in the UI. For automatic updates,
+   * this should be false, to avoid interrupting the update process with pre-approval dialogs.
+   */
+  suspend fun updateAll(canAskPreApprovalNow: Boolean) {
+    val appsToUpdate = updates.value ?: updates.firstOrNull() ?: return
+    updateInstaller.updateAll(appsToUpdate, canAskPreApprovalNow)
   }
 }