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 Mute command #253

Merged
merged 6 commits into from
Sep 22, 2023
Merged

Add Mute command #253

merged 6 commits into from
Sep 22, 2023

Conversation

catumin
Copy link
Contributor

@catumin catumin commented Sep 14, 2023

Resolves #243.

Add the ability to immediately set volume to 0% as requested in #243. I bound it to _ since "Shift + -" made sense to me, as it's basically super volume decrease.

Signed-off-by: Bailey Kasin <[email protected]>
@aome510
Copy link
Owner

aome510 commented Sep 15, 2023

Hi @BKasin, thanks for the PR. Ideally, mute should be a toggle command. Can you update the PR to support that?

@catumin
Copy link
Contributor Author

catumin commented Sep 16, 2023

@aome510 I agree that that would be preferable, but am not sure how to best implement it.

@aome510
Copy link
Owner

aome510 commented Sep 17, 2023

@aome510 I agree that that would be preferable, but am not sure how to best implement it.

Yeah, it's kinda tricky. One way is to add a new field called MuteState: Option<u32> in

pub struct PlayerState {
pub devices: Vec<Device>,
pub playback: Option<rspotify_model::CurrentPlaybackContext>,
pub playback_last_updated_time: Option<std::time::Instant>,
/// a buffered state to speedup the feedback of playback metadata update to user
// Related issue: https://github.com/aome510/spotify-player/issues/109
pub buffered_playback: Option<SimplifiedPlayback>,
pub queue: Option<rspotify_model::CurrentUserQueue>,
}
, which is None is not mute and Some(previous_volume_percent) if is mute. Handling "mute toggle" command will involve updating that state and sending a PlayerRequest::Volume request.

Track the old volume as mute_state: u32, which is None if
not currently muted.

Signed-off-by: Bailey Kasin <[email protected]>
@catumin
Copy link
Contributor Author

catumin commented Sep 21, 2023

Thank you for the guidance, I definitely was over-complicating it mentally.

Copy link
Owner

@aome510 aome510 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks.

It would be nice to also have UI for the mute functionality. Can you change

format!(
"repeat: {} | shuffle: {} | volume: {}% | device: {}",
<&'static str>::from(playback.repeat_state),
playback.shuffle_state,
playback.volume.unwrap_or_default(),
playback.device_name,
),
as well?

Maybe displayvolume: old_percent% (muted) if muted or volume: current_percent% if not.

@catumin
Copy link
Contributor Author

catumin commented Sep 22, 2023

Yeah, a little later today when I get home I'll do that.

Signed-off-by: Bailey Kasin <[email protected]>
@aome510
Copy link
Owner

aome510 commented Sep 22, 2023

Thanks @BKasin, after testing the change locally, I thought that the command logic could be simplified a bit more and we also need to handle the case when user increases/decreases the volume on mute. I updated the PR according to implement the above changes in a3930ac

@catumin
Copy link
Contributor Author

catumin commented Sep 22, 2023

Ah, yeah that is definitely a lot better. Thank you!

@aome510 aome510 merged commit bac9abb into aome510:master Sep 22, 2023
3 checks passed
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.

mute
2 participants