Refactor Config structs according to features #800

CPerezz · 2021-01-31T13:36:34Z

As it was stated in #394, 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 #394

As it was stated in Spotifyd#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 Spotifyd#349

robinvd · 2021-02-08T11:06:14Z

src/config.rs

@@ -494,28 +505,52 @@ impl SharedConfigValues {
            }
        }

+        cfg_if::cfg_if! {


i think this might work as well (and be nicer)

merge!(general fields) #[cfg(alsa)] merge!(alsa fields)

Yep. That's a good one.

robinvd

Thanks for the PR!

im not completely sure if changing the config layout like this is a breaking change or not.

Cargo.toml

CPerezz · 2021-02-08T11:16:18Z

im not completely sure if changing the config layout like this is a breaking change or not.

I think so. Basically when you're now setting None for these config options, you should directly not specify them or some similar thing. I'd need to check if the configfiles parsing behaves on a friendly way with this. If not, I'll need to refactor this code too.

If it's done by clap or some similar crate, then once compiled that will be handled automatically, otherways I'll need to edit also the configfile parsing.

Also will need to add docs for the different config options considering alsa and non-alsa features.

robinvd · 2021-02-08T11:19:24Z

alright good to know, then we have to bump the version

slondr · 2021-02-08T16:46:48Z

We already have another MR that's "waiting" on 0.4 to be merged, we can add this one to the list.

Maybe we should make version branches to keep track?

CPerezz · 2021-02-08T22:29:51Z

So the macro stuff has been already addressed. And tested.

I'm just concerned about one thing regarding the SpotifyConfig struct.

Should the alsa_backend-only fields be Option<T> or just T? Since as the issue mentions, they need to be expressed if the feature is set.

I made the refactor having the fields like:

#[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,

But I can change them to be Option<String>.

Once this is clarified, It might need to be specified in the mdbook a bit more clearly IMO. Then the PR will be ready I think.

robinvd · 2021-02-08T22:44:08Z

If it is a plain string, and i specify it in the config but not command line will it then fail?

CPerezz · 2021-02-08T22:49:04Z

If it is a plain string, and i specify it in the config but not command line will it then fail?

Exactly! Good point! Let's make it an Option as it was in the beginning, I thought it was like this due to the backend possibilities but it's also because of being a config param that can be missing too.

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.

robinvd · 2021-03-03T09:09:58Z

I think this looks good! And we should release this in the next breaking change!

mainrs · 2021-03-27T21:18:32Z

The breaking change concern could be addressed by explicitly telling serde to ignore unknown fields. I do remember there was a way but I can't write it down off the top of my head.

mainrs · 2021-03-27T21:20:37Z

So the macro stuff has been already addressed. And tested.

I'm just concerned about one thing regarding the SpotifyConfig struct.

Should the alsa_backend-only fields be Option<T> or just T? Since as the issue mentions, they need to be expressed if the feature is set.

I made the refactor having the fields like:
#[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,
But I can change them to be Option<String>.

Once this is clarified, It might need to be specified in the mdbook a bit more clearly IMO. Then the PR will be ready I think.

I do remember that spotifyd also had a default option that, when no config is supplied, it uses the Default implementation. Setting this to String would break it, as the CLI wouldn't work without the configuration values.

This is something we could (and I think even should!) consider for a large breaking change at some point. I'd rather have mandatory fields than some magic that works in the background (and not even on all machines) .

CPerezz · 2021-04-26T10:40:53Z

Sorry @sirwindfield but this got lost between tons of notifications. The attributes you mention are indeed Option<String> so I'm unsure if this needs any refactoring before it can be approved/landed.

stale · 2021-07-25T12:18:44Z

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.

CPerezz · 2022-08-24T10:57:13Z

@mainrs @robinvd is this something that is still not desired?

eladyn

Don't know if it's worth commenting on this PR, but if there is still interest from the maintainer side, the below can be taken into consideration.

eladyn · 2022-08-26T10:05:52Z

src/config.rs

+        global_section.mixer = "PCM1".to_string();
+
+        // The test only makes sense if both sections differ.
+        assert!(spotifyd_section != global_section, true);


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

Suggested change

assert!(spotifyd_section != global_section, true);

assert!(spotifyd_section != global_section);

eladyn · 2022-08-26T10:10:23Z

src/config.rs

+        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()
+            }
+        }


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()

eladyn · 2022-08-26T10:11:48Z

src/config.rs

+    #[test]
+    fn test_section_merging() {
+        let mut spotifyd_section = SharedConfigValues::default();
+        global_section.mixer = "PCM".to_string();


Suggested change

global_section.mixer = "PCM".to_string();

spotifyd_section.mixer = "PCM".to_string();

eladyn · 2022-08-26T10:12:36Z

src/config.rs

+        // Add the new field to spotifyd section.
+        spotifyd_section.mixer = "PMC".to_string();


This shouldn't be necessary, right?

eladyn · 2022-08-26T10:19:05Z

src/config.rs

@@ -738,4 +752,27 @@ mod tests {
        spotifyd_section.username = Some("testUserName".to_string());
        assert_eq!(merged_config, spotifyd_section);
    }
+
+    #[cfg(features = "alsa_backend")]


Suggested change

#[cfg(features = "alsa_backend")]

#[cfg(feature = "alsa_backend")]

eladyn · 2022-08-26T10:21:14Z

src/config.rs

+
+    #[cfg(features = "alsa_backend")]
+    #[test]
+    fn test_section_merging() {


This needs to be renamed.

eladyn · 2022-08-26T10:30:03Z

src/setup.rs

-    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!();


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

eladyn · 2022-08-26T10:34:24Z

src/setup.rs

+        #[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,
        },


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,

},

