Collection of work to improve efficiency of app

src/cmd/cmd.cpp

@@ -91,7 +91,7 @@ void selectiveSyncFixup(OCC::SyncJournalDb *journal, const QSet<QString> &newLis
 
     bool ok;
 
-    const auto oldBlackListSet = journal->getSelectiveSyncList(SyncJournalDb::SelectiveSyncBlackList, &ok);
+    const auto oldBlackListSet = journal->getSelectiveSyncList(SyncJournalDb::SelectiveSyncBlackList, ok);
     if (ok) {
         const auto changes = (oldBlackListSet - newListSet) + (newListSet - oldBlackListSet);
         for (const auto &it : changes) {

src/common/restartmanager.cpp

@@ -32,7 +32,8 @@ RestartManager::RestartManager(std::function<int(int, char **)> &&main)
     : _main(main)
 {
     Q_ASSERT(!_instance);
-    _instance = this;
+    if (!_instance)
+        _instance = this;
 }
 
 RestartManager::~RestartManager()
@@ -59,6 +60,9 @@ int RestartManager::exec(int argc, char **argv) const
 void RestartManager::requestRestart()
 {
     Q_ASSERT(_instance);
+    if (!_instance)
+        return;
+
     qCInfo(lcRestart) << "Restarting application with PID" << QCoreApplication::applicationPid();
 
     QString pathToLaunch = QCoreApplication::applicationFilePath();

src/common/syncjournaldb.cpp

@@ -1705,32 +1705,31 @@ void SyncJournalDb::setErrorBlacklistEntry(const SyncJournalErrorBlacklistRecord
     query->exec();
 }
 
-QSet<QString> SyncJournalDb::getSelectiveSyncList(SyncJournalDb::SelectiveSyncListType type, bool *ok)
+QSet<QString> SyncJournalDb::getSelectiveSyncList(SyncJournalDb::SelectiveSyncListType type, bool &ok)
 {
     QSet<QString> result;
-    OC_ASSERT(ok);
 
     QMutexLocker locker(&_mutex);
     if (!checkConnect()) {
-        *ok = false;
+        ok = false;
         return result;
     }
 
     const auto query = _queryManager.get(PreparedSqlQueryManager::GetSelectiveSyncListQuery, QByteArrayLiteral("SELECT path FROM selectivesync WHERE type=?1"), _db);
     if (!query) {
-        *ok = false;
+        ok = false;
         return result;
     }
 
     query->bindValue(1, int(type));
     if (!query->exec()) {
-        *ok = false;
+        ok = false;
         return result;
     }
     while (true) {
         auto next = query->next();
         if (!next.ok) {
-            *ok = false;
+            ok = false;
             return result;
         }
         if (!next.hasData)
@@ -1742,7 +1741,7 @@ QSet<QString> SyncJournalDb::getSelectiveSyncList(SyncJournalDb::SelectiveSyncLi
         }
         result.insert(entry);
     }
-    *ok = true;
+    ok = true;
 
     return result;
 }

src/common/syncjournaldb.h

@@ -168,8 +168,11 @@ class OCSYNC_EXPORT SyncJournalDb : public QObject
     };
     Q_ENUM(SelectiveSyncListType)
 
-    /* return the specified list from the database */
-    QSet<QString> getSelectiveSyncList(SelectiveSyncListType type, bool *ok);
+    /* return the specified list from the database
+     * ok param holds value indicating "success" of the operation. If the sync list cannot be retrieved ok will be false
+     */
+    QSet<QString> getSelectiveSyncList(SelectiveSyncListType type, bool &ok);
+
     /* Write the selective sync list (remove all other entries of that list */
     void setSelectiveSyncList(SelectiveSyncListType type, const QSet<QString> &list);
 

src/common/vfs.cpp

@@ -47,18 +47,17 @@ QString Vfs::underlyingFileName(const QString &fileName) const
 
 Vfs::~Vfs() = default;
 
-Optional<Vfs::Mode> Vfs::modeFromString(const QString &str)
+Vfs::Mode Vfs::modeFromString(const QString &str)
 {
     // Note: Strings are used for config and must be stable
     // keep in sync with: QString Utility::enumToString(Vfs::Mode mode)
-    if (str == QLatin1String("off")) {
-        return Off;
-    } else if (str == QLatin1String("suffix")) {
+    // which is defined BELOW
+    if (str == QLatin1String("suffix")) {
         return WithSuffix;
     } else if (str == QLatin1String("wincfapi")) {
         return WindowsCfApi;
     }
-    return {};
+    return Off;
 }
 
 template <>
@@ -72,9 +71,9 @@ QString Utility::enumToString(Vfs::Mode mode)
     case Vfs::Mode::WindowsCfApi:
         return QStringLiteral("wincfapi");
     case Vfs::Mode::Off:
+    default:
         return QStringLiteral("off");
     }
-    Q_UNREACHABLE();
 }
 
 Result<void, QString> Vfs::checkAvailability(const QString &path, Vfs::Mode mode)
@@ -242,7 +241,8 @@ Vfs::Mode OCC::VfsPluginManager::bestAvailableVfsMode() const
     } else if (isVfsPluginAvailable(Vfs::Off)) {
         return Vfs::Off;
     }
-    Q_UNREACHABLE();
+
+    return Vfs::Off;
 }
 
 std::unique_ptr<Vfs> OCC::VfsPluginManager::createVfsFromPlugin(Vfs::Mode mode) const

