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

Disable album art embedding #59

Open
pkel opened this issue Oct 13, 2020 · 3 comments
Open

Disable album art embedding #59

pkel opened this issue Oct 13, 2020 · 3 comments

Comments

@pkel
Copy link

pkel commented Oct 13, 2020

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
Copy link
Author

pkel commented Oct 13, 2020

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.

@geigerzaehler
Copy link
Owner

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.

@wisp3rwind
Copy link
Collaborator

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.

pkel added a commit to pkel/beets-alternatives that referenced this issue Oct 17, 2020
@pkel pkel mentioned this issue Oct 17, 2020
kergoth pushed a commit to kergoth/beets-alternatives that referenced this issue Nov 4, 2021
kergoth added a commit to kergoth/beets-alternatives that referenced this issue Nov 4, 2021
* copy-art:
  Introduce copy_album_art option
  Fix typo
  Introduce embed option. Solves geigerzaehler#59.

Signed-off-by: Christopher Larson <kergoth@gmail.com>
kergoth added a commit to kergoth/beets-alternatives that referenced this issue Nov 5, 2021
* kergoth-copy-art:
  Operate only on the albums which have files in the external
  COPY_ART for symlink views
  Use lexists in matched_album_action
  For copy_art, only include albums if they're the only album in the dest_dir
  For copy_art, only include albums if all the items share a dest_dir
  Fix an error with copy_album_art_pp
  Hack album art post processing.
  Introduce copy_album_art option
  Fix typo
  Introduce embed option. Solves geigerzaehler#59.

Signed-off-by: Christopher Larson <kergoth@gmail.com>
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

No branches or pull requests

3 participants