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.

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

Adds androidStopForegroundOnCompleted to AudioServiceConfig which will stop foreground service when AudioProcessingState == AudioProcessingState.completed #1054

Open
wants to merge 16 commits into
base: minor
Choose a base branch
from

Conversation

skiluk
Copy link

@skiluk skiluk commented Dec 15, 2023

This PR adds a AudioServiceConfig parameter androidStopForegroundOnCompleted which will stop foreground service when AudioProcessingState == AudioProcessingState.completed. I added this as a Feature Request as an app we are working on requires a solution like this.

We have an app that uses multiple audio files to play to the user in one stream of audio. Throughout this process we pause/unpause so the androidStopForegroundOnPause causes issues when playing in background as we do not have permissions to startForeground from the background. If we turn set androidStopForegroundOnPause to false, the notification is not hidden when the audio is completed or the user removes from the task list.

Using the feature added in this PR would allow the notification to be dismissed when the audio is completed.

See the feature request issue here:
#1053

Pre-launch Checklist

  • I read the [CONTRIBUTING.md] and followed the process outlined there for submitting PRs.
  • My change is not breaking and lands in minor branch OR my change is breaking and lands in major branch.
  • If I'm the first to contribute to the next version, I incremented the version number in pubspec.yaml according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change (format: * DESCRIPTION OF YOUR CHANGE (@your-git-username)).
  • I updated/added relevant documentation (doc comments with ///).
  • I ran dart analyze.
  • I ran dart format.
  • I ran flutter test and all tests are passing.

@ryanheise
Copy link
Owner

as we do not have permissions to startForeground from the background.

Have you had a look at #932 (comment) >

@skiluk
Copy link
Author

skiluk commented Dec 16, 2023

Hi Ryan, yes thanks. I have looked at all of the threads regarding this issue (I believe). We do not think the battery optimization workaround is the best approach for us though we considered it. For our application, it seems better to control when stopForeground being called when the audio is complete or stopped rather than changing the battery optimization level so we can start/stop the foreground service whenever we want. We do not really need to start/stop foreground service during playback, we just have audio process with multiple pauses though for the user, it is one continual audio stream. Hope this makes sense.

@ryanheise
Copy link
Owner

It would also be possible to use the current implementation as is and just emit a !playing state from your app to trigger when you want to stop foreground, but I'll have a think about the various combinations that need to be supported. It may turn out that we don't need to support all combinations since some of them are meaningless. E.g. Perhaps stopForeground should always be called from the completed state.

@skiluk
Copy link
Author

skiluk commented Dec 17, 2023

Thanks, Ryan. Yes, I agree that would be another solution. I added the parameter option in this PR in case others do not want stopForeground when audio is completed. Thanks for taking the time to think through this.

@skiluk
Copy link
Author

skiluk commented Dec 19, 2023

I may have misunderstood your response. In the current implementation emitting a !playing state does NOT trigger stopForeground. We would need adding that code to a non-playing state, such as the completed state. The PR implements it in the completed state with a parameter to turn that off/on.

@ryanheise
Copy link
Owner

I may have misunderstood your response. In the current implementation emitting a !playing state does NOT trigger stopForeground.

It should if you use the default config parameter stopForegroundOnPause.

@skiluk
Copy link
Author

skiluk commented Dec 19, 2023

Right. That is what I was explaining above. Our audio pauses/unpauses throughout playback. We don't want to keep stopping foreground for each but only when it is complete. We run into exceptions for not having permissions to start foreground while in background.

Could be just implementing stopForeground when state is completed.

@ryanheise
Copy link
Owner

Yes, it would be nice if it turns out that it always makes sense to stopForeground on completed.

The question is whether anyone has any use case where, for some reason, they don't want to stopForeground on completed, although it feels right that logically the pause state is similar to the completed state in this context.

@skiluk
Copy link
Author

skiluk commented Feb 19, 2024

Hi @ryanheise, if you have a specific reservation about this PR, I'd be happy to address it. I believe these changes will help others who are running into this same issue where the app requires stopForeground on audio complete (not on pause). I'm open to making any necessary adjustments.

@monoblaine
Copy link

Hey @skiluk, I was looking for a solution to this problem and came across your PR. I tried your patch on a phone with Android 8 and the result was positive: The media notification disappeared on completed state. However nothing changed on a phone with Android 14. Maybe @ryanheise was (#462 (comment)) right: One does not simply dismiss a media notification on Android.

@Colton127
Copy link

Thank you for this. The ability to control the lifecycle of the audio service is a must, given that we can no longer resume the service without direct user input.

I made a fork of this branch with a few minor modifications that I believe better align with best practices: https://github.com/Colton127/audio_service

AudioProcessingState.idle: Stop foreground service and remove notification. It successfully removes the notification on Android 14 and Android 8, but not 11, which I think may not be possible.

AudioProcessingState.completed: Stop foreground service, but keep notification active, allowing users to restart playback at a later time through the notification.

I believe the lifecycle of the service should mirror that of Spotify and YouTube: The service remains active until the notification or app is swiped away by the user.

@ryanheise
Copy link
Owner

Hi @Colton127 , I would be interested to review the diffs of what changed, although I see your diff contains a lot of formatting changes which unfortunately makes it difficult to review.

In any case, the problem with all of the various proposals is that they are all different. This means that each person may have their own preferred way that it would work. So what I'm thinking is that the plugin should provide a sensible default, but then should also provide direct access to the underlying stopForeground API so that anyone who needs flexibility can call it exactly when they want, and disable the androidStopForegroundOnPause option.

By the way @skiluk , I think you can already emulate this today without any code changes by setting androidStopForegroundOnPause: false and then whenever you've finished playing the last media item in your queue, set the processing state to idle which will effectively call stopForeground. So the only difference between this and my potentially exposing the stopForeground method for you to call directly when you want is that perhaps the latter would allow also exposing the different flag parameter options. I suspect you don't need those, however, and so setting the processing state to idle should suffice.

Regarding the androidStopForegroundOnPause option, this is supposed to be the sensible default, and there is an opportunity with the next major release to make some changes to this if we think it's current behaviour is not enough. I think it is sensible to automatically call stopForeground on pause and idle, as it currently does, but also on complete and on error, which it does not currently do. So if I were to make a change to the default behaviour, it would be to rename this option to something more inclusive, and make it stopForeground on pause/idle/complete/error.

Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants