From ed20b2385da467e1baa696369463b489b443d98a Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 14 May 2021 20:28:33 +0200 Subject: [PATCH 1/5] Track::exportMetadata(): Pass parameter by const-ref --- src/sources/soundsourceproxy.cpp | 4 +++- src/track/track.cpp | 17 ++++++----------- src/track/track.h | 4 ++-- 3 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/sources/soundsourceproxy.cpp b/src/sources/soundsourceproxy.cpp index e34760716d2..598e6172977 100644 --- a/src/sources/soundsourceproxy.cpp +++ b/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" diff --git a/src/track/track.cpp b/src/track/track.cpp index 421c8be731a..b0e3e0951bf 100644 --- a/src/track/track.cpp +++ b/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" @@ -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,8 +1416,8 @@ 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 @@ -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 diff --git a/src/track/track.h b/src/track/track.h index 8bf499a5f22..6708a930bb0 100644 --- a/src/track/track.h +++ b/src/track/track.h @@ -447,8 +447,8 @@ class Track : public QObject { 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 From 1a84a44dde020dc1020cd523570d7075db5dbc12 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sat, 15 May 2021 15:24:38 +0200 Subject: [PATCH 2/5] Track: Reduce visibility of member function --- src/track/track.h | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/track/track.h b/src/track/track.h index 6708a930bb0..742e5fb18f2 100644 --- a/src/track/track.h +++ b/src/track/track.h @@ -331,13 +331,6 @@ class Track : public QObject { mixxx::TrackMetadata importedMetadata, const QDateTime& metadataSynchronized = QDateTime()); - /// 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 mergeImportedMetadata( - const mixxx::TrackMetadata& importedMetadata); - mixxx::TrackMetadata getMetadata( bool* pMetadataSynchronized = nullptr) const; @@ -445,6 +438,12 @@ 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 mergeImportedMetadata( + const mixxx::TrackMetadata& importedMetadata); ExportTrackMetadataResult exportMetadata( const mixxx::MetadataSource& metadataSource, From 36d0cf6f47eda4966215c8acca0e86776c7ad5f3 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sat, 15 May 2021 15:54:28 +0200 Subject: [PATCH 3/5] Track: Rename member functions for imported metadata --- src/library/dlgtagfetcher.cpp | 2 +- src/library/dlgtrackinfo.cpp | 9 ++++++--- src/sources/soundsourceproxy.cpp | 10 +++++----- src/test/trackmetadata_test.cpp | 10 +++++----- src/track/track.cpp | 8 ++++---- src/track/track.h | 9 ++++++--- src/track/trackrecord.cpp | 8 ++++---- src/track/trackrecord.h | 2 +- 8 files changed, 32 insertions(+), 26 deletions(-) diff --git a/src/library/dlgtagfetcher.cpp b/src/library/dlgtagfetcher.cpp index 34b220906a9..2e3eeac7ac2 100644 --- a/src/library/dlgtagfetcher.cpp +++ b/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 diff --git a/src/library/dlgtrackinfo.cpp b/src/library/dlgtrackinfo.cpp index 791db79dccd..6821848b743 100644 --- a/src/library/dlgtrackinfo.cpp +++ b/src/library/dlgtrackinfo.cpp @@ -627,10 +627,13 @@ 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)); + pTrack->setCoverInfo( + std::move(guessedCoverInfo)); populateFields(*pTrack); } diff --git a/src/sources/soundsourceproxy.cpp b/src/sources/soundsourceproxy.cpp index 598e6172977..bf44cea95df 100644 --- a/src/sources/soundsourceproxy.cpp +++ b/src/sources/soundsourceproxy.cpp @@ -546,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 @@ -562,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) { @@ -593,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; diff --git a/src/test/trackmetadata_test.cpp b/src/test/trackmetadata_test.cpp index af16af07fe6..5f7d17a1591 100644 --- a/src/test/trackmetadata_test.cpp +++ b/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()); } diff --git a/src/track/track.cpp b/src/track/track.cpp index b0e3e0951bf..70449aaa374 100644 --- a/src/track/track.cpp +++ b/src/track/track.cpp @@ -117,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 @@ -228,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; } @@ -1422,7 +1422,7 @@ ExportTrackMetadataResult Track::exportMetadata( // 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(); diff --git a/src/track/track.h b/src/track/track.h index 742e5fb18f2..6c2ab90633a 100644 --- a/src/track/track.h +++ b/src/track/track.h @@ -326,8 +326,11 @@ class Track : public QObject { bool refreshCoverImageDigest( const QImage& loadedImage = QImage()); - // Set/get track metadata all at once. - void importMetadata( + /// Set track metadata after importing from the source. + /// + /// 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 = QDateTime()); @@ -442,7 +445,7 @@ class Track : public QObject { /// and only available from file tags. /// /// Returns true if the track has been modified and false otherwise. - bool mergeImportedMetadata( + bool mergeExtraMetadataFromSource( const mixxx::TrackMetadata& importedMetadata); ExportTrackMetadataResult exportMetadata( diff --git a/src/track/trackrecord.cpp b/src/track/trackrecord.cpp index 37f2b4fed11..782963ec841 100644 --- a/src/track/trackrecord.cpp +++ b/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()); diff --git a/src/track/trackrecord.h b/src/track/trackrecord.h index f0c347fe3f7..9a4c3dca9dd 100644 --- a/src/track/trackrecord.h +++ b/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 From 2cb6f7b770a5cbbaa73b5b5516993dc2e4636b9b Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sat, 15 May 2021 16:15:22 +0200 Subject: [PATCH 4/5] Use move instead of copy --- src/sources/soundsourceproxy.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/sources/soundsourceproxy.cpp b/src/sources/soundsourceproxy.cpp index bf44cea95df..f94c4f1f44b 100644 --- a/src/sources/soundsourceproxy.cpp +++ b/src/sources/soundsourceproxy.cpp @@ -683,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; From 1a6a5264baeffeba04b9b2dcb16bd6052c4996f6 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sat, 15 May 2021 16:17:00 +0200 Subject: [PATCH 5/5] Track: Fix missing synchronization time stamp on metadata import --- src/library/dlgtrackinfo.cpp | 3 ++- src/track/track.h | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/library/dlgtrackinfo.cpp b/src/library/dlgtrackinfo.cpp index 6821848b743..04f8ba6a68b 100644 --- a/src/library/dlgtrackinfo.cpp +++ b/src/library/dlgtrackinfo.cpp @@ -631,7 +631,8 @@ void DlgTrackInfo::slotImportMetadataFromFile() { Track::newTemporary(std::move(fileAccess)); DEBUG_ASSERT(pTrack); pTrack->replaceMetadataFromSource( - std::move(trackMetadata)); + std::move(trackMetadata), + metadataSynchronized); pTrack->setCoverInfo( std::move(guessedCoverInfo)); populateFields(*pTrack); diff --git a/src/track/track.h b/src/track/track.h index 6c2ab90633a..b94ea6fb858 100644 --- a/src/track/track.h +++ b/src/track/track.h @@ -332,7 +332,7 @@ class Track : public QObject { /// with file tags, either by importing or exporting the metadata. void replaceMetadataFromSource( mixxx::TrackMetadata importedMetadata, - const QDateTime& metadataSynchronized = QDateTime()); + const QDateTime& metadataSynchronized); mixxx::TrackMetadata getMetadata( bool* pMetadataSynchronized = nullptr) const;