Embed option

[pkel]

I tried to implement @wisp3rwind's proposal to fix issue #59. What do you think?

[wisp3rwind]

This looks good already! However, I think I'd prefer the config option to be

Another approach to the configuration might be to add an alternatives..sync-art = ["no"|"embed"|"copy"] option.

(without copy for now). That'd reflect better that a typical (I think) would only want either of the options to be set, and we wouldn't accumulate boolean options if other methods to sync art were added. What do you think @geigerzaehler, @pkel?

[pkel]

I see a few disadvantages:

• it's an unnecessary restriction for the users
• internally we'd still have to derive bools from the tertiary option
• if we fall back to convert.embed we should also respect convert.copy_album_art, what if both are true?
• can we put the fallback logic in simple words for the documentation?
• convert is an official plugin, why not reuse the established configuration logic?

But I have no strong opinion. My use case is to copy not embed so I'd be happy with your proposal too.

[wisp3rwind]

I see a few disadvantages:

* it's an unnecessary restriction for the users

* internally we'd still have to derive bools from the tertiary option

* if we fall back to `convert.embed` we should also respect `convert.copy_album_art`, what if both are true?

* can we put the fallback logic in simple words for the documentation?

* convert is an official plugin, why not reuse the established configuration logic?

But I have no strong opinion. My use case is to copy not embed so I'd be happy with your proposal too.

Fair enough, thanks for laying out in detail your reasoning here! There's one small typo in the new documentation, otherwise this looks good to merge 🎉

[geigerzaehler]

✨ and thanks for documenting it in detail.

Could you please add a test that checks that we don’t embed if the option is set to false? And could you also add an entry in CHANGELOG.md? This should be under a new “Upcoming” heading.

[kergoth]

I see a few disadvantages:

* it's an unnecessary restriction for the users

* internally we'd still have to derive bools from the tertiary option

* if we fall back to `convert.embed` we should also respect `convert.copy_album_art`, what if both are true?

* can we put the fallback logic in simple words for the documentation?

* convert is an official plugin, why not reuse the established configuration logic?

But I have no strong opinion. My use case is to copy not embed so I'd be happy with your proposal too.

Fair enough, thanks for laying out in detail your reasoning here! There's one small typo in the new documentation, otherwise this looks good to merge 🎉

I think the sync_art option option may work better for when we enhance the copy_art branch to handle Symlink Views. There are unclear semantics around copy_album_art and a symlink view, does it make sense to copy art into a symlink tree, or should it symlink the art? If the latter, the option name doesn't make sense. So we'd likely want either an additional boolean, or go the sync_art option and add a 'link' option to it.. or we just make the assumption that either copy or embed actually means link for a symlink view.