Merge pull request mixxxdj#13719 from daschuer/gh11128_2

Fix scrolling issue with coverart columns visible

Author: acolombier

src/library/dao/trackdao.cpp

@@ -54,8 +54,6 @@ class TrackDAO : public QObject, public virtual DAO, public virtual GlobalTrackC
             const QList<mixxx::FileInfo>& fileInfos,
             ResolveTrackIdFlags flags = ResolveTrackIdFlag::ResolveOnly);
 
-    TrackId getTrackIdByRef(
-            const TrackRef& trackRef) const;
     QList<TrackRef> getAllTrackRefs(
             const QDir& rootDir) const;
 
@@ -154,16 +152,9 @@ class TrackDAO : public QObject, public virtual DAO, public virtual GlobalTrackC
     TrackId addTracksAddTrack(
             const TrackPointer& pTrack,
             bool unremove);
-    TrackPointer addTracksAddFile(
-            const mixxx::FileAccess& fileAccess,
-            bool unremove);
     TrackPointer addTracksAddFile(
             const QString& filePath,
-            bool unremove) {
-        return addTracksAddFile(
-                mixxx::FileAccess(mixxx::FileInfo(filePath)),
-                unremove);
-    }
+            bool unremove);
     void addTracksFinish(bool rollback = false);
 
     bool updateTrack(const Track& track) const;

src/library/dao/trackdao.h

