diff --git a/app/src/main/kotlin/ca/ksamad/musicremote/mpd/MpdClient.kt b/app/src/main/kotlin/ca/ksamad/musicremote/mpd/MpdClient.kt index a19a3ec..880fc92 100644 --- a/app/src/main/kotlin/ca/ksamad/musicremote/mpd/MpdClient.kt +++ b/app/src/main/kotlin/ca/ksamad/musicremote/mpd/MpdClient.kt @@ -1,7 +1,9 @@ package ca.ksamad.musicremote.mpd +import ca.ksamad.musicremote.mpd.model.MpdAlbum import ca.ksamad.musicremote.mpd.model.MpdSong import ca.ksamad.musicremote.mpd.model.MpdStatus +import ca.ksamad.musicremote.mpd.model.PlayerState import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers @@ -9,6 +11,7 @@ import kotlinx.coroutines.Job import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.cancel import kotlinx.coroutines.cancelAndJoin +import kotlinx.coroutines.delay import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.isActive @@ -28,15 +31,15 @@ import java.io.IOException * can push change notifications. Each `changed:` event triggers a refresh on * the command connection, which flows out through [status]/[currentSong]. * - * Why two connections: `idle` blocks its connection indefinitely, so it can't - * also carry commands. Splitting them lets the UI stay live (idle) while still - * issuing actions (command) without racing on one socket. + * **Resilience.** A quiet command connection would otherwise be reaped by MPD's + * `connection_timeout`, so a periodic [keepalive] `ping` keeps it warm and + * detects death early. Any transport failure triggers a transparent + * [reconnect][triggerReconnect] (staying on the player, not bouncing the user to + * the connect screen); only repeated failures surface as + * [MpdConnectionState.Error]. Server-side `ACK` errors are *not* treated as + * connection failures. * - * All blocking socket I/O is dispatched to [ioDispatcher]. Observe the exposed - * [StateFlow]s from the UI; call [connect]/[disconnect] to manage the link. - * - * A [connectionFactory] is injectable so tests can supply fake transports; it - * defaults to a real TCP [MpdConnection.connect]. + * A [connectionFactory] is injectable so tests can supply fake transports. */ class MpdClient( private val ioDispatcher: CoroutineDispatcher = Dispatchers.IO, @@ -49,11 +52,15 @@ class MpdClient( private var commandConn: MpdConnection? = null private var idleConn: MpdConnection? = null private var idleJob: Job? = null + private var keepaliveJob: Job? = null + private var reconnectJob: Job? = null - /** - * True while a [disconnect] is in progress, so the idle loop can tell an - * intentional socket close apart from a real connection drop. - */ + // Remembered so a background reconnect can reopen without UI involvement. + private var host: String? = null + private var port: Int = MpdConnection.DEFAULT_PORT + private var password: String? = null + + /** True while an intentional [disconnect]/[shutdown] is in progress. */ @Volatile private var shuttingDown = false @@ -70,46 +77,55 @@ class MpdClient( /** * Open both connections, authenticate, prime the initial state, and start - * the idle loop. Safe to await; failures land in [connectionState] as + * the idle + keepalive loops. On failure the state becomes * [MpdConnectionState.Error] and the call throws. */ suspend fun connect(host: String, port: Int = MpdConnection.DEFAULT_PORT, password: String? = null) { disconnect() // ensure a clean slate if re-connecting shuttingDown = false + this.host = host + this.port = port + this.password = password _connectionState.value = MpdConnectionState.Connecting try { - withContext(ioDispatcher) { - // Command connection: a finite read timeout so a wedged server - // surfaces as an error instead of hanging a UI action forever. - val command = connectionFactory(host, port, COMMAND_READ_TIMEOUT_MS) - // Idle connection: no read timeout — it must park indefinitely. - val idle = connectionFactory(host, port, 0) - if (password != null) { - command.execute(MpdCommands.password(password)) - idle.execute(MpdCommands.password(password)) - } - commandConn = command - idleConn = idle - } + openConnections() refresh() _connectionState.value = MpdConnectionState.Connected startIdleLoop() + startKeepalive() } catch (e: IOException) { failAndClose(e.message ?: "connection failed") throw e } } - /** Tear down the idle loop and both connections; returns to [MpdConnectionState.Disconnected]. */ + /** (Re)open and authenticate both sockets using the remembered credentials. */ + private suspend fun openConnections() { + withContext(ioDispatcher) { + val h = host ?: throw MpdConnectionException("no host") + // Command connection: finite read timeout so a wedged server surfaces + // as an error instead of hanging a UI action forever. + val command = connectionFactory(h, port, COMMAND_READ_TIMEOUT_MS) + // Idle connection: no read timeout — it must park indefinitely. + val idle = connectionFactory(h, port, 0) + password?.let { pw -> + command.execute(MpdCommands.password(pw)) + idle.execute(MpdCommands.password(pw)) + } + commandConn = command + idleConn = idle + } + } + + /** Tear everything down; returns to [MpdConnectionState.Disconnected]. */ suspend fun disconnect() { shuttingDown = true + stopBackgroundJobs() // Closing the idle socket unblocks the loop's parked `idle` read. withContext(ioDispatcher) { idleConn?.close() commandConn?.close() } - idleJob?.cancelAndJoin() - idleJob = null idleConn = null commandConn = null if (_connectionState.value !is MpdConnectionState.Error) { @@ -118,22 +134,13 @@ class MpdClient( shuttingDown = false } - /** - * Non-suspending teardown for owner destruction (e.g. `ViewModel.onCleared`). - * Cancels the internal scope and drops the sockets without waiting. - */ + /** Non-suspending teardown for owner destruction (e.g. `ViewModel.onCleared`). */ fun shutdown() { shuttingDown = true - try { - idleConn?.close() - } catch (_: IOException) { - } - try { - commandConn?.close() - } catch (_: IOException) { - } - idleConn = null - commandConn = null + idleJob?.cancel() + keepaliveJob?.cancel() + reconnectJob?.cancel() + closeConnectionsQuietly() scope.cancel() _connectionState.value = MpdConnectionState.Disconnected } @@ -142,8 +149,7 @@ class MpdClient( // // These don't optimistically update the flows: the change they cause makes // the server emit an idle event, which refreshes state through the idle - // loop. That keeps the app in lockstep with the server (and with other - // clients) rather than guessing. + // loop, keeping us in lockstep with the server (and other clients). suspend fun play() = run(MpdCommands.play()) suspend fun playPos(pos: Int) = run(MpdCommands.playPos(pos)) @@ -153,9 +159,9 @@ class MpdClient( suspend fun previous() = run(MpdCommands.previous()) suspend fun pause(paused: Boolean) = run(MpdCommands.pause(paused)) - /** Flip play/pause based on the latest known [status]. No-op if state is unknown. */ + /** Flip play/pause based on the latest known [status]. */ suspend fun togglePause() { - val playing = _status.value?.state == ca.ksamad.musicremote.mpd.model.PlayerState.PLAY + val playing = _status.value?.state == PlayerState.PLAY run(MpdCommands.pause(playing)) } @@ -171,29 +177,51 @@ class MpdClient( conn.execute(MpdCommands.playlistInfo()).split().mapNotNull { MpdSong.from(it) } } + /** Every album in the library. */ + suspend fun albums(): List = withCommand { conn -> + MpdAlbum.listFrom(conn.execute(MpdCommands.listAlbums())) + } + + /** Replace the queue with an album and start playing it. */ + suspend fun playAlbum(album: String, albumArtist: String?) = withCommand { conn -> + conn.execute(MpdCommands.clear()) + conn.execute(MpdCommands.findAddAlbum(album, albumArtist)) + conn.execute(MpdCommands.play()) + } + // --- Internals ---------------------------------------------------------- - /** Run a fire-and-forget command whose response we don't need. */ private suspend fun run(commandLine: String) { withCommand { it.execute(commandLine) } } - /** Serialize access to the command connection and run [block] on the I/O dispatcher. */ + /** + * Serialize access to the command connection. A server `ACK` is rethrown as-is + * (the connection is fine); a transport failure triggers a reconnect. + */ private suspend fun withCommand(block: (MpdConnection) -> T): T = withContext(ioDispatcher) { commandMutex.withLock { - val conn = commandConn - ?: throw MpdConnectionException("not connected") - block(conn) + val conn = commandConn ?: throw MpdConnectionException("not connected") + try { + block(conn) + } catch (e: MpdAckException) { + throw e // server rejected the command; connection is healthy + } catch (e: IOException) { + triggerReconnect("command failed: ${e.message}") + throw e + } } } - /** Re-read `status` and `currentsong` into the flows. */ + /** Re-read `status` and `currentsong` into the flows (single round-trip). */ private suspend fun refresh() { val (status, song) = withCommand { conn -> - val status = MpdStatus.from(conn.execute(MpdCommands.status()).toMap()) - val song = MpdSong.from(conn.execute(MpdCommands.currentSong()).toMap()) - status to song + val (statusResp, songResp) = conn.executeList( + MpdCommands.status(), + MpdCommands.currentSong(), + ) + MpdStatus.from(statusResp.toMap()) to MpdSong.from(songResp.toMap()) } _status.value = status _currentSong.value = song @@ -204,26 +232,75 @@ class MpdClient( idleJob = scope.launch { try { while (isActive) { - // Blocks here until the server reports a change (or we close - // the socket during disconnect, which throws below). val changed = withContext(ioDispatcher) { idle.execute(MpdCommands.idle()).getAll("changed") } - if (changed.isEmpty()) continue - // Any of these subsystems affect what we currently show. - if (changed.any { it in REFRESHING_SUBSYSTEMS }) { - refresh() - } + if (changed.any { it in REFRESHING_SUBSYSTEMS }) refresh() } } catch (e: IOException) { - if (!shuttingDown) failAndClose(e.message ?: "idle connection lost") + if (!shuttingDown) triggerReconnect("idle connection lost: ${e.message}") } } } - /** Record an error state and drop the connections (best-effort). */ - private fun failAndClose(message: String) { - _connectionState.value = MpdConnectionState.Error(message) + /** + * Periodic `ping` so MPD's `connection_timeout` never reaps our (often quiet) + * command connection, and so a dead socket is noticed promptly. + */ + private fun startKeepalive() { + keepaliveJob = scope.launch { + while (isActive) { + delay(KEEPALIVE_INTERVAL_MS) + try { + withCommand { it.execute(MpdCommands.ping()) } + } catch (_: IOException) { + // withCommand already triggered a reconnect on a transport error. + return@launch + } + } + } + } + + /** Start a background reconnect unless one is already running or we're shutting down. */ + @Synchronized + private fun triggerReconnect(reason: String) { + if (shuttingDown || reconnectJob?.isActive == true) return + reconnectJob = scope.launch { reconnectLoop() } + } + + private suspend fun reconnectLoop() { + stopBackgroundJobs() + closeConnectionsQuietly() + _connectionState.value = MpdConnectionState.Connecting + var attempt = 0 + while (scope.isActive && !shuttingDown) { + attempt++ + try { + openConnections() + refresh() + _connectionState.value = MpdConnectionState.Connected + startIdleLoop() + startKeepalive() + return + } catch (e: IOException) { + if (attempt >= MAX_RECONNECT_ATTEMPTS) { + failAndClose(e.message ?: "reconnect failed") + return + } + delay(RECONNECT_BACKOFF_MS * attempt) + } + } + } + + /** Cancel the idle + keepalive loops (but not a running reconnect). */ + private suspend fun stopBackgroundJobs() { + idleJob?.cancelAndJoin() + keepaliveJob?.cancelAndJoin() + idleJob = null + keepaliveJob = null + } + + private fun closeConnectionsQuietly() { try { idleConn?.close() } catch (_: IOException) { @@ -236,8 +313,16 @@ class MpdClient( commandConn = null } + private fun failAndClose(message: String) { + _connectionState.value = MpdConnectionState.Error(message) + closeConnectionsQuietly() + } + companion object { private const val COMMAND_READ_TIMEOUT_MS = 10_000 + private const val KEEPALIVE_INTERVAL_MS = 25_000L + private const val RECONNECT_BACKOFF_MS = 1_000L + private const val MAX_RECONNECT_ATTEMPTS = 5 /** Idle subsystems that change something [status]/[currentSong] reflects. */ private val REFRESHING_SUBSYSTEMS = setOf("player", "mixer", "options", "playlist") diff --git a/app/src/main/kotlin/ca/ksamad/musicremote/mpd/MpdCommands.kt b/app/src/main/kotlin/ca/ksamad/musicremote/mpd/MpdCommands.kt index 45e64e3..3703fc0 100644 --- a/app/src/main/kotlin/ca/ksamad/musicremote/mpd/MpdCommands.kt +++ b/app/src/main/kotlin/ca/ksamad/musicremote/mpd/MpdCommands.kt @@ -66,4 +66,17 @@ object MpdCommands { fun clear() = MpdProtocol.command("clear") fun add(uri: String) = MpdProtocol.command("add", uri) fun deleteId(songId: Int) = MpdProtocol.command("deleteid", songId.toString()) + + // --- Database / library ------------------------------------------------- + + /** Every album, grouped by album artist (`Album`/`AlbumArtist` lines). */ + fun listAlbums() = MpdProtocol.command("list", "album", "group", "albumartist") + + /** Append every track of an album to the queue (optionally scoped to an artist). */ + fun findAddAlbum(album: String, albumArtist: String?) = + if (albumArtist.isNullOrEmpty()) { + MpdProtocol.command("findadd", "album", album) + } else { + MpdProtocol.command("findadd", "album", album, "albumartist", albumArtist) + } } diff --git a/app/src/main/kotlin/ca/ksamad/musicremote/mpd/MpdConnection.kt b/app/src/main/kotlin/ca/ksamad/musicremote/mpd/MpdConnection.kt index 480e57a..1361ed5 100644 --- a/app/src/main/kotlin/ca/ksamad/musicremote/mpd/MpdConnection.kt +++ b/app/src/main/kotlin/ca/ksamad/musicremote/mpd/MpdConnection.kt @@ -65,6 +65,28 @@ class MpdConnection( fun execute(name: String, vararg args: String): MpdResponse = execute(MpdProtocol.command(name, *args)) + /** + * Run several commands as a single batch (`command_list_ok_begin`), returning + * one [MpdResponse] per command in order. One network round-trip instead of + * N — worth it on a slow server. Each command's response is delimited by + * `list_OK`, and the batch ends with a final `OK`. + */ + fun executeList(vararg commandLines: String): List { + val payload = buildString { + append("command_list_ok_begin") + for (c in commandLines) { + append('\n') + append(c) + } + append('\n') + append("command_list_end") + } + write(payload) + val responses = List(commandLines.size) { readResponse() } // each stops at list_OK + readResponse() // consume the final OK terminating the batch + return responses + } + private fun write(commandLine: String) { output.write(commandLine.toByteArray(Charsets.UTF_8)) output.write('\n'.code) @@ -78,7 +100,9 @@ class MpdConnection( val line = readLine() ?: throw MpdConnectionException("connection closed mid-response") when { - line == MpdProtocol.OK -> return MpdResponse(values, binary) + // OK ends a command; list_OK ends one command within a command list. + line == MpdProtocol.OK || line == MpdProtocol.LIST_OK -> + return MpdResponse(values, binary) line.startsWith("ACK ") -> throw MpdAckException.parse(line) ?: MpdConnectionException("malformed ACK: $line") diff --git a/app/src/main/kotlin/ca/ksamad/musicremote/mpd/model/MpdAlbum.kt b/app/src/main/kotlin/ca/ksamad/musicremote/mpd/model/MpdAlbum.kt new file mode 100644 index 0000000..73ff95b --- /dev/null +++ b/app/src/main/kotlin/ca/ksamad/musicremote/mpd/model/MpdAlbum.kt @@ -0,0 +1,28 @@ +package ca.ksamad.musicremote.mpd.model + +import ca.ksamad.musicremote.mpd.MpdResponse + +/** An album in the library, optionally attributed to an album artist. */ +data class MpdAlbum( + val name: String, + val albumArtist: String?, +) { + companion object { + /** + * Parse the response of `list album group albumartist`. MPD emits an + * `AlbumArtist:` line followed by the `Album:` lines belonging to it, so + * we track the current artist and attach it to each album. + */ + fun listFrom(response: MpdResponse): List { + val albums = ArrayList() + var currentArtist: String? = null + for ((key, value) in response.values) { + when (key) { + "AlbumArtist" -> currentArtist = value.ifEmpty { null } + "Album" -> if (value.isNotEmpty()) albums.add(MpdAlbum(value, currentArtist)) + } + } + return albums + } + } +} diff --git a/app/src/main/kotlin/ca/ksamad/musicremote/playback/MpdConnectionManager.kt b/app/src/main/kotlin/ca/ksamad/musicremote/playback/MpdConnectionManager.kt index a480d7b..9a6bc87 100644 --- a/app/src/main/kotlin/ca/ksamad/musicremote/playback/MpdConnectionManager.kt +++ b/app/src/main/kotlin/ca/ksamad/musicremote/playback/MpdConnectionManager.kt @@ -5,9 +5,12 @@ import ca.ksamad.musicremote.data.ConnectionSettings import ca.ksamad.musicremote.data.SettingsRepository import ca.ksamad.musicremote.mpd.MpdClient import ca.ksamad.musicremote.mpd.MpdConnectionState +import ca.ksamad.musicremote.mpd.model.MpdAlbum import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.delay import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow @@ -54,7 +57,19 @@ class MpdConnectionManager(context: Context) { @Volatile private var pendingVolume: Int? = null + // Volume writes are coalesced through a conflated channel + throttle so that + // holding a volume key (or dragging the OS remote-volume slider) sends at + // most one `setvol` per interval with the latest value — a tiny MPD server + // (e.g. a Pi Zero) is easily overwhelmed by a flood of them otherwise. + private val volumeRequests = Channel(Channel.CONFLATED) + init { + scope.launch { + for (target in volumeRequests) { + runCatching { client.setVolume(target) } + delay(VOLUME_THROTTLE_MS) + } + } // Auto-connect on startup if a server was saved. scope.launch { val saved = settingsRepo.settingsOrNull.first() @@ -109,10 +124,18 @@ class MpdConnectionManager(context: Context) { fun previous() = fire { previous() } fun togglePlayPause() = fire { togglePause() } fun seekTo(seconds: Double) = fire { seekCurrent(seconds) } - fun setVolume(volume: Int) = fire { setVolume(volume) } + fun setVolume(volume: Int) { + // Coalesced + throttled by the volumeRequests consumer (see init). + volumeRequests.trySend(volume.coerceIn(0, 100)) + } fun setRepeat(on: Boolean) = fire { setRepeat(on) } fun setRandom(on: Boolean) = fire { setRandom(on) } + fun playAlbum(album: String, albumArtist: String?) = fire { playAlbum(album, albumArtist) } + + /** One-shot library fetch; returns empty on any failure. */ + suspend fun loadAlbums(): List = runCatching { client.albums() }.getOrDefault(emptyList()) + /** * True when we own the volume: connected to a server that exposes a mixer. * Used to decide whether hardware volume keys drive the *server* (silently, @@ -135,5 +158,6 @@ class MpdConnectionManager(context: Context) { private companion object { const val VOLUME_STEP = 5 + const val VOLUME_THROTTLE_MS = 120L } } diff --git a/app/src/main/kotlin/ca/ksamad/musicremote/ui/AlbumsScreen.kt b/app/src/main/kotlin/ca/ksamad/musicremote/ui/AlbumsScreen.kt new file mode 100644 index 0000000..dcfe164 --- /dev/null +++ b/app/src/main/kotlin/ca/ksamad/musicremote/ui/AlbumsScreen.kt @@ -0,0 +1,91 @@ +package ca.ksamad.musicremote.ui + +import androidx.compose.foundation.clickable +import androidx.compose.foundation.layout.Box +import androidx.compose.foundation.layout.fillMaxSize +import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.lazy.LazyColumn +import androidx.compose.foundation.lazy.items +import androidx.compose.material.icons.Icons +import androidx.compose.material.icons.automirrored.filled.ArrowBack +import androidx.compose.material.icons.filled.Album +import androidx.compose.material3.CircularProgressIndicator +import androidx.compose.material3.ExperimentalMaterial3Api +import androidx.compose.material3.Icon +import androidx.compose.material3.IconButton +import androidx.compose.material3.ListItem +import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.Scaffold +import androidx.compose.material3.Text +import androidx.compose.material3.TopAppBar +import androidx.compose.runtime.Composable +import androidx.compose.runtime.getValue +import androidx.compose.runtime.produceState +import androidx.compose.ui.Alignment +import androidx.compose.ui.Modifier +import androidx.compose.ui.text.style.TextOverflow +import androidx.compose.ui.unit.dp +import ca.ksamad.musicremote.mpd.model.MpdAlbum + +/** + * Browse every album on the server. Tapping one replaces the queue with that + * album and starts playing it, then returns to the now-playing screen. + * + * (Album artwork is item #5 — for now each row shows a placeholder icon.) + */ +@OptIn(ExperimentalMaterial3Api::class) +@Composable +fun AlbumsScreen(vm: PlayerViewModel, onBack: () -> Unit, onPlay: () -> Unit) { + // null = still loading. + val albums by produceState?>(initialValue = null) { + value = vm.loadAlbums().sortedWith( + compareBy({ it.albumArtist?.lowercase() ?: "" }, { it.name.lowercase() }), + ) + } + + Scaffold( + topBar = { + TopAppBar( + title = { Text(albums?.let { "Albums (${it.size})" } ?: "Albums") }, + navigationIcon = { + IconButton(onClick = onBack) { + Icon(Icons.AutoMirrored.Filled.ArrowBack, contentDescription = "Back") + } + }, + ) + }, + ) { innerPadding -> + val current = albums + when { + current == null -> Box( + Modifier.fillMaxSize().padding(innerPadding), + contentAlignment = Alignment.Center, + ) { CircularProgressIndicator() } + + current.isEmpty() -> Box( + Modifier.fillMaxSize().padding(innerPadding), + contentAlignment = Alignment.Center, + ) { Text("No albums found", color = MaterialTheme.colorScheme.onSurfaceVariant) } + + else -> LazyColumn(modifier = Modifier.padding(innerPadding)) { + items(current) { album -> + ListItem( + modifier = Modifier.clickable { + vm.playAlbum(album.name, album.albumArtist) + onPlay() + }, + leadingContent = { + Icon(Icons.Filled.Album, contentDescription = null) + }, + headlineContent = { + Text(album.name, maxLines = 1, overflow = TextOverflow.Ellipsis) + }, + supportingContent = album.albumArtist?.let { + { Text(it, maxLines = 1, overflow = TextOverflow.Ellipsis) } + }, + ) + } + } + } + } +} diff --git a/app/src/main/kotlin/ca/ksamad/musicremote/ui/MusicRemoteApp.kt b/app/src/main/kotlin/ca/ksamad/musicremote/ui/MusicRemoteApp.kt index 7240165..eb84960 100644 --- a/app/src/main/kotlin/ca/ksamad/musicremote/ui/MusicRemoteApp.kt +++ b/app/src/main/kotlin/ca/ksamad/musicremote/ui/MusicRemoteApp.kt @@ -31,26 +31,36 @@ import ca.ksamad.musicremote.data.ConnectionSettings * brief loading splash while it reads settings / auto-connects, the connect form * (first run, after disconnect, or on error), or the now-playing screen. */ +/** Sub-screens layered over the player (no nav library needed for this few). */ +private enum class PlayerOverlay { None, Settings, Albums } + @Composable fun MusicRemoteApp(vm: PlayerViewModel = viewModel()) { val screen by vm.screen.collectAsStateWithLifecycle() - var showSettings by rememberSaveable { mutableStateOf(false) } + var overlay by rememberSaveable { mutableStateOf(PlayerOverlay.None) } - // Settings is only meaningful over the player; leaving it (e.g. after a + // Overlays only make sense over the player; leaving it (e.g. after a // reset/disconnect) drops us back to the normal screen flow. LaunchedEffect(screen) { - if (screen !is AppScreen.Player) showSettings = false + if (screen !is AppScreen.Player) overlay = PlayerOverlay.None } when (val s = screen) { is AppScreen.Loading -> LoadingScreen() is AppScreen.Connect -> ConnectScreen(vm, s.settings, s.error) - is AppScreen.Player -> - if (showSettings) { - SettingsScreen(vm, onBack = { showSettings = false }) - } else { - NowPlayingScreen(vm, onOpenSettings = { showSettings = true }) - } + is AppScreen.Player -> when (overlay) { + PlayerOverlay.Settings -> SettingsScreen(vm, onBack = { overlay = PlayerOverlay.None }) + PlayerOverlay.Albums -> AlbumsScreen( + vm, + onBack = { overlay = PlayerOverlay.None }, + onPlay = { overlay = PlayerOverlay.None }, + ) + PlayerOverlay.None -> NowPlayingScreen( + vm, + onOpenSettings = { overlay = PlayerOverlay.Settings }, + onOpenLibrary = { overlay = PlayerOverlay.Albums }, + ) + } } } diff --git a/app/src/main/kotlin/ca/ksamad/musicremote/ui/NowPlayingScreen.kt b/app/src/main/kotlin/ca/ksamad/musicremote/ui/NowPlayingScreen.kt index ebc4af2..dabea6f 100644 --- a/app/src/main/kotlin/ca/ksamad/musicremote/ui/NowPlayingScreen.kt +++ b/app/src/main/kotlin/ca/ksamad/musicremote/ui/NowPlayingScreen.kt @@ -11,6 +11,7 @@ import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.size import androidx.compose.material.icons.Icons import androidx.compose.material.icons.filled.Cast +import androidx.compose.material.icons.filled.LibraryMusic import androidx.compose.material.icons.filled.Pause import androidx.compose.material.icons.filled.PlayArrow import androidx.compose.material.icons.filled.Repeat @@ -51,7 +52,11 @@ import kotlinx.coroutines.delay * including changes made from other clients. */ @Composable -fun NowPlayingScreen(vm: PlayerViewModel, onOpenSettings: () -> Unit) { +fun NowPlayingScreen( + vm: PlayerViewModel, + onOpenSettings: () -> Unit, + onOpenLibrary: () -> Unit, +) { val status by vm.status.collectAsStateWithLifecycle() val song by vm.currentSong.collectAsStateWithLifecycle() val serverHost by vm.serverHost.collectAsStateWithLifecycle() @@ -62,8 +67,11 @@ fun NowPlayingScreen(vm: PlayerViewModel, onOpenSettings: () -> Unit) { .padding(24.dp), horizontalAlignment = Alignment.CenterHorizontally, ) { - // --- Top bar: settings ---------------------------------------------- + // --- Top bar: library + settings ------------------------------------ Row(Modifier.fillMaxWidth(), horizontalArrangement = Arrangement.End) { + IconButton(onClick = onOpenLibrary) { + Icon(Icons.Filled.LibraryMusic, contentDescription = "Albums") + } IconButton(onClick = onOpenSettings) { Icon(Icons.Filled.Settings, contentDescription = "Settings") } diff --git a/app/src/main/kotlin/ca/ksamad/musicremote/ui/PlayerViewModel.kt b/app/src/main/kotlin/ca/ksamad/musicremote/ui/PlayerViewModel.kt index a879dd6..6a49805 100644 --- a/app/src/main/kotlin/ca/ksamad/musicremote/ui/PlayerViewModel.kt +++ b/app/src/main/kotlin/ca/ksamad/musicremote/ui/PlayerViewModel.kt @@ -67,4 +67,7 @@ class PlayerViewModel(application: Application) : AndroidViewModel(application) fun setVolume(volume: Int) = manager.setVolume(volume) fun setRepeat(on: Boolean) = manager.setRepeat(on) fun setRandom(on: Boolean) = manager.setRandom(on) + + fun playAlbum(album: String, albumArtist: String?) = manager.playAlbum(album, albumArtist) + suspend fun loadAlbums() = manager.loadAlbums() } diff --git a/app/src/test/kotlin/ca/ksamad/musicremote/mpd/MpdConnectionTest.kt b/app/src/test/kotlin/ca/ksamad/musicremote/mpd/MpdConnectionTest.kt index 2964661..4c51803 100644 --- a/app/src/test/kotlin/ca/ksamad/musicremote/mpd/MpdConnectionTest.kt +++ b/app/src/test/kotlin/ca/ksamad/musicremote/mpd/MpdConnectionTest.kt @@ -80,6 +80,27 @@ class MpdConnectionTest { assertArrayEquals(payload, resp.binary) } + @Test + fun executeList_splitsResponsesOnListOk() { + // command_list_ok_begin: each sub-response ends with list_OK, then final OK. + val server = "OK MPD 0.23.5\n" + + "volume: 50\nstate: play\nlist_OK\n" + + "file: a.mp3\nTitle: A\nlist_OK\n" + + "OK\n" + val out = ByteArrayOutputStream() + val c = conn(server, out) + c.handshake() + + val responses = c.executeList("status", "currentsong") + assertEquals(2, responses.size) + assertEquals("50", responses[0]["volume"]) + assertEquals("play", responses[0]["state"]) + assertEquals("a.mp3", responses[1]["file"]) + assertEquals("A", responses[1]["Title"]) + + assertTrue(out.toString("UTF-8").startsWith("command_list_ok_begin\nstatus\ncurrentsong\ncommand_list_end\n")) + } + @Test fun execute_throwsOnEofMidResponse() { val c = conn("OK MPD 0.23.5\nvolume: 50\n") // no OK terminator diff --git a/app/src/test/kotlin/ca/ksamad/musicremote/mpd/model/MpdModelTest.kt b/app/src/test/kotlin/ca/ksamad/musicremote/mpd/model/MpdModelTest.kt index c1ce1b5..53b6aa6 100644 --- a/app/src/test/kotlin/ca/ksamad/musicremote/mpd/model/MpdModelTest.kt +++ b/app/src/test/kotlin/ca/ksamad/musicremote/mpd/model/MpdModelTest.kt @@ -95,6 +95,31 @@ class MpdModelTest { assertEquals(2, songs[1].id) } + @Test + fun albums_parseGroupedByArtist() { + // `list album group albumartist` style output. + val resp = MpdResponse( + listOf( + "AlbumArtist" to "Pink Floyd", + "Album" to "Animals", + "Album" to "The Wall", + "AlbumArtist" to "Daft Punk", + "Album" to "Discovery", + ) + ) + val albums = MpdAlbum.listFrom(resp) + assertEquals(3, albums.size) + assertEquals(MpdAlbum("Animals", "Pink Floyd"), albums[0]) + assertEquals(MpdAlbum("The Wall", "Pink Floyd"), albums[1]) + assertEquals(MpdAlbum("Discovery", "Daft Punk"), albums[2]) + } + + @Test + fun albums_albumWithoutArtistGroupHasNullArtist() { + val resp = MpdResponse(listOf("Album" to "Untitled")) + assertEquals(listOf(MpdAlbum("Untitled", null)), MpdAlbum.listFrom(resp)) + } + @Test fun statistics_parseCounts() { val stats = MpdStatistics.from( diff --git a/docs/TODO.md b/docs/TODO.md index 81aa564..39edcb8 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -68,14 +68,19 @@ Notes: verified on the FiiO via injected key events (a rotary volume knob may no emit `VOLUME_UP/DOWN` — hardware-dependent). Album art in the notification/session is pending item #5. -## [ ] 4. Library browse — albums +## [x] 4. Library browse — albums -A screen to browse all albums on the server (artists can come later). +Done. `AlbumsScreen` (library icon on the player) lists every album via +`list album group albumartist` (`MpdCommands.listAlbums` → `MpdAlbum.listFrom`), +sorted by artist then title. Tapping an album replaces the queue and plays it +(`MpdClient.playAlbum` = `clear` + `findadd album … albumartist …` + `play`), +then returns to the now-playing screen. -- MPD commands: `list album group albumartist` (or `list album`), and - `find album ""` to fetch an album's tracks; add to the queue with - `add`/`findadd`. Extend `MpdCommands` + `MpdClient`. -- Grid or list of albums → tap to view/queue tracks. +- Verified live: 149 albums listed and played correctly on the FiiO. +- Placeholder disc icon per row (real art is item #5). +- Interaction choice: tap = **replace queue + play** (the direct "play this + album" gesture). Could later add a long-press / menu for "add to queue" and an + album-detail/track view. Artist browse is a future extension. ## [ ] 5. Album / artist images @@ -88,7 +93,7 @@ Pull artwork for the now-playing track and for the album browse grid. - Needs an image loader + caching. Coil (`io.coil-kt`) is the standard Compose choice; a custom `MpdArtFetcher` could feed it. Dependency → `deps.json` regen. -## [ ] 6. BUG: idle connection drops after a few minutes → kicked to connect page +## [x] 6. BUG: idle connection drops after a few minutes → kicked to connect page After a few minutes idling, the app surfaces **"connection closed mid-response"** and falls back to the connect screen. MALP does not do this. @@ -108,8 +113,27 @@ and falls back to the connect screen. MALP does not do this. **auto-reconnect transparently** (re-open connections, re-issue `idle`, resync state) and keep showing the player. Consider a keepalive ping and, for backgrounded playback control, a foreground service / partial wakelock. -- PARTIALLY ADDRESSED by item #3: the `mediaPlayback` foreground service now - keeps the process/connection alive in the background, which should stop Doze / - wifi power-save from tearing the sockets down (the most likely cause). Still - worth verifying over a long idle, and adding transparent auto-reconnect + - keepalive ping as defense-in-depth (e.g. against MPD `connection_timeout`). +- PARTIALLY ADDRESSED by item #3: the `mediaPlayback` foreground service keeps + the process/connection alive in the background. + +**FIXED.** Root cause was two-fold: (a) the quiet command connection was reaped +by MPD's `connection_timeout`, and (b) the idle loop treated *any* `IOException` +(including that reap) as fatal → `Error` → connect page. Plus a Pi-Zero overload +angle: volume-key floods + 2-round-trip refreshes. Fixes, all in `MpdClient` +/`MpdConnectionManager`/`MpdConnection`: + +- **Keepalive `ping`** every 25s on the command connection — prevents the + `connection_timeout` reap and detects a dead socket early. +- **Transparent auto-reconnect** (`triggerReconnect`/`reconnectLoop`): a transport + drop reopens both sockets in the background (state → `Connecting`, "Connecting…" + splash, player stays), retrying with backoff; only after + `MAX_RECONNECT_ATTEMPTS` does it surface `Error`. `MpdAckException` is + explicitly *not* treated as a connection failure. +- **Volume throttle**: `setvol` writes are coalesced through a conflated channel + (~1 per 120 ms, latest value) — kills the flood from holding volume keys while + keeping accurate accumulation via `pendingVolume`. +- **Command-list refresh**: `status`+`currentsong` fetched in one round-trip + (`MpdConnection.executeList`, `command_list_ok_begin`). + +Verified on the FiiO: Wi-Fi drop → "Connecting…" → auto-recovers to the player +(no connect-page bounce); 6 rapid volume presses = exactly −30, throttled.