Merge pull request #11230 from daschuer/gh10608

Refectoring that aims to fix Rekordbox crash on macOS #10608

src/library/autodj/autodjfeature.cpp

@@ -216,11 +216,12 @@ void AutoDJFeature::slotCrateChanged(CrateId crateId) {
         }
         // No child item for crate found
         // -> Create and append a new child item for this crate
-        QList<TreeItem*> rows;
-        rows.append(new TreeItem(crate.getName(), crate.getId().toVariant()));
+        // TODO() Use here std::span to get around the heap alloctaion of
+        // std::vector for a single element.
+        std::vector<std::unique_ptr<TreeItem>> rows;
+        rows.push_back(std::make_unique<TreeItem>(crate.getName(), crate.getId().toVariant()));
         QModelIndex parentIndex = m_childModel.index(0, 0);
-        m_childModel.insertTreeItemRows(rows, m_crateList.length(), parentIndex);
-        DEBUG_ASSERT(rows.isEmpty()); // ownership passed to m_childModel
+        m_childModel.insertTreeItemRows(std::move(rows), m_crateList.length(), parentIndex);
         m_crateList.append(crate);
     } else {
         // Crate does not exist or is not a source for AutoDJ

src/library/banshee/bansheeplaylistmodel.cpp

@@ -34,11 +34,34 @@
 #define CLM_PLAYCOUNT "timesplayed"
 #define CLM_COMPOSER "composer"
 #define CLM_PREVIEW "preview"
+#define CLM_CRATE "crate"
 
 namespace {
 
 QAtomicInt sTableNumber;
 
+const QString kTrackId = QStringLiteral(CLM_TRACK_ID);
+const QString kViewOrder = QStringLiteral(CLM_VIEW_ORDER);
+const QString kArtist = QStringLiteral(CLM_ARTIST);
+const QString kTitle = QStringLiteral(CLM_TITLE);
+const QString kDuration = QStringLiteral(CLM_DURATION);
+const QString kUri = QStringLiteral(CLM_URI);
+const QString kAlbum = QStringLiteral(CLM_ALBUM);
+const QString kAlbumArtist = QStringLiteral(CLM_ALBUM_ARTIST);
+const QString kYear = QStringLiteral(CLM_YEAR);
+const QString kRating = QStringLiteral(CLM_RATING);
+const QString kGenre = QStringLiteral(CLM_GENRE);
+const QString kGrouping = QStringLiteral(CLM_GROUPING);
+const QString kTracknumber = QStringLiteral(CLM_TRACKNUMBER);
+const QString kDateadded = QStringLiteral(CLM_DATEADDED);
+const QString kBpm = QStringLiteral(CLM_BPM);
+const QString kBitrate = QStringLiteral(CLM_BITRATE);
+const QString kComment = QStringLiteral(CLM_COMMENT);
+const QString kPlaycount = QStringLiteral(CLM_PLAYCOUNT);
+const QString kComposer = QStringLiteral(CLM_COMPOSER);
+const QString kPreview = QStringLiteral(CLM_PREVIEW);
+const QString kCrate = QStringLiteral(CLM_CRATE);
+
 } // namespace
 
 BansheePlaylistModel::BansheePlaylistModel(QObject* pParent, TrackCollectionManager* pTrackCollectionManager, BansheeDbConnection* pConnection)
@@ -187,38 +210,51 @@ void BansheePlaylistModel::setTableModel(int playlistId) {
         }
     }
 
