Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add mute and unmute formats for Volume and PulseVolume widgets #4779

Merged
merged 2 commits into from
Apr 30, 2024

Conversation

shyguyCreate
Copy link
Contributor

Rewrite of #4151

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for opening your first PR into Qtile!
If you have not heard from us in a while, please feel free to ping one of the devs or anyone who has commented on the PR, as sometimes things fall through the cracks.
You can also join the chat room for real-time discussion, see the community links.
For details on what PRs might need to include before we will merge them please see the docs.

@shyguyCreate
Copy link
Contributor Author

shyguyCreate commented Apr 26, 2024

Help. Can someone point me out how to get the PulseAudio volume value when mute, it always returns -1.

I can't find anything in pulse_volume.py.

EDIT: Found it.

libqtile/widget/volume.py Outdated Show resolved Hide resolved
@elParaguayo
Copy link
Member

Agree with @tych0 - this needs some work. Having get_volume() return a tuple of the volume level and whether the device is muted or not seems like the correct approach.

Copy link
Member

@elParaguayo elParaguayo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be a nice addition to the code. However, I'm marking this one as "request changes` for now because:
a) it needs changes as suggested by tych0
b) the volume widgets are likely to be used by a large portion of the userbase so I want to take a close look before this gets merged.

@elParaguayo
Copy link
Member

Why close this?

@shyguyCreate
Copy link
Contributor Author

Sorry I panic, I think I messed up the commit history.

@elParaguayo
Copy link
Member

Don't worry about that. Keep going!

@shyguyCreate
Copy link
Contributor Author

For the record, I did messed up the history right? Because I see 10+ commits that are not mine.

@shyguyCreate shyguyCreate reopened this Apr 28, 2024
@elParaguayo
Copy link
Member

If you look at this PR page you'll see it says it's only adding one commit. The other ones were probably because you did a merge to bring the branch in line with the current master. However, github knows those commits aren't needed here.

@shyguyCreate
Copy link
Contributor Author

Next time I have to update the master I'll ask before undoing everything.

@elParaguayo
Copy link
Member

Just rebase onto master rather than try to merge changes into the branch.

@shyguyCreate
Copy link
Contributor Author

Question: I have heard bad things about using --force on push, and after playing with git rebase and getting my commit on top, I really need to use --force with git push?

@elParaguayo
Copy link
Member

There are definitely times when you shouldn't use force (eg when pushing to a branch that other users are tracking and making changes to). However, for a PR here, we're happy for you to use force.

@shyguyCreate
Copy link
Contributor Author

All right, thanks

libqtile/widget/volume.py Outdated Show resolved Hide resolved
@tych0 tych0 merged commit 29e2a04 into qtile:master Apr 30, 2024
21 checks passed
@tych0
Copy link
Member

tych0 commented Apr 30, 2024

Thanks!

@shyguyCreate shyguyCreate deleted the vol_widget branch May 1, 2024 01:18
@elParaguayo
Copy link
Member

elParaguayo commented May 30, 2024

This broke the Volume widget. We had a method called mute (which is called by clicking on the widget) but this is overwritten by defining a variable called mute during __init__.

We need better tests!

@shyguyCreate
Copy link
Contributor Author

It should now be fixed in the new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants