create plugin 'remove_[data_track]s' (formerly skip_data_tracks_and_fix_track_numbers)#388
create plugin 'remove_[data_track]s' (formerly skip_data_tracks_and_fix_track_numbers)#388crnkv wants to merge 7 commits into
Conversation
Sophist-UK
left a comment
Sophist-UK
left a comment
There was a problem hiding this comment.
This looks useful and generally well written.
|
|
||
| def remove_datatracks_from_release(album, metadata, release): | ||
| try: | ||
| log.info("{0}: Infomation: {1}".format(PLUGIN_NAME, "Removing data tracks / silence tracks from release "+release['title'],)) |
There was a problem hiding this comment.
Given that this is going to be a public plugin, I think this should only be shown IF the plugin is actually going to remove tracks,
| del disc['tracks'][i] | ||
| disc['track-count'] = len(disc['tracks']) | ||
| if len(datatrack_positions) != 0: | ||
| log.info("{0}: Infomation: {1}".format(PLUGIN_NAME, "Removed data tracks / silence tracks of positions: "+str(sorted(datatrack_positions)),)) |
There was a problem hiding this comment.
-
Personally I would like this to say "Removed X data / silence tracks, original positions: 2, 3, 5, 8.
-
log.info("{0}: Information: Removed {1} data / silence tracks, original positions: {2}", [PLUGIN_NAME, len(datatrack_positions), datatrack_positions]). This is better style, and better suited for future language translations. -
Perhaps better shown after the tracks have been renumbered.
| <li> -- totaltracks: 5 -- </li> | ||
| </ul> | ||
| This is automatic once the album is loaded. Users wouldn't be aware of the existence of data tracks and silence tracks. | ||
| <br /><br /> |
There was a problem hiding this comment.
Can you please include the MBID of a real release that exhibits as many of these examples in real-life as possible (so that we can have an example to use in testing)?
| or track['artist-credit'][0]['artist']['name'] == '[data]' \ | ||
| or track['recording']['artist-credit'][0]['artist']['name'] == '[data]' \ | ||
| or track['title'] == '[silence]' \ | ||
| or track['recording']['title'] == '[silence]' : |
There was a problem hiding this comment.
I think you should also include:
or track['~datatrack']in this list of triggers and avoid the need for the Track Metadata Processor to double check for this below.
There was a problem hiding this comment.
This is not possible because the %_datatrack% metadata (MusicBrainz variable) is not part of the album metadata or track metadata retrieved from the MusicBrainz JSON web service.
It seems that none of the arguments passed to the album_metadata_processor contains this _datatrack or ~datatrack boolean flag, while only the arguments passed to the track_metadata_processor gets populated with such flag(s) by MusicBrainz Picard through some logical inferences.
So, logically speaking, MusicBrainz Picard couldn't see no more information than what's already in the album_metadata_processor. There shouldn't be cases where the album JSON says no datatrack yet Picard still deduce the %_datatrack% flag. This really is just a double check.
If it happens, something very wierd must be happening, and I want to stay away from it.
Plus, if I continue to remove tracks within the track_metadata_processor based on the ~datatrack flag (yes it is possible), a lot more work has to be done: tracknumber, totaltracks, ~absolutetracknumber, ~musicbrainz_tracknumber, ~totalalbumtracks should be all updated asyncly, and these tracks are mutually dependent, that's quite a nightmare...
There was a problem hiding this comment.
Yes - I think it is as per these lines of code in Picard's album.py.
And I am NOT suggesting that you do anything in the Track Metadata Processor - I am suggesting that you eliminate it entirely.
There was a problem hiding this comment.
I spent some time looking into the codes. It seems that the lines you pointed out are for another scenario (that I missed out in my script), which is when the album JSON contains a json['media'][0]['data-tracks'] node (which is not seen in a real album example)
The ~datatrack flags per track are actually set in these lines. And, yes, as long as the if condition in those lines won't change in the future, my if conditions here would be more than sufficient - I'll remove the double check
| except Exception as ex: | ||
| log.error("{0}: Error: {1}".format(PLUGIN_NAME, ex,)) | ||
|
|
||
| def doublecheck_metadata(album, metadata, track, release): |
|
Sorry, one more comment which is the length of the filename - can we have a shorter name please? Also I feel the plugin name could be snappier - yes, it does what it says on the can, but how about "Remove [data] tracks" instead? |
crnkv
changed the title
all suggestions accepted. any other problems? can we proceed? |
2 participants
I wrote a plugin that is intended to present the track list in line with online digital music platforms. It automatically skips data tracks and silent tracks once the album metadata is loaded / retrieved.