Apply tag-changing plugins before the change is displayed.

[Hawke]

Currently it seems that plugins which change the tag values (such as the zero plugin) happen after the tag deltas have been displayed to the user.

So this means that beets will sometimes report a change when there is none.

For example I use the zero plugin to clear the albumartist tag when it would otherwise be set to 'various artists'. Unfortunately, this means that when the 'mbsync' plugin runs, it sees that the old value is '' and the "new" value (from musicbrainz) is 'various artists', and beets displays a change for every VA track.

It seems like the flow should go:
old values→new value lookup→tag rewriting→display changes → save changes
whereas it is currently
old values→ new value lookup→display changes→tag rewriting → save changes.

Not sure if the tag change display is specific to the mbsync plugin or if this a general flow (and would therefore apply to 'beet update' as well)

[sampsyo]

This would definitely be a usability enhancement. I'm not 100% clear where these plugins should interpose (I think they'd need to modify candidates from the data sources directly), but I'm sure we can design something worthwhile.

Can we make a list of plugins that work like this? Is it just zero?

[rafi]

.. and the, ftintitle, scrub

[sampsyo]

ftintitle makes sense (although it doesn't currently have an automatic hook; see #485), but scrub is a bit different in that it doesn't affect metadata, only on-disk tags.

[rafi]

And the the plugin?

[sampsyo]

Well, currently, the is only used for template formatting—it doesn't actually affect metadata.

[m-urban]

Is there any update on this? I would love to rewrite some tags with a plugin tapping into the "write" event and seeing the "real" changes reflected in the output would be really cool.
However, I would also like to have these changes reflected in the library and potentially file names, so a hook to intercept the fetched meta data sources would be awesome!

[m-urban]

I have added a few lines to the code (17d87cc), which would solve this for me: With those lines, plugins could tap into the events albuminfo_received and trackinfo_received, which provide a callback with the respective album/track info dictionaries, which can then be used to further manipulate the fetched data. Consequently, applied string manipulations show up in the red diff view as well.

[sampsyo]

@m-urban Seems good to me! Can you please add documentation for these new events? It would also be really helpful to examine which existing plugins should use the new hooks.

[m-urban]

Right now, this is more a proof of concept, really. There's a couple pitfalls:

• Right now, the events are only triggered by the MusicBrainz plugin. They would need to be added to all other sources as well.
• Both proposed events pass over the fetched AlbumInfo and TrackInfo objects. Now it's a little tricky to keep all tag-variables in scope, in order to really manipulate the *Info objects and their members. I don't have a particularly strong Python background… is there anything similar to pointers or references?
• If multiple plugins subscribe to these events, who determines, which plugin performs its manipulations first? One plugin might change a tag only to have its changes overwritten by a different one later.
• Moreover it's fairly inefficient, since the modifications are made to all fetched data before the user / autotagger decide which set of data to process.

Other than that, it appears possible to have plugins, such as ftintitle, zero and the, make use of the new infrastructure with reasonable efforts…

[sampsyo]

Ah! Good points all. Maybe we should start a branch to start hacking on this in more detail. For example, maybe we should emit the vents from beets.autotag.hooks so we don't need to add it on a per-source basis.

I'm not quite sure I understand the second bullet—what's currently tricky about manipulating those objects? They are in need of a redesign (they should be more like ordinary dictionaries rather than objects with long lists of fields) anyway, so we could use this change as an excuse to do that now.

Multiple handlers is a very good point, related to #1463. I have no immediate solution.

I'm not overly worried about the potential inefficiency, but we could consider somehow moving the event emission to after the scoring is complete.

[m-urban]

Initially I planned to emit the events from within the AlbumInfo / TrackInfo's constructors, however, I believe that changes in the event handlers could potentially be overwritten again, which would render the changes useless. The chaining of plugin handlers doesn't exactly simplify this either.

Regarding my second point: The *Info objects are passed to the handler. The handler modifies some tags and returns. However, it returns without passing the modified *Info object back to the event emitter. We somewhat need to guarantee that future clients of the *Info object access the modified version. Now if the *Info object and its members were "C-style" pointers, that would be easy to achieve.

[sampsyo]

I think that's OK—since objects are accessed by reference in Python, and never copied unless explicitly requested, multiple handlers should all see and mutate the same data.

[m-urban]

If threaded: is enabled - would multiple plugins potentially try to modify the meta data concurrently? That could be dangerous. How about the single-threaded mode: would multiple subscribed plugins run sequentially one after another?

[sampsyo]

Since the threading in the importer is pipelined, only one task at a time is in any given state. So, in particular, a single Info object will only be touched in one thread at a time, so plugins will see the objects in sequence.

[m-urban]

Thank you, for clarifying threading - sounds well designed, so this will not become an issue here.

I have just committed 69088e4, which moves the proposed events to the hooks interface, so plugins don't need to be touched.

There's one more thought: If, for instance, I want to rewrite the title of every track I import, a later mbsync would revert those changes. Therefore, I've also added code to the hooks interface, that allows a subscribed plugin to apply the exact same changes to mbsync-fetched (or any other plugin) data, in order to minimize the diff. Maybe you can have a look at the commit mentioned above and tell me what you think.

[sampsyo]

This looks like it's on the right track! I agree that doing this in the hooks infrastructure will affect mbsync (and anything else that fetches metadata). Cool.

Sorry for not being able to figure this out from the GitHub interface, but are these commits on a branch? If so, we can start putting together the documentation and such there.

[m-urban]

I've forked the repo, there's a branch here: tag-change-hook. There isn't much going on there other than the changes you've just seen, but I can add some documentation and update the changelog as well. That way, people could start making use of this infrastructure already. As I am not using the way above mentioned ftintitle and the plugins, I am not yet sure, how to integrate these, though.

[sampsyo]

Looks great. I'm all for merging the additional events even before we figure out how to apply them to the existing plugins.

[m-urban]

Great, I've created a PR #1499

[Kernald]

I'm not overly worried about the potential inefficiency, but we could consider somehow moving the event emission to after the scoring is complete.

I don't know what the final decision was, but moving the event emission to after the scoring looks counter-productive to me. When updating tags like when mbsync is running, you would have some diffs, even if the change is e.g. e feat. moved by ftInTitle. Maybe we should emit two events, one before and one after, and choose on a per-plugin basis?

[sampsyo]

Sure, I could definitely buy that some metadata changes want to affect scores and others do not.