Write tags only when changed

[geigerzaehler]

This was previously discussed in #527 and some questions came up.

Unintentionally capturing modifications to other fields, such as when mgfile is assigned

It’s better to be on the safe side and perform unnecessary writes instead of missing necessary ones. Also
mgfile should never ever be changed.

Missing changes when doing other kinds of modifications, like the delete operation used by the scrub plugin

delete is handled here and tested here

The potential inefficiency of reading each field twice on each write, especially for images

Files are not read twice since all tags are cached when the mgfile is created.

[sampsyo]

Sorry for the extremely long delay on reviewing this! I've had a few notes I wanted to jot down here but haven't gotten around to typing them up. My apologies. (Also, it now seems like we may be converging on another solution in #644, so these thoughts may be irrelevant anyway. 😳)

Overall, I totally understand the motivation for centralizing this logic in __setattr__, but having the laziness check outside of the various translation layers in the MediaFields has a few unfortunate consequences. The first is just performance—it's unfortunate that we need to read the attribute multiple times in order to write it and invoke the translation machinery every time—but that's secondary. It can also lead to some observable behavior changes:

• In __init__, we assign to the mgfile field of the MediaFile. This sets the dirty flag in the current implementation.
• Say you assign to art (i.e., the legacy CoverArtField). This causes the images array to change to have just one element. But if you assigned the right image into art, that assignment would not be detected as a change. So the change to the length of images would not be persisted on save().
• Similarly, the current implementation without the dirty flag will (e.g.) normalize weird formatting in ReplayGain fields when they're assigned. With the dirty flag, the field only gets saved (and the formatting updated) if the encoded value actually changes.

These issues might be mostly benign, but it's good to keep them in mind if we do this (and possibly to provide a way to work around dirty-tracking to avoid them when necessary).

[geigerzaehler]

Thanks for the deep insight and digging up bugs I haven’t thought about. Points 2 and 3 also apply to #644, so I’d like to shift the discussion there. In fact, we should postpone this PR until #644 is resolved since it implements parts of this.

In __init__, we assign to the mgfile field of the MediaFile. This sets the dirty flag in the current implementation.

That’s why I set the dirty flag to false at the end of __init__. The tests show that this works just fine.

[arcresu]

@geigerzaehler I know it's been quite a long time since you opened this PR, but as a heads up, we're planning to extract MediaFile to an independent Python package & git repository (see #3237). Do you still think this PR is worth pursuing? If so we'll need to move it over to the new repository.

[jtpavlock]

Closing as abandoned/deals with MediaFile