search

faiface / beep

A little package that brings sound to any Go application. Suitable for playback and audio-processing.
MIT License
2.08k stars 152 forks source link

Close method #58

Closed avivklas closed 5 years ago

avivklas commented 5 years ago

Moved context and player close operation to their own separated public method

faiface commented 5 years ago

Have you tested if repeatedly closing and initializing the speaker works?

avivklas commented 5 years ago

Yes, I did. Using my application. My application is a music player in a smart speaker built with raspberry pi, and I wanted to release the device every time user clicks on the pause button. Using the code in the pull request and calling speaker.Close() when user clicks pause and speaker.Init() when user clicks play achieves exactly what I've wanted and properly handles the resource, I believe.

Anyway, the effect of calling speaker.Close() and then speaker.Init() is identical the effect of calling speaker.Init() multiple times in the same runtime, which worked before, although it's not the proper usage of beep.

I think that this method still bothers you probably because this library intended to support a use case of more finite and foreground application. My application is background application that runs as a service and this is why I needed this method. What do you think of my usage? Are you interested in improving the usage of this case? If so, I offer my help with designing and implementing a more robust api for this case at your guidelines. It currently works great for me, though, but maybe a better api for background services will make this library more appealing for applications that require tighter resource control.

faiface commented 5 years ago

Don't worry, I'm fine with this function :D. I was just pointing out a little bug that is there that doesn't cause anything visible, but here it is: player.Close() and context.Close() gets called multiple times. Why? Because you never set player to nil and so the condition if player != nil always passes. You inserted a condition on the channel, but it would be better to instead set the player to nil and remove the condition about the channel and setting it to nil.

Then I'll merge :)

avivklas commented 5 years ago

Fixed

faiface commented 5 years ago

Thanks! One more request, though, the doc is good, but is somewhat cumbersome and has some typos and missing article (a/the). If you don't wanna bother with that I can fix all that for you. Let me know! Sorry I didn't request it earlier, I was kinda busy with other things and I haven't noticed it.

avivklas commented 5 years ago

Done :)

faiface commented 5 years ago

Okay, this is good now :)

Thanks a lot for this and sorry if I appeared hostile at times, I didn't mean to.

avivklas commented 5 years ago

You didn't :) I'm happy using your library and now happier that it suites my needs better. Please feel free to ask me for additional help directly.