(fix) Playlists: keep correct track selection (# position) when sorting

src/library/autodj/autodjfeature.cpp

@@ -5,6 +5,7 @@
 
 #include "library/autodj/autodjprocessor.h"
 #include "library/autodj/dlgautodj.h"
+#include "library/dao/trackschema.h"
 #include "library/library.h"
 #include "library/parser.h"
 #include "library/trackcollection.h"

src/library/banshee/bansheeplaylistmodel.cpp

@@ -4,6 +4,7 @@
 
 #include "library/banshee/bansheedbconnection.h"
 #include "library/dao/playlistdao.h"
+#include "library/dao/trackschema.h"
 #include "library/queryutil.h"
 #include "library/trackcollectionmanager.h"
 #include "mixer/playermanager.h"

src/library/baseexternalplaylistmodel.cpp

@@ -136,7 +136,7 @@ void BaseExternalPlaylistModel::setPlaylistById(int playlistId) {
     // The ordering of columns is relevant (see below)!
     auto playlistViewColumns = QStringList{
             QStringLiteral("track_id"),
-            QStringLiteral("position"),
+            PLAYLISTTRACKSTABLE_POSITION,
             QStringLiteral("'' AS ") + LIBRARYTABLE_PREVIEW};
     const auto queryString =
             QStringLiteral(

src/library/basesqltablemodel.cpp

@@ -164,15 +164,18 @@ void BaseSqlTableModel::clearRows() {
         beginRemoveRows(QModelIndex(), 0, m_rowInfo.size() - 1);
         m_rowInfo.clear();
         m_trackIdToRows.clear();
+        m_trackPosToRow.clear();
         endRemoveRows();
     }
     DEBUG_ASSERT(m_rowInfo.isEmpty());
     DEBUG_ASSERT(m_trackIdToRows.isEmpty());
+    DEBUG_ASSERT(m_trackPosToRow.isEmpty());
 }
 
 void BaseSqlTableModel::replaceRows(
         QVector<RowInfo>&& rows,
-        TrackId2Rows&& trackIdToRows) {
+        TrackId2Rows&& trackIdToRows,
+        TrackPos2Row&& trackPosToRows) {
     // NOTE(uklotzde): Use r-value references for parameters here, because
     // conceptually those parameters should replace the corresponding internal
     // member variables. Currently Qt4/5 doesn't support move semantics and
@@ -182,12 +185,16 @@ void BaseSqlTableModel::replaceRows(
     // its container types in the future this code becomes even more efficient.
     DEBUG_ASSERT(rows.empty() == trackIdToRows.empty());
     DEBUG_ASSERT(rows.size() >= trackIdToRows.size());
+    if (hasPositionColumn()) {
+        DEBUG_ASSERT(rows.size() == trackPosToRows.size());
+    }
     if (rows.isEmpty()) {
         clearRows();
     } else {
         beginInsertRows(QModelIndex(), 0, rows.size() - 1);
         m_rowInfo = rows;
         m_trackIdToRows = trackIdToRows;
+        m_trackPosToRow = trackPosToRows;
         endInsertRows();
     }
 }
@@ -246,18 +253,23 @@ void BaseSqlTableModel::select() {
     QVector<RowInfo> rowInfos;
     QSet<TrackId> trackIds;
     int idColumn = -1;
+    int posColumn = -1;
     while (query.next()) {
         QSqlRecord sqlRecord = query.record();
 
         if (idColumn < 0) {
             idColumn = sqlRecord.indexOf(m_idColumn);
         }
 
+        if (posColumn == -1 && hasPositionColumn()) {
+            posColumn = sqlRecord.indexOf(PLAYLISTTABLE_POSITION);
+        }
+
         // TODO(XXX): Can we get rid of the hard-coded assumption that
         // the the first column always contains the id?
         DEBUG_ASSERT(idColumn == kIdColumn);
 
-        VERIFY_OR_DEBUG_ASSERT(idColumn >= 0) {
+        VERIFY_OR_DEBUG_ASSERT(idColumn != -1) {
             qCritical()
                     << "ID column not available in database query results:"
                     << m_idColumn;
@@ -269,11 +281,11 @@ void BaseSqlTableModel::select() {
 
         RowInfo rowInfo;
         rowInfo.trackId = trackId;
-        // current position defines the ordering
-        rowInfo.order = rowInfos.size();
-        rowInfo.metadata.reserve(sqlRecord.count());
+        rowInfo.row = rowInfos.size();
+
+        rowInfo.columnValues.reserve(sqlRecord.count());
         for (int i = 0; i < m_tableColumns.size(); ++i) {
-            rowInfo.metadata.push_back(sqlRecord.value(i));
+            rowInfo.columnValues.push_back(sqlRecord.value(i));
         }
         rowInfos.push_back(rowInfo);
     }
@@ -298,9 +310,9 @@ void BaseSqlTableModel::select() {
             // separate removed tracks (order == -1) from present tracks (order ==
             // 0). Otherwise we sort by the order that filterAndSort returned to us.
             if (m_trackSourceOrderBy.isEmpty()) {
-                rowInfo.order = m_trackSortOrder.contains(rowInfo.trackId) ? 0 : -1;
+                rowInfo.row = m_trackSortOrder.contains(rowInfo.trackId) ? 0 : -1;
             } else {
-                rowInfo.order = m_trackSortOrder.value(rowInfo.trackId, -1);
+                rowInfo.row = m_trackSortOrder.value(rowInfo.trackId, -1);
             }
         }
     }
@@ -317,8 +329,7 @@ void BaseSqlTableModel::select() {
     trackIdToRows.reserve(rowInfos.size());
     for (int i = 0; i < rowInfos.size(); ++i) {
         const RowInfo& rowInfo = rowInfos[i];
-
-        if (rowInfo.order == -1) {
+        if (rowInfo.row == -1) {
             // We've reached the end of valid rows. Resize rowInfo to cut off
             // this and all further elements.
             rowInfos.resize(i);
@@ -330,10 +341,22 @@ void BaseSqlTableModel::select() {
     // number of total rows returned by the query
     DEBUG_ASSERT(trackIdToRows.size() <= rowInfos.size());
 
+    TrackPos2Row trackPosToRows;
+    if (hasPositionColumn()) {
+        // We expect as many positions as we have rows
+        trackPosToRows.reserve(rowInfos.size());
+        for (int i = 0; i < rowInfos.size(); ++i) {
+            const RowInfo& rowInfo = rowInfos[i];
+            trackPosToRows.insert(rowInfo.getPosition(posColumn), i);
+        }
+        DEBUG_ASSERT(trackPosToRows.size() == rowInfos.size());
+    }
+
     // We're done! Issue the update signals and replace the main maps.
     replaceRows(
             std::move(rowInfos),
-            std::move(trackIdToRows));
+            std::move(trackIdToRows),
+            std::move(trackPosToRows));
     // Both rowInfo and trackIdToRows (might) have been moved and
     // must not be used afterwards!
 
@@ -651,13 +674,13 @@ QVariant BaseSqlTableModel::rawValue(
             return previewDeckTrackId() == trackId;
         }
 
-        const QVector<QVariant>& columns = rowInfo.metadata;
+        const QVector<QVariant>& columnValues = rowInfo.columnValues;
         if (sDebug) {
             qDebug() << "Returning table-column value"
-                    << columns.at(column)
-                    << "for column" << column;
+                     << columnValues.at(column)
+                     << "for column" << column;
         }
-        return columns[column];
+        return columnValues[column];
     }
 
     // Otherwise, return the information from the track record cache for the

src/library/basesqltablemodel.h

@@ -56,6 +56,9 @@ class BaseSqlTableModel : public BaseTrackTableModel {
     const QVector<int> getTrackRows(TrackId trackId) const override {
         return m_trackIdToRows.value(trackId);
     }
+    int getTrackRowByPosition(int position) const override {
+        return m_trackPosToRow.value(position);
+    }
 
     void search(const QString& searchText, const QString& extraFilter = QString()) override;
     const QString currentSearch() const override;
@@ -99,6 +102,10 @@ class BaseSqlTableModel : public BaseTrackTableModel {
 
     QList<TrackRef> getTrackRefs(const QModelIndexList& indices) const;
 
+    bool hasPositionColumn() {
+        return fieldIndex(ColumnCache::COLUMN_PLAYLISTTRACKSTABLE_POSITION) >= 0;
+    }
+
     QSqlDatabase m_database;
     QString m_tableName;
 
@@ -120,26 +127,40 @@ class BaseSqlTableModel : public BaseTrackTableModel {
 
     struct RowInfo {
         TrackId trackId;
-        int order;
-        QVector<QVariant> metadata;
+        int row;
+        QVector<QVariant> columnValues;
+
+        int getPosition(int posCol) const {
+            if (posCol < 0) {
+                return -1;
+            }
+            bool ok = false;
+            int pos = columnValues.at(posCol).toInt(&ok);
+            if (ok) {
+                return pos;
+            }
+            return -1;
+        }
 
         bool operator<(const RowInfo& other) const {
             // -1 is greater than anything
-            if (order == -1) {
+            if (row == -1) {
                 return false;
-            } else if (other.order == -1) {
+            } else if (other.row == -1) {
                 return true;
             }
-            return order < other.order;
+            return row < other.row;
         }
     };
 
     typedef QHash<TrackId, QVector<int>> TrackId2Rows;
+    typedef QHash<int, int> TrackPos2Row;
 
     void clearRows();
     void replaceRows(
             QVector<RowInfo>&& rows,
-            TrackId2Rows&& trackIdToRows);
+            TrackId2Rows&& trackIdToRows,
+            TrackPos2Row&& trackPosToRows);
 
     QVector<RowInfo> m_rowInfo;
 
@@ -150,6 +171,7 @@ class BaseSqlTableModel : public BaseTrackTableModel {
     bool m_bInitialized;
     QHash<TrackId, int> m_trackSortOrder;
     TrackId2Rows m_trackIdToRows;
+    TrackPos2Row m_trackPosToRow;
     QString m_currentSearch;
     QString m_currentSearchFilter;
     QVector<QHash<int, QVariant>> m_headerInfo;

src/library/dao/playlistdao.cpp

@@ -4,6 +4,7 @@
 #include <QtDebug>
 
 #include "library/autodj/autodjprocessor.h"
+#include "library/dao/trackschema.h"
 #include "library/queryutil.h"
 #include "moc_playlistdao.cpp"
 #include "util/db/dbconnection.h"

src/library/dao/playlistdao.h

@@ -8,23 +8,6 @@
 #include "track/trackid.h"
 #include "util/class.h"
 
-#define PLAYLIST_TABLE "Playlists"
-#define PLAYLIST_TRACKS_TABLE "PlaylistTracks"
-
-const QString PLAYLISTTABLE_ID = QStringLiteral("id");
-const QString PLAYLISTTABLE_NAME = QStringLiteral("name");
-const QString PLAYLISTTABLE_POSITION = QStringLiteral("position");
-const QString PLAYLISTTABLE_HIDDEN = QStringLiteral("hidden");
-const QString PLAYLISTTABLE_DATECREATED = QStringLiteral("date_created");
-const QString PLAYLISTTABLE_DATEMODIFIED = QStringLiteral("date_modified");
-
-const QString PLAYLISTTRACKSTABLE_TRACKID = QStringLiteral("track_id");
-const QString PLAYLISTTRACKSTABLE_POSITION = QStringLiteral("position");
-const QString PLAYLISTTRACKSTABLE_PLAYLISTID = QStringLiteral("playlist_id");
-const QString PLAYLISTTRACKSTABLE_DATETIMEADDED = QStringLiteral("pl_datetime_added");
-
-#define AUTODJ_TABLE "Auto DJ"
-
 class AutoDJProcessor;
 class QSqlDatabase;
 

src/library/dao/trackschema.h

@@ -5,6 +5,11 @@
 #define LIBRARY_TABLE "library"
 #define TRACKLOCATIONS_TABLE "track_locations"
 
+#define PLAYLIST_TABLE "Playlists"
+#define PLAYLIST_TRACKS_TABLE "PlaylistTracks"
+
+#define AUTODJ_TABLE "Auto DJ"
+
 const QString LIBRARYTABLE_ID = QStringLiteral("id");
 const QString LIBRARYTABLE_ARTIST = QStringLiteral("artist");
 const QString LIBRARYTABLE_TITLE = QStringLiteral("title");
@@ -56,6 +61,18 @@ const QString TRACKLOCATIONSTABLE_FILESIZE = QStringLiteral("filesize");
 const QString TRACKLOCATIONSTABLE_FSDELETED = QStringLiteral("fs_deleted");
 const QString TRACKLOCATIONSTABLE_NEEDSVERIFICATION = QStringLiteral("needs_verification");
 
+const QString PLAYLISTTABLE_ID = QStringLiteral("id");
+const QString PLAYLISTTABLE_NAME = QStringLiteral("name");
+const QString PLAYLISTTABLE_POSITION = QStringLiteral("position");
+const QString PLAYLISTTABLE_HIDDEN = QStringLiteral("hidden");
+const QString PLAYLISTTABLE_DATECREATED = QStringLiteral("date_created");
+const QString PLAYLISTTABLE_DATEMODIFIED = QStringLiteral("date_modified");
+
+const QString PLAYLISTTRACKSTABLE_TRACKID = QStringLiteral("track_id");
+const QString PLAYLISTTRACKSTABLE_POSITION = QStringLiteral("position");
+const QString PLAYLISTTRACKSTABLE_PLAYLISTID = QStringLiteral("playlist_id");
+const QString PLAYLISTTRACKSTABLE_DATETIMEADDED = QStringLiteral("pl_datetime_added");
+
 const QString REKORDBOX_ANALYZE_PATH = "analyze_path";
 
 namespace mixxx {

src/library/playlisttablemodel.cpp

@@ -335,6 +335,19 @@ void PlaylistTableModel::shuffleTracks(const QModelIndexList& shuffle, const QMo
     m_pTrackCollectionManager->internalCollection()->getPlaylistDAO().shuffleTracks(m_iPlaylistId, positions, allIds);
 }
 
+const QList<int> PlaylistTableModel::getSelectedPositions(const QModelIndexList& indices) const {
+    if (indices.isEmpty()) {
+        return {};
+    }
+    QList<int> positions;
+    const int positionColumn = fieldIndex(ColumnCache::COLUMN_PLAYLISTTRACKSTABLE_POSITION);
+    for (auto idx : indices) {
+        int pos = idx.siblingAtColumn(positionColumn).data().toInt();
+        positions.append(pos);
+    }
+    return positions;
+}
+
 mixxx::Duration PlaylistTableModel::getTotalDuration(const QModelIndexList& indices) {
     if (indices.isEmpty()) {
         return mixxx::Duration::empty();

src/library/playlisttablemodel.h

@@ -33,6 +33,7 @@ class PlaylistTableModel final : public TrackSetTableModel {
 
     /// Get the total duration of all tracks referenced by the given model indices
     mixxx::Duration getTotalDuration(const QModelIndexList& indices);
+    const QList<int> getSelectedPositions(const QModelIndexList& indices) const override;
 
     Capabilities getCapabilities() const final;
 

src/library/trackmodel.h

@@ -128,6 +128,15 @@ class TrackModel {
     // Gets the rows of the track in the current result set. Returns an
     // empty list if the track ID is not present in the result set.
     virtual const QVector<int> getTrackRows(TrackId trackId) const = 0;
+    virtual int getTrackRowByPosition(int position) const {
+        Q_UNUSED(position);
+        return -1;
+    }
+
+    virtual const QList<int> getSelectedPositions(const QModelIndexList& indices) const {
+        Q_UNUSED(indices);
+        return {};
+    }
 
     virtual void search(const QString& searchText, const QString& extraFilter=QString()) = 0;
     virtual const QString currentSearch() const = 0;

src/test/autodjprocessor_test.cpp

@@ -10,6 +10,7 @@
 #include "control/controlpotmeter.h"
 #include "control/controlpushbutton.h"
 #include "engine/engine.h"
+#include "library/dao/trackschema.h"
 #include "library/playlisttablemodel.h"
 #include "mixer/basetrackplayer.h"
 #include "mixer/playerinfo.h"