src/common/vfs.h

@@ -123,7 +123,7 @@ class OCSYNC_EXPORT Vfs : public QObject
     };
     Q_ENUM(ConvertToPlaceholderResult)
 
-    static Optional<Mode> modeFromString(const QString &str);
+    static Mode modeFromString(const QString &str);
 
     static Result<void, QString> checkAvailability(const QString &path, OCC::Vfs::Mode mode);
 

src/gui/accountmanager.cpp

@@ -239,6 +239,7 @@ void AccountManager::saveAccount(Account *account, bool saveCredentials)
         }
 
         // HACK: Save http_user also as user
+        // Refactoring todo: is this still valid? I don't find uses of httpUserC() aside from this instance
         if (account->_settingsMap.contains(httpUserC()))
             settings->setValue(userC(), account->_settingsMap.value(httpUserC()));
     }
@@ -429,7 +430,7 @@ AccountStatePtr AccountManager::addAccountState(std::unique_ptr<AccountState> &&
     // this slot can't be connected until the account state exists because saveAccount uses the state
     connect(rawAccount, &Account::wantsAccountSaved, this, [rawAccount, this] {
         // persis the account, not the credentials, we don't know whether they are ready yet
-        // Refactor todo: how about we make those two completely different saves? then we can ditch this lambda
+        // Refactoring todo: how about we make those two completely different saves? then we can ditch this lambda
         saveAccount(rawAccount, false);
     });
 

src/gui/accountsettings.cpp

@@ -55,6 +55,11 @@ namespace OCC {
 
 Q_LOGGING_CATEGORY(lcAccountSettings, "gui.account.settings", QtInfoMsg)
 
+// Refactoring todo: devise a correct handling of null account state in this ctr.
+// also move all this connect stuff to a "connect" method.
+// also ditch the lambdas which should actually be functions (private if necessary)
+// Also refactoring todo: split the controller behavior out into a controller. A widget should NOT contain
+// business or controller logic!
 AccountSettings::AccountSettings(const AccountStatePtr &accountState, QWidget *parent)
     : QWidget(parent)
     , ui(new Ui::AccountSettings)
@@ -67,9 +72,6 @@ AccountSettings::AccountSettings(const AccountStatePtr &accountState, QWidget *p
     // the QPointer properly, but as a stopgap to catch null states asap before they trickle down into other areas:
     Q_ASSERT(_accountState);
 
-    // Refactor todo: devise a correct handling of null account state in this ctr.
-    // also move all this connect stuff to a "connect" method.
-    // also ditch the lambdas which should actually be functions (private if necessary)
 
     _model = new FolderStatusModel(this);
 
@@ -93,6 +95,9 @@ AccountSettings::AccountSettings(const AccountStatePtr &accountState, QWidget *p
     connect(_accountState.data(), &AccountState::stateChanged, this, &AccountSettings::slotAccountStateChanged);
     slotAccountStateChanged();
 
+    // Refactoring todo: eval why this is created/destructed each time the user invokes the manageAccount button. 
+    // normal impl creates actions as members of either gui, or better, a controller, and controller updates action states
+    // in line with updates it receives.
     connect(ui->manageAccountButton, &QToolButton::clicked, this, [this] {
         QMenu *menu = new QMenu(this);
         menu->setAttribute(Qt::WA_DeleteOnClose);
@@ -194,7 +199,7 @@ void AccountSettings::slotCustomContextMenuRequested(Folder *folder)
     }
 
     // Add an action to open the folder on the server in a webbrowser:
-    // Refactor todo: why are we using the folder accountState AND the local member? shouldn't the folder have the same account state
+    // Refactoring todo: why are we using the folder accountState AND the local member? shouldn't the folder have the same account state
     // as this settings panel?!
     if (folder->accountState()->account()->capabilities().privateLinkPropertyAvailable()) {
         QString path = folder->remotePathTrailingSlash();
@@ -274,7 +279,7 @@ void AccountSettings::showSelectiveSyncDialog(Folder *folder)
     selectiveSync->setDavUrl(folder->webDavUrl());
     bool ok;
     selectiveSync->setFolderInfo(
-        folder->remotePath(), folder->displayName(), folder->journalDb()->getSelectiveSyncList(SyncJournalDb::SelectiveSyncBlackList, &ok));
+        folder->remotePath(), folder->displayName(), folder->journalDb()->getSelectiveSyncList(SyncJournalDb::SelectiveSyncBlackList, ok));
     Q_ASSERT(ok);
 
     auto *modalWidget = new AccountModalWidget(tr("Choose what to sync"), selectiveSync, this);
@@ -292,15 +297,12 @@ void AccountSettings::slotAddFolder()
         return;
     }
 
-    FolderMan::instance()->setSyncEnabled(false); // do not start more syncs.
-
     FolderWizard *folderWizard = new FolderWizard(_accountState, this);
     folderWizard->setAttribute(Qt::WA_DeleteOnClose);
 
     connect(folderWizard, &QDialog::accepted, this, &AccountSettings::slotFolderWizardAccepted);
     connect(folderWizard, &QDialog::rejected, this, [] {
         qCInfo(lcAccountSettings) << "Folder wizard cancelled";
-        FolderMan::instance()->setSyncEnabled(true);
     });
 
     addModalLegacyDialog(folderWizard, AccountSettings::ModalWidgetSizePolicy::Expanding);
@@ -314,20 +316,21 @@ void AccountSettings::slotFolderWizardAccepted()
     }
 
     FolderWizard *folderWizard = qobject_cast<FolderWizard *>(sender());
-    qCInfo(lcAccountSettings) << "Folder wizard completed";
-
-    const auto config = folderWizard->result();
+    if (!folderWizard)
+        return;
 
-    auto folder = FolderMan::instance()->addFolderFromFolderWizardResult(_accountState, config);
+    qCInfo(lcAccountSettings) << "Folder wizard completed";
 
-    if (!config.selectiveSyncBlackList.isEmpty() && OC_ENSURE(folder && !config.useVirtualFiles)) {
-        folder->journalDb()->setSelectiveSyncList(SyncJournalDb::SelectiveSyncBlackList, config.selectiveSyncBlackList);
+    auto config = folderWizard->result();
 
-        // The user already accepted the selective sync dialog. everything is in the white list
-        folder->journalDb()->setSelectiveSyncList(SyncJournalDb::SelectiveSyncWhiteList, {QLatin1String("/")});
+    // The gui should not allow users to selectively choose any sync lists if vfs is enabled, but this kind of check was
+    // originally in play here so...keep it just in case.
+    if (config.useVirtualFiles && !config.selectiveSyncBlackList.empty()) {
+        config.selectiveSyncBlackList.clear();
     }
-    FolderMan::instance()->setSyncEnabled(true);
-    FolderMan::instance()->scheduleAllFolders();
+
+    // Refactoring todo: turn this into a signal/requestAddFolder
+    FolderMan::instance()->addFolderFromGui(_accountState, config);
 }
 
 void AccountSettings::slotRemoveCurrentFolder(Folder *folder)
@@ -345,7 +348,15 @@ void AccountSettings::slotRemoveCurrentFolder(Folder *folder)
     messageBox->addButton(tr("Cancel"), QMessageBox::NoRole);
     connect(messageBox, &QMessageBox::finished, this, [messageBox, yesButton, folder, this] {
         if (messageBox->clickedButton() == yesButton) {
-            FolderMan::instance()->removeFolder(folder);
+            // Refactoring todo: this should just emit a signal to request removing the folder - let the FolderMan do the right stuff
+            FolderMan::instance()->removeFolderSettings(folder);
+            FolderMan::instance()->removeFolderSync(folder);
+            // Refactoring todo: I don't understand why this is called when a folder is removed as the slot seems to be more
+            // geared to loading/adding newly discovered spaces *when the spaces manager notifies something has changed*
+            // instead we seem to "conveniently" recycle the meaning of the handler to cover other changes as well.
+            // also, I *really* don't understand why we are using a single shot timer to "schedule" this in the main event
+            // loop when we are already in the main event loop aren't we (ie we are IN a gui slot already)? I am seeing this
+            // a lot and so far have no explanation as to what the reason or intent is.
             QTimer::singleShot(0, this, &AccountSettings::slotSpacesUpdated);
         }
     });
@@ -362,9 +373,6 @@ void AccountSettings::slotEnableVfsCurrentFolder(Folder *folder)
 
         // Change the folder vfs mode and load the plugin
         folder->setVirtualFilesEnabled(true);
-
-        // don't schedule the folder, it might not be ready yet.
-        // it will schedule its self once set up
     }
 }
 
@@ -599,18 +607,24 @@ void AccountSettings::slotSpacesUpdated()
     }
 
     // Check if we should add new spaces automagically, or only signal that there are unsynced spaces.
