Remove OC10SyncRoot

Also simplify the path validity checks.

Author: erikjv

src/gui/folderman.cpp

@@ -768,29 +768,40 @@ static QString canonicalPath(const QString &path)
 
 static QString checkPathForSyncRootMarkingRecursive(const QString &path, FolderMan::NewFolderType folderType, const QUuid &accountUuid)
 {
+    // First do basic checks if this path is usable
+    const QFileInfo selectedPathInfo(path);
+    if (!selectedPathInfo.isDir()) {
+        return FolderMan::tr("The selected path is not a folder.");
+    }
+
+    if (numberOfSyncJournals(selectedPathInfo.filePath()) != 0) {
+        return FolderMan::tr("The folder %1 is used in a folder sync connection.").arg(QDir::toNativeSeparators(selectedPathInfo.filePath()));
+    }
+
+    // Check for sync root markings in this folder
     std::pair<QString, QUuid> existingTags = Utility::getDirectorySyncRootMarkings(path);
     if (!existingTags.first.isEmpty()) {
         if (existingTags.first != Theme::instance()->orgDomainName()) {
             // another application uses this as spaces root folder
-            return FolderMan::tr("Folder '%1' is already in use by application %2!").arg(path, existingTags.first);
+            return FolderMan::tr("Folder '%1' is already in use by application %2.").arg(path, existingTags.first);
         }
 
         // Looks good, it's our app, let's check the account tag:
         switch (folderType) {
         case FolderMan::NewFolderType::SpacesFolder:
             if (existingTags.second == accountUuid) {
-                // Nice, that's what we like, the sync root for our account in our app. No error.
+                // Nice, that's what we like, the sync root for our account in our app. No error, and we don't need to check the parent folders.
                 return {};
             }
             [[fallthrough]];
-        case FolderMan::NewFolderType::OC10SyncRoot:
-            [[fallthrough]];
         case FolderMan::NewFolderType::SpacesSyncRoot:
             // It's our application but we don't want to create a spaces folder, so it must be another space root
             return FolderMan::tr("Folder '%1' is already in use by another account.").arg(path);
         }
     }
 
+    // This folder is ok, but check the parent folders if they are used
+
     QString parent = QFileInfo(path).path();
     if (parent == path) { // root dir, stop recursing
         return {};
@@ -799,51 +810,7 @@ static QString checkPathForSyncRootMarkingRecursive(const QString &path, FolderM
     return checkPathForSyncRootMarkingRecursive(parent, folderType, accountUuid);
 }
 
-QString FolderMan::checkPathValidityRecursive(const QString &path, FolderMan::NewFolderType folderType, const QUuid &accountUuid)
-{
-    if (path.isEmpty()) {
-        return FolderMan::tr("No valid folder selected!");
-    }
-
-#ifdef Q_OS_WIN
-    Utility::NtfsPermissionLookupRAII ntfs_perm;
-#endif
-
-    auto pathLengthCheck = Folder::checkPathLength(path);
-    if (!pathLengthCheck) {
-        return pathLengthCheck.error();
-    }
-
-    const QFileInfo selectedPathInfo(path);
-    if (!selectedPathInfo.exists()) {
-        const QString parentPath = selectedPathInfo.path();
-        if (parentPath != path) {
-            return checkPathValidityRecursive(parentPath, folderType, accountUuid);
-        }
-        return FolderMan::tr("The selected path does not exist!");
-    }
-
-    if (numberOfSyncJournals(selectedPathInfo.filePath()) != 0) {
-        return FolderMan::tr("The folder %1 is used in a folder sync connection!").arg(QDir::toNativeSeparators(selectedPathInfo.filePath()));
-    }
-
-    // At this point we know there is no syncdb in the parent hierarchy, check for spaces sync root.
-
-    if (!selectedPathInfo.isDir()) {
-        return FolderMan::tr("The selected path is not a folder!");
-    }
-
-    if (!selectedPathInfo.isWritable()) {
-        return FolderMan::tr("You have no permission to write to the selected folder!");
-    }
-
-    return checkPathForSyncRootMarkingRecursive(path, folderType, accountUuid);
-}
-
 /*
- * OC10 folder:
- *  - sync root not in syncdb folder
- *  - sync root not in spaces root
  * with spaces:
  *  - spaces sync root not in syncdb folder
  *  - spaces sync root not in another spaces sync root
@@ -852,37 +819,67 @@ QString FolderMan::checkPathValidityRecursive(const QString &path, FolderMan::Ne
  *  - space *can* be in sync root
  *  - space not in spaces sync root of other account (check with account uuid)
  */
-QString FolderMan::checkPathValidityForNewFolder(const QString &path, NewFolderType folderType, const QUuid &accountUuid) const
+QString FolderMan::checkPathValidity(const QString &path, NewFolderType folderType, const QUuid &accountUuid)
 {
-    // check if the local directory isn't used yet in another ownCloud sync
+    // Do checks against existing folders. This catches cases where:
+    // - the user deleted a folder from the filesystem, but there is still a folder sync connection in the configuration
+    // - check if this folder contains other folders for spaces
+    // If so, error out.
     const auto cs = Utility::fsCaseSensitivity();
-
     const QString userDir = QDir::cleanPath(canonicalPath(path)) + QLatin1Char('/');
-    for (auto f : _folders) {
+    for (auto f : FolderMan::instance()->folders()) {
         const QString folderDir = QDir::cleanPath(canonicalPath(f->path())) + QLatin1Char('/');
-
         if (QString::compare(folderDir, userDir, cs) == 0) {
-            return tr("There is already a sync from the server to this local folder. "
-                      "Please pick another local folder!");
+            return tr("There is already a sync from the server to this local folder.");
         }
         if (FileSystem::isChildPathOf(folderDir, userDir)) {
-            return tr("The local folder %1 already contains a folder used in a folder sync connection. "
-                      "Please pick another local folder!")
+            return tr("The local folder %1 already contains a folder used in a folder sync connection.")
                 .arg(QDir::toNativeSeparators(path));
         }
 
         if (FileSystem::isChildPathOf(userDir, folderDir)) {
-            return tr("The local folder %1 is already contained in a folder used in a folder sync connection. "
-                      "Please pick another local folder!")
+            return tr("The local folder %1 is already contained in a folder used in a folder sync connection.")
                 .arg(QDir::toNativeSeparators(path));
         }
     }
 
-    const auto result = checkPathValidityRecursive(path, folderType, accountUuid);
-    if (!result.isEmpty()) {
-        return tr("%1 Please pick another local folder!").arg(result);
+    // Now run filesystem based checks
+    return checkPathValidityRecursive(path, folderType, accountUuid);
+}
+
+QString FolderMan::checkPathValidityRecursive(const QString &path, NewFolderType folderType, const QUuid &accountUuid)
+{
+    if (path.isEmpty()) {
+        return FolderMan::tr("No valid folder selected.");
+    }
+
+#ifdef Q_OS_WIN
+    Utility::NtfsPermissionLookupRAII ntfs_perm;
+#endif
+
+    auto pathLengthCheck = Folder::checkPathLength(path);
+    if (!pathLengthCheck) {
+        return pathLengthCheck.error();
+    }
+
+    // If this is a new folder, recurse up to the first parent that exists, to see if we can use that to create a new folder
+    const QFileInfo selectedPathInfo(path);
+    if (!selectedPathInfo.exists()) {
+        const QString parentPath = selectedPathInfo.path();
+        if (parentPath != path) {
+            return checkPathValidityRecursive(parentPath, folderType, accountUuid);
+        }
+        return FolderMan::tr("The selected path does not exist.");
+    }
+
+    // At this point we have an existing folder, so check if we can create files/folders here.
+    // Note: we don't need to write to any parent, so we don't need to check writablility when traversing the parents.
+    if (!selectedPathInfo.isWritable()) {
+        return FolderMan::tr("You have no permission to write to the selected folder.");
     }
-    return {};
+
+    // Now check if none of the parents is already used.
+    return checkPathForSyncRootMarkingRecursive(canonicalPath(path), folderType, accountUuid);
 }
 
 QString FolderMan::findGoodPathForNewSyncFolder(
@@ -906,7 +903,7 @@ QString FolderMan::findGoodPathForNewSyncFolder(
     {
         QString folder = normalisedPath;
         for (int attempt = 2; attempt <= 100; ++attempt) {
-            if (!QFileInfo::exists(folder) && FolderMan::instance()->checkPathValidityForNewFolder(folder, folderType, accountUuid).isEmpty()) {
+            if (!QFileInfo::exists(folder) && checkPathValidity(folder, folderType, accountUuid).isEmpty()) {
                 return canonicalPath(folder);
             }
             folder = normalisedPath + QStringLiteral(" (%1)").arg(attempt);
@@ -1050,7 +1047,7 @@ void FolderMan::addFolderFromGui(AccountState *accountState, const SyncConnectio
     setSyncEnabled(true);
 }
 
-QString FolderMan::suggestSyncFolder(NewFolderType folderType, const QUuid &accountUuid)
+QString FolderMan::suggestSyncFolder(NewFolderType folderType, const QUuid &accountUuid)//XXX
 {
     return FolderMan::instance()->findGoodPathForNewSyncFolder(QDir::homePath(), Theme::instance()->appName(), folderType, accountUuid);
 }

src/gui/folderman.h

@@ -85,7 +85,6 @@ class OWNCLOUDGUI_EXPORT FolderMan : public QObject
      * Or in case of a space folder, that if the new folder is in a Space sync root, it is the sync root of the same account.
      */
     enum class NewFolderType {
-        OC10SyncRoot, // todo: #43
         SpacesSyncRoot,
         SpacesFolder,
     };
@@ -130,8 +129,13 @@ class OWNCLOUDGUI_EXPORT FolderMan : public QObject
 
     static QString suggestSyncFolder(NewFolderType folderType, const QUuid &accountUuid);
 
-
-    static QString checkPathValidityRecursive(const QString &path, FolderMan::NewFolderType folderType, const QUuid &accountUuid);
+    /**
+     * @brief Check a path for validity.
+     *
+     * We do not allow putting a folder inside another folder that is already syncing to a server. We also disallow a space to be put into the default sync root
+     * of another (possibly branded) client, nor in the defaut sync root of another account in our own client.
+     */
+    static QString checkPathValidity(const QString &path, NewFolderType folderType, const QUuid &accountUuid);
 
     static std::unique_ptr<FolderMan> createInstance();
 
@@ -230,14 +234,6 @@ class OWNCLOUDGUI_EXPORT FolderMan : public QObject
 
     SocketApi *socketApi();
 
-    /**
-     * Check if @a path is a valid path for a new folder considering the already sync'ed items.
-     * Make sure that this folder, or any subfolder is not sync'ed already.
-     *
-     * @returns an empty string if it is allowed, or an error if it is not allowed
-     */
-    QString checkPathValidityForNewFolder(const QString &path, NewFolderType folderType, const QUuid &accountUuid) const;
-
     /**
      * Attempts to find a non-existing, acceptable path for creating a new sync folder.
      *
@@ -428,6 +424,8 @@ private Q_SLOTS:
     // pair this with _socketApi->slotUnregisterPath(folder);
     void registerFolderWithSocketApi(Folder *folder);
 
+    static QString checkPathValidityRecursive(const QString &path, NewFolderType folderType, const QUuid &accountUuid);
+
     QSet<Folder *> _disabledFolders;
 
     QString _folderConfigPath;

src/gui/folderwizard/folderwizard.cpp

@@ -59,17 +59,10 @@ QString FolderWizardPrivate::formatWarnings(const QStringList &warnings, bool is
     return ret;
 }
 
-// todo: #43
 QString FolderWizardPrivate::defaultSyncRoot() const
 {
-    // this should never happen when we have set up the account using spaces - there is ALWAYS a default root when spaces are in play
-    // and they are always in play so this check is bogus. todo: #43
-    if (!_account->hasDefaultSyncRoot()) {
-        const auto folderType = FolderMan::NewFolderType::SpacesSyncRoot; // todo: #43 : FolderMan::NewFolderType::OC10SyncRoot;
-        return FolderMan::suggestSyncFolder(folderType, _account->uuid());
-    } else {
-        return _account->defaultSyncRoot();
-    }
+    Q_ASSERT(_account->hasDefaultSyncRoot());
+    return _account->defaultSyncRoot();
 }
 
 QUuid FolderWizardPrivate::uuid() const

src/gui/folderwizard/folderwizardlocalpath.cpp

@@ -65,7 +65,7 @@ bool FolderWizardLocalPath::isComplete() const
 {
     auto folderType = FolderMan::NewFolderType::SpacesFolder;
     auto accountUuid = folderWizardPrivate()->uuid();
-    QString errorStr = FolderMan::instance()->checkPathValidityForNewFolder(localPath(), folderType, accountUuid);
+    QString errorStr = FolderMan::checkPathValidity(localPath(), folderType, accountUuid);
 
     bool isOk = errorStr.isEmpty();
     QStringList warnStrings;

src/gui/newaccountwizard/advancedsettingspagecontroller.cpp

@@ -226,7 +226,7 @@ bool AdvancedSettingsPageController::validateSyncRoot(const QString &rootPath)
         return false;
     }
 
-    QString invalidPathErrorMessage = FolderMan::checkPathValidityRecursive(rootPath, FolderMan::NewFolderType::SpacesSyncRoot, {});
+    QString invalidPathErrorMessage = FolderMan::checkPathValidity(rootPath, FolderMan::NewFolderType::SpacesSyncRoot, {});
     if (!invalidPathErrorMessage.isEmpty()) {
         _errorField->setText(errorMessageTemplate.arg(rootPath, invalidPathErrorMessage));
         return false;