Playlists: keep correct track selection (position) when sorting

Author: ronso0

src/library/baseexternalplaylistmodel.cpp

@@ -26,6 +26,7 @@ BaseExternalPlaylistModel::BaseExternalPlaylistModel(QObject* parent,
           m_playlistTracksTable(playlistTracksTable),
           m_trackSource(trackSource),
           m_currentPlaylistId(kInvalidPlaylistId) {
+    setPositionColumn(PLAYLISTTRACKSTABLE_POSITION);
 }
 
 BaseExternalPlaylistModel::~BaseExternalPlaylistModel() {

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 (!m_positionColumn.isEmpty()) {
+        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,6 +253,7 @@ void BaseSqlTableModel::select() {
     QVector<RowInfo> rowInfos;
     QSet<TrackId> trackIds;
     int idColumn = -1;
+    int posColumn = -1;
     while (query.next()) {
         QSqlRecord sqlRecord = query.record();
 
@@ -257,7 +265,7 @@ void BaseSqlTableModel::select() {
         // 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;
@@ -271,6 +279,18 @@ void BaseSqlTableModel::select() {
         rowInfo.trackId = trackId;
         // current position defines the ordering
         rowInfo.order = rowInfos.size();
+
+        if (posColumn == -1 && !m_positionColumn.isEmpty()) {
+            posColumn = sqlRecord.indexOf(m_positionColumn);
+        }
+        if (posColumn != -1) {
+            bool ok = false;
+            int pos = sqlRecord.value(posColumn).toInt(&ok);
+            if (ok) {
+                rowInfo.position = pos;
+            }
+        }
+
         rowInfo.metadata.reserve(sqlRecord.count());
         for (int i = 0; i < m_tableColumns.size(); ++i) {
             rowInfo.metadata.push_back(sqlRecord.value(i));
@@ -330,10 +350,24 @@ void BaseSqlTableModel::select() {
     // number of total rows returned by the query
     DEBUG_ASSERT(trackIdToRows.size() <= rowInfos.size());
 
+    TrackPos2Row trackPosToRows;
+    if (posColumn != -1) {
+        // 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.position, i);
+        }
+    }
+    if (!m_positionColumn.isEmpty()) {
+        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!
 

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;
@@ -92,6 +95,9 @@ class BaseSqlTableModel : public BaseTrackTableModel {
             QString trackIdColumn,
             QStringList tableColumns,
             QSharedPointer<BaseTrackCache> trackSource);
+    void setPositionColumn(QString posColName) {
+        m_positionColumn = std::move(posColName);
+    }
     void initHeaderProperties() override;
     virtual void initSortColumnMapping();
 
@@ -106,6 +112,7 @@ class BaseSqlTableModel : public BaseTrackTableModel {
     QString m_tableOrderBy;
     int m_columnIndexBySortColumnId[static_cast<int>(TrackModel::SortColumnId::IdMax)];
     QMap<int, TrackModel::SortColumnId> m_sortColumnIdByColumnIndex;
+    QString m_positionColumn;
 
   private slots:
     void tracksChanged(const QSet<TrackId>& trackIds);
@@ -122,6 +129,7 @@ class BaseSqlTableModel : public BaseTrackTableModel {
     struct RowInfo {
         TrackId trackId;
         int order;
+        int position; // used by playlist models only
         QVector<QVariant> metadata;
 
         bool operator<(const RowInfo& other) const {
@@ -136,11 +144,13 @@ class BaseSqlTableModel : public BaseTrackTableModel {
     };
 
     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;
 
@@ -151,6 +161,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/playlisttablemodel.cpp

@@ -24,6 +24,7 @@ PlaylistTableModel::PlaylistTableModel(QObject* parent,
             &PlaylistDAO::tracksChanged,
             this,
             &PlaylistTableModel::playlistsChanged);
+    setPositionColumn(PLAYLISTTRACKSTABLE_POSITION);
 }
 
 void PlaylistTableModel::initSortColumnMapping() {
@@ -323,6 +324,20 @@ 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;
+    // TODO Transpose m_trackPosToRow ?? Would it be faster?
+    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

@@ -35,6 +35,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/widget/wtracktableview.cpp

@@ -1429,7 +1429,19 @@ void WTrackTableView::doSortByColumn(int headerSection, Qt::SortOrder sortOrder)
     }
 
     // Save the selection
-    const QList<TrackId> selectedTrackIds = getSelectedTrackIds();
+    // If this is a track model that may contain a track multiple times (playlist),
+    // we store the positions so we can reselect only the current selection after sorting,
+    // not all occurrences of selected tracks.
+    QList<TrackId> selectedTrackIds;
+    QList<int> selectedTrackPositions;
+    bool usePositions = pTrackModel->hasCapabilities(TrackModel::Capability::Reorder);
+    if (usePositions) {
+        const QModelIndexList indices = getSelectedRows();
+        selectedTrackPositions = pTrackModel->getSelectedPositions(indices);
+    } else {
+        selectedTrackIds = getSelectedTrackIds();
+    }
+
     int savedHScrollBarPos = horizontalScrollBar()->value();
     // Save the column of focused table cell.
     // The cell is not necessarily part of the selection, but even if it's
@@ -1442,13 +1454,56 @@ void WTrackTableView::doSortByColumn(int headerSection, Qt::SortOrder sortOrder)
 
     sortByColumn(headerSection, sortOrder);
 
-    selectTracksById(selectedTrackIds, prevColumn);
+    if (usePositions) {
+        selectTracksByPosition(selectedTrackPositions, prevColumn);
+    } else {
+        selectTracksById(selectedTrackIds, prevColumn);
+    }
 
     // This seems to be broken since at least Qt 5.12: no scrolling is issued
     // scrollTo(first, QAbstractItemView::EnsureVisible);
     horizontalScrollBar()->setValue(savedHScrollBarPos);
 }
 
+void WTrackTableView::selectTracksByPosition(const QList<int>& positions, int prevColumn) {
+    if (positions.isEmpty()) {
+        return;
+    }
+    TrackModel* pTrackModel = getTrackModel();
+    QItemSelectionModel* pSelectionModel = selectionModel();
+    pSelectionModel->reset(); // remove current selection
+
+    // Find previously selected tracks and store respective rows for reselection.
+    QList<int> rows;
+    for (int pos : positions) {
+        rows.append(pTrackModel->getTrackRowByPosition(pos));
+    }
+
+    // Select the first row of the previous selection.
+    // This scrolls to that row and with the leftmost cell being focused we have
+    // a starting point (currentIndex) for navigation with Up/Down keys.
+    // Replaces broken scrollTo() (see comment below)
+    if (!rows.isEmpty()) {
+        selectRow(rows.first());
+    }
+
+    // Refocus the cell in the column that was focused before sorting.
+    // With this, any Up/Down key press moves the selection and keeps the
+    // horizontal scrollbar position we will restore below.
+    QModelIndex restoreIndex = model()->index(currentIndex().row(), prevColumn);
+    if (restoreIndex.isValid()) {
+        setCurrentIndex(restoreIndex);
+    }
+
+    // Restore previous selection (doesn't affect focused cell).
+    for (int row : rows) {
+        pSelectionModel->select(model()->index(row, prevColumn),
+                QItemSelectionModel::Select | QItemSelectionModel::Rows);
+    }
+}
+
+// Don't use this on playlists since they may contain a TrackId multiple times.
+// See doSortByColumn.
 void WTrackTableView::selectTracksById(const QList<TrackId>& trackIds, int prevColum) {
     TrackModel* pTrackModel = getTrackModel();
     QAbstractItemModel* pItemModel = model();
@@ -1459,14 +1514,6 @@ void WTrackTableView::selectTracksById(const QList<TrackId>& trackIds, int prevC
     // Find previously selected tracks and store respective rows for reselection.
     QMap<int, int> selectedRows;
     for (const auto& trackId : trackIds) {
-        // TODO(rryan) slowly fixing the issues with BaseSqlTableModel. This
-        // code is broken for playlists because it assumes each trackid is in
-        // the table once. This will erroneously select all instances of the
-        // track for playlists, but it works fine for every other view. The way
-        // to fix this that we should do is to delegate the selection saving to
-        // the TrackModel. This will allow the playlist table model to use the
-        // table index as the unique id instead of this code stupidly using
-        // trackid.
         const auto rows = pTrackModel->getTrackRows(trackId);
         for (int row : rows) {
             // Restore sort order by rows, so the following commands will act as expected

src/widget/wtracktableview.h

@@ -55,6 +55,7 @@ class WTrackTableView : public WLibraryTableView {
     void moveRows(QList<int> selectedRows, int destRow);
     void moveSelectedTracks(QKeyEvent* event);
     void selectTracksById(const QList<TrackId>& tracks, int prevColumn);
+    void selectTracksByPosition(const QList<int>& positions, int prevColum);
 
     double getBackgroundColorOpacity() const {
         return m_backgroundColorOpacity;