beets-alternatives icon indicating copy to clipboard operation
beets-alternatives copied to clipboard

Disable album art embedding

Open pkel opened this issue 3 years ago • 3 comments

This issue is follow-up for a question raised in #58.

I said:

I'm confused by the implementation of sync_art / embed. From reading the code it seems that album art is embedded even if convert.embed is set to false. More specifically, I think SYNC_ART is set independent of the configuration option.

@geigerzaehler answered:

Yes, you’re right. Do you expect covert.embed to control that behavior (even though it configures a different plugin)? Do we need our own configuration setting for this? Feel free to discuss this in an issue.

pkel avatar Oct 13 '20 06:10 pkel

I think the convert.embed option is considered when a song is added to the alternative directory:

https://github.com/geigerzaehler/beets-alternatives/blob/master/beetsplug/alternatives.py#L339

So yes, my expectation is that also beet alt update considers the convert.embed config option.

Stepping back a little, I think it would be better (and easier to implement) to have a separate alternatives.name.embed option. Similar to the copy_album_art option I propose in #58.

pkel avatar Oct 13 '20 06:10 pkel

Stepping back a little, I think it would be better (and easier to implement) to have a separate alternatives.name.embed option

For me this seems to be the right way forward, too.

geigerzaehler avatar Oct 13 '20 08:10 geigerzaehler

Yeah, embed should really be a per-alternative option. Maybe, for backwards compatibility, it could default to convert.embed if not set, i.e.

self._embed = config.get(dict).get(
    'embed',
    convert_plugin.config["embed"].get(bool)
)

In fact, the _embed handling should probably be moved to the External() class rather than ExternalConvert(): You're actually correct that art embedding is not handled consistently. ExternalConvert() checks converts embed option when initially adding items to the library, but it inherits External()s methods which will embed the art anyway on the next alt update.

I'll see if I can find some time to fix this, which could then serve as a starting point for your other PR. I won't be able to do so before the weekend, however.

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

wisp3rwind avatar Oct 13 '20 12:10 wisp3rwind