@@ -567,36 +567,30 @@ void LibraryScanner::slotTrackExists(const QString& trackPath) {
 }
 
 void LibraryScanner::slotAddNewTrack(const QString& trackPath) {
-    //kLogger.debug() << "slotAddNewTrack" << trackPath;
+    // kLogger.debug() << "slotAddNewTrack" << trackPath;
     ScopedTimer timer(u"LibraryScanner::addNewTrack");
     // For statistics tracking and to detect moved tracks
     TrackPointer pTrack = m_trackDao.addTracksAddFile(
             trackPath,
             false);
-    if (pTrack) {
-        DEBUG_ASSERT(!pTrack->isDirty());
-        // The track's actual location might differ from the
-        // given trackPath
-        const QString trackLocation(pTrack->getLocation());
-        // Acknowledge successful track addition
-        if (m_scannerGlobal) {
-            m_scannerGlobal->trackAdded(trackLocation);
-        }
-        // Signal the main instance of TrackDAO, that there is
-        // a new track in the database.
-        emit trackAdded(pTrack);
-        emit progressLoading(trackLocation);
-    } else {
-        // Acknowledge failed track addition
-        // TODO(XXX): Is it really intended to acknowledge a failed
-        // track addition with a trackAdded() signal??
-        if (m_scannerGlobal) {
-            m_scannerGlobal->trackAdded(trackPath);
-        }
-        kLogger.warning()
-                << "Failed to add track to library:"
-                << trackPath;
+    if (!pTrack) {
+        // This happens only when there is an issue with the database which
+        // has been logged already. No need for yet another warning here.
+        return;
+    }
+
+    DEBUG_ASSERT(!pTrack->isDirty());
+    // The track's actual location might differ from the
+    // given trackPath
+    const QString trackLocation = pTrack->getLocation();
+    // Acknowledge successful track addition
+    if (m_scannerGlobal) {
+        m_scannerGlobal->trackAdded(trackLocation);
     }
+    // Signal the main instance of TrackDAO, that there is
+    // a new track in the database.
+    emit trackAdded(pTrack);
+    emit progressLoading(trackLocation);
 }
 
 bool LibraryScanner::changeScannerState(ScannerState newState) {

src/library/scanner/libraryscanner.cpp

@@ -573,11 +573,6 @@ TrackPointer TrackCollection::getTrackByRef(
     return m_trackDao.getTrackByRef(trackRef);
 }
 
-TrackId TrackCollection::getTrackIdByRef(
-        const TrackRef& trackRef) const {
-    return m_trackDao.getTrackIdByRef(trackRef);
-}
-
 TrackPointer TrackCollection::getOrAddTrack(
         const TrackRef& trackRef,
         bool* pAlreadyInLibrary) {

src/library/trackcollection.cpp

@@ -85,9 +85,6 @@ class TrackCollection : public QObject,
 
     bool updateAutoDjCrate(CrateId crateId, bool isAutoDjSource);
 
-    TrackId getTrackIdByRef(
-            const TrackRef& trackRef) const;
-
   signals:
     // Forwarded signals from LibraryScanner
     void scanTrackAdded(TrackPointer pTrack);

src/library/trackcollection.h

@@ -285,7 +285,7 @@ BaseTrackPlayerImpl::~BaseTrackPlayerImpl() {
 }
 
 TrackPointer BaseTrackPlayerImpl::loadFakeTrack(bool bPlay, double filebpm) {
-    TrackPointer pTrack(Track::newTemporary());
+    TrackPointer pTrack = Track::newTemporary();
     pTrack->setAudioProperties(
             mixxx::kEngineChannelCount,
             mixxx::audio::SampleRate(44100),

src/mixer/basetrackplayer.cpp

@@ -544,7 +544,7 @@ importTrackMetadataAndCoverImageUnavailable() {
 //static
 std::pair<mixxx::MetadataSource::ImportResult, QDateTime>
 SoundSourceProxy::importTrackMetadataAndCoverImageFromFile(
-        mixxx::FileAccess trackFileAccess,
+        const mixxx::FileAccess& trackFileAccess,
         mixxx::TrackMetadata* pTrackMetadata,
         QImage* pCoverImage,
         bool resetMissingTagMetadata) {
@@ -553,24 +553,25 @@ SoundSourceProxy::importTrackMetadataAndCoverImageFromFile(
         // https://github.com/mixxxdj/mixxx/issues/9944
         return importTrackMetadataAndCoverImageUnavailable();
     }
-    TrackPointer pTrack;
-    // Lock the global track cache while accessing the file to ensure
-    // that no metadata is written. Since locking individual files
-    // is not possible the whole cache has to be locked.
-    GlobalTrackCacheLocker locker;
-    pTrack = locker.lookupTrackByRef(TrackRef::fromFileInfo(trackFileAccess.info()));
-    if (pTrack) {
-        // We can safely unlock the cache if the track object is already cached.
-        locker.unlockCache();
-    } else {
-        // If the track object is not cached we need to keep the cache
-        // locked and create a temporary track object instead.
-        pTrack = Track::newTemporary(std::move(trackFileAccess));
+
+    // Since the Track object below is only used to read the meta data from
+    // the file, we use a temporary track that is discarded in an incomplete
+    // state without being saved to the database or the track file's meta data
+    // after the meta data import has been completed.
+    // Lock the track via the global track cache while accessing the file to
+    // ensure that no metadata is written by another thread.
+    { // cacheResolver scope
+        const bool temporary = true;
+        auto cacheResolver = GlobalTrackCacheResolver(trackFileAccess, temporary);
+        TrackPointer pTrack = cacheResolver.getTrack();
+        // In the unlikely case that the file has disappeart since the check above
+        // pTrack is null and the next call will log appropriate warnings and
+        // returns importTrackMetadataAndCoverImageUnavailable();
+        return SoundSourceProxy(pTrack).importTrackMetadataAndCoverImage(
+                pTrackMetadata,
+                pCoverImage,
+                resetMissingTagMetadata);
     }
-    return SoundSourceProxy(pTrack).importTrackMetadataAndCoverImage(
-            pTrackMetadata,
-            pCoverImage,
-            resetMissingTagMetadata);
 }
 
 std::pair<mixxx::MetadataSource::ImportResult, QDateTime>

src/sources/soundsourceproxy.cpp

@@ -102,7 +102,7 @@ class SoundSourceProxy {
     /// while reading.
     static std::pair<mixxx::MetadataSource::ImportResult, QDateTime>
     importTrackMetadataAndCoverImageFromFile(
-            mixxx::FileAccess trackFileAccess,
+            const mixxx::FileAccess& trackFileAccess,
             mixxx::TrackMetadata* pTrackMetadata,
             QImage* pCoverImage,
             bool resetMissingTagMetadata);

src/sources/soundsourceproxy.h

@@ -93,41 +93,43 @@ TEST_F(GlobalTrackCacheTest, resolveByFileInfo) {
 
     const TrackId trackId(QVariant(1));
 
-    TrackPointer track;
-    {
+    TrackPointer pTrack;
+    { // resolver scope
         auto testFileAccess = mixxx::FileAccess(mixxx::FileInfo(getTestDir().filePath(kTestFile)));
-        GlobalTrackCacheResolver resolver(testFileAccess);
-        track = resolver.getTrack();
-        EXPECT_TRUE(static_cast<bool>(track));
-        EXPECT_EQ(2, track.use_count());
+        auto resolver = GlobalTrackCacheResolver(testFileAccess);
+        pTrack = resolver.getTrack();
+        EXPECT_TRUE(static_cast<bool>(pTrack));
+        // track, GlobalTrackCacheResolver::m_strongPtr and GlobalTrackCache::m_incompleteTrack
+        EXPECT_EQ(3, pTrack.use_count());
 
         resolver.initTrackIdAndUnlockCache(trackId);
+        EXPECT_EQ(2, pTrack.use_count());
     }
-    EXPECT_EQ(1, track.use_count());
+    EXPECT_EQ(1, pTrack.use_count());
 
-    TrackWeakPointer trackWeak(track);
+    TrackWeakPointer trackWeak(pTrack);
     EXPECT_EQ(1, trackWeak.use_count());
 
-    TrackPointer trackCopy = track;
+    TrackPointer trackCopy = pTrack;
     EXPECT_EQ(2, trackCopy.use_count());
-    EXPECT_EQ(2, track.use_count());
+    EXPECT_EQ(2, pTrack.use_count());
     EXPECT_EQ(2, trackWeak.use_count());
 
     trackCopy.reset();
-    EXPECT_EQ(1, track.use_count());
+    EXPECT_EQ(1, pTrack.use_count());
     EXPECT_EQ(1, trackWeak.use_count());
 
     auto trackById = GlobalTrackCacheLocker().lookupTrackById(trackId);
-    EXPECT_EQ(track, trackById);
+    EXPECT_EQ(pTrack, trackById);
     EXPECT_EQ(2, trackById.use_count());
-    EXPECT_EQ(2, track.use_count());
+    EXPECT_EQ(2, pTrack.use_count());
     EXPECT_EQ(2, trackWeak.use_count());
 
     trackById.reset();
     EXPECT_EQ(1, trackWeak.use_count());
-    EXPECT_EQ(track, TrackPointer(trackWeak.lock()));
+    EXPECT_EQ(pTrack, TrackPointer(trackWeak.lock()));
 
-    track.reset();
+    pTrack.reset();
     EXPECT_EQ(0, trackWeak.use_count());
     EXPECT_EQ(TrackPointer(), TrackPointer(trackWeak.lock()));
 
@@ -169,7 +171,7 @@ TEST_F(GlobalTrackCacheTest, concurrentDelete) {
         TrackPointer track;
         {
             auto testFileAccess = mixxx::FileAccess(testFile);
-            GlobalTrackCacheResolver resolver(testFileAccess);
+            auto resolver = GlobalTrackCacheResolver(testFileAccess);
             track = resolver.getTrack();
             EXPECT_TRUE(static_cast<bool>(track));
             trackId = track->getId();