djcas9 / komanda

The IRC Client For Developers
http://komanda.io
MIT License
1.78k stars 75 forks source link

Sound Notifications #136

Closed AlexKvazos closed 10 years ago

AlexKvazos commented 10 years ago

Hey there!

Hello, I have to say that I am so happy to send this pull request. This is the first time I contribute to a GitHub project by adding a new feature. I am used to doing non-significant stuff and such.

This PR implements sound notifications to the client. Komanda does have notifications for OS X however they don't seem to work on when the app is built. I asked some people if it was a good idea to add sounds and it seems like it is.

Sound Settings The sounds can be toggled. I know that most people like to just chill on channels and they don't want to be bugged all the time, so a sounds tab on the settings panel will let you turn off some or all sounds.

screen shot 2014-06-25 at 4 07 09 am A screenshot of the sound settings on their default values

Testing I develop using an OS X machine running Mavericks. This feature should be compatible with any platform supported by node-webkit but do let me know if it doesn't work for you. The client and the sounds work perfectly while using the grunt run task or using the built standalone.

Cheers!

Developers...

Adding sounds is dead easy. Let me give you a quick walkthrough of how to do so.

1.- Your sound files should be in .ogg format and should go inside the app/sounds folder. Remember its file name.

2.- Go into main.js Line 137. This is the function that will add all the sounds into the ionSound plugin. Put the name of your file WITHOUT the extension in the sounds array. Followed by a colon and the volume level as a single decimal number. from 1.0 (max) all the way down to 0.0 (min). screen shot 2014-06-25 at 4 17 37 am This is how the object looks like as of today Jun 25, 2014

3.- Now to play any sound just run the following:

$.ionSound.play("sound_name");

"sound_name" should be on the sound array I showed you above. That is pretty much it.

4.- (NOTE) - I added a listener to main.js called "komanda:soundnotification". I run this event when messages are sent and here I check if they have that sound enabled to play it or not. This function should be self-explainable.

Special thanks To everyone on the Komanda IRC channel that helped me get RequireJS working. Including the library was somewhat painful. At least I now know how to do it on future contributions.

ionDen's plugin: Ion.Sound Project can be viewed here: https://github.com/IonDen/ion.sound Licensed under: MIT The library was modified a little bit so that it would use only ogg files so that it would work with node-webkit.

AlexKvazos commented 10 years ago

Sorry for the loooong comment list. I explained the code I added. Cheers!

xqwzts commented 10 years ago

Great PR btw, thanks for going into all the details and explaining it all.

Minor suggestion: When you say you've modified the ionSound lib to work with node-webkit, can you keep track of what exactly you had to change? [maybe here in the PR or as a comment in the code] That way if the lib needs to be updated at some future point it would be easy to replay the modifications and keep it compatible with Komanda.

Even more minor suggestion: Your commit notes were helpful and concise, and they did a much better job of explaining the code in the commits than I would have understood by reading it myself. Would you consider adding them as short comments in the code itself? Specifically these comments: 1, 2, 3, 4, 5 are the type that I would find useful in the code when reading through it for future enhancements or fixes. That said I'm not sure what the project's comment policy is, if there is one, maybe @mephux can chime in here.

AlexKvazos commented 10 years ago

ion.sound.js now has what it original had on lines 43 - 48 but I commented it out. It is an if statement that was evaluating to true when it shouldn't be. Then on L#140, that array had a default sound, it got removed too.

AlexKvazos commented 10 years ago

And the comments you mentioned are now comments in the actual files.

AlexKvazos commented 10 years ago

Note After testing I realized that for sounds to work you have to build the client. Running it with grunt's run task will not work and a console message will display that the file was not found.

eugene-bulkin commented 10 years ago

That's an acceptable issue in my opinion. End-user won't be using grunt run but will be running binaries.