From fbd70f2205d60e85a99630095dde7c535131c53b Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 24 Apr 2018 22:03:40 +0200 Subject: [PATCH 1/6] Fix integration of external track libraries Add a missing override to BaseExternalTrackModel. I wonder if there are more of these "gems" hidden inside the code?! This was a tough one, even though it looks like an easy fix. Implementation inheritance is evil!! --- src/library/baseexternaltrackmodel.cpp | 33 +++++++++++++++++++++++---------- src/library/baseexternaltrackmodel.h | 2 +- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/src/library/baseexternaltrackmodel.cpp b/src/library/baseexternaltrackmodel.cpp index 0713790eff..7f4c00a241 100644 --- a/src/library/baseexternaltrackmodel.cpp +++ b/src/library/baseexternaltrackmodel.cpp @@ -58,20 +58,33 @@ TrackPointer BaseExternalTrackModel::getTrack(const QModelIndex& index) const { TrackPointer pTrack = m_pTrackCollection->getTrackDAO() .getOrAddTrack(location, true, &track_already_in_library); - // If this track was not in the Mixxx library it is now added and will be - // saved with the metadata from iTunes. If it was already in the library - // then we do not touch it so that we do not over-write the user's metadata. - if (pTrack && !track_already_in_library) { - pTrack->setArtist(artist); - pTrack->setTitle(title); - pTrack->setAlbum(album); - pTrack->setYear(year); - pTrack->setGenre(genre); - pTrack->setBpm(bpm); + if (pTrack) { + // If this track was not in the Mixxx library it is now added and will be + // saved with the metadata from iTunes. If it was already in the library + // then we do not touch it so that we do not over-write the user's metadata. + if (!track_already_in_library) { + pTrack->setArtist(artist); + pTrack->setTitle(title); + pTrack->setAlbum(album); + pTrack->setYear(year); + pTrack->setGenre(genre); + pTrack->setBpm(bpm); + } + } else { + qWarning() << "Failed to load external track" << location; } return pTrack; } +TrackId BaseExternalTrackModel::getTrackId(const QModelIndex& index) const { + const auto track = getTrack(index); + if (track) { + return track->getId(); + } else { + return TrackId(); + } +} + void BaseExternalTrackModel::trackLoaded(QString group, TrackPointer pTrack) { if (group == m_previewDeckGroup) { // If there was a previously loaded track, refresh its rows so the diff --git a/src/library/baseexternaltrackmodel.h b/src/library/baseexternaltrackmodel.h index eb4900a63a..7558f960e6 100644 --- a/src/library/baseexternaltrackmodel.h +++ b/src/library/baseexternaltrackmodel.h @@ -7,7 +7,6 @@ #include "library/trackmodel.h" #include "library/basesqltablemodel.h" -#include "track/track.h" class TrackCollection; @@ -22,6 +21,7 @@ class BaseExternalTrackModel : public BaseSqlTableModel { ~BaseExternalTrackModel() override; CapabilitiesFlags getCapabilities() const override; + TrackId getTrackId(const QModelIndex& index) const override; TrackPointer getTrack(const QModelIndex& index) const override; void trackLoaded(QString group, TrackPointer pTrack) override; bool isColumnInternal(int column) override; From 0f635b6ca904b0bde0afa86ddc54fd73cde0ec58 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 24 Apr 2018 22:36:02 +0200 Subject: [PATCH 2/6] Reword comment --- src/library/baseexternaltrackmodel.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/library/baseexternaltrackmodel.cpp b/src/library/baseexternaltrackmodel.cpp index 7f4c00a241..53a66f0c51 100644 --- a/src/library/baseexternaltrackmodel.cpp +++ b/src/library/baseexternaltrackmodel.cpp @@ -60,8 +60,9 @@ TrackPointer BaseExternalTrackModel::getTrack(const QModelIndex& index) const { if (pTrack) { // If this track was not in the Mixxx library it is now added and will be - // saved with the metadata from iTunes. If it was already in the library - // then we do not touch it so that we do not over-write the user's metadata. + // saved with the metadata from external library. If it was already in the + // Mixxx library then we do not touch it so that we do not over-write the + // user's metadata. if (!track_already_in_library) { pTrack->setArtist(artist); pTrack->setTitle(title); From 1dc6401178d912ee9bdbab2fa1cae735b3f4ad16 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 24 Apr 2018 15:31:44 +0200 Subject: [PATCH 3/6] Delete unused member function --- src/library/dao/trackdao.cpp | 8 -------- src/library/dao/trackdao.h | 2 -- 2 files changed, 10 deletions(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 4ecadc0eb5..5316da2e40 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -179,14 +179,6 @@ QString TrackDAO::getTrackLocation(TrackId trackId) { return trackLocation; } -/** Check if a track exists in the library table already. - @param file_location The full path to the track on disk, including the filename. - @return true if the track is found in the library table, false otherwise. -*/ -bool TrackDAO::trackExistsInDatabase(const QString& absoluteFilePath) { - return getTrackId(absoluteFilePath).isValid(); -} - void TrackDAO::saveTrack(Track* pTrack) { DEBUG_ASSERT(pTrack); if (pTrack->isDirty()) { diff --git a/src/library/dao/trackdao.h b/src/library/dao/trackdao.h index 7c33fe269c..9a911f889e 100644 --- a/src/library/dao/trackdao.h +++ b/src/library/dao/trackdao.h @@ -43,8 +43,6 @@ class TrackDAO : public QObject, public virtual DAO, public virtual GlobalTrackC QList getTrackIds(const QList& files); QList getTrackIds(const QDir& dir); - bool trackExistsInDatabase(const QString& absoluteFilePath); - // WARNING: Only call this from the main thread instance of TrackDAO. TrackPointer getTrack(TrackId trackId) const; From d6b7e12d72516152d7f01758331304ba075f2eeb Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 24 Apr 2018 15:38:56 +0200 Subject: [PATCH 4/6] Hide internal utility function --- src/library/dao/trackdao.cpp | 33 +++++++++++++++++++-------------- src/library/dao/trackdao.h | 1 - 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 5316da2e40..161eed8c28 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -36,8 +36,26 @@ #include "util/timer.h" #include "util/math.h" + +namespace { + enum { UndefinedRecordIndex = -2 }; +void markTrackLocationsAsDeleted(QSqlDatabase database, const QString& directory) { + //qDebug() << "TrackDAO::markTrackLocationsAsDeleted" << QThread::currentThread() << m_database.connectionName(); + QSqlQuery query(database); + query.prepare("UPDATE track_locations " + "SET fs_deleted=1 " + "WHERE directory=:directory"); + query.bindValue(":directory", directory); + if (!query.exec()) { + LOG_FAILED_QUERY(query) + << "Couldn't mark tracks in" << directory << "as deleted."; + } +} + +} // anonymous namespace + TrackDAO::TrackDAO(CueDAO& cueDao, PlaylistDAO& playlistDao, AnalysisDao& analysisDao, @@ -84,7 +102,7 @@ void TrackDAO::finish() { // directories as deleted. // TODO(XXX) This doesn't handle sub-directories of deleted directories. for (const auto& dir: deletedHashDirs) { - markTrackLocationsAsDeleted(dir); + markTrackLocationsAsDeleted(m_database, dir); } transaction.commit(); } @@ -1502,19 +1520,6 @@ void TrackDAO::markUnverifiedTracksAsDeleted() { } } -void TrackDAO::markTrackLocationsAsDeleted(const QString& directory) { - //qDebug() << "TrackDAO::markTrackLocationsAsDeleted" << QThread::currentThread() << m_database.connectionName(); - QSqlQuery query(m_database); - query.prepare("UPDATE track_locations " - "SET fs_deleted=1 " - "WHERE directory=:directory"); - query.bindValue(":directory", directory); - if (!query.exec()) { - LOG_FAILED_QUERY(query) - << "Couldn't mark tracks in" << directory << "as deleted."; - } -} - // Look for moved files. Look for files that have been marked as // "deleted on disk" and see if another "file" with the same name and // files size exists in the track_locations table. That means the file has diff --git a/src/library/dao/trackdao.h b/src/library/dao/trackdao.h index 9a911f889e..f3ce490cca 100644 --- a/src/library/dao/trackdao.h +++ b/src/library/dao/trackdao.h @@ -90,7 +90,6 @@ class TrackDAO : public QObject, public virtual DAO, public virtual GlobalTrackC void markTracksInDirectoriesAsVerified(const QStringList& directories); void invalidateTrackLocationsInLibrary(); void markUnverifiedTracksAsDeleted(); - void markTrackLocationsAsDeleted(const QString& directory); bool detectMovedTracks(QSet* pTracksMovedSetOld, QSet* pTracksMovedSetNew, const QStringList& addedTracks, From e7ee1e507f9010a869c60e7e706094e6458fd5d7 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 25 Apr 2018 00:56:19 +0200 Subject: [PATCH 5/6] Improve error handling and logging when loading/adding tracks --- src/library/dao/trackdao.cpp | 50 +++++++++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 21 deletions(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 161eed8c28..06f1cae3cf 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -121,6 +121,8 @@ TrackId TrackDAO::getTrackId(const QString& absoluteFilePath) { if (query.exec()) { if (query.next()) { trackId = TrackId(query.value(query.record().indexOf("id"))); + } else { + qDebug() << "TrackDAO::getTrackId(): Track location not found in library:" << absoluteFilePath; } } else { LOG_FAILED_QUERY(query); @@ -151,6 +153,10 @@ QList TrackDAO::getTrackIds(const QList& files) { while (query.next()) { trackIds.append(TrackId(query.value(idColumn))); } + DEBUG_ASSERT(trackIds.size() <= files.size()); + if (trackIds.size() < files.size()) { + qDebug() << "TrackDAO::getTrackIds(): Found only" << trackIds.size() << "of" << files.size() << "tracks in library"; + } } else { LOG_FAILED_QUERY(query); } @@ -1904,36 +1910,38 @@ void TrackDAO::detectCoverArtForTracksWithoutCover(volatile const bool* pCancel, TrackPointer TrackDAO::getOrAddTrack(const QString& trackLocation, bool processCoverArt, bool* pAlreadyInLibrary) { - const TrackId trackId(getTrackId(trackLocation)); + const TrackId trackId = getTrackId(trackLocation); const bool trackAlreadyInLibrary = trackId.isValid(); + if (pAlreadyInLibrary) { + *pAlreadyInLibrary = trackAlreadyInLibrary; + } TrackPointer pTrack; if (trackAlreadyInLibrary) { pTrack = getTrack(trackId); + if (!pTrack) { + qWarning() << "Failed to load track" + << trackLocation; + return pTrack; + } } else { // Add Track to library -- unremove if it was previously removed. pTrack = addSingleTrack(trackLocation, true); - } - - // addTrack or getTrack may fail. - if (!pTrack) { - qWarning() << "Failed to load track" - << trackLocation; - return pTrack; - } - - // If the track wasn't in the library already then it has not yet been - // checked for cover art. If processCoverArt is true then we should request - // cover processing via CoverArtCache asynchronously. - if (processCoverArt && !trackAlreadyInLibrary) { - CoverArtCache* pCache = CoverArtCache::instance(); - if (pCache != nullptr) { - pCache->requestGuessCover(pTrack); + if (!pTrack) { + qWarning() << "Failed to add track" + << trackLocation; + return pTrack; + } + DEBUG_ASSERT(pTrack); + // If the track wasn't in the library already then it has not yet been + // checked for cover art. If processCoverArt is true then we should request + // cover processing via CoverArtCache asynchronously. + if (processCoverArt) { + CoverArtCache* pCache = CoverArtCache::instance(); + if (pCache != nullptr) { + pCache->requestGuessCover(pTrack); + } } - } - - if (pAlreadyInLibrary != nullptr) { - *pAlreadyInLibrary = trackAlreadyInLibrary; } return pTrack; From 5f8659db2c1ecb1661577ea16c9c316bbaf670ff Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 26 Apr 2018 00:26:32 +0200 Subject: [PATCH 6/6] Extenals libraries: Convert native to canonical location --- src/library/baseexternalplaylistmodel.cpp | 6 ++++-- src/library/baseexternaltrackmodel.cpp | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/library/baseexternalplaylistmodel.cpp b/src/library/baseexternalplaylistmodel.cpp index 731c742368..7529523896 100644 --- a/src/library/baseexternalplaylistmodel.cpp +++ b/src/library/baseexternalplaylistmodel.cpp @@ -21,8 +21,9 @@ BaseExternalPlaylistModel::~BaseExternalPlaylistModel() { } TrackPointer BaseExternalPlaylistModel::getTrack(const QModelIndex& index) const { - QString location = index.sibling( + QString nativeLocation = index.sibling( index.row(), fieldIndex("location")).data().toString(); + QString location = QDir::fromNativeSeparators(nativeLocation); if (location.isEmpty()) { // Track is lost @@ -150,7 +151,8 @@ void BaseExternalPlaylistModel::trackLoaded(QString group, TrackPointer pTrack) // The external table has foreign Track IDs, so we need to compare // by location for (int row = 0; row < rowCount(); ++row) { - QString location = index(row, fieldIndex("location")).data().toString(); + QString nativeLocation = index(row, fieldIndex("location")).data().toString(); + QString location = QDir::fromNativeSeparators(nativeLocation); if (location == pTrack->getLocation()) { m_previewDeckTrackId = TrackId(index(row, 0).data()); //Debug() << "foreign track id" << m_previewDeckTrackId; diff --git a/src/library/baseexternaltrackmodel.cpp b/src/library/baseexternaltrackmodel.cpp index 53a66f0c51..3dfd3df083 100644 --- a/src/library/baseexternaltrackmodel.cpp +++ b/src/library/baseexternaltrackmodel.cpp @@ -47,7 +47,8 @@ TrackPointer BaseExternalTrackModel::getTrack(const QModelIndex& index) const { QString genre = index.sibling(index.row(), fieldIndex("genre")).data().toString(); float bpm = index.sibling(index.row(), fieldIndex("bpm")).data().toString().toFloat(); - QString location = index.sibling(index.row(), fieldIndex("location")).data().toString(); + QString nativeLocation = index.sibling(index.row(), fieldIndex("location")).data().toString(); + QString location = QDir::fromNativeSeparators(nativeLocation); if (location.isEmpty()) { // Track is lost @@ -104,7 +105,8 @@ void BaseExternalTrackModel::trackLoaded(QString group, TrackPointer pTrack) { // The external table has foreign Track IDs, so we need to compare // by location for (int row = 0; row < rowCount(); ++row) { - QString location = index(row, fieldIndex("location")).data().toString(); + QString nativeLocation = index(row, fieldIndex("location")).data().toString(); + QString location = QDir::fromNativeSeparators(nativeLocation); if (location == pTrack->getLocation()) { m_previewDeckTrackId = TrackId(index(row, 0).data()); //qDebug() << "foreign track id" << m_previewDeckTrackId;