From d7fe5159405834f877e5b60bc0be2ce0692ce420 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 3 Jun 2026 10:49:32 -0400 Subject: [PATCH] fix(android): output picker CI - passive discovery + LongMethod Two failures on the slice's final dev tip: 1. OutputPickerController referenced MediaRouter.CALLBACK_FLAG_PASSIVE_DISCOVERY which doesn't exist in androidx.mediarouter 1.7.0 - the spec hallucinated it. Passive discovery is the default behavior when addCallback is called with no flag argument. Use the 2-arg overload for the init block and downgradeDiscovery; keep CALLBACK_FLAG_REQUEST_DISCOVERY for upgradeDiscovery. 2. NowPlayingBody grew to 82 lines after the Task 5 output-picker wiring (state collection + permission launcher + LaunchedEffect + conditional Sheet). Extracted the BLUETOOTH_CONNECT permission plumbing into rememberBluetoothPermissionState, the Column layout into NowPlayingContent, and the scrubber+transport pair (which share the smoothed playback position) into PlaybackControlsBlock. NowPlayingBody is back to ~34 lines and the new helpers each sit well under detekt's 60-line LongMethod cap. --- .../player/output/OutputPickerController.kt | 26 ++-- .../minstrel/player/ui/NowPlayingScreen.kt | 147 ++++++++++++------ 2 files changed, 116 insertions(+), 57 deletions(-) diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputPickerController.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputPickerController.kt index ba829d7a..8af4ce1f 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputPickerController.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputPickerController.kt @@ -24,11 +24,11 @@ data class RouteSnapshot( /** * Hilt-singleton facade over [MediaRouter]. Owns the callback - * lifecycle — passive discovery at process start (route list updates - * without forcing Bluetooth scans), upgrades to active discovery - * while the picker sheet is open so newly-paired devices appear - * promptly. The [routesState] StateFlow projects the current route - * + sorted available list as a [RouteSnapshot]; the picker + * lifecycle — default passive behavior at process start (route list + * updates without forcing Bluetooth scans), upgrades to active + * discovery while the picker sheet is open so newly-paired devices + * appear promptly. The [routesState] StateFlow projects the current + * route + sorted available list as a [RouteSnapshot]; the picker * ViewModel collects it. * * Mirrors the OutputPickerController role described in @@ -71,7 +71,12 @@ class OutputPickerController @Inject constructor( } init { - mediaRouter.addCallback(selector, callback, MediaRouter.CALLBACK_FLAG_PASSIVE_DISCOVERY) + // Two-arg addCallback registers with no discovery flag — + // androidx.mediarouter 1.7.0's default passive behavior: + // route-list updates flow through onRouteAdded/Removed/Changed + // without forcing Bluetooth scans. (There is no + // CALLBACK_FLAG_PASSIVE_DISCOVERY constant; absent flag = passive.) + mediaRouter.addCallback(selector, callback) } /** @@ -85,12 +90,13 @@ class OutputPickerController @Inject constructor( } /** - * Downgrade callback registration back to passive — call when the - * picker sheet closes so we don't keep Bluetooth scanning on for - * battery cost. Same idempotent re-registration semantics. + * Downgrade callback registration back to default passive behavior + * — call when the picker sheet closes so we don't keep Bluetooth + * scanning on for battery cost. Two-arg overload = no flag = + * passive. Same idempotent re-registration semantics. */ fun downgradeDiscovery() { - mediaRouter.addCallback(selector, callback, MediaRouter.CALLBACK_FLAG_PASSIVE_DISCOVERY) + mediaRouter.addCallback(selector, callback) } /** diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/ui/NowPlayingScreen.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/ui/NowPlayingScreen.kt index 11940581..4115a767 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/ui/NowPlayingScreen.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/ui/NowPlayingScreen.kt @@ -284,29 +284,41 @@ private fun NowPlayingBody( val outputViewModel: OutputPickerViewModel = hiltViewModel() val routes by outputViewModel.routes.collectAsStateWithLifecycle() val sheetVisible by outputViewModel.sheetVisible.collectAsStateWithLifecycle() + val permissionDenied = rememberBluetoothPermissionState(sheetVisible) + NowPlayingContent( + inner = inner, + state = state, + track = track, + navController = navController, + viewModel = viewModel, + trackActionsViewModel = trackActionsViewModel, + routes = routes, + isLiked = isLiked, + onChipTapped = outputViewModel::onChipTapped, + ) + if (sheetVisible) { + OutputPickerSheet( + snapshot = routes, + permissionDenied = permissionDenied, + onRouteSelected = outputViewModel::onRouteSelected, + onDismiss = outputViewModel::onSheetDismissed, + ) + } +} - // One-shot BLUETOOTH_CONNECT permission flow. The launcher is - // remembered across recompositions; the LaunchedEffect fires - // exactly once when the sheet opens for the first time per - // composition. Subsequent opens are no-ops; the system remembers - // the user's choice. - val context = LocalContext.current - var permissionDenied by remember { mutableStateOf(false) } - val permissionLauncher = rememberLauncherForActivityResult( - contract = ActivityResultContracts.RequestPermission(), - ) { granted -> - permissionDenied = !granted - } - LaunchedEffect(sheetVisible) { - if (sheetVisible && - ContextCompat.checkSelfPermission( - context, - Manifest.permission.BLUETOOTH_CONNECT, - ) != PackageManager.PERMISSION_GRANTED - ) { - permissionLauncher.launch(Manifest.permission.BLUETOOTH_CONNECT) - } - } +@Composable +@Suppress("LongParameterList") // mirrors NowPlayingBody — pure layout wiring +private fun NowPlayingContent( + inner: androidx.compose.foundation.layout.PaddingValues, + state: com.fabledsword.minstrel.player.PlayerUiState, + track: com.fabledsword.minstrel.models.TrackRef, + navController: NavHostController, + viewModel: PlayerViewModel, + trackActionsViewModel: TrackActionsViewModel, + routes: RouteSnapshot, + isLiked: Boolean, + onChipTapped: () -> Unit, +) { Column( modifier = Modifier .fillMaxSize() @@ -341,37 +353,78 @@ private fun NowPlayingBody( Spacer(Modifier.height(8.dp)) DeviceChip( route = routes.current, - onClick = outputViewModel::onChipTapped, + onClick = onChipTapped, modifier = Modifier.align(Alignment.CenterHorizontally), ) } Spacer(Modifier.height(4.dp)) - val smoothPositionMs by rememberSmoothPositionMs( - positionMs = state.positionMs, - durationMs = state.durationMs, - isPlaying = state.isPlaying, - ) - ScrubberRow( - positionMs = smoothPositionMs, - durationMs = state.durationMs, - onSeek = viewModel::seekTo, - ) - Spacer(Modifier.height(4.dp)) - TransportRow( - isPlaying = state.isPlaying, - onPrev = viewModel::skipToPrevious, - onPlayPause = { if (state.isPlaying) viewModel.pause() else viewModel.play() }, - onNext = viewModel::skipToNext, - ) + PlaybackControlsBlock(state = state, viewModel = viewModel) } - if (sheetVisible) { - OutputPickerSheet( - snapshot = routes, - permissionDenied = permissionDenied, - onRouteSelected = outputViewModel::onRouteSelected, - onDismiss = outputViewModel::onSheetDismissed, - ) +} + +/** + * Scrubber + transport pair, sharing the smoothed playback position. + * Extracted from [NowPlayingContent] to keep that body under detekt's + * LongMethod ceiling — the two rows belong together (the scrubber's + * smoothed position would otherwise need to be hoisted into the + * caller just to thread it into the row below). + */ +@Composable +private fun PlaybackControlsBlock( + state: com.fabledsword.minstrel.player.PlayerUiState, + viewModel: PlayerViewModel, +) { + val smoothPositionMs by rememberSmoothPositionMs( + positionMs = state.positionMs, + durationMs = state.durationMs, + isPlaying = state.isPlaying, + ) + ScrubberRow( + positionMs = smoothPositionMs, + durationMs = state.durationMs, + onSeek = viewModel::seekTo, + ) + Spacer(Modifier.height(4.dp)) + TransportRow( + isPlaying = state.isPlaying, + onPrev = viewModel::skipToPrevious, + onPlayPause = { if (state.isPlaying) viewModel.pause() else viewModel.play() }, + onNext = viewModel::skipToNext, + ) +} + +/** + * One-shot BLUETOOTH_CONNECT permission flow tied to picker-sheet + * visibility. The launcher is remembered across recompositions; the + * LaunchedEffect fires when the sheet opens and the permission is not + * already granted. Subsequent opens are no-ops once the user has + * answered — the system remembers their choice. Returns whether the + * user explicitly denied, so the sheet can surface the rationale row. + * + * Extracted from [NowPlayingBody] to keep that body under detekt's + * LongMethod ceiling — the permission plumbing is incidental to the + * player layout. + */ +@Composable +private fun rememberBluetoothPermissionState(sheetVisible: Boolean): Boolean { + val context = LocalContext.current + var permissionDenied by remember { mutableStateOf(false) } + val permissionLauncher = rememberLauncherForActivityResult( + contract = ActivityResultContracts.RequestPermission(), + ) { granted -> + permissionDenied = !granted } + LaunchedEffect(sheetVisible) { + if (sheetVisible && + ContextCompat.checkSelfPermission( + context, + Manifest.permission.BLUETOOTH_CONNECT, + ) != PackageManager.PERMISSION_GRANTED + ) { + permissionLauncher.launch(Manifest.permission.BLUETOOTH_CONNECT) + } + } + return permissionDenied } private fun shouldShowChip(snapshot: RouteSnapshot): Boolean {