Improve synchronization of track metadata and file tags

src/library/dao/trackdao.cpp

@@ -1344,33 +1344,6 @@ TrackPointer TrackDAO::getTrackById(TrackId trackId) const {
         SoundSourceProxy(pTrack).updateTrackFromSource();
     }
 
-    // Data migration: Reload track total from file tags if not initialized
-    // yet. The added column "tracktotal" has been initialized with the
-    // default value "//".
-    // See also: Schema revision 26 in schema.xml
-    if (pTrack->getTrackTotal() == "//") {
-        // Reload track total from file tags if the special track
-        // total migration value "//" indicates that the track total
-        // is missing and needs to be reloaded.
-        qDebug() << "Reloading value for 'tracktotal' once-only from file"
-                " to replace the default value introduced with a previous"
-                " schema upgrade";
-        mixxx::TrackMetadata trackMetadata;
-        if (SoundSourceProxy(pTrack).importTrackMetadata(&trackMetadata) == mixxx::MetadataSource::ImportResult::Succeeded) {
-            // Copy the track total from the temporary track object
-            pTrack->setTrackTotal(trackMetadata.getTrackInfo().getTrackTotal());
-            // Also set the track number if it is still empty due
-            // to insufficient parsing capabilities of Mixxx in
-            // previous versions.
-            if (!trackMetadata.getTrackInfo().getTrackNumber().isEmpty() && pTrack->getTrackNumber().isEmpty()) {
-                pTrack->setTrackNumber(trackMetadata.getTrackInfo().getTrackNumber());
-            }
-        } else {
-            qWarning() << "Failed to reload value for 'tracktotal' from file tags:"
-                    << trackLocation;
-        }
-    }
-
     // Listen to dirty and changed signals
     connect(pTrack.get(),
             &Track::dirty,

src/sources/soundsourceproxy.cpp

@@ -323,44 +323,38 @@ void SoundSourceProxy::updateTrackFromSource(
     // at all and this information would get lost entirely otherwise!
     mixxx::TrackMetadata trackMetadata;
     bool metadataSynchronized = false;
-    m_pTrack->getTrackMetadata(&trackMetadata, &metadataSynchronized);
+    m_pTrack->readTrackMetadata(&trackMetadata, &metadataSynchronized);
     // 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 mergeImportedMetadata = false;
     if (metadataSynchronized &&
             (importTrackMetadataMode == ImportTrackMetadataMode::Once)) {
-        if (kLogger.debugEnabled()) {
-            kLogger.debug()
-                    << "Skip importing of track metadata and embedded cover art from file"
-                    << getUrl().toString();
-        }
-        return; // abort
+        // No (re-)import needed or desired, only merge missing properties
+        mergeImportedMetadata = true;
     }
 
     // Embedded cover art is imported together with the track's metadata.
     // But only if the user has not selected external cover art for this
     // track!
     QImage coverImg;
     DEBUG_ASSERT(coverImg.isNull());
-    QImage* pCoverImg; // pointer is also used as a flag
-    const CoverInfoRelative coverInfoOld = m_pTrack->getCoverInfo();
-    // Only re-import cover art if it is save to update, e.g. never
-    // discard a users's custom choice! We are using a whitelisting
-    // filter here that explicitly checks all valid preconditions
-    // when it is permissible to update/replace existing cover art.
-    if (((coverInfoOld.source == CoverInfo::UNKNOWN) || (coverInfoOld.source == CoverInfo::GUESSED)) &&
-            ((coverInfoOld.type == CoverInfo::NONE) || (coverInfoOld.type == CoverInfo::METADATA))) {
-        // Should import and update embedded cover art
-        pCoverImg = &coverImg;
-    } else {
-        if (kLogger.debugEnabled()) {
-            kLogger.debug()
-                    << "Skip importing of embedded cover art from file"
-                    << getUrl().toString();
+    QImage* pCoverImg = nullptr; // pointer also serves as a flag
+    if (!mergeImportedMetadata) {
+        const CoverInfoRelative coverInfoOld = m_pTrack->getCoverInfo();
+        if (coverInfoOld.source == CoverInfo::USER_SELECTED &&
+                coverInfoOld.type == CoverInfo::FILE) {
+            // Ignore embedded cover art
+            if (kLogger.debugEnabled()) {
+                kLogger.debug()
+                        << "Skip importing of embedded cover art from file"
+                        << getUrl().toString();
+            }
+        } else {
+            // (Re-)import embedded cover art
+            pCoverImg = &coverImg;
         }
-        // Skip import of embedded cover art
-        pCoverImg = nullptr;
     }
 
     // Parse the tags stored in the audio file
@@ -373,68 +367,75 @@ void SoundSourceProxy::updateTrackFromSource(
                 << (pCoverImg ? "and embedded cover art" : "")
                 << "from file"
                 << getUrl().toString();
-        // Continue for now, but the abort may follow shortly if the
-        // track already has metadata (see below)
     }
-    if (metadataSynchronized) {
-        // 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!
-        if (metadataImported.first != mixxx::MetadataSource::ImportResult::Succeeded) {
-            return; // abort
-        }
-        if (kLogger.debugEnabled()) {
-            kLogger.debug()
-                    << "Updating track metadata"
-                    << (pCoverImg ? "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"
-                    << getUrl().toString();
+
+    if (metadataImported.first != mixxx::MetadataSource::ImportResult::Succeeded) {
+        if (mergeImportedMetadata) {
+            // Nothing to do if no metadata imported
+            return;
+        } else if (trackMetadata.getTrackInfo().getTitle().trimmed().isEmpty()) {
+            // Only parse artist and title if both fields are empty to avoid
+            // inconsistencies. Otherwise the file name (without extension)
+            // is used as the title and the artist is unmodified.
+            //
+            // TODO(XXX): Disable splitting of artist/title in settings, i.e.
+            // optionally don't split even if both title and artist are empty?
+            // Some users might want to import the whole file name of untagged
+            // files as the title without splitting the artist:
+            //     https://www.mixxx.org/forums/viewtopic.php?f=3&t=12838
+            // NOTE(uklotzde, 2019-09-26): Whoever needs this should simply set
+            // splitArtistTitle = false here and compile their custom version!
+            // It is not worth extending the settings and injecting them into
+            // SoundSourceProxy for just a few people.
+            const bool splitArtistTitle =
+                    trackMetadata.getTrackInfo().getArtist().trimmed().isEmpty();
+            const auto trackFile = m_pTrack->getFileInfo();
+            kLogger.info()
+                    << "Parsing missing"
+                    << (splitArtistTitle ? "artist/title" : "title")
+                    << "from file name:"
+                    << trackFile;
+            if (trackMetadata.refTrackInfo().parseArtistTitleFromFileName(trackFile.fileName(), splitArtistTitle) &&
+                    metadataImported.second.isNull()) {
+                // Since this is also some kind of metadata import, we mark the
+                // track's metadata as synchronized with the time stamp of the file.
+                metadataImported.second = trackFile.fileLastModified();
+            }
         }
     }
 
-    // Fallback: If the title field is empty then try to populate title
-    // (and optionally artist) from the file name. This might happen if
-    // tags are unavailable, unreadable, or partially/completely missing.
-    if (trackMetadata.getTrackInfo().getTitle().trimmed().isEmpty()) {
-        // Only parse artist and title if both fields are empty to avoid
-        // inconsistencies. Otherwise the file name (without extension)
-        // is used as the title and the artist is unmodified.
-        //
-        // TODO(XXX): Disable splitting of artist/title in settings, i.e.
-        // optionally don't split even if both title and artist are empty?
-        // Some users might want to import the whole file name of untagged
-        // files as the title without splitting the artist:
-        //     https://www.mixxx.org/forums/viewtopic.php?f=3&t=12838
-        // NOTE(uklotzde, 2019-09-26): Whoever needs this should simply set
-        // splitArtistTitle = false here and compile their custom version!
-        // It is not worth extending the settings and injecting them into
-        // SoundSourceProxy for just a few people.
-        const bool splitArtistTitle =
-                trackMetadata.getTrackInfo().getArtist().trimmed().isEmpty();
-        const auto trackFile = m_pTrack->getFileInfo();
-        kLogger.info()
-                << "Parsing missing"
-                << (splitArtistTitle ? "artist/title" : "title")
-                << "from file name:"
-                << trackFile;
-        if (trackMetadata.refTrackInfo().parseArtistTitleFromFileName(trackFile.fileName(), splitArtistTitle) &&
-                metadataImported.second.isNull()) {
-            // Since this is also some kind of metadata import, we mark the
-            // track's metadata as synchronized with the time stamp of the file.
-            metadataImported.second = trackFile.fileLastModified();
+    if (mergeImportedMetadata) {
+        // Partial import of properties that are not (yet) stored
+        // in the database
+        m_pTrack->mergeImportedMetadata(trackMetadata);
+    } else {
+        // Full import
+        if (metadataSynchronized) {
+            // 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!
+            if (metadataImported.first != mixxx::MetadataSource::ImportResult::Succeeded) {
+                return; // abort
+            }
+            if (kLogger.debugEnabled()) {
+                kLogger.debug()
+                        << "Updating track metadata"
+                        << (pCoverImg ? "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"
+                        << getUrl().toString();
+            }
         }
+        m_pTrack->importMetadata(trackMetadata, metadataImported.second);
     }
 
-    m_pTrack->setTrackMetadata(trackMetadata, metadataImported.second);
-
     if (pCoverImg) {
         // If the pointer is not null then the cover art should be guessed
         // from the embedded metadata

src/test/trackupdate_test.cpp

@@ -57,14 +57,14 @@ TEST_F(TrackUpdateTest, parseModifiedCleanOnce) {
     pTrack->markClean();
 
     mixxx::TrackMetadata trackMetadataBefore;
-    pTrack->getTrackMetadata(&trackMetadataBefore);
+    pTrack->readTrackMetadata(&trackMetadataBefore);
     auto coverInfoBefore = pTrack->getCoverInfo();
 
     SoundSourceProxy(pTrack).updateTrackFromSource(
             SoundSourceProxy::ImportTrackMetadataMode::Once);
 
     mixxx::TrackMetadata trackMetadataAfter;
-    pTrack->getTrackMetadata(&trackMetadataAfter);
+    pTrack->readTrackMetadata(&trackMetadataAfter);
     auto coverInfoAfter = pTrack->getCoverInfo();
 
     // Not updated
@@ -79,14 +79,14 @@ TEST_F(TrackUpdateTest, parseModifiedCleanAgainSkipCover) {
     pTrack->markClean();
 
     mixxx::TrackMetadata trackMetadataBefore;
-    pTrack->getTrackMetadata(&trackMetadataBefore);
+    pTrack->readTrackMetadata(&trackMetadataBefore);
     auto coverInfoBefore = pTrack->getCoverInfo();
 
     SoundSourceProxy(pTrack).updateTrackFromSource(
             SoundSourceProxy::ImportTrackMetadataMode::Again);
 
     mixxx::TrackMetadata trackMetadataAfter;
-    pTrack->getTrackMetadata(&trackMetadataAfter);
+    pTrack->readTrackMetadata(&trackMetadataAfter);
     auto coverInfoAfter = pTrack->getCoverInfo();
 
     // Updated
@@ -105,14 +105,14 @@ TEST_F(TrackUpdateTest, parseModifiedCleanAgainUpdateCover) {
     pTrack->markClean();
 
     mixxx::TrackMetadata trackMetadataBefore;
-    pTrack->getTrackMetadata(&trackMetadataBefore);
+    pTrack->readTrackMetadata(&trackMetadataBefore);
     auto coverInfoBefore = pTrack->getCoverInfo();
 
     SoundSourceProxy(pTrack).updateTrackFromSource(
             SoundSourceProxy::ImportTrackMetadataMode::Again);
 
     mixxx::TrackMetadata trackMetadataAfter;
-    pTrack->getTrackMetadata(&trackMetadataAfter);
+    pTrack->readTrackMetadata(&trackMetadataAfter);
     auto coverInfoAfter = pTrack->getCoverInfo();
 
     // Updated
@@ -126,14 +126,14 @@ TEST_F(TrackUpdateTest, parseModifiedDirtyAgain) {
     auto pTrack = newTestTrackParsedModified();
 
     mixxx::TrackMetadata trackMetadataBefore;
-    pTrack->getTrackMetadata(&trackMetadataBefore);
+    pTrack->readTrackMetadata(&trackMetadataBefore);
     auto coverInfoBefore = pTrack->getCoverInfo();
 
     SoundSourceProxy(pTrack).updateTrackFromSource(
             SoundSourceProxy::ImportTrackMetadataMode::Again);
 
     mixxx::TrackMetadata trackMetadataAfter;
-    pTrack->getTrackMetadata(&trackMetadataAfter);
+    pTrack->readTrackMetadata(&trackMetadataAfter);
     auto coverInfoAfter = pTrack->getCoverInfo();
 
     // Updated

src/track/albuminfo.cpp

@@ -3,18 +3,6 @@
 
 namespace mixxx {
 
-void AlbumInfo::resetUnsupportedValues() {
-#if defined(__EXTRA_METADATA__)
-    setCopyright(QString());
-    setLicense(QString());
-    setMusicBrainzArtistId(QString());
-    setMusicBrainzReleaseId(QString());
-    setMusicBrainzReleaseGroupId(QString());
-    setRecordLabel(QString());
-    setReplayGain(ReplayGain());
-#endif // __EXTRA_METADATA__
-}
-
 bool operator==(const AlbumInfo& lhs, const AlbumInfo& rhs) {
     return (lhs.getArtist() == rhs.getArtist()) &&
 #if defined(__EXTRA_METADATA__)

src/track/albuminfo.h

@@ -33,9 +33,6 @@ class AlbumInfo final {
     AlbumInfo& operator=(AlbumInfo&&) = default;
     AlbumInfo& operator=(const AlbumInfo&) = default;
 
-    // TODO(XXX): Remove after all new fields have been added to the library
-    void resetUnsupportedValues();
-
     // Adjusts floating-point properties to match their string representation
     // in file tags to account for rounding errors.
     void normalizeBeforeExport() {

src/track/bpm.h

@@ -25,6 +25,12 @@ class Bpm final {
 
     // Adjusts floating-point values to match their string representation
     // in file tags to account for rounding errors.
+    // NOTE(2019-02-19, uklotzde): The pre-normalization cannot prevent
+    // repeated export of metadata for files with ID3 tags that are only
+    // able to store the BPM value with integer precision! In case of a
+    // fractional value the ID3 metadata is always detected as modified
+    // and will be exported regardless if it has actually been modified
+    // or not.
     void normalizeBeforeExport() {
         m_value = normalizeValue(m_value);
     }