Let DownloadJob own the downloader lifecycle

Assisted-by: Claude:claude-opus-5-5

app/src/main/java/eu/kanade/tachiyomi/data/download/DownloadJob.kt

@@ -13,31 +13,27 @@ import androidx.work.WorkManager
 import androidx.work.WorkerParameters
 import dev.zacsweers.metro.Inject
 import eu.kanade.tachiyomi.data.notification.Notifications
-import eu.kanade.tachiyomi.util.system.NetworkState
 import eu.kanade.tachiyomi.util.system.activeNetworkState
-import eu.kanade.tachiyomi.util.system.networkStateFlow
 import eu.kanade.tachiyomi.util.system.notificationBuilder
 import eu.kanade.tachiyomi.util.system.setForegroundSafely
-import kotlinx.coroutines.coroutineScope
+import kotlinx.coroutines.delay
 import kotlinx.coroutines.flow.Flow
-import kotlinx.coroutines.flow.combineTransform
-import kotlinx.coroutines.flow.launchIn
 import kotlinx.coroutines.flow.map
-import kotlinx.coroutines.flow.onEach
 import mihon.app.di.AppGraph
 import mihon.core.metro.metroGraph
 import tachiyomi.domain.download.service.DownloadPreferences
 import tachiyomi.i18n.R
