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

microphone support #740

Merged
merged 2 commits into from
Jun 20, 2020
Merged

microphone support #740

merged 2 commits into from
Jun 20, 2020

Conversation

remi-dupre
Copy link
Contributor

This is (apparently) the third attempt for #352.

Note that this is a duplicate of #371, which I didn't use since there was quite a lot of conflicts and some changes had to be done anyway. I took a similar approach so this may not be an issue.

Should I update the manpages? Running generate.sh seems to introduce ~1600 changes to the file right now :(

NOTE: The sound module is quite big, should we consider an attempt of splitting it in separate modules (eg: alsa, pulsoaudio, and generic module)? I could attempt to do this in this PR if you think this may be relevant.

@ammgws
Copy link
Collaborator

ammgws commented Jun 12, 2020

I haven't had a chance to look at the code yet, but does this address the comments from the previous PR about duplicating the client?

@remi-dupre
Copy link
Contributor Author

I haven't had a chance to look at the code yet, but does this address the comments from the previous PR about duplicating the client?

Yes, it's using a single client to take track of states for both sources and sinks.

@ammgws
Copy link
Collaborator

ammgws commented Jun 13, 2020

Can you update the docs too?

@remi-dupre
Copy link
Contributor Author

Can you update the docs too?

Done!

Also, it just made me realize that I totally forgot to support ALSA in this PR. In #371 it seems pretty short and straightforward, however this part seems to have slightly changed since. I currently don't have any setup to test/experiment with ALSA but I'll try to dive into it :)

@ammgws
Copy link
Collaborator

ammgws commented Jun 14, 2020

Pretty sure ALSA supports mic already - all you have to do is set name = "Capture" or whatever your mic shows up as in the output of amixer

@remi-dupre
Copy link
Contributor Author

Oh indeed, you're right!

@ammgws
Copy link
Collaborator

ammgws commented Jun 14, 2020

Can you fix the blocks.md conflict?

the sound module is quite big, should we consider an attempt of splitting it in separate modules (eg: alsa, pulsoaudio, and generic module)? I could attempt to do this in this PR if you think this may be relevant.

We can leave this for another PR, though I think some other blocks are similarly large.

@remi-dupre remi-dupre force-pushed the microphone-support branch 2 times, most recently from 23d1bfe to d840404 Compare June 14, 2020 15:27
@remi-dupre
Copy link
Contributor Author

Can you fix the blocks.md conflict?

✔️
It seems that some encoding issue appeared, I'm not sure how I introduced that

We can leave this for another PR, though I think some other blocks are similarly large.

Do you think it would be relevant to spend some time on it? I'd love to spend some time on this project and sometime (I found it a few days ago, it's awesome, thanks for your work btw!). Plus, I'm kind of a tidying nerd ^^'

Updating the manpages still generates a huge diff, am I missing something? Maybe this is just a pandoc version issue? (pandoc -v: 2.9.2.1)

@ammgws
Copy link
Collaborator

ammgws commented Jun 15, 2020

The encoding issue was due to another PR.

I think it would be better to spend time on end user features such as new blocks, but if you're interested in tidying up then it can't hurt to give it a go and see what we can come up with.

I'm not sure about the manpages as they were added by atheriel, perhaps you could check some past PRs and see if it was the same diff volume for them?

@remi-dupre
Copy link
Contributor Author

I'm not sure about the manpages as they were added by atheriel, perhaps you could check some past PRs and see if it was the same diff volume for them?

I think I was overthinking, it was just not udpated for some time and git seems to have a hard time dealing with this big update, many lines counted in the diff actually didn't change.

@ammgws
Copy link
Collaborator

ammgws commented Jun 20, 2020

Thanks!

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

Successfully merging this pull request may close these issues.

Support for PulseAudio sources in the sound block
2 participants