ryanheise / audio_service

Flutter plugin to play audio in the background while the screen is off.
797 stars 479 forks source link

Use streams instead of callbacks for playback state and queue changes #18

Closed hacker1024 closed 5 years ago

hacker1024 commented 5 years ago

Is it possible to connect in two widgets at once, and have both receive callbacks? I have my queue viewing screen, and I also have a media control bar.

hacker1024 commented 5 years ago

Maybe allow us to subscribe to streams without calling connect again?

hacker1024 commented 5 years ago

After some research, it looks like streams are a good option. Look how @rxlabz's audioplayer sends the state and position.

Java side: https://github.com/rxlabz/audioplayer/blob/4358e7af6e4b91279dee6250f6afb06088a5cc43/android/src/main/java/bz/rxla/audioplayer/AudioplayerPlugin.java#L147-L162

Dart side: https://github.com/rxlabz/audioplayer/blob/4358e7af6e4b91279dee6250f6afb06088a5cc43/lib/audioplayer.dart#L80-L83

ryanheise commented 5 years ago

I think a streams-based API is a good idea and I definitely want to do that before the 0.1.0 release.

At the moment I am prioritising bug fixes and missing features, but let's keep this issue open to collect use cases and API suggestions.

hacker1024 commented 5 years ago

I'm writing my own plugin to test my streams idea, because it's vital to my app. I'm mostly copying your code, though. If all goes well, I should be able to make a pull request when I'm done.

hacker1024 commented 5 years ago

I've implemented it: #20 .

hacker1024 commented 5 years ago

At the moment I am prioritising bug fixes and missing features, but let's keep this issue open to collect use cases and API suggestions.

I'd classify this as a missing feature. You can do it in a normal Android app.

ryanheise commented 5 years ago

Thanks! It's looking good. There is a small technical issue and some naming suggestions below.

Technical issue: In the Android media session APIs, a playback state change encapsulates a state like playing and also the current playback position and current control capabilities (see PlaybackStateCompat. According to the docs, these are bundled together because they tend to change together. Right from the beginning, I knew that I'd eventually need to provide a class to encapsulate all of these for when I finally got around to a streaming API but I delayed it because I couldn't immediately think of two different names, one for the enum of states like playing and one for the class that encapsulates the "whole" set of fields that make up the playback state. Anyway, we should have such a class, and the stream should be based on it. It might even be a good idea to name this new class PlaybackState and then rename the existing enum to something else, although I'm not yet sure what that should be. (PlayState?, AudioServiceState?)

Naming suggestions: Instead of playingMediaItem, I would suggest currentMediaItem which is more consistent with the underlying native APIs. The Stream suffix on all streams is probably good. I haven't seen a standard on the naming of streams. The audioplayer plugin you referenced uses an onCurrentMediaItemChanged style of naming while the BLoC pattern tends to use the bold currentMediaItem as if to say "everything's a stream, no need to point it out in the name". Of these 3, I probably like the "Stream" suffix more but I'd be interested to do a quick survey to see what naming convention is more common and then conform to it.

hacker1024 commented 5 years ago

With the PlaybackState issue - I think the encapsulating class should be called PlaybackState, to match Android's APIs. I have no idea what to name the enum, though I noticed all the enum fields are to do with what kind of preparation state the audio's in - buffering, forwarding, etc, so maybe PreperationState?

I like currentMediaItem better, too. I didn't think of it. With the stream naming pattern, I find onCurrentMediaItemChanged to be a bit off-topic. Streams can be used to listen to, but it's not their only use. The name could easily be misleading in other use cases. I like the Stream suffix because I find it easier to read quickly and understand what's going on. I can see the argument for excluding it, though, as it is quite verbose.

I guess the naming choices are ultimately yours to make, unless you actually run a survey.

ryanheise commented 5 years ago

I've named the encapsulating class PlaybackState and renamed the old enum to BasicPlaybackState for want of a better term. I liked the Stream suffix so I've kept it in, and renamed playingMediaItemStream to currentMediaItemStream.

The example is updated to cancel the subscription to currentMediaItemStream and also to check if it's already subscribed when connecting. Since connect is called both in initState and in the case of AppLifecycleState.resumed, I suspect there is a case where they might both be called and you might get subscribed twice. I think the case was when I started the app remotely via flutter run while the screen was turned off and initState was called. Then I turned on the screen and the resume state was triggered (not that it would be easy for a user to do this).

hacker1024 commented 5 years ago

Why re-open the issue?

hacker1024 commented 5 years ago

I see in your example, if the subscription is already connected, you don't do anything. Wouldn't it be safer to cancel the old one and create a new one, just in case anything weird's happened to the old subscription?

ryanheise commented 5 years ago

I accidentally closed the issue earlier, but will close it now that code is committed.

In this example, the only weird thing that can happen to the subscription is that it is cancelled, and whenever the subscription is cancelled it is immediately set to null. Thus on analysis we can be certain that as long as the subscription is not null, it is valid to use. It would also be wasteful to cancel a good subscription to create a new one, so the if-condition here is just to detect a "double" connect and ignore the second one.

github-actions[bot] commented 2 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs, or use StackOverflow if you need help with audio_service.