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

Refactor Config structs according to features #800

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
robinvd marked this conversation as resolved.
Show resolved Hide resolved

[target."cfg(target_os = \"macos\")".dependencies]
whoami = "0.9.0"
Expand Down
8 changes: 7 additions & 1 deletion docs/src/config/File.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -114,7 +120,7 @@ device_type = "speaker"

- **`use_keyring`** config entry / **`--use-keyring`** CLI flag <!-- omit in toc -->

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:

Expand Down
63 changes: 50 additions & 13 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -317,14 +317,17 @@ pub struct SharedConfigValues {
volume_controller: Option<VolumeController>,

/// The audio device
#[cfg(feature = "alsa_backend")]
#[structopt(long, value_name = "string")]
device: Option<String>,

/// The control device
#[cfg(feature = "alsa_backend")]
#[structopt(long, value_name = "string")]
control: Option<String>,

/// The mixer to use
#[cfg(feature = "alsa_backend")]
#[structopt(long, value_name = "string")]
mixer: Option<String>,

Expand Down Expand Up @@ -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)
Expand All @@ -441,18 +445,25 @@ 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)
.field("volume_normalisation", &self.volume_normalisation)
.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()
}
}
Comment on lines +456 to +466
Copy link
Member

Choose a reason for hiding this comment

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

By using the below variant, we could get rid of the cfg_if crate and the effect should be the same.

Suggested change
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()
}
}
#[cfg(feature = "alsa_backend")]
{
deb_struct
.field("device", &self.device)
.field("control", &self.control)
.field("mixer", &self.mixer);
}
deb_struct.finish()

}
}

Expand Down Expand Up @@ -494,7 +505,6 @@ impl SharedConfigValues {
}
}

// Handles Option<T> merging.
merge!(
backend,
username,
Expand All @@ -505,9 +515,6 @@ impl SharedConfigValues {
bitrate,
initial_volume,
device_name,
mixer,
control,
device,
volume_controller,
cache_path,
on_song_change_hook,
Expand All @@ -517,6 +524,9 @@ impl SharedConfigValues {
use_mpris
);

#[cfg(feature = "alsa_backend")]
merge!(mixer, control, device);

// Handles boolean merging.
self.use_keyring |= other.use_keyring;
self.volume_normalisation |= other.volume_normalisation;
Expand Down Expand Up @@ -550,12 +560,13 @@ pub(crate) struct SpotifydConfig {
pub(crate) use_mpris: bool,
pub(crate) cache: Option<Cache>,
pub(crate) backend: Option<String>,
#[cfg(feature = "alsa_backend")]
pub(crate) audio_device: Option<String>,
#[allow(unused)]
#[cfg(feature = "alsa_backend")]
pub(crate) control_device: Option<String>,
#[allow(unused)]
#[cfg(feature = "alsa_backend")]
pub(crate) mixer: Option<String>,
#[allow(unused)]
#[cfg_attr(not(feature = "alsa_backend"), allow(dead_code))]
pub(crate) volume_controller: VolumeController,
pub(crate) initial_volume: Option<u16>,
pub(crate) device_name: String,
Expand Down Expand Up @@ -683,8 +694,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,
Expand Down Expand Up @@ -738,4 +752,27 @@ mod tests {
spotifyd_section.username = Some("testUserName".to_string());
assert_eq!(merged_config, spotifyd_section);
}

#[cfg(features = "alsa_backend")]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[cfg(features = "alsa_backend")]
#[cfg(feature = "alsa_backend")]

#[test]
fn test_section_merging() {
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be renamed.

let mut spotifyd_section = SharedConfigValues::default();
global_section.mixer = "PCM".to_string();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
global_section.mixer = "PCM".to_string();
spotifyd_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);
Copy link
Member

Choose a reason for hiding this comment

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

assert!() tests by itself, whether the expression is true.

Suggested change
assert!(spotifyd_section != global_section, true);
assert!(spotifyd_section != global_section);


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();
Comment on lines +774 to +775
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be necessary, right?

assert_eq!(merged_config, spotifyd_section);
}
}
30 changes: 20 additions & 10 deletions src/setup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +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)]
Comment on lines +26 to +28
Copy link
Member

Choose a reason for hiding this comment

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

At least with the changes below and on Rust 1.63, none of those should be necessary any more.

pub(crate) fn initial_state(
handle: Handle,
config: config::SpotifydConfig,
) -> main_loop::MainLoopState {
#[cfg(feature = "alsa_backend")]
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<dyn FnMut() -> Box<dyn Mixer>> = unimplemented!();
Copy link
Member

Choose a reason for hiding this comment

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

This will panic, if run. You can just omit the = ... part.

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();
Expand All @@ -54,15 +60,12 @@ pub(crate) fn initial_state(
linear_scaling: linear,
}) as Box<dyn mixer::Mixer>
}) as Box<dyn FnMut() -> Box<dyn Mixer>>
}
}
};

#[cfg(not(feature = "alsa_backend"))]
let mut mixer = {
info!("Using software volume controller.");
Box::new(|| Box::new(mixer::softmixer::SoftMixer::open(None)) as Box<dyn Mixer>)
}} else {
info!("Using software volume controller.");
mixer = Box::new(|| Box::new(mixer::softmixer::SoftMixer::open(None)) as Box<dyn Mixer>)
as Box<dyn FnMut() -> Box<dyn Mixer>>
}
};

let cache = config.cache;
Expand Down Expand Up @@ -138,10 +141,17 @@ 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: config.audio_device.clone(),
audio_device: None,
},
Comment on lines +144 to 155
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[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: config.audio_device.clone(),
audio_device: None,
},
audio_setup: main_loop::AudioSetup {
mixer,
backend,
#[cfg(feature = "alsa_backend")]
audio_device: config.audio_device,
#[cfg(not(feature = "alsa_backend"))]
audio_device: None,
},

spotifyd_state: main_loop::SpotifydState {
ctrl_c_stream: Box::new(ctrl_c(&handle).flatten_stream()),
Expand Down