Automatically update track metadata from file tags if outdated

CMakeLists.txt

@@ -812,6 +812,7 @@ add_library(mixxx-lib STATIC EXCLUDE_FROM_ALL
   src/soundio/soundmanagerutil.cpp
   src/sources/audiosource.cpp
   src/sources/audiosourcestereoproxy.cpp
+  src/sources/metadatasource.cpp
   src/sources/metadatasourcetaglib.cpp
   src/sources/readaheadframebuffer.cpp
   src/sources/soundsource.cpp

src/library/dao/trackdao.cpp

@@ -20,6 +20,7 @@
 #include "library/dao/libraryhashdao.h"
 #include "library/dao/playlistdao.h"
 #include "library/dao/trackschema.h"
+#include "library/library_prefs.h"
 #include "library/queryutil.h"
 #include "library/trackset/crate/cratestorage.h"
 #include "moc_trackdao.cpp"
@@ -852,8 +853,9 @@ TrackPointer TrackDAO::addTracksAddFile(
 
     // Initially (re-)import the metadata for the newly created track
     // from the file.
-    SoundSourceProxy(pTrack).updateTrackFromSource();
-    if (!pTrack->isSourceSynchronized()) {
+    SoundSourceProxy(pTrack).updateTrackFromSource(
+            SoundSourceProxy::UpdateTrackFromSourceMode::Once);
+    if (!pTrack->checkSourceSynchronized()) {
         qWarning() << "TrackDAO::addTracksAddFile:"
                 << "Failed to parse track metadata from file"
                 << pTrack->getLocation();
@@ -1490,7 +1492,20 @@ TrackPointer TrackDAO::getTrackById(TrackId trackId) const {
         // file. This import might have never been completed successfully
         // before, so just check and try for every track that has been
         // freshly loaded from the database.
-        SoundSourceProxy(pTrack).updateTrackFromSource();
+        auto updateTrackFromSourceMode =
+                SoundSourceProxy::UpdateTrackFromSourceMode::Once;
+        if (m_pConfig &&
+                m_pConfig->getValueString(
+                                 mixxx::library::prefs::kSyncTrackMetadataConfigKey)
+                                .toInt() == 1) {
+            // An implicit re-import and update is performed if the
+            // user has enabled export of file tags in the preferences.
+            // Either they want to keep their file tags synchronized or
+            // not, no exceptions!
+            updateTrackFromSourceMode =
+                    SoundSourceProxy::UpdateTrackFromSourceMode::Newer;
+        }
+        SoundSourceProxy(pTrack).updateTrackFromSource(updateTrackFromSourceMode);
         if (kLogger.debugEnabled() && pTrack->isDirty()) {
             kLogger.debug()
                     << "Updated track metadata from file tags:"

src/library/library_prefs.cpp

@@ -28,7 +28,8 @@ const ConfigKey mixxx::library::prefs::kSearchDebouncingTimeoutMillisConfigKey =
                 mixxx::library::prefs::kConfigGroup,
                 QStringLiteral("SearchDebouncingTimeoutMillis")};
 
-const ConfigKey mixxx::library::prefs::kSyncTrackMetadataExportConfigKey =
+// The "Export" suffix in the key is kept for backward compatibility
+const ConfigKey mixxx::library::prefs::kSyncTrackMetadataConfigKey =
         ConfigKey{
                 mixxx::library::prefs::kConfigGroup,
                 QStringLiteral("SyncTrackMetadataExport")};

src/library/library_prefs.h

@@ -20,7 +20,9 @@ extern const ConfigKey kEditMetadataSelectedClickConfigKey;
 
 const bool kEditMetadataSelectedClickDefault = false;
 
-extern const ConfigKey kSyncTrackMetadataExportConfigKey;
+extern const ConfigKey kSyncTrackMetadataConfigKey;
+
+extern const ConfigKey kSeratoMetadataExportConfigKey;
 
 extern const ConfigKey kSeratoMetadataExportConfigKey;
 

src/library/trackcollectionmanager.cpp

@@ -285,7 +285,7 @@ void TrackCollectionManager::exportTrackMetadata(
             (pTrack->isDirty() &&
                     m_pConfig &&
                     m_pConfig->getValueString(
-                                     mixxx::library::prefs::kSyncTrackMetadataExportConfigKey)
+                                     mixxx::library::prefs::kSyncTrackMetadataConfigKey)
                                     .toInt() == 1)) {
         switch (mode) {
         case TrackMetadataExportMode::Immediate:

src/preferences/dialog/dlgpreflibrary.cpp

@@ -110,7 +110,7 @@ DlgPrefLibrary::DlgPrefLibrary(
 #endif
     builtInFormats->setText(builtInFormatsStr);
 
-    connect(checkBox_SyncTrackMetadataExport,
+    connect(checkBox_SyncTrackMetadata,
             &QCheckBox::toggled,
             this,
             &DlgPrefLibrary::slotSyncTrackMetadataExportToggled);
@@ -177,7 +177,7 @@ void DlgPrefLibrary::initializeDirList() {
 
 void DlgPrefLibrary::slotResetToDefaults() {
     checkBox_library_scan->setChecked(false);
-    checkBox_SyncTrackMetadataExport->setChecked(false);
+    checkBox_SyncTrackMetadata->setChecked(false);
     checkBox_SeratoMetadataExport->setChecked(false);
     checkBox_use_relative_path->setChecked(false);
     checkBox_show_rhythmbox->setChecked(true);
@@ -197,8 +197,8 @@ void DlgPrefLibrary::slotUpdate() {
     initializeDirList();
     checkBox_library_scan->setChecked(m_pConfig->getValue(
             ConfigKey("[Library]","RescanOnStartup"), false));
-    checkBox_SyncTrackMetadataExport->setChecked(
-            m_pConfig->getValue(kSyncTrackMetadataExportConfigKey, false));
+    checkBox_SyncTrackMetadata->setChecked(
+            m_pConfig->getValue(kSyncTrackMetadataConfigKey, false));
     checkBox_SeratoMetadataExport->setChecked(
             m_pConfig->getValue(kSeratoMetadataExportConfigKey, false));
     checkBox_use_relative_path->setChecked(m_pConfig->getValue(
@@ -365,8 +365,8 @@ void DlgPrefLibrary::slotApply() {
     m_pConfig->set(ConfigKey("[Library]","RescanOnStartup"),
                 ConfigValue((int)checkBox_library_scan->isChecked()));
     m_pConfig->set(
-            kSyncTrackMetadataExportConfigKey,
-            ConfigValue{checkBox_SyncTrackMetadataExport->isChecked()});
+            kSyncTrackMetadataConfigKey,
+            ConfigValue{checkBox_SyncTrackMetadata->isChecked()});
     m_pConfig->set(
             kSeratoMetadataExportConfigKey,
             ConfigValue{checkBox_SeratoMetadataExport->isChecked()});
@@ -454,7 +454,7 @@ void DlgPrefLibrary::slotSearchDebouncingTimeoutMillisChanged(int searchDebounci
 }
 
 void DlgPrefLibrary::slotSyncTrackMetadataExportToggled() {
-    if (isVisible() && checkBox_SyncTrackMetadataExport->isChecked()) {
+    if (isVisible() && checkBox_SyncTrackMetadata->isChecked()) {
         mixxx::DlgTrackMetadataExport::showMessageBoxOncePerSession();
     }
 }

src/preferences/dialog/dlgpreflibrarydlg.ui

@@ -132,16 +132,19 @@
      </property>
      <layout class="QGridLayout" name="gridLayout_3">
       <item row="0" column="0" colspan="2">
-       <widget class="QCheckBox" name="checkBox_SyncTrackMetadataExport">
+       <widget class="QCheckBox" name="checkBox_SyncTrackMetadata">
         <property name="text">
-         <string>Automatically write modified track metadata from the library into file tags</string>
+         <string>Synchronize track metadata with file tags</string>
+        </property>
+        <property name="toolTip">
+         <string>Automatically write modified track metadata into file tags and re-import metadata from updated file tags</string>
         </property>
        </widget>
       </item>
       <item row="1" column="0">
        <widget class="QCheckBox" name="checkBox_SeratoMetadataExport">
         <property name="text">
-         <string>Write Serato Metadata to files (experimental)</string>
+         <string>Write Serato metadata to files (experimental)</string>
         </property>
        </widget>
       </item>
@@ -443,7 +446,7 @@
   <tabstop>PushButtonAddDir</tabstop>
   <tabstop>PushButtonRelocateDir</tabstop>
   <tabstop>PushButtonRemoveDir</tabstop>
-  <tabstop>checkBox_SyncTrackMetadataExport</tabstop>
+  <tabstop>checkBox_SyncTrackMetadata</tabstop>
   <tabstop>checkBox_library_scan</tabstop>
   <tabstop>checkBoxEditMetadataSelectedClicked</tabstop>
   <tabstop>checkBox_use_relative_path</tabstop>

src/sources/metadatasource.cpp

@@ -0,0 +1,27 @@
+#include "sources/metadatasource.h"
+
+#include "util/logger.h"
+
+namespace mixxx {
+
+namespace {
+
+const Logger kLogger("MetadataSource");
+
+} // anonymous namespace
+
+// static
+QDateTime MetadataSource::getFileSynchronizedAt(const QFileInfo& fileInfo) {
+    const QDateTime lastModifiedUtc = fileInfo.lastModified().toUTC();
+    // Ignore bogus values like 1970-01-01T00:00:00.000 UTC
+    // that are obviously incorrect and don't provide any
+    // information.
+    if (lastModifiedUtc.isValid() &&
+            // Only defined if valid
+            lastModifiedUtc.toMSecsSinceEpoch() == 0) {
+        return QDateTime{};
+    }
+    return lastModifiedUtc;
+}
+
+} // namespace mixxx

src/sources/metadatasource.h

@@ -1,8 +1,8 @@
 #pragma once
 
 #include <QDateTime>
+#include <QFileInfo>
 #include <QImage>
-
 #include <utility>
 
 #include "track/trackmetadata.h"
@@ -20,6 +20,8 @@ class MetadataSource {
   public:
     virtual ~MetadataSource() = default;
 
+    static QDateTime getFileSynchronizedAt(const QFileInfo& fileInfo);
+
     enum class ImportResult {
         Succeeded,
         Failed,

src/sources/metadatasourcetaglib.cpp

@@ -72,29 +72,18 @@ class AiffFile : public TagLib::RIFF::AIFF::File {
     }
 };
 
-inline QDateTime getSourceSynchronizedAt(const QFileInfo& fileInfo) {
-    const QDateTime lastModifiedUtc = fileInfo.lastModified().toUTC();
-    // Ignore bogus values like 1970-01-01T00:00:00.000 UTC
-    // that are obviously incorrect and don't provide any
-    // information.
-    if (lastModifiedUtc.isValid() &&
-            // Only defined if valid
-            lastModifiedUtc.toMSecsSinceEpoch() == 0) {
-        return QDateTime{};
-    }
-    return lastModifiedUtc;
-}
-
 } // anonymous namespace
 
 std::pair<MetadataSourceTagLib::ImportResult, QDateTime>
 MetadataSourceTagLib::afterImport(ImportResult importResult) const {
-    return std::make_pair(importResult, getSourceSynchronizedAt(QFileInfo(m_fileName)));
+    return std::make_pair(importResult,
+            MetadataSource::getFileSynchronizedAt(QFileInfo(m_fileName)));
 }
 
 std::pair<MetadataSourceTagLib::ExportResult, QDateTime>
 MetadataSourceTagLib::afterExport(ExportResult exportResult) const {
-    return std::make_pair(exportResult, getSourceSynchronizedAt(QFileInfo(m_fileName)));
+    return std::make_pair(exportResult,
+            MetadataSource::getFileSynchronizedAt(QFileInfo(m_fileName)));
 }
 
 std::pair<MetadataSource::ImportResult, QDateTime>

src/sources/soundsourceproxy.cpp

@@ -516,6 +516,20 @@ SoundSourceProxy::importTrackMetadataAndCoverImage(
             pCoverImage);
 }
 
+namespace {
+
+inline bool shouldUpdateTrackMetadatafromSource(
+        SoundSourceProxy::UpdateTrackFromSourceMode mode,
+        mixxx::TrackRecord::SourceSyncStatus sourceSyncStatus) {
+    return mode == SoundSourceProxy::UpdateTrackFromSourceMode::Always ||
+            (mode == SoundSourceProxy::UpdateTrackFromSourceMode::Newer &&
+                    sourceSyncStatus == mixxx::TrackRecord::SourceSyncStatus::Outdated) ||
+            (mode == SoundSourceProxy::UpdateTrackFromSourceMode::Once &&
+                    sourceSyncStatus == mixxx::TrackRecord::SourceSyncStatus::Void);
+}
+
+} // namespace
+
 bool SoundSourceProxy::updateTrackFromSource(
         UpdateTrackFromSourceMode mode) {
     DEBUG_ASSERT(m_pTrack);
@@ -539,9 +553,9 @@ bool SoundSourceProxy::updateTrackFromSource(
     // values if the corresponding file tags are missing. Depending
     // on the file type some kind of tags might even not be supported
     // at all and this information would get lost entirely otherwise!
-    bool headerParsed = false;
+    mixxx::TrackRecord::SourceSyncStatus sourceSyncStatus;
     mixxx::TrackMetadata trackMetadata =
-            m_pTrack->getMetadata(&headerParsed);
+            m_pTrack->getMetadata(&sourceSyncStatus);
 
     // Save for later to replace the unreliable and imprecise audio
     // properties imported from file tags (see below).
@@ -554,17 +568,13 @@ bool SoundSourceProxy::updateTrackFromSource(
     DEBUG_ASSERT(coverImg.isNull());
     QImage* pCoverImg = nullptr; // pointer also serves as a flag
 
-    // If the file tags have already been parsed at least once, the
-    // existing track metadata should not be updated implicitly, i.e.
-    // if the user did not explicitly choose to (re-)import metadata
-    // explicitly from this file.
-    bool mergeExtraMetadataFromSource = false;
-    if (headerParsed && mode == UpdateTrackFromSourceMode::Once) {
-        // No (re-)import needed or desired, only merge missing properties
-        mergeExtraMetadataFromSource = true;
-    } else {
-        // Import the cover initially or when a reimport has been requested
+    const bool updateMetadataFromSource =
+            shouldUpdateTrackMetadatafromSource(mode, sourceSyncStatus);
+
+    // Decide if cover art needs to be re-imported
+    if (updateMetadataFromSource) {
         const auto coverInfo = m_pTrack->getCoverInfo();
+        // Avoid replacing user selected cover art with guessed cover art!
         if (coverInfo.source == CoverInfo::USER_SELECTED &&
                 coverInfo.type == CoverInfo::FILE) {
             // Ignore embedded cover art
@@ -592,16 +602,26 @@ bool SoundSourceProxy::updateTrackFromSource(
                 << "from file"
                 << getUrl().toString();
         // make sure that the trackMetadata was not messed up due to the failure
-        trackMetadata = m_pTrack->getMetadata(&headerParsed);
+        mixxx::TrackRecord::SourceSyncStatus sourceSyncStatusNew;
+        trackMetadata = m_pTrack->getMetadata(&sourceSyncStatusNew);
+        if (sourceSyncStatus != sourceSyncStatusNew) {
+            // Do not continue after detecting an inconsistency due to
+            // race conditions while restoring the track metadata.
+            // This is almost impossible, but it may happen. The preceding
+            // warning message already identifies the track that is affected.
+            kLogger.critical() << "Aborting update of track metadata from source "
+                                  "due to unexpected inconsistencies during recovery";
+            return false;
+        }
     }
 
     // Partial import
-    if (mergeExtraMetadataFromSource) {
-        // No reimport of embedded cover image desired in this case
+    if (!updateMetadataFromSource) {
+        // No reimport of embedded cover image desired in this case.
+        // Only import and merge extra metadata that might be missing
+        // in the database.
         DEBUG_ASSERT(!pCoverImg);
         if (metadataImportedFromSource.first == mixxx::MetadataSource::ImportResult::Succeeded) {
-            // Partial import of properties that are not (yet) stored
-            // in the database
             return m_pTrack->mergeExtraMetadataFromSource(trackMetadata);
         } else {
             // Nothing to do if no metadata has been imported
@@ -610,23 +630,20 @@ bool SoundSourceProxy::updateTrackFromSource(
     }
 
     // Full import
-    if (headerParsed) {
-        // Metadata has been synchronized successfully at least
-        // once in the past. Only overwrite this information if
-        // new data has actually been imported, otherwise abort
-        // and preserve the existing data!
+    DEBUG_ASSERT(updateMetadataFromSource);
+    if (sourceSyncStatus == mixxx::TrackRecord::SourceSyncStatus::Void) {
+        DEBUG_ASSERT(pCoverImg);
         if (kLogger.debugEnabled()) {
             kLogger.debug()
-                    << "Updating track metadata"
-                    << (pCoverImg ? "and embedded cover art" : "")
-                    << "from file"
+                    << "Initializing track metadata and embedded cover art from file"
                     << getUrl().toString();
         }
     } else {
-        DEBUG_ASSERT(pCoverImg);
         if (kLogger.debugEnabled()) {
             kLogger.debug()
-                    << "Initializing track metadata and embedded cover art from file"
+                    << "Re-importing track metadata"
+                    << (pCoverImg ? "and embedded cover art" : "")
+                    << "from file"
                     << getUrl().toString();
         }
     }