+    // Refactoring todo answer to above: we should not be loading spaces in any gui. Instead I would expect that the gui merely updates
+    // it's view/state in response to new or removed spaces. The clue is that the main actor here is FolderMan - connect FolderMan to whoever
+    // emits the newly discovered spaces (or whatever this is) and let it deal with it.
+    // A wrinkle here is that this slot is called in several places in the gui, apparently just to trigger refresh or similar? Not good!
     if (Theme::instance()->syncNewlyDiscoveredSpaces()) {
+        // Refactoring todo: why is this scheduled for "later" on the main event loop? aren't we already there?
+        // where does this slot run if not on the main thread?
+        // what needs to be processed "before" this loading routine that requires scheduling it for later?
+        // if anything we should consider running the loading routines in a worker thread to avoid *blocking* the main
+        // event loop.
         QTimer::singleShot(0, this, [this, unsycnedSpaces]() {
-            // Refactor todo: I see zero reason to make a copy of the member. Just use the member!
-            auto accountStatePtr = _accountState;
-
             for (GraphApi::Space *newSpace : unsycnedSpaces) {
                 // TODO: Problem: when a space is manually removed, this will re-add it!
                 qCInfo(lcAccountSettings) << "Adding sync connection for newly discovered space" << newSpace->displayName();
 
                 const QString localDir(_accountState->account()->defaultSyncRoot());
                 const QString folderName = FolderMan::instance()->findGoodPathForNewSyncFolder(
-                    localDir, newSpace->displayName(), FolderMan::NewFolderType::SpacesFolder, accountStatePtr->account()->uuid());
+                    localDir, newSpace->displayName(), FolderMan::NewFolderType::SpacesFolder, _accountState->account()->uuid());
 
                 FolderMan::SyncConnectionDescription fwr;
                 fwr.davUrl = QUrl(newSpace->drive().getRoot().getWebDavUrl());
@@ -619,7 +633,7 @@ void AccountSettings::slotSpacesUpdated()
                 fwr.displayName = newSpace->displayName();
                 fwr.useVirtualFiles = Utility::isWindows() ? Theme::instance()->showVirtualFilesOption() : false;
                 fwr.priority = newSpace->priority();
-                FolderMan::instance()->addFolderFromFolderWizardResult(accountStatePtr, fwr);
+                FolderMan::instance()->addFolderFromGui(_accountState, fwr);
             }
 
             _unsyncedSpaces = 0;

src/gui/accountstate.cpp

@@ -633,10 +633,16 @@ bool AccountState::isSettingUp() const
     return _settingUp;
 }
 
+// Refactoring todo: We currently call this from owncloudgui to set the val true then false at a later point
+// actual consumer is in the AccountSettings gui - it shows a spinny from the time it's set to
+// true to when it switches to false. IMO this does NOT belong in the AccountState if it can't
+// manage the value itself. It needs to go elsewhere, ideally the owncloudgui can just call
+// into the account settings directly as any good controller would
 void AccountState::setSettingUp(bool settingUp)
 {
     if (_settingUp != settingUp) {
         _settingUp = settingUp;
+        // for goodness sake, just send the new value
         Q_EMIT isSettingUpChanged();
     }
 }

src/gui/application.cpp

@@ -103,6 +103,11 @@ Application::Application(Platform *platform, const QString &displayLanguage, boo
 
     connect(FolderMan::instance()->socketApi(), &SocketApi::shareCommandReceived, _gui.data(), &ownCloudGui::slotShowShareDialog);
 
+    // Refactoring example: this is oversimplified and really belongs in a dedicated app builder impl but the idea is illustrated:
+    // don't handling everything "locally" -> request that the best entity for the job do it. Then make the proper connections between
+    // requestor and responsible handler in a clearly defined, central location (eg an app builder, but for now, this will do)
+    connect(_gui, &ownCloudGui::requestSetUpSyncFoldersForAccount, FolderMan::instance(), &FolderMan::setUpInitialSyncFolders);
+
 #ifdef WITH_AUTO_UPDATER
     // Update checks
     UpdaterScheduler *updaterScheduler = new UpdaterScheduler(this, this);