Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces remote media session support for Wear OS, adding a new ViewModel to manage media sessions and a skeleton activity. Several critical issues were identified in the implementation, including a type mismatch in the session flow, compilation errors caused by calling suspend functions within non-suspend lambdas, and potential memory leaks due to unreleased MediaController instances. Additionally, there are unused properties in both the ViewModel and the Activity that should be addressed.
| @RequiresApi(Build.VERSION_CODES.CUR_DEVELOPMENT) | ||
| private val sessionFlow: Flow<List<MediaSession.Token>> = callbackFlow { | ||
| // [START android_wear_remote_media_session_get_sessions] | ||
| trySendBlocking(mediaSessionManager.getActiveSessionsForPackage(application.packageName)) |
There was a problem hiding this comment.
The initial emission of sessionFlow has a type mismatch. getActiveSessionsForPackage returns a list of platform MediaController objects, but the flow expects List<MediaSession.Token>. You should map the controllers to their session tokens and handle potential nullability of the manager and its result.
| trySendBlocking(mediaSessionManager.getActiveSessionsForPackage(application.packageName)) | |
| trySendBlocking(mediaSessionManager?.getActiveSessionsForPackage(application.packageName)?.map { it.sessionToken } ?: emptyList()) |
| private val controllerFlow: Flow<MediaController?> = | ||
| sessionFlow | ||
| .distinctUntilChanged() | ||
| .map { tokens -> | ||
| tokens.map { | ||
| SessionToken.createSessionToken(application, it).await() | ||
| } | ||
| } | ||
| .map { tokens -> | ||
| tokens.map { MediaController.Builder(application, it).buildAsync().await() } | ||
| } | ||
| .mapNotNull { controllers -> controllers.firstOrNull() } | ||
| .flatMapLatest { controller -> controller.controllerEventFlow.map { controller } } |
There was a problem hiding this comment.
This block contains a compilation error and a memory leak.
- Compilation Error: The
await()function is a suspend function and cannot be called inside the non-suspend lambda ofIterable.map(lines 61 and 65). - Memory Leak:
MediaControllerinstances are created for all tokens but only the first is used, leaking the others immediately. Furthermore, the used controller is never released.MediaControllermust be released to avoid leaking system resources.
Consider refactoring to process only the first token and manage the controller's lifecycle using callbackFlow and awaitClose.
private val controllerFlow: Flow<MediaController?> =
sessionFlow
.distinctUntilChanged()
.flatMapLatest { tokens ->
val token = tokens.firstOrNull() ?: return@flatMapLatest kotlinx.coroutines.flow.flowOf(null)
callbackFlow {
val sessionToken = SessionToken.createSessionToken(application, token).await()
val controller = MediaController.Builder(application, sessionToken).buildAsync().await()
val listener = object : Player.Listener {
override fun onEvents(player: Player, events: Player.Events) {
trySendBlocking(controller)
}
}
controller.addListener(listener)
trySendBlocking(controller)
awaitClose {
controller.removeListener(listener)
controller.release()
}
}
}| // CHANGED: A private property to hold the current controller instance. | ||
| private var currentController: MediaController? = null |
There was a problem hiding this comment.
The property currentController and its associated comment are unused and should be removed to maintain code cleanliness.
|
|
||
| class RemoteMediaSessionActivity : ComponentActivity() { | ||
|
|
||
| private lateinit var viewModel: RemoteMediaActivityViewModel |
There was a problem hiding this comment.
The viewModel property is initialized but never used in the activity. Consider using it to observe the media state or remove it if this is just a skeleton activity.
|
|
||
| defaultConfig { | ||
| applicationId = "com.example.wear" | ||
| minSdk = 33 |
There was a problem hiding this comment.
Can you change this minSdk to reflect the right version? This will remove the SDK version check in the activity
| sourceCompatibility = JavaVersion.VERSION_21 | ||
| targetCompatibility = JavaVersion.VERSION_21 | ||
| } | ||
| useLibrary("wear-sdk") |
There was a problem hiding this comment.
Should this be a code snippet to be documented in DAC?
| </intent-filter> | ||
| <!-- [END android_wear_remote_media_session_manifest] --> | ||
| <intent-filter> | ||
| <action android:name="com.google.wear.services.media.action.REMOTE_MEDIA_ACTIVITY" /> |
There was a problem hiding this comment.
Why not including this in the code snippet?
2 participants
No description provided.