-    QStringList tableColumns;
-    tableColumns
-         << CLM_TRACK_ID // 0
-         << CLM_VIEW_ORDER
-         << CLM_PREVIEW; // 3
-
-    QStringList trackSourceColumns;
-    trackSourceColumns
-         << CLM_TRACK_ID // 0
-         << CLM_ARTIST
-         << CLM_TITLE
-         << CLM_DURATION
-         << CLM_URI
-         << CLM_ALBUM
-         << CLM_ALBUM_ARTIST
-         << CLM_YEAR
-         << CLM_RATING
-         << CLM_GENRE
-         << CLM_GROUPING
-         << CLM_TRACKNUMBER
-         << CLM_DATEADDED
-         << CLM_BPM
-         << CLM_BITRATE
-         << CLM_COMMENT
-         << CLM_PLAYCOUNT
-         << CLM_COMPOSER;
-
-    QSharedPointer<BaseTrackCache> trackSource(
-            new BaseTrackCache(m_pTrackCollectionManager->internalCollection(), m_tempTableName, CLM_TRACK_ID,
-                    trackSourceColumns, false));
-
-    setTable(m_tempTableName, CLM_TRACK_ID, tableColumns, trackSource);
+    const QString idColumn = kTrackId;
+
+    QStringList tableColumns = {
+            kTrackId,
+            kViewOrder,
+            kPreview};
+
+    QStringList trackSourceColumns = {
+            kTrackId,
+            kArtist,
+            kTitle,
+            kDuration,
+            kUri,
+            kAlbum,
+            kAlbumArtist,
+            kYear,
+            kRating,
+            kGenre,
+            kGrouping,
+            kTracknumber,
+            kDateadded,
+            kBpm,
+            kBitrate,
+            kComment,
+            kPlaycount,
+            kComposer};
+    QStringList searchColumns = {
+            kArtist,
+            kAlbum,
+            kAlbumArtist,
+            kUri,
+            kGrouping,
+            kComment,
+            kTitle,
+            kGenre};
+
+    auto trackSource = QSharedPointer<BaseTrackCache>::create(
+            m_pTrackCollectionManager->internalCollection(),
+            m_tempTableName,
+            idColumn,
+            std::move(trackSourceColumns),
+            std::move(searchColumns),
+            false);
+
+    setTable(m_tempTableName, idColumn, std::move(tableColumns), trackSource);
     setSearch("");
     setDefaultSort(fieldIndex(PLAYLISTTRACKSTABLE_POSITION), Qt::AscendingOrder);
     setSort(defaultSortColumn(), defaultSortOrder());

src/library/baseexternallibraryfeature.cpp

