Merge pull request #3866 from uklotzde/trackmetadata

Track metadata: Renamings, reorderings, minor bugfix

src/library/dlgtagfetcher.cpp

@@ -220,7 +220,7 @@ void DlgTagFetcher::apply() {
                 trackRelease.releaseGroupId);
     }
 #endif // __EXTRA_METADATA__
-    m_track->importMetadata(
+    m_track->replaceMetadataFromSource(
             std::move(trackMetadata),
             // Prevent re-import of outdated metadata from file tags
             // by explicitly setting the synchronization time stamp

src/library/dlgtrackinfo.cpp

@@ -627,10 +627,14 @@ void DlgTrackInfo::slotImportMetadataFromFile() {
     // We cannot reuse m_pLoadedTrack, because it might already been
     // modified and we don't want to lose those changes.
     // TODO: Populate fields from TrackRecord instead of Track
-    TrackPointer pTrack = Track::newTemporary(std::move(fileAccess));
+    const TrackPointer pTrack =
+            Track::newTemporary(std::move(fileAccess));
     DEBUG_ASSERT(pTrack);
-    pTrack->importMetadata(std::move(trackMetadata));
-    pTrack->setCoverInfo(std::move(guessedCoverInfo));
+    pTrack->replaceMetadataFromSource(
+            std::move(trackMetadata),
+            metadataSynchronized);
+    pTrack->setCoverInfo(
+            std::move(guessedCoverInfo));
     populateFields(*pTrack);
 }
 

src/sources/soundsourceproxy.cpp

@@ -331,7 +331,9 @@ SoundSourceProxy::exportTrackMetadataBeforeSaving(
         pMetadataSource = proxy.m_pSoundSource;
     }
     if (pMetadataSource) {
-        return pTrack->exportMetadata(pMetadataSource, pConfig);
+        return pTrack->exportMetadata(
+                *pMetadataSource,
+                pConfig);
     } else {
         kLogger.warning()
                 << "Unable to export track metadata into file"
@@ -544,10 +546,10 @@ bool SoundSourceProxy::updateTrackFromSource(
     // 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;
+    bool mergeExtraMetadataFromSource = false;
     if (metadataSynchronized && mode == UpdateTrackFromSourceMode::Once) {
         // No (re-)import needed or desired, only merge missing properties
-        mergeImportedMetadata = true;
+        mergeExtraMetadataFromSource = true;
     }
 
     // Save for later to replace the unreliable and imprecise audio
@@ -560,7 +562,7 @@ bool SoundSourceProxy::updateTrackFromSource(
     QImage coverImg;
     DEBUG_ASSERT(coverImg.isNull());
     QImage* pCoverImg = nullptr; // pointer also serves as a flag
-    if (!mergeImportedMetadata) {
+    if (!mergeExtraMetadataFromSource) {
         const auto coverInfo = m_pTrack->getCoverInfo();
         if (coverInfo.source == CoverInfo::USER_SELECTED &&
                 coverInfo.type == CoverInfo::FILE) {
@@ -591,13 +593,13 @@ bool SoundSourceProxy::updateTrackFromSource(
     }
 
     // Partial import
-    if (mergeImportedMetadata) {
+    if (mergeExtraMetadataFromSource) {
         // No reimport of embedded cover image desired in this case
         DEBUG_ASSERT(!pCoverImg);
         if (metadataImported.first == mixxx::MetadataSource::ImportResult::Succeeded) {
             // Partial import of properties that are not (yet) stored
             // in the database
-            return m_pTrack->mergeImportedMetadata(trackMetadata);
+            return m_pTrack->mergeExtraMetadataFromSource(trackMetadata);
         } else {
             // Nothing to do if no metadata has been imported
             return false;
@@ -681,7 +683,9 @@ bool SoundSourceProxy::updateTrackFromSource(
         return false;
     }
 
-    m_pTrack->importMetadata(trackMetadata, metadataImported.second);
+    m_pTrack->replaceMetadataFromSource(
+            std::move(trackMetadata),
+            metadataImported.second);
 
     bool pendingBeatsImport = m_pTrack->getBeatsImportStatus() == Track::ImportStatus::Pending;
     bool pendingCueImport = m_pTrack->getCueImportStatus() == Track::ImportStatus::Pending;

src/test/trackmetadata_test.cpp

@@ -62,7 +62,7 @@ TEST_F(TrackMetadataTest, parseArtistTitleFromFileName) {
     }
 }
 
-TEST_F(TrackMetadataTest, mergeImportedMetadata) {
+TEST_F(TrackMetadataTest, mergeExtraMetadataFromSource) {
     // Existing track metadata (stored in the database) without extra properties
     mixxx::TrackRecord oldTrackRecord;
     mixxx::TrackMetadata* pOldTrackMetadata = oldTrackRecord.ptrMetadata();
@@ -156,7 +156,7 @@ TEST_F(TrackMetadataTest, mergeImportedMetadata) {
     mixxx::TrackRecord mergedTrackRecord = oldTrackRecord;
     ASSERT_EQ(mergedTrackRecord.getMetadata(), oldTrackRecord.getMetadata());
     ASSERT_NE(newTrackMetadata, *pOldTrackMetadata);
-    mergedTrackRecord.mergeImportedMetadata(newTrackMetadata);
+    mergedTrackRecord.mergeExtraMetadataFromSource(newTrackMetadata);
 
     mixxx::TrackMetadata* pMergedTrackMetadata = mergedTrackRecord.ptrMetadata();
     EXPECT_EQ(pOldTrackMetadata->getStreamInfo(), pMergedTrackMetadata->getStreamInfo());
@@ -225,7 +225,7 @@ TEST_F(TrackMetadataTest, mergeImportedMetadata) {
     pMergedTrackInfo->setTitle(QString());
     pMergedAlbumInfo->setArtist("");
     pMergedAlbumInfo->setTitle(QString());
-    mergedTrackRecord.mergeImportedMetadata(newTrackMetadata);
+    mergedTrackRecord.mergeExtraMetadataFromSource(newTrackMetadata);
     EXPECT_EQ(QString(""), pMergedTrackInfo->getArtist());
     EXPECT_EQ(QString(), pMergedTrackInfo->getTitle());
     EXPECT_EQ(QString(""), pMergedAlbumInfo->getArtist());
@@ -234,11 +234,11 @@ TEST_F(TrackMetadataTest, mergeImportedMetadata) {
     // Check that the placeholder for track total is replaced with the actual property
     ASSERT_NE(mixxx::TrackRecord::kTrackTotalPlaceholder, pNewTrackInfo->getTrackTotal());
     pMergedTrackInfo->setTrackTotal(mixxx::TrackRecord::kTrackTotalPlaceholder);
-    mergedTrackRecord.mergeImportedMetadata(newTrackMetadata);
+    mergedTrackRecord.mergeExtraMetadataFromSource(newTrackMetadata);
     EXPECT_EQ(pNewTrackInfo->getTrackTotal(), pMergedTrackInfo->getTrackTotal());
     // ...but if track total is missing entirely it should be preserved
     ASSERT_NE(QString(), pNewTrackInfo->getTrackTotal());
     pMergedTrackInfo->setTrackTotal(QString());
-    mergedTrackRecord.mergeImportedMetadata(newTrackMetadata);
+    mergedTrackRecord.mergeExtraMetadataFromSource(newTrackMetadata);
     EXPECT_EQ(QString(), pMergedTrackInfo->getTrackTotal());
 }

src/track/track.cpp

@@ -5,6 +5,7 @@
 
 #include "engine/engine.h"
 #include "moc_track.cpp"
+#include "sources/metadatasource.h"
 #include "track/beatfactory.h"
 #include "track/beatmap.h"
 #include "track/trackref.h"
@@ -116,7 +117,7 @@ void Track::relocate(
     // the updated location from the database.
 }
 
-void Track::importMetadata(
+void Track::replaceMetadataFromSource(
         mixxx::TrackMetadata importedMetadata,
         const QDateTime& metadataSynchronized) {
     // Information stored in Serato tags is imported separately after
@@ -227,10 +228,10 @@ void Track::importMetadata(
     }
 }
 
-bool Track::mergeImportedMetadata(
+bool Track::mergeExtraMetadataFromSource(
         const mixxx::TrackMetadata& importedMetadata) {
     QMutexLocker lock(&m_qMutex);
-    if (!m_record.mergeImportedMetadata(importedMetadata)) {
+    if (!m_record.mergeExtraMetadataFromSource(importedMetadata)) {
         // Not modified
         return false;
     }
@@ -1337,14 +1338,8 @@ CoverInfo Track::getCoverInfoWithLocation() const {
 }
 
 ExportTrackMetadataResult Track::exportMetadata(
-        mixxx::MetadataSourcePointer pMetadataSource,
-        UserSettingsPointer pConfig) {
-    VERIFY_OR_DEBUG_ASSERT(pMetadataSource) {
-        kLogger.warning()
-                << "Cannot export track metadata:"
-                << getLocation();
-        return ExportTrackMetadataResult::Failed;
-    }
+        const mixxx::MetadataSource& metadataSource,
+        const UserSettingsPointer& pConfig) {
     // Locking shouldn't be necessary here, because this function will
     // be called after all references to the object have been dropped.
     // But it doesn't hurt much, so let's play it safe ;)
@@ -1421,13 +1416,13 @@ ExportTrackMetadataResult Track::exportMetadata(
     // Otherwise floating-point values like the bpm value might become
     // inconsistent with the actual value stored by the beat grid!
     mixxx::TrackMetadata normalizedFromRecord;
-    if ((pMetadataSource->importTrackMetadataAndCoverImage(&importedFromFile, nullptr).first ==
-            mixxx::MetadataSource::ImportResult::Succeeded)) {
+    if ((metadataSource.importTrackMetadataAndCoverImage(&importedFromFile, nullptr).first ==
+                mixxx::MetadataSource::ImportResult::Succeeded)) {
         // Prevent overwriting any file tags that are not yet stored in the
         // library database! This will in turn update the current metadata
         // that is stored in the database. New columns that need to be populated
         // from file tags cannot be filled during a database migration.
-        m_record.mergeImportedMetadata(importedFromFile);
+        m_record.mergeExtraMetadataFromSource(importedFromFile);
 
         // Prepare export by cloning and normalizing the metadata
         normalizedFromRecord = m_record.getMetadata();
@@ -1488,7 +1483,7 @@ ExportTrackMetadataResult Track::exportMetadata(
             << "New metadata (modified)"
             << normalizedFromRecord;
     const auto trackMetadataExported =
-            pMetadataSource->exportTrackMetadata(normalizedFromRecord);
+            metadataSource.exportTrackMetadata(normalizedFromRecord);
     switch (trackMetadataExported.first) {
     case mixxx::MetadataSource::ExportResult::Succeeded:
         // After successfully exporting the metadata we record the fact

src/track/track.h

@@ -326,17 +326,13 @@ class Track : public QObject {
     bool refreshCoverImageDigest(
             const QImage& loadedImage = QImage());
 
-    // Set/get track metadata all at once.
-    void importMetadata(
-            mixxx::TrackMetadata importedMetadata,
-            const QDateTime& metadataSynchronized = QDateTime());
-
-    /// Merge additional metadata that is not (yet) stored in the database
-    /// and only available from file tags.
+    /// Set track metadata after importing from the source.
     ///
-    /// Returns true if the track has been modified and false otherwise.
-    bool mergeImportedMetadata(
-            const mixxx::TrackMetadata& importedMetadata);
+    /// The timestamp tracks when metadata has last been synchronized
+    /// with file tags, either by importing or exporting the metadata.
+    void replaceMetadataFromSource(
+            mixxx::TrackMetadata importedMetadata,
+            const QDateTime& metadataSynchronized);
 
     mixxx::TrackMetadata getMetadata(
             bool* pMetadataSynchronized = nullptr) const;
@@ -445,10 +441,16 @@ class Track : public QObject {
     void importPendingCueInfosMarkDirtyAndUnlock(
             QMutexLocker* pLock);
 
+    /// Merge additional metadata that is not (yet) stored in the database
+    /// and only available from file tags.
+    ///
+    /// Returns true if the track has been modified and false otherwise.
+    bool mergeExtraMetadataFromSource(
+            const mixxx::TrackMetadata& importedMetadata);
 
     ExportTrackMetadataResult exportMetadata(
-            mixxx::MetadataSourcePointer pMetadataSource,
-            UserSettingsPointer pConfig);
+            const mixxx::MetadataSource& metadataSource,
+            const UserSettingsPointer& pConfig);
 
     // Information about the actual properties of the
     // audio stream is only available after opening the

src/track/trackrecord.cpp

@@ -108,11 +108,11 @@ bool copyIfNotEmpty(
 
 } // anonymous namespace
 
-bool TrackRecord::mergeImportedMetadata(
-        const TrackMetadata& importedFromFile) {
+bool TrackRecord::mergeExtraMetadataFromSource(
+        const TrackMetadata& importedMetadata) {
     bool modified = false;
     TrackInfo* pMergedTrackInfo = m_metadata.ptrTrackInfo();
-    const TrackInfo& importedTrackInfo = importedFromFile.getTrackInfo();
+    const TrackInfo& importedTrackInfo = importedMetadata.getTrackInfo();
     if (pMergedTrackInfo->getTrackTotal() == kTrackTotalPlaceholder) {
         pMergedTrackInfo->setTrackTotal(importedTrackInfo.getTrackTotal());
         // Also set the track number if it is still empty due
@@ -180,7 +180,7 @@ bool TrackRecord::mergeImportedMetadata(
             pMergedTrackInfo->ptrWork(),
             importedTrackInfo.getWork());
     AlbumInfo* pMergedAlbumInfo = refMetadata().ptrAlbumInfo();
-    const AlbumInfo& importedAlbumInfo = importedFromFile.getAlbumInfo();
+    const AlbumInfo& importedAlbumInfo = importedMetadata.getAlbumInfo();
     modified |= mergeReplayGainMetadataProperty(
             pMergedAlbumInfo->ptrReplayGain(),
             importedAlbumInfo.getReplayGain());

src/track/trackrecord.h

@@ -111,7 +111,7 @@ class TrackRecord final {
     // data when needed.
     //
     // Returns true if any property has been modified or false otherwise.
-    bool mergeImportedMetadata(
+    bool mergeExtraMetadataFromSource(
             const TrackMetadata& importedMetadata);
 
     /// Update the stream info after opening the audio stream during