Fix the track order during drag and drop Lp1829601

src/library/analysisfeature.cpp

@@ -193,10 +193,8 @@ void AnalysisFeature::cleanupAnalyzer() {
 }
 
 bool AnalysisFeature::dropAccept(QList<QUrl> urls, QObject* pSource) {
-    Q_UNUSED(pSource);
-    QList<QFileInfo> files = DragAndDropHelper::supportedTracksFromUrls(urls, false, true);
-    // Adds track, does not insert duplicates, handles unremoving logic.
-    QList<TrackId> trackIds = m_pTrackCollection->getTrackDAO().addMultipleTracks(files, true);
+    QList<TrackId> trackIds = m_pTrackCollection->resolveTrackIdsFromUrls(urls,
+            !pSource);
     analyzeTracks(trackIds);
     return trackIds.size() > 0;
 }

src/library/autodj/autodjfeature.cpp

@@ -138,26 +138,18 @@ void AutoDJFeature::activate() {
 }
 
 bool AutoDJFeature::dropAccept(QList<QUrl> urls, QObject* pSource) {
-    // If a track is dropped onto a playlist's name, but the track isn't in the
+    // If a track is dropped onto the Auto DJ tree node, but the track isn't in the
     // library, then add the track to the library before adding it to the
-    // playlist.
-    QList<QFileInfo> files = DragAndDropHelper::supportedTracksFromUrls(urls, false, true);
-    QList<TrackId> trackIds;
-    if (pSource) {
-        trackIds = m_pTrackCollection->getTrackDAO().getTrackIds(files);
-        m_pTrackCollection->unhideTracks(trackIds);
-    } else {
-        trackIds = m_pTrackCollection->getTrackDAO().addMultipleTracks(files, true);
-    }
-
-    // remove tracks that could not be added
-    for (int trackIdIndex = 0; trackIdIndex < trackIds.size(); trackIdIndex++) {
-        if (!trackIds.at(trackIdIndex).isValid()) {
-            trackIds.removeAt(trackIdIndex--);
-        }
+    // Auto DJ playlist.
+    // pSource != nullptr it is a drop from inside Mixxx and indicates all
+    // tracks already in the DB
+    QList<TrackId> trackIds = m_pTrackCollection->resolveTrackIdsFromUrls(urls,
+            !pSource);
+    if (trackIds.isEmpty()) {
+        return false;
     }
 
-    // Return whether the tracks were appended.
+    // Return whether appendTracksToPlaylist succeeded.
     return m_playlistDao.appendTracksToPlaylist(trackIds, m_iAutoDJPlaylistId);
 }
 

src/library/crate/cratefeature.cpp

@@ -200,26 +200,20 @@ void updateTreeItemForTrackSelection(
 bool CrateFeature::dropAcceptChild(const QModelIndex& index, QList<QUrl> urls,
                                    QObject* pSource) {
     CrateId crateId(crateIdFromIndex(index));
-    if (!crateId.isValid()) {
+    VERIFY_OR_DEBUG_ASSERT(crateId.isValid()) {
         return false;
     }
-    QList<QFileInfo> files = DragAndDropHelper::supportedTracksFromUrls(urls, false, true);
-    QList<TrackId> trackIds;
-    if (pSource) {
-        trackIds = m_pTrackCollection->getTrackDAO().getTrackIds(files);
-        m_pTrackCollection->unhideTracks(trackIds);
-    } else {
-        // Adds track, does not insert duplicates, handles unremoving logic.
-        trackIds = m_pTrackCollection->getTrackDAO().addMultipleTracks(files, true);
-    }
-    qDebug() << "CrateFeature::dropAcceptChild adding tracks"
-            << trackIds.size() << " to crate "<< crateId;
-    // remove tracks that could not be added
-    for (int trackIdIndex = 0; trackIdIndex < trackIds.size(); ++trackIdIndex) {
-        if (!trackIds.at(trackIdIndex).isValid()) {
-            trackIds.removeAt(trackIdIndex--);
-        }
+    // If a track is dropped onto a crate's name, but the track isn't in the
+    // library, then add the track to the library before adding it to the
+    // playlist.
+    // pSource != nullptr it is a drop from inside Mixxx and indicates all
+    // tracks already in the DB
+    QList<TrackId> trackIds = m_pTrackCollection->resolveTrackIdsFromUrls(urls,
+            !pSource);
+    if (!trackIds.size()) {
+        return false;
     }
+
     m_pTrackCollection->addCrateTracks(crateId, trackIds);
     return true;
 }

src/library/crate/cratetablemodel.cpp

@@ -146,15 +146,8 @@ int CrateTableModel::addTracks(const QModelIndex& index,
     Q_UNUSED(index);
     // If a track is dropped but it isn't in the library, then add it because
     // the user probably dropped a file from outside Mixxx into this crate.
-    QList<QFileInfo> fileInfoList;
-    foreach(QString fileLocation, locations) {
-        QFileInfo fileInfo(fileLocation);
-        if (fileInfo.exists()) {
-            fileInfoList.append(fileInfo);
-        }
-    }
-
-    QList<TrackId> trackIds(m_pTrackCollection->getTrackDAO().addMultipleTracks(fileInfoList, true));
+    QList<TrackId> trackIds = m_pTrackCollection->resolveTrackIdsFromLocations(
+            locations);
     if (m_pTrackCollection->addCrateTracks(m_selectedCrate, trackIds)) {
         select();
         return trackIds.size();

src/library/dao/trackdao.cpp

@@ -135,22 +135,73 @@ TrackId TrackDAO::getTrackId(const QString& absoluteFilePath) {
     return trackId;
 }
 
-QList<TrackId> TrackDAO::getTrackIds(const QList<QFileInfo>& files) {
+QList<TrackId> TrackDAO::resolveTrackIds(const QList<QFileInfo>& files,
+        ResolveTrackIdFlags flags) {
     QList<TrackId> trackIds;
     trackIds.reserve(files.size());
 
+    // Create a temporary database of the paths of all the imported tracks.
     QSqlQuery query(m_database);
-    {
-        QStringList pathList;
-        pathList.reserve(files.size());
-        for (const auto& file: files) {
-            pathList << file.absoluteFilePath();
+    query.prepare(
+            "CREATE TEMP TABLE playlist_import "
+            "(location varchar (512))");
+    if (!query.exec()) {
+        LOG_FAILED_QUERY(query);
+        return trackIds;
+    }
+
+    QStringList pathList;
+    pathList.reserve(files.size());
+    for (const auto& file: files) {
+        pathList << "(" + SqlStringFormatter::format(m_database, file.absoluteFilePath()) + ")";
+    }
+
+    // Add all the track paths temporary to this database.
+    query.prepare(
+            "INSERT INTO playlist_import (location) "
+            "VALUES " + pathList.join(','));
+    if (!query.exec()) {
+        LOG_FAILED_QUERY(query);
+    }
+
+    if (flags & ResolveTrackIdFlag::AddMissing) {
+        // Prepare to add tracks to the database.
+        // This also begins an SQL transaction.
+        addTracksPrepare();
+
+        // Any tracks not already in the database need to be added.
+        query.prepare("SELECT location FROM playlist_import "
+                "WHERE NOT EXISTS (SELECT location FROM track_locations "
+                "WHERE playlist_import.location = track_locations.location)");
+        if (!query.exec()) {
+            LOG_FAILED_QUERY(query);
         }
-        query.prepare(QString("SELECT library.id FROM library INNER JOIN "
-                              "track_locations ON library.location = track_locations.id "
-                              "WHERE track_locations.location in (%1)").arg(
-                                      SqlStringFormatter::formatList(m_database, pathList)));
+        const int locationColumn = query.record().indexOf("location");
+        while (query.next()) {
+            QString location = query.value(locationColumn).toString();
+            const QFileInfo fileInfo(location);
+            addTracksAddFile(fileInfo, true);
+        }
+
+        // Finish adding tracks to the database.
+        addTracksFinish();
     }
+
+    query.prepare(
+            "SELECT library.id FROM playlist_import "
+            "INNER JOIN track_locations ON playlist_import.location = track_locations.location "
+            "INNER JOIN library ON library.location = track_locations.id "
+            // the order by clause enforces the native sorting which is used anyway
+            // hopefully optimized away. TODO() verify.
+            "ORDER BY playlist_import.ROWID");
+
+    // Old syntax for a shorter but less readable query. TODO() check performance gain
+    // query.prepare(
+    //    "SELECT library.id FROM playlist_import, "
+    //    "track_locations, library WHERE library.location = track_locations.id "
+    //    "AND playlist_import.location = track_locations.location");
+    //    "ORDER BY playlist_import.ROWID");
+
     if (query.exec()) {
         const int idColumn = query.record().indexOf("id");
         while (query.next()) {
@@ -164,6 +215,12 @@ QList<TrackId> TrackDAO::getTrackIds(const QList<QFileInfo>& files) {
         LOG_FAILED_QUERY(query);
     }
 
+    // Drop the temporary playlist-import table.
+    query.prepare("DROP TABLE IF EXISTS playlist_import");
+    if (!query.exec()) {
+        LOG_FAILED_QUERY(query);
+    }
+
     return trackIds;
 }
 
@@ -687,107 +744,7 @@ TrackPointer TrackDAO::addSingleTrack(const QFileInfo& fileInfo, bool unremove)
     return pTrack;
 }
 
-QList<TrackId> TrackDAO::addMultipleTracks(
-        const QList<QFileInfo>& fileInfoList,
-        bool unremove) {
-    // Prepare to add tracks to the database.
-    // This also begins an SQL transaction.
-    addTracksPrepare();
-
-    // Create a temporary database of the paths of all the imported tracks.
-    QSqlQuery query(m_database);
-    query.prepare("CREATE TEMP TABLE playlist_import "
-                  "(add_index INTEGER PRIMARY KEY, location varchar (512))");
-    if (!query.exec()) {
-        LOG_FAILED_QUERY(query);
-        addTracksFinish(true);
-        return QList<TrackId>();
-    }
-
-    // Add all the track paths to this database.
-    query.prepare("INSERT INTO playlist_import (add_index, location) "
-                  "VALUES (:add_index, :location)");
-    int index = 0;
-    for (const auto& fileInfo: fileInfoList) {
-        query.bindValue(":add_index", index);
-        query.bindValue(":location", fileInfo.absoluteFilePath());
-        if (!query.exec()) {
-            LOG_FAILED_QUERY(query);
-        }
-        index++;
-    }
-
-    // If imported-playlist tracks are to be unremoved, do that for all playlist
-    // tracks that were already in the database.
-    if (unremove) {
-        query.prepare("SELECT library.id FROM playlist_import, "
-                      "track_locations, library WHERE library.location = track_locations.id "
-                      "AND playlist_import.location = track_locations.location");
-        if (!query.exec()) {
-            LOG_FAILED_QUERY(query);
-        }
-
-        int idColumn = query.record().indexOf("id");
-        QStringList idStringList;
-        while (query.next()) {
-            TrackId trackId(query.value(idColumn));
-            idStringList.append(trackId.toString());
-        }
-
-        query.prepare(QString("UPDATE library SET mixxx_deleted=0 "
-                              "WHERE id in (%1) AND mixxx_deleted=1")
-                      .arg(idStringList.join(",")));
-        if (!query.exec()) {
-            LOG_FAILED_QUERY(query);
-        }
-    }
-
-    // Any tracks not already in the database need to be added.
-    query.prepare("SELECT add_index, location FROM playlist_import "
-                  "WHERE NOT EXISTS (SELECT location FROM track_locations "
-                  "WHERE playlist_import.location = track_locations.location)");
-    if (!query.exec()) {
-        LOG_FAILED_QUERY(query);
-    }
-    const int addIndexColumn = query.record().indexOf("add_index");
-    while (query.next()) {
-        int addIndex = query.value(addIndexColumn).toInt();
-        const QFileInfo fileInfo(fileInfoList.at(addIndex));
-        addTracksAddFile(fileInfo, unremove);
-    }
-
-    // Now that we have imported any tracks that were not already in the
-    // library, re-select ordering by playlist_import.add_index to return
-    // the list of track ids in the order that they were requested to be
-    // added.
-    QList<TrackId> trackIds;
-    query.prepare("SELECT library.id FROM playlist_import, "
-                  "track_locations, library WHERE library.location = track_locations.id "
-                  "AND playlist_import.location = track_locations.location "
-                  "ORDER BY playlist_import.add_index");
-    if (!query.exec()) {
-        LOG_FAILED_QUERY(query);
-    }
-    int idColumn = query.record().indexOf("id");
-    while (query.next()) {
-        TrackId trackId(query.value(idColumn));
-        trackIds.append(trackId);
-    }
-
-    // Drop the temporary playlist-import table.
-    query.prepare("DROP TABLE IF EXISTS playlist_import");
-    if (!query.exec()) {
-        LOG_FAILED_QUERY(query);
-    }
-
-    // Finish adding tracks to the database.
-    addTracksFinish();
-
-    // Return the list of track IDs added to the database.
-    return trackIds;
-}
-
-bool TrackDAO::onHidingTracks(
+bool TrackDAO::hideTracks(
         const QList<TrackId>& trackIds) {
     QStringList idList;
     for (const auto& trackId: trackIds) {
@@ -811,15 +768,16 @@ void TrackDAO::afterHidingTracks(
 // up in the library views again.
 // This function should get called if you drag-and-drop a file that's been
 // "hidden" from Mixxx back into the library view.
-bool TrackDAO::onUnhidingTracks(
+bool TrackDAO::unhideTracks(
         const QList<TrackId>& trackIds) {
     QStringList idList;
     for (const auto& trackId: trackIds) {
         idList.append(trackId.toString());
     }
-    FwdSqlQuery query(m_database, QString(
+    FwdSqlQuery query(m_database,
             "UPDATE library SET mixxx_deleted=0 "
-                  "WHERE id in (%1)").arg(idList.join(",")));
+            "WHERE mixxx_deleted!=0 "
+            "AND id in (" + idList.join(",") + ")");
     return !query.hasError() && query.execPrepared();
 }
 

src/library/dao/trackdao.h

@@ -24,6 +24,14 @@ class LibraryHashDAO;
 class TrackDAO : public QObject, public virtual DAO, public virtual GlobalTrackCacheRelocator {
     Q_OBJECT
   public:
+
+    enum class ResolveTrackIdFlag : int {
+        ResolveOnly = 0,
+        UnhideHidden = 1,
+        AddMissing = 2
+    };
+    Q_DECLARE_FLAGS(ResolveTrackIdFlags, ResolveTrackIdFlag)
+
     // The 'config object' is necessary because users decide ID3 tags get
     // synchronized on track metadata change
     TrackDAO(
@@ -40,7 +48,8 @@ class TrackDAO : public QObject, public virtual DAO, public virtual GlobalTrackC
     void finish();
 
     TrackId getTrackId(const QString& absoluteFilePath);
-    QList<TrackId> getTrackIds(const QList<QFileInfo>& files);
+    QList<TrackId> resolveTrackIds(const QList<QFileInfo> &files,
+            ResolveTrackIdFlags flags);
     QList<TrackId> getTrackIds(const QDir& dir);
 
     // WARNING: Only call this from the main thread instance of TrackDAO.
@@ -51,19 +60,17 @@ class TrackDAO : public QObject, public virtual DAO, public virtual GlobalTrackC
     QString getTrackLocation(TrackId trackId);
 
     TrackPointer addSingleTrack(const QFileInfo& fileInfo, bool unremove);
-    QList<TrackId> addMultipleTracks(const QList<QFileInfo>& fileInfoList, bool unremove);
-
     void addTracksPrepare();
     TrackPointer addTracksAddFile(const QFileInfo& fileInfo, bool unremove);
     TrackId addTracksAddTrack(const TrackPointer& pTrack, bool unremove);
     void addTracksFinish(bool rollback = false);
 
-    bool onHidingTracks(
+    bool hideTracks(
             const QList<TrackId>& trackIds);
     void afterHidingTracks(
             const QList<TrackId>& trackIds);
 
-    bool onUnhidingTracks(
+    bool unhideTracks(
             const QList<TrackId>& trackIds);
     void afterUnhidingTracks(
             const QList<TrackId>& trackIds);
@@ -159,4 +166,7 @@ class TrackDAO : public QObject, public virtual DAO, public virtual GlobalTrackC
     DISALLOW_COPY_AND_ASSIGN(TrackDAO);
 };
 
+
+Q_DECLARE_OPERATORS_FOR_FLAGS(TrackDAO::ResolveTrackIdFlags)
+
 #endif //TRACKDAO_H

src/library/librarytablemodel.cpp

@@ -61,11 +61,8 @@ void LibraryTableModel::setTableModel(int id) {
 int LibraryTableModel::addTracks(const QModelIndex& index,
                                  const QList<QString>& locations) {
     Q_UNUSED(index);
-    QList<QFileInfo> fileInfoList;
-    foreach (QString fileLocation, locations) {
-        fileInfoList.append(QFileInfo(fileLocation));
-    }
-    QList<TrackId> trackIds = m_pTrackCollection->getTrackDAO().addMultipleTracks(fileInfoList, true);
+    QList<TrackId> trackIds = m_pTrackCollection->resolveTrackIdsFromLocations(
+            locations);
     select();
     return trackIds.size();
 }