nicklan / pnmixer

Volume mixer for the system tray
GNU General Public License v3.0
152 stars 32 forks source link

display channel in tooltip #88

Closed landroni closed 9 years ago

landroni commented 9 years ago

Another minor request. Currently pnmixer displays Volume: 57%. As other mixers do (again xfce4-mixer panel plugin), it would be useful to display which channel for the current card is being used by pnmixer. E.g.: Master: 68%

From the context, it should be obvious that it's the volume level.... Otherwise, to know which card channel is in use the user must dig into Prefs > Devices.

hasufell commented 9 years ago

What about Master Volume: 57%? Or is that too verbose? I could take care of that I guess. Doesn't look like a major change.

elboulangero commented 9 years ago

Thinking of it, PNMixer may use another soundcard than the card selected in preferences (if card is not available, or unplugged at runtime). So displaying also the card name would make sense, as well as channel name. Maybe on two lines ? Or would it be too much ?

landroni commented 9 years ago

Master Volume: 57% could work, I think.

landroni commented 9 years ago

@elboulangero

Actually this is a point I've been thinking, as it's kinda missing in the xfce4-mixer plugin. One way to do it, as you seem to suggest, would be:

 HDA Intel PCH (Master)
 Volume: 57%

Maybe make it an option to include card (channel) info...

elboulangero commented 9 years ago

Yep, I'm not very keen on adding an option (too much work if you ask me), just adding it like that would seem fine to me, I don't think that would disturb anyone. At least, it wouldn't disturb me, I didn't even know there was a tooltip in the first place...

landroni commented 9 years ago

I also think this would work. And it's definitely useful in cases of multiple soundcards. I guess you can always release it like this and wait to see if anyone ever bothers to complain... :)

hasufell commented 9 years ago

check branch display-channel-tooltip, more specifically commit f1c9d3f097d9e0a528e9296a59bd9eae883fa119

elboulangero commented 9 years ago

Hmm, there's something wrong with this commit: you check for the selected card and the selected channel. But in some situation, PNMixer may operate on another card than the one selected, and maybe another channel. So here, we want to display the active card, and also the active channel.

This is really related to the work I did in the branch https://github.com/nicklan/pnmixer/tree/usb-dac-segfault-on-hotplug. Part of the problem is already solved in this branch, since there are some functions to retrieve the active stuff: alsa_get_active_card() and alsa_get_active_channel(). If you base your branch on usb-dac-segfault-on-hotplug, then I think you just need to replace your get_selected... calls by alsa_get_active..., remove free(), and that's it.

elboulangero commented 9 years ago

Also, regarding the tooltip string, I like the suggestion of @landroni:

 HDA Intel PCH (Master)
 Volume: 57%

The info provided here covers the fact that PNMixer may operate on another card than the one selected in config, so the info (card name and channel) is useful for an user with a USB soundcard that is not always connected. What do you think ?

hasufell commented 9 years ago

This is really related to the work I did in the branch https://github.com/nicklan/pnmixer/tree/usb-dac-segfault-on-hotplug.

I didn't have a look there yet, so I had no idea. We should merge this first then to master. Basing a feature branch off of a hotfix branch will cause confusion.

landroni commented 9 years ago

Looks good. I'm wondering if maybe this arrangement would work better: (Master) Volume: 57%

This way there is clear visual separation for user, it's obviously clear what Master stands for (channel), and that 57% corresponds to volume level. I know this all will ultimately depend on who has final say, so just a thought...

PS I based my comments on the commit, ignoring the recent spate of comments....

elboulangero commented 9 years ago

@hasufell Branch usb-dac-segfault-on-hotplug merged in to master !

hasufell commented 9 years ago

check out 479120e4d7398d9df81e262134239f262c68b6b7

landroni commented 9 years ago

I'm having trouble with pulling GIT:

git pull (6389) U src/callbacks.c Pull is not possible because you have unmerged files. Please, fix them up in the work tree, and then use 'git add/rm ' as appropriate to mark resolution, or use 'git commit -a'. git pull (6389) returned '1'

hasufell commented 9 years ago

yes, I force-pushed. recreate your branch

landroni commented 9 years ago

Sorry, not that good with GIT. How do I recreate the branch?

hasufell commented 9 years ago
git checkout master
git branch -d display-channel-tooltip
git checkout -b display-channel-tooltip
git pull origin display-channel-tooltip
landroni commented 9 years ago

I managed to

git branch -D display-channel-tooltip

But all other commands fail:

geek@liv-inspiron:~/Build/Devel/pnmixer$ git checkout master
src/callbacks.c: needs merge
error: you need to resolve your current index first

geek@liv-inspiron:~/Build/Devel/pnmixer$ git checkout -b display-channel-tooltip
src/callbacks.c: needs merge
error: you need to resolve your current index first
hasufell commented 9 years ago

git checkout -f master

landroni commented 9 years ago

Still issues:

geek@liv-inspiron:~/Build/Devel/pnmixer$ git checkout -f master
Already on 'master'
Your branch is behind 'origin/master' by 46 commits, and can be fast-forwarded.
  (use "git pull" to update your local branch)

geek@liv-inspiron:~/Build/Devel/pnmixer$ git checkout -b display-channel-tooltip
src/callbacks.c: needs merge
error: you need to resolve your current index first

And git pull still fails.

elboulangero commented 9 years ago

I suggest you just clone a new repo and then checkout the branch, except if you really enjoy fighting with git ;)

landroni commented 9 years ago

Good call!

landroni commented 9 years ago

I think this looks good... It's definitely useful.

elboulangero commented 9 years ago

:+1:

hasufell commented 9 years ago

merged