@@ -58,7 +58,8 @@ void BaseExternalLibraryFeature::onRightClick(const QPoint& globalPos) {
 
 void BaseExternalLibraryFeature::onRightClickChild(
         const QPoint& globalPos, const QModelIndex& index) {
-    //Save the model index so we can get it in the action slots...
+    // Save the model index so we can get it in the action slots...
+    // Make sure that this is reset when the related TreeItem is deleted.
     m_lastRightClickedIndex = index;
     QMenu menu(m_pSidebarWidget);
     menu.addAction(m_pAddToAutoDJAction);
@@ -132,8 +133,8 @@ void BaseExternalLibraryFeature::appendTrackIdsFromRightClickIndex(
 
     DEBUG_ASSERT(pPlaylist);
     *pPlaylist = m_lastRightClickedIndex.data().toString();
-    QScopedPointer<BaseSqlTableModel> pPlaylistModelToAdd(
-            getPlaylistModelForPlaylist(*pPlaylist));
+    std::unique_ptr<BaseSqlTableModel> pPlaylistModelToAdd(
+            createPlaylistModelForPlaylist(*pPlaylist));
 
     if (!pPlaylistModelToAdd || !pPlaylistModelToAdd->initialized()) {
         qDebug() << "BaseExternalLibraryFeature::appendTrackIdsFromRightClickIndex "

src/library/baseexternallibraryfeature.h

@@ -3,9 +3,10 @@
 #include <QAction>
 #include <QModelIndex>
 #include <QPointer>
+#include <memory>
 
-#include "library/libraryfeature.h"
 #include "library/dao/playlistdao.h"
+#include "library/libraryfeature.h"
 #include "util/parented_ptr.h"
 
 class BaseSqlTableModel;
@@ -26,9 +27,10 @@ class BaseExternalLibraryFeature : public LibraryFeature {
 
   protected:
     // Must be implemented by external Libraries copied to Mixxx DB
-    virtual BaseSqlTableModel* getPlaylistModelForPlaylist(const QString& playlist) {
+    virtual std::unique_ptr<BaseSqlTableModel> createPlaylistModelForPlaylist(
+            const QString& playlist) {
         Q_UNUSED(playlist);
-        return nullptr;
+        return {};
     }
     // Must be implemented by external Libraries not copied to Mixxx DB
     virtual void appendTrackIdsFromRightClickIndex(QList<TrackId>* trackIds, QString* pPlaylist);
@@ -43,12 +45,17 @@ class BaseExternalLibraryFeature : public LibraryFeature {
     QModelIndex lastRightClickedIndex() const {
         return m_lastRightClickedIndex;
     }
+    void clearLastRightClickedIndex() {
+        m_lastRightClickedIndex = QModelIndex();
+    };
 
     TrackCollection* const m_pTrackCollection;
 
   private:
     void addToAutoDJ(PlaylistDAO::AutoDJSendLoc loc);
 
+    // Caution: Make sure this is reset whenever the library tree is updated,
+    // so that the internalPointer() does not become dangling
     QModelIndex m_lastRightClickedIndex;
 
     parented_ptr<QAction> m_pAddToAutoDJAction;

src/library/baseplaylistfeature.cpp

@@ -618,7 +618,7 @@ void BasePlaylistFeature::htmlLinkClicked(const QUrl& link) {
   * This method queries the database and does dynamic insertion
 */
 QModelIndex BasePlaylistFeature::constructChildModel(int selected_id) {
-    QList<TreeItem*> data_list;
+    std::vector<std::unique_ptr<TreeItem>> data_list;
     int selected_row = -1;
 
     int row = 0;
@@ -633,17 +633,17 @@ QModelIndex BasePlaylistFeature::constructChildModel(int selected_id) {
         }
 
         // Create the TreeItem whose parent is the invisible root item
-        TreeItem* item = new TreeItem(playlistLabel, playlistId);
-        item->setBold(m_playlistsSelectedTrackIsIn.contains(playlistId));
+        auto pItem = std::make_unique<TreeItem>(playlistLabel, playlistId);
+        pItem->setBold(m_playlistsSelectedTrackIsIn.contains(playlistId));
 
-        decorateChild(item, playlistId);
-        data_list.append(item);
+        decorateChild(pItem.get(), playlistId);
+        data_list.push_back(std::move(pItem));
 
         ++row;
     }
 
     // Append all the newly created TreeItems in a dynamic way to the childmodel
-    m_childModel.insertTreeItemRows(data_list, 0);
+    m_childModel.insertTreeItemRows(std::move(data_list), 0);
     if (selected_row == -1) {
         return QModelIndex();
     }
@@ -671,6 +671,7 @@ void BasePlaylistFeature::updateChildModel(int playlistId) {
   * Clears the child model dynamically, but the invisible root item remains
   */
 void BasePlaylistFeature::clearChildModel() {
+    m_lastRightClickedIndex = QModelIndex();
     m_childModel.removeRows(0, m_childModel.rowCount());
 }
 

src/library/baseplaylistfeature.h

@@ -70,6 +70,8 @@ class BasePlaylistFeature : public BaseTrackSetFeature {
     virtual void clearChildModel();
     virtual QList<IdAndLabel> createPlaylistLabels() = 0;
     virtual QString fetchPlaylistLabel(int playlistId) = 0;
+
+    /// borrows pChild which must not be null, TODO: use gsl::not_null
     virtual void decorateChild(TreeItem* pChild, int playlistId) = 0;
     virtual void addToAutoDJ(PlaylistDAO::AutoDJSendLoc loc);
 

src/library/basesqltablemodel.cpp

@@ -334,16 +334,16 @@ void BaseSqlTableModel::select() {
              << m_rowInfo.size();
 }
 
-void BaseSqlTableModel::setTable(const QString& tableName,
-        const QString& idColumn,
-        const QStringList& tableColumns,
+void BaseSqlTableModel::setTable(QString tableName,
+        QString idColumn,
+        QStringList tableColumns,
         QSharedPointer<BaseTrackCache> trackSource) {
     if (sDebug) {
         qDebug() << this << "setTable" << tableName << tableColumns << idColumn;
     }
-    m_tableName = tableName;
-    m_idColumn = idColumn;
-    m_tableColumns = tableColumns;
+    m_tableName = std::move(tableName);
+    m_idColumn = std::move(idColumn);
+    m_tableColumns = std::move(tableColumns);
 
     if (m_trackSource) {
         disconnect(m_trackSource.data(),

src/library/basesqltablemodel.h

@@ -84,9 +84,10 @@ class BaseSqlTableModel : public BaseTrackTableModel {
             const QVariant& value,
             int role) final;
 
-    void setTable(const QString& tableName, const QString& trackIdColumn,
-                  const QStringList& tableColumns,
-                  QSharedPointer<BaseTrackCache> trackSource);
+    void setTable(QString tableName,
+            QString trackIdColumn,
+            QStringList tableColumns,
+            QSharedPointer<BaseTrackCache> trackSource);
     void initHeaderProperties() override;
     virtual void initSortColumnMapping();
 

src/library/basetrackcache.cpp

@@ -17,35 +17,21 @@ constexpr bool sDebug = false;
 }  // namespace
 
 BaseTrackCache::BaseTrackCache(TrackCollection* pTrackCollection,
-                               const QString& tableName,
-                               const QString& idColumn,
-                               const QStringList& columns,
-                               bool isCaching)
-        : m_tableName(tableName),
-          m_idColumn(idColumn),
+        QString tableName,
+        QString idColumn,
+        QStringList columns,
+        QStringList searchColumns,
+        bool isCaching)
+        : m_tableName(std::move(tableName)),
+          m_idColumn(std::move(idColumn)),
           m_columnCount(columns.size()),
           m_columnsJoined(columns.join(",")),
-          m_columnCache(columns),
-          m_pQueryParser(new SearchQueryParser(pTrackCollection)),
+          m_columnCache(std::move(columns)),
+          m_pQueryParser(std::make_unique<SearchQueryParser>(
+                  pTrackCollection, std::move(searchColumns))),
           m_bIndexBuilt(false),
           m_bIsCaching(isCaching),
           m_database(pTrackCollection->database()) {
-    m_searchColumns << "artist"
-                    << "album"
-                    << "album_artist"
-                    << "location"
-                    << "grouping"
-                    << "comment"
-                    << "title"
-                    << "genre"
-                    << "crate";
-
-    // Convert all the search column names to their field indexes because we use
-    // them a bunch.
-    m_searchColumnIndices.resize(m_searchColumns.size());
-    for (int i = 0; i < m_searchColumns.size(); ++i) {
-        m_searchColumnIndices[i] = m_columnCache.fieldIndex(m_searchColumns[i]);
-    }
 }
 
 BaseTrackCache::~BaseTrackCache() {
@@ -125,10 +111,6 @@ void BaseTrackCache::ensureCached(const QSet<TrackId>& trackIds) {
     updateTracksInIndex(trackIds);
 }
 
-void BaseTrackCache::setSearchColumns(const QStringList& columns) {
-    m_searchColumns = columns;
-}
-
 const TrackPointer& BaseTrackCache::getRecentTrack(TrackId trackId) const {
     DEBUG_ASSERT(m_bIsCaching);
     // Only refresh the recently used track if the identifiers
@@ -488,7 +470,6 @@ void BaseTrackCache::filterAndSort(const QSet<TrackId>& trackIds,
     const std::unique_ptr<QueryNode> pQuery =
             m_pQueryParser->parseQuery(
                     searchQuery,
-                    m_searchColumns,
                     queryFragments.join(" AND "));
 
     QString filter = pQuery->toSql();

src/library/basetrackcache.h

@@ -43,10 +43,11 @@ class BaseTrackCache : public QObject {
     /// The order of the `columns` list parameter defines the initial/default
     /// order of columns in the library view.
     BaseTrackCache(TrackCollection* pTrackCollection,
-                   const QString& tableName,
-                   const QString& idColumn,
-                   const QStringList& columns,
-                   bool isCaching);
+            QString tableName,
+            QString idColumn,
+            QStringList columns,
+            QStringList searchColumns,
+            bool isCaching);
     ~BaseTrackCache() override;
 
     // Rebuild the BaseTrackCache index from the SQL table. This can be
@@ -73,7 +74,6 @@ class BaseTrackCache : public QObject {
     virtual bool isCached(TrackId trackId) const;
     virtual void ensureCached(TrackId trackId);
     virtual void ensureCached(const QSet<TrackId>& trackIds);
-    virtual void setSearchColumns(const QStringList& columns);
 
   signals:
     void tracksChanged(const QSet<TrackId>& trackIds);
@@ -126,9 +126,6 @@ class BaseTrackCache : public QObject {
 
     const StringCollator m_collator;
 
-    QStringList m_searchColumns;
-    QVector<int> m_searchColumnIndices;
-
     // Temporary storage for filterAndSort()
 
     QVector<TrackId> m_trackOrder;