search

doublesymmetry / react-native-track-player

A fully fledged audio module created for music apps. Provides audio playback, external media controls, background mode and more!
https://rntp.dev/
Apache License 2.0
3.18k stars 981 forks source link

fix(android): android notification removal flag when mediasession focus changed #2181

Closed lovegaoshi closed 8 months ago

lovegaoshi commented 8 months ago

I started noticing in my app using nightly, the notification when media is not played will get removed when app enters background. This is caused by stopForeground is set to always remove notification when ForegroundGracePeriod expires. this fix reverts the removeNotificationWhenNotOngoing flag in stopForeground introduced in b13a92208eaf9607a12cfa7596dfa6ae4f4a4c8e and fixes my issue.

puckey commented 8 months ago

@dcvz @lovegaoshi nice catch – looking over this code again, should we remove AudioPlayerState.ERROR from REMOVABLE_STATES? Since if you press play again, it will retry loading.. And currently it will just remove the notification if it reaches this state. The same goes for AudioPlayerState.STOPPED – you could press play again, for example.

dcvz commented 8 months ago

@dcvz @lovegaoshi nice catch – looking over this code again, should we remove AudioPlayerState.ERROR from REMOVABLE_STATES? Since if you press play again, it will retry loading.. And currently it will just remove the notification if it reaches this state. The same goes for AudioPlayerState.STOPPED – you could press play again, for example.

I wondered but didn’t want to try to make such a critical change leading up to v4 release. I also know you were testing extensively there and thought I might be missing something.

puckey commented 8 months ago

@dcvz @lovegaoshi nice catch – looking over this code again, should we remove AudioPlayerState.ERROR from REMOVABLE_STATES? Since if you press play again, it will retry loading.. And currently it will just remove the notification if it reaches this state. The same goes for AudioPlayerState.STOPPED – you could press play again, for example.

I wondered but didn’t want to try to make such a critical change leading up to v4 release. I also know you were testing extensively there and thought I might be missing something.

Yes, this code is based on https://github.com/Automattic/pocket-casts-android/blob/ee8da0c095560ef64a82d3a31464491b8d713104/modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/PlaybackService.kt#L218 – but looking at it again, I think we can diverge from it.. In any case, I removed it from the fork I am using for RG, because I want users to be able to press play again if a stream stops loading due to a connection error or loss of internet..

dcvz commented 8 months ago

I think there's a balance here. We /are/ supposed to make sure we go out of foreground when we're not actively playing media (according to Google docs). The grace period was introduced to allow library users to still be able to take action whenever this is supposed to happen. If we disable it for error and also stop, we're limiting when the player would actually do that..

I also get the benefit of just leaving it so the user is able to press play again confidently any time afterward. But now the service is just endlessly in the foreground state. I think if the user hits stop, it is a good indication that playback is meant to stop.. and in combination with the grace period, if they change their mind shortly after, they can still resume.

For errors it's trickier. User likely doesn't know an error happened (unless you have some kind of audio cue). If they notice playback stopped, they're likely to press play again a short time after that - so I think a good grace period might still be able to handle that.