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

Fix sound player ui #37

Closed
wants to merge 2 commits into from

Conversation

mrducnguyen
Copy link
Contributor

Thanks for this sounds package! It's very useful.

I ran into an issue when using SoundPlayerUI in one of my app.

When the sound is playing, and navigator happens (i.e. back to previous screen), the sound would keep on playing in the background.

Assertion error is thrown on dispose(), but it was not the culprit (I asked a question here to flutter team). I found the handling of async/await in SoundPlayerUI is not consistent and could hide more issues than the one I ran into.

Before I recognised, I ended up rewrote all .then().catchError() into normal try... catch... with async/await :P

This fixed the issue that sound left playing in background for me.

I ran into another issue when I paused the player, switch to another app, then switch back, click resume, ERR_PLAYER_IS_NULL was thrown from the platform invocation. Will try to look at it later

…ed in sound kept playing in the background

- Employ async/await syntax for consistency and avoiding hidden errors

- Handle only handle-able exceptions. Let other errors bubble up

- Use single quote instead of mixing single/double quotes
Copy link
Owner

@bsutton bsutton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some concerns around the pause/play/resume methods.
Specifically you have removed the calls to setState when the _playerState is change.
From recollection these calls were important as the forced a rebuild of the ui to reflect the state change.
I've accepted them for the moment but will require additional testing.
Can you comment on why you removed them?

I also have some concerns around the play/stop/pause/resume methods now being awaited. Part of the original intent was to have these return immediately incase the os is slow to respond. We are currently looking to improve the event model (onStopped, onStarted, on Paused ...). One idea is that when you click a 'stop' button it doesn't change the UI state until the 'onStopped' method arrives from the OS. This is somewhat problematic as you probably don't want the user spamming the stop button. The idea is to try to have the UI always reflect the OS's state rather than our assumed state. This will need more thought.
In the mean time I accepted your awaits.

bsutton added a commit that referenced this pull request Sep 8, 2020
@bsutton
Copy link
Owner

bsutton commented Sep 8, 2020

Due to the merge conflicts I've manually merged this PR, so whilst I'm closing the PR it has been merged.

Thanks for the contribution!

If we don't get to pushing a new release today it will happen early next week.

@bsutton bsutton closed this Sep 8, 2020
@mrducnguyen
Copy link
Contributor Author

The reason I removed setState for _playerState

  • _playerState is already a setter which in turn make call to setState

For the await/async model, SoundPlayerUI support play/resume only, there will be no "race condition" since Dart is a single-threaded (well, some people would claim that this is wrong because you can spawn Isolate, which is similar to Thread in Java/C++, but also is very different, see below for a video explaining this term beautifully from Flutter).

If user's spamming the play/pause button, multiple await _player.resume() or await _player.pause() will be placed on the event queue, and they will be resolved asynchronously down to the Android MediaPlayer (and I assumed the same for iOS)

Isolates and Event Loops

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.

2 participants