Improve synchronization of track metadata and file tags

src/library/coverartcache.cpp

@@ -5,6 +5,7 @@
 
 #include "library/coverartcache.h"
 #include "library/coverartutils.h"
+#include "util/compatibility.h"
 #include "util/logger.h"
 
 
@@ -201,25 +202,26 @@ void CoverArtCache::requestGuessCover(TrackPointer pTrack) {
 }
 
 void CoverArtCache::guessCover(TrackPointer pTrack) {
-    if (pTrack) {
-        if (kLogger.debugEnabled()) {
-            kLogger.debug()
-                    << "Guessing cover art for"
-                    << pTrack->getFileInfo();
-        }
-        CoverInfo cover = CoverArtUtils::guessCoverInfo(*pTrack);
-        pTrack->setCoverInfo(cover);
+    VERIFY_OR_DEBUG_ASSERT(pTrack) {
+        return;
+    }
+    if (kLogger.debugEnabled()) {
+        kLogger.debug()
+                << "Guessing cover art for"
+                << pTrack->getFileInfo();
     }
+    pTrack->setCoverInfo(
+            CoverArtUtils::guessCoverInfo(*pTrack));
 }
 
 void CoverArtCache::guessCovers(QList<TrackPointer> tracks) {
     if (kLogger.debugEnabled()) {
         kLogger.debug()
                 << "Guessing cover art for"
                 << tracks.size()
-                << "tracks";
+                << "track(s)";
     }
-    foreach (TrackPointer pTrack, tracks) {
+    for (TrackPointer pTrack : qAsConst(tracks)) {
         guessCover(pTrack);
     }
 }

src/library/coverartutils.cpp

@@ -80,32 +80,29 @@ QImage CoverArtUtils::loadCover(const CoverInfo& info) {
 }
 
 //static
-CoverInfo CoverArtUtils::guessCoverInfo(const Track& track) {
-    CoverInfo coverInfo;
-
-    coverInfo.trackLocation = track.getLocation();
-    coverInfo.source = CoverInfo::GUESSED;
-
+CoverInfoRelative CoverArtUtils::guessCoverInfo(
+        const Track& track,
+        QImage embeddedCover) {
     const auto trackFile = track.getFileInfo();
-    QImage image = extractEmbeddedCover(trackFile, track.getSecurityToken());
+
+    QImage image;
+    if (embeddedCover.isNull()) {
+        image = extractEmbeddedCover(trackFile, track.getSecurityToken());
+    } else {
+        image = std::move(embeddedCover);
+    }
     if (!image.isNull()) {
-        // TODO() here we my introduce a duplicate hash code
-        coverInfo.hash = calculateHash(image);
-        coverInfo.coverLocation = QString();
+        CoverInfoRelative coverInfo;
+        coverInfo.source = CoverInfo::GUESSED;
         coverInfo.type = CoverInfo::METADATA;
-        qDebug() << "CoverArtUtils::guessCover found metadata art" << coverInfo;
+        coverInfo.hash = calculateHash(image);
+        DEBUG_ASSERT(coverInfo.coverLocation.isNull());
         return coverInfo;
     }
 
-    QLinkedList<QFileInfo> possibleCovers = findPossibleCoversInFolder(
-            trackFile.directory());
-    coverInfo = selectCoverArtForTrack(track, possibleCovers);
-    if (coverInfo.type == CoverInfo::FILE) {
-        qDebug() << "CoverArtUtils::guessCover found file art" << coverInfo;
-    } else {
-        qDebug() << "CoverArtUtils::guessCover didn't find art" << coverInfo;
-    }
-    return coverInfo;
+    const QLinkedList<QFileInfo> possibleCovers =
+            findPossibleCoversInFolder(trackFile.directory());
+    return selectCoverArtForTrack(track, possibleCovers);
 }
 
 //static
@@ -130,13 +127,14 @@ QLinkedList<QFileInfo> CoverArtUtils::findPossibleCoversInFolder(const QString&
 }
 
 //static
-CoverInfo CoverArtUtils::selectCoverArtForTrack(
+CoverInfoRelative CoverArtUtils::selectCoverArtForTrack(
         const Track& track,
         const QLinkedList<QFileInfo>& covers) {
 
-    CoverInfoRelative coverInfoRelative =
-            selectCoverArtForTrack(track.getFileInfo(), track.getAlbum(), covers);
-    return CoverInfo(coverInfoRelative, track.getLocation());
+    return selectCoverArtForTrack(
+            track.getFileInfo(),
+            track.getAlbum(),
+            covers);
 }
 
 //static
@@ -145,6 +143,9 @@ CoverInfoRelative CoverArtUtils::selectCoverArtForTrack(
         const QString& albumName,
         const QLinkedList<QFileInfo>& covers) {
     CoverInfoRelative coverInfoRelative;
+    DEBUG_ASSERT(coverInfoRelative.type == CoverInfo::NONE);
+    DEBUG_ASSERT(coverInfoRelative.hash == 0);
+    DEBUG_ASSERT(coverInfoRelative.coverLocation.isNull());
     coverInfoRelative.source = CoverInfo::GUESSED;
     if (covers.isEmpty()) {
         return coverInfoRelative;
@@ -205,12 +206,10 @@ CoverInfoRelative CoverArtUtils::selectCoverArtForTrack(
     if (bestInfo != NULL) {
         QImage image(bestInfo->filePath());
         if (!image.isNull()) {
-            coverInfoRelative.source = CoverInfo::GUESSED;
             coverInfoRelative.type = CoverInfo::FILE;
             // TODO() here we may introduce a duplicate hash code
             coverInfoRelative.hash = calculateHash(image);
             coverInfoRelative.coverLocation = bestInfo->fileName();
-            return coverInfoRelative;
         }
     }
 

src/library/coverartutils.h

@@ -51,13 +51,19 @@ class CoverArtUtils {
     };
 
     // Guesses the cover art for the provided track.
-    static CoverInfo guessCoverInfo(const Track& track);
+    // If a non-null image is provided as an optional argument
+    // then this is assumed to contain the embedded cover art
+    // that has already been extracted from the track's file
+    // beforehand.
+    static CoverInfoRelative guessCoverInfo(
+            const Track& track,
+            QImage embeddedCover = QImage());
 
     static QLinkedList<QFileInfo> findPossibleCoversInFolder(
             const QString& folder);
 
     // Selects an appropriate cover file from provided list of image files.
-    static CoverInfo selectCoverArtForTrack(
+    static CoverInfoRelative selectCoverArtForTrack(
             const Track& track,
             const QLinkedList<QFileInfo>& covers);
 

src/library/dao/trackdao.cpp

@@ -1344,33 +1344,6 @@ TrackPointer TrackDAO::getTrackById(TrackId trackId) const {
         SoundSourceProxy(pTrack).updateTrackFromSource();
     }
 
-    // Data migration: Reload track total from file tags if not initialized
-    // yet. The added column "tracktotal" has been initialized with the
-    // default value "//".
-    // See also: Schema revision 26 in schema.xml
-    if (pTrack->getTrackTotal() == "//") {
-        // Reload track total from file tags if the special track
-        // total migration value "//" indicates that the track total
-        // is missing and needs to be reloaded.
-        qDebug() << "Reloading value for 'tracktotal' once-only from file"
-                " to replace the default value introduced with a previous"
-                " schema upgrade";
-        mixxx::TrackMetadata trackMetadata;
-        if (SoundSourceProxy(pTrack).importTrackMetadata(&trackMetadata) == mixxx::MetadataSource::ImportResult::Succeeded) {
-            // Copy the track total from the temporary track object
-            pTrack->setTrackTotal(trackMetadata.getTrackInfo().getTrackTotal());
-            // Also set the track number if it is still empty due
-            // to insufficient parsing capabilities of Mixxx in
-            // previous versions.
-            if (!trackMetadata.getTrackInfo().getTrackNumber().isEmpty() && pTrack->getTrackNumber().isEmpty()) {
-                pTrack->setTrackNumber(trackMetadata.getTrackInfo().getTrackNumber());
-            }
-        } else {
-            qWarning() << "Failed to reload value for 'tracktotal' from file tags:"
-                    << trackLocation;
-        }
-    }
-
     // Listen to dirty and changed signals
     connect(pTrack.get(),
             &Track::dirty,
@@ -1968,7 +1941,7 @@ void TrackDAO::detectCoverArtForTracksWithoutCover(volatile const bool* pCancel,
             // TODO() here we may introduce a duplicate hash code
             updateQuery.bindValue(":coverart_hash",
                                   CoverArtUtils::calculateHash(image));
-            updateQuery.bindValue(":coverart_location", "");
+            updateQuery.bindValue(":coverart_location", QString()); // NULL
             updateQuery.bindValue(":track_id", track.trackId.toVariant());
             if (!updateQuery.exec()) {
                 LOG_FAILED_QUERY(updateQuery) << "failed to write metadata cover";
@@ -1978,6 +1951,8 @@ void TrackDAO::detectCoverArtForTracksWithoutCover(volatile const bool* pCancel,
             continue;
         }
 
+        // This optimization is the reason for not using CoverArtUtils::guessCoverInfo()
+        // that needs to determine the possible cover for every single track.
         if (track.directoryPath != currentDirectoryPath) {
             possibleCovers.clear();
             currentDirectoryPath = track.directoryPath;
@@ -1988,6 +1963,7 @@ void TrackDAO::detectCoverArtForTracksWithoutCover(volatile const bool* pCancel,
 
         CoverInfoRelative coverInfo = CoverArtUtils::selectCoverArtForTrack(
             trackFile, track.trackAlbum, possibleCovers);
+        DEBUG_ASSERT(coverInfo.source != CoverInfo::UNKNOWN);
 
         updateQuery.bindValue(":coverart_type",
                               static_cast<int>(coverInfo.type));

src/library/dlgcoverartfullsize.cpp

@@ -166,18 +166,19 @@ void DlgCoverArtFullSize::slotCoverFound(const QObject* pRequestor,
 
 // slots to handle signals from the context menu
 void DlgCoverArtFullSize::slotReloadCoverArt() {
-    if (m_pLoadedTrack != nullptr) {
-        auto coverInfo =
-                CoverArtUtils::guessCoverInfo(*m_pLoadedTrack);
-        slotCoverInfoSelected(coverInfo);
+    if (!m_pLoadedTrack) {
+        return;
     }
+    slotCoverInfoSelected(
+            CoverArtUtils::guessCoverInfo(*m_pLoadedTrack));
 }
 
-void DlgCoverArtFullSize::slotCoverInfoSelected(const CoverInfoRelative& coverInfo) {
-    // qDebug() << "DlgCoverArtFullSize::slotCoverInfoSelected" << coverInfo;
-    if (m_pLoadedTrack != nullptr) {
-        m_pLoadedTrack->setCoverInfo(coverInfo);
+void DlgCoverArtFullSize::slotCoverInfoSelected(
+        const CoverInfoRelative& coverInfo) {
+    if (!m_pLoadedTrack) {
+        return;
     }
+    m_pLoadedTrack->setCoverInfo(coverInfo);
 }
 
 void DlgCoverArtFullSize::mousePressEvent(QMouseEvent* event) {

src/library/dlgtrackinfo.cpp

@@ -281,9 +281,7 @@ void DlgTrackInfo::slotReloadCoverArt() {
     VERIFY_OR_DEBUG_ASSERT(m_pLoadedTrack) {
         return;
     }
-    CoverInfo coverInfo =
-            CoverArtUtils::guessCoverInfo(*m_pLoadedTrack);
-    slotCoverInfoSelected(coverInfo);
+    slotCoverInfoSelected(CoverArtUtils::guessCoverInfo(*m_pLoadedTrack));
 }
 
 void DlgTrackInfo::slotCoverInfoSelected(const CoverInfoRelative& coverInfo) {

src/sources/metadatasourcetaglib.cpp

@@ -26,12 +26,12 @@ Logger kLogger("MetadataSourceTagLib");
 const bool kExportTrackMetadataIntoTemporaryFile = true;
 
 // Appended to the original file name of the temporary file used for writing
-const QString kSafelyWritableTempFileSuffix = "_temp";
+const QString kSafelyWritableTempFileSuffix = QStringLiteral("_temp");
 
 // Appended to the original file name for renaming and before deleting this
 // file. Should not be longer than kSafelyWritableTempFileSuffix to avoid
 // potential failures caused by exceeded path length.
-const QString kSafelyWritableOrigFileSuffix = "_orig";
+const QString kSafelyWritableOrigFileSuffix = QStringLiteral("_orig");
 
 // Workaround for missing functionality in TagLib 1.11.x that
 // doesn't support to read text chunks from AIFF files.