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

Attempt at getting an async audio player working #36

Merged
merged 4 commits into from
Sep 11, 2023
Merged

Conversation

bijington
Copy link
Collaborator

Fixes #25

@jfversluis
Copy link
Owner

Looks good to me! Simple but effective :D

@bijington
Copy link
Collaborator Author

I can't decide if this is the best API surface for the async part. Currently we could use the player as follows:

CancellationTokenSource cancellationTokenSource;

public async Task Play()
{
    await asyncAudioPlayer.PlayAsync(cancellationTokenSource.Token);
}

public void Stop()
{
    cancellationTokenSource.Cancel();
}

Is this fine or should we consider providing a Stop method that would call cancellationTokenSource.Cancel(); in the AsyncAudioPlayer?

@jfversluis
Copy link
Owner

Hm yeah I do think I prefer to (also) have a stop method in the player.

One doesn't rule out the other, right? You can still use the CancellationToken manually if that's what you want, or you call Stop and we will do it for you.

@bijington bijington marked this pull request as ready for review September 9, 2023 22:32
@bijington
Copy link
Collaborator Author

@jfversluis I think this is ready for a review now. One thing I wasn't completely sure on was the lack of properties on AsyncAudioPlayer like Volume, etc. but users can pass in an IAudioPlayer into the ctor to allow them to customise bits if they wanted.

@jfversluis
Copy link
Owner

One thing I wasn't completely sure on was the lack of properties on AsyncAudioPlayer like Volume, etc. but users can pass in an IAudioPlayer into the ctor to allow them to customise bits if they wanted.

Yeah let's see if people can figure it out or not, or maybe add a little bit of documentation around that?

@jfversluis
Copy link
Owner

Thanks for making this happen ✨

@jfversluis jfversluis merged commit 748f960 into main Sep 11, 2023
2 checks passed
@jfversluis jfversluis deleted the async-playback branch September 11, 2023 08:31
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.

Play() should be declared as async as it is its default behavior.
2 participants