Fix sort multivalue tags plugin #272
Conversation
|
|
||
| # Exclude the following tags because the sort order is related to other tags or has a meaning like primary artist |
There was a problem hiding this comment.
Isn't the same true for at least artists, albumartists, work, musicbrainz_workid, label, catalognumber, country, date and releasetype?
There was a problem hiding this comment.
Yes - this list is not correct. artist, artistsort, albumartist, albumartistsort are strings and not multi-value. It should have artists, ~albumartists, ~artists_sort, ~albumartists_sort in instead. And I think I agree about label and catalognumber (which match each other in sequence). releasetype is already in the list.
When I think (hard) about it, I can see that a medley could have multiple works in the sequence they are played, and that country and date are probably also in lock-step. So they should be added too.
I don't think this makes sense, you're trading one additional global variable at the module level (which is something we already have in plenty of picards modules) against a class with one static method and a class-level attribute. |
|
Ok. Just seemed to make more sense for plugins to have a consistent style and contain everything in a class But I can switch back easily enough. |
| # Exclude the following tags because the sort order is related to other tags or has a meaning like primary artist | ||
| _sort_multivalue_tags_exclude = [ | ||
| 'artists', '~artists_sort', 'musicbrainz_artistid', | ||
| 'albumartist', '~albumartists_sort', 'musicbrainz_albumartistid', |
There was a problem hiding this comment.
After #268 albumartists should be added here as well.
There was a problem hiding this comment.
Its a typo - albumartist should say albumartists.
|
Diff comments addressed. |
| 'releasetype', | ||
| ] | ||
|
|
||
| def sort_multivalue_tags(album, metadata, *args): | ||
| for tag in metadata.keys(): |
There was a problem hiding this comment.
You're getting the value a few lines later anyway, so you can just use metadata.iteritems here.
There was a problem hiding this comment.
Only once we have killed off the id3metadata class. Until then we cannot use __getitem__ (which I suspect iteritems uses internally) because it calls get which is overloaded. Which is why your PR to kill ID3metadata is a good idea!!!
There was a problem hiding this comment.
Oh, right, rawitems was the one that acts like dict.items or dict.iteritems.
|
Should hopefully be ready to merge now. |
Exclude certain multi-value tags from sorting because the order is important. Get raw list because id3metadata sub-class overrides the getall method. Make a class to avoid polluting the global namespace with the exclude_tags variable.
and move back to non-class form.
|
Does this plugin actually work as it is? If I enable it, it just splits tags like the track title etc. that are not multi-valued. |
|
It was working on my PC when I submitted it - though I haven't tested it against the latest daily build. The code is pretty simple - I can't see how it can split a tag. When I get the chance I will retest it. It should e.g. sort Performer tags. |
|
The problem seems to be that |
|
hmm - yes - metadata has an override for the standard dict iteritems which yields multi-value variables one-by-one. However, I am unsure why it does this - the only place I can find metadata.iteritems used elsewhere is at line c. 144 in file.py and this looks like it is expecting iteritems to yield a single multivalue not several single-values. In metadatabox.py c. line 381 we explicitly use the original dict.iteritems. I can easily change the line to read |
|
Please use |
Exclude certain multi-value tags from sorting because the order is important.