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

Standardized Album Artist Name(s) #105

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
2 participants
@rdswift
Copy link
Contributor

commented Jun 1, 2017

Plugin to add two new tags with standardized album artist name(s) for
use when both standardized and credited names are required.

StdAlbumArtist = Standardized name of first artist in the AlbumArtist.

StdAlbumArtists = Standardized names of all artists in the AlbumArtist,
separated by semi-colons.

Standardized Album Artist Name(s)
Plugin to add two new tags with standardized album artist name(s) for
use when both standardized and credited names are required.

StdAlbumArtist = Standardized name of first artist in the AlbumArtist.

StdAlbumArtists = Standardized names of all artists in the AlbumArtist,
separated by semi-colons.
@Sophist-UK

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2017

A few comments:

  1. Have you checked that the standardized artist names are not already in the XML provided for the album rather than making new requests for them? (You should not make extra WS calls if you do not need to as these will slow down album loading quite a lot.)

  2. Because you are needing standardized names for (potentially) several artists, you will need to make several ws requests for artists and therefore probably need more coordinating code in your plugin to ensure you have all the artists before you finalise the album.

  3. You should think about using AlbumArtists and StdAlbumArtists as a way to replace all the artists in AlbumArtist with standardised names rather than just the first one. See other plugins (like Smart Title Text or Abbreviated Artists v2) for code to use as a starting point.

@rdswift

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2017

I haven't been able to find any documentation with respect to writing plugins for Picard, so I've been going through the Picard source code to try to follow how the plugins are called and what information is passed. I'll admit that I'm still a bit fuzzy on some aspects.

As to your questions:

  1. I hadn't, but after a quick look it appears that the information might be there as part of the included artist information. That will definitely simplify things and eliminate the extra ws queries. Thanks for bringing this to my attention.

  2. This took a lot of time and testing, but I think the coordinating code that I used as a basis (with a couple of small tweaks) is looking after that. This is likely a moot point now, considering your point 1.

  3. If I understand your comment, I think that's already being done (sort of). This plugin creates two tags: (1) StdAlbumArtist which contains the standardized name for the first artist in musicbrainz_albumartistid; and (2) StdAlbumArtists which contains the standardized names for all artists in musicbrainz_albumartistid, separated by a semi-colon. Perhaps it would be a bit more intuitive if the tags were changed to: (1) FirstStdAlbumArtist which contains the standardized name for the first artist in musicbrainz_albumartistid; and (2) StdAlbumArtist which contains the standardized names for all artists in musicbrainz_albumartistid, separated by the specified join phrase.

I have a need for the standardized name of the first artist as part of my file naming script, but others may not. For that reason, I'm wondering if it would make more sense to do that in a separate plugin. In any event, I'm going to close this pull request and re-work the code a bit before resubmitting.

Thanks for your comments. I appreciate the insight into the way Picard works with plugins.

@rdswift

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2017

Will incorporate Sophist-UK's comments and resubmit.

@rdswift rdswift closed this Jun 1, 2017

@Sophist-UK

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2017

Bob:

  1. If you want to see the XML returned to Picard do the following:
    a. Turn on debugging - open the Help / Error & Debug log, and tick the checkbox
    b. Load an album
    c. Look in the debug log for the URL that Picard uses to get XML, and copy and paste this URL into your browser.

  2. The documentation is definitely lacking - so using existing plugins as examples is your best bet. And navigating the XML to find what you want is a bit hot and miss to get right.

  3. Next time, leave the PR open and just add commits to your own repo (and sync if you have cloned your repo onto your PC). Your changes will automatically be reflected in the PR.

Update to version 0.2
+ Complete rewrite to utilize the album metadata processor rather than
  the track metadata processor

+ Eliminate all extra calls to the ws service by using the information
  already available in the album xml parameter.

+ Slight change to plugin name.

+ Added revision history in comments.
@rdswift

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2017

  1. Good idea, but I already figured it out by going through the code for Picard. Debug is still very useful.

  2. Sorting through the XML took a bit of time, but not too bad. Checking for the joinphrase attribute tripped me up for a while because I forgot to check for its existence before trying to access it.

  3. ARGH! It never crossed my mind that the PR would be automatically updated. Something to remember for the future. I'll try reopening it and see what happens.

Thanks again for your help to point me in the right direction.

@rdswift rdswift reopened this Jun 2, 2017

rdswift added some commits Jun 2, 2017

Cleanup from code review
* Remove unused import item.

* Add static method identifier.

@rdswift rdswift closed this Jun 8, 2017

@rdswift

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2017

Completely rewritten. Will create a new pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.