Add direction field to sound block to enable microphone control #371
Conversation
| @@ -673,12 +901,19 @@ impl Block for Sound { | |||
| if name.as_str() == self.id { | |||
| match e.button { | |||
| MouseButton::Right => self.device.toggle()?, | |||
| MouseButton::Left => | |||
There was a problem hiding this comment.
What's the reason for moving away from spawn_child_async() here?
There was a problem hiding this comment.
That looks like an artifact from my previous pull request. I think the version on the master is superior.
There was a problem hiding this comment.
Can you then rebase your pull request?
| "volume_full" => " VOL ", | ||
| "volume_half" => " VOL ", | ||
| "volume_empty" => " VOL ", | ||
| // This icon has no spaces around it because it is manually set as text. (sound.rs) | ||
| "volume_muted" => "MUTED", |
There was a problem hiding this comment.
Can you explain the reason for changing this? And adding the crossed icon?
There was a problem hiding this comment.
To separate the icon from the volume percentage field. The output icons start with "volume" and the microphone icons with "mic". The "crossed" icon was previously called "volume_muted" but only affects the percentage field.
The name "crossed" is more generic and since the sound field is now responsible for sound volume and microphone gain I thought that would fit better.
|
Looks much better! I left a few comments for you. |
|
Do you agree with my comments? |
There was a problem hiding this comment.
Hi, first of all: great work!
I'm the original author of the pulseaudio support, so I know the pain working with pulseaudio.
I've looked over your PR and it looks overall good, but I found some parts that could be simplified.
As the pulseaudio code is very complex, I'd like to keep it as simple as possible.
Also the Cargo.lock shouldn't be included (if not necessary) to ease the merge.
If you struggling with git feel free to ping me.
| } | ||
|
|
||
| #[cfg(feature = "pulseaudio")] | ||
| lazy_static! { | ||
| static ref PULSEAUDIO_CLIENT: Result<PulseAudioClient> = PulseAudioClient::new(); | ||
| static ref PULSEAUDIO_CLIENT_SINK: Result<PulseAudioClient> = PulseAudioClient::new(Direction::Output); | ||
| static ref PULSEAUDIO_CLIENT_SOURCE: Result<PulseAudioClient> = PulseAudioClient::new(Direction::Input); |
There was a problem hiding this comment.
Is there a specific reason why you duplicate the client?
I think one should be enough, this should keep the already pretty high complexity a bit lower. The only overhead we get with one client would be requesting both default sink/source at startup and indistinguishable events, which we already have for all sinks / sources.
| self.volume(sink_info.volume); | ||
| self.muted = sink_info.mute; | ||
| } | ||
| } |
There was a problem hiding this comment.
Merging the PulseAudioSinkInfo and PulseAudioSourceInfo struct would enable some code deduplication here.
| self.text.set_text(format!("{:02}%", volume)); | ||
| self.text.set_state(State::Idle); | ||
| } | ||
| } |
There was a problem hiding this comment.
The code can be deduplicated when the match self.direction is only used for selecting the icons.
| @@ -673,12 +901,19 @@ impl Block for Sound { | |||
| if name.as_str() == self.id { | |||
| match e.button { | |||
| MouseButton::Right => self.device.toggle()?, | |||
| MouseButton::Left => | |||
There was a problem hiding this comment.
Can you then rebase your pull request?
| |_| { } | ||
| ); | ||
| } | ||
| } |
| format!("pulseaudio connection failed with error: {}", err), | ||
| )) | ||
| } | ||
| } |
|
Hey @SWW13, sorry for the late reply, unfortunately I don't have much time at the moment. Still, I'd like to merge this feature somehow, would you mind helping me out? |
|
I'm also a bit short on time, maybe I find time for it in the next weeks. |
|
Would be awesome if this feature would be implemented. I'm currently learning Rust, so I probably cannot help out myself, but maybe someone finds a little time in the near future. |
|
Anyone still working on this? @fedario ? |
It would be nice if someone could help me merge this, since it works so far. |
|
Seems like deduplicating the client is the only major hurdle left |
|
Superceded by #740 |
This is the second attempt of this: #352
This pull request:
Edit: Closes #277.