DB v37: Synchronize file modified time with track source on metadata import/export #3978

uklotzde · 2021-06-10T21:56:33Z

The last, and most complicated PR.

Before

Set the boolean flag header_parsed = true after track metadata has initially been imported from the file (= source). Then this flag is never modified again ever after.

After

Store a time stamp source_synchronized_ms (milliseconds since Unix epoch origin) that reflects the corresponding file last modified time after importing/exporting track metadata from/to source. In Mixxx source is equivalent to file, because we only support local files.

Please note that we don't need to distinguish between import and export here! If the time stamp equals that of the file we consider the metadata in the library as synchronized with its source.

The new field is initialized with NULL and set or updated whenever importing/exporting track metadata.

The legacy flag is still maintained for backwards compatibility. I have deliberately renamed all usage of the legacy flag to header parsed.This reflects the column name in the database schema and avoids confusion with the new field.

Why

This new database field would allow to detect when metadata needs to be reimported after files have changed on the file system, e.g. after editing metadata with an external tag editor.

This feature has been requested frequently for Mixxx, both in the forums and on the bug tracker:
https://bugs.launchpad.net/mixxx/+bug/905669
https://bugs.launchpad.net/mixxx/+bug/1204164
https://bugs.launchpad.net/mixxx/+bug/1085691

The aoide media tracker already supports this field right from the start and works as expected. Unfortunately, I don't have any plans to improve and extend the legacy library scanner in Mixxx. It remains as is.

Possible Extensions (not part of this PR)

Add a new library column in the UI

…xport

daschuer

Did I understand it correct, that "source_synchronized_ms" always matched the file time of the track?
What happens if the metadata is written to the file by Mixxx?
I have not found the palace where we update the time after writing.
Does this happen implicit?

I know of some tagging tools that allow to write the metadata without touching the file time.
Will this cause issues here?
I think the only thing will happen is that Mixxx does not recognize the new Metadata.
Is there a Risk that Mixxx restores the old data unintended?

daschuer · 2021-06-13T22:34:06Z

src/track/trackrecord.cpp