eladyn · 2022-08-26T10:41:33Z

src/setup.rs

+// We need to allow these lints since the conditional compilation is
+// what makes them appear.
+#[allow(unused_must_use, unreachable_code, unused_variables)]


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

mainrs · 2022-08-26T12:02:07Z

Hello! I do not have the time to maintain the project actively right now. I think it would still be a good idea to get this PR done, since it just does not make sense to have them in the same struct.

The current code base of spotifyd feels more like patch work then anything else to be honest. The tokio 1.0 rewrite did help though. I definitely am interested in seeing how librespot does these days. Maybe some changes allow us to make the whole client more lightweight in general. Or restructure some features/code modules all together.

I'll definitely investigate this. I cannot tell you when though.

CPerezz · 2022-08-29T08:24:36Z

Thanks for the review comments @eladyn

Will wait to see if @mainrs has more time in the future and address this.

Thank you all! :)

CPerezz added 2 commits January 31, 2021 14:35

Update Cargo.lock

bd2c13f

CPerezz mentioned this pull request Jan 31, 2021

Warnings during cargo check in macos-latest target #801

Closed

robinvd reviewed Feb 8, 2021

View reviewed changes

robinvd requested changes Feb 8, 2021

View reviewed changes

Cargo.toml Show resolved Hide resolved

slondr added this to the v0.4.0 milestone Feb 8, 2021

CPerezz added 2 commits February 8, 2021 23:15

Refactor merge! macro cfg code

8835bb0

Add tests for merge! macro adaptation

a827faf

CPerezz requested a review from robinvd February 8, 2021 22:29

CPerezz added 2 commits February 9, 2021 00:34

Bring back Option<String> for Playback types

58d55a5

Add suggestions to remove alsa-dependand configs

f14aea3

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.

CPerezz requested a review from mainrs February 8, 2021 23:44

Conditionally create AudioSetup depending on alsa

b2d5d80

robinvd approved these changes Mar 3, 2021

View reviewed changes

stale bot added the wontfix Issues that will not be fixed under any circumstances label Jul 25, 2021

stale bot closed this Aug 4, 2021

eladyn reviewed Aug 26, 2022

View reviewed changes

slondr reopened this Aug 30, 2022

eladyn removed the wontfix Issues that will not be fixed under any circumstances label Sep 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Config structs according to features #800

Refactor Config structs according to features #800

CPerezz commented Jan 31, 2021 •

edited

robinvd Feb 8, 2021 •

edited

CPerezz Feb 8, 2021

robinvd left a comment •

edited

CPerezz commented Feb 8, 2021 •

edited

robinvd commented Feb 8, 2021

slondr commented Feb 8, 2021

CPerezz commented Feb 8, 2021

robinvd commented Feb 8, 2021

CPerezz commented Feb 8, 2021

robinvd commented Mar 3, 2021

mainrs commented Mar 27, 2021

mainrs commented Mar 27, 2021

CPerezz commented Apr 26, 2021

stale bot commented Jul 25, 2021

CPerezz commented Aug 24, 2022

eladyn left a comment

eladyn Aug 26, 2022

eladyn Aug 26, 2022

eladyn Aug 26, 2022

eladyn Aug 26, 2022

eladyn Aug 26, 2022

eladyn Aug 26, 2022

eladyn Aug 26, 2022

eladyn Aug 26, 2022

eladyn Aug 26, 2022

mainrs commented Aug 26, 2022 •

edited

CPerezz commented Aug 29, 2022

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

	global_section.mixer = "PCM".to_string();
	spotifyd_section.mixer = "PCM".to_string();

		// Add the new field to spotifyd section.
		spotifyd_section.mixer = "PMC".to_string();

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

Refactor Config structs according to features #800

Are you sure you want to change the base?

Refactor Config structs according to features #800

Conversation

CPerezz commented Jan 31, 2021 • edited

robinvd Feb 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robinvd left a comment • edited

Choose a reason for hiding this comment

CPerezz commented Feb 8, 2021 • edited

robinvd commented Feb 8, 2021

slondr commented Feb 8, 2021

CPerezz commented Feb 8, 2021

robinvd commented Feb 8, 2021

CPerezz commented Feb 8, 2021

robinvd commented Mar 3, 2021

mainrs commented Mar 27, 2021

mainrs commented Mar 27, 2021

CPerezz commented Apr 26, 2021

stale bot commented Jul 25, 2021

CPerezz commented Aug 24, 2022

eladyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mainrs commented Aug 26, 2022 • edited

CPerezz commented Aug 29, 2022

CPerezz commented Jan 31, 2021 •

edited

robinvd Feb 8, 2021 •

edited

robinvd left a comment •

edited

CPerezz commented Feb 8, 2021 •

edited

mainrs commented Aug 26, 2022 •

edited