Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make channel ID and name configurable in DefaultMediaNotificationProvider #104

Closed
kevinguitar opened this issue Jul 4, 2022 · 3 comments
Closed

Make channel ID and name configurable in DefaultMediaNotificationProvider #104

kevinguitar opened this issue Jul 4, 2022 · 3 comments
Assignees
@christosts
Labels
enhancement

Comments

@kevinguitar
Copy link

Use case description

After integrating your DefaultMediaNotificationProvider with the MediaSessionService, I found that there are a few points worth to be discussed, but before jumping into them, I'd like to make sure that I understand the purpose of this class correctly.

Our use case is quite straightforward, displaying media notification for audio playback with custom actions, and the notification building/ bitmap handling in DefaultMediaNotificationProvider is exactly what I need, so I chose to extend this class, provide own BitmapLoader implementation, and override the member functions to achieve the custom actions rather than providing full-blown implementation from scratch.

Everything is working fine, but here are a few things that our team and I think would be nice to improve:

  • The Default naming prefix of the class made me think that we should provide our own implementation originally, but turns out it's quite flexible and we can just extend from it. Maybe a better naming is possible here?
  • Notification channel id and name are too generic and can't be overridden, we actually would like to provide translations for the channel naming if possible.
  • The only way to provide own playback icons is to override by the same resource name, we think that a proper API to override the resources will be a neater solution.

Please let us know your thought about those points, we'll be glad if any of them makes sense to you! Thanks as always.
@marcbaechinger marcbaechinger self-assigned this Jul 4, 2022
@marcbaechinger marcbaechinger changed the title Feature requests for DefaultMediaNotificationProvider Make channel ID and name configurable in DefaultMediaNotificationProvider Jul 4, 2022
@marcbaechinger
Copy link
Contributor

The naming comes from our convention to have an interface defined from a component like MediaNotification.Provider. Apps can provide their own or then use the default implementation provided by the library that is DefaultMediaNotificationProvider in this case. We try to make the default implementation as flexible and customizable as possible, so if we do a good job, most app don't need to implement their own can can go with the default. I don't think the naming schema is in question for these reasons.

You can override the drawables resource names for existing buttons as you already mentioned. The API allows you to do your own buttons by overriding DefaultMediaNotificationProvider.getMediaButtons. This way you can create your own CommandButtons for playback or custom commands as well. These buttons can have any resources you chose. Please note that any change you make for the MediaNotification.Provider should be reflected in the allowed commands of the session for Android 13 that is using a mini player based on the media session only (eg. hiding skip to next/previous). I don't think you can customize the icons of playback commands in the mini. player. So take a look at that to make sure your icons are consistent on for instance Android 12 (Notification with PendingIntents that allow customization of the icons) and Android 13 (mini player with standard icons defined by System UI).

Lastly, we should make the channel ID and the channel name configurable for the DefaultMediaNotificationProvider. I rename this issue to this as I think this is what we should provide out of the feature requests.

@kevinguitar
Copy link
Author

@marcbaechinger Thanks for the reply and tip!

The API allows you to do your own buttons by overriding DefaultMediaNotificationProvider.getMediaButtons

Sorry for not being clear, I actually meant the small notification icon (media3_notification_small_icon), as the createNotification function is final, the only way to provide the icon is to override it in the top-level module, it's working fine but just a bit implicit and hard to trace, do you think we can at least have an API to provide own small icon?

rohitjoins pushed a commit that referenced this issue Jul 21, 2022
@christosts @rohitjoins
Make DefaultMediaNotificationProvider more configurable
436ff6d 
Add a Builder to constructor DefaultMediaNotificationProvider. The
Builder can also set the provider's:
- notification ID
- notification channel ID
- notification channel name

The change adds an API for apps to set the small icon in notifications.

#minor-release
Issue: #104
PiperOrigin-RevId: 462111536
@christosts christosts mentioned this issue Jul 22, 2022
Open
@christosts
Copy link
Contributor

We changed DefaultMediaNotificationProvider so that

  • The notification ID, the channel name and channel ID can be set when constructing the instance. We moved construction to a builder to support this better.
    • Note that channel name must be provided as a string resource id, default is default_notification_channel_name.
  • Add a setter to configure the small icon.

The commit will appear here once it's pushed to the main branch. I'll close this issue but feel free to re-open if something did not work as expected.

microkatz pushed a commit that referenced this issue Nov 22, 2022
@christosts @microkatz
Make DefaultMediaNotificationProvider more configurable
62a2d76 
Add a Builder to constructor DefaultMediaNotificationProvider. The
Builder can also set the provider's:
- notification ID
- notification channel ID
- notification channel name

The change adds an API for apps to set the small icon in notifications.

#minor-release
Issue: #104
PiperOrigin-RevId: 462111536
(cherry picked from commit 436ff6d)
@androidx androidx locked and limited conversation to collaborators Mar 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@christosts christosts

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@marcbaechinger @kevinguitar @christosts