Merge pull request #12436 from ronso0/lib-add-dir-error

(fix UX) Library: add feedback to directory operations (add, remove, relink)

src/coreservices.cpp

@@ -357,7 +357,7 @@ void CoreServices::initialize(QApplication* pApp) {
     // the uninitialized singleton instance!
     m_pPlayerManager->bindToLibrary(m_pLibrary.get());
 
-    bool hasChanged_MusicDir = false;
+    bool musicDirAdded = false;
 
     if (m_pTrackCollectionManager->internalCollection()->loadRootDirs().isEmpty()) {
         // TODO(XXX) this needs to be smarter, we can't distinguish between an empty
@@ -371,10 +371,9 @@ void CoreServices::initialize(QApplication* pApp) {
                 tr("Choose music library directory"),
                 QStandardPaths::writableLocation(
                         QStandardPaths::MusicLocation));
-        if (!fd.isEmpty()) {
-            // adds Folder to database.
-            m_pLibrary->slotRequestAddDir(fd);
-            hasChanged_MusicDir = true;
+        // request to add directory to database.
+        if (!fd.isEmpty() && m_pLibrary->requestAddDir(fd)) {
+            musicDirAdded = true;
         }
     }
 
@@ -425,7 +424,7 @@ void CoreServices::initialize(QApplication* pApp) {
 
     // Scan the library directory. Do this after the skinloader has
     // loaded a skin, see issue #6625
-    if (rescan || hasChanged_MusicDir || m_pSettingsManager->shouldRescanLibrary()) {
+    if (rescan || musicDirAdded || m_pSettingsManager->shouldRescanLibrary()) {
         m_pTrackCollectionManager->startLibraryScan();
     }
 

src/library/browse/browsefeature.cpp

@@ -46,10 +46,6 @@ BrowseFeature::BrowseFeature(
           m_proxyModel(&m_browseModel, true),
           m_pSidebarModel(new FolderTreeModel(this)),
           m_pLastRightClickedItem(nullptr) {
-    connect(this,
-            &BrowseFeature::requestAddDir,
-            pLibrary,
-            &Library::slotRequestAddDir);
     connect(&m_browseModel,
             &BrowseTableModel::restoreModelState,
             this,
@@ -184,7 +180,9 @@ void BrowseFeature::slotAddToLibrary() {
         return;
     }
     QString spath = m_pLastRightClickedItem->getData().toString();
-    emit requestAddDir(spath);
+    if (!m_pLibrary->requestAddDir(spath)) {
+        return;
+    }
 
     QMessageBox msgBox;
     msgBox.setIcon(QMessageBox::Warning);

src/library/dao/directorydao.cpp

@@ -79,10 +79,17 @@ DirectoryDAO::AddResult DirectoryDAO::addDirectory(
     if (!newDir.exists() || !newDir.isDir()) {
         kLogger.warning()
                 << "Failed to add"
-                << newDir
+                << newDir.location()
                 << ": Directory does not exist or is inaccessible";
         return AddResult::InvalidOrMissingDirectory;
     }
+    if (!newDir.isReadable()) {
+        kLogger.warning()
+                << "Aborting to to add"
+                << newDir.location()
+                << ": Directory can not be read";
+        return AddResult::UnreadableDirectory;
+    }
     const auto newCanonicalLocation = newDir.canonicalLocation();
     DEBUG_ASSERT(!newCanonicalLocation.isEmpty());
     QList<mixxx::FileInfo> obsoleteChildDirs;
@@ -91,7 +98,7 @@ DirectoryDAO::AddResult DirectoryDAO::addDirectory(
     // Worst that can happen is that we have orphan tracks in the database that
     // would be moved to missing after the rescan (which is required anyway after
     // having added a new dir).
-    for (auto&& oldDir : loadAllDirectories(true)) {
+    for (auto&& oldDir : loadAllDirectories(true /* ignore missing */)) {
         const auto oldCanonicalLocation = oldDir.canonicalLocation();
         DEBUG_ASSERT(!oldCanonicalLocation.isEmpty());
         if (mixxx::FileInfo::isRootSubCanonicalLocation(
@@ -126,7 +133,7 @@ DirectoryDAO::AddResult DirectoryDAO::addDirectory(
         if (RemoveResult::Ok != removeDirectory(oldDir)) {
             kLogger.warning()
                     << "Failed to remove obsolete child directory"
-                    << oldDir;
+                    << oldDir.location();
             DEBUG_ASSERT(!"removeDirectory failed");
             continue;
         }
@@ -161,7 +168,7 @@ DirectoryDAO::RemoveResult DirectoryDAO::removeDirectory(
     return RemoveResult::Ok;
 }
 
-QList<RelocatedTrack> DirectoryDAO::relocateDirectory(
+std::pair<DirectoryDAO::RelocateResult, QList<RelocatedTrack>> DirectoryDAO::relocateDirectory(
         const QString& oldDirectory,
         const QString& newDirectory) const {
     // Don't verify the old directory with
@@ -170,7 +177,28 @@ QList<RelocatedTrack> DirectoryDAO::relocateDirectory(
     //  valid location.
     // Just work with the QString; in case of an invalid path track relocation
     // will simply fail if the database query yields no results.
-    DEBUG_ASSERT(newDirectory == mixxx::FileInfo(newDirectory).location());
+    const mixxx::FileInfo newFileInfo(newDirectory);
+    DEBUG_ASSERT(newDirectory == newFileInfo.location());
+
+    if (!newFileInfo.exists() || !newFileInfo.isDir()) {
+        kLogger.warning()
+                << "Aborting to relocate"
+                << oldDirectory
+                << ": "
+                << newDirectory
+                << "does not exist or is inaccessible";
+        return {RelocateResult::InvalidOrMissingDirectory, {}};
+    }
+    if (!newFileInfo.isReadable()) {
+        kLogger.warning()
+                << "Aborting to relocate"
+                << oldDirectory
+                << ": "
+                << newDirectory
+                << "can not be read";
+        return {RelocateResult::UnreadableDirectory, {}};
+    }
+
     // TODO(rryan): This method could use error reporting. It can fail in
     // mysterious ways for example if a track in the oldDirectory also has a zombie
     // track location in newDirectory then the replace query will fail because the
@@ -183,7 +211,7 @@ QList<RelocatedTrack> DirectoryDAO::relocateDirectory(
     if (!query.exec()) {
         LOG_FAILED_QUERY(query) << "could not relocate directory"
                                 << oldDirectory << "to" << newDirectory;
-        return {};
+        return {RelocateResult::SqlError, {}};
     }
 
     // Appending '/' is required to disambiguate files from parent
@@ -199,7 +227,7 @@ QList<RelocatedTrack> DirectoryDAO::relocateDirectory(
     query.bindValue(":oldDirectoryPrefix", oldDirectoryPrefix);
     if (!query.exec()) {
         LOG_FAILED_QUERY(query) << "could not relocate path of tracks";
-        return {};
+        return {RelocateResult::SqlError, {}};
     }
 
     QList<DbId> loc_ids;
@@ -233,10 +261,10 @@ QList<RelocatedTrack> DirectoryDAO::relocateDirectory(
         query.bindValue(QStringLiteral(":id"), loc_ids.at(i).toVariant());
         if (!query.exec()) {
             LOG_FAILED_QUERY(query) << "could not relocate path of track";
-            return {};
+            return {RelocateResult::SqlError, relocatedTracks};
         }
     }
 
     qDebug() << "Relocated tracks:" << relocatedTracks.size();
-    return relocatedTracks;
+    return {RelocateResult::Ok, relocatedTracks};
 }

src/library/dao/directorydao.h

@@ -20,6 +20,7 @@ class DirectoryDAO : public DAO {
         Ok,
         AlreadyWatching,
         InvalidOrMissingDirectory,
+        UnreadableDirectory,
         SqlError,
     };
     AddResult addDirectory(const mixxx::FileInfo& newDir) const;
@@ -31,8 +32,14 @@ class DirectoryDAO : public DAO {
     };
     RemoveResult removeDirectory(const mixxx::FileInfo& oldDir) const;
 
+    enum class RelocateResult {
+        Ok,
+        InvalidOrMissingDirectory,
+        UnreadableDirectory,
+        SqlError,
+    };
     // TODO: Move this function out of the DAO
-    QList<RelocatedTrack> relocateDirectory(
+    std::pair<RelocateResult, QList<RelocatedTrack>> relocateDirectory(
             const QString& oldDirectory,
             const QString& newDirectory) const;
 };

src/library/library.cpp

@@ -579,7 +579,7 @@ void Library::onSkinLoadFinished() {
     m_pSidebarModel->activateDefaultSelection();
 }
 
-void Library::slotRequestAddDir(const QString& dir) {
+bool Library::requestAddDir(const QString& dir) {
     // We only call this method if the user has picked a new directory via a
     // file dialog. This means the system sandboxer (if we are sandboxed) has
     // granted us permission to this folder. Create a security bookmark while we
@@ -589,24 +589,59 @@ void Library::slotRequestAddDir(const QString& dir) {
     QDir directory(dir);
     Sandbox::createSecurityTokenForDir(directory);
 
-    if (!m_pTrackCollectionManager->addDirectory(mixxx::FileInfo(dir))) {
-        QMessageBox::information(nullptr,
-                tr("Add Directory to Library"),
-                tr("Could not add the directory to your library. Either this "
-                   "directory is already in your library or you are currently "
-                   "rescanning your library."));
+    DirectoryDAO::AddResult result =
+            m_pTrackCollectionManager->addDirectory(mixxx::FileInfo(dir));
+    QString error;
+    switch (result) {
+    case DirectoryDAO::AddResult::Ok:
+        break;
+    case DirectoryDAO::AddResult::AlreadyWatching:
+        error = tr("This or a parent directory is already in your library.");
+        break;
+    case DirectoryDAO::AddResult::InvalidOrMissingDirectory:
+        error = tr(
+                "This or a listed directory does not exist or is inaccessible.\n"
+                "Aborting the operation to avoid library inconsistencies");
+        break;
+    case DirectoryDAO::AddResult::UnreadableDirectory:
+        error = tr(
+                "This directory can not be read.");
+        break;
+    case DirectoryDAO::AddResult::SqlError:
+        error = tr(
+                "An unknown error occurred.\n"
+                "Aborting the operation to avoid library inconsistencies");
+        break;
+    default:
+        return false;
     }
-    // set at least one directory in the config file so that it will be possible
-    // to downgrade from 1.12
-    if (m_pConfig->getValueString(kLegacyDirectoryConfigKey).length() < 1) {
-        m_pConfig->set(kLegacyDirectoryConfigKey, dir);
+    if (!error.isEmpty()) {
+        QMessageBox::information(nullptr,
+                tr("Can't add Directory to Library"),
+                tr("Could not add <b>%1</b> to your library.\n\n%2")
+                        .arg(directory.absolutePath(), error));
+        return false;
     }
+
+    return true;
 }
 
-void Library::slotRequestRemoveDir(const QString& dir, LibraryRemovalType removalType) {
+bool Library::requestRemoveDir(const QString& dir, LibraryRemovalType removalType) {
     // Remove the directory from the directory list.
-    if (!m_pTrackCollectionManager->removeDirectory(mixxx::FileInfo(dir))) {
-        return;
+    DirectoryDAO::RemoveResult result =
+            m_pTrackCollectionManager->removeDirectory(mixxx::FileInfo(dir));
+    if (result != DirectoryDAO::RemoveResult::Ok) {
+        switch (result) {
+        case DirectoryDAO::RemoveResult::NotFound:
+        case DirectoryDAO::RemoveResult::SqlError:
+            QMessageBox::information(nullptr,
+                    tr("Can't remove Directory from Library"),
+                    tr("An unknown error occurred."));
+            break;
+        default:
+            DEBUG_ASSERT(!"unreachable");
+        }
+        return false;
     }
 
     switch (removalType) {
@@ -625,32 +660,36 @@ void Library::slotRequestRemoveDir(const QString& dir, LibraryRemovalType remova
         DEBUG_ASSERT(!"unreachable");
     }
 
-    // Also update the config file if necessary so that downgrading is still
-    // possible.
-    QString confDir = m_pConfig->getValueString(kLegacyDirectoryConfigKey);
-
-    if (QDir(dir) == QDir(confDir)) {
-        const QList<mixxx::FileInfo> dirList =
-                m_pTrackCollectionManager->internalCollection()->loadRootDirs();
-        if (dirList.isEmpty()) {
-            // Save empty string so that an old version of mixxx knows it has to
-            // ask for a new directory.
-            m_pConfig->set(kLegacyDirectoryConfigKey, QString());
-        } else {
-            m_pConfig->set(kLegacyDirectoryConfigKey, dirList.first().location());
-        }
-    }
+    return true;
 }
 
-void Library::slotRequestRelocateDir(const QString& oldDir, const QString& newDir) {
-    m_pTrackCollectionManager->relocateDirectory(oldDir, newDir);
+bool Library::requestRelocateDir(const QString& oldDir, const QString& newDir) {
+    DirectoryDAO::RelocateResult result =
+            m_pTrackCollectionManager->relocateDirectory(oldDir, newDir);
+    if (result == DirectoryDAO::RelocateResult::Ok) {
+        return true;
+    }
 
-    // also update the config file if necessary so that downgrading is still
-    // possible
-    QString conDir = m_pConfig->getValueString(kLegacyDirectoryConfigKey);
-    if (oldDir == conDir) {
-        m_pConfig->set(kLegacyDirectoryConfigKey, newDir);
+    QString error;
+    switch (result) {
+    case DirectoryDAO::RelocateResult::InvalidOrMissingDirectory:
+        error = tr(
+                "This directory does not exist or is inaccessible.");
+        break;
+    case DirectoryDAO::RelocateResult::UnreadableDirectory:
+        error = tr(
+                "This directory can not be read.");
+        break;
+    default:
+        DEBUG_ASSERT(!"unreachable");
+    }
+    if (!error.isEmpty()) {
+        QMessageBox::information(nullptr,
+                tr("Relink Directory"),
+                tr("Could not relink <b>%1</b> to <b>%2</b>.\n\n%3")
+                        .arg(oldDir, newDir, error));
     }
+    return false;
 }
 
 void Library::setFont(const QFont& font) {

src/library/library.h

@@ -102,6 +102,10 @@ class Library: public QObject {
     /// and shows the results by switching the view.
     void searchTracksInCollection(const QString& query);
 
+    bool requestAddDir(const QString& directory);
+    bool requestRemoveDir(const QString& directory, LibraryRemovalType removalType);
+    bool requestRelocateDir(const QString& previousDirectory, const QString& newDirectory);
+
 #ifdef __ENGINEPRIME__
     std::unique_ptr<mixxx::LibraryExporter> makeLibraryExporter(QWidget* parent);
 #endif
@@ -115,9 +119,6 @@ class Library: public QObject {
     void slotRefreshLibraryModels();
     void slotCreatePlaylist();
     void slotCreateCrate();
-    void slotRequestAddDir(const QString& directory);
-    void slotRequestRemoveDir(const QString& directory, LibraryRemovalType removalType);
-    void slotRequestRelocateDir(const QString& previousDirectory, const QString& newDirectory);
     void onSkinLoadFinished();
     void slotSaveCurrentViewState() const;
     void slotRestoreCurrentViewState() const;