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

Copy album art #58

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Copy album art #58

wants to merge 1 commit into from

Conversation

pkel
Copy link

@pkel pkel commented Oct 11, 2020

This is my first take on #43 . It's the first time I look into beets (plugin) code. I will need your input to bring this into shape.

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.

PS: Thanks for developing this awesome plugin.

@geigerzaehler
Copy link
Owner

Thanks for tackling this, @pkel! This is already a great start. The next step woulds be to document this feature in README.md and write some tests.

The documentation should mention the fine points of this implementation:

  • We only add album art if the whole album matches the selector.
  • We base the location on the directory of the first track on the album
  • We potentially override album art when multiple songs from the same album share a directory
  • We never clean up cover art when the album is removed from the collection.

The tests should include the following test cases

  • Album art is copied if album is matched
  • Album art is not copied if album is not matched
  • Album art is not copied for single tracks where album is not matched
  • Album art copy works even if albums share the same directory
  • Album art copy does nothing for symlink view (This is not yet implemented)

After this we can tweak the implementation.

If you need more help, feel free to ask for it.

From reading the code it seems that album art is embedded even if convert.embed is set to false.

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.

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