address network quality review findings

- fall back to the legacy max_bitrate/stream_format keys so existing settings survive the upgrade
- keep the last network mode when there is no active network instead of reporting mobile, so a connectivity gap does not flip modes and cause spurious reloads
- skip the quality reload when the player has ended or advanced mid-read
- keep the reload collector alive across transient failures
- edit stream quality from a local dialog draft so a fast second tap is not lost
- cover the mode mapping, migration, paused reload and equal-quality no-op with tests

app/src/main/kotlin/com/subtracks/data/net/NetworkMode.kt

@@ -0,0 +1,54 @@
+package com.subtracks.data.net
+
+import android.content.Context
+import android.net.ConnectivityManager
+import android.net.Network
+import android.net.NetworkCapabilities
+import kotlinx.coroutines.channels.awaitClose
+import kotlinx.coroutines.flow.Flow
+import kotlinx.coroutines.flow.callbackFlow
+import kotlinx.coroutines.flow.distinctUntilChanged
+
+enum class NetworkMode(
+    val key: String,
+) {
+    Wifi("wifi"),
+    Mobile("mobile"),
+}
+
+internal fun networkModeFor(
+    metered: Boolean?,
+    last: NetworkMode,
+): NetworkMode =
+    when (metered) {
+        true -> NetworkMode.Mobile
+        false -> NetworkMode.Wifi
+        null -> last
+    }
+
+fun networkMode(context: Context): Flow<NetworkMode> {
+    val connectivity = context.applicationContext.getSystemService(ConnectivityManager::class.java)
+    return callbackFlow {
+        var last = NetworkMode.Wifi
+
+        fun push() {
+            val metered = connectivity?.let { if (it.activeNetwork == null) null else it.isActiveNetworkMetered }
+            last = networkModeFor(metered, last)
+            this@callbackFlow.trySend(last)
+        }
+        val callback =
+            object : ConnectivityManager.NetworkCallback() {
+                override fun onAvailable(network: Network) = push()
+
+                override fun onLost(network: Network) = push()
+
+                override fun onCapabilitiesChanged(
+                    network: Network,
+                    capabilities: NetworkCapabilities,
+                ) = push()
+            }
+        val registered = runCatching { connectivity?.registerDefaultNetworkCallback(callback) }.isSuccess
+        push()
+        awaitClose { if (registered) runCatching { connectivity?.unregisterNetworkCallback(callback) } }
+    }.distinctUntilChanged()
+}

app/src/main/kotlin/com/subtracks/data/net/NetworkMonitor.kt

@@ -1,48 +0,0 @@
-package com.subtracks.data.net
-
-import android.content.Context
-import android.net.ConnectivityManager
-import android.net.Network
-import android.net.NetworkCapabilities
-import kotlinx.coroutines.channels.awaitClose
-import kotlinx.coroutines.flow.Flow
-import kotlinx.coroutines.flow.callbackFlow
-import kotlinx.coroutines.flow.conflate
-import kotlinx.coroutines.flow.distinctUntilChanged
-
-enum class NetworkMode(
-    val key: String,
-) {
-    Wifi("wifi"),
-    Mobile("mobile"),
-}
-
-class NetworkMonitor(
-    context: Context,
-) {
-    private val connectivity = context.applicationContext.getSystemService(ConnectivityManager::class.java)
-
-    val mode: Flow<NetworkMode> =
-        callbackFlow {
-            val callback =
-                object : ConnectivityManager.NetworkCallback() {
-                    override fun onAvailable(network: Network) = push()
-
-                    override fun onLost(network: Network) = push()
-
-                    override fun onCapabilitiesChanged(
-                        network: Network,
-                        capabilities: NetworkCapabilities,
-                    ) = push()
-
-                    private fun push() {
-                        this@callbackFlow.trySend(current())
-                    }
-                }
-            val registered = runCatching { connectivity?.registerDefaultNetworkCallback(callback) }.isSuccess
-            this@callbackFlow.trySend(current())
-            awaitClose { if (registered) runCatching { connectivity?.unregisterNetworkCallback(callback) } }
-        }.distinctUntilChanged().conflate()
-
-    private fun current(): NetworkMode = if (connectivity?.isActiveNetworkMetered == true) NetworkMode.Mobile else NetworkMode.Wifi
-}

app/src/main/kotlin/com/subtracks/data/prefs/UserPreferences.kt

@@ -4,6 +4,7 @@ import android.content.Context
 import androidx.datastore.core.DataStore
 import androidx.datastore.preferences.core.Preferences
 import androidx.datastore.preferences.core.edit
