diff --git a/AGENTS.md b/AGENTS.md index 617237a..5a22163 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -375,6 +375,50 @@ it. It is never published, so it has to be asked for. Upstream would close this with two lines — `def play(self, pos=None)` in `player/coordinator.py` and the MPD backend. Coil needs no change when it lands: the ladder's first rung starts succeeding. +## The media session, and the four things media3 will not tell you + +`PhonieboxPlayer` is a `SimpleBasePlayer` whose playback happens on the box, and `feature-media`'s +whole job is translating between the two. Four of media3's contracts are silent when broken — nothing +logs, nothing throws, a control simply does nothing — and all four were broken at some point: + +- **A session the service was never handed shows no notification.** + `MediaNotificationManager.updateNotification` asks `isSessionAdded` before anything else, and media3 + registers a session itself only when a controller binds with `MediaSessionService.SERVICE_INTERFACE` + (or the legacy browser action) or a media button intent arrives — `onBind` returns null for any + other action *without* consulting `onGetSession`. Coil's own binding is neither, so + `PhonieboxMediaService.onCreate` must call **`addSession`** itself. Without it no notification is + ever posted in either session mode, and the same early return calls + `maybeStopForegroundService(removeNotifications = true)`, which cancels media3's notification id. + That is why `MediaSessionBinder` now binds *with* the media3 action, too. +- **Notification ids have one owner each.** 1001 is media3's, and it cancels that id whenever it has + nothing to show; 1002 is the quiet "ready" notification automatic mode waits behind. They used to + share one id, which meant two writers with no ordering between them and a foreground service that + could end up with no notification at all. The quiet one follows `PlayerStatus.hasContent` — the + exact complement of media3's `shouldShowNotification` — so one of the two is up and never both. +- **A command's future must not be complete when it is returned.** `SimpleBasePlayer` shows its + optimistic placeholder state only while a returned future is pending: `updateStateForPendingOperation` + short-circuits on `isDone()` and calls `getState()` straight back, which still describes the box as + it was before the command was sent. `PhonieboxPlayer.send` therefore stays pending until the box has + been told and, where the outcome is published, until it says so — bounded, because `invalidateState` + is ignored while anything is pending. +- **`mediaItemIndex` is a timeline index, not a queue position**, and `C.INDEX_UNSET` means "media3 + resolved this to nothing". Both are read in one place, `seekIntentFor`, whose KDoc carries the + reasoning; the short version is that a fallback timeline's only index means "the playing song", so + passing it to `playAt` restarts the album, and that "unset" only carries meaning when the timeline + really is the queue and shuffle is off. + +Two smaller ones, same character: `COMMAND_GET_CURRENT_MEDIA_ITEM` has to be in the available +commands or the legacy session publishes no position and no duration (`PlayerWrapper` gates them on +it) and the system's media control shows no seek bar; and a timeline item's UID must come from the +queue row it occupies rather than from `playerstatus`, or a track change reads as a whole new playlist +and a queue holding one file twice can throw `"Duplicate MediaItemData UID in playlist"` out of +`getState()`. + +`feature-media` has no Robolectric and should keep it that way: the load-bearing decisions live in +top-level functions — `timelineIndexFor`, `seekIntentFor`, `uidFor` — precisely because `getState()` +needs a `Looper` and they do not. Everything else here is service lifecycle and platform notification +behaviour, which only a device can tell you about. + ## Stand-in cover art A Phoniebox library is largely ripped CDs and home-made folders, so a great deal of it has no diff --git a/CHANGELOG.md b/CHANGELOG.md index b6a3439..4b39302 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,21 @@ automatically from the `## [x.y.z]` heading matching `versionName` in `app/build ## [Unreleased] +## [1.1.1] - 2026-08-12 + +### Fixed +- **The playback controls now actually appear.** Coil is supposed to put the playing track in your + notification shade and on your lock screen, and it never did — the session was built correctly and + then never handed to the part of media3 that posts the notification, so nothing was ever shown in + either of the two control modes. In automatic mode the same fault could leave the quiet "ready" + notification cancelled behind it +- The controls also work properly now that they are there. The lock screen shows a **seek bar with + elapsed and remaining time**; play and pause **respond to the first tap** instead of appearing to + do nothing until the box got round to answering; ⏮ restarts the track part-way in and goes back a + track near the beginning, the way every other player does; a mute button sent from a car stereo + mutes rather than toggling, so it no longer unmutes an already-muted box; and tapping a track in a + car's playlist goes to that track instead of occasionally restarting the album + ## [1.1.0] - 2026-08-11 ### Added diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 63f98ca..7e55ddd 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -20,8 +20,8 @@ android { applicationId = "app.coilforphoniebox" minSdk = 26 targetSdk = 36 - versionCode = 6 - versionName = "1.1.0" + versionCode = 7 + versionName = "1.1.1" } androidResources { diff --git a/app/src/testDebug/kotlin/app/coilforphoniebox/screenshot/FakeRepositories.kt b/app/src/testDebug/kotlin/app/coilforphoniebox/screenshot/FakeRepositories.kt index 4dd20fe..d534494 100644 --- a/app/src/testDebug/kotlin/app/coilforphoniebox/screenshot/FakeRepositories.kt +++ b/app/src/testDebug/kotlin/app/coilforphoniebox/screenshot/FakeRepositories.kt @@ -87,6 +87,8 @@ class FakePlayerRepository( override suspend fun setVolume(level: Int): Result = Result.success(Unit) override suspend fun changeVolume(step: Int): Result = Result.success(Unit) override suspend fun toggleMute(): Result = Result.success(Unit) + + override suspend fun setMuted(muted: Boolean): Result = Result.success(Unit) override suspend fun startSleepTimer(minutes: Int): Result = Result.success(Unit) override suspend fun cancelSleepTimer(): Result = Result.success(Unit) override suspend fun refreshSleepTimer(): Result = Result.success(Unit) diff --git a/core-data/src/main/kotlin/app/coilforphoniebox/data/repository/PlayerRepositoryImpl.kt b/core-data/src/main/kotlin/app/coilforphoniebox/data/repository/PlayerRepositoryImpl.kt index 147ca05..314dd3d 100644 --- a/core-data/src/main/kotlin/app/coilforphoniebox/data/repository/PlayerRepositoryImpl.kt +++ b/core-data/src/main/kotlin/app/coilforphoniebox/data/repository/PlayerRepositoryImpl.kt @@ -433,7 +433,10 @@ class PlayerRepositoryImpl @Inject constructor( transport.call(Commands.changeVolume(step)).unit() override suspend fun toggleMute(): Result = - transport.call(Commands.mute(!transport.currentVolume().muted)).unit() + setMuted(!transport.currentVolume().muted) + + override suspend fun setMuted(muted: Boolean): Result = + transport.call(Commands.mute(muted)).unit() override suspend fun startSleepTimer(minutes: Int): Result { // `GenericTimerClass.start` logs "Ignoring start command" and returns when its timer diff --git a/core-domain/src/main/kotlin/app/coilforphoniebox/domain/repository/PlayerRepository.kt b/core-domain/src/main/kotlin/app/coilforphoniebox/domain/repository/PlayerRepository.kt index 2ca290f..6c296b8 100644 --- a/core-domain/src/main/kotlin/app/coilforphoniebox/domain/repository/PlayerRepository.kt +++ b/core-domain/src/main/kotlin/app/coilforphoniebox/domain/repository/PlayerRepository.kt @@ -115,6 +115,15 @@ interface PlayerRepository { suspend fun changeVolume(step: Int): Result suspend fun toggleMute(): Result + /** + * Mutes or unmutes outright, for a caller that knows which of the two it wants. + * + * The media session is one: a controller asking to mute sends "muted = true", not "the + * other one" — and answering that with [toggleMute] unmutes a box that was already muted. + * The box's own command takes an absolute state, so nothing is lost by saying so. + */ + suspend fun setMuted(muted: Boolean): Result + /** * Sets the timer that stops playback after [minutes], replacing a running one. * diff --git a/fastlane/metadata/android/de-DE/changelogs/7.txt b/fastlane/metadata/android/de-DE/changelogs/7.txt new file mode 100644 index 0000000..b25626d --- /dev/null +++ b/fastlane/metadata/android/de-DE/changelogs/7.txt @@ -0,0 +1,3 @@ +Die Wiedergabesteuerung von Coil erscheint jetzt in der Benachrichtigungsleiste und auf dem Sperrbildschirm. Bisher war sie dort überhaupt nicht zu sehen. + +Und sie funktioniert richtig: mit Fortschrittsleiste und Zeitanzeige, mit Play und Pause, die schon beim ersten Tippen reagieren, und einer Zurück-Taste, die einen laufenden Titel neu startet. diff --git a/fastlane/metadata/android/en-US/changelogs/7.txt b/fastlane/metadata/android/en-US/changelogs/7.txt new file mode 100644 index 0000000..e2693a7 --- /dev/null +++ b/fastlane/metadata/android/en-US/changelogs/7.txt @@ -0,0 +1,3 @@ +Coil's playback controls now appear in your notification shade and on the lock screen. They never showed up at all before this. + +They work properly too: a progress bar with the time, play and pause that react to the first tap, and a previous button that starts the current track again part-way in. diff --git a/fastlane/metadata/android/es-ES/changelogs/7.txt b/fastlane/metadata/android/es-ES/changelogs/7.txt new file mode 100644 index 0000000..ce18b06 --- /dev/null +++ b/fastlane/metadata/android/es-ES/changelogs/7.txt @@ -0,0 +1,3 @@ +Los controles de reproducción de Coil ya aparecen en el panel de notificaciones y en la pantalla de bloqueo: hasta ahora no se mostraban en absoluto. + +Y funcionan de verdad: barra de progreso con el tiempo, reproducir y pausar que responden al primer toque, y un botón de anterior que reinicia la pista que ya ha empezado. diff --git a/fastlane/metadata/android/fr-FR/changelogs/7.txt b/fastlane/metadata/android/fr-FR/changelogs/7.txt new file mode 100644 index 0000000..1ef46ab --- /dev/null +++ b/fastlane/metadata/android/fr-FR/changelogs/7.txt @@ -0,0 +1,3 @@ +Les commandes de lecture de Coil apparaissent enfin dans le volet des notifications et sur l'écran de verrouillage : elles ne s'y affichaient pas du tout jusqu'ici. + +Et elles fonctionnent vraiment : barre de progression avec la durée, lecture et pause qui répondent au premier appui, et un bouton précédent qui reprend le titre en cours depuis le début. diff --git a/fastlane/metadata/android/nl-NL/changelogs/7.txt b/fastlane/metadata/android/nl-NL/changelogs/7.txt new file mode 100644 index 0000000..dd3e451 --- /dev/null +++ b/fastlane/metadata/android/nl-NL/changelogs/7.txt @@ -0,0 +1,3 @@ +De afspeelknoppen van Coil verschijnen nu in het meldingenpaneel en op het vergrendelscherm. Tot nu toe waren ze daar helemaal niet te zien. + +En ze werken ook echt: een voortgangsbalk met de tijd, afspelen en pauzeren die meteen reageren, en een vorige-knop die een al begonnen nummer opnieuw start. diff --git a/feature-media/src/main/kotlin/app/coilforphoniebox/media/MediaSessionBinder.kt b/feature-media/src/main/kotlin/app/coilforphoniebox/media/MediaSessionBinder.kt index 49aa128..e65fd03 100644 --- a/feature-media/src/main/kotlin/app/coilforphoniebox/media/MediaSessionBinder.kt +++ b/feature-media/src/main/kotlin/app/coilforphoniebox/media/MediaSessionBinder.kt @@ -6,6 +6,7 @@ import android.content.ServiceConnection import android.os.IBinder import android.util.Log import androidx.media3.common.util.UnstableApi +import androidx.media3.session.MediaSessionService import app.coilforphoniebox.domain.model.SessionMode import app.coilforphoniebox.domain.repository.SettingsRepository import dagger.hilt.android.qualifiers.ApplicationContext @@ -83,11 +84,18 @@ class MediaSessionBinder @Inject constructor( if (wanted && allowed) bindNow() else unbindNow() } + /** + * The action matters. `MediaSessionService.onBind` answers only its own service action and + * the legacy browser one, and returns null for anything else — so an actionless bind keeps + * the service alive without ever establishing a connection, which left the bound-client + * count here scoring a binding the platform had not made. + */ private fun bindNow() { if (bound) return bound = runCatching { context.bindService( - PhonieboxMediaService.serviceIntent(context), + PhonieboxMediaService.serviceIntent(context) + .setAction(MediaSessionService.SERVICE_INTERFACE), connection, Context.BIND_AUTO_CREATE, ) diff --git a/feature-media/src/main/kotlin/app/coilforphoniebox/media/PhonieboxMediaService.kt b/feature-media/src/main/kotlin/app/coilforphoniebox/media/PhonieboxMediaService.kt index 5d82065..9f74013 100644 --- a/feature-media/src/main/kotlin/app/coilforphoniebox/media/PhonieboxMediaService.kt +++ b/feature-media/src/main/kotlin/app/coilforphoniebox/media/PhonieboxMediaService.kt @@ -26,8 +26,10 @@ import app.coilforphoniebox.transport.ConnectionManager import dagger.hilt.android.AndroidEntryPoint import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.cancel +import kotlinx.coroutines.delay import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.map import kotlinx.coroutines.launch @@ -37,8 +39,9 @@ import javax.inject.Inject * Holds the media session, and with it the connection to the box. * * In the default "only while Coil is open" mode the app binds to this service, so it - * lives exactly as long as the UI and posts a notification only once the box actually - * plays — media3 handles that transition. In automatic mode the service is started + * lives exactly as long as the UI and posts a notification only once the box has something + * loaded — media3 handles that transition, provided the session was handed to it with + * [addSession], without which it posts nothing at all. In automatic mode the service is started * outright and keeps a low-priority notification up while it waits, because a foreground * service has to be visible (§8.3). When controls are switched off entirely the service is * not meant to exist at all: nothing binds or starts it, and it stops itself if it is @@ -66,6 +69,9 @@ class PhonieboxMediaService : MediaSessionService() { private var automatic = false private var networkCallback: ConnectivityManager.NetworkCallback? = null + /** The pending "nothing needs this any more" stop, so a rebind can call it off. */ + private var stopJob: Job? = null + /** * How many clients are bound. In the default mode the service should exist while the * UI does and while something plays, and not a moment longer — a connection nobody is @@ -83,8 +89,8 @@ class PhonieboxMediaService : MediaSessionService() { val player = PhonieboxPlayer(scope, playerRepository, boxRepository) phonieboxPlayer = player - // Shares its notification id with the quiet status notification below, so the - // media notification simply replaces it once playback starts. + // An id of its own, and media3 owns it: it cancels that id whenever it has nothing to + // show, which is why the quiet status notification cannot live there too. setMediaNotificationProvider( DefaultMediaNotificationProvider.Builder(this) .setChannelId(CHANNEL_PLAYBACK) @@ -93,9 +99,32 @@ class PhonieboxMediaService : MediaSessionService() { .apply { setSmallIcon(texts.smallIcon) }, ) - session = MediaSession.Builder(this, player) + val built = MediaSession.Builder(this, player) .apply { launchIntent()?.let { setSessionActivity(it) } } .build() + session = built + + // media3 shows nothing for a session it has not been handed: the first thing + // `MediaNotificationManager.updateNotification` asks is `isSessionAdded`, and media3 + // only registers a session itself when a controller binds with its own service action + // or a media button intent arrives. Coil's binding is neither, so without this line the + // notification is never posted at all — and the same early return cancels the id below. + addSession(built) + + // media3 catches Android's refusal to promote the service and reports it here; with no + // listener set it is swallowed, and the symptom is controls that silently never appear. + setListener( + object : MediaSessionService.Listener { + override fun onForegroundServiceStartNotAllowedException() { + Log.w(TAG, "Android refused to start the media session in the foreground") + // Automatic mode has to show something, and this notification is allowed + // where a foreground promotion is not. + if (automatic) { + notificationManager().notify(STATUS_NOTIFICATION_ID, statusNotification()) + } + } + }, + ) observeIdleState() observeSessionModeSetting() @@ -109,13 +138,22 @@ class PhonieboxMediaService : MediaSessionService() { if (intent?.action == ACTION_START_AUTOMATIC) { automatic = true // Started with startForegroundService, so something visible has to go up now. - startForeground(NOTIFICATION_ID, statusNotification()) + promoteWithStatusNotification() watchNetwork() } return result } + /** + * Automatic mode is a foreground service by contract (§8.3) — it was started outright and + * has to stay visible whether or not the box happens to be playing this second. media3 + * detaches the service as soon as playback stops, so the requirement is forced back on here. + */ + override fun onUpdateNotification(session: MediaSession, startInForegroundRequired: Boolean) { + super.onUpdateNotification(session, startInForegroundRequired || automatic) + } + override fun onBind(intent: Intent?): IBinder? { boundClients++ return super.onBind(intent) @@ -133,32 +171,69 @@ class PhonieboxMediaService : MediaSessionService() { } /** - * While automatic mode waits for the box to start playing there is nothing for media3 - * to show, but the service still has to be visible. Once playback begins, media3's own - * notification takes over the same id, and this quiet one goes with it. + * Keeps the quiet status notification in step with media3's own, and ends the service in + * the default mode when there is nothing left to do. * - * The same signal ends the service in the default mode: nothing playing, nobody bound, - * nothing to do. + * The status notification follows whether the box has anything *loaded*, not whether it is + * playing. `hasContent` is the exact complement of the condition media3 posts under — + * `MediaNotificationManager.shouldShowNotification` wants a non-empty timeline and a state + * other than idle — so one of the two is on screen and never both, and which one no longer + * depends on the order two collectors happen to run in. A *paused* box keeps its media + * controls, which is when they are wanted most. */ private fun observeIdleState() { scope.launch { playerRepository.status - .map { it.state } + .map { it.hasContent to it.state } .distinctUntilChanged() - .collect { state -> - if (automatic && state != PlaybackState.PLAY) { - notificationManager().notify(NOTIFICATION_ID, statusNotification()) - } + .collect { (hasContent, state) -> + if (automatic) updateStatusNotification(hasContent) if (state != PlaybackState.PLAY) stopIfNoLongerNeeded() } } } + private fun updateStatusNotification(hasContent: Boolean) { + if (hasContent) { + // media3 has the shade from here, on its own id. Two notifications for one box + // would be one too many. + notificationManager().cancel(STATUS_NOTIFICATION_ID) + } else { + // `startForeground` rather than `notify`: media3 has just detached the service and + // taken its notification down, and automatic mode has to stay foreground. + promoteWithStatusNotification() + } + } + + private fun promoteWithStatusNotification() { + runCatching { startForeground(STATUS_NOTIFICATION_ID, statusNotification()) } + .onFailure { Log.w(TAG, "Could not keep the automatic session in the foreground", it) } + } + + /** + * Ends the service when nothing needs it, after a moment's grace. + * + * The grace is the point. `currentStatus()` is what the box last *published*, which lags a + * command by up to a quarter of a second and by however long the box's own sequential + * socket takes (§6) — so a user who taps play and pockets the phone unbinds the UI while + * the box still reports "not playing", and stopping on that reading would tear the session + * down exactly as the notification was about to appear. + */ private fun stopIfNoLongerNeeded() { - if (automatic || boundClients > 0) return - if (connectionManager.currentStatus().state == PlaybackState.PLAY) return - Log.i(TAG, "Nothing playing and nothing bound; stopping") - stopSelf() + if (automatic || boundClients > 0) { + stopJob?.cancel() + stopJob = null + return + } + + stopJob?.cancel() + stopJob = scope.launch { + delay(STOP_GRACE_MILLIS) + if (automatic || boundClients > 0) return@launch + if (connectionManager.currentStatus().state == PlaybackState.PLAY) return@launch + Log.i(TAG, "Nothing playing and nothing bound; stopping") + stopSelf() + } } /** @@ -240,6 +315,11 @@ class PhonieboxMediaService : MediaSessionService() { } override fun onDestroy() { + stopJob?.cancel() + stopJob = null + + clearListener() + networkCallback?.let { callback -> runCatching { getSystemService(ConnectivityManager::class.java)?.unregisterNetworkCallback(callback) @@ -296,9 +376,21 @@ class PhonieboxMediaService : MediaSessionService() { const val CHANNEL_PLAYBACK = "playback" const val CHANNEL_STATUS = "connection" - /** Shared with media3's own notification, so one replaces the other. */ + /** media3's own, and media3's alone — it cancels this id whenever it has nothing to show. */ const val NOTIFICATION_ID = 1001 + /** + * The quiet "ready" notification automatic mode waits behind. + * + * An id of its own rather than media3's: sharing one meant two writers with no ordering + * between them, and media3 cancelling the id was enough to leave a foreground service + * with no notification at all. + */ + const val STATUS_NOTIFICATION_ID = 1002 + + /** Long enough for a command in flight to come back as a published status (§8.1). */ + private const val STOP_GRACE_MILLIS = 2_000L + const val ACTION_START_AUTOMATIC = "app.coilforphoniebox.media.START_AUTOMATIC" fun automaticIntent(context: Context): Intent = diff --git a/feature-media/src/main/kotlin/app/coilforphoniebox/media/PhonieboxPlayer.kt b/feature-media/src/main/kotlin/app/coilforphoniebox/media/PhonieboxPlayer.kt index 9004142..494250b 100644 --- a/feature-media/src/main/kotlin/app/coilforphoniebox/media/PhonieboxPlayer.kt +++ b/feature-media/src/main/kotlin/app/coilforphoniebox/media/PhonieboxPlayer.kt @@ -2,6 +2,7 @@ package app.coilforphoniebox.media import android.net.Uri import android.os.Looper +import androidx.media3.common.C import androidx.media3.common.DeviceInfo import androidx.media3.common.MediaItem import androidx.media3.common.MediaMetadata @@ -18,11 +19,15 @@ import app.coilforphoniebox.domain.repository.BoxRepository import app.coilforphoniebox.domain.repository.PlayerRepository import com.google.common.util.concurrent.Futures import com.google.common.util.concurrent.ListenableFuture +import com.google.common.util.concurrent.SettableFuture import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.map +import kotlinx.coroutines.isActive import kotlinx.coroutines.launch +import kotlinx.coroutines.withTimeoutOrNull /** * Where [status] sits in [queue], or null when the two cannot be reconciled. @@ -51,13 +56,137 @@ internal fun timelineIndexFor(queue: List, status: PlayerStatus): In return position } +/** What a media3 seek turns into on the box. */ +internal sealed interface SeekIntent { + /** media3 resolved the seek to nothing, so nothing is sent. */ + data object Ignore : SeekIntent + data object Next : SeekIntent + data object Previous : SeekIntent + + /** A jump to a *queue position* — never a timeline index unless the two are the same thing. */ + data class ToQueuePosition(val position: Int) : SeekIntent + data class WithinTrack(val seconds: Double) : SeekIntent +} + +/** + * What the box should be told, for a seek media3 has already resolved against its own timeline. + * + * Three things make this more than a `when` on the command, and all three were wrong before it + * existed: + * + * - **Two index spaces.** `mediaItemIndex` is a *timeline* index. It is also a queue position, but + * only while the timeline is the queue — when [timelineIndexFor] declined and the timeline is the + * playing song alone, the only index there is means "this song", and handing that to `playAt` + * walks the box to queue position zero and restarts the album. Hence [queuePosition], which is + * that function's answer: non-null means the indices are queue positions. + * - **`C.INDEX_UNSET` means "do nothing".** `BasePlayer.ignoreSeek` sends it when there is nothing + * to seek to, and sending `next` anyway runs the box's own `end_of_playlist_next_action` at the + * end of a queue (§"The queue"). It only carries that meaning in the real queue, though: a + * one-item timeline has no next or previous item, so media3 says "unset" about every one of them + * and the signal is noise. With [shuffle] on it is noise too — media3's timeline has no shuffle + * order, so it reports the last *position* as the end even though MPD would pick another track. + * - **Previous is position-dependent.** Past `maxSeekToPreviousPosition` (media3's default three + * seconds) "previous" means restarting the current track, which media3 passes down as position + * zero on the current index. In the fallback timeline it cannot make that call for us, so + * [elapsedSeconds] makes it here against the same boundary. + * + * A top-level function for the same reason [timelineIndexFor] is one: `getState()` needs a `Looper` + * and this does not. + */ +internal fun seekIntentFor( + seekCommand: Int, + mediaItemIndex: Int, + positionMs: Long, + queuePosition: Int?, + shuffle: Boolean, + elapsedSeconds: Double?, +): SeekIntent { + val queued = queuePosition != null + val resolvedToNothing = mediaItemIndex == C.INDEX_UNSET && queued && !shuffle + + return when (seekCommand) { + Player.COMMAND_SEEK_TO_NEXT, Player.COMMAND_SEEK_TO_NEXT_MEDIA_ITEM -> + if (resolvedToNothing) SeekIntent.Ignore else SeekIntent.Next + + Player.COMMAND_SEEK_TO_PREVIOUS -> when { + resolvedToNothing -> SeekIntent.Ignore + queued -> + if (mediaItemIndex == queuePosition && positionMs == 0L) { + SeekIntent.WithinTrack(0.0) + } else { + SeekIntent.Previous + } + + (elapsedSeconds ?: 0.0) > MAX_SEEK_TO_PREVIOUS_SECONDS -> SeekIntent.WithinTrack(0.0) + else -> SeekIntent.Previous + } + + Player.COMMAND_SEEK_TO_PREVIOUS_MEDIA_ITEM -> + if (resolvedToNothing) SeekIntent.Ignore else SeekIntent.Previous + + // A seek naming an item is a jump to that queue position — a tap in Android Auto's + // queue, or "play track five". Any `positionMs` that came with it is dropped: the box + // cannot arrive at a position *and* an offset in one command, and getting to the right + // track matters more than starting it at second 30. + Player.COMMAND_SEEK_TO_MEDIA_ITEM -> + if (queued && mediaItemIndex >= 0) { + SeekIntent.ToQueuePosition(mediaItemIndex) + } else { + // The one item of a fallback timeline is the playing song, so this is a seek + // inside it — commonly back to the start. + withinTrack(positionMs) + } + + // COMMAND_SEEK_IN_CURRENT_MEDIA_ITEM, and anything later media3 routes the same way. + else -> withinTrack(positionMs) + } +} + +private fun withinTrack(positionMs: Long): SeekIntent = + if (positionMs == C.TIME_UNSET) { + // "The default position", which for a box already playing that song is where it is. + SeekIntent.Ignore + } else { + SeekIntent.WithinTrack(positionMs.coerceAtLeast(0L) / 1000.0) + } + +/** media3's own boundary between "previous track" and "start this one again". */ +private const val MAX_SEEK_TO_PREVIOUS_SECONDS = + C.DEFAULT_MAX_SEEK_TO_PREVIOUS_POSITION_MS / 1000.0 + +/** + * A timeline item's identity, and it has to be the queue row's rather than the status's. + * + * media3 compares UIDs position by position to decide what changed + * (`SimpleBasePlayer.getTimelineChangeReason`), and refuses a playlist that repeats one + * (`"Duplicate MediaItemData UID in playlist"`, thrown straight out of `getState()`). The playing + * item's *metadata* comes from `playerstatus`, so it used to take its uid from there too — from a + * field the box need not send, in which case every track change rewrote two UIDs and each one read + * as a wholesale playlist change. Worse, a queue holding the same file twice can leave + * `playerstatus.songid` pointing at the other copy, and that collision is the crash. + * + * MPD's queue id is unique within the queue; without one the position separates two copies of the + * same file. + */ +internal fun uidFor(entry: QueueEntry): String = entry.songId ?: "${entry.position}:${entry.url}" + +/** + * Identity for the single-item timeline, where there is no queue row to borrow one from. Nothing + * compares it against a neighbour, so anything stable for as long as the song lasts will do. + */ +internal fun fallbackUid(status: PlayerStatus): String = + status.songId ?: status.file ?: CURRENT_ITEM_UID + +internal const val CURRENT_ITEM_UID = "coil-current" + /** * A media3 player whose playback happens somewhere else entirely. * * `SimpleBasePlayer` exists precisely for this case (the Cast scenario): [getState] is - * assembled from the most recent `playerstatus`, and every command fires an RPC and - * returns immediately. The UI is therefore optimistic and gets corrected by the next - * published status a quarter of a second later (§8.1). + * assembled from the most recent `playerstatus`, and every command fires an RPC. No button + * waits on the network — media3 shows the outcome optimistically the moment it is asked — but + * the command does stay *pending* until the box has caught up, which is what makes that + * optimism last long enough to see (§8.1, and [send]). * * The useful side effect of `COMMAND_SET_DEVICE_VOLUME` is that the phone's hardware * volume buttons change the Phoniebox while the session is active. @@ -141,13 +270,18 @@ class PhonieboxPlayer( builder.setPlaylist( current.queue.mapIndexed { position, entry -> // The playing item keeps being built from `playerstatus`: that is the - // authoritative metadata, and the only item there is cover art for. - if (position == index) mediaItemFor(current) else mediaItemFor(entry) + // authoritative metadata, and the only item there is cover art for. Its + // *identity* stays the queue row's, though — see [uidFor]. + if (position == index) { + mediaItemFor(current, uid = uidFor(entry)) + } else { + mediaItemFor(entry) + } }, ) builder.setCurrentMediaItemIndex(index) } else if (status.hasContent) { - builder.setPlaylist(listOf(mediaItemFor(current))) + builder.setPlaylist(listOf(mediaItemFor(current, uid = fallbackUid(status)))) builder.setCurrentMediaItemIndex(0) } @@ -178,7 +312,7 @@ class PhonieboxPlayer( val durationUs = entry.durationSeconds ?.takeIf { it > 0 } ?.let { (it * 1_000_000).toLong() } - ?: androidx.media3.common.C.TIME_UNSET + ?: C.TIME_UNSET val metadata = MediaMetadata.Builder() .setTitle(entry.title) @@ -188,9 +322,7 @@ class PhonieboxPlayer( .setIsPlayable(true) .build() - // MPD's queue id is unique within the queue; without one, the position keeps two - // copies of the same file in one queue from colliding — media3 requires unique UIDs. - return MediaItemData.Builder(entry.songId ?: "${entry.position}:${entry.url}") + return MediaItemData.Builder(uidFor(entry)) .setMediaItem( MediaItem.Builder() .setMediaId(entry.url) @@ -198,17 +330,17 @@ class PhonieboxPlayer( .build(), ) .setDurationUs(durationUs) - .setIsSeekable(durationUs != androidx.media3.common.C.TIME_UNSET) + .setIsSeekable(durationUs != C.TIME_UNSET) .setIsDynamic(false) .build() } - private fun mediaItemFor(current: Snapshot): MediaItemData { + private fun mediaItemFor(current: Snapshot, uid: Any): MediaItemData { val status = current.status val durationUs = status.durationSeconds ?.takeIf { it > 0 } ?.let { (it * 1_000_000).toLong() } - ?: androidx.media3.common.C.TIME_UNSET + ?: C.TIME_UNSET val metadata = MediaMetadata.Builder() .setTitle(status.title) @@ -222,7 +354,7 @@ class PhonieboxPlayer( .setIsPlayable(true) .build() - return MediaItemData.Builder(status.songId ?: status.file ?: CURRENT_ITEM_UID) + return MediaItemData.Builder(uid) .setMediaItem( MediaItem.Builder() .setMediaId(status.file ?: CURRENT_ITEM_UID) @@ -230,80 +362,138 @@ class PhonieboxPlayer( .build(), ) .setDurationUs(durationUs) - .setIsSeekable(durationUs != androidx.media3.common.C.TIME_UNSET) + .setIsSeekable(durationUs != C.TIME_UNSET) .setIsDynamic(false) .build() } - override fun handleSetPlayWhenReady(playWhenReady: Boolean): ListenableFuture<*> = fireAndForget { + override fun handleSetPlayWhenReady(playWhenReady: Boolean): ListenableFuture<*> = send { if (playWhenReady) player.play() else player.pause() + awaitStatus { (it.state == PlaybackState.PLAY) == playWhenReady } } override fun handlePrepare(): ListenableFuture<*> = Futures.immediateVoidFuture() - override fun handleStop(): ListenableFuture<*> = fireAndForget { + override fun handleStop(): ListenableFuture<*> = send { // Coil never sends anything beyond playback control; pausing is as far as it goes. player.pause() + awaitStatus { it.state != PlaybackState.PLAY } } override fun handleSeek( mediaItemIndex: Int, positionMs: Long, seekCommand: Int, - ): ListenableFuture<*> = fireAndForget { - when (seekCommand) { - Player.COMMAND_SEEK_TO_NEXT, Player.COMMAND_SEEK_TO_NEXT_MEDIA_ITEM -> player.next() - Player.COMMAND_SEEK_TO_PREVIOUS, Player.COMMAND_SEEK_TO_PREVIOUS_MEDIA_ITEM -> - player.previous() - - // A seek naming an item is a jump to that queue position — a tap in Android Auto's - // queue, or "play track five". Any `positionMs` that came with it is dropped: the - // box has no way to arrive at a position *and* an offset in one command, and - // getting to the right track matters more than starting it at second 30. - Player.COMMAND_SEEK_TO_MEDIA_ITEM -> player.playAt(mediaItemIndex) - - else -> player.seekTo(positionMs / 1000.0) + ): ListenableFuture<*> { + val current = snapshot + val intent = seekIntentFor( + seekCommand = seekCommand, + mediaItemIndex = mediaItemIndex, + positionMs = positionMs, + // The same call [getState] makes, so the handler and the timeline it published + // cannot disagree about which index space media3 is speaking in. + queuePosition = timelineIndexFor(current.queue, current.status), + shuffle = current.status.shuffle, + elapsedSeconds = current.status.elapsedSeconds, + ) + + return when (intent) { + SeekIntent.Ignore -> Futures.immediateVoidFuture() + SeekIntent.Next -> send { player.next() } + SeekIntent.Previous -> send { player.previous() } + is SeekIntent.ToQueuePosition -> send { player.playAt(intent.position) } + is SeekIntent.WithinTrack -> send { player.seekTo(intent.seconds) } } } - override fun handleSetDeviceVolume(deviceVolume: Int, flags: Int): ListenableFuture<*> = - fireAndForget { player.setVolume(deviceVolume) } + override fun handleSetDeviceVolume(deviceVolume: Int, flags: Int): ListenableFuture<*> = send { + player.setVolume(deviceVolume) + awaitVolume { it.level == deviceVolume } + } + // No confirmation for the two relative ones: media3's placeholder assumes a step of one, so + // holding it until the box answers would only show a wrong number for longer. override fun handleIncreaseDeviceVolume(flags: Int): ListenableFuture<*> = - fireAndForget { player.changeVolume(VOLUME_STEP) } + send { player.changeVolume(VOLUME_STEP) } override fun handleDecreaseDeviceVolume(flags: Int): ListenableFuture<*> = - fireAndForget { player.changeVolume(-VOLUME_STEP) } + send { player.changeVolume(-VOLUME_STEP) } - override fun handleSetDeviceMuted(muted: Boolean, flags: Int): ListenableFuture<*> = - fireAndForget { player.toggleMute() } + /** + * Mute is absolute, not a toggle: `VolumeProviderCompat` turns a car stereo's mute key into + * `setDeviceMuted(true)`, and answering that with a toggle unmutes a box already muted. + */ + override fun handleSetDeviceMuted(muted: Boolean, flags: Int): ListenableFuture<*> = send { + player.setMuted(muted) + awaitVolume { it.muted == muted } + } override fun handleSetShuffleModeEnabled(shuffleModeEnabled: Boolean): ListenableFuture<*> = - fireAndForget { player.setShuffle(shuffleModeEnabled) } - - override fun handleSetRepeatMode(repeatMode: Int): ListenableFuture<*> = fireAndForget { - player.setRepeat( - when (repeatMode) { - Player.REPEAT_MODE_ONE -> RepeatMode.ONE - Player.REPEAT_MODE_ALL -> RepeatMode.ALL - else -> RepeatMode.OFF - }, - ) + send { + player.setShuffle(shuffleModeEnabled) + awaitStatus { it.shuffle == shuffleModeEnabled } + } + + override fun handleSetRepeatMode(repeatMode: Int): ListenableFuture<*> = send { + val mode = when (repeatMode) { + Player.REPEAT_MODE_ONE -> RepeatMode.ONE + Player.REPEAT_MODE_ALL -> RepeatMode.ALL + else -> RepeatMode.OFF + } + player.setRepeat(mode) + awaitStatus { it.repeat == mode } } /** - * Every command returns immediately rather than waiting for the box: a lock screen - * button that blocks for a network round trip feels broken even when it works. + * Sends a command, and stays pending until the box has caught up. + * + * The future is the whole mechanism, and an immediate one throws it away. + * `SimpleBasePlayer` shows its optimistic placeholder — the pressed pause button, the new + * volume — only while a returned future is still running: hand it one that is already + * complete and `updateStateForPendingOperation` takes its `isDone()` short circuit and asks + * `getState()` straight back, which still describes the box as it was before the command was + * even sent. The button then does not move until the next published status, a quarter of a + * second later at best, which is exactly how a working control comes to look broken. + * + * So this finishes when the box has been told and, where the outcome is something the box + * reports, when it says so — [awaitStatus] and [awaitVolume] bound that wait, because + * `invalidateState` is ignored while anything is pending and a future that never completes + * would freeze the session. */ - private fun fireAndForget(block: suspend () -> Unit): ListenableFuture<*> { - scope.launch { block() } - return Futures.immediateVoidFuture() + private fun send(block: suspend () -> Unit): ListenableFuture<*> { + // A command arriving as the service goes down: the coroutine would never start, and so + // would never complete the future either. + if (!scope.isActive) return Futures.immediateVoidFuture() + + val pending = SettableFuture.create() + scope.launch { + try { + block() + } finally { + pending.set(Unit) + } + } + return pending + } + + private suspend fun awaitStatus(reached: (PlayerStatus) -> Boolean) { + withTimeoutOrNull(CONFIRM_MILLIS) { player.status.first(reached) } + } + + private suspend fun awaitVolume(reached: (VolumeStatus) -> Boolean) { + withTimeoutOrNull(CONFIRM_MILLIS) { player.volume.first(reached) } } private companion object { - const val CURRENT_ITEM_UID = "coil-current" const val VOLUME_STEP = 5 + /** + * How long a command may hold the optimistic state while waiting for the box to publish + * the outcome. Long enough for the box's sequential socket to get round to it (§6), + * short enough that a box which never agrees is not left holding the session. + */ + const val CONFIRM_MILLIS = 1_000L + val AVAILABLE_COMMANDS: Player.Commands = Player.Commands.Builder() .addAll( Player.COMMAND_PLAY_PAUSE, @@ -318,6 +508,11 @@ class PhonieboxPlayer( // where it must, so a tap in a queue list always does something honest. Player.COMMAND_SEEK_TO_MEDIA_ITEM, Player.COMMAND_GET_TIMELINE, + // Without this the legacy session publishes PLAYBACK_POSITION_UNKNOWN and no + // duration — `PlayerWrapper.createPlaybackStateCompat` reads it to decide + // whether positions may be shared at all — so the system's own media control + // shows no seek bar, and every remote controller sees a null current item. + Player.COMMAND_GET_CURRENT_MEDIA_ITEM, Player.COMMAND_GET_METADATA, Player.COMMAND_SET_SHUFFLE_MODE, Player.COMMAND_SET_REPEAT_MODE, diff --git a/feature-media/src/test/kotlin/app/coilforphoniebox/media/SeekIntentTest.kt b/feature-media/src/test/kotlin/app/coilforphoniebox/media/SeekIntentTest.kt new file mode 100644 index 0000000..b8598c4 --- /dev/null +++ b/feature-media/src/test/kotlin/app/coilforphoniebox/media/SeekIntentTest.kt @@ -0,0 +1,170 @@ +package app.coilforphoniebox.media + +import androidx.media3.common.C +import androidx.media3.common.Player +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * media3 resolves a seek against the timeline it was given and hands down the result; these are + * the readings of that result that were wrong, and each one reached the box as a command it + * should not have been sent. + */ +class SeekIntentTest { + + private fun intent( + seekCommand: Int, + mediaItemIndex: Int = 0, + positionMs: Long = C.TIME_UNSET, + queuePosition: Int? = 3, + shuffle: Boolean = false, + elapsedSeconds: Double? = null, + ) = seekIntentFor( + seekCommand = seekCommand, + mediaItemIndex = mediaItemIndex, + positionMs = positionMs, + queuePosition = queuePosition, + shuffle = shuffle, + elapsedSeconds = elapsedSeconds, + ) + + /** The end of a trusted queue: sending `next` here runs the box's end-of-playlist action. */ + @Test + fun `an unresolved next in the real queue is ignored`() { + assertEquals( + SeekIntent.Ignore, + intent(Player.COMMAND_SEEK_TO_NEXT, mediaItemIndex = C.INDEX_UNSET), + ) + assertEquals( + SeekIntent.Ignore, + intent(Player.COMMAND_SEEK_TO_NEXT_MEDIA_ITEM, mediaItemIndex = C.INDEX_UNSET), + ) + } + + /** + * The fallback timeline is one item, so media3 says "nothing to seek to" about every next and + * previous. Reading that as "do nothing" would disable the notification's buttons outright. + */ + @Test + fun `an unresolved next outside the queue still means next`() { + assertEquals( + SeekIntent.Next, + intent( + Player.COMMAND_SEEK_TO_NEXT, + mediaItemIndex = C.INDEX_UNSET, + queuePosition = null, + ), + ) + } + + /** + * With shuffle on, media3 calls the last position the end of the timeline — it has no shuffle + * order to consult — while MPD would happily pick another track. + */ + @Test + fun `an unresolved next with shuffle on still means next`() { + assertEquals( + SeekIntent.Next, + intent(Player.COMMAND_SEEK_TO_NEXT, mediaItemIndex = C.INDEX_UNSET, shuffle = true), + ) + } + + @Test + fun `a resolved next means next`() { + assertEquals(SeekIntent.Next, intent(Player.COMMAND_SEEK_TO_NEXT, mediaItemIndex = 4)) + } + + /** Position zero on the current item is how media3 spells "start this track again". */ + @Test + fun `previous on the current item at position zero restarts the track`() { + assertEquals( + SeekIntent.WithinTrack(0.0), + intent(Player.COMMAND_SEEK_TO_PREVIOUS, mediaItemIndex = 3, positionMs = 0L), + ) + } + + @Test + fun `previous naming another item goes back a track`() { + assertEquals( + SeekIntent.Previous, + intent(Player.COMMAND_SEEK_TO_PREVIOUS, mediaItemIndex = 2, positionMs = 0L), + ) + } + + /** + * Outside the queue media3 cannot tell the two apart — a one-item timeline has no previous + * item, so every previous arrives as position zero on index zero — so the same boundary is + * applied here against what the box says it has played. + */ + @Test + fun `previous outside the queue follows the elapsed position`() { + assertEquals( + SeekIntent.WithinTrack(0.0), + intent( + Player.COMMAND_SEEK_TO_PREVIOUS, + positionMs = 0L, + queuePosition = null, + elapsedSeconds = 40.0, + ), + ) + assertEquals( + SeekIntent.Previous, + intent( + Player.COMMAND_SEEK_TO_PREVIOUS, + positionMs = 0L, + queuePosition = null, + elapsedSeconds = 1.5, + ), + ) + } + + @Test + fun `a queue row tapped in a controller is a jump to that position`() { + assertEquals( + SeekIntent.ToQueuePosition(6), + intent(Player.COMMAND_SEEK_TO_MEDIA_ITEM, mediaItemIndex = 6, positionMs = 0L), + ) + } + + /** + * The crash-adjacent one: outside the queue, index 0 is the *playing song*, not queue + * position 0 — sending it to `playAt` would restart the album from its first track. + */ + @Test + fun `seeking to the only item of a fallback timeline stays inside the track`() { + assertEquals( + SeekIntent.WithinTrack(12.5), + intent( + Player.COMMAND_SEEK_TO_MEDIA_ITEM, + mediaItemIndex = 0, + positionMs = 12_500L, + queuePosition = null, + ), + ) + } + + @Test + fun `an ordinary scrub seeks inside the track`() { + assertEquals( + SeekIntent.WithinTrack(90.0), + intent(Player.COMMAND_SEEK_IN_CURRENT_MEDIA_ITEM, positionMs = 90_000L), + ) + } + + /** `C.TIME_UNSET` divided down is a seek to some 292 million years before the track began. */ + @Test + fun `an unset position is ignored rather than sent as a number`() { + assertEquals( + SeekIntent.Ignore, + intent(Player.COMMAND_SEEK_IN_CURRENT_MEDIA_ITEM, positionMs = C.TIME_UNSET), + ) + } + + @Test + fun `a negative position is clamped rather than sent`() { + assertEquals( + SeekIntent.WithinTrack(0.0), + intent(Player.COMMAND_SEEK_IN_CURRENT_MEDIA_ITEM, positionMs = -5_000L), + ) + } +} diff --git a/feature-media/src/test/kotlin/app/coilforphoniebox/media/TimelineUidTest.kt b/feature-media/src/test/kotlin/app/coilforphoniebox/media/TimelineUidTest.kt new file mode 100644 index 0000000..60aa3a9 --- /dev/null +++ b/feature-media/src/test/kotlin/app/coilforphoniebox/media/TimelineUidTest.kt @@ -0,0 +1,62 @@ +package app.coilforphoniebox.media + +import app.coilforphoniebox.domain.model.PlayerStatus +import app.coilforphoniebox.domain.model.QueueEntry +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotEquals +import org.junit.Test + +/** + * media3 identifies timeline items by UID: it diffs them position by position to tell a track + * change from a new playlist, and throws `"Duplicate MediaItemData UID in playlist"` out of + * `getState()` if one repeats. Both of those used to be reachable, because the playing item took + * its identity from `playerstatus` while its neighbours took theirs from the queue. + */ +class TimelineUidTest { + + private fun entry(position: Int, url: String, songId: String? = null) = + QueueEntry(position = position, url = url, title = url, songId = songId) + + @Test + fun `a queue id identifies the row`() { + assertEquals("42", uidFor(entry(0, "A/01.mp3", songId = "42"))) + } + + /** MPD did not send ids, so position has to separate two copies of one file. */ + @Test + fun `without a queue id the position separates identical files`() { + val first = uidFor(entry(0, "A/01.mp3")) + val second = uidFor(entry(1, "A/01.mp3")) + + assertNotEquals(first, second) + } + + /** + * The crash. The same file twice, and a status whose `songid` belongs to the *other* copy: + * building the playing item's uid from the status would repeat the id already used one row + * up. Taking it from the row it actually occupies cannot collide, whatever the status says. + */ + @Test + fun `the playing row keeps its own identity whatever the status reports`() { + val queue = listOf( + entry(0, "A/01.mp3", songId = "7"), + entry(1, "A/01.mp3", songId = "8"), + ) + val status = PlayerStatus(file = "A/01.mp3", playlistPosition = 1, songId = "7") + + val index = timelineIndexFor(queue, status) + assertEquals(1, index) + + val uids = queue.map { uidFor(it) } + assertEquals(uids.size, uids.toSet().size) + // The row's id, not the status's — which is the id of the copy above it. + assertEquals("8", uidFor(queue[index!!])) + } + + @Test + fun `the single item timeline falls back through what the status has`() { + assertEquals("9", fallbackUid(PlayerStatus(file = "A/01.mp3", songId = "9"))) + assertEquals("A/01.mp3", fallbackUid(PlayerStatus(file = "A/01.mp3"))) + assertEquals(CURRENT_ITEM_UID, fallbackUid(PlayerStatus())) + } +}