From 400f751bb43c3e8b35fefe9b1a3f8eb3ae0be236 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 29 Aug 2021 10:38:45 +0200 Subject: [PATCH 1/7] DB: Update track metadata from file tags if outdated When loading a track from the database check if the underlying file has been modified. If the time stamp is newer than the timestamp of the last synchronization then update track metadata from file tags. But only if export of track metadata into file tags has been enabled by the user to reduce surprising side-effects to a minimum. Introducing a new configuration option for this feature has deliberately been avoided to reduce the number of possible configurations. Either you want to keep your file tags synchronized or not, no exceptions! # Conflicts: # src/sources/metadatasourcetaglib.cpp --- CMakeLists.txt | 1 + src/library/dao/trackdao.cpp | 21 ++++++-- src/sources/metadatasource.cpp | 27 +++++++++++ src/sources/metadatasource.h | 4 +- src/sources/metadatasourcetaglib.cpp | 19 ++------ src/sources/soundsourceproxy.cpp | 71 +++++++++++++++++----------- src/sources/soundsourceproxy.h | 25 ++++++---- src/test/autodjprocessor_test.cpp | 3 +- src/test/coverartutils_test.cpp | 3 +- src/test/soundproxy_test.cpp | 16 ++++--- src/test/trackupdate_test.cpp | 21 ++++---- src/track/track.cpp | 19 +++++--- src/track/track.h | 8 ++-- src/track/trackrecord.cpp | 50 +++++++++++++++----- src/track/trackrecord.h | 17 ++++++- src/widget/wtrackmenu.cpp | 2 +- 16 files changed, 211 insertions(+), 96 deletions(-) create mode 100644 src/sources/metadatasource.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 56410991431..fa91ecc6e78 100644 --- a/CMakeLists.txt +++ b/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 diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 026542b40af..175471ad6fe 100644 --- a/src/library/dao/trackdao.cpp +++ b/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::kSyncTrackMetadataExportConfigKey) + .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:" diff --git a/src/sources/metadatasource.cpp b/src/sources/metadatasource.cpp new file mode 100644 index 00000000000..30c571ad5bb --- /dev/null +++ b/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 diff --git a/src/sources/metadatasource.h b/src/sources/metadatasource.h index 2ee4c65e010..852e5c05011 100644 --- a/src/sources/metadatasource.h +++ b/src/sources/metadatasource.h @@ -1,8 +1,8 @@ #pragma once #include +#include #include - #include #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, diff --git a/src/sources/metadatasourcetaglib.cpp b/src/sources/metadatasourcetaglib.cpp index 37dfad843ca..d9be95f71d3 100644 --- a/src/sources/metadatasourcetaglib.cpp +++ b/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::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::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 diff --git a/src/sources/soundsourceproxy.cpp b/src/sources/soundsourceproxy.cpp index cc773234016..a88514e910e 100644 --- a/src/sources/soundsourceproxy.cpp +++ b/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(); } } diff --git a/src/sources/soundsourceproxy.h b/src/sources/soundsourceproxy.h index 0a26cac367a..ecf0b8df768 100644 --- a/src/sources/soundsourceproxy.h +++ b/src/sources/soundsourceproxy.h @@ -105,18 +105,25 @@ class SoundSourceProxy { /// Controls which (metadata/coverart) and how tags are (re-)imported from /// audio files when creating a SoundSourceProxy. + /// + /// Cover art is only re-imported and updated if it has been guessed from + /// metadata to prevent overwriting a custom choice. enum class UpdateTrackFromSourceMode { // Import both track metadata and cover image once for new track objects. // Otherwise the request is ignored and the track object is not modified. Once, - // (Re-)Import the track's metadata and cover art. Cover art is - // only updated if it has been guessed from metadata to prevent - // overwriting a custom choice. - Again, - // If omitted both metadata and cover image will be imported at most - // once for each track object to avoid overwriting modified data in - // the library. - Default = Once, + /// (Re-)Import the track's metadata and cover art if the file's modification + /// time stamp is newer than the last synchronization time stamp. + /// + /// Source synchronization time stamps have been introduced by v2.4.0. + /// For existing files in the library this time stamp is undefined until + /// metadata is manually re-imported! In this case we cannot determine + /// if the file tags contain updated data and need to skip the implicit + /// re-import to prevent overwriting precious user data. + Newer, + // Unconditionally (re-)import the track's metadata and cover art, independent + // of when the file has last been modified and the synchronization time stamp. + Always, }; /// Updates file type, metadata, and cover image of the track object @@ -142,7 +149,7 @@ class SoundSourceProxy { /// /// Returns true if the track has been modified and false otherwise. bool updateTrackFromSource( - UpdateTrackFromSourceMode mode = UpdateTrackFromSourceMode::Default); + UpdateTrackFromSourceMode mode); /// Opening the audio source through the proxy will update the /// audio properties of the corresponding track object. Returns diff --git a/src/test/autodjprocessor_test.cpp b/src/test/autodjprocessor_test.cpp index 145fa416c2f..57638ea6376 100644 --- a/src/test/autodjprocessor_test.cpp +++ b/src/test/autodjprocessor_test.cpp @@ -170,7 +170,8 @@ class AutoDJProcessorTest : public LibraryTest { static TrackPointer newTestTrack(TrackId trackId) { TrackPointer pTrack( Track::newDummy(kTrackLocationTest, trackId)); - EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource()); + EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource( + SoundSourceProxy::UpdateTrackFromSourceMode::Once)); return pTrack; } diff --git a/src/test/coverartutils_test.cpp b/src/test/coverartutils_test.cpp index 8bf8635dfea..f9b3206ed02 100644 --- a/src/test/coverartutils_test.cpp +++ b/src/test/coverartutils_test.cpp @@ -114,7 +114,8 @@ TEST_F(CoverArtUtilTest, searchImage) { // Looking for a track with embedded cover. pTrack = Track::newTemporary(kTrackLocationTest); - EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource()); + EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource( + SoundSourceProxy::UpdateTrackFromSourceMode::Once)); CoverInfo result = pTrack->getCoverInfoWithLocation(); EXPECT_EQ(result.type, CoverInfo::METADATA); EXPECT_EQ(result.source, CoverInfo::GUESSED); diff --git a/src/test/soundproxy_test.cpp b/src/test/soundproxy_test.cpp index 067a883c43a..be2bb34b472 100644 --- a/src/test/soundproxy_test.cpp +++ b/src/test/soundproxy_test.cpp @@ -213,7 +213,8 @@ TEST_F(SoundSourceProxyTest, openEmptyFile) { TEST_F(SoundSourceProxyTest, readArtist) { auto pTrack = Track::newTemporary(kTestDir, "artist.mp3"); SoundSourceProxy proxy(pTrack); - EXPECT_TRUE(proxy.updateTrackFromSource()); + EXPECT_TRUE(proxy.updateTrackFromSource( + SoundSourceProxy::UpdateTrackFromSourceMode::Once)); EXPECT_EQ("Test Artist", pTrack->getArtist()); } @@ -224,33 +225,36 @@ TEST_F(SoundSourceProxyTest, readNoTitle) { auto pTrack1 = Track::newTemporary( kTestDir, "empty.mp3"); SoundSourceProxy proxy1(pTrack1); - EXPECT_TRUE(proxy1.updateTrackFromSource()); + EXPECT_TRUE(proxy1.updateTrackFromSource( + SoundSourceProxy::UpdateTrackFromSourceMode::Once)); EXPECT_EQ("empty", pTrack1->getTitle()); // Test a reload also works pTrack1->setTitle(""); EXPECT_TRUE(proxy1.updateTrackFromSource( - SoundSourceProxy::UpdateTrackFromSourceMode::Again)); + SoundSourceProxy::UpdateTrackFromSourceMode::Always)); EXPECT_EQ("empty", pTrack1->getTitle()); // Test a file with other metadata but no title auto pTrack2 = Track::newTemporary( kTestDir, "cover-test-png.mp3"); SoundSourceProxy proxy2(pTrack2); - EXPECT_TRUE(proxy2.updateTrackFromSource()); + EXPECT_TRUE(proxy2.updateTrackFromSource( + SoundSourceProxy::UpdateTrackFromSourceMode::Once)); EXPECT_EQ("cover-test-png", pTrack2->getTitle()); // Test a reload also works pTrack2->setTitle(""); EXPECT_TRUE(proxy2.updateTrackFromSource( - SoundSourceProxy::UpdateTrackFromSourceMode::Again)); + SoundSourceProxy::UpdateTrackFromSourceMode::Always)); EXPECT_EQ("cover-test-png", pTrack2->getTitle()); // Test a file with a title auto pTrack3 = Track::newTemporary( kTestDir, "cover-test-jpg.mp3"); SoundSourceProxy proxy3(pTrack3); - EXPECT_TRUE(proxy3.updateTrackFromSource()); + EXPECT_TRUE(proxy3.updateTrackFromSource( + SoundSourceProxy::UpdateTrackFromSourceMode::Once)); EXPECT_EQ("test22kMono", pTrack3->getTitle()); } diff --git a/src/test/trackupdate_test.cpp b/src/test/trackupdate_test.cpp index 3b4b0f4709d..92a7132ae4a 100644 --- a/src/test/trackupdate_test.cpp +++ b/src/test/trackupdate_test.cpp @@ -29,8 +29,9 @@ class TrackUpdateTest : public MixxxTest, SoundSourceProviderRegistration { static TrackPointer newTestTrackParsed() { auto pTrack = newTestTrack(); - EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource()); - EXPECT_TRUE(pTrack->isSourceSynchronized()); + EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource( + SoundSourceProxy::UpdateTrackFromSourceMode::Once)); + EXPECT_TRUE(pTrack->checkSourceSynchronized()); EXPECT_TRUE(hasTrackMetadata(pTrack)); EXPECT_TRUE(hasCoverArt(pTrack)); pTrack->markClean(); @@ -66,7 +67,7 @@ TEST_F(TrackUpdateTest, parseModifiedCleanOnce) { const auto coverInfoAfter = pTrack->getCoverInfo(); // Verify that the track has not been modified - ASSERT_TRUE(pTrack->isSourceSynchronized()); + ASSERT_TRUE(pTrack->checkSourceSynchronized()); ASSERT_FALSE(pTrack->isDirty()); ASSERT_EQ(trackMetadataBefore, trackMetadataAfter); ASSERT_EQ(coverInfoBefore, coverInfoAfter); @@ -80,13 +81,13 @@ TEST_F(TrackUpdateTest, parseModifiedCleanAgainSkipCover) { const auto coverInfoBefore = pTrack->getCoverInfo(); EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource( - SoundSourceProxy::UpdateTrackFromSourceMode::Again)); + SoundSourceProxy::UpdateTrackFromSourceMode::Always)); const auto trackMetadataAfter = pTrack->getMetadata(); const auto coverInfoAfter = pTrack->getCoverInfo(); // Updated - EXPECT_TRUE(pTrack->isSourceSynchronized()); + EXPECT_TRUE(pTrack->checkSourceSynchronized()); EXPECT_TRUE(pTrack->isDirty()); EXPECT_NE(trackMetadataBefore, trackMetadataAfter); EXPECT_EQ(coverInfoBefore, coverInfoAfter); @@ -104,13 +105,13 @@ TEST_F(TrackUpdateTest, parseModifiedCleanAgainUpdateCover) { const auto coverInfoBefore = pTrack->getCoverInfo(); EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource( - SoundSourceProxy::UpdateTrackFromSourceMode::Again)); + SoundSourceProxy::UpdateTrackFromSourceMode::Always)); const auto trackMetadataAfter = pTrack->getMetadata(); const auto coverInfoAfter = pTrack->getCoverInfo(); // Updated - EXPECT_TRUE(pTrack->isSourceSynchronized()); + EXPECT_TRUE(pTrack->checkSourceSynchronized()); EXPECT_TRUE(pTrack->isDirty()); EXPECT_NE(trackMetadataBefore, trackMetadataAfter); EXPECT_NE(coverInfoBefore, coverInfoAfter); @@ -123,14 +124,16 @@ TEST_F(TrackUpdateTest, parseModifiedDirtyAgain) { const auto coverInfoBefore = pTrack->getCoverInfo(); EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource( - SoundSourceProxy::UpdateTrackFromSourceMode::Again)); + SoundSourceProxy::UpdateTrackFromSourceMode::Always)); const auto trackMetadataAfter = pTrack->getMetadata(); const auto coverInfoAfter = pTrack->getCoverInfo(); // Updated - EXPECT_TRUE(pTrack->isSourceSynchronized()); + EXPECT_TRUE(pTrack->checkSourceSynchronized()); EXPECT_TRUE(pTrack->isDirty()); EXPECT_NE(trackMetadataBefore, trackMetadataAfter); EXPECT_EQ(coverInfoBefore, coverInfoAfter); } + +// TODO: Add tests for SoundSourceProxy::UpdateTrackFromSourceMode::Newer diff --git a/src/track/track.cpp b/src/track/track.cpp index e3125bffb6c..c5303995e63 100644 --- a/src/track/track.cpp +++ b/src/track/track.cpp @@ -242,10 +242,11 @@ bool Track::mergeExtraMetadataFromSource( } mixxx::TrackMetadata Track::getMetadata( - bool* pSourceSynchronized) const { + mixxx::TrackRecord::SourceSyncStatus* pSourceSyncStatus) const { const auto locked = lockMutex(&m_qMutex); - if (pSourceSynchronized) { - *pSourceSynchronized = m_record.isSourceSynchronized(); + if (pSourceSyncStatus) { + *pSourceSyncStatus = + m_record.checkSourceSyncStatus(m_fileAccess.info()); } return m_record.getMetadata(); } @@ -473,9 +474,10 @@ void Track::emitChangedSignalsForAllMetadata() { emit keyChanged(); } -bool Track::isSourceSynchronized() const { +bool Track::checkSourceSynchronized() const { const auto locked = lockMutex(&m_qMutex); - return m_record.isSourceSynchronized(); + return m_record.checkSourceSyncStatus(m_fileAccess.info()) == + mixxx::TrackRecord::SourceSyncStatus::Synchronized; } void Track::setSourceSynchronizedAt(const QDateTime& sourceSynchronizedAt) { @@ -1444,8 +1446,11 @@ ExportTrackMetadataResult Track::exportMetadata( // TODO(XXX): Use sourceSynchronizedAt to decide if metadata // should be (re-)imported before exporting it. The file might // have been updated by external applications. Overwriting - // this modified metadata might not be intended. - if (!m_bMarkedForMetadataExport && !m_record.isSourceSynchronized()) { + // this modified metadata is probably not be intended if not + // explicitly requested by m_bMarkedForMetadataExport. + if (!m_bMarkedForMetadataExport && + m_record.checkSourceSyncStatus(m_fileAccess.info()) != + mixxx::TrackRecord::SourceSyncStatus::Synchronized) { // If the metadata has never been imported from file tags it // must be exported explicitly once. This ensures that we don't // overwrite existing file tags with completely different diff --git a/src/track/track.h b/src/track/track.h index c5595d141b4..5f97b0d2271 100644 --- a/src/track/track.h +++ b/src/track/track.h @@ -155,8 +155,9 @@ class Track : public QObject { // Returns ReplayGain mixxx::ReplayGain getReplayGain() const; - // Indicates if the metadata has been parsed from file tags. - bool isSourceSynchronized() const; + /// Checks if the internal metadata is in-sync with the + /// metadata stored in file tags. + bool checkSourceSynchronized() const; // The date/time of the last import or export of metadata void setSourceSynchronizedAt(const QDateTime& sourceSynchronizedAt); @@ -391,7 +392,8 @@ class Track : public QObject { const QDateTime& sourceSynchronizedAt); mixxx::TrackMetadata getMetadata( - bool* pHeaderParsed = nullptr) const; + mixxx::TrackRecord::SourceSyncStatus* + pSourceSyncStatus = nullptr) const; mixxx::TrackRecord getRecord( bool* pDirty = nullptr) const; diff --git a/src/track/trackrecord.cpp b/src/track/trackrecord.cpp index f8f2a47a352..d607f224287 100644 --- a/src/track/trackrecord.cpp +++ b/src/track/trackrecord.cpp @@ -1,5 +1,6 @@ #include "track/trackrecord.h" +#include "sources/metadatasource.h" #include "track/keyfactory.h" #include "util/logger.h" @@ -129,25 +130,50 @@ bool TrackRecord::updateSourceSynchronizedAt( } setSourceSynchronizedAt(sourceSynchronizedAt); m_headerParsed = sourceSynchronizedAt.isValid(); - DEBUG_ASSERT(isSourceSynchronized()); return true; } -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 - // triggers! +TrackRecord::SourceSyncStatus TrackRecord::checkSourceSyncStatus( + const FileInfo& fileInfo) const { + // This method cannot be used to update m_headerParsed after modifying + // m_sourceSynchronizedAt during a short moment of inconsistency. + // Otherwise the debug assertion triggers! DEBUG_ASSERT(m_headerParsed || !getSourceSynchronizedAt().isValid()); + // Legacy fallback: The property sourceSynchronizedAt has been added later. + // Files that have been added before that time and that have never been + // re-imported will only have that legacy flag set while sourceSynchronizedAt + // is still invalid. + if (!m_headerParsed) { + // Enforce initial import of metadata if it hasn't succeeded + // at least once yet. + return SourceSyncStatus::Void; + } if (getSourceSynchronizedAt().isValid()) { - return true; + const QDateTime fileSourceSynchronizedAt = + MetadataSource::getFileSynchronizedAt(fileInfo.asQFileInfo()); + if (fileSourceSynchronizedAt.isValid()) { + if (getSourceSynchronizedAt() < fileSourceSynchronizedAt) { + return SourceSyncStatus::Outdated; + } else { + if (getSourceSynchronizedAt() > fileSourceSynchronizedAt) { + kLogger.warning() + << "Internal source synchronization time stamp" + << getSourceSynchronizedAt() + << "is ahead of" + << fileSourceSynchronizedAt + << "for file" + << mixxx::FileInfo(fileInfo); + } + return SourceSyncStatus::Synchronized; + } + } else { + kLogger.warning() + << "Failed to obtain synchronization time stamp for file" + << mixxx::FileInfo(fileInfo); + } } - // Legacy fallback: The property sourceSynchronizedAt has been - // added later. Files that have been added before that time - // and that have never been re-imported will only have that - // legacy flag set while sourceSynchronizedAt is still invalid. - return m_headerParsed; + return SourceSyncStatus::Unknown; } bool TrackRecord::replaceMetadataFromSource( diff --git a/src/track/trackrecord.h b/src/track/trackrecord.h index b7737387236..a524eb9fa5d 100644 --- a/src/track/trackrecord.h +++ b/src/track/trackrecord.h @@ -111,7 +111,22 @@ class TrackRecord final { const QString& keyText, track::io::key::Source keySource); - bool isSourceSynchronized() const; + enum class SourceSyncStatus { + /// The metadata has not been imported yet. + Void, + + /// The status could not be determined for whatever reason, + /// e.g. insufficient data, inaccessible file, ... + Unknown, + + /// The metadata in Mixxx is up-to-date. + Synchronized, + + /// The metadata in Mixxx is older than the metadata stored in file tags. + Outdated, + }; + SourceSyncStatus checkSourceSyncStatus( + const FileInfo& fileInfo) const; bool replaceMetadataFromSource( TrackMetadata&& importedMetadata, const QDateTime& sourceSynchronizedAt); diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index cf7e0993a76..cfad66315d2 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -909,7 +909,7 @@ class ImportMetadataFromFileTagsTrackPointerOperation : public mixxx::TrackPoint // to override the information within Mixxx! Custom cover art must be // reloaded separately. SoundSourceProxy(pTrack).updateTrackFromSource( - SoundSourceProxy::UpdateTrackFromSourceMode::Again); + SoundSourceProxy::UpdateTrackFromSourceMode::Always); } }; From 3ad6fcdf83e7bda78e7598d2214c87e28127beb4 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sat, 21 Aug 2021 22:13:06 +0200 Subject: [PATCH 2/7] UI: Reword options for track metadata sync with file tags --- src/library/dao/trackdao.cpp | 2 +- src/library/library_prefs.cpp | 3 ++- src/library/library_prefs.h | 4 +++- src/library/trackcollectionmanager.cpp | 2 +- src/preferences/dialog/dlgpreflibrary.cpp | 14 +++++++------- src/preferences/dialog/dlgpreflibrarydlg.ui | 11 +++++++---- 6 files changed, 21 insertions(+), 15 deletions(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 175471ad6fe..b695982163e 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1496,7 +1496,7 @@ TrackPointer TrackDAO::getTrackById(TrackId trackId) const { SoundSourceProxy::UpdateTrackFromSourceMode::Once; if (m_pConfig && m_pConfig->getValueString( - mixxx::library::prefs::kSyncTrackMetadataExportConfigKey) + 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. diff --git a/src/library/library_prefs.cpp b/src/library/library_prefs.cpp index 020b930dcd8..f7cf06120cc 100644 --- a/src/library/library_prefs.cpp +++ b/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")}; diff --git a/src/library/library_prefs.h b/src/library/library_prefs.h index 32077ebc3b3..9312ec196bf 100644 --- a/src/library/library_prefs.h +++ b/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; diff --git a/src/library/trackcollectionmanager.cpp b/src/library/trackcollectionmanager.cpp index 761c64cceeb..d10e7762e51 100644 --- a/src/library/trackcollectionmanager.cpp +++ b/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: diff --git a/src/preferences/dialog/dlgpreflibrary.cpp b/src/preferences/dialog/dlgpreflibrary.cpp index cf7b43dac81..9b1a7491762 100644 --- a/src/preferences/dialog/dlgpreflibrary.cpp +++ b/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(); } } diff --git a/src/preferences/dialog/dlgpreflibrarydlg.ui b/src/preferences/dialog/dlgpreflibrarydlg.ui index 76ba872c915..04b8d28595f 100644 --- a/src/preferences/dialog/dlgpreflibrarydlg.ui +++ b/src/preferences/dialog/dlgpreflibrarydlg.ui @@ -132,16 +132,19 @@ - + - Automatically write modified track metadata from the library into file tags + Synchronize track metadata with file tags + + + Automatically write modified track metadata into file tags and re-import metadata from updated file tags - Write Serato Metadata to files (experimental) + Write Serato metadata to files (experimental) @@ -443,7 +446,7 @@ PushButtonAddDir PushButtonRelocateDir PushButtonRemoveDir - checkBox_SyncTrackMetadataExport + checkBox_SyncTrackMetadata checkBox_library_scan checkBoxEditMetadataSelectedClicked checkBox_use_relative_path From a33b9bb0e99bd9ee2762cc70080c1ea3364e659a Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 6 Sep 2021 11:27:52 +0200 Subject: [PATCH 3/7] Library preferences: Reword file synchronization options --- src/preferences/dialog/dlgpreflibrarydlg.ui | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/preferences/dialog/dlgpreflibrarydlg.ui b/src/preferences/dialog/dlgpreflibrarydlg.ui index 04b8d28595f..e4965a92256 100644 --- a/src/preferences/dialog/dlgpreflibrarydlg.ui +++ b/src/preferences/dialog/dlgpreflibrarydlg.ui @@ -134,10 +134,10 @@ - Synchronize track metadata with file tags + Synchronize library track metadata from/to file tags - Automatically write modified track metadata into file tags and re-import metadata from updated file tags + Automatically write modified track metadata from the library into file tags and re-import metadata from updated file tags into the library From 4576f9bd32baeb192c4ef11318b5b90179d59d23 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 6 Sep 2021 11:39:06 +0200 Subject: [PATCH 4/7] Track source sync: Log possible reasons for runtime errors --- src/track/trackrecord.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/track/trackrecord.cpp b/src/track/trackrecord.cpp index d607f224287..72eebcea3e3 100644 --- a/src/track/trackrecord.cpp +++ b/src/track/trackrecord.cpp @@ -163,14 +163,16 @@ TrackRecord::SourceSyncStatus TrackRecord::checkSourceSyncStatus( << "is ahead of" << fileSourceSynchronizedAt << "for file" - << mixxx::FileInfo(fileInfo); + << mixxx::FileInfo(fileInfo) + << ": Has this file been replaced with an older version?"; } return SourceSyncStatus::Synchronized; } } else { kLogger.warning() << "Failed to obtain synchronization time stamp for file" - << mixxx::FileInfo(fileInfo); + << mixxx::FileInfo(fileInfo) + << ": Is this file missing or inaccessible?"; } } return SourceSyncStatus::Unknown; From 278ec4fb46ee5ea99e984e8ef1b644a9c91435a7 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 16 Sep 2021 00:20:42 +0200 Subject: [PATCH 5/7] Only re-import Serato tags if Serato metadata export is enabled --- src/library/dao/trackdao.cpp | 5 ++++- src/sources/soundsourceproxy.cpp | 25 +++++++++++++++++++++++-- src/sources/soundsourceproxy.h | 1 + src/test/autodjprocessor_test.cpp | 4 +++- src/test/coverartutils_test.cpp | 1 + src/test/soundproxy_test.cpp | 6 ++++++ src/test/trackupdate_test.cpp | 9 +++++++-- src/widget/wtrackmenu.cpp | 11 ++++++++++- 8 files changed, 55 insertions(+), 7 deletions(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index fd07a7ad356..077784975bd 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -853,6 +853,7 @@ TrackPointer TrackDAO::addTracksAddFile( // Initially (re-)import the metadata for the newly created track // from the file. SoundSourceProxy(pTrack).updateTrackFromSource( + m_pConfig, SoundSourceProxy::UpdateTrackFromSourceMode::Once); if (!pTrack->checkSourceSynchronized()) { qWarning() << "TrackDAO::addTracksAddFile:" @@ -1504,7 +1505,9 @@ TrackPointer TrackDAO::getTrackById(TrackId trackId) const { updateTrackFromSourceMode = SoundSourceProxy::UpdateTrackFromSourceMode::Newer; } - SoundSourceProxy(pTrack).updateTrackFromSource(updateTrackFromSourceMode); + SoundSourceProxy(pTrack).updateTrackFromSource( + m_pConfig, + updateTrackFromSourceMode); if (kLogger.debugEnabled() && pTrack->isDirty()) { kLogger.debug() << "Updated track metadata from file tags:" diff --git a/src/sources/soundsourceproxy.cpp b/src/sources/soundsourceproxy.cpp index a88514e910e..5d169962f24 100644 --- a/src/sources/soundsourceproxy.cpp +++ b/src/sources/soundsourceproxy.cpp @@ -3,6 +3,7 @@ #include #include +#include "library/library_prefs.h" #include "sources/audiosourcetrackproxy.h" #ifdef __MAD__ @@ -518,7 +519,7 @@ SoundSourceProxy::importTrackMetadataAndCoverImage( namespace { -inline bool shouldUpdateTrackMetadatafromSource( +inline bool shouldUpdateTrackMetadataFromSource( SoundSourceProxy::UpdateTrackFromSourceMode mode, mixxx::TrackRecord::SourceSyncStatus sourceSyncStatus) { return mode == SoundSourceProxy::UpdateTrackFromSourceMode::Always || @@ -528,9 +529,23 @@ inline bool shouldUpdateTrackMetadatafromSource( sourceSyncStatus == mixxx::TrackRecord::SourceSyncStatus::Void); } +inline bool shouldImportSeratoTagsFromSource( + const UserSettingsPointer& pConfig, + mixxx::TrackRecord::SourceSyncStatus sourceSyncStatus) { + // Only reimport track metadata from Serato markers if export of + // Serato markers is enabled. This should ensure that track color, + // cue points, and loops do not get lost after they have been + // modified in Mixxx. + // A reimport of metadata happens if + // sourceSyncStatus == mixxx::TrackRecord::SourceSyncStatus::Outdated + return sourceSyncStatus != mixxx::TrackRecord::SourceSyncStatus::Outdated || + pConfig->getValue(mixxx::library::prefs::kSeratoMetadataExportConfigKey); +} + } // namespace bool SoundSourceProxy::updateTrackFromSource( + const UserSettingsPointer& pConfig, UpdateTrackFromSourceMode mode) { DEBUG_ASSERT(m_pTrack); @@ -569,7 +584,7 @@ bool SoundSourceProxy::updateTrackFromSource( QImage* pCoverImg = nullptr; // pointer also serves as a flag const bool updateMetadataFromSource = - shouldUpdateTrackMetadatafromSource(mode, sourceSyncStatus); + shouldUpdateTrackMetadataFromSource(mode, sourceSyncStatus); // Decide if cover art needs to be re-imported if (updateMetadataFromSource) { @@ -631,6 +646,12 @@ bool SoundSourceProxy::updateTrackFromSource( // Full import DEBUG_ASSERT(updateMetadataFromSource); + if (!shouldImportSeratoTagsFromSource( + pConfig, + sourceSyncStatus)) { + // Reset Serato tags to disable the (re-)import + trackMetadata.refTrackInfo().refSeratoTags() = {}; + } if (sourceSyncStatus == mixxx::TrackRecord::SourceSyncStatus::Void) { DEBUG_ASSERT(pCoverImg); if (kLogger.debugEnabled()) { diff --git a/src/sources/soundsourceproxy.h b/src/sources/soundsourceproxy.h index ecf0b8df768..87d85089aae 100644 --- a/src/sources/soundsourceproxy.h +++ b/src/sources/soundsourceproxy.h @@ -149,6 +149,7 @@ class SoundSourceProxy { /// /// Returns true if the track has been modified and false otherwise. bool updateTrackFromSource( + const UserSettingsPointer& pConfig, UpdateTrackFromSourceMode mode); /// Opening the audio source through the proxy will update the diff --git a/src/test/autodjprocessor_test.cpp b/src/test/autodjprocessor_test.cpp index 57638ea6376..67d039c9ace 100644 --- a/src/test/autodjprocessor_test.cpp +++ b/src/test/autodjprocessor_test.cpp @@ -167,10 +167,12 @@ class AutoDJProcessorTest : public LibraryTest { static TrackId nextTrackId(TrackId trackId) { return TrackId(trackId.value() + 1); } - static TrackPointer newTestTrack(TrackId trackId) { + + TrackPointer newTestTrack(TrackId trackId) { TrackPointer pTrack( Track::newDummy(kTrackLocationTest, trackId)); EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource( + config(), SoundSourceProxy::UpdateTrackFromSourceMode::Once)); return pTrack; } diff --git a/src/test/coverartutils_test.cpp b/src/test/coverartutils_test.cpp index f9b3206ed02..0bec9234b99 100644 --- a/src/test/coverartutils_test.cpp +++ b/src/test/coverartutils_test.cpp @@ -115,6 +115,7 @@ TEST_F(CoverArtUtilTest, searchImage) { // Looking for a track with embedded cover. pTrack = Track::newTemporary(kTrackLocationTest); EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource( + config(), SoundSourceProxy::UpdateTrackFromSourceMode::Once)); CoverInfo result = pTrack->getCoverInfoWithLocation(); EXPECT_EQ(result.type, CoverInfo::METADATA); diff --git a/src/test/soundproxy_test.cpp b/src/test/soundproxy_test.cpp index be2bb34b472..770abebe4c9 100644 --- a/src/test/soundproxy_test.cpp +++ b/src/test/soundproxy_test.cpp @@ -214,6 +214,7 @@ TEST_F(SoundSourceProxyTest, readArtist) { auto pTrack = Track::newTemporary(kTestDir, "artist.mp3"); SoundSourceProxy proxy(pTrack); EXPECT_TRUE(proxy.updateTrackFromSource( + config(), SoundSourceProxy::UpdateTrackFromSourceMode::Once)); EXPECT_EQ("Test Artist", pTrack->getArtist()); } @@ -226,12 +227,14 @@ TEST_F(SoundSourceProxyTest, readNoTitle) { kTestDir, "empty.mp3"); SoundSourceProxy proxy1(pTrack1); EXPECT_TRUE(proxy1.updateTrackFromSource( + config(), SoundSourceProxy::UpdateTrackFromSourceMode::Once)); EXPECT_EQ("empty", pTrack1->getTitle()); // Test a reload also works pTrack1->setTitle(""); EXPECT_TRUE(proxy1.updateTrackFromSource( + config(), SoundSourceProxy::UpdateTrackFromSourceMode::Always)); EXPECT_EQ("empty", pTrack1->getTitle()); @@ -240,12 +243,14 @@ TEST_F(SoundSourceProxyTest, readNoTitle) { kTestDir, "cover-test-png.mp3"); SoundSourceProxy proxy2(pTrack2); EXPECT_TRUE(proxy2.updateTrackFromSource( + config(), SoundSourceProxy::UpdateTrackFromSourceMode::Once)); EXPECT_EQ("cover-test-png", pTrack2->getTitle()); // Test a reload also works pTrack2->setTitle(""); EXPECT_TRUE(proxy2.updateTrackFromSource( + config(), SoundSourceProxy::UpdateTrackFromSourceMode::Always)); EXPECT_EQ("cover-test-png", pTrack2->getTitle()); @@ -254,6 +259,7 @@ TEST_F(SoundSourceProxyTest, readNoTitle) { kTestDir, "cover-test-jpg.mp3"); SoundSourceProxy proxy3(pTrack3); EXPECT_TRUE(proxy3.updateTrackFromSource( + config(), SoundSourceProxy::UpdateTrackFromSourceMode::Once)); EXPECT_EQ("test22kMono", pTrack3->getTitle()); } diff --git a/src/test/trackupdate_test.cpp b/src/test/trackupdate_test.cpp index 92a7132ae4a..c08ccb6c0d8 100644 --- a/src/test/trackupdate_test.cpp +++ b/src/test/trackupdate_test.cpp @@ -27,9 +27,10 @@ class TrackUpdateTest : public MixxxTest, SoundSourceProviderRegistration { return Track::newTemporary(kTestDir, "TOAL_TPE2.mp3"); } - static TrackPointer newTestTrackParsed() { + TrackPointer newTestTrackParsed() { auto pTrack = newTestTrack(); EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource( + config(), SoundSourceProxy::UpdateTrackFromSourceMode::Once)); EXPECT_TRUE(pTrack->checkSourceSynchronized()); EXPECT_TRUE(hasTrackMetadata(pTrack)); @@ -39,7 +40,7 @@ class TrackUpdateTest : public MixxxTest, SoundSourceProviderRegistration { return pTrack; } - static TrackPointer newTestTrackParsedModified() { + TrackPointer newTestTrackParsedModified() { auto pTrack = newTestTrackParsed(); pTrack->setArtist(pTrack->getArtist() + pTrack->getArtist()); auto coverInfo = pTrack->getCoverInfo(); @@ -61,6 +62,7 @@ TEST_F(TrackUpdateTest, parseModifiedCleanOnce) { // Re-update from source should have no effect ASSERT_FALSE(SoundSourceProxy(pTrack).updateTrackFromSource( + config(), SoundSourceProxy::UpdateTrackFromSourceMode::Once)); const auto trackMetadataAfter = pTrack->getMetadata(); @@ -81,6 +83,7 @@ TEST_F(TrackUpdateTest, parseModifiedCleanAgainSkipCover) { const auto coverInfoBefore = pTrack->getCoverInfo(); EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource( + config(), SoundSourceProxy::UpdateTrackFromSourceMode::Always)); const auto trackMetadataAfter = pTrack->getMetadata(); @@ -105,6 +108,7 @@ TEST_F(TrackUpdateTest, parseModifiedCleanAgainUpdateCover) { const auto coverInfoBefore = pTrack->getCoverInfo(); EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource( + config(), SoundSourceProxy::UpdateTrackFromSourceMode::Always)); const auto trackMetadataAfter = pTrack->getMetadata(); @@ -124,6 +128,7 @@ TEST_F(TrackUpdateTest, parseModifiedDirtyAgain) { const auto coverInfoBefore = pTrack->getCoverInfo(); EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource( + config(), SoundSourceProxy::UpdateTrackFromSourceMode::Always)); const auto trackMetadataAfter = pTrack->getMetadata(); diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index cfad66315d2..6466e7ff3ff 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -902,6 +902,12 @@ void WTrackMenu::slotOpenInFileBrowser() { namespace { class ImportMetadataFromFileTagsTrackPointerOperation : public mixxx::TrackPointerOperation { + public: + explicit ImportMetadataFromFileTagsTrackPointerOperation( + UserSettingsPointer pConfig) + : m_pConfig(std::move(pConfig)) { + } + private: void doApply( const TrackPointer& pTrack) const override { @@ -909,8 +915,11 @@ class ImportMetadataFromFileTagsTrackPointerOperation : public mixxx::TrackPoint // to override the information within Mixxx! Custom cover art must be // reloaded separately. SoundSourceProxy(pTrack).updateTrackFromSource( + m_pConfig, SoundSourceProxy::UpdateTrackFromSourceMode::Always); } + + const UserSettingsPointer m_pConfig; }; } // anonymous namespace @@ -935,7 +944,7 @@ void WTrackMenu::slotImportMetadataFromFileTags() { const auto progressLabelText = tr("Importing metadata of %n track(s) from file tags", "", getTrackCount()); const auto trackOperator = - ImportMetadataFromFileTagsTrackPointerOperation(); + ImportMetadataFromFileTagsTrackPointerOperation(m_pConfig); applyTrackPointerOperation( progressLabelText, &trackOperator, From f315803dc36442306bb5b4bf1cf9c51a7a131f28 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 16 Sep 2021 09:40:27 +0200 Subject: [PATCH 6/7] Library Prefs: Reword and explain Serato metadata synchronization --- src/library/library_prefs.cpp | 3 ++- src/library/library_prefs.h | 4 ++-- src/preferences/dialog/dlgpreflibrary.cpp | 4 ++-- src/preferences/dialog/dlgpreflibrarydlg.ui | 7 +++++-- src/sources/soundsourceproxy.cpp | 2 +- src/track/track.cpp | 2 +- 6 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/library/library_prefs.cpp b/src/library/library_prefs.cpp index f7cf06120cc..b3b1e3d7316 100644 --- a/src/library/library_prefs.cpp +++ b/src/library/library_prefs.cpp @@ -34,7 +34,8 @@ const ConfigKey mixxx::library::prefs::kSyncTrackMetadataConfigKey = mixxx::library::prefs::kConfigGroup, QStringLiteral("SyncTrackMetadataExport")}; -const ConfigKey mixxx::library::prefs::kSeratoMetadataExportConfigKey = +// The naming is unchanged for backward compatibility +const ConfigKey mixxx::library::prefs::kSyncSeratoMetadataConfigKey = ConfigKey{ mixxx::library::prefs::kConfigGroup, QStringLiteral("SeratoMetadataExport")}; diff --git a/src/library/library_prefs.h b/src/library/library_prefs.h index 9312ec196bf..cf3bf3b6cb3 100644 --- a/src/library/library_prefs.h +++ b/src/library/library_prefs.h @@ -22,9 +22,9 @@ const bool kEditMetadataSelectedClickDefault = false; extern const ConfigKey kSyncTrackMetadataConfigKey; -extern const ConfigKey kSeratoMetadataExportConfigKey; +extern const ConfigKey kSyncSeratoMetadataConfigKey; -extern const ConfigKey kSeratoMetadataExportConfigKey; +extern const ConfigKey kSyncSeratoMetadataConfigKey; } // namespace prefs diff --git a/src/preferences/dialog/dlgpreflibrary.cpp b/src/preferences/dialog/dlgpreflibrary.cpp index 9b1a7491762..de2903222c9 100644 --- a/src/preferences/dialog/dlgpreflibrary.cpp +++ b/src/preferences/dialog/dlgpreflibrary.cpp @@ -200,7 +200,7 @@ void DlgPrefLibrary::slotUpdate() { checkBox_SyncTrackMetadata->setChecked( m_pConfig->getValue(kSyncTrackMetadataConfigKey, false)); checkBox_SeratoMetadataExport->setChecked( - m_pConfig->getValue(kSeratoMetadataExportConfigKey, false)); + m_pConfig->getValue(kSyncSeratoMetadataConfigKey, false)); checkBox_use_relative_path->setChecked(m_pConfig->getValue( ConfigKey("[Library]","UseRelativePathOnExport"), false)); checkBox_show_rhythmbox->setChecked(m_pConfig->getValue( @@ -368,7 +368,7 @@ void DlgPrefLibrary::slotApply() { kSyncTrackMetadataConfigKey, ConfigValue{checkBox_SyncTrackMetadata->isChecked()}); m_pConfig->set( - kSeratoMetadataExportConfigKey, + kSyncSeratoMetadataConfigKey, ConfigValue{checkBox_SeratoMetadataExport->isChecked()}); m_pConfig->set(ConfigKey("[Library]","UseRelativePathOnExport"), ConfigValue((int)checkBox_use_relative_path->isChecked())); diff --git a/src/preferences/dialog/dlgpreflibrarydlg.ui b/src/preferences/dialog/dlgpreflibrarydlg.ui index e4965a92256..ddc0597857e 100644 --- a/src/preferences/dialog/dlgpreflibrarydlg.ui +++ b/src/preferences/dialog/dlgpreflibrarydlg.ui @@ -137,14 +137,17 @@ Synchronize library track metadata from/to file tags - Automatically write modified track metadata from the library into file tags and re-import metadata from updated file tags into the library + Automatically write modified track metadata from the library into file tags and reimport metadata from updated file tags into the library - Write Serato metadata to files (experimental) + Synchronize Serato track metadata from/to file tags (experimental) + + + Keeps track color, beat grid, bpm lock, cue points, and loops synchronized with SERATO_MARKERS/MARKERS2 file tags.<br/><br/>WARNING: Enabling this option also enables the reimport of Serato metadata after files have been modified outside of Mixxx. On reimport existing metadata in Mixxx is replaced with the metadata found in file tags. Custom metadata not included in file tags like loop colors is lost. diff --git a/src/sources/soundsourceproxy.cpp b/src/sources/soundsourceproxy.cpp index 5d169962f24..47ff592204f 100644 --- a/src/sources/soundsourceproxy.cpp +++ b/src/sources/soundsourceproxy.cpp @@ -539,7 +539,7 @@ inline bool shouldImportSeratoTagsFromSource( // A reimport of metadata happens if // sourceSyncStatus == mixxx::TrackRecord::SourceSyncStatus::Outdated return sourceSyncStatus != mixxx::TrackRecord::SourceSyncStatus::Outdated || - pConfig->getValue(mixxx::library::prefs::kSeratoMetadataExportConfigKey); + pConfig->getValue(mixxx::library::prefs::kSyncSeratoMetadataConfigKey); } } // namespace diff --git a/src/track/track.cpp b/src/track/track.cpp index 314283c4cea..d48885ebaf7 100644 --- a/src/track/track.cpp +++ b/src/track/track.cpp @@ -1459,7 +1459,7 @@ ExportTrackMetadataResult Track::exportMetadata( return ExportTrackMetadataResult::Skipped; } - if (pConfig->getValue(mixxx::library::prefs::kSeratoMetadataExportConfigKey)) { + if (pConfig->getValue(mixxx::library::prefs::kSyncSeratoMetadataConfigKey)) { const auto streamInfo = m_record.getStreamInfoFromSource(); VERIFY_OR_DEBUG_ASSERT(streamInfo && streamInfo->getSignalInfo().isValid() && From 5dfc1808b862d4bb3397d68500db31900fe55165 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 17 Sep 2021 14:33:41 +0200 Subject: [PATCH 7/7] Add missing const qualifiers in test classes --- src/test/autodjprocessor_test.cpp | 2 +- src/test/trackupdate_test.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/test/autodjprocessor_test.cpp b/src/test/autodjprocessor_test.cpp index 67d039c9ace..9ec3ae33d06 100644 --- a/src/test/autodjprocessor_test.cpp +++ b/src/test/autodjprocessor_test.cpp @@ -168,7 +168,7 @@ class AutoDJProcessorTest : public LibraryTest { return TrackId(trackId.value() + 1); } - TrackPointer newTestTrack(TrackId trackId) { + TrackPointer newTestTrack(TrackId trackId) const { TrackPointer pTrack( Track::newDummy(kTrackLocationTest, trackId)); EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource( diff --git a/src/test/trackupdate_test.cpp b/src/test/trackupdate_test.cpp index c08ccb6c0d8..4cd0a44e30b 100644 --- a/src/test/trackupdate_test.cpp +++ b/src/test/trackupdate_test.cpp @@ -27,7 +27,7 @@ class TrackUpdateTest : public MixxxTest, SoundSourceProviderRegistration { return Track::newTemporary(kTestDir, "TOAL_TPE2.mp3"); } - TrackPointer newTestTrackParsed() { + TrackPointer newTestTrackParsed() const { auto pTrack = newTestTrack(); EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource( config(), @@ -40,7 +40,7 @@ class TrackUpdateTest : public MixxxTest, SoundSourceProviderRegistration { return pTrack; } - TrackPointer newTestTrackParsedModified() { + TrackPointer newTestTrackParsedModified() const { auto pTrack = newTestTrackParsed(); pTrack->setArtist(pTrack->getArtist() + pTrack->getArtist()); auto coverInfo = pTrack->getCoverInfo();