+import androidx.datastore.preferences.core.intPreferencesKey
 import androidx.datastore.preferences.core.stringPreferencesKey
 import androidx.datastore.preferences.preferencesDataStore
 import com.subtracks.data.net.NetworkMode
@@ -54,7 +55,9 @@ class UserPreferences(
     }
 
     fun streamQuality(mode: NetworkMode): Flow<StreamQuality> =
-        store.data.map { prefs -> decodeStreamQuality(prefs[streamQualityKey(mode)]) }
+        store.data.map { prefs ->
+            prefs[streamQualityKey(mode)]?.let(::decodeStreamQuality) ?: legacyStreamQuality(prefs)
+        }
 
     suspend fun setStreamQuality(
         mode: NetworkMode,
@@ -71,14 +74,20 @@ class UserPreferences(
 
     private fun encode(quality: StreamQuality) = "${quality.maxBitrate}|${quality.format.orEmpty()}"
 
-    private fun decodeStreamQuality(stored: String?): StreamQuality {
+    private fun decodeStreamQuality(stored: String): StreamQuality {
-        val parts = stored?.split("|") ?: return StreamQuality()
+        val parts = stored.split("|")
         return StreamQuality(
             maxBitrate = parts.getOrNull(0)?.toIntOrNull() ?: 0,
             format = parts.getOrNull(1)?.takeIf { it.isNotEmpty() },
         )
     }
 
+    private fun legacyStreamQuality(prefs: Preferences): StreamQuality =
+        StreamQuality(
+            maxBitrate = prefs[LEGACY_BITRATE] ?: 0,
+            format = prefs[LEGACY_FORMAT]?.takeIf { it.isNotEmpty() },
+        )
+
     private fun decode(
         stored: String?,
         defaultSort: String,
@@ -91,6 +100,11 @@ class UserPreferences(
             starred = StarredFilter.entries.getOrElse(parts[2].toIntOrNull() ?: 0) { StarredFilter.Any },
         )
     }
+
+    private companion object {
+        val LEGACY_BITRATE = intPreferencesKey("max_bitrate")
+        val LEGACY_FORMAT = stringPreferencesKey("stream_format")
+    }
 }
 
 fun createUserPreferences(context: Context): UserPreferences = UserPreferences(context.preferences)

app/src/main/kotlin/com/subtracks/di/AppModule.kt

@@ -5,7 +5,7 @@ import android.os.Handler
 import android.os.Looper
 import android.widget.Toast
 import com.subtracks.data.db.createAndroidDatabase
-import com.subtracks.data.net.NetworkMonitor
+import com.subtracks.data.net.networkMode
 import com.subtracks.data.prefs.createUserPreferences
 import com.subtracks.data.repo.ArtworkSeedRepository
 import com.subtracks.data.repo.ArtworkSeedStore
@@ -39,8 +39,7 @@ fun appModule(
     single { createAndroidDatabase(context) }
     single { http }
     single { createUserPreferences(context) }
-    single { NetworkMonitor(context.applicationContext) }
+    single { SourceRepository(get(), get(), get(), networkMode = networkMode(context.applicationContext), showMessage = toast) }
-    single { SourceRepository(get(), get(), get(), networkMode = get<NetworkMonitor>().mode, showMessage = toast) }
     single { LibraryRepository(get(), get(), toast) }
     single<ArtworkSeedStore> { ArtworkSeedRepository(get()) }
     single { SyncManager(get(), get(), get()) }

app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt

@@ -7,6 +7,7 @@ import com.subtracks.data.repo.QueueRepository
 import com.subtracks.data.repo.QueueSnapshot
 import com.subtracks.data.repo.QueueWindowItem
 import com.subtracks.data.repo.SourceRepository
+import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.CoroutineDispatcher
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
@@ -135,7 +136,14 @@ class PlaybackController(
             }
         }
         scope.launch {
-            sourceRepository.quality.drop(1).collect { reloadQuality() }
+            sourceRepository.quality.drop(1).collect {
+                try {
+                    reloadQuality()
+                } catch (e: CancellationException) {
+                    throw e
+                } catch (_: Exception) {
+                }
+            }
         }
     }
 
@@ -710,8 +718,12 @@ class PlaybackController(
             if (snapshot.size == 0L) return@withLock
             val sourceId = queueSourceId
             if (sourceId == null || sourceRepository.activeSourceIdOnce() != sourceId) return@withLock
-            val position = currentPosition() ?: return@withLock
+            if (player.isEnded) return@withLock
-            loadWindow(position, autoplay = player.playWhenReady && !player.isEnded, startPositionMs = player.currentPositionMs)
+            val index = player.currentIndex
+            val position = (windowStart + index).coerceIn(0, snapshot.size - 1)
+            val positionMs = player.currentPositionMs
+            if (player.currentIndex != index) return@withLock
+            loadWindow(position, autoplay = player.playWhenReady, startPositionMs = positionMs)
         }
 
     private fun jumpTo(target: Long) {

app/src/main/kotlin/com/subtracks/ui/settings/SettingsScreen.kt

@@ -204,6 +204,7 @@ private fun QualityDialog(
     onSelect: (StreamQuality) -> Unit,
     onDismiss: () -> Unit,
 ) {
+    var draft by remember(quality) { mutableStateOf(quality) }
     AlertDialog(
         onDismissRequest = onDismiss,
         title = { Text(title) },
@@ -212,15 +213,21 @@ private fun QualityDialog(
                 ChoiceGroup(
                     header = "Maximum bitrate",
                     options = bitrateOptions.map { it to bitrateLabel(it) },
-                    selected = quality.maxBitrate,
+                    selected = draft.maxBitrate,
-                    onSelect = { onSelect(quality.copy(maxBitrate = it)) },
+                    onSelect = {
+                        draft = draft.copy(maxBitrate = it)
+                        onSelect(draft)
+                    },
                 )
                 Spacer(Modifier.height(16.dp))
                 ChoiceGroup(
                     header = "Preferred format",
                     options = streamFormats.map { it to (it ?: "Use server default") },
-                    selected = quality.format,
+                    selected = draft.format,
-                    onSelect = { onSelect(quality.copy(format = it)) },
+                    onSelect = {
+                        draft = draft.copy(format = it)
+                        onSelect(draft)
+                    },
                 )
             }
         },

app/src/test/kotlin/com/subtracks/data/net/NetworkModeTest.kt

@@ -0,0 +1,18 @@
+package com.subtracks.data.net
+
+import org.junit.Assert.assertEquals
+import org.junit.Test
+
+class NetworkModeTest {
+    @Test
+    fun meteredIsMobileAndUnmeteredIsWifi() {
+        assertEquals(NetworkMode.Mobile, networkModeFor(true, NetworkMode.Wifi))
+        assertEquals(NetworkMode.Wifi, networkModeFor(false, NetworkMode.Mobile))
+    }
+
+    @Test
+    fun noActiveNetworkKeepsTheLastMode() {
+        assertEquals(NetworkMode.Wifi, networkModeFor(null, NetworkMode.Wifi))
+        assertEquals(NetworkMode.Mobile, networkModeFor(null, NetworkMode.Mobile))
+    }
+}

app/src/test/kotlin/com/subtracks/data/prefs/UserPreferencesTest.kt

@@ -1,6 +1,9 @@
 package com.subtracks.data.prefs
 
 import androidx.datastore.preferences.core.PreferenceDataStoreFactory
+import androidx.datastore.preferences.core.edit
+import androidx.datastore.preferences.core.intPreferencesKey
+import androidx.datastore.preferences.core.stringPreferencesKey
 import androidx.test.ext.junit.runners.AndroidJUnit4
 import com.subtracks.data.net.NetworkMode
 import kotlinx.coroutines.ExperimentalCoroutinesApi
@@ -55,6 +58,28 @@ class UserPreferencesTest {
             assertEquals(StreamQuality(320, null), prefs.streamQuality(NetworkMode.Wifi).first())
             assertEquals(StreamQuality(96, "opus"), prefs.streamQuality(NetworkMode.Mobile).first())
 
+            file.delete()
+        }
+
+    @Test
+    fun streamQualityFallsBackToTheLegacySingleValue() =
+        runTest {
+            val file = File.createTempFile("user-prefs", ".preferences_pb").apply { delete() }
+            val store = PreferenceDataStoreFactory.create(scope = backgroundScope) { file }
+            store.edit { prefs ->
+                prefs[intPreferencesKey("max_bitrate")] = 192
+                prefs[stringPreferencesKey("stream_format")] = "mp3"
+            }
+            val prefs = UserPreferences(store)
+
+            assertEquals(StreamQuality(192, "mp3"), prefs.streamQuality(NetworkMode.Wifi).first())
+            assertEquals(StreamQuality(192, "mp3"), prefs.streamQuality(NetworkMode.Mobile).first())
+
+            prefs.setStreamQuality(NetworkMode.Mobile, StreamQuality(64, null))
+
+            assertEquals(StreamQuality(192, "mp3"), prefs.streamQuality(NetworkMode.Wifi).first())
+            assertEquals(StreamQuality(64, null), prefs.streamQuality(NetworkMode.Mobile).first())
+
             file.delete()
         }
 }

app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt

@@ -20,6 +20,7 @@ import com.subtracks.data.repo.SourceRepository
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.asCoroutineDispatcher
 import kotlinx.coroutines.delay
+import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.runBlocking
 import okhttp3.OkHttpClient
@@ -39,6 +40,7 @@ class PlaybackControllerTest {
 
     private lateinit var db: SubtracksDatabase
     private lateinit var prefs: UserPreferences
+    private lateinit var networkMode: MutableStateFlow<NetworkMode>
     private lateinit var sources: SourceRepository
     private lateinit var queues: QueueRepository
     private lateinit var handle: FakePlayerHandle
@@ -53,7 +55,8 @@ class PlaybackControllerTest {
                 .setDriver(BundledSQLiteDriver())
                 .build()
         prefs = fakeUserPreferences()
-        sources = SourceRepository(db, OkHttpClient(), prefs)
+        networkMode = MutableStateFlow(NetworkMode.Wifi)
+        sources = SourceRepository(db, OkHttpClient(), prefs, networkMode = networkMode)
         queues = QueueRepository(db)
         handle = FakePlayerHandle()
         controller = PlaybackController(sources, queues, FakePlayerConnection(handle), dispatcher = dispatcher)
@@ -1039,6 +1042,69 @@ class PlaybackControllerTest {
         assertEquals(listOf("prepare", "play"), handle.operations.takeLast(2))
     }
 
+    @Test
+    fun changingStreamQualityWhilePausedReloadsWithoutResuming() {
+        seedAlbum(60, sourceId = 1)
+        runBlocking { prefs.setStreamQuality(NetworkMode.Wifi, StreamQuality(64, null)) }
+        await { sources.quality.value == StreamQuality(64, null) }
+
+        controller.playAlbum(1, "al1", 10)
+        await { handle.playWhenReady }
+        controller.togglePlayPause()
+        await { !handle.playWhenReady }
+        handle.positionMs = 33_000L
+        val before = handle.operations.size
+        val windows = handle.operations.count { it.startsWith("setWindow") }
+
+        runBlocking { prefs.setStreamQuality(NetworkMode.Wifi, StreamQuality(128, "opus")) }
+        await { handle.operations.count { it.startsWith("setWindow") } > windows }
+
+        assertEquals(33_000L, handle.positionMs)
+        assertFalse(handle.playWhenReady)
+        assertTrue(handle.operations.drop(before).none { it.startsWith("prepare") || it.startsWith("play") })
+    }
+
+    @Test
+    fun switchingToTheOtherModeReloadsWithItsQuality() {
+        seedAlbum(60, sourceId = 1)
+        runBlocking {
+            prefs.setStreamQuality(NetworkMode.Wifi, StreamQuality(320, null))
+            prefs.setStreamQuality(NetworkMode.Mobile, StreamQuality(96, "opus"))
+        }
+        await { sources.quality.value == StreamQuality(320, null) }
+
+        controller.playAlbum(1, "al1", 10)
+        await { handle.operations.contains("play") }
+        handle.positionMs = 12_000L
+        val windows = handle.operations.count { it.startsWith("setWindow") }
+
+        networkMode.value = NetworkMode.Mobile
+        await { sources.quality.value == StreamQuality(96, "opus") }
+        await { handle.operations.count { it.startsWith("setWindow") } > windows }
+
+        assertEquals("s11", handle.currentItem?.id)
+        assertEquals(12_000L, handle.positionMs)
+    }
+
+    @Test
+    fun switchingToTheOtherModeWithEqualQualityDoesNotReload() {
+        seedAlbum(60, sourceId = 1)
+        runBlocking {
+            prefs.setStreamQuality(NetworkMode.Wifi, StreamQuality(96, "opus"))
+            prefs.setStreamQuality(NetworkMode.Mobile, StreamQuality(96, "opus"))
+        }
+        await { sources.quality.value == StreamQuality(96, "opus") }
+
+        controller.playAlbum(1, "al1", 10)
+        await { handle.operations.contains("play") }
+        val windows = handle.operations.count { it.startsWith("setWindow") }
+
+        networkMode.value = NetworkMode.Mobile
+        runBlocking { delay(300) }
+
+        assertEquals(windows, handle.operations.count { it.startsWith("setWindow") })
+    }
+
     private fun seedAlbum(
         count: Int,
         sourceId: Long,