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

[conf] Gracefully handle the change of config options for modifying playlists #748

Merged
merged 4 commits into from
Jun 16, 2019

Conversation

chme
Copy link
Collaborator

@chme chme commented May 30, 2019

@ejurgensen I think it would be best not to break existing config files. This pr readds the old options in the 'mpd' section and displays a deprecation error in the log if they are present.

In a future version the deprecated options could then be dropped completely.

@ejurgensen
Copy link
Member

Yes, agree, what do you think about using CFGF_DEPRECATED and/or CFGF_DROP? See libconfuse/libconfuse#24

Then perhaps the "Handle deprecated config options" would not be required. In any case I think that code should be in mpd.c, not in conffile.c.

BTW maybe "default_playlist_directory" should be called "saved_playlists_dir": 1) we use "dir" later in the config file, but "directory" is not used in other option names, 2) I don't really understand the "default" part...

@chme
Copy link
Collaborator Author

chme commented Jun 15, 2019

I've updated the pr to use CFGF_DEPRECATED and moved the deprecation handling to mpd.c.
CFGF_DROP would only be useful, if we would write the config file after parsing it.

In order to get the deprecation messages from libconfuse in the forked-daapd log, it is necessary to set cfg_set_error_function. I don't really like the modification i have done to logger.c/h, but could not think of a better approach.

There seems to be a bug in libconfuse that leads to multiple log entries for a deprecated option (i have to investigate further before opening an issue for libconfuse - at first glance this seems to happen, if the deprecated option is followed by comments). But having multiple entries in the log is better than none :-)

@ejurgensen ejurgensen merged commit a28e370 into owntone:master Jun 16, 2019
@ejurgensen
Copy link
Member

Merged, thanks!

@chme chme deleted the conf_deprecated branch June 17, 2019 19:20
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.

None yet

2 participants