beetbox / beets

music library manager and MusicBrainz tagger
http://beets.io/
MIT License
12.95k stars 1.82k forks source link

Apply the album-level media field from AlbumInfo #2511

Open asafsarid opened 7 years ago

asafsarid commented 7 years ago

Problem

When I get a result from beatport, and I want to catch the $media variable in the path section, it is blank, other parameters seems to be ok. For example $data_source is returning Beatport as it should according to https://github.com/beetbox/beets/blob/master/beetsplug/beatport.py#L422

Running this command in verbose (-vv) mode:


Asswel - Times Out(8 items)                                                                                                             
Tagging:                                                                                                                                                                                      
    Asswel - Times Out                                                                                                                                                                        
URL:                                                                                                                                                                                          
    http://beatport.com/release/times-out/1965715                                                                                                                                             
(Similarity: 84.1%) (tracks, source) (Beatport, Digital, 2017, Soundfield)
$ ls -la                                                                                                                                                                                 
drwxrwxr-x 7 xxx xxx 4096 Apr 13 13:26 .
drwxr-xr-x 4 xxx xxx 4096 Apr 13 13:26 ..
drwxrwxr-x 2 xxx xxx 4096 Apr 13 13:26 Asswel - Times Out () [FLAC]

Happening on every album from beatport

Setup

My configuration (output of beet config) is:

path:
    default: $albumartist - $album ($media) [$format]/%if{$disc_layer,$disc_layer/}$track - $title
sampsyo commented 7 years ago

Sure; sounds good! I don't know whether the media file is available from the Beatport API, but if it is, we should capture it.

sampsyo commented 7 years ago

Actually... it looks like we are trying to use the string "Digital" for $media. I'm not sure why this isn't showing up in your path... https://github.com/beetbox/beets/blob/master/beetsplug/beatport.py#L421

asafsarid commented 7 years ago

Yes, I saw this as well https://github.com/beetbox/beets/blob/master/beetsplug/beatport.py#L421 Looking at the code seems like it should always be Digital (should it?). Anyway I checked few times to see that my config is fine and the $media variable returns nothing (empty string). While other variables such as catalognum album artist etc are fine.

sampsyo commented 7 years ago

Hmm; it looks like we only actually apply track-level media information: https://github.com/beetbox/beets/blob/master/beets/autotag/__init__.py#L152

The fault lies with the apply_metadata function, then, rather than with the beatport plugin itself. We should totally apply the album-level version of the field too.

tigranl commented 7 years ago

Hi, @sampsyo! How to get media information from Beatport release object? I don't see media field on the api page. Maybe I should use /source-types method?

sampsyo commented 7 years ago

Hello again, @tigranl!

I think the idea here is that music downloaded from Beatport is always on digital media—it's the nature of the service. So, re-reading my previous comment, it looks like we just need to take that value of the media field on AlbumInfo in apply_metadata and apply_album_metadata and transfer it to the newly-imported music. Does that make sense?

tigranl commented 7 years ago

It does, but I'm not sure how to set media field in apply_metadata. It seems like album_info already contains media information.

sampsyo commented 7 years ago

I think we currently have this:

        if track_info.media is not None:
            item.media = track_info.media

but we should also look for the media on album_info, if it exists.

tigranl commented 7 years ago
if album_info.media is not None:
    item.media = album_info.media

Do you mean this?

sampsyo commented 7 years ago

Yeah, totally; that looks right to me. 👍 Of course, it would be worth giving it a try with the Beatport plugin enabled...

tigranl commented 7 years ago

Unfortunately beets was able to find all my music using only musicbrainz. Is there a way to make the Beatport a default catalogue?

sampsyo commented 7 years ago

For this sort of thing, I often use the -t (timid) mode in the importer. That option always lets me manually select the match I want—so if there’s metadata in Beatport, I can select that.

tigranl commented 7 years ago

Even using -t it still matches only Musicbrainz metadata.

sampsyo commented 7 years ago

Hmm; maybe it would be worth double checking that (a) the plugin is enabled and (b) you’re using an album that exists on Beatport, by searching in its web interface. Otherwise, the verbose log might offer clues about what th plugin is doing.

tigranl commented 7 years ago

I don't know what's the problem, but I didn't have any luck in troubleshooting. Anyway, entering ID manually works fine. I tested your idea to look for the media on album_info, and it seems that $media variable is still blank.

sampsyo commented 7 years ago

Cool: manual ID entry sounds great!

That’s mysterious, though—any interesting results from print-debugging in that function?

tigranl commented 7 years ago

So, item.media prints actually give a delivery mechanism (Digital).

sampsyo commented 7 years ago

Hmm, so it must be lost somewhere along the way. Perhaps it’s time for a PR so other people can give the patch a try?