Merge pull request #3978 from uklotzde/source-synchronized-timestamp

DB v37: Synchronize file modified time with track source on metadata import/export

res/schema.xml

@@ -557,4 +557,13 @@ reapplying those migrations.
           GROUP BY PlaylistTracks.track_id);
     </sql>
   </revision>
+  <revision version="37" min_compatible="3">
+    <description>
+      Add source_synchronized_ms column to library table
+    </description>
+    <!-- source_synchronized_ms: in milliseconds since 1970-01-01T00:00:00.000 UTC -->
+    <sql>
+      ALTER TABLE library ADD COLUMN source_synchronized_ms INTEGER DEFAULT NULL;
+    </sql>
+  </revision>
 </schema>

src/database/mixxxdb.cpp

@@ -12,7 +12,7 @@
 const QString MixxxDb::kDefaultSchemaFile(":/schema.xml");
 
 //static
-const int MixxxDb::kRequiredSchemaVersion = 36;
+const int MixxxDb::kRequiredSchemaVersion = 37;
 
 namespace {
 

src/library/dao/trackdao.cpp

@@ -396,6 +396,7 @@ void TrackDAO::addTracksPrepare() {
             "played,"
             "mixxx_deleted,"
             "header_parsed,"
+            "source_synchronized_ms,"
             "channels,"
             "samplerate,"
             "bitrate,"
@@ -443,6 +444,7 @@ void TrackDAO::addTracksPrepare() {
             ":played,"
             ":mixxx_deleted,"
             ":header_parsed,"
+            ":source_synchronized_ms,"
             ":channels,"
             ":samplerate,"
             ":bitrate,"
@@ -549,7 +551,17 @@ void bindTrackLibraryValues(
             trackMetadata.getStreamInfo().getDuration().toDoubleSeconds());
 
     pTrackLibraryQuery->bindValue(":header_parsed",
-            track.getMetadataSynchronized() ? 1 : 0);
+            track.isSourceSynchronized() ? 1 : 0);
+    const QDateTime sourceSynchronizedAt =
+            track.getSourceSynchronizedAt();
+    if (sourceSynchronizedAt.isValid()) {
+        DEBUG_ASSERT(sourceSynchronizedAt.timeSpec() == Qt::UTC);
+        pTrackLibraryQuery->bindValue(":source_synchronized_ms",
+                sourceSynchronizedAt.toMSecsSinceEpoch());
+    } else {
+        pTrackLibraryQuery->bindValue(":source_synchronized_ms",
+                QVariant());
+    }
 
     const PlayCounter& playCounter = track.getPlayCounter();
     pTrackLibraryQuery->bindValue(":timesplayed", playCounter.getTimesPlayed());
@@ -828,7 +840,7 @@ TrackPointer TrackDAO::addTracksAddFile(
     // Initially (re-)import the metadata for the newly created track
     // from the file.
     SoundSourceProxy(pTrack).updateTrackFromSource();
-    if (!pTrack->isMetadataSynchronized()) {
+    if (!pTrack->isSourceSynchronized()) {
         qWarning() << "TrackDAO::addTracksAddFile:"
                 << "Failed to parse track metadata from file"
                 << pTrack->getLocation();
@@ -1189,9 +1201,20 @@ bool setTrackFiletype(const QSqlRecord& record, const int column,
     return false;
 }
 
-bool setTrackMetadataSynchronized(const QSqlRecord& record, const int column,
-                          TrackPointer pTrack) {
-    pTrack->setMetadataSynchronized(record.value(column).toBool());
+bool setTrackHeaderParsed(const QSqlRecord& record, const int column, TrackPointer pTrack) {
+    pTrack->setHeaderParsedFromTrackDAO(record.value(column).toBool());
+    return false;
+}
+
+bool setTrackSourceSynchronizedAt(const QSqlRecord& record, const int column, TrackPointer pTrack) {
+    QDateTime sourceSynchronizedAt;
+    const QVariant value = record.value(column);
+    if (value.isValid() && value.canConvert<quint64>()) {
+        const quint64 msecsSinceEpoch = qvariant_cast<quint64>(value);
+        sourceSynchronizedAt.setTimeSpec(Qt::UTC);
+        sourceSynchronizedAt.setMSecsSinceEpoch(msecsSinceEpoch);
+    }
+    pTrack->setSourceSynchronizedAt(sourceSynchronizedAt);
     return false;
 }
 
@@ -1332,7 +1355,8 @@ TrackPointer TrackDAO::getTrackById(TrackId trackId) const {
             {"last_played_at", setTrackLastPlayedAt},
             {"played", setTrackPlayed},
             {"datetime_added", setTrackDateAdded},
-            {"header_parsed", setTrackMetadataSynchronized},
+            {"header_parsed", setTrackHeaderParsed},
+            {"source_synchronized_ms", setTrackSourceSynchronizedAt},
 
             // Audio properties are set together at once. Do not change the
             // ordering of these columns or put other columns in between them!
@@ -1600,6 +1624,7 @@ bool TrackDAO::updateTrack(Track* pTrack) const {
             "last_played_at=:last_played_at,"
             "played=:played,"
             "header_parsed=:header_parsed,"
+            "source_synchronized_ms=:source_synchronized_ms,"
             "channels=:channels,"
             "bitrate=:bitrate,"
             "samplerate=:samplerate,"

src/library/dlgtrackinfo.cpp

@@ -674,7 +674,7 @@ void DlgTrackInfo::slotImportMetadataFromFile() {
     mixxx::TrackRecord trackRecord = m_pLoadedTrack->getRecord();
     mixxx::TrackMetadata trackMetadata = trackRecord.getMetadata();
     QImage coverImage;
-    const auto [importResult, metadataSynchronized] =
+    const auto [importResult, sourceSynchronizedAt] =
             SoundSourceProxy(m_pLoadedTrack)
                     .importTrackMetadataAndCoverImage(
                             &trackMetadata, &coverImage);
@@ -688,7 +688,7 @@ void DlgTrackInfo::slotImportMetadataFromFile() {
             coverImage);
     trackRecord.replaceMetadataFromSource(
             std::move(trackMetadata),
-            metadataSynchronized);
+            sourceSynchronizedAt);
     trackRecord.setCoverInfo(
             std::move(guessedCoverInfo));
     replaceTrackRecord(

src/sources/metadatasourcetaglib.cpp

@@ -75,20 +75,20 @@ class AiffFile : public TagLib::RIFF::AIFF::File {
     }
 };
 
-inline QDateTime getMetadataSynchronized(const QFileInfo& fileInfo) {
-    return fileInfo.lastModified();
+inline QDateTime getSourceSynchronizedAt(const QFileInfo& fileInfo) {
+    return fileInfo.lastModified().toUTC();
 }
 
 } // anonymous namespace
 
 std::pair<MetadataSourceTagLib::ImportResult, QDateTime>
 MetadataSourceTagLib::afterImport(ImportResult importResult) const {
-    return std::make_pair(importResult, getMetadataSynchronized(QFileInfo(m_fileName)));
+    return std::make_pair(importResult, getSourceSynchronizedAt(QFileInfo(m_fileName)));
 }
 
 std::pair<MetadataSourceTagLib::ExportResult, QDateTime>
 MetadataSourceTagLib::afterExport(ExportResult exportResult) const {
-    return std::make_pair(exportResult, getMetadataSynchronized(QFileInfo(m_fileName)));
+    return std::make_pair(exportResult, getSourceSynchronizedAt(QFileInfo(m_fileName)));
 }
 
 std::pair<MetadataSource::ImportResult, QDateTime>

src/sources/soundsourceproxy.cpp

@@ -539,9 +539,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 metadataSynchronized = false;
+    bool headerParsed = false;
     mixxx::TrackMetadata trackMetadata =
-            m_pTrack->getMetadata(&metadataSynchronized);
+            m_pTrack->getMetadata(&headerParsed);
 
     // Save for later to replace the unreliable and imprecise audio
     // properties imported from file tags (see below).
@@ -559,7 +559,7 @@ bool SoundSourceProxy::updateTrackFromSource(
     // if the user did not explicitly choose to (re-)import metadata
     // explicitly from this file.
     bool mergeExtraMetadataFromSource = false;
-    if (metadataSynchronized && mode == UpdateTrackFromSourceMode::Once) {
+    if (headerParsed && mode == UpdateTrackFromSourceMode::Once) {
         // No (re-)import needed or desired, only merge missing properties
         mergeExtraMetadataFromSource = true;
     } else {
@@ -592,7 +592,7 @@ bool SoundSourceProxy::updateTrackFromSource(
                 << "from file"
                 << getUrl().toString();
         // make sure that the trackMetadata was not messed up due to the failure
-        trackMetadata = m_pTrack->getMetadata(&metadataSynchronized);
+        trackMetadata = m_pTrack->getMetadata(&headerParsed);
     }
 
     // Partial import
@@ -610,7 +610,7 @@ bool SoundSourceProxy::updateTrackFromSource(
     }
 
     // Full import
-    if (metadataSynchronized) {
+    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

src/test/trackupdate_test.cpp

@@ -30,7 +30,7 @@ class TrackUpdateTest : public MixxxTest, SoundSourceProviderRegistration {
     static TrackPointer newTestTrackParsed() {
         auto pTrack = newTestTrack();
         EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource());
-        EXPECT_TRUE(pTrack->isMetadataSynchronized());
+        EXPECT_TRUE(pTrack->isSourceSynchronized());
         EXPECT_TRUE(hasTrackMetadata(pTrack));
         EXPECT_TRUE(hasCoverArt(pTrack));
         pTrack->markClean();
@@ -66,7 +66,7 @@ TEST_F(TrackUpdateTest, parseModifiedCleanOnce) {
     const auto coverInfoAfter = pTrack->getCoverInfo();
 
     // Verify that the track has not been modified
-    ASSERT_TRUE(pTrack->isMetadataSynchronized());
+    ASSERT_TRUE(pTrack->isSourceSynchronized());
     ASSERT_FALSE(pTrack->isDirty());
     ASSERT_EQ(trackMetadataBefore, trackMetadataAfter);
     ASSERT_EQ(coverInfoBefore, coverInfoAfter);
@@ -86,7 +86,7 @@ TEST_F(TrackUpdateTest, parseModifiedCleanAgainSkipCover) {
     const auto coverInfoAfter = pTrack->getCoverInfo();
 
     // Updated
-    EXPECT_TRUE(pTrack->isMetadataSynchronized());
+    EXPECT_TRUE(pTrack->isSourceSynchronized());
     EXPECT_TRUE(pTrack->isDirty());
     EXPECT_NE(trackMetadataBefore, trackMetadataAfter);
     EXPECT_EQ(coverInfoBefore, coverInfoAfter);
@@ -110,7 +110,7 @@ TEST_F(TrackUpdateTest, parseModifiedCleanAgainUpdateCover) {
     const auto coverInfoAfter = pTrack->getCoverInfo();
 
     // Updated
-    EXPECT_TRUE(pTrack->isMetadataSynchronized());
+    EXPECT_TRUE(pTrack->isSourceSynchronized());
     EXPECT_TRUE(pTrack->isDirty());
     EXPECT_NE(trackMetadataBefore, trackMetadataAfter);
     EXPECT_NE(coverInfoBefore, coverInfoAfter);
@@ -129,7 +129,7 @@ TEST_F(TrackUpdateTest, parseModifiedDirtyAgain) {
     const auto coverInfoAfter = pTrack->getCoverInfo();
 
     // Updated
-    EXPECT_TRUE(pTrack->isMetadataSynchronized());
+    EXPECT_TRUE(pTrack->isSourceSynchronized());
     EXPECT_TRUE(pTrack->isDirty());
     EXPECT_NE(trackMetadataBefore, trackMetadataAfter);
     EXPECT_EQ(coverInfoBefore, coverInfoAfter);

src/track/track.cpp

@@ -135,7 +135,7 @@ void Track::relocate(
 
 void Track::replaceMetadataFromSource(
         mixxx::TrackMetadata importedMetadata,
-        const QDateTime& metadataSynchronized) {
+        const QDateTime& sourceSynchronizedAt) {
     // Information stored in Serato tags is imported separately after
     // importing the metadata (see below). The Serato tags BLOB itself
     // is updated together with the metadata.
@@ -164,7 +164,7 @@ void Track::replaceMetadataFromSource(
                 m_record.getMetadata().getTrackInfo().getReplayGain();
         bool modified = m_record.replaceMetadataFromSource(
                 std::move(importedMetadata),
-                metadataSynchronized);
+                sourceSynchronizedAt);
         const auto newReplayGain =
                 m_record.getMetadata().getTrackInfo().getReplayGain();
 
@@ -245,10 +245,10 @@ bool Track::mergeExtraMetadataFromSource(
 }
 
 mixxx::TrackMetadata Track::getMetadata(
-        bool* pMetadataSynchronized) const {
+        bool* pSourceSynchronized) const {
     const QMutexLocker locked(&m_qMutex);
-    if (pMetadataSynchronized) {
-        *pMetadataSynchronized = m_record.getMetadataSynchronized();
+    if (pSourceSynchronized) {
+        *pSourceSynchronized = m_record.isSourceSynchronized();
     }
     return m_record.getMetadata();
 }
@@ -461,16 +461,23 @@ void Track::emitChangedSignalsForAllMetadata() {
     emit keyChanged();
 }
 
-void Track::setMetadataSynchronized(bool metadataSynchronized) {
+bool Track::isSourceSynchronized() const {
     QMutexLocker lock(&m_qMutex);
-    if (compareAndSet(m_record.ptrMetadataSynchronized(), metadataSynchronized)) {
+    return m_record.isSourceSynchronized();
+}
+
+void Track::setSourceSynchronizedAt(const QDateTime& sourceSynchronizedAt) {
+    DEBUG_ASSERT(!sourceSynchronizedAt.isValid() ||
+            sourceSynchronizedAt.timeSpec() == Qt::UTC);
+    QMutexLocker lock(&m_qMutex);
+    if (compareAndSet(m_record.ptrSourceSynchronizedAt(), sourceSynchronizedAt)) {
         markDirtyAndUnlock(&lock);
     }
 }
 
-bool Track::isMetadataSynchronized() const {
+QDateTime Track::getSourceSynchronizedAt() const {
     QMutexLocker lock(&m_qMutex);
-    return m_record.getMetadataSynchronized();
+    return m_record.getSourceSynchronizedAt();
 }
 
 QString Track::getInfo() const {
@@ -1430,12 +1437,11 @@ ExportTrackMetadataResult Track::exportMetadata(
     // be called after all references to the object have been dropped.
     // But it doesn't hurt much, so let's play it safe ;)
     QMutexLocker lock(&m_qMutex);
-    // TODO(XXX): m_record.getMetadataSynchronized() currently is a
-    // boolean flag, but it should become a time stamp in the future.
-    // We could take this time stamp and the file's last modification
-    // time stamp into account and might decide to skip importing
-    // the metadata again.
-    if (!m_bMarkedForMetadataExport && !m_record.getMetadataSynchronized()) {
+    // 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()) {
         // 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
@@ -1577,10 +1583,7 @@ ExportTrackMetadataResult Track::exportMetadata(
         // This information (flag or time stamp) is stored in the database.
         // The database update will follow immediately after returning from
         // this operation!
-        // TODO(XXX): Replace bool with QDateTime
-        DEBUG_ASSERT(!trackMetadataExported.second.isNull());
-        //pTrack->setMetadataSynchronized(trackMetadataExported.second);
-        m_record.setMetadataSynchronized(!trackMetadataExported.second.isNull());
+        m_record.updateSourceSynchronizedAt(trackMetadataExported.second);
         if (kLogger.debugEnabled()) {
             kLogger.debug()
                     << "Exported track metadata:"

src/track/track.h

@@ -72,6 +72,7 @@ class Track : public QObject {
                     STORED false NOTIFY durationChanged)
     Q_PROPERTY(QString info READ getInfo STORED false NOTIFY infoChanged)
     Q_PROPERTY(QString titleInfo READ getTitleInfo STORED false NOTIFY infoChanged)
+    Q_PROPERTY(QDateTime sourceSynchronizedAt READ getSourceSynchronizedAt STORED false)
 
     mixxx::FileAccess getFileAccess() const {
         // Copying QFileInfo is thread-safe due to implicit sharing,
@@ -148,9 +149,18 @@ class Track : public QObject {
     mixxx::ReplayGain getReplayGain() const;
 
     // Indicates if the metadata has been parsed from file tags.
-    bool isMetadataSynchronized() const;
-    // Only used by a free function in TrackDAO!
-    void setMetadataSynchronized(bool metadataSynchronized);
+    bool isSourceSynchronized() const;
+
+    void setHeaderParsedFromTrackDAO(bool headerParsed) {
+        // Always operating on a newly created, exclusive instance! No need
+        // to lock the mutex.
+        DEBUG_ASSERT(!m_record.m_headerParsed);
+        m_record.m_headerParsed = headerParsed;
+    }
+
+    // The date/time of the last import or export of metadata
+    void setSourceSynchronizedAt(const QDateTime& sourceSynchronizedAt);
+    QDateTime getSourceSynchronizedAt() const;
 
     void setDateAdded(const QDateTime& dateAdded);
     QDateTime getDateAdded() const;
@@ -336,10 +346,10 @@ class Track : public QObject {
     /// with file tags, either by importing or exporting the metadata.
     void replaceMetadataFromSource(
             mixxx::TrackMetadata importedMetadata,
-            const QDateTime& metadataSynchronized);
+            const QDateTime& sourceSynchronizedAt);
 
     mixxx::TrackMetadata getMetadata(
-            bool* pMetadataSynchronized = nullptr) const;
+            bool* pHeaderParsed = nullptr) const;
 
     mixxx::TrackRecord getRecord(
             bool* pDirty = nullptr) const;