-
-
Notifications
You must be signed in to change notification settings - Fork 481
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
API redesign - feedback needed #86
Comments
Thanks for asking for our feedback. |
Thanks @hacker1024 , that sounds reasonable. Happy to hear any more thoughts @rohansohonee , @itog , @Dekkee , @BeMacized , @alexelisenko Otherwise I'll go ahead with Option 2. |
Well. It looks like i didnt face your problems or just used lib in wrong way. Anyway first variant is worse due to more complexity with new method that could be redundant (in my case). Third seems nice because it eliminates entity that you should worry about. But it cant beat the second one because of its extensibility.
Well. I’ll vote for second. |
@ryanheise Option 2 seems to be reasonable |
Thanks to everyone for weighing in! I've implemented Option 2 based on the consensus and committed (f5d828e). Assuming nobody finds any problems, I'll release this to pub.dev tomorrow with an appropriate semantic version to indicate the breaking change. |
Version 0.4.0 is now published on pub.dev. |
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. |
Hi all. I'd like to do a quick poll on a proposed API change for the background task. This is motivated by some recent changes to Flutter's background Dart execution API, but what I have in mind may also just generally simplify the API.
Currently, the background task must call:
The run() method initialises the plugin, and this really should be the first method that you call within the background task. The problem is that in order to implement the various callbacks above, you need some shared state which needs to be created before run() is invoked, like this:
This means that in practice, run() isn't actually the first method called, and at least on the flutter dev channel, initialisation doesn't complete successfully. So below are the alternative proposals:
Option 1
Just insert an
init
method that you should call first, like this:Option 2
Change the argument of run() to be a builder:
And, redefine CustomAudioPlayer so that it implements a new interface BackgroundAudioTask and overrides the callbacks it needs, rather than the old style of composition. I originally went for composition since this seemed to be encouraged in the Flutter framework, but now that the project is more mature, it's clearer that inheritance probably solves the problem better. The interface would look like this:
Option 3
Following on from Option 2, we can go one step further and completely eliminate the AudioServiceBackground API which is a static-based API and replace it by BackgroundAudioTask which is instantiated, and where the constructor of this class serves the purpose of initialising the plugin. All of the static methods currently in AudioServiceBackground can now be inherited by the BackgroundAudioTask subclass. So now, your background task entrypoint would look like this:
CustomAudioPlayer();
Where this class extends BackgroundAudioTask and calls the super constructor, and provides overrides of the desired callbacks.
This is attractive in its simplicity, although unlike Option 2, it is theoretically possible for someone to write code in the initialiser list of their subclass constructor that tries to load other plugins before super() has had a chance to fully initialise things. But maybe that can just be addressed by documentation, and anyway, it is possible for a programmer to misuse the plugin in other ways (e.g. by inserting code above the one line above).
My thoughts
Here I'll try to taint the results of the poll ;-) I'd prefer Option 2 or Option 3 since Option 1 keeps around an old problem we've had where you have to implement all of the callbacks in the run() method which just redirect to some other custom class, and then define all of those callback headers again, which seems like a redundant step when this is the most common way that people will actually use the plugin.
The text was updated successfully, but these errors were encountered: