refactor: [DC-257] Folder to ensure it always has a sync engine

src/common/filesystembase.cpp

@@ -141,16 +141,7 @@ void FileSystem::setFileReadOnly(const QString &filename, bool readonly)
     file.setPermissions(permissions);
 }
 
-void FileSystem::setFolderMinimumPermissions(const QString &filename)
-{
-#ifdef Q_OS_MAC
-    QFile::Permissions perm = QFile::ReadOwner | QFile::WriteOwner | QFile::ExeOwner;
-    QFile file(filename);
-    file.setPermissions(perm);
-#else
-    Q_UNUSED(filename);
-#endif
-}
+void FileSystem::setFolderMinimumPermissions(const QString &filename) { }
 
 
 void FileSystem::setFileReadOnlyWeak(const QString &filename, bool readonly)

src/common/filesystembase.h

@@ -54,8 +54,7 @@ namespace FileSystem {
      * List of characters not allowd in filenames on Windows
      */
     constexpr_list auto IllegalFilenameCharsWindows = {
-        QLatin1Char('\\'), QLatin1Char(':'), QLatin1Char('?'), QLatin1Char('*'), QLatin1Char('"'), QLatin1Char('>'), QLatin1Char('<'), QLatin1Char('|')
-    };
+        QLatin1Char('\\'), QLatin1Char(':'), QLatin1Char('?'), QLatin1Char('*'), QLatin1Char('"'), QLatin1Char('>'), QLatin1Char('<'), QLatin1Char('|')};
 
     /**
      * @brief Mark the file as hidden  (only has effects on windows)

src/common/syncjournaldb.h

@@ -46,7 +46,7 @@ class OCSYNC_EXPORT SyncJournalDb : public QObject
 {
     Q_OBJECT
 public:
-    explicit SyncJournalDb(const QString &dbFilePath, QObject *parent = nullptr);
+    explicit SyncJournalDb(const QString &dbFilePath, QObject *parent);
     ~SyncJournalDb() override;
 
     /// Create a journal path for a specific configuration

src/common/vfs.cpp

@@ -236,7 +236,7 @@ Vfs::Mode OCC::VfsPluginManager::bestAvailableVfsMode() const
     return Vfs::Off;
 }
 
-std::unique_ptr<Vfs> OCC::VfsPluginManager::createVfsFromPlugin(Vfs::Mode mode) const
+Vfs *OCC::VfsPluginManager::createVfsFromPlugin(Vfs::Mode mode, QObject *parent) const
 {
     auto name = Utility::enumToString(mode);
     if (name.isEmpty())
@@ -261,7 +261,7 @@ std::unique_ptr<Vfs> OCC::VfsPluginManager::createVfsFromPlugin(Vfs::Mode mode)
         return nullptr;
     }
 
-    auto vfs = std::unique_ptr<Vfs>(qobject_cast<Vfs *>(factory->create(nullptr)));
+    Vfs *vfs = qobject_cast<Vfs *>(factory->create(parent));
     if (!vfs) {
         qCCritical(lcPlugin) << "Plugin" << loader.fileName() << "does not create a Vfs instance";
         return nullptr;

src/common/vfs.h

@@ -141,7 +141,7 @@ class OCSYNC_EXPORT Vfs : public QObject
     using AvailabilityResult = Result<VfsItemAvailability, AvailabilityError>;
 
 public:
-    explicit Vfs(QObject* parent = nullptr);
+    explicit Vfs(QObject *parent);
     ~Vfs() override;
 
     virtual Mode mode() const = 0;
@@ -302,7 +302,7 @@ class OCSYNC_EXPORT VfsPluginManager
     Vfs::Mode bestAvailableVfsMode() const;
 
     /// Create a VFS instance for the mode, returns nullptr on failure.
-    std::unique_ptr<Vfs> createVfsFromPlugin(Vfs::Mode mode) const;
+    Vfs *createVfsFromPlugin(Vfs::Mode mode, QObject *parent) const;
 
     static const VfsPluginManager &instance();
 

src/gui/CMakeLists.txt

@@ -118,6 +118,7 @@ add_subdirectory(newaccountwizard)
 add_subdirectory(socketapi)
 add_subdirectory(spaces)
 add_subdirectory(FoldersGui)
+add_subdirectory(foldermanagement)
 
 target_include_directories(owncloudGui PUBLIC
     ${CMAKE_SOURCE_DIR}/src/3rdparty/QProgressIndicator

src/gui/folder.h

@@ -45,10 +45,12 @@ class SyncRunFileLog;
 class FolderWatcher;
 class LocalDiscoveryTracker;
 
+
 /**
  * @brief The FolderDefinition class
  * @ingroup gui
  */
+
 class OWNCLOUDGUI_EXPORT FolderDefinition
 {
 public:
@@ -83,9 +85,12 @@ class OWNCLOUDGUI_EXPORT FolderDefinition
     void setVirtualFilesMode(Vfs::Mode mode) { _virtualFilesMode = mode; }
 
     /// Ensure / as separator and trailing /.
+    /// calling set here also ensures the canonical local path is properly created
     void setLocalPath(const QString &path);
     QString localPath() const { return _localPath; }
 
+    QString canonicalPath() const { return _canonicalLocalPath; }
+
 
     /// Remove ending /, then ensure starting '/': so "/foo/bar" and "/".
     void setTargetPath(const QString &path);
@@ -141,6 +146,8 @@ class OWNCLOUDGUI_EXPORT FolderDefinition
     QString _displayName;
     /// path on local machine (always trailing /)
     QString _localPath;
+    /// canonical local path
+    QString _canonicalLocalPath;
     /// path on remote (usually no trailing /, exception "/")
     QString _targetPath;
     bool _deployed = false;
@@ -174,11 +181,10 @@ class OWNCLOUDGUI_EXPORT Folder : public QObject
     };
     Q_ENUM(ChangeReason)
 
-    static void prepareFolder(const QString &path);
 
     /** Create a new Folder
      */
-    Folder(const FolderDefinition &definition, AccountState *accountState, std::unique_ptr<Vfs> &&vfs, bool ignoreHiddenFiles, QObject *parent = nullptr);
+    Folder(const FolderDefinition &definition, AccountState *accountState, SyncJournalDb *journal, Vfs *vfs, bool ignoreHiddenFiles, QObject *parent);
 
     ~Folder() override;
     /**
@@ -224,7 +230,7 @@ class OWNCLOUDGUI_EXPORT Folder : public QObject
     /**
      * canonical local folder path, always ends with /
      */