+import kotlin.time.Duration.Companion.seconds
 
 /**
- * This worker is used to manage the downloader. The system can decide to stop the worker, in
- * which case the downloader is also stopped. It's also stopped while there's no network available.
+ * This worker owns the lifecycle of the downloader: it starts the downloader and stops it when the
+ * worker is stopped by the system or there's no suitable network available.
  */
 class DownloadJob(context: Context, workerParams: WorkerParameters) : CoroutineWorker(context, workerParams) {
 
     private val graph: AppGraph = context.metroGraph()
 
-    @Inject private lateinit var downloadManager: DownloadManager
+    @Inject private lateinit var downloader: Downloader
 
     @Inject private lateinit var downloadPreferences: DownloadPreferences
 
@@ -62,48 +58,35 @@ class DownloadJob(context: Context, workerParams: WorkerParameters) : CoroutineW
     }
 
     override suspend fun doWork(): Result {
-        var networkCheck = checkNetworkState(
-            applicationContext.activeNetworkState(),
-            downloadPreferences.downloadOnlyOverWifi.get(),
-        )
-        var active = networkCheck && downloadManager.downloaderStart()
-
-        if (!active) {
+        var networkIssue = networkIssue()
+        if (networkIssue != null) {
+            downloader.stop(networkIssue)
             return Result.failure()
         }
 
-        setForegroundSafely()
+        if (!downloader.start()) return Result.failure()
 
-        coroutineScope {
-            combineTransform(
-                applicationContext.networkStateFlow(),
-                downloadPreferences.downloadOnlyOverWifi.changes(),
-                transform = { a, b -> emit(checkNetworkState(a, b)) },
-            )
-                .onEach { networkCheck = it }
-                .launchIn(this)
-        }
+        try {
+            setForegroundSafely()
 
-        // Keep the worker running when needed
-        while (active) {
-            active = !isStopped && downloadManager.isRunning && networkCheck
+            while (networkIssue == null && downloader.isRunning) {
+                delay(1.seconds)
+                networkIssue = networkIssue()
+            }
+        } finally {
+            if (downloader.isRunning && (networkIssue != null || isStopped)) downloader.stop(networkIssue)
         }
 
         return Result.success()
     }
 
-    private fun checkNetworkState(state: NetworkState, requireWifi: Boolean): Boolean {
-        return if (state.isOnline) {
-            val noWifi = requireWifi && !state.isWifi
-            if (noWifi) {
-                downloadManager.downloaderStop(
-                    applicationContext.getString(R.string.download_notifier_text_only_wifi),
-                )
-            }
-            !noWifi
-        } else {
-            downloadManager.downloaderStop(applicationContext.getString(R.string.download_notifier_no_network))
-            false
+    private fun networkIssue(): String? {
+        val state = applicationContext.activeNetworkState()
+        return when {
+            !state.isOnline -> applicationContext.getString(R.string.download_notifier_no_network)
+            downloadPreferences.downloadOnlyOverWifi.get() && !state.isWifi ->
+                applicationContext.getString(R.string.download_notifier_text_only_wifi)
+            else -> null
         }
     }
 
@@ -118,18 +101,6 @@ class DownloadJob(context: Context, workerParams: WorkerParameters) : CoroutineW
                 .enqueueUniqueWork(TAG, ExistingWorkPolicy.REPLACE, request)
         }
 
-        fun stop(context: Context) {
-            WorkManager.getInstance(context)
-                .cancelUniqueWork(TAG)
-        }
-
-        fun isRunning(context: Context): Boolean {
-            return WorkManager.getInstance(context)
-                .getWorkInfosForUniqueWork(TAG)
-                .get()
-                .let { list -> list.count { it.state == WorkInfo.State.RUNNING } == 1 }
-        }
-
         fun isRunningFlow(context: Context): Flow<Boolean> {
             return WorkManager.getInstance(context)
                 .getWorkInfosForUniqueWorkLiveData(TAG)

app/src/main/java/eu/kanade/tachiyomi/data/download/DownloadManager.kt

@@ -52,30 +52,19 @@ class DownloadManager(
     private val pendingDeleter: DownloadPendingDeleter,
 ) {
 
-    val isRunning: Boolean
-        get() = downloader.isRunning
-
     val queueState
         get() = downloader.queueState
 
-    // For use by DownloadService only
-    fun downloaderStart() = downloader.start()
-    fun downloaderStop(reason: String? = null) = downloader.stop(reason)
-
     val isDownloaderRunning
         get() = DownloadJob.isRunningFlow(context)
 
     /**
-     * Tells the downloader to begin downloads.
+     * Starts the download worker, which runs the downloader.
      */
     fun startDownloads() {
         if (downloader.isRunning) return
 
-        if (DownloadJob.isRunning(context)) {
-            downloader.start()
-        } else {
-            DownloadJob.start(context)
-        }
+        DownloadJob.start(context)
     }
 
     /**
@@ -141,7 +130,9 @@ class DownloadManager(
      * @param autoStart whether to start the downloader after enqueing the chapters.
      */
     suspend fun downloadChapters(manga: Manga, chapters: List<Chapter>, autoStart: Boolean = true) {
-        downloader.queueChapters(manga, chapters, autoStart)
+        if (downloader.queueChapters(manga, chapters, autoStart)) {
+            startDownloads()
+        }
     }
 
     /**
@@ -155,7 +146,7 @@ class DownloadManager(
             addAll(0, downloads)
             reorderQueue(this)
         }
-        if (!DownloadJob.isRunning(context)) startDownloads()
+        startDownloads()
     }
 
     /**

app/src/main/java/eu/kanade/tachiyomi/data/download/Downloader.kt

@@ -154,8 +154,6 @@ class Downloader(
         }
 
         isPaused = false
-
-        DownloadJob.stop(context)
     }
 
     /**
@@ -264,11 +262,12 @@ class Downloader(
      * @param manga the manga of the chapters to download.
      * @param chapters the list of chapters to download.
      * @param autoStart whether to start the downloader after enqueing the chapters.
+     * @return true if the downloader should be started.
      */
-    suspend fun queueChapters(manga: Manga, chapters: List<Chapter>, autoStart: Boolean) {
-        if (chapters.isEmpty()) return
+    suspend fun queueChapters(manga: Manga, chapters: List<Chapter>, autoStart: Boolean): Boolean {
+        if (chapters.isEmpty()) return false
 
-        val source = sourceManager.get(manga.source) as? HttpSource ?: return
+        val source = sourceManager.get(manga.source) as? HttpSource ?: return false
         val wasEmpty = queueState.value.isEmpty()
         val chaptersToQueue = chapters.asSequence()
             // Filter out those already downloaded.
@@ -305,9 +304,10 @@ class Downloader(
                         NotificationHandler.openUrl(context, LibraryUpdateNotifier.HELP_WARNING_URL),
                     )
                 }
-                DownloadJob.start(context)
+                return true
             }
         }
+        return false
     }
 
     /**