Address PR Feedback

Author: jmandel

app/src/main/java/org/schabi/newpipe/download/DownloadDialog.java

@@ -70,6 +70,7 @@
 import org.schabi.newpipe.util.SimpleOnSeekBarChangeListener;
 import org.schabi.newpipe.util.StreamItemAdapter;
 import org.schabi.newpipe.util.StreamItemAdapter.StreamInfoWrapper;
+import org.schabi.newpipe.util.StreamLabelUtils;
 import org.schabi.newpipe.util.ThemeHelper;
 
 import java.io.File;
@@ -1132,7 +1133,8 @@ private void continueSelectedDownload(@NonNull final StoredFileHelper storage) {
             );
         }
 
-        final String qualityLabel = buildQualityLabel(selectedStream);
+        final String qualityLabel =
+                StreamLabelUtils.getQualityLabel(requireContext(), selectedStream);
 
         DownloadManagerService.startMission(
                 context,
@@ -1155,24 +1157,4 @@ private void continueSelectedDownload(@NonNull final StoredFileHelper storage) {
 
         dismiss();
     }
-
-    @Nullable
-    private String buildQualityLabel(@NonNull final Stream stream) {
-        if (stream instanceof VideoStream) {
-            return ((VideoStream) stream).getResolution();
-        } else if (stream instanceof AudioStream) {
-            final int bitrate = ((AudioStream) stream).getAverageBitrate();
-            return bitrate > 0 ? bitrate + "kbps" : null;
-        } else if (stream instanceof SubtitlesStream) {
-            final SubtitlesStream subtitlesStream = (SubtitlesStream) stream;
-            final String language = subtitlesStream.getDisplayLanguageName();
-            if (subtitlesStream.isAutoGenerated()) {
-                return language + " (" + getString(R.string.caption_auto_generated) + ")";
-            }
-            return language;
-        }
-
-        final MediaFormat format = stream.getFormat();
-        return format != null ? format.getSuffix() : null;
-    }
 }

app/src/main/java/org/schabi/newpipe/download/DownloadStatusRepository.kt

@@ -23,53 +23,77 @@ import us.shandian.giga.service.MissionState
 import kotlin.coroutines.resume
 import kotlin.coroutines.resumeWithException
 