-    QString path() const { return _canonicalLocalPath; }
+    QString path() const { return _definition.canonicalPath(); }
 
     /**
      * cleaned canonical folder path, like path() but never ends with a /
@@ -304,10 +310,7 @@ class OWNCLOUDGUI_EXPORT Folder : public QObject
 
 
     // TODO: don't expose
-    SyncJournalDb *journalDb()
-    {
-        return &_journal;
-    }
+    SyncJournalDb *journalDb() { return _journal; }
     // TODO: don't expose
     SyncEngine &syncEngine()
     {
@@ -353,8 +356,6 @@ class OWNCLOUDGUI_EXPORT Folder : public QObject
 
     uint32_t sortPriority() const { return _definition.priority(); }
 
-    static Result<void, QString> checkPathLength(const QString &path);
-
     /**
      *
      * @return The corresponding space object or null
@@ -462,8 +463,6 @@ private Q_SLOTS:
 private:
     void showSyncResultPopup();
 
-    bool checkLocalPath();
-
     SyncOptions loadSyncOptions();
 
     /**
@@ -497,7 +496,7 @@ private Q_SLOTS:
 
     QPointer<AccountState> _accountState;
     FolderDefinition _definition;
-    QString _canonicalLocalPath; // As returned with QFileInfo:canonicalFilePath.  Always ends with "/"
+    // QString _canonicalLocalPath; // As returned with QFileInfo:canonicalFilePath.  Always ends with "/"
 
     SyncResult _syncResult;
     QScopedPointer<SyncEngine> _engine;
@@ -514,7 +513,7 @@ private Q_SLOTS:
     /// Reset when no follow-up is requested.
     int _consecutiveFollowUpSyncs = 0;
 
-    mutable SyncJournalDb _journal;
+    SyncJournalDb *_journal = nullptr;
 
     QScopedPointer<SyncRunFileLog> _fileLog;
 
@@ -562,6 +561,8 @@ private Q_SLOTS:
     // it is also false that it is never null - it is reset in wipeForRemoval
     // extra fun is I have no idea what happens to the instance in the SyncOptions - is it still alive relative to the engine?
     // I don't see any handling of the engine or SyncOptions whatsoever in wipeForRemoval so we'll need to go spelunking.
-    QSharedPointer<Vfs> _vfs;
+    // final endpoint is to make this a raw pointer, pass it around and let the deps wrap it in a qpointer
+    // reconsider parenting before this step is taken
+    Vfs *_vfs;
 };
 }

src/gui/folderman.cpp

@@ -14,13 +14,15 @@
 
 #include "folderman.h"
 
+#include "accessmanager.h"
 #include "account.h"
 #include "accountmanager.h"
 #include "accountstate.h"
-#include "accessmanager.h"
 #include "common/asserts.h"
 #include "configfile.h"
 #include "folder.h"
+#include "foldermanagement/folderbuilder.h"
+#include "foldermanagement/foldermanagementutils.h"
 #include "gui/networkinformation.h"
 #include "guiutility.h"
 #include "libsync/syncengine.h"
@@ -45,13 +47,11 @@ using namespace std::chrono;
 using namespace std::chrono_literals;
 
 namespace {
-qsizetype numberOfSyncJournals(const QString &path)
-{
-    return QDir(path).entryList({QStringLiteral(".sync_*.db"), QStringLiteral("._sync_*.db")}, QDir::Hidden | QDir::Files).size();
-}
 }
 
 namespace OCC {
+
+
 Q_LOGGING_CATEGORY(lcFolderMan, "gui.folder.manager", QtInfoMsg)
 
 void TrayOverallStatusResult::addResult(Folder *f)
@@ -242,6 +242,7 @@ bool FolderMan::addFoldersFromConfigByAccount(QSettings &settings, AccountState
 
         Folder *folder = addFolder(account, folderDefinition);
         if (!folder) {
+            // todo: decide if we should actively remove the folder data from the config! I think we should but let's see
             continue;
         }
 
@@ -306,7 +307,7 @@ void FolderMan::loadSpacesAndCreateFolders(AccountState *accountState, bool useV
         // prepare the root - reality check this as I think the user can change this from default? Yes they can but not for auto-loaded
         // folders eg on account creation, which is what is happening here.
         const QString localDir(accountState->account()->defaultSyncRoot());
-        if (!prepareFolder(localDir)) {
+        if (!FolderManagementUtils::prepareFolder(localDir)) {
             return;
         }
 
@@ -699,6 +700,15 @@ bool FolderMan::validateFolderDefinition(const FolderDefinition &folderDefinitio
 {
     if (folderDefinition.id().isEmpty() || folderDefinition.journalPath().isEmpty() || !ensureFilesystemSupported(folderDefinition))
         return false;
+
+    QString pathCheck = FolderManagementUtils::validateFolderPath(folderDefinition.localPath());
+    if (!pathCheck.isEmpty()) {
+        // does this warrant popping an error dialog?
+        qCWarning(lcFolderMan) << "Folder definition path check failed with error: " << pathCheck;
+        return false;
+    }
+
+
     return true;
 }
 
@@ -716,18 +726,18 @@ Folder *FolderMan::addFolder(AccountState *accountState, const FolderDefinition
     QUuid accountId = accountState->account()->uuid();
 
     if (Folder *f = folder(accountId, folderDefinition.spaceId())) {
-        qCWarning(lcFolderMan) << "Trying to add folder" << folderDefinition.localPath() << "but it already exists in folder list";
+        qCWarning(lcFolderMan) << "Trying to add new folder for " << folderDefinition.localPath() << "but it already exists in folder list";
         return f;
     }
 
-    auto vfs = VfsPluginManager::instance().createVfsFromPlugin(folderDefinition.virtualFilesMode());
-    if (!vfs) {
-        qCWarning(lcFolderMan) << "Could not load plugin for mode" << folderDefinition.virtualFilesMode();
+    FolderBuilder builder(folderDefinition);
+    auto folder = builder.buildFolder(accountState, _ignoreHiddenFiles, this);
+
+    if (!folder) {
+        qCWarning(lcFolderMan) << "Unable to create Folder for " << folder->path() << " with spaceId " << folderDefinition.spaceId();
         return nullptr;
     }
 
-    auto folder = new Folder(folderDefinition, accountState, std::move(vfs), _ignoreHiddenFiles, this);
-
     qCInfo(lcFolderMan) << "Adding folder to Folder Map " << folder << folder->path();
     QString spaceId = folder->spaceId();
     // always add the folder even if it had a setup error - future add special handling for incomplete folders if possible
@@ -965,7 +975,7 @@ static QString checkPathForSyncRootMarkingRecursive(const QString &path, FolderM
         return FolderMan::tr("The selected path is not a folder.");
     }
 
-    if (numberOfSyncJournals(selectedPathInfo.filePath()) != 0) {
+    if (FolderManagementUtils::numberOfSyncJournals(selectedPathInfo.filePath()) != 0) {
         return FolderMan::tr("The folder %1 is used in a folder sync connection.").arg(QDir::toNativeSeparators(selectedPathInfo.filePath()));
     }
 
@@ -1048,9 +1058,9 @@ QString FolderMan::findExistingFolderAndCheckValidity(const QString &path, NewFo
     QNtfsPermissionCheckGuard ntfs_perm;
 #endif
 
-    auto pathLengthCheck = Folder::checkPathLength(path);
-    if (!pathLengthCheck) {
-        return pathLengthCheck.error();
+    auto pathLengthCheck = FolderManagementUtils::checkPathLength(path);
+    if (!pathLengthCheck.isEmpty()) {
+        return pathLengthCheck;
     }
 
     // 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
@@ -1148,7 +1158,7 @@ void FolderMan::slotReloadSyncOptions()
 
 Folder *FolderMan::addFolderFromScratch(AccountState *accountState, FolderDefinition &&folderDefinition, bool useVfs)
 {
-    if (!FolderMan::prepareFolder(folderDefinition.localPath())) {
+    if (!FolderManagementUtils::prepareFolder(folderDefinition.localPath())) {
         return nullptr;
     }
 
@@ -1170,9 +1180,9 @@ Folder *FolderMan::addFolderFromScratch(AccountState *accountState, FolderDefini
         if (newFolder->vfs().mode() != Vfs::WindowsCfApi) {
             Utility::setupFavLink(folderDefinition.localPath());
         }
-        qCDebug(lcFolderMan) << "Local sync folder" << folderDefinition.localPath() << "successfully created!";
+        qCDebug(lcFolderMan) << "Local sync folder" << folderDefinition.localPath() << "successfully created";
     } else {
-        qCWarning(lcFolderMan) << "Failed to create local sync folder!";
+        qCWarning(lcFolderMan) << "Failed to create local sync folder: " << folderDefinition.localPath();
     }
 
     // we should not emit any folder list change from this function because it can be called in bulk as well as individual operations
@@ -1218,19 +1228,7 @@ QString FolderMan::suggestSyncFolder(NewFolderType folderType, const QUuid &acco
     return FolderMan::instance()->findGoodPathForNewSyncFolder(QDir::homePath(), Theme::instance()->appName(), folderType, accountUuid);
 }
 
-bool FolderMan::prepareFolder(const QString &folder)
-{
-    if (!QFileInfo::exists(folder)) {
-        if (!OC_ENSURE(QDir().mkpath(folder))) {
-            return false;
-        }
-        // mac only
-        FileSystem::setFolderMinimumPermissions(folder);
-        // this is for windows - it sets up a desktop.ini file to handle the icon and deals with persmissions.
-        Folder::prepareFolder(folder);
-    }
-    return true;
-}
+
 
 std::unique_ptr<FolderMan> FolderMan::createInstance()
 {

src/gui/folderman.h

@@ -52,6 +52,7 @@ class TrayOverallStatusResult
     SyncResult _overallStatus;
 };
 
+
 /**
  * @brief The FolderMan class
  * @ingroup gui
@@ -168,13 +169,16 @@ class OWNCLOUDGUI_EXPORT FolderMan : public QObject
     std::optional<qsizetype> setupFoldersFromConfig();
 
     /**
+     *
+     *  Extremely important refactoring todo: addFolder should not be public!
+     *  It is currently "required" for some tests which is not really cool, as it does not represent a complete/standalone impl.
+     *
      *  core step in any add folder routine. it validates the definition, instantiates vfs, instantiates the folder and validates whether
      *  it had setup errors.
      *
-     *  it is up to the caller to connect the folder, save it to settings, etc.
+     *  it is up to the caller to create the local sync folder (corresponding to the local path in the def) using the new FolderManagementUtils::prepareFolder,
+     *  connect the folder, save it to settings, etc.
      *
-     *  Refactoring todo: this should not be public! it is currently "required" for some tests which is not really cool, as it does not represent
-     *  a complete/standalone impl.
      */
     Folder *addFolder(AccountState *accountState, const FolderDefinition &folderDefinition);
 
@@ -356,13 +360,6 @@ private Q_SLOTS:
 private:
     explicit FolderMan();
 
-    /**
-     * @brief prepareFolder sets up the folder with mac and windows specific operations
-     * @param folder path
-     * @return true if the folder path exists or can be successfully created
-     */
-    [[nodiscard]] static bool prepareFolder(const QString &folder);
-
     /**
      * Adds a folder "from scratch" as oppossd to from config, which requires less setup than when you create the folder
      * from some dynamic operation (eg folders from new account or via the gui add folder sync operations).