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 sound.volumeStep config option #20020

Closed
wants to merge 1 commit into from
Closed

add sound.volumeStep config option #20020

wants to merge 1 commit into from

Conversation

dermetfan
Copy link
Member

@dermetfan dermetfan commented Oct 31, 2016

Motivation for this change

Issue #17068

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • OS X
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

allows user to configure the amount by which volume is increased/decreased by sound.enableMediaKeys

allows user to configure the amount by which volume is increased/decreased by `sound.enableMediaKeys`
@mention-bot
Copy link

@dermetfan, thanks for your PR! By analyzing the history of the files in this pull request, we identified @oxij, @edolstra and @nbp to be potential reviewers.

@edolstra
Copy link
Member

edolstra commented Oct 31, 2016

I had not noticed the addition of sound.enableMediaKeys (57ab189) but IMHO that sort of thing belongs in the desktop environment, not in the global NixOS configuration. It's not even clear how this interacts with e.g. PulseAudio, multiple sound devices, per-application volume settings etc. So let's not add to it.

@dermetfan
Copy link
Member Author

@edolstra Fair point. sound.enableMediaKeys does not work if PulseAudio is enabled, at least not without hardware.pulseaudio.systemWide = true or other extra work, because amixer is run as root and cannot connect to the user's PulseAudio daemon. That took me a while to figure out when I worked on this change, so even though it's disappointing, I understand your opinion.

@oxij
Copy link
Member

oxij commented Nov 1, 2016

Eelco Dolstra notifications@github.com writes:

... IMHO that sort of thing belong in the desktop environment, not in
the global NixOS configuration.

I disagree. IMHO media keys should work from bare ttys (because they
can, also what about all the headless "music box" PCs (I happen to have
one)?).

It's not even clear how this interacts with e.g.PulseAudio, multiple sound devices, per-application volume settings etc. So let's not add to it.

I agree, however, that the name is misleading in that context. I should
have named it sound.alsa.enableMediaKeys and implemented
sound.alsa.config (which would put stuff into /etc/asound.conf).

PulseAudio

That code is for alsa, as described above.

multiple sound devices

It does the right thing by controlling the Master volume of the default
sound device. People with multiple audio cards should configure their
alsa.confs anyway.

per-application volume settings

Not supported in alsa, AFAIK.

vcunat pushed a commit that referenced this pull request Nov 2, 2016
joachifm pushed a commit that referenced this pull request Nov 5, 2016
Taken from #20020.

(cherry picked from commit 6f17cb1)

Otherwise, the patch in 77d8b46 fails
to apply (see e.g., https://hydra.nixos.org/build/43141220/nixlog/1/raw)
@dermetfan
Copy link
Member Author

How about this example configuration.nix then? For backwards compatibility, let's make sound.enableMediaKeys an alias to sound.alsa.mediaKeys.enable.

sound.alsa.mediaKeys = {
  enable = true;
  volumeStep = "1";
};

Also, I would note the incompatibility between PulseAudio and sound.alsa.mediaKeys.enable in the documentation of hardware.pulseaudio.enable so other's don't run into the problem I described in #20020 (comment).

@oxij
Copy link
Member

oxij commented Dec 22, 2016 via email

@dermetfan dermetfan closed this Dec 30, 2016
@dermetfan dermetfan deleted the patch-1 branch December 30, 2016 19:50
@dermetfan dermetfan restored the patch-1 branch December 30, 2016 19:50
@dermetfan dermetfan deleted the patch-1 branch December 30, 2016 19:52
@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
adrianpk added a commit to adrianpk/nixpkgs that referenced this pull request May 31, 2024
Taken from NixOS#20020.

(cherry picked from commit 6f17cb1)

Otherwise, the patch in 77d8b46 fails
to apply (see e.g., https://hydra.nixos.org/build/43141220/nixlog/1/raw)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants