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

feat(notification): add sound player executable option #1301

Merged
merged 5 commits into from
Dec 10, 2020
Merged

feat(notification): add sound player executable option #1301

merged 5 commits into from
Dec 10, 2020

Conversation

warmar
Copy link
Contributor

@warmar warmar commented Dec 9, 2020

Description

Allows the default sound player to be overridden. I was having trouble on Ubuntu with the default player which sound-player chose.

Testing

Ran locally

New dependencies

None

@warmar warmar requested a review from jef as a code owner December 9, 2020 23:20
@warmar warmar changed the title Add sound player override feat: Add sound player executable option Dec 9, 2020
Pass the path to the player executable as a players array to play-sound so that it will check if the executable exists.
@jef
Copy link
Owner

jef commented Dec 10, 2020

Cool! Looks good to me. Quick question though, what sound player do you have? It seems like the supported libraries are sufficient, but maybe that's not the case.

@warmar
Copy link
Contributor Author

warmar commented Dec 10, 2020

Cool! Looks good to me. Quick question though, what sound player do you have? It seems like the supported libraries are sufficient, but maybe that's not the case.

By default, play-sound detects aplay, which is indeed installed on my system. When it plays a sound, however, all I hear is static, buzzing, and popping. Even when I run aplay from the command line, this happens. I have not yet been able to identify the problem.

For now, I just switched to using vlc and that has been working fine.

EDIT: Looks like installing sox (play) also resolved the issue. I can still see this option being useful on systems with several of the default supported players installed.

@jef
Copy link
Owner

jef commented Dec 10, 2020

Looks like installing sox (play) also resolved the issue. I can still see this option being useful on systems with several of the default supported players installed.

I also use sox on my system, so I never ran into that one before! Thanks for the clarification.

I agree, the default list is somewhat limiting and it also causes the behavior on Windows to post INFO: Could not find files for the given pattern(s).

See (somewhat) related: #1035

This will be good to keep in mind when we start to move to the new config.

Thanks for the great contribution!

@jef jef changed the title feat: Add sound player executable option feat(notification): add sound player executable option Dec 10, 2020
@jef jef merged commit 8d19231 into jef:main Dec 10, 2020
@warmar warmar deleted the sound-player-override branch December 10, 2020 02:03
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.

2 participants