Add Snippets for notifications and picture-in-picture#905
Add Snippets for notifications and picture-in-picture#905alabiaga wants to merge 8 commits intoandroid:mainfrom
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 comprehensive notification snippets, including permission handling, channel management, and various styles such as BigPicture, Messaging, and Media using Media3. It also includes manifest configurations for notifications and Picture-in-Picture. Feedback highlights the need for proper Compose side-effect management to prevent resource leaks with ExoPlayer and MediaSession, the use of idiomatic Kotlin properties, and the correction of invalid XML comment syntax.
| @Composable | ||
| fun deleteNotificationId(channelId: String) { | ||
| val notificationManager = | ||
| LocalContext.current.getSystemService(Context.NOTIFICATION_SERVICE) as NotificationManager | ||
| notificationManager.deleteNotificationChannel(channelId) | ||
| } |
There was a problem hiding this comment.
The function deleteNotificationId performs a side effect (deleting a notification channel) directly in the composition body. In Compose, side effects should be triggered by events or wrapped in a LaunchedEffect to avoid executing them on every recomposition. Additionally, the function name is misleading as it deletes a channel, not an ID.
There was a problem hiding this comment.
added comment and modified method name
There was a problem hiding this comment.
Can you remove the @composable annotation then?
There was a problem hiding this comment.
modified it so that i can remove Composable
| val context = LocalContext.current | ||
| val bitmapImage = BitmapFactory.decodeResource( | ||
| context.resources, | ||
| com.example.compose.snippets.R.drawable.dog |
There was a problem hiding this comment.
Since com.example.compose.snippets.R is already imported, you can use the simple name R.drawable.dog for better readability.
| com.example.compose.snippets.R.drawable.dog | |
| R.drawable.dog |
| } | ||
|
|
||
| // [START android_notification_create_group_channel] | ||
| @Composable |
There was a problem hiding this comment.
Same thing here, doesn't seem like a compostable, if it is, include the LaunchedEffect
|
|
||
| @OptIn(UnstableApi::class) | ||
| @Composable | ||
| fun notificationStyles() { |
There was a problem hiding this comment.
Should any of these be composables? I think you only need context right, so perhaps these should be viewmodel background thread kicked off using AndroidViewModel to get access to context?
There was a problem hiding this comment.
Just used contextcompat and just passing in context. I am taking the examples from DAC so i want to make it as simple as possible.
| <application> | ||
| <!-- [START android_jpip_manifest_config] --> | ||
| <activity | ||
| android:name="com.example.compose.snippets.pictureinpicture.NavOrVideoCallJpipActivity" |
There was a problem hiding this comment.
Is the Jpip crucial for meaning here? Accidental name?
There was a problem hiding this comment.
did you mean to remove Jpip from NavOrVideoCallJpipActivity?
There was a problem hiding this comment.
Should these rather be vectors?
kkuan2011
left a comment
kkuan2011
left a comment
There was a problem hiding this comment.
looks good from a snippets repo perspective
| limitations under the License. | ||
| --> | ||
| <manifest xmlns:android="http://schemas.android.com/apk/res/android"> | ||
| <!-- [START android_manifest_config] --> |
There was a problem hiding this comment.
note: it's actually not required to put mainfest snippets in the repo, but up to you!
3 participants
Migrate hardcoded embedded code snippets to github snippets for notifications and Picture-in-Picture