Refactor Config structs according to features

[CPerezz]

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

[CPerezz]

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]

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

[slondr]

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]

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]

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

[CPerezz]

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.

[robinvd]

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

[mainrs]

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]

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]

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[bot]]

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]

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

[mainrs]

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]

Thanks for the review comments @eladyn

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

Thank you all! :)