+    VERIFY_OR_DEBUG_ASSERT(sourceSynchronizedAt.isValid()) {
+        // Cannot be reset after it has been set at least once.
+        // This is required to prevent unintended and repeated
+        // reimporting of metadata from file tags.


I don't understand why we cannot change the time after it was set once.

Reset time stamp = forget that we have imported the metadata at least once. As a consequence the import would be repeated when loading the track, thereby losing all intermediate edits in Mixxx. Works as before with the boolean flag, that ensures that metadata is only imported once and then only explicitly upon request.

daschuer · 2021-06-13T22:37:03Z

src/track/trackrecord.cpp

+bool TrackRecord::isSourceSynchronized() const {
+    // This method cannot be used to update m_headerParsed
+    // after modifying m_sourceSynchronizedAt during a short
+    // moment of inconsistency. Otherwise the debug assertion


Is "Otherwise" correct here? Or should it be "This will violate the debug assertion"

Otherwise = if used (vs. "cannot be used")

Now I am completely confused ...

This method requires that both members are in sync and must only be invoked while they are consistent. This is not guaranteed while updating them. In this case we must repeat some of the code although it seems like we could simply use m_headerParsed = isSourceSynchronized() for this purpose.

If the comment is confusing I could delete it. But I guarantee that someone will try to do this at some point to remove redundancy.

Unfortunately, we need to keep and deal with this redundancy for backward compatibility.

Ah OK, I understand it now.

If the comment is confusing I could delete it.

In general more comments is better then less. How about adding the explanations form your comments above instead of the original comment? The forbidden code snipped is the best of it.

daschuer · 2021-06-13T23:02:52Z

Did I understand it correct, that "source_synchronized_ms" always matched the file time of the track?

Yes, I have found it.

uklotzde · 2021-06-13T23:08:47Z

If files tags are modified by external tools without touching the file's modified time stamp then neither Mixxx nor any other application is able to detect these changes. It is out of scope and out of our control.

How to deal with this is out of the scope of this PR. If we consider to take the new time stamp into account it could only get better. Currently we neither detect external changes that need to be imported, nor do we prevent overwriting external file modifications on export.

daschuer · 2021-06-13T23:12:49Z

Ah OK, So this should be safe.

uklotzde · 2021-06-13T23:14:46Z

How to safely use the time stamp for smart synchronization is out of the scope. First we need to start storing and tracking it. Currently there is no automatic synchronization at all in Mixxx.

daschuer · 2021-06-14T08:07:09Z

While testing this branch, I notice that the key value in the track property dialog changes from Traditionl to OpenKey notation after reload metadata from file. Is this related to this branch? Probably not.

Also not a regression from this branch, but it prevents testing it:

If I click "reload metadata from file" and leave the dialog with OK I got this:

critical [Main] DEBUG ASSERT: "m_recentTrackId == pTrack->getId()" in function void BaseTrackCache::replaceRecentTrack(TrackId, TrackPointer) const at /home/daniel/workspace/qt5mixxx/src/library/basetrackcache.cpp:165
critical [Main] DEBUG ASSERT: "!pTrack" in function void BaseTrackCache::replaceRecentTrack(TrackId, TrackPointer) const at /home/daniel/workspace/qt5mixxx/src/library/basetrackcache.cpp:198

uklotzde · 2021-06-14T08:23:58Z

Unrelated

While testing this branch, I notice that the key value in the track property dialog changes from Traditionl to OpenKey notation after reload metadata from file. Is this related to this branch? Probably not.

Also not a regression from this branch, but it prevents testing it:

If I click "reload metadata from file" and leave the dialog with OK I got this:
critical [Main] DEBUG ASSERT: "m_recentTrackId == pTrack->getId()" in function void BaseTrackCache::replaceRecentTrack(TrackId, TrackPointer) const at /home/daniel/workspace/qt5mixxx/src/library/basetrackcache.cpp:165
critical [Main] DEBUG ASSERT: "!pTrack" in function void BaseTrackCache::replaceRecentTrack(TrackId, TrackPointer) const at /home/daniel/workspace/qt5mixxx/src/library/basetrackcache.cpp:198

Probably caused by #3906. I'll take care of that.

…ynchronized-timestamp # Conflicts: # src/track/trackrecord.cpp

daschuer · 2021-06-17T08:47:16Z

There is now a mismatch between

Re-Import Metadata from file (wong capitalizationd btw.)
Import Metadata from Musicbrains

the later is applied immediately to track while the later requires Apply

daschuer · 2021-06-17T09:00:02Z

OK that is an old issue, I will file a bug.

daschuer · 2021-06-17T09:04:30Z

https://bugs.launchpad.net/mixxx/+bug/1932270

daschuer

This one LGTM, Thank you.

uklotzde · 2021-06-17T09:28:49Z

There is now a mismatch between
* Re-Import Metadata from file (wong capitalizationd btw.)

* Import Metadata from Musicbrains
the later is applied immediately to track while the later requires Apply

The MusicBrainz import also came to my mind after fixing the file import bug. Unfortunately, I didn't find time to analyze it. I agree, let's better fix it separately. Thanks for filing a bug!

The MusicBrainz import dialog should ideally not operate directly on the Track object, which requires to rewrite a major part of it.

Synchronize file modified time with track source on metadata import/e…

38e51a4

…xport

uklotzde added this to the 2.4.0 milestone Jun 10, 2021

github-actions bot added code quality library labels Jun 10, 2021

uklotzde changed the title ~~Synchronize file modified time with track source on metadata import/export~~ DB v37: Synchronize file modified time with track source on metadata import/export Jun 11, 2021

daschuer reviewed Jun 13, 2021

View reviewed changes

uklotzde mentioned this pull request Jun 15, 2021

Fix reimport of track metadata #3998

Merged

Merge branch 'main' of git@github.com:mixxxdj/mixxx.git into source-s…

de39993

…ynchronized-timestamp # Conflicts: # src/track/trackrecord.cpp

daschuer approved these changes Jun 17, 2021

View reviewed changes

daschuer merged commit 20cfad2 into mixxxdj:main Jun 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DB v37: Synchronize file modified time with track source on metadata import/export #3978

DB v37: Synchronize file modified time with track source on metadata import/export #3978

uklotzde commented Jun 10, 2021 •

edited

Loading

daschuer left a comment

daschuer Jun 13, 2021

uklotzde Jun 13, 2021 •

edited

Loading

daschuer Jun 13, 2021

uklotzde Jun 13, 2021

daschuer Jun 13, 2021

uklotzde Jun 13, 2021

uklotzde Jun 13, 2021

daschuer Jun 14, 2021

daschuer commented Jun 13, 2021

uklotzde commented Jun 13, 2021

daschuer commented Jun 13, 2021

uklotzde commented Jun 13, 2021

daschuer commented Jun 14, 2021

uklotzde commented Jun 14, 2021

daschuer commented Jun 17, 2021

daschuer commented Jun 17, 2021

daschuer commented Jun 17, 2021

daschuer left a comment •

edited

Loading

uklotzde commented Jun 17, 2021

DB v37: Synchronize file modified time with track source on metadata import/export #3978

DB v37: Synchronize file modified time with track source on metadata import/export #3978

Conversation

uklotzde commented Jun 10, 2021 • edited Loading

Before

After

Why

Possible Extensions (not part of this PR)

daschuer left a comment

Choose a reason for hiding this comment

daschuer Jun 13, 2021

Choose a reason for hiding this comment

uklotzde Jun 13, 2021 • edited Loading

Choose a reason for hiding this comment

daschuer Jun 13, 2021

Choose a reason for hiding this comment

uklotzde Jun 13, 2021

Choose a reason for hiding this comment

daschuer Jun 13, 2021

Choose a reason for hiding this comment

uklotzde Jun 13, 2021

Choose a reason for hiding this comment

uklotzde Jun 13, 2021

Choose a reason for hiding this comment

daschuer Jun 14, 2021

Choose a reason for hiding this comment

daschuer commented Jun 13, 2021

uklotzde commented Jun 13, 2021

daschuer commented Jun 13, 2021

uklotzde commented Jun 13, 2021

daschuer commented Jun 14, 2021

uklotzde commented Jun 14, 2021

daschuer commented Jun 17, 2021

daschuer commented Jun 17, 2021

daschuer commented Jun 17, 2021

daschuer left a comment • edited Loading

Choose a reason for hiding this comment

uklotzde commented Jun 17, 2021

uklotzde commented Jun 10, 2021 •

edited

Loading

uklotzde Jun 13, 2021 •

edited

Loading

daschuer left a comment •

edited

Loading