refactor method of checking path for vfs support

killed deadwood
renamed Vfs::checkAvailability and changed return type to simple string
updated callers and added copious todo's related to this topic of having local paths that don't support vfs even when vfs is otherwise available. See DC-219 for that

Author: modSpike

src/common/vfs.cpp

@@ -68,7 +68,7 @@ QString Utility::enumToString(Vfs::Mode mode)
     }
 }
 
-Result<void, QString> Vfs::checkAvailability(const QString &path, Vfs::Mode mode)
+QString Vfs::pathSupportDetail(const QString &path, Vfs::Mode mode)
 {
 #ifdef Q_OS_WIN
     if (mode == Mode::WindowsCfApi) {

src/common/vfs.h

@@ -125,7 +125,17 @@ class OCSYNC_EXPORT Vfs : public QObject
 
     static Mode modeFromString(const QString &str);
 
-    static Result<void, QString> checkAvailability(const QString &path, OCC::Vfs::Mode mode);
+    /**
+     * @brief pathSupportDetail - needs a better name but the concept is that this function checks to see if the path is supported for vfs.
+     * if it is not supported the return string contains the "reason" it's not supported.
+     * @param path to check to for support
+     * @param mode which we want to check - note that the only arg that makes sense here at all is Vfs::WindowsCfApi
+     * @return empty string if the mode is supported on the given path. If the string is not empty, it contains a string explaining the
+     * reason the mode is not supported.
+     *
+     * as of this writing, the possible root reasons are: path is a drive, path is on a network drive, path fs is not ntfs.
+     */
+    static QString pathSupportDetail(const QString &path, OCC::Vfs::Mode mode);
 
     enum class AvailabilityError
     {

src/gui/FoldersGui/accountfolderscontroller.cpp

@@ -161,35 +161,35 @@ void AccountFoldersController::buildMenuActions()
     itemActions.push_back(_chooseSync);
     connect(_chooseSync, &QAction::triggered, this, &AccountFoldersController::onChooseSync);
 
-
-    // Only show "Enable VFS" if a VFS mode is available
-    /*  const auto mode = VfsPluginManager::instance().bestAvailableVfsMode();
-      if (mode == Vfs::WindowsCfApi) {
-          _enableVfs = new QAction(tr("Enable virtual file support"), this);
-
-          if (FolderMan::instance()->checkVfsAvailability(folder->path(), mode)) {
-              if (mode == Vfs::WindowsCfApi) {
-                  QAction *enableVfsAction = menu->addAction(tr("Enable virtual file support"));
-                  connect(enableVfsAction, &QAction::triggered, this, [folder, this] { slotEnableVfsCurrentFolder(folder); });
-              }
-          }
-      };
-
-      if (Theme::instance()->showVirtualFilesOption()) {
-          if (Theme::instance()->forceVirtualFilesOption()) {
-              if (!folder->virtualFilesEnabled()) {
-                  // VFS is currently disabled, but is forced on by theming (e.g. due to a theme change)
-                  maybeShowEnableVfs();
+    /* auto maybeShowEnableVfs = [folder, menu, this]() {
+        // Only show "Enable VFS" if a VFS mode is available
+          const auto mode = VfsPluginManager::instance().bestAvailableVfsMode();
+          if (mode == Vfs::WindowsCfApi) {
+              _enableVfs = new QAction(tr("Enable virtual file support"), this);
+
+              if (FolderMan::instance()->checkVfsAvailability(folder->path(), mode)) {
+                  if (mode == Vfs::WindowsCfApi) {
+                      QAction *enableVfsAction = menu->addAction(tr("Enable virtual file support"));
+                      connect(enableVfsAction, &QAction::triggered, this, [folder, this] { slotEnableVfsCurrentFolder(folder); });
+                  }
               }
-          } else {
-              if (folder->virtualFilesEnabled()) {
-                  menu->addAction(tr("Disable virtual file support"), this, [folder, this] { slotDisableVfsCurrentFolder(folder); });
+          };
+
+          if (Theme::instance()->showVirtualFilesOption()) {
+              if (Theme::instance()->forceVirtualFilesOption()) {
+                  if (!folder->virtualFilesEnabled()) {
+                      // VFS is currently disabled, but is forced on by theming (e.g. due to a theme change)
+                      maybeShowEnableVfs();
+                  }
               } else {
-                  maybeShowEnableVfs();
+                  if (folder->virtualFilesEnabled()) {
+                      menu->addAction(tr("Disable virtual file support"), this, [folder, this] { slotDisableVfsCurrentFolder(folder); });
+                  } else {
+                      maybeShowEnableVfs();
+                  }
               }
-          }
-      _enableVfs = new QAction(this);
-    */
+          _enableVfs = new QAction(this);
+        */
 
     connect(_view, &AccountFoldersView::requestActionsUpdate, this, &AccountFoldersController::updateActions);
     _view->setFolderActions(itemActions);
@@ -234,10 +234,10 @@ void AccountFoldersController::onEnableVfs()
     if (!_currentFolder || _currentFolder->virtualFilesEnabled() || VfsPluginManager::instance().bestAvailableVfsMode() != Vfs::WindowsCfApi)
         return;
 
-    qCInfo(lcAccountView) << "Enabling vfs support for folder" << folder->path();
+    qCInfo(lcAccountFoldersController) << "Enabling vfs support for folder" << _currentFolder->path();
 
     // Change the folder vfs mode and load the plugin
-    folder->setVirtualFilesEnabled(true);
+    _currentFolder->setVirtualFilesEnabled(true);
 }
 
 void AccountFoldersController::onDisableVfs()
@@ -288,9 +288,9 @@ void AccountFoldersController::updateActions()
 
     _removeSync->setEnabled(_currentFolder);
 
-    if (_toggleVfs) {
-        _toggleVfs->setText(_currentFolder && _currentFolder->virtualFilesEnabled() ? tr("Disable virtual file support") : tr("Enable virtual file support");
-        _toggleVfs->setEnabled();
+    if (_enableVfs) {
+        _enableVfs->setText(_currentFolder && _currentFolder->virtualFilesEnabled() ? tr("Disable virtual file support") : tr("Enable virtual file support"));
+        _enableVfs->setEnabled(_currentFolder->canSync());
     }
 
     _chooseSync->setEnabled(_currentFolder && _currentFolder->isAvailable() && !_currentFolder->virtualFilesEnabled());

src/gui/accountsettings.cpp

@@ -226,7 +226,7 @@ void AccountSettings::slotCustomContextMenuRequested(Folder *folder)
             auto maybeShowEnableVfs = [folder, menu, this]() {
                 // Only show "Enable VFS" if a VFS mode is available
                 const auto mode = VfsPluginManager::instance().bestAvailableVfsMode();
-                if (FolderMan::instance()->checkVfsAvailability(folder->path(), mode)) {
+                if (Vfs::pathSupportDetail(folder->path(), mode).isEmpty()) {
                     if (mode == Vfs::WindowsCfApi) {
                         QAction *enableVfsAction = menu->addAction(tr("Enable virtual file support"));
                         connect(enableVfsAction, &QAction::triggered, this, [folder, this] { slotEnableVfsCurrentFolder(folder); });

src/gui/folder.cpp

@@ -513,9 +513,14 @@ void Folder::startVfs()
     OC_ENFORCE(_vfs);
     OC_ENFORCE(_vfs->mode() == _definition.virtualFilesMode());
 
-    const auto result = Vfs::checkAvailability(path(), _vfs->mode());
-    if (!result) {
-        _syncResult.appendErrorString(result.error());
+    // todo: DC-219 check to see if this sync error makes it to the gui or not. We need to more clearly inform the user that they should
+    // create a new sync connection to the remote folder, and use improved checks in the folder wizard to make sure vfs is supported
+    // on the chosen path.
+    // ideally we want a check like this before we even get here, eg in the area of load folder from config - but need to be sure there is
+    // a place to clearly instruct the user that the folder sync needs to be re-built with a valid local path
+    QString result = Vfs::pathSupportDetail(path(), _vfs->mode());
+    if (!result.isEmpty()) {
+        _syncResult.appendErrorString(result);
         setSyncState(SyncResult::SetupError);
         return;
     }

src/gui/folderman.h

@@ -259,12 +259,6 @@ class OWNCLOUDGUI_EXPORT FolderMan : public QObject
 
     void setDirtyProxy();
 
-    /** Whether or not vfs is supported in the location. */
-    bool checkVfsAvailability(const QString &path, Vfs::Mode mode = VfsPluginManager::instance().bestAvailableVfsMode()) const;
-
-    /** If the folder configuration is no longer supported this will return an error string */
-    Result<void, QString> unsupportedConfiguration(const QString &path) const;
-
     [[nodiscard]] bool isSpaceSynced(GraphApi::Space *space) const;
 
     /**

src/gui/folderstatusmodel.cpp

@@ -212,10 +212,6 @@ QVariant FolderStatusModel::data(const QModelIndex &index, int role) const
 
     auto getErrors = [f] {
         auto errors = f->syncResult().errorStrings();
-        const Result<void, QString> notLegacyError = FolderMan::instance()->unsupportedConfiguration(f->path());
-        if (!notLegacyError) {
-            errors.append(notLegacyError.error());
-        }
         if (f->syncResult().hasUnresolvedConflicts()) {
             errors.append(tr("There are unresolved conflicts."));
         }

src/gui/folderwizard/folderwizard.cpp

@@ -102,7 +102,7 @@ FolderWizardPrivate::FolderWizardPrivate(FolderWizard *q, Account *account)
 QString FolderWizardPrivate::initialLocalPath() const
 {
     return FolderMan::instance()->findGoodPathForNewSyncFolder(
-        defaultSyncRoot(), _spacesPage->currentSpace()->displayName(), FolderMan::NewFolderType::SpacesSyncRoot, _account->uuid());
+        defaultSyncRoot(), _spacesPage->currentSpace()->displayName(), FolderMan::NewFolderType::SpacesFolder, _account->uuid());
 }
 
 uint32_t FolderWizardPrivate::priority() const
@@ -131,12 +131,16 @@ QString FolderWizardPrivate::displayName() const
 
 bool FolderWizardPrivate::useVirtualFiles() const
 {
+    // todo: see other todo's related to DC-219. Basically we should simply not allow user to pick a local path that is not supported
+    // for vfs if vfs is generally available. "Use virtual files" should not rely on the folder path check at all, as we should
+    // block use of unsupported paths from the start (generally via account wizard and folder wizard gui's)
     const auto mode = VfsPluginManager::instance().bestAvailableVfsMode();
     const bool useVirtualFiles = (Theme::instance()->forceVirtualFilesOption() && mode == Vfs::WindowsCfApi) || (_folderWizardSelectiveSyncPage && _folderWizardSelectiveSyncPage->useVirtualFiles());
     if (useVirtualFiles) {
-        const auto availability = Vfs::checkAvailability(initialLocalPath(), mode);
-        if (!availability) {
-            auto msg = new QMessageBox(QMessageBox::Warning, FolderWizard::tr("Virtual files are not available for the selected folder"), availability.error(), QMessageBox::Ok, ocApp()->gui()->settingsDialog());
+        QString vfsSupported = Vfs::pathSupportDetail(initialLocalPath(), mode);
+        if (!vfsSupported.isEmpty()) {
+            auto msg = new QMessageBox(QMessageBox::Warning, FolderWizard::tr("Virtual files are not available for the selected folder"), vfsSupported,
+                QMessageBox::Ok, ocApp()->gui()->settingsDialog());
             msg->setAttribute(Qt::WA_DeleteOnClose);
             msg->open();
             return false;
@@ -166,8 +170,11 @@ void FolderWizard::sendResult()
 FolderMan::SyncConnectionDescription FolderWizard::result()
 {
     Q_D(FolderWizard);
-
+    // DC-219 todo:
+    // check this local path to see if it supports vfs if vfs is in play - this check should happen before the wizard is completed,
+    //     eg validate whatever page has the user choice of local path
     const QString localPath = d->_folderWizardSourcePage->localPath();
+    // leave this until we fix the weird handling in account wizard that leaves the sync root empty if selective sync is chosen
     if (!d->_account->hasDefaultSyncRoot()) {
         if (FileSystem::isChildPathOf(localPath, d->defaultSyncRoot())) {
             d->_account->setDefaultSyncRoot(d->defaultSyncRoot());

src/gui/folderwizard/folderwizardlocalpath.cpp

@@ -63,6 +63,10 @@ QString FolderWizardLocalPath::localPath() const
 
 bool FolderWizardLocalPath::isComplete() const
 {
+    // todo: DC-219 we need a check here to see if the chosen local folder path supports vfs, provided vfs is generally available,
+    // and reject the path with reason it failed if it's no good. Regardless of whether the user currently wants to use vfs or not,
+    // we should not accept local paths that *can't* support vfs if they change their mind later.
+    // use Vfs::pathSupportDetail to get the reason vfs is not supported (path is supported if the return string is empty)
     auto folderType = FolderMan::NewFolderType::SpacesFolder;
     auto accountUuid = folderWizardPrivate()->uuid();
     QString errorStr = FolderMan::instance()->checkPathValidity(localPath(), folderType, accountUuid);

src/gui/newaccountwizard/advancedsettingspagecontroller.cpp

@@ -235,9 +235,13 @@ bool AdvancedSettingsPageController::validateSyncRoot(const QString &rootPath)
     // I'm not testing the case that vfs is actually the chosen mode here as if vfs is available we should just block dirs that don't support it,
     // the idea being that eg if they change the mode in the page after they select the dir, or use selective sync with vfs later, at least we know the default
     // root they select here will not cause any issues down the road
-    if (_vfsIsAvailable && !FolderMan::instance()->checkVfsAvailability(rootPath)) {
-        _errorField->setText(errorMessageTemplate.arg(rootPath, tr("selected path does not support using virtual file system.")));
-        return false;
+    // todo: use this same approach when fixing the folder wizard as part of DC-219
+    if (_vfsIsAvailable) {
+        QString pathSupported = Vfs::pathSupportDetail(rootPath, Vfs::Mode::WindowsCfApi);
+        if (!pathSupported.isEmpty()) {
+            _errorField->setText(errorMessageTemplate.arg(rootPath, tr("selected path does not support using virtual file system. %1").arg(pathSupported)));
+            return false;
+        }
     }
 
     return true;