mokasin / apw

Small and simple Awesome WM widget to control volume of Pulseaudio.
37 stars 33 forks source link

Theme integration and mixer launching #5

Closed alandmoore closed 10 years ago

alandmoore commented 10 years ago

Hi, thanks for writing apw, it's great.

I created two small additions:

If you like them, feel free to merge.

mokasin commented 10 years ago

A few remarks:

Thx for your contribution! Good ideas.

alandmoore commented 10 years ago

Sorry, that was a very sloppy pull request. I had left those functions in there from a previous false start on the color customization.

I've removed them and cleaned up the history.

I guess closing the app may be redundant, I just like the "popup" type interface like tray widgets tend to do. It also keeps 15 copies of pavucontrol from coming up if you keep clicking the widget.

Would it be better to just check if the mixer is already running to just run one copy?

On 12/15/2013 04:00 PM, mokasin wrote:

A few remarks:

  • Please remove the "deleted temp files" commit (for example using git rebase -i)
  • Are the /pulseWidget.set_foo_color()/ functions really necessary? Seems to be enough, that you can change colours through /beautiful/ variables?
  • To close the mixer app from the widget is IMHO kinda redundant. Awesome seems to be quiet able to close windows ;).

Thx for your contribution! Good ideas.

— Reply to this email directly or view it on GitHub https://github.com/mokasin/apw/pull/5#issuecomment-30622988.

alandmoore commented 10 years ago

Maybe the best approach on the mixer is just to make the mixer command configurable with a method call and simply run it without the pkill stuff. If you like that idea, I can code that and make a new pull request.

On 12/15/2013 04:00 PM, mokasin wrote:

A few remarks:

  • Please remove the "deleted temp files" commit (for example using git rebase -i)
  • Are the /pulseWidget.set_foo_color()/ functions really necessary? Seems to be enough, that you can change colours through /beautiful/ variables?
  • To close the mixer app from the widget is IMHO kinda redundant. Awesome seems to be quiet able to close windows ;).

Thx for your contribution! Good ideas.

— Reply to this email directly or view it on GitHub https://github.com/mokasin/apw/pull/5#issuecomment-30622988.

mokasin commented 10 years ago

Sounds good. Would be great if you could refactor it down to two clean commits: One for theming, one for the mixer spawning. Thx.

mokasin commented 10 years ago

At the moment the one commit adds those functions and the second removes them again.

mokasin commented 10 years ago

I'm sorry, but there is no :SetMixer() method :). Or I would be totally blind.

Two pull requests might be nice. You could achieve that with cherry picking: create a new branch, cherry pick one commit and delete it in the former branch and register the new branch as a pull request.

But it's such a small change, that I'm willing to accept the request anyway this time.

alandmoore commented 10 years ago

Sorry, just not very good at this git thing yet. I know I wrote the function, somehow it just didn't end up in the commit.

alandmoore commented 10 years ago

All the commits should be correct now, and I've updated the documentation as well to reflect the new options.

mokasin commented 10 years ago

Thx, merging right now.

mokasin commented 10 years ago

For the record, it would have been nice, if you'd rebased to current master. But it's okay this time ;).

mokasin commented 10 years ago

Okay, I'm closing this manually, since Github didn't recognised the rebased merge automatically.

alandmoore commented 10 years ago

Sorry for all the trouble; this is the first time I've collaborated on github. It's been educational.

Thanks for your patience.

mokasin commented 10 years ago

No problemo.