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

Added alternative to disable changing volume #750

Merged
merged 2 commits into from
Mar 9, 2023
Merged

Conversation

Icelk
Copy link
Contributor

@Icelk Icelk commented Jan 7, 2021

I added an alternative to "softvol" and "alsa"; "none".
If remotely changing volume, or resetting it on restart is undesired, this is a fix.

slondr
slondr previously approved these changes Jan 24, 2021
@slondr slondr requested a review from a team January 24, 2021 04:07
@Icelk
Copy link
Contributor Author

Icelk commented Jan 24, 2021

Is this a good solution or should I investigate if we can disable remote changing of volume? Maybe change the mixer in librespot to an Option? This just ignores input.
Perhaps the best way to move forward is to implement this and later change the librespot API?

@Icelk
Copy link
Contributor Author

Icelk commented Feb 19, 2021

What do you @JojiiOfficial @robinvd @sirwindfield think about this?

@Icelk
Copy link
Contributor Author

Icelk commented Feb 19, 2021

Does anybody know why the lint check is not successful?

@stale
Copy link

stale bot commented Jul 6, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix Issues that will not be fixed under any circumstances label Jul 6, 2021
@Icelk
Copy link
Contributor Author

Icelk commented Jul 9, 2021

Still waiting for response.

@stale stale bot removed the wontfix Issues that will not be fixed under any circumstances label Jul 9, 2021
@stale
Copy link

stale bot commented Nov 21, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix Issues that will not be fixed under any circumstances label Nov 21, 2021
@Icelk
Copy link
Contributor Author

Icelk commented Nov 23, 2021

Still waiting for response.

@stale stale bot removed the wontfix Issues that will not be fixed under any circumstances label Nov 23, 2021
@slondr
Copy link
Member

slondr commented Jan 14, 2022

Looks like all the checks are passing now.

@Icelk
Copy link
Contributor Author

Icelk commented Jan 14, 2022

I’m a bit confused now.
I recently saw in spotify-tui the mobile clients have volume disabled. They advertised it to the Spotify API, and spotify-tui recognises it.

Should we merge this and then use the same struct for disabling it properly (if it’s possible)? If that’s the case, I think librespot needs to support the feature.

@eladyn
Copy link
Member

eladyn commented Mar 6, 2023

I recently saw in spotify-tui the mobile clients have volume disabled. They advertised it to the Spotify API, and spotify-tui recognises it.

Should we merge this and then use the same struct for disabling it properly (if it’s possible)? If that’s the case, I think librespot needs to support the feature.

@Icelk While looking over the code today, I found by chance the following lines:

spotifyd/src/setup.rs

Lines 69 to 80 in fbcbeca

#[cfg(feature = "alsa_backend")]
let volume_ctrl = if matches!(
config.volume_controller,
config::VolumeController::AlsaLinear
) {
VolumeCtrl::Linear
} else {
VolumeCtrl::default()
};
#[cfg(not(feature = "alsa_backend"))]
let volume_ctrl = VolumeCtrl::default();

I was wondering, why this struct exists, since the actual volume control is done by the mixer, which has nothing to do with this setting. Apparently, this exists to signal to Spotify clients the type of volume control this device implements. And, most importantly, it has three possible states: Log, Linear and Fixed. I think the latter could be quite interesting to us.

If you're still interested after all this time, this PR has been lying around, I suspect that it might be possible to turn off the client volume control via this setting! If not, that's completely understandable and okay too, of course!

@Icelk
Copy link
Contributor Author

Icelk commented Mar 6, 2023

Thanks for the consideration! You were completely correct.

I rewrote this PR to be up to date with the latest master, and implemented your idea. Thanks for the tip!

@Icelk
Copy link
Contributor Author

Icelk commented Mar 6, 2023

This PR now doesn't seem to have any issues. Can I improve anything @eladyn ?

Copy link
Member

@eladyn eladyn left a comment

Choose a reason for hiding this comment

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

Wow! I wouldn't have expected such a fast response after such a long time. Thank you for the effort.

Just some comments on the code that might remove some duplicate code. The rest looks fine!

src/no_mixer.rs Outdated Show resolved Hide resolved
src/setup.rs Outdated Show resolved Hide resolved
src/setup.rs Outdated Show resolved Hide resolved
@Icelk
Copy link
Contributor Author

Icelk commented Mar 8, 2023

Wow! I wouldn't have expected such a fast response after such a long time. Thank you for the effort.

No problem :)

Just some comments on the code that might remove some duplicate code. The rest looks fine!

Just pushed some fixes.

Thanks for your swift responses!

Copy link
Member

@eladyn eladyn left a comment

Choose a reason for hiding this comment

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

LGTM and is working great. Thank you!

@eladyn eladyn requested review from slondr and a team March 8, 2023 18:51
@Icelk
Copy link
Contributor Author

Icelk commented Mar 8, 2023

Thanks!

Copy link
Member

@slondr slondr left a comment

Choose a reason for hiding this comment

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

very awesome, thanks for the work on this!

@slondr slondr merged commit c31850d into Spotifyd:master Mar 9, 2023
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.

3 participants