Separate notification displaying from the service lifecycle

[nt4f04uNd]

Is your feature request related to a problem? Please describe.
Currently notification can be created if playing == true and processingState == idle.
But also currently the service will be stopped if playing == true and processingState == idle, removing the notification as well.

This makes it inconsistent and therefore hard to understand from the point of the user of this API, especially, because it's not documented.

Describe the solution you'd like
What we actually would like is to let processingState to manage the lifecycle of the service and playing and other parameters to update the look of the notification, because notification can exist without the service running, then also update the documentation accrodingly (#655)

Describe alternatives you've considered
No

Additional context
Extracted from discussion in #834

[nt4f04uNd]

Let's break down what use cases might exist, that might help to come up with what and how we should change

1. when create notification
   a. I want to create a notification without making the service foreground
   b. I want to start the playback and then create a notification for it (currently supported)
2. when stop service
   a. I want to stop the service, but keep the notification around and be able to update it. If service is destroyed (when there's no activity), pressing notification controls should start it again
   b. I want to stop the service and remove the notification (currently supported)
3. when service is not running (cases 1. a., 2. a.), I should be able to remove the notification

Now let's take a look of what set of properties in PlaybackState could support this, which seems to sufficiently cover all of them

1. a.

notificationCreated=null|false -> true, processingState=idle

1. b.

notificationCreated=any value (no-op), processingState=idle -> not idle

2. a.

notificationCreated=true, processingState=not idle -> idle

2. b.

notificationCreated=null|false, processingState=not idle -> idle

notificationCreated=true -> false, processingState=idle

What the new parameter should do

The notification is by default bound to processingState, i.e. it's created when processingState=not idle

• notificationCreated can be used to create and update fake notification (fake=when idle)
• notificationCreated=false is only for fake notification and should not do anything when processingState=not idle

Fixing edge cases and separating responsibilities of playing and processingState

The confusing part is that the service "starting" and stopping is spread along these two parameters. In quotes, because from the API user perspective it might look like service starts when notification is created, however in fact it's created as soon as any MediaBrowserCompat.ConnectionCallback connects. Here and further I mean by that the creation of the notification.

Let's separate responsibilieties of them and make processingState be a starter and stopper of the service, and all other parameters no-op (unless notificationCreated is true). That means playing doesn't do anything unless the processingState state is updated to not idle

• When it's not idle - playing still manages whether the service is foreground or not
• The service is stopped by idle

Other than a more clean API, that will as well ensure the user of it will pay close attention to both parameters, because now they have to run and stop the service with the processingState. Thus, it will exlude invalid states when processingState is always idle, but there's a notification that can't be removed when you stop, because it only happens when processingState changes from not idle to idle.

[ryanheise]

What do you mean by "fake" notification?

Just copying my comment from #849 :

I originally wanted to get rid of notificationCreated completely so that it would be possible to display the notification even before entering the foreground state. Maybe with !idle being used to determine whether updateNotification should show the notification rather than notificationCreated.

[nt4f04uNd]

What do you mean by "fake" notification?

By that I mean the notification created/updated when state is idle. That's the same notification, naming it like this was misleading, sorry.

Just copying my comment from #849 :

I think you didn't understand. This new notificationCreated parameter in PlaybackState has nothing to do with the variable we currently have on the native side. Please make sure to re-read what I propose it to do.

[ryanheise]

I didn't mean to imply that it was the same thing, in fact mine is quite a different approach but it seemed you wanted the discussion of my approach over here as well so that we can consider the different approaches together.

[nt4f04uNd]

Ok I see. It seems to me that with your approach 2. a. and 3 could not be accomplished, could they?

[ryanheise]

With 2a, I think I could support that via androidResumeOnClick = true. I think 3 should also be supported by setting the state to idle.

[nt4f04uNd]

I think 3 should also be supported by setting the state to idle.

How do you distinguish between 1a and 3? In both cases you have idle

With 2a, I think I could support that via androidResumeOnClick = true.

I mean, you could, but wouldn't notificationCreated make all of this more clear? It would have an imperative control over notification rather a vague collection of heuristic states and config parameters, that's hard to document and understand as a user.

[ryanheise]

I think 3 should also be supported by setting the state to idle.

How do you distinguish between 1a and 3? In both cases you have idle

Well I don't want to just map states to states. I would rather map state transitions. So inside the Android setState code it would always update the notification unless the state is idle. The moment the app decides it wants to show a notification, it should move out of the idle state. That won't in itself enter the foreground state, that will come later once the isActuallyPlaying condition becomes true.

With 2a, I think I could support that via androidResumeOnClick = true.

I mean, you could, but wouldn't notificationCreated make all of this more clear? It would have an imperative control over notification rather a vague collection of heuristic states and config parameters, that's hard to document and understand as a user.

I'm not necessarily against making androidResumeOnClick one of those ones we turn into a dynamic config option. At the moment there is too much static config in the config object, but I don't necessarily want to go too far in the other direction either and have too much dynamic config in the state object. So I'd first want to understand the use cases for it. In any case, the feature we're talking about is what the androidResumeOnClick option is for.

[nt4f04uNd]

I see it now. I don't like the fact that there's a moment where notification is shown but cannot be updated, i.e. when androidResumeOnClick=true, and we just became idle. Ideally, if we have a notification we should always be able to update it with mediaItem, for example.

Also, I don't like the name of androidResumeOnClick for what you want it to do, it looks like it was initially thought in mind to accomplish this https://developer.android.com/guide/topics/media-apps/mediabuttons#restarting-inactive-mediasessions, not keeping the notification around.

[ryanheise]

I see it now. I don't like the fact that there's a moment where notification is shown but cannot be updated, i.e. when androidResumeOnClick=true, and we just became idle. Ideally, if we have a notification we should always be able to update it with mediaItem, for example.

That's a fair point. Inside the Java code, the condition could be something like !idle || notificationCreated.

I'm also in favour of renaming androidResumeOnClick.

[ryanheise]

I forgot to mention that what I said above should not just consider setState but also setMetadata since that will also update the notification.

[nt4f04uNd]

Another thing is to remove this notification you need to set androidResumeOnClick=false go !idle and then idle again. Do you still feel like not adding notificationCreated?

[ryanheise]

Simple solution to that: notification is cancelled as soon as idle && !androidResumeOnClick.

[nt4f04uNd]

At this point both of our approaches seem to be pretty similar, SGTM then.

[ryanheise]

One thing I just want to be careful of with this change is that I don't cause a regression of #462 which I think is when I first introduced the Java notificationCreated variable. From memory, as soon as Android 11 came out, the plugin started failing because Android 11 didn't like calling notificationManager.notify before startForeground had occurred, even though it was working fine prior to Android 11. See 3f71779

[ryanheise]

Just thinking on this more, are you able to share the actual use case where you want to show the notification before entering foreground?

I want to ensure that whatever solution is adopted that the most common scenario has very little friction, and that would be to start showing the notification when entering foreground. Then a secondary consideration is how to ensure that other use cases can also be supported, even if they might have more friction than the common scenario.

I understand well the common scenario of most media apps which is that the notification only appears after the first time you actually start playing. Of course I can imagine other apps with special requirements, but I'm just curious to know what they are before making an API decision.

[ryanheise]

Maybe there is also a way to combine both approaches where by default the plugin tries to implement best practices for a media app by showing the notification on startForeground, but then in addition to that we can have another state parameter such as notificationCreated (or maybe notificationDisplayed) which is nullable. When it is null, we get the plugin managing the display of the notification, but when it's not null, you get to choose exactly when you want the notification shown.

[nt4f04uNd]

To be honest the intention behind this proposal was to make the lifecycle more clear. Adding new functionality, that is creating a notification beforehand the service, is a needed step to adopt the proposed model.

However, I don't have an actual use case to share with you, unfortunately. So it's a tradeoff between clarity (as it seems to me) of new parameter and a functionality that no one will probably ever use, but we can't know for sure. Supporting it however, is not a big hassle, so I would choose clarity.

That said, I don't mind if you want to keep the current model.