(fix) avoid crash after removing Quick Link

src/library/browse/browsefeature.cpp

@@ -24,6 +24,8 @@ const QString kViewName = QStringLiteral("BROWSEHOME");
 
 const QString kQuickLinksSeparator = QStringLiteral("-+-");
 
+const ConfigKey kQuickLinksCfgKey = ConfigKey("[Browse]", "QuickLinks");
+
 #if defined(__LINUX__)
 const QStringList removableDriveRootPaths() {
     QStringList paths;
@@ -45,12 +47,23 @@ BrowseFeature::BrowseFeature(
           m_pTrackCollection(pLibrary->trackCollectionManager()->internalCollection()),
           m_browseModel(this, pLibrary->trackCollectionManager(), pRecordingManager),
           m_proxyModel(&m_browseModel, true),
-          m_pSidebarModel(new FolderTreeModel(this)),
-          m_pLastRightClickedItem(nullptr) {
+          m_pSidebarModel(new FolderTreeModel(this)) {
     connect(&m_browseModel,
             &BrowseTableModel::restoreModelState,
             this,
             &LibraryFeature::restoreModelState);
+    connect(m_pSidebarModel,
+            &QAbstractItemModel::rowsAboutToBeRemoved,
+            this,
+            &BrowseFeature::invalidateRightClickIndex);
+    connect(m_pSidebarModel,
+            &QAbstractItemModel::rowsAboutToBeInserted,
+            this,
+            &BrowseFeature::invalidateRightClickIndex);
+    connect(m_pSidebarModel,
+            &QAbstractItemModel::modelAboutToBeReset,
+            this,
+            &BrowseFeature::invalidateRightClickIndex);
 
     m_pAddQuickLinkAction = new QAction(tr("Add to Quick Links"),this);
     connect(m_pAddQuickLinkAction,
@@ -157,34 +170,36 @@ QVariant BrowseFeature::title() {
 }
 
 void BrowseFeature::slotAddQuickLink() {
-    if (!m_pLastRightClickedItem) {
+    const QString path = getLastRightClickedPath();
+    if (path.isEmpty()) {
         return;
     }
 
-    QVariant vpath = m_pLastRightClickedItem->getData();
-    QString spath = vpath.toString();
-    QString name = extractNameFromPath(spath);
+    const QString name = extractNameFromPath(path);
 
-    QModelIndex parent = m_pSidebarModel->index(m_pQuickLinkItem->parentRow(), 0);
+    const QModelIndex parent = m_pSidebarModel->index(m_pQuickLinkItem->parentRow(), 0);
     std::vector<std::unique_ptr<TreeItem>> rows;
     // TODO() Use here std::span to get around the heap allocation of
     // std::vector for a single element.
-    rows.push_back(std::make_unique<TreeItem>(name, vpath));
+    rows.push_back(std::make_unique<TreeItem>(name, path));
     m_pSidebarModel->insertTreeItemRows(std::move(rows), m_pQuickLinkItem->childRows(), parent);
 
-    m_quickLinkList.append(spath);
+    m_quickLinkList.append(path);
     saveQuickLinks();
 }
 
 void BrowseFeature::slotAddToLibrary() {
-    if (!m_pLastRightClickedItem) {
+    const QString path = getLastRightClickedPath();
+    if (path.isEmpty()) {
         return;
     }
-    QString spath = m_pLastRightClickedItem->getData().toString();
-    if (!m_pLibrary->requestAddDir(spath)) {
+
+    if (!m_pLibrary->requestAddDir(path)) {
         return;
     }
 
+    // TODO Check if this really added a new directory. Ignore if it's a child
+    // of an already watched directory. Notify if it failed for another reason.
     QMessageBox msgBox;
     msgBox.setIcon(QMessageBox::Warning);
     // strings are dupes from DlgPrefLibrary
@@ -212,41 +227,35 @@ void BrowseFeature::slotLibraryScanFinished() {
 }
 
 void BrowseFeature::slotRemoveQuickLink() {
-    if (!m_pLastRightClickedItem) {
+    const QString path = getLastRightClickedPath();
+    if (path.isEmpty()) {
         return;
     }
 
-    QString spath = m_pLastRightClickedItem->getData().toString();
-    int index = m_quickLinkList.indexOf(spath);
-
-    if (index == -1) {
+    int quickLinkIndex = m_quickLinkList.indexOf(path);
+    if (quickLinkIndex == -1) {
         return;
     }
 
-    m_pLastRightClickedItem = nullptr;
+    // Quick Links' parent is QModelIndex(), so we can call this without parent
+    // and still get the QAbstractItemModel::hasIndex() match.
     QModelIndex parent = m_pSidebarModel->index(m_pQuickLinkItem->parentRow(), 0);
-    m_pSidebarModel->removeRow(index, parent);
+    m_pSidebarModel->removeRow(quickLinkIndex, parent);
 
-    m_quickLinkList.removeAt(index);
+    m_quickLinkList.removeAt(quickLinkIndex);
     saveQuickLinks();
 }
 
 void BrowseFeature::slotRefreshDirectoryTree() {
-    if (!m_pLastRightClickedItem) {
-        return;
-    }
-
-    const auto* pItem = m_pLastRightClickedItem;
-    if (!pItem->getData().isValid()) {
+    if (!m_lastRightClickedIndex.isValid()) {
         return;
     }
 
-    const QString path = pItem->getData().toString();
+    const QString path = getLastRightClickedPath();
     m_pSidebarModel->removeChildDirsFromCache(QStringList{path});
 
     // Update child items
-    const QModelIndex index = m_pSidebarModel->index(pItem->parentRow(), 0);
-    onLazyChildExpandation(index);
+    onLazyChildExpandation(m_lastRightClickedIndex);
 }
 
 TreeItemModel* BrowseFeature::sidebarModel() const {
@@ -275,11 +284,20 @@ void BrowseFeature::activate() {
 // Note: This is executed whenever you single click on an child item
 // Single clicks will not populate sub folders
 void BrowseFeature::activateChild(const QModelIndex& index) {
-    TreeItem *item = static_cast<TreeItem*>(index.internalPointer());
-    qDebug() << "BrowseFeature::activateChild " << item->getLabel() << " "
-             << item->getData().toString();
+    if (!index.isValid()) {
+        return;
+    }
+    TreeItem* pItem = static_cast<TreeItem*>(index.internalPointer());
+    if (!(pItem && pItem->getData().isValid())) {
+        return;
+    }
+    qDebug() << "BrowseFeature::activateChild " << pItem->getLabel() << " "
+             << pItem->getData().toString();
 
-    QString path = item->getData().toString();
+    QString path = pItem->getData().toString();
+    if (path.isEmpty()) {
+        return;
+    }
     if (path == QUICK_LINK_NODE || path == DEVICE_NODE) {
         emit saveModelState();
         // Clear the tracks view
@@ -310,45 +328,36 @@ void BrowseFeature::activateChild(const QModelIndex& index) {
 }
 
 void BrowseFeature::onRightClickChild(const QPoint& globalPos, const QModelIndex& index) {
-    TreeItem *item = static_cast<TreeItem*>(index.internalPointer());
-    if (!item) {
+    TreeItem* pItem = static_cast<TreeItem*>(index.internalPointer());
+    if (!pItem) {
         return;
     }
 
-    QString path = item->getData().toString();
+    QString path = pItem->getData().toString();
 
     if (path == QUICK_LINK_NODE || path == DEVICE_NODE) {
         return;
     }
 
-    // Make sure that this is reset when TreeItems are deleted in onLazyChildExpandation()
-    m_pLastRightClickedItem = item;
+    // Make sure that this is reset whenever the tree changes
+    // and it may have become a dangling pointer
+    m_lastRightClickedIndex = index;
 
     QMenu menu(m_pSidebarWidget);
 
-    if (item->parent()->getData().toString() == QUICK_LINK_NODE) {
-        // This is a QuickLink
+    if (pItem->parent()->getData().toString() == QUICK_LINK_NODE ||
+            m_quickLinkList.contains(path)) {
+        // This is a QuickLink or path is in the Quick Link list
         menu.addAction(m_pRemoveQuickLinkAction);
-        menu.addAction(m_pRefreshDirTreeAction);
-        menu.exec(globalPos);
-        onLazyChildExpandation(index);
-        return;
-    }
-
-    if (m_quickLinkList.contains(path)) {
-        // Path is in the Quick Link list
-        menu.addAction(m_pRemoveQuickLinkAction);
-        menu.addAction(m_pRefreshDirTreeAction);
-        menu.exec(globalPos);
-        onLazyChildExpandation(index);
-        return;
+    } else {
+        menu.addAction(m_pAddQuickLinkAction);
     }
 
-    menu.addAction(m_pAddQuickLinkAction);
+    // TODO Check if we already watch this path or a parent and don't show or
+    // disable this action.
     menu.addAction(m_pAddtoLibraryAction);
     menu.addAction(m_pRefreshDirTreeAction);
     menu.exec(globalPos);
-    onLazyChildExpandation(index);
 }
 
 namespace {
@@ -402,36 +411,30 @@ std::vector<std::unique_ptr<TreeItem>> createRemovableDevices() {
 // This is called whenever you double click or use the triangle symbol to expand
 // the subtree. The method will read the subfolders.
 void BrowseFeature::onLazyChildExpandation(const QModelIndex& index) {
-    // The selected item might have been removed just before this function
-    // is invoked!
-    // NOTE(2018-04-21, uklotzde): The following checks prevent a crash
-    // that would otherwise occur after a quick link has been removed.
-    // Especially the check of QVariant::isValid() seems to be essential.
-    // See also: https://github.com/mixxxdj/mixxx/issues/8270
-    // After migration to Qt5 the implementation of all LibraryFeatures
-    // should be revisited, because I consider these checks only as a
-    // temporary workaround.
+    // Caution: Make sure the passed index still exists in the model.
+    // In case it has been removed or replaced, it is still "valid", but
+    // holds dangling internalPointer() that causes a crash.
+    // These sanity checks will pass in such case.
     if (!index.isValid()) {
         return;
     }
-    TreeItem *item = static_cast<TreeItem*>(index.internalPointer());
-    if (!(item && item->getData().isValid())) {
+    TreeItem* pItem = static_cast<TreeItem*>(index.internalPointer());
+    if (!(pItem && pItem->getData().isValid())) {
         return;
     }
 
-    qDebug() << "BrowseFeature::onLazyChildExpandation " << item->getLabel()
-             << " " << item->getData();
+    qDebug() << "BrowseFeature::onLazyChildExpandation " << pItem->getLabel()
+             << " " << pItem->getData();
 
-    QString path = item->getData().toString();
+    QString path = pItem->getData().toString();
 
     // If the item is a built-in node, e.g., 'QuickLink' return
     if (path.isEmpty() || path == QUICK_LINK_NODE) {
         return;
     }
 
-    m_pLastRightClickedItem = nullptr;
     // Before we populate the subtree, we need to delete old subtrees
-    m_pSidebarModel->removeRows(0, item->childRows(), index);
+    m_pSidebarModel->removeRows(0, pItem->childRows(), index);
 
     // List of subfolders or drive letters
     std::vector<std::unique_ptr<TreeItem>> folders;
@@ -499,16 +502,20 @@ QString BrowseFeature::getRootViewHtml() const {
 }
 
 void BrowseFeature::saveQuickLinks() {
-    m_pConfig->set(ConfigKey("[Browse]","QuickLinks"),ConfigValue(
-        m_quickLinkList.join(kQuickLinksSeparator)));
+    m_pConfig->setValue<QString>(kQuickLinksCfgKey,
+            m_quickLinkList.join(kQuickLinksSeparator));
 }
 
 void BrowseFeature::loadQuickLinks() {
-    if (m_pConfig->getValueString(ConfigKey("[Browse]","QuickLinks")).isEmpty()) {
+    if (!m_pConfig->exists(kQuickLinksCfgKey)) {
+        // New profile, create default Quick Links
         m_quickLinkList = getDefaultQuickLinks();
     } else {
-        m_quickLinkList = m_pConfig->getValueString(
-            ConfigKey("[Browse]","QuickLinks")).split(kQuickLinksSeparator);
+        const QString quickLinks = m_pConfig->getValueString(
+                kQuickLinksCfgKey);
+        if (!quickLinks.isEmpty()) {
+            m_quickLinkList = quickLinks.split(kQuickLinksSeparator);
+        }
     }
 }
 
@@ -585,3 +592,18 @@ QStringList BrowseFeature::getDefaultQuickLinks() const {
 void BrowseFeature::releaseBrowseThread() {
     m_browseModel.releaseBrowseThread();
 }
+
+QString BrowseFeature::getLastRightClickedPath() const {
+    if (!m_lastRightClickedIndex.isValid()) {
+        return {};
+    }
+    TreeItem* pItem = static_cast<TreeItem*>(m_lastRightClickedIndex.internalPointer());
+    VERIFY_OR_DEBUG_ASSERT(pItem && pItem->getData().isValid()) {
+        return {};
+    }
+    return pItem->getData().toString();
+}
+
+void BrowseFeature::invalidateRightClickIndex() {
+    m_lastRightClickedIndex = QModelIndex();
+}

src/library/browse/browsefeature.h

@@ -50,6 +50,7 @@ class BrowseFeature : public LibraryFeature {
     void onLazyChildExpandation(const QModelIndex& index) override;
     void slotLibraryScanStarted();
     void slotLibraryScanFinished();
+    void invalidateRightClickIndex();
 
   signals:
     void setRootIndex(const QModelIndex&);
@@ -63,6 +64,7 @@ class BrowseFeature : public LibraryFeature {
     std::vector<std::unique_ptr<TreeItem>> getChildDirectoryItems(const QString& path) const;
     void saveQuickLinks();
     void loadQuickLinks();
+    QString getLastRightClickedPath() const;
 
     TrackCollection* const m_pTrackCollection;
 
@@ -76,7 +78,7 @@ class BrowseFeature : public LibraryFeature {
 
     // Caution: Make sure this is reset whenever the library tree is updated,
     // so that the internalPointer() does not become dangling
-    TreeItem* m_pLastRightClickedItem;
+    QModelIndex m_lastRightClickedIndex;
     TreeItem* m_pQuickLinkItem;
     QStringList m_quickLinkList;
     QPointer<WLibrarySidebar> m_pSidebarWidget;

src/library/browse/foldertreemodel.cpp

@@ -28,21 +28,25 @@ FolderTreeModel::~FolderTreeModel() {
 // Note that BrowseFeature inserts folder trees dynamically and rowCount()
 // is only called if necessary.
 bool FolderTreeModel::hasChildren(const QModelIndex& parent) const {
-    TreeItem *item = static_cast<TreeItem*>(parent.internalPointer());
-    // Usually the child count is 0 because we do lazy initialization
-    // However, for, buid-in items such as 'Quick Links' there exist
-    // child items at init time
-    if (item->getData().toString() == QUICK_LINK_NODE) {
-        return true;
+    TreeItem* pItem = static_cast<TreeItem*>(parent.internalPointer());
+    VERIFY_OR_DEBUG_ASSERT(pItem) {
+        return false;
+    }
+    // For Quick Link node we simply return the row count.
+    // That way we always have the real (uncached) state.
+    if (pItem->getData().toString() == QUICK_LINK_NODE) {
+        return rowCount(parent) > 0;
     }
-    //Can only happen on Windows
-    if (item->getData().toString() == DEVICE_NODE) {
+    // For the 'Removable Devices' node we always return true so WLibrarySidebar
+    // thinks the node is expandable and any attempt to expand it will invoke
+    // LibraryFeature::onLazyChildExpandation() and update the tree.
+    if (pItem->getData().toString() == DEVICE_NODE) {
         return true;
     }
 
     // In all other cases the getData() points to a folder
-    QString folder = item->getData().toString();
-    return directoryHasChildren(folder);
+    const QString path = pItem->getData().toString();
+    return directoryHasChildren(path);
 }
 
 bool FolderTreeModel::directoryHasChildren(const QString& path) const {
@@ -102,7 +106,9 @@ bool FolderTreeModel::directoryHasChildren(const QString& path) const {
     // filesystems that do not fully implement readdir such as JFS.
     if (directory == nullptr || (unknown_count == total_count && total_count > 0)) {
         QDir dir(path);
-        QFileInfoList all = dir.entryInfoList(QDir::Dirs | QDir::NoDotAndDotDot);
+        // Instead of costly entryInfoList() we use entryList() which doesn't
+        // create a QFileInfo cache (only if sort flag is not set!).
+        const QStringList all = dir.entryList(QDir::Dirs | QDir::NoDotAndDotDot);
         has_children = all.count() > 0;
     }
 #endif