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

Let mopidy mute at app-level the audio #512

Closed
wants to merge 10 commits into from
Closed

Let mopidy mute at app-level the audio #512

wants to merge 10 commits into from

Conversation

txomon
Copy link
Member

@txomon txomon commented Sep 17, 2013

Create a playback interface to mute the music (maintaining before volume), doing it at app level (not system-wide). Also created the mpd binding, by using the default audio output as the main audio output.

@kingosticks
Copy link
Member

Have you seen this pulseaudio bug https://bugzilla.gnome.org/show_bug.cgi?id=672401? I don't use pulseaudio myself but I don't think the fixed version of gstreamer0.10-plugins-good (0.10.32) has made it to the debian package repos etc yet. Just something to maybe watch out for.

@txomon
Copy link
Member Author

txomon commented Sep 17, 2013

Yeah, sharp stuff to care about... Need to research further about it thought

@adamcik
Copy link
Member

adamcik commented Oct 6, 2013

Fixup the test failures, and the non-software mixer cases and this is ready to go in.

@@ -37,8 +39,9 @@ def outputs(context):

Shows information about all outputs.
"""
ena = 0 if context.core.playback.get_mute().get() else 1
Copy link
Member

Choose a reason for hiding this comment

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

Write the variable name out completely: enabled

@jodal
Copy link
Member

jodal commented Oct 6, 2013

I'd like some tests as well, on both the MPD and core part. The audio part is OK as is.

@@ -13,7 +13,8 @@ def disableoutput(context, outputid):

Turns an output off.
"""
raise MpdNotImplemented # TODO
if int(outputid) == 0:
Copy link
Member

Choose a reason for hiding this comment

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

Is outputid really always 0? (I've no idea, just seems odd).

EDIT: Yes it seems it is (because we now only support one output, I'm guessing).

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I just took 0 as reference because it is hardcoded up there, so I just suppose that 0 is correct. A future implementation with several outputs should be changing this. Now atm is valid.

@txomon
Copy link
Member Author

txomon commented Oct 7, 2013

@jodal I have finally done nearly all the changes, still needed:

  • get/set mute should be switching between app/mixer level
  • more tests to be done

I am sorry to say that about the tests, I have no time atm for them, so I would truly appreciate if someone could make some, else, this will take some time on the future.

@jodal
Copy link
Member

jodal commented Oct 7, 2013

This is small enough that I'm willing to add the tests myself just to get it merged :-)

@txomon
Copy link
Member Author

txomon commented Oct 7, 2013

Don't contain yourself! I have 5 hours of class now...

@ghost ghost assigned jodal Oct 7, 2013
@txomon
Copy link
Member Author

txomon commented Oct 8, 2013

I have been reviewing the code, and I didn't do the app/mixer differentiation because I don't know how to do mixer level mute! Anyway, I will push now one commit preparing it

@adamcik
Copy link
Member

adamcik commented Oct 8, 2013

See #512 (comment) and #512 (comment)

@adamcik
Copy link
Member

adamcik commented Oct 8, 2013

s/adam/thomas/ Adamcik is a last name, a lot of people seem to be mixing this up lately. :-)

@jodal
Copy link
Member

jodal commented Oct 9, 2013

I've now posted a new PR #529 that polishes this PR into something that should be mergeable.

@adamcik
Copy link
Member

adamcik commented Oct 10, 2013

Closing in favour of jodal's PR which wraps up the missing tests etc.

@adamcik adamcik closed this Oct 10, 2013
@jodal
Copy link
Member

jodal commented Oct 10, 2013

Merging #529 would eventually have closed this one as well, as its a strict superset of this 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

4 participants