Collection of work to improve efficiency of app (#12081)

* example cases for enhancing handling of previous "assert" conditions

* started collecting migration related code to a single function in FolderMan named migrateFolderDefinition
improved loading from config to only save the extracted FolderDefintions if they were migrated

started prepping for deeper cuts related to Folder::saveToSettings, plus overhaul of impl and uses of FolderMan::addFolderFromFolderWizardResult and addFolderFromWizard

* fix for failing unit tests. Hopefully fixes squish tests too.

* many changes to give FolderDefinition a clear public interface with accessors, all members are now private, FolderMan has been removed as friend

refactored addFolderInternal -> moved the small amount of actual folder adding code out and renamed it connectFolder. Also refactored unloadFolder -> moved some parts to removeFolderSync (formerly removeFolder which was easily confused with internal remove operations) and renamed it to disconnectFolder to match connectFolder activity.

put an end to the extra careful handling of "unexpected" Vfs modes - basically now if the mode isn't among our legit set, it just assumes VfsMode = off

added loads of todo's and other notes for next steps

* BAH! I didn't commit these = failing build :(

* first stab at consolidating all code paths to use addFolder

* woops - missed this small but important change before last merge

* corrected "ok" param logic for SyncJournalDb::getSelectiveSyncList

ok param originally took a pointer to bool which makes no sense since it's a simple flag value, and most importantly not allowed to be nullptr. It should therefore be a ref.

* added many Refactoring todo's

Refactored the auto-loading of spaces/ocis folders on adding account. Moved the bulk of the activity to FolderMan where it belongs.
saving is still wonky and done in the folder sometimes but now I should be able to finally finish this persistence task now that loading spaces/folders from account is in the FolderMan.

* fixed the saving on adding folders - finally!
need to validate the auto-save on changing pause or vfs mode as I think we will still get too many saves when enabling/disabling vfs in the Folder but otherwise it's in good shape now
last steps are to relocate the removeFromSettings operations from Folder to FolderMan

* saves are now updated - all folder persistence is managed by FolderMan now
there are a couple of wonky bits but they should be ironed out in future when we convert direct calls from Gui to FolderMan to signals requesting xyz change
I have to do one more pass comparing functionality in AddFolderFromWizared to AddFolder, will tidy up and finally rename AddFolderFromWizard to something more sensible

* this is as done as I can get it for now.

main todo is to discuss some points with Erik to funny finish this.

* updated some comments + removed handling for legacy folder config groups (they were already migrated/eliminated in 5.0)

* home stretch. Erik and I worked out the strategy for scheduling folders for sync and calling setSyncEnabled(false/true) depending on activity *in* the FolderMan, so there is not some third party making the decisions.

Unfortunately I was not able to fully eliminate calls to FolderMan::setSycnEnabled and make it private as it is still used in the SpaceMigration routine. That needs deep review/refactoring so I'm leaving it alone for now.

Also refactored adding account folders from config one last time. It's now renamed addFoldersFromConfigByAccount and does just that.

* worked out the final open questions with Erik so this is "done" in terms of planned intent.

I still have to debug/fix the squish test failure on ocis

* found an error from last refactoring of FolderMan::setupFoldersFromConfig - all better now.

Also fixed broken connections that trigger saving a folder to config after change in pause or vfs mode at runtime

* addressed Erik's requested review changes

* this should fix the current squish test failures - revisit once qa issue #12136 is resolved

* removed tests for obsolete/no longer supported folder migrations

Author: modSpike

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);