search

Beluki / GaGa

A minimal radio player for the Windows Tray.
35 stars 13 forks source link

Implemented Volume Control #1

Closed joshimoo closed 10 years ago

joshimoo commented 10 years ago

Still getting used, to github and this was the first time that I used gitflow. I hope I done this correctly if not please let me know.

I tried to stay with your code style and make my code look like yours. If you have any thoughts on the volume implementation please let me know.

Best regards Joshua Moody

Implemented a basic Volume Control

Added a method to clamp an input value between min/max

Minor Refactoring:

Beluki commented 10 years ago

Hi. Thanks for contributing!

I tried to stay with your code style and make my code look like yours.

From a quick look, your code looks perfect in both style and semantics. Thanks for following the current style, it helps to keep everything clean. Refactoring is fine too. The only thing I would change is to keep the actual volume as a double in the menu items .Tag property instead of parsing them on each click.

Implemented a basic Volume Control

At first, I wanted to implement the volume control by scrolling the mouse wheel over the icon, but it turns out that the notification area doesn't receive WM_MOUSEWHEEL messages. The only way would be low-level hooks and I haven't decided yet whether that's worth the trouble.

I'll probably release a new GaGa in a few days. If I merge this, would you mind if I list your name/nick in a Contributors file under the Documentation folder?

joshimoo commented 10 years ago

Volume Control:

I replaced the volume parsing in the commit 38fdda7 with this lambda.

new MenuItem("10%", (sender, args) => player.ChangeVolume(0.1)),
new MenuItem("25%", (sender, args) => player.ChangeVolume(0.25)),

Did you look at the first commit 762f5f1 or did I mess up the commit order? Would you prefer to you use the original approach instead but using the Tag property to hold onto the double?

Contribution:

would you mind if I list your name/nick in a Contributors file under the Documentation folder?

No problem, I would be happy about that =)

For the future:

I was planning on implementing global Hotkeys, by hooking the message loop, Would that be okay for you, or is this something you are already working on?

Thank you for your feedback and the great software =)

Beluki commented 10 years ago

I replaced the volume parsing in the commit 38fdda7 with this lambda.

Ooops, I missed that. I only looked at the first commit (before the lambdas). That's way better than the double parsing.

I was planning on implementing global Hotkeys, by hooking the message loop, Would that be okay for you, or is this something you are already working on?

For the next release I'll merge the volume control and change a few small things here and there (e.g. allow to use a different "streams.ini" file on startup). I'll probably allow muting the player at any time too, instead of only while playing (so that the volume control integrates with muting and we can put a "0%" or "muted" in the menuitems).

I think its best to wait until this next release is done to add new features.

That said, if we want to start adding things such as global hotkeys, it might make sense to have an additional configuration file that keeps track of whether they are enabled/disabled. This could be handy for other things, such as the last radio station played or the last volume level. I still haven't decided whether I want this or not.

For the time being, feel free to experiment with hooking. Even without the configuration file, it can be handy for keyboards that have multimedia keys because the play, mute and stop keys in those keyboards could mimic the mouse left, middle and right click. The best way to go about it would be probably a tiny isolated class or library (e.g. mINI-style).

Thank you for your feedback and the great software =)

It makes me happy that other people use it too. If they even contribute to it, then it's awesome. =)

joshimoo commented 10 years ago

it might make sense to have an additional configuration file allow to use a different "streams.ini" file on startup

Sounds good, would you want to work together on this after the next release? I will experiment with the Hot keys and wait for the new release to add additional features,

Beluki commented 10 years ago

Sounds good, would you want to work together on this after the next release?

I think we should discuss further features/releases on IRC, at this point we are using this pull request as a chat anyway. I'm usually on Freenode (same nick as here: "Beluki").

Beluki commented 10 years ago

Merging this, as I'm going to start work on the next release soon. Thanks!