-
Notifications
You must be signed in to change notification settings - Fork 472
Add support to make AndroidForegroundService optional to MediaElement #2658
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.
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
base: main
Are you sure you want to change the base?
Add support to make AndroidForegroundService optional to MediaElement #2658
Conversation
Updated MediaElement to include AndroidForegroundServiceEnabled property for better control over media playback on Android. Adjusted related methods and classes to handle this new property, ensuring the foreground service is utilized appropriately during media playback. Removed unnecessary properties and streamlined the popup media element configuration.
bijington
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the general approach looks good. I am surprised it takes such little code change.
I think we should add an Is prefix to the property to follow typical Boolean naming conventions (e.g. IsAndroidForegroundServiceEnabled)
I understand that you wanted to have Android as the initial prefix here. Why did we do this for the AndroidViewType property in stead of making it a property that only exists for Android? Was it to simplify the code that needs to be written?
src/CommunityToolkit.Maui.MediaElement/MediaElementOptions.shared.cs
Outdated
Show resolved
Hide resolved
Renamed `AndroidForegroundServiceEnabled` to `IsAndroidForegroundServiceEnabled` across multiple files, including XAML, C#, and shared code. Updated the default value for the foreground service option to true in `MediaElementOptions`. Added unit tests to verify the default behavior and the ability to disable the foreground service setting. These changes improve clarity and maintainability by standardizing the naming convention.
I honestly have no idea how or when we came up with that. I am simply using the same convention used in texture view as it is has already been established and keeping it the same made sense to me. edit: I think it was so that we can use it in xaml. |
|
Do these code changes also remove the foreground service permission from the Android manifest file? That is the real problem, because that is what Google checks when you submit to the app store. |
|
Hi all, we just got rejected out app since the MediaElement control added this permission. We only use this control to display a video in the Splash screen. When can we get this new CommunityToolkit nuget version? In the meantime, we will appeal the rejection, however, this is just to see if they understand the situation. I would appreciate it if we could move a little faster. Thanks |
|
@Elinares-82 there's an easy work around for this in the meantime. You just need to remove like 3 lines of code from the MAUI project, build it, and then include the package you just built in your app instead of the Nuget package. It took me like an hour to get a version in my app without the foreground permission requirement. |
Thanks!, I will give it a try. |
I think a PR opened within a few hours is moving pretty quickly. If you would like things to move more quickly please free to get involved and help out |
Sure, give me the permissions and I click on Approve button. The Play Store is rejecting our apps since you decided to add permissions by default, why? Let the developers add or not the permissions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you added the property inside the MediaElement, how is the behavior if I have two media elements in different pages, one setting foreground to true and other to false, what will be the behavior? I didn't see code to tear down the ForegroundService if it changes to false in fly.
But I think it should only be something that you set on AppBuilder and only once
|
That I could see is prevent to start the service here |
The behavior if one media element is using the service and the other is not is that one will have access to the service and the other will not. This PR was meant to allow input and let ppl see how it could be done. It is marked as draft and I explicitly created it so I could gather feedback and make changes. I was hoping for this exact sort of input. Do we want to have it as init only and apply to any media elements that launch? Or do we want to allow each media element the option at runtime to decide? If one media element is already using the service and the second one is not using it it will not tear down the service. I do not believe that is possible without tearing down the media element using the service. The service is tightly integrated with media session and is as close to default implementation as I could create based on platform limitations. The only thing I could do to make it more like the google example is to move the Exoplayer Builder into the service itself. That was the original goal but do to various factors in the original media2 to media3 PR it never happened. I would prefer to keep this as simple as possible for maintainability while allowing developers the choice to do what they want. My goal here is to add the option to opt out of the service at runtime. Do you want it set in builder only? Or does anyone have any better suggestions? I am not looking at adding anything complex or doing something new. I simply want to add developer choice. Anyone who has an opinion please speak up. I still think of this as being in design phase. This PR was purely to allow ppl to see the idea I had for design. |
What if we can have both?, We can have an initial flag to disable it for all the media elements, and force with the new IsAndroidForegroundServiceEnabled to start individually? I do not know if this make sense. I the other hand, I know that each team have processes, I don't like to sound rude at all. I'd just like to know when this version might be coming out. Thanks, |
|
I was reading back through old discussions because I've been confused. I think there are two different issues being discussed. I'm going to call them issue 1 and issue 2 here. Issue 1 -- MediaElement should only turn on a foreground service if it is requested I think this PR will likely fix what I call issue 1, but it won't fix what I call issue 2. @ne0rrmatrix Is this PR intended to fix both issue 1 and 2? Or are you only trying to fix issue 1? @Elinares-82 I think you have an issue 2 problem (which is what I had when I joined these threads several months ago). Unless ne0rrmatrix says otherwise, I don't think this PR is going to fix your issue. |
|
@mrucker ty for the clarification. I will update the PR to address both issues. I appreciate all the help and input. |
|
(Also, I seem to be the one to blame for the confusion. Looking back through the discussion history, I hijacked discussion #2225, which was initially about issue 1, and started talking about issue 2 because I didn't understand the difference at the time. Sorry everyone.) |
You are totally right; I think this will not fix my issue. Google rejected our app since in the Manifest are several permissions like FOREGROUND_SERVICE_MEDIA_PLAYBACK and FOREGROUND_SERVICE, but we just display a promo video in the splash screen. We are stuck at this point. The only way that I can see is use an old version or use an alternative like Blazor component. |
|
Updating the service to address both issues will require the developer to add permissions manually to manifest and will only allow the service to run if permissions and foreground service are turned on. My questions is how do we want to deal with that. The reason we set the permissions ourselves was to fix the issue with constant bug reports where the solution was to simply add the service and intent filter to the manifest. |
Yes... That's kind of the problem we never solved in an old discussion about it... Which is why I've never offered my own PR... There didn't seem to be a great solution. |
I think, if this is documented all of us have to read and follow the guidance. For me is normal to add the permissions that the controls need. But, is there a possibility to disabled to add the permissions to the Manifest if the new Flag is turn it on? In a complex scenario use a project property to disable this in the source generator (I'm assuming that these permissions are being added using a source generator) |
It is easy to remove the permission requirements if you are able to build the MediaElement yourself and create your own Nuget Package. It's not ideal, but if you're really up against a wall it will allow you to release your app. I'd send you my team's custom Nuget package we've been using to remove this permission, but handing out code to strangers on the internet doesn't seem like a best practice 😄. |
@Elinares-82 there are tools that you allow you to edit the manifest on android app, so when you build your project and it generates the apk or aab you can unzip it, remove the permissions that you don't want and zip it again to ship into stores. I already did that for many apps that I worked and never had issues with that, and since are tools for it, I would say, isn't something unusual |
I think these are the lines that I have to comment out. I'm trying to use the generated "custom" DLL but I'm getting several compiler errors so I will try the @pictos suggestion. Thanks to all for your help and recommendations. |
|
Give me an hour or two and I will update this PR to address issues brought up. I have an idea of how we can do this and have everyone get what they want. |
|
@Elinares-82 Those two and I think you'd also need to get these two (it's been a while since I made the change) |
- Added a new service for media playback in `AndroidManifest.xml` with intent filters for media button actions and media session service. - Changed default value of `AndroidForegroundServiceEnabled` to `false` in `MediaElementOptions.shared.cs`. - Removed unnecessary permission attributes from `MauiMediaElement.android.cs`. - Updated test cases in `AppBuilderExtensionsTests.cs` to reflect the new default value for `AndroidForegroundServiceEnabled`.
|
I've been thinking about this and how we handle the breaking change for users - I know this breaks the existing pattern but what if we move the Boolean property from the options class and make it a parameter on |
I am happy to do that. Can you give me an example of what that looks like. I can't actually visualize or imagine what it is exactly that you mean. .UseMauiCommunityToolkitMediaElement(static options =>
{
options.SetDefaultAndroidViewType(AndroidViewType.TextureView);
options.SetDefaultAndroidForegroundService(true);
}) This is new behavior? Can you provide example of how you would suggest we change it? |
|
Yes of course - sorry I realise my statement was a little abstract. I am thinking this: .UseMauiCommunityToolkitMediaElement(
static options =>
{
options.SetDefaultAndroidViewType(AndroidViewType.TextureView);
},
enableForegroundService: true); |
That looks like a great idea. I will look into how to implement that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
@ne0rrmatrix did you have a look at this approach? |
|
@bijington I got sidetracked and forgot about this PR. I am looking at it now. |
No rush! I just thought I'd check as I saw the update from GitHub just now |
Renamed SetDefaultAndroidForegroundService to SetDefaultAndroidForegroundServiceEnabled for clarity and consistency. Made IsAndroidForegroundServiceEnabled a public static property in MediaElementOptions and updated its usage throughout the codebase. Improved documentation and updated all tests and usages to reflect the new method name. Added MediaElementOptionsTests to verify default/static configuration behavior. Refactored related logic in MediaManager.android.cs for clarity, and reformatted AndroidManifest.xml for readability. These changes enhance clarity, testability, and maintainability of MediaElement's Android Foreground Service configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
src/CommunityToolkit.Maui.MediaElement/Services/MediaControlsService.android.cs
Show resolved
Hide resolved
samples/CommunityToolkit.Maui.Sample/Platforms/Android/AndroidManifest.xml
Show resolved
Hide resolved
src/CommunityToolkit.Maui.MediaElement/Views/MauiMediaElement.android.cs
Show resolved
Hide resolved
|
@ne0rrmatrix sorry I am struggling to follow what the result of this PR will mean for developers. I see foreground now defaults to true but what does it mean for permissions? They will now need to add permissions in themselves which is fine. If a developer upgrades from a version to this one will the permissions fail early enough for a developer to catch it? |
The purpose of this PR is to allow users to opt out of of using Rich Media Notifications and not use a an android service. It would require everyone who currently uses MediaElement to start adding all the permissions and everything we were handling ourselves to make life easier. This PR is much requested as loads of developers have use cases where they neither want nor need a long lasting service or do they want to use notifications. I see many downsides to adding this. The only upside is it will clear out a few bug reports about being unable to disable the service. The big downside will be a lot of bug reports that will repeat many times over the next year or two. They will repeat and never go away. That is the down side. |
They will need to add permissions that are not currently added by us. It is complicated. It will fail unexpectedly otherwise. They need to add all the permissions we currently add. If they do not it will result in unexpected behavior depending on state of app. I have done nothing to mitigate that at this time. |
|
The issue with permissions is if the app is currently installed and it is upgraded it may not fail if permissions are not set correctly. That is a huge issue. There would be no error and no unexpected behavior. The app does not request permissions from user. It does not need to for rich media notifications. As for developer if they fail to include them it fails silently and the notifications are just not shown at all. |
|
Thanks. I completely agree with moving towards an opt-in approach that this PR brings. The bit that I want to cover is how seamless the upgrade path is to help developers and ultimately avoid introducing bug reports. We can add details to the release notes but that won't stop it entirely. I was just hoping there is something we could do at compile time to help here |
|
For developers that currently expect it to just work they might have issues. Everyone who uses default options will need to add all the permissions. Those that want to opt out will not have to add anything. The biggest issue we have faced with mediaElement is people not reading and/or following directions for permissions. After automating that the number of bugs went down to close to zero. Can we add an analyzer that checks for permissions in manifest and refuse to compile if they are missing? Maybe? Not sure if htat would work. But it is an idea I could look into? Maybe have analyzer check boolean for options and if it sees it in default or true then check if any permissions are missing? |
|
That might be a nice future addition, for now maybe we just make it clear in the extension method when a developer registers to use it in the foreground. I still think I prefer the idea of forcing developers to specify whether they want it foreground or background when initialising the toolkit as it covers th scenario of existing developers upgrading - they will receive a compiler error and be forced to understand why |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
| .UseMauiCommunityToolkitMarkup() | ||
| .UseMauiCommunityToolkitCamera() | ||
| .UseMauiCommunityToolkitMediaElement(static options => | ||
| { |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sample app sets the foreground service to disabled (false), which contradicts the purpose of demonstrating the feature to the community. For a sample application, it would be more appropriate to either enable the service (true) to show the full functionality, or add a comment explaining why it's disabled in the sample.
| { | |
| { | |
| // Foreground service is disabled by default in this sample to keep the setup simple. | |
| // Set this to true when you want to demonstrate the full Android foreground service behavior. |



Description of Change
Updated
MediaElementto includeAndroidForegroundServiceEnabledproperty for better control over media playback on Android. Adjusted related methods and classes to handle this new property, ensuring the foreground service is utilized appropriately during media playback. Removed unnecessary properties and streamlined the popup media element configuration.Linked Issues
PR Checklist
approved(bug) orChampioned(feature/proposal)mainat time of PRAdditional information
This PR is for the community to see how making android service optional could be implemented.
Builder OptionsMediaElement.shared.csMediaElementOptions