diff --git a/src/library/browse/browsefeature.cpp b/src/library/browse/browsefeature.cpp index f87930b2acd4..2d6c9a20e7d3 100644 --- a/src/library/browse/browsefeature.cpp +++ b/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> 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(name, vpath)); + rows.push_back(std::make_unique(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(index.internalPointer()); - qDebug() << "BrowseFeature::activateChild " << item->getLabel() << " " - << item->getData().toString(); + if (!index.isValid()) { + return; + } + TreeItem* pItem = static_cast(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(index.internalPointer()); - if (!item) { + TreeItem* pItem = static_cast(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> 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(index.internalPointer()); - if (!(item && item->getData().isValid())) { + TreeItem* pItem = static_cast(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> 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(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(m_lastRightClickedIndex.internalPointer()); + VERIFY_OR_DEBUG_ASSERT(pItem && pItem->getData().isValid()) { + return {}; + } + return pItem->getData().toString(); +} + +void BrowseFeature::invalidateRightClickIndex() { + m_lastRightClickedIndex = QModelIndex(); +} diff --git a/src/library/browse/browsefeature.h b/src/library/browse/browsefeature.h index dd633e75365c..d809406a1c0d 100644 --- a/src/library/browse/browsefeature.h +++ b/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> 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 m_pSidebarWidget; diff --git a/src/library/browse/foldertreemodel.cpp b/src/library/browse/foldertreemodel.cpp index e2cf3a8cd3dd..3c19760ef017 100644 --- a/src/library/browse/foldertreemodel.cpp +++ b/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(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(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 diff --git a/src/library/treeitemmodel.cpp b/src/library/treeitemmodel.cpp index d2b06230ebd8..4a088aba3e94 100644 --- a/src/library/treeitemmodel.cpp +++ b/src/library/treeitemmodel.cpp @@ -143,13 +143,13 @@ int TreeItemModel::rowCount(const QModelIndex& parent) const { return 0; } - TreeItem* parentItem; + TreeItem* pParentItem; if (parent.isValid()) { - parentItem = static_cast(parent.internalPointer()); + pParentItem = static_cast(parent.internalPointer()); } else { - parentItem = getRootItem(); + pParentItem = getRootItem(); } - return parentItem->childRows(); + return pParentItem->childRows(); } // Populates the model and notifies the view.