-sealed interface DownloadStatus {
-    data object None : DownloadStatus
-    data class InProgress(val running: Boolean) : DownloadStatus
-    data class Completed(val info: CompletedDownload) : DownloadStatus
+enum class DownloadStage {
+    Pending,
+    Running,
+    Finished
 }
 
-data class CompletedDownload(
+data class DownloadHandle(
+    val serviceId: Int,
+    val url: String,
+    val streamUid: Long,
+    val storageUri: Uri?,
+    val timestamp: Long,
+    val kind: Char
+)
+
+data class DownloadEntry(
+    val handle: DownloadHandle,
     val displayName: String?,
     val qualityLabel: String?,
     val mimeType: String?,
     val fileUri: Uri?,
     val parentUri: Uri?,
-    val fileAvailable: Boolean
+    val fileAvailable: Boolean,
+    val stage: DownloadStage
 )
 
 object DownloadStatusRepository {
 
-    fun observe(context: Context, serviceId: Int, url: String): Flow<DownloadStatus> = callbackFlow {
+    /**
+     * Keeps a one-off binding to [DownloadManagerService] alive for as long as the caller stays
+     * subscribed. We prime the channel with the latest persisted snapshot and then forward every
+     * mission event emitted by the service-bound handler. Once the consumer cancels the flow we
+     * make sure to unregister the handler and unbind the service to avoid leaking the connection.
+     */
+    fun observe(context: Context, serviceId: Int, url: String): Flow<List<DownloadEntry>> = callbackFlow {
         if (serviceId < 0 || url.isBlank()) {
-            trySend(DownloadStatus.None)
+            trySend(emptyList())
             close()
             return@callbackFlow
         }
 
         val appContext = context.applicationContext
         val intent = Intent(appContext, DownloadManagerService::class.java)
         appContext.startService(intent)
+        // The download manager service only notifies listeners while a client is bound, so the flow
+        // keeps a foreground-style binding alive for its entire lifetime. Holding on to
+        // applicationContext avoids leaking short-lived UI contexts.
         var binder: DownloadManagerBinder? = null
         var registeredCallback: Handler.Callback? = null
 
         val connection = object : ServiceConnection {
             override fun onServiceConnected(name: ComponentName?, service: IBinder?) {
                 val downloadBinder = service as? DownloadManagerBinder
                 if (downloadBinder == null) {
-                    trySend(DownloadStatus.None)
+                    trySend(emptyList())
                     appContext.unbindService(this)
                     close()
                     return
                 }
                 binder = downloadBinder
-                trySend(downloadBinder.getDownloadStatus(serviceId, url, false).toDownloadStatus())
+                // First delivery: snapshot persisted on disk so the UI paints immediately even
+                // before the service emits new events.
+                trySend(downloadBinder.getDownloadStatuses(serviceId, url, true).toDownloadEntries())
 
                 val callback = Handler.Callback { message: Message ->
                     val mission = message.obj
                     if (mission.matches(serviceId, url)) {
-                        val snapshot = downloadBinder.getDownloadStatus(serviceId, url, false)
-                        trySend(snapshot.toDownloadStatus())
+                        // Each mission event carries opaque state, so we fetch a fresh snapshot to
+                        // guarantee consistent entries while the download progresses or finishes.
+                        val snapshots = downloadBinder.getDownloadStatuses(serviceId, url, false)
+                        trySend(snapshots.toDownloadEntries())
                     }
                     false
                 }
@@ -80,48 +104,57 @@ object DownloadStatusRepository {
             override fun onServiceDisconnected(name: ComponentName?) {
                 registeredCallback?.let { callback -> binder?.removeMissionEventListener(callback) }
                 binder = null
-                trySend(DownloadStatus.None)
+                trySend(emptyList())
             }
         }
 
         val bound = appContext.bindService(intent, connection, Context.BIND_AUTO_CREATE)
         if (!bound) {
-            trySend(DownloadStatus.None)
+            trySend(emptyList())
             close()
             return@callbackFlow
         }
 
         awaitClose {
+            // When the collector disappears we remove listeners and unbind immediately to avoid
+            // holding the service forever; the service will rebind on the next subscription.
             registeredCallback?.let { callback -> binder?.removeMissionEventListener(callback) }
             runCatching { appContext.unbindService(connection) }
         }
     }
 
-    suspend fun refresh(context: Context, serviceId: Int, url: String): DownloadStatus {
-        if (serviceId < 0 || url.isBlank()) return DownloadStatus.None
+    suspend fun refresh(context: Context, serviceId: Int, url: String): List<DownloadEntry> {
+        if (serviceId < 0 || url.isBlank()) return emptyList()
         return withBinder(context) { binder ->
-            binder.getDownloadStatus(serviceId, url, true).toDownloadStatus()
+            binder.getDownloadStatuses(serviceId, url, true).toDownloadEntries()
         }
     }
 
-    suspend fun deleteFile(context: Context, serviceId: Int, url: String): Boolean {
-        if (serviceId < 0 || url.isBlank()) return false
+    suspend fun deleteFile(context: Context, handle: DownloadHandle): Boolean {
+        if (handle.serviceId < 0 || handle.url.isBlank()) return false
         return withBinder(context) { binder ->
-            binder.deleteFinishedMission(serviceId, url, true)
+            binder.deleteFinishedMission(handle.serviceId, handle.url, handle.storageUri, handle.timestamp, true)
         }
     }
 
-    suspend fun removeLink(context: Context, serviceId: Int, url: String): Boolean {
-        if (serviceId < 0 || url.isBlank()) return false
+    suspend fun removeLink(context: Context, handle: DownloadHandle): Boolean {
+        if (handle.serviceId < 0 || handle.url.isBlank()) return false
         return withBinder(context) { binder ->
-            binder.deleteFinishedMission(serviceId, url, false)
+            binder.deleteFinishedMission(handle.serviceId, handle.url, handle.storageUri, handle.timestamp, false)
         }
     }
 
+    /**
+     * Helper that briefly binds to [DownloadManagerService], executes [block] against its binder
+     * and tears everything down in one place. All callers should use this to prevent scattering
+     * ad-hoc bind/unbind logic across the codebase.
+     */
     private suspend fun <T> withBinder(context: Context, block: (DownloadManagerBinder) -> T): T {
         val appContext = context.applicationContext
         val intent = Intent(appContext, DownloadManagerService::class.java)
         appContext.startService(intent)
+        // The direct call path still needs the service running long enough to complete the
+        // binder transaction, so we explicitly start it before establishing the short-lived bind.
         return suspendCancellableCoroutine { continuation ->
             val connection = object : ServiceConnection {
                 override fun onServiceConnected(name: ComponentName?, service: IBinder?) {
@@ -176,32 +209,83 @@ object DownloadStatusRepository {
 
     @VisibleForTesting
     @MainThread
-    internal fun DownloadManager.DownloadStatusSnapshot?.toDownloadStatus(): DownloadStatus {
-        if (this == null || state == MissionState.None) {
-            return DownloadStatus.None
+    internal fun List<DownloadManager.DownloadStatusSnapshot>.toDownloadEntries(): List<DownloadEntry> {
+        return buildList {
+            for (snapshot in this@toDownloadEntries) {
+                snapshot.toDownloadEntry()?.let { add(it) }
+            }
         }
-        return when (state) {
-            MissionState.Pending, MissionState.PendingRunning ->
-                DownloadStatus.InProgress(state == MissionState.PendingRunning)
-            MissionState.Finished -> {
-                val mission = finishedMission
-                if (mission == null) {
-                    DownloadStatus.None
-                } else {
-                    val storage = mission.storage
-                    val hasStorage = storage != null && !storage.isInvalid()
-                    val info = CompletedDownload(
-                        displayName = storage?.getName(),
-                        qualityLabel = mission.qualityLabel,
-                        mimeType = if (hasStorage) storage!!.getType() else null,
-                        fileUri = if (hasStorage) storage!!.getUri() else null,
-                        parentUri = if (hasStorage) storage!!.getParentUri() else null,
-                        fileAvailable = fileExists && hasStorage
-                    )
-                    DownloadStatus.Completed(info)
-                }
+    }
+
+    @VisibleForTesting
+    @MainThread
+    internal fun DownloadManager.DownloadStatusSnapshot.toDownloadEntry(): DownloadEntry? {
+        val stage = when (state) {
+            MissionState.Pending -> DownloadStage.Pending
+            MissionState.PendingRunning -> DownloadStage.Running
+            MissionState.Finished -> DownloadStage.Finished
+            else -> return null
+        }
+
+        val (metadata, storage) = when (stage) {
+            DownloadStage.Finished -> finishedMission?.let {
+                MissionMetadata(
+                    serviceId = it.serviceId,
+                    url = it.source,
+                    streamUid = it.streamUid,
+                    timestamp = it.timestamp,
+                    kind = it.kind,
+                    qualityLabel = it.qualityLabel
+                ) to it.storage
             }
-            else -> DownloadStatus.None
+            else -> pendingMission?.let {
+                MissionMetadata(
+                    serviceId = it.serviceId,
+                    url = it.source,
+                    streamUid = it.streamUid,
+                    timestamp = it.timestamp,
+                    kind = it.kind,
+                    qualityLabel = it.qualityLabel
+                ) to it.storage
+            }
+        } ?: return null
+
+        val hasStorage = storage != null && !storage.isInvalid()
+        val fileUri = storage?.getUri()
+        val parentUri = storage?.getParentUri()
+
+        val handle = DownloadHandle(
+            serviceId = metadata.serviceId,
+            url = metadata.url ?: "",
+            streamUid = metadata.streamUid,
+            storageUri = fileUri,
+            timestamp = metadata.timestamp,
+            kind = metadata.kind
+        )
+
+        val fileAvailable = when (stage) {
+            DownloadStage.Finished -> hasStorage && fileExists
+            DownloadStage.Pending, DownloadStage.Running -> false
         }
+
+        return DownloadEntry(
+            handle = handle,
+            displayName = storage?.getName(),
+            qualityLabel = metadata.qualityLabel,
+            mimeType = if (hasStorage) storage.getType() else null,
+            fileUri = fileUri,
+            parentUri = parentUri,
+            fileAvailable = fileAvailable,
+            stage = stage
+        )
     }
+
+    private data class MissionMetadata(
+        val serviceId: Int,
+        val url: String?,
+        val streamUid: Long,
+        val timestamp: Long,
+        val kind: Char,
+        val qualityLabel: String?
+    )
 }