From 5ddc36145cf6b8a04a6775591545f9f20b01cc2e Mon Sep 17 00:00:00 2001 From: CPerezz Date: Sun, 31 Jan 2021 14:35:08 +0100 Subject: [PATCH 1/7] Refactor Config structs according to features As it was stated in #349, structs like `SharedConfigValues` or `SpotifydConfig` contain fields like `mixer`, `control` and `device` which only apply if `alsa-backend` feature is enabled. Therefore, it would be nice if these fields were only documented when the feature is enabled as well as used. Closes #349 --- Cargo.toml | 1 + src/config.rs | 110 +++++++++++++++++++++++++++++++++----------------- src/setup.rs | 30 ++++++++------ 3 files changed, 92 insertions(+), 49 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 6a5036fd..4c88195f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,6 +34,7 @@ xdg = "2.2" librespot = { version = "0.1.1", default-features = false, features = ["with-tremor"] } toml = "0.5.8" color-eyre = "0.5" +cfg-if = "1" [target."cfg(target_os = \"macos\")".dependencies] whoami = "0.9.0" diff --git a/src/config.rs b/src/config.rs index bfa862db..d861acf7 100644 --- a/src/config.rs +++ b/src/config.rs @@ -317,16 +317,19 @@ pub struct SharedConfigValues { volume_controller: Option, /// The audio device + #[cfg(feature = "alsa-backend")] #[structopt(long, value_name = "string")] - device: Option, + device: String, /// The control device + #[cfg(feature = "alsa-backend")] #[structopt(long, value_name = "string")] - control: Option, + control: String, /// The mixer to use + #[cfg(feature = "alsa-backend")] #[structopt(long, value_name = "string")] - mixer: Option, + mixer: String, /// The device name displayed in Spotify #[structopt(long, short, value_name = "string")] @@ -429,7 +432,8 @@ impl fmt::Debug for SharedConfigValues { None }; - f.debug_struct("SharedConfigValues") + let mut deb_struct = f.debug_struct("SharedConfigValues"); + deb_struct .field("username", &username_value) .field("username_cmd", &username_cmd_value) .field("password", &password_value) @@ -441,9 +445,6 @@ impl fmt::Debug for SharedConfigValues { .field("no-audio-cache", &self.no_audio_cache) .field("backend", &self.backend) .field("volume_controller", &self.volume_controller) - .field("device", &self.device) - .field("control", &self.control) - .field("mixer", &self.mixer) .field("device_name", &self.device_name) .field("bitrate", &self.bitrate) .field("initial_volume", &self.initial_volume) @@ -451,8 +452,18 @@ impl fmt::Debug for SharedConfigValues { .field("normalisation_pregain", &self.normalisation_pregain) .field("zeroconf_port", &self.zeroconf_port) .field("proxy", &self.proxy) - .field("device_type", &self.device_type) - .finish() + .field("device_type", &self.device_type); + cfg_if::cfg_if! { + if #[cfg(feature = "alsa-backend")] { + deb_struct + .field("device", &self.device) + .field("control", &self.control) + .field("mixer", &self.mixer) + .finish() + } else { + deb_struct.finish() + } + } } } @@ -494,28 +505,52 @@ impl SharedConfigValues { } } + cfg_if::cfg_if! { + if #[cfg(feature = "alsa-backend")] { + merge!( + backend, + username, + username_cmd, + password, + password_cmd, + normalisation_pregain, + bitrate, + initial_volume, + device_name, + mixer, + control, + device, + volume_controller, + cache_path, + on_song_change_hook, + zeroconf_port, + proxy, + device_type, + use_mpris + ); + } else { + merge!( + backend, + username, + username_cmd, + password, + password_cmd, + normalisation_pregain, + bitrate, + initial_volume, + device_name, + volume_controller, + cache_path, + on_song_change_hook, + zeroconf_port, + proxy, + device_type, + use_mpris + ); + } + } + // Handles Option merging. - merge!( - backend, - username, - username_cmd, - password, - password_cmd, - normalisation_pregain, - bitrate, - initial_volume, - device_name, - mixer, - control, - device, - volume_controller, - cache_path, - on_song_change_hook, - zeroconf_port, - proxy, - device_type, - use_mpris - ); // Handles boolean merging. self.use_keyring |= other.use_keyring; @@ -550,12 +585,12 @@ pub(crate) struct SpotifydConfig { pub(crate) use_mpris: bool, pub(crate) cache: Option, pub(crate) backend: Option, - pub(crate) audio_device: Option, - #[allow(unused)] - pub(crate) control_device: Option, - #[allow(unused)] - pub(crate) mixer: Option, - #[allow(unused)] + #[cfg(feature = "alsa-backend")] + pub(crate) audio_device: String, + #[cfg(feature = "alsa-backend")] + pub(crate) control_device: String, + #[cfg(feature = "alsa-backend")] + pub(crate) mixer: String, pub(crate) volume_controller: VolumeController, pub(crate) initial_volume: Option, pub(crate) device_name: String, @@ -683,8 +718,11 @@ pub(crate) fn get_internal_config(config: CliConfig) -> SpotifydConfig { use_mpris: config.shared_config.use_mpris.unwrap_or(true), cache, backend: Some(backend), + #[cfg(feature = "alsa-backend")] audio_device: config.shared_config.device, + #[cfg(feature = "alsa-backend")] control_device: config.shared_config.control, + #[cfg(feature = "alsa-backend")] mixer: config.shared_config.mixer, volume_controller, initial_volume, diff --git a/src/setup.rs b/src/setup.rs index e2b3ceac..1ca965d4 100644 --- a/src/setup.rs +++ b/src/setup.rs @@ -1,5 +1,3 @@ -#[cfg(feature = "alsa_backend")] -use crate::alsa_mixer; use crate::{config, main_loop}; use futures::{self, Future}; #[cfg(feature = "dbus_keyring")] @@ -27,8 +25,9 @@ pub(crate) fn initial_state( handle: Handle, config: config::SpotifydConfig, ) -> main_loop::MainLoopState { - #[cfg(feature = "alsa_backend")] - let mut mixer = { + let mut mixer; + cfg_if::cfg_if! { + if #[cfg(feature = "alsa-backend")] { let local_audio_device = config.audio_device.clone(); let local_control_device = config.control_device.clone(); let local_mixer = config.mixer.clone(); @@ -44,7 +43,7 @@ pub(crate) fn initial_state( config.volume_controller, config::VolumeController::AlsaLinear ); - Box::new(move || { + mixer = Box::new(alsa_mixer::AlsaMixer { device: local_control_device .clone() @@ -53,16 +52,14 @@ pub(crate) fn initial_state( mixer: local_mixer.clone().unwrap_or_else(|| "Master".to_string()), linear_scaling: linear, }) as Box - }) as Box Box> + } } - }; - - #[cfg(not(feature = "alsa_backend"))] - let mut mixer = { - info!("Using software volume controller."); - Box::new(|| Box::new(mixer::softmixer::SoftMixer::open(None)) as Box) + } else { + info!("Using software volume controller."); + mixer = Box::new(|| Box::new(mixer::softmixer::SoftMixer::open(None)) as Box) as Box Box> + } }; let cache = config.cache; @@ -136,12 +133,19 @@ pub(crate) fn initial_state( }; let backend = find_backend(backend.as_ref().map(String::as_ref)); + let audio_device; + cfg_if::cfg_if! { + if #[cfg(feature = "alsa-backend")] { + audio_device = Some(config.audio_device.clone()) + }else { + audio_device = None + }}; main_loop::MainLoopState { librespot_connection: main_loop::LibreSpotConnection::new(connection, discovery_stream), audio_setup: main_loop::AudioSetup { mixer, backend, - audio_device: config.audio_device.clone(), + audio_device, }, spotifyd_state: main_loop::SpotifydState { ctrl_c_stream: Box::new(ctrl_c(&handle).flatten_stream()), From bd2c13f12031414d96d93b1f373b7499c14549cd Mon Sep 17 00:00:00 2001 From: CPerezz Date: Sun, 31 Jan 2021 15:12:25 +0100 Subject: [PATCH 2/7] Update Cargo.lock --- Cargo.lock | 1 + 1 file changed, 1 insertion(+) diff --git a/Cargo.lock b/Cargo.lock index b1e4d5de..863c0505 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3082,6 +3082,7 @@ name = "spotifyd" version = "0.3.0" dependencies = [ "alsa 0.3.0", + "cfg-if 1.0.0", "chrono", "color-eyre", "daemonize", From 8835bb04c4209f06fe850a6ebdb7029887273c6e Mon Sep 17 00:00:00 2001 From: CPerezz Date: Mon, 8 Feb 2021 23:15:33 +0100 Subject: [PATCH 3/7] Refactor merge! macro cfg code --- src/config.rs | 65 ++++++++++++++++----------------------------------- 1 file changed, 20 insertions(+), 45 deletions(-) diff --git a/src/config.rs b/src/config.rs index d861acf7..9929126c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -505,52 +505,27 @@ impl SharedConfigValues { } } - cfg_if::cfg_if! { - if #[cfg(feature = "alsa-backend")] { - merge!( - backend, - username, - username_cmd, - password, - password_cmd, - normalisation_pregain, - bitrate, - initial_volume, - device_name, - mixer, - control, - device, - volume_controller, - cache_path, - on_song_change_hook, - zeroconf_port, - proxy, - device_type, - use_mpris - ); - } else { - merge!( - backend, - username, - username_cmd, - password, - password_cmd, - normalisation_pregain, - bitrate, - initial_volume, - device_name, - volume_controller, - cache_path, - on_song_change_hook, - zeroconf_port, - proxy, - device_type, - use_mpris - ); - } - } + merge!( + backend, + username, + username_cmd, + password, + password_cmd, + normalisation_pregain, + bitrate, + initial_volume, + device_name, + volume_controller, + cache_path, + on_song_change_hook, + zeroconf_port, + proxy, + device_type, + use_mpris + ); - // Handles Option merging. + #[cfg(feature = "alsa-backend")] + merge!(mixer, control, device); // Handles boolean merging. self.use_keyring |= other.use_keyring; From a827faf31ce4d634c659d60bf477c2701cbff0e8 Mon Sep 17 00:00:00 2001 From: CPerezz Date: Mon, 8 Feb 2021 23:28:54 +0100 Subject: [PATCH 4/7] Add tests for merge! macro adaptation --- src/config.rs | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/config.rs b/src/config.rs index 9929126c..0b4c7033 100644 --- a/src/config.rs +++ b/src/config.rs @@ -751,4 +751,27 @@ mod tests { spotifyd_section.username = Some("testUserName".to_string()); assert_eq!(merged_config, spotifyd_section); } + + #[cfg(features = "alsa-backend")] + #[test] + fn test_section_merging() { + let mut spotifyd_section = SharedConfigValues::default(); + global_section.mixer = "PCM".to_string(); + + let global_section = SharedConfigValues::default(); + global_section.mixer = "PCM1".to_string(); + + // The test only makes sense if both sections differ. + assert!(spotifyd_section != global_section, true); + + let file_config = FileConfig { + global: Some(global_section), + spotifyd: Some(spotifyd_section.clone()), + }; + let merged_config = file_config.get_merged_sections().unwrap(); + + // Add the new field to spotifyd section. + spotifyd_section.mixer = "PMC".to_string(); + assert_eq!(merged_config, spotifyd_section); + } } From 58d55a584745ae160ec23f313e818e614f15cae8 Mon Sep 17 00:00:00 2001 From: CPerezz Date: Tue, 9 Feb 2021 00:34:57 +0100 Subject: [PATCH 5/7] Bring back Option for Playback types --- src/config.rs | 36 ++++++++++++++++++------------------ src/setup.rs | 27 +++++++++++++-------------- 2 files changed, 31 insertions(+), 32 deletions(-) diff --git a/src/config.rs b/src/config.rs index 0b4c7033..85265948 100644 --- a/src/config.rs +++ b/src/config.rs @@ -317,19 +317,19 @@ pub struct SharedConfigValues { volume_controller: Option, /// The audio device - #[cfg(feature = "alsa-backend")] + #[cfg(feature = "alsa_backend")] #[structopt(long, value_name = "string")] - device: String, + device: Option, /// The control device - #[cfg(feature = "alsa-backend")] + #[cfg(feature = "alsa_backend")] #[structopt(long, value_name = "string")] - control: String, + control: Option, /// The mixer to use - #[cfg(feature = "alsa-backend")] + #[cfg(feature = "alsa_backend")] #[structopt(long, value_name = "string")] - mixer: String, + mixer: Option, /// The device name displayed in Spotify #[structopt(long, short, value_name = "string")] @@ -454,7 +454,7 @@ impl fmt::Debug for SharedConfigValues { .field("proxy", &self.proxy) .field("device_type", &self.device_type); cfg_if::cfg_if! { - if #[cfg(feature = "alsa-backend")] { + if #[cfg(feature = "alsa_backend")] { deb_struct .field("device", &self.device) .field("control", &self.control) @@ -524,7 +524,7 @@ impl SharedConfigValues { use_mpris ); - #[cfg(feature = "alsa-backend")] + #[cfg(feature = "alsa_backend")] merge!(mixer, control, device); // Handles boolean merging. @@ -560,12 +560,12 @@ pub(crate) struct SpotifydConfig { pub(crate) use_mpris: bool, pub(crate) cache: Option, pub(crate) backend: Option, - #[cfg(feature = "alsa-backend")] - pub(crate) audio_device: String, - #[cfg(feature = "alsa-backend")] - pub(crate) control_device: String, - #[cfg(feature = "alsa-backend")] - pub(crate) mixer: String, + #[cfg(feature = "alsa_backend")] + pub(crate) audio_device: Option, + #[cfg(feature = "alsa_backend")] + pub(crate) control_device: Option, + #[cfg(feature = "alsa_backend")] + pub(crate) mixer: Option, pub(crate) volume_controller: VolumeController, pub(crate) initial_volume: Option, pub(crate) device_name: String, @@ -693,11 +693,11 @@ pub(crate) fn get_internal_config(config: CliConfig) -> SpotifydConfig { use_mpris: config.shared_config.use_mpris.unwrap_or(true), cache, backend: Some(backend), - #[cfg(feature = "alsa-backend")] + #[cfg(feature = "alsa_backend")] audio_device: config.shared_config.device, - #[cfg(feature = "alsa-backend")] + #[cfg(feature = "alsa_backend")] control_device: config.shared_config.control, - #[cfg(feature = "alsa-backend")] + #[cfg(feature = "alsa_backend")] mixer: config.shared_config.mixer, volume_controller, initial_volume, @@ -752,7 +752,7 @@ mod tests { assert_eq!(merged_config, spotifyd_section); } - #[cfg(features = "alsa-backend")] + #[cfg(features = "alsa_backend")] #[test] fn test_section_merging() { let mut spotifyd_section = SharedConfigValues::default(); diff --git a/src/setup.rs b/src/setup.rs index 1ca965d4..7467baca 100644 --- a/src/setup.rs +++ b/src/setup.rs @@ -1,3 +1,5 @@ +#[cfg(feature = "alsa_backend")] +use crate::alsa_mixer; use crate::{config, main_loop}; use futures::{self, Future}; #[cfg(feature = "dbus_keyring")] @@ -21,13 +23,18 @@ use std::{io, process::exit}; use tokio_core::reactor::Handle; use tokio_signal::ctrl_c; +// We need to allow these lints since the conditional compilation is +// what makes them appear. +#[allow(unused_must_use, unreachable_code, unused_variables)] pub(crate) fn initial_state( handle: Handle, config: config::SpotifydConfig, ) -> main_loop::MainLoopState { - let mut mixer; + // We just need to initialize the mixer. The contents will be mutated for sure + // and therefore we don't need to worry since all of the branches are covered. + let mut mixer: Box Box> = unimplemented!(); cfg_if::cfg_if! { - if #[cfg(feature = "alsa-backend")] { + if #[cfg(feature = "alsa_backend")] { let local_audio_device = config.audio_device.clone(); let local_control_device = config.control_device.clone(); let local_mixer = config.mixer.clone(); @@ -43,7 +50,7 @@ pub(crate) fn initial_state( config.volume_controller, config::VolumeController::AlsaLinear ); - mixer = + Box::new(move || { Box::new(alsa_mixer::AlsaMixer { device: local_control_device .clone() @@ -52,10 +59,9 @@ pub(crate) fn initial_state( mixer: local_mixer.clone().unwrap_or_else(|| "Master".to_string()), linear_scaling: linear, }) as Box - - } + }) as Box Box> } - } else { + }} else { info!("Using software volume controller."); mixer = Box::new(|| Box::new(mixer::softmixer::SoftMixer::open(None)) as Box) as Box Box> @@ -133,19 +139,12 @@ pub(crate) fn initial_state( }; let backend = find_backend(backend.as_ref().map(String::as_ref)); - let audio_device; - cfg_if::cfg_if! { - if #[cfg(feature = "alsa-backend")] { - audio_device = Some(config.audio_device.clone()) - }else { - audio_device = None - }}; main_loop::MainLoopState { librespot_connection: main_loop::LibreSpotConnection::new(connection, discovery_stream), audio_setup: main_loop::AudioSetup { mixer, backend, - audio_device, + audio_device: config.audio_device, }, spotifyd_state: main_loop::SpotifydState { ctrl_c_stream: Box::new(ctrl_c(&handle).flatten_stream()), From f14aea3f542544ec2497c91163cef38d7edb0bdd Mon Sep 17 00:00:00 2001 From: CPerezz Date: Tue, 9 Feb 2021 00:43:47 +0100 Subject: [PATCH 6/7] Add suggestions to remove alsa-dependand configs In the wiki mdbook it's not stated clearly that the config options that refer to `mixer`, `controller` and `device` can be directly commented out since they're only used when spotifyd is compiled with `alsa_backend` feature. --- docs/src/config/File.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/docs/src/config/File.md b/docs/src/config/File.md index 2cc4a487..c45c1e29 100644 --- a/docs/src/config/File.md +++ b/docs/src/config/File.md @@ -42,13 +42,19 @@ backend = "alsa" # The alsa audio device to stream audio to. To get a # list of valid devices, run `aplay -L`, +# +# Comment out or remove if you are not using the `alsa_backend`. device = "alsa_audio_device" # omit for macOS # The alsa control device. By default this is the same # name as the `device` field. +# +# Comment out or remove if you are not using the `alsa_backend`. control = "alsa_audio_device" # omit for macOS # The alsa mixer used by `spotifyd`. +# +# Comment out or remove if you are not using the `alsa_backend`. mixer = "PCM" # The volume controller. Each one behaves different to @@ -114,7 +120,7 @@ device_type = "speaker" - **`use_keyring`** config entry / **`--use-keyring`** CLI flag - This features leverages [Linux's DBus Secret Service API][secret-storage-specification] or native macOS keychain in order to forgo the need to store your password directly in the config file. To use it, complile with the `dbus_keyring` feature and set the `use-keyring` config entry to `true` or pass the `--use-keyring` CLI flag during start to the daemon. Remove the `password` and/or `password_cmd` config entries. + This features leverages [Linux's DBus Secret Service API][secret-storage-specification] or native macOS keychain in order to forgo the need to store your password directly in the config file. To use it, complile with the `dbus_keyring` feature and set the `use-keyring` config entry to `true` or pass the `--use-keyring` CLI flag during start to the daemon. Remove the `password` and/or `password_cmd` config entries. Your keyring entry needs to have the following attributes set: From b2d5d80cfbb412d1b9206bac2a5815e2cac01db8 Mon Sep 17 00:00:00 2001 From: CPerezz Date: Tue, 9 Feb 2021 19:44:05 +0100 Subject: [PATCH 7/7] Conditionally create AudioSetup depending on alsa --- src/config.rs | 1 + src/setup.rs | 7 +++++++ 2 files changed, 8 insertions(+) diff --git a/src/config.rs b/src/config.rs index 85265948..93b40b52 100644 --- a/src/config.rs +++ b/src/config.rs @@ -566,6 +566,7 @@ pub(crate) struct SpotifydConfig { pub(crate) control_device: Option, #[cfg(feature = "alsa_backend")] pub(crate) mixer: Option, + #[cfg_attr(not(feature = "alsa_backend"), allow(dead_code))] pub(crate) volume_controller: VolumeController, pub(crate) initial_volume: Option, pub(crate) device_name: String, diff --git a/src/setup.rs b/src/setup.rs index 7467baca..8ab51839 100644 --- a/src/setup.rs +++ b/src/setup.rs @@ -141,11 +141,18 @@ pub(crate) fn initial_state( let backend = find_backend(backend.as_ref().map(String::as_ref)); main_loop::MainLoopState { librespot_connection: main_loop::LibreSpotConnection::new(connection, discovery_stream), + #[cfg(feature = "alsa_backend")] audio_setup: main_loop::AudioSetup { mixer, backend, audio_device: config.audio_device, }, + #[cfg(not(feature = "alsa_backend"))] + audio_setup: main_loop::AudioSetup { + mixer, + backend, + audio_device: None, + }, spotifyd_state: main_loop::SpotifydState { ctrl_c_stream: Box::new(ctrl_c(&handle).flatten_stream()), shutting_down: false,