Allow `mb_track_extract` to have the final say for all fields

[myndzi]

Description

In the source, the mb.album_info function calls track_info for each recording in a release... but then overrides certain values such as title, artist, and length. This unexpectedly causes hooks for mb_track_extract that return values for these fields to not have those values honored.

This PR defers calling the hook when mb.track_info is called from mb.album_info until after these album-context modifications have been made, thus allowing plugins to have the final say on the values for any fields.

Fixes #4512.

To Do

• [n/a] Documentation. (If you've add a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
• [x] Changelog. (Add an entry to docs/changelog.rst near the top of the document.)
• [x] Tests. (Encouraged but not strictly required.)

[myndzi]

(Integration testing with my plugin revealed an oversight -- I passed data=track to the hook in album_info which didn't match the track['recording'] that gets passed to track_info and from there to mb_track_extract. I updated the tests to be self-referential to their own data to catch this sort of thing, and fixed it)

[stale[bot]]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[sampsyo]

Super sorry for letting this PR languish, since it is clearly almost ready. One thing I would love to understand more deeply, but don't quite yet, is why we need a extract_hook flag to the track_info function. I understand from the comment is that there is a problem with "interference" between track_info and album_info, but I don't quite grok what that interference consists of… I would be interested in thinking through this a bit so we can simplify the interface.

[myndzi]

@sampsyo Apologies, I was checking occasionally on the issue but not the PR. I didn't notice you'd asked a question :(

Super sorry for letting this PR languish, since it is clearly almost ready. One thing I would love to understand more deeply, but don't quite yet, is why we need a extract_hook flag to the track_info function. I understand from the comment is that there is a problem with "interference" between track_info and album_info, but I don't quite grok what that interference consists of… I would be interested in thinking through this a bit so we can simplify the interface.

I honestly don't remember anything about it, I'd given up on this PR. I believe this comment in the test offers some additional context: https://github.com/beetbox/beets/pull/4513/files#diff-a7c72f70798fb60fdc13080e14bdf135969e77be5c2b4a639124e8ee8ea1393aR502-R505

and here: https://github.com/beetbox/beets/pull/4513/files#diff-a7c72f70798fb60fdc13080e14bdf135969e77be5c2b4a639124e8ee8ea1393aR531-R532

By my vague recollection, when processing a full album, the function that handles that calls the function that processes a track. But then, after that function returns, it sets values on the tracks. So, if it goes album -> track -> hook (modify track) -> back to album, album can overwrite the result of the track-processing hook.

[myndzi]

https://github.com/beetbox/beets/blob/57b5642b7cb6cacc54c8b0d619724acd19cb09df/beets/autotag/mb.py#L373-L384

This is probably what I was referring to. The code assigns the result of "track_info(...)" to "ti", then sets properties on "ti". The diff is confusing to read because Python, but the changes are actually to separate functions. The code here:

https://github.com/beetbox/beets/pull/4513/files#diff-ff51bca3f15a32bf8652a69e127b582652187bae2fd2fd7a1ccb58d0feeb97b2R283

is in the track_info function, and calls the hook to allow it to modify the track results.

The code here:

https://github.com/beetbox/beets/pull/4513/files#diff-ff51bca3f15a32bf8652a69e127b582652187bae2fd2fd7a1ccb58d0feeb97b2R379

is in the album info function, and tells the track info function to not run the hooks, since the result of the hook might be overwritten. Then, after performing its normal work, it separately calls the hook for each track (instead of allowing track_data to call the hook):

https://github.com/beetbox/beets/pull/4513/files#diff-ff51bca3f15a32bf8652a69e127b582652187bae2fd2fd7a1ccb58d0feeb97b2R397-R401

I was attempting to make minimal change since I wasn't super familiar with the code base and didn't want to go all refactor-y on someone else's code, especially when I didn't understand it all that well. Moving these lines:

https://github.com/beetbox/beets/blob/57b5642b7cb6cacc54c8b0d619724acd19cb09df/beets/autotag/mb.py#L381-L395

into the track_info function (by supplying an optional argument or something, for when the additional context that's only on the album data is available) would eliminate the need to pass a flag.

[stale[bot]]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[arsaboo]

It looks like this PR was close to getting merged...may be we keep it open for now.