Advanced account configuration for new wizard #9566

fmoc · 2022-04-05T16:24:50Z

Contributes to #9249.

This PR implements the remaining configuration options missing in the new wizard. It adds an hidden-by-default advanced options group box to the final wizard page.

I think we can just squash this PR entirely before merging it.

TheOneRing · 2022-04-07T12:25:25Z

Please add this pr to the original changelog entry.

Please call toNativeSeperators on the path.

fmoc · 2022-04-07T12:40:08Z

Also missing:

use a line edit instead of just a button for the local directory
theme option to hide advanced options completely
another confirmation dialog if VFS is an experimental feature

TheOneRing · 2022-04-07T12:56:25Z

Please check that those are supported:

client/src/libsync/theme.h

Lines 274 to 277 in 951432c

    
               /** 
        
                * Skip the advanced page and create a sync with the default settings 
        
                */ 
        
               virtual bool wizardSkipAdvancedPage() const;

client/src/libsync/theme.h

Lines 407 to 411 in 951432c

    
                * @brief Whether to show the option to create folders using "virtual files". 
        
                * 
        
                * By default, this is the same as enableExperimentalFreatures() 
        
                */ 
        
               virtual bool showVirtualFilesOption() const;

client/src/libsync/theme.h

Line 413 in 951432c

virtual bool forceVirtualFilesOption() const;

Only active if non experimental

fmoc · 2022-04-07T13:03:08Z

Can we rename those options easily, or should we rather keep the names and adjust the comments?

dragotin

Just a few thoughts

dragotin · 2022-04-07T13:10:33Z

src/gui/folderman.cpp

@@ -1588,25 +1589,40 @@ Folder *FolderMan::addFolder(AccountStatePtr accountStatePtr, const QString &loc
        folderDefinition.navigationPaneClsid = QUuid::createUuid();
 #endif

-    auto f = addFolder(accountStatePtr, folderDefinition);
+    auto finalize = [this, accountStatePtr, syncMode, localFolder](const FolderDefinition &folderDefinition, const QStringList &blacklist = {}) {
+        auto f = addFolder(accountStatePtr, folderDefinition);


One char variable names, really?

dragotin · 2022-04-07T13:13:43Z

src/gui/folderman.cpp

@@ -1588,25 +1589,40 @@ Folder *FolderMan::addFolder(AccountStatePtr accountStatePtr, const QString &loc
        folderDefinition.navigationPaneClsid = QUuid::createUuid();
 #endif

-    auto f = addFolder(accountStatePtr, folderDefinition);
+    auto finalize = [this, accountStatePtr, syncMode, localFolder](const FolderDefinition &folderDefinition, const QStringList &blacklist = {}) {


I know lambdas are cool.

But in this case (for example) it is just not needed imho. The reasons are:

The encapsulated code is just used once.

The code flow is cluttered because you just move code from the place where it is used to the beginning of the method. Why?

For me, this makes things just more confusing.

dragotin · 2022-04-07T13:23:24Z

src/gui/newwizard/pages/accountconfiguredwizardpage.cpp

+    }
+
+    connect(_ui->chooseLocalDirectoryButton, &QPushButton::clicked, this, [=]() {
+        auto dialog = new QFileDialog(this, tr("Choose"), defaultSyncTargetDir);


is that leaking? It is probably going to be removed with the parent class, but why not set Qt::WA_DeleteOnClose?

dragotin · 2022-04-07T13:26:22Z

src/gui/owncloudgui.cpp

    // ensure we are connected and fetch the capabilities
    auto validator = new ConnectionValidator(account, account.data());
-    QObject::connect(validator, &ConnectionValidator::connectionResult, account.data(), [accountStatePtr](ConnectionValidator::Status status, const QStringList &errors) {
+
+    QObject::connect(validator, &ConnectionValidator::connectionResult, account.data(), [accountStatePtr, localFolder, syncMode](ConnectionValidator::Status status, const QStringList &errors) {


In my book this should be a proper slot method. Too big for a lambda: When you read the surrounding method, you loose context over this huge lambda.

dragotin · 2022-04-07T13:28:44Z

src/gui/owncloudgui.cpp

-            // reenable sync, which is disabled while the wizard is shown
-            FolderMan::instance()->setSyncEnabled(true);
+        connect(_wizardController, &Wizard::SetupWizardController::finished, ocApp(),
+            [this](AccountPtr newAccount, const QString &localFolder, Wizard::SyncMode syncMode) {


same here. Separate slots are not forbidden ;-)

src/gui/newwizard/pages/accountconfiguredwizardpage.cpp

src/gui/newwizard/pages/accountconfiguredwizardpage.h

src/gui/newwizard/syncmode.h

src/gui/folderman.cpp

TheOneRing · 2022-04-11T10:51:49Z

Is enabling vfs in this context meant to work already?

fmoc · 2022-04-11T12:42:22Z

I quickly finished this and pushed the changes. I'll squash this PR soon.

fmoc · 2022-04-11T12:56:49Z

Seems like I broke the automatic merge. Will rebase on master and squash the history.

src/gui/newwizard/setupwizardcontroller.cpp

src/gui/newwizard/syncmode.cpp

TheOneRing · 2022-04-12T08:22:52Z

src/gui/newwizard/pages/accountconfiguredwizardpage.cpp

+    // this may be overwritten by a branding option (see below)
+    _ui->useVfsRadioButton->setEnabled(vfsIsAvailable);
+
+    _ui->useVfsRadioButton->setText(tr("Use &virtual files instead of downloading content immediately"));


Wrong default on Windows.

TheOneRing · 2022-04-12T08:26:01Z

src/gui/askexperimentalvirtualfilesfeaturemessagebox.cpp

+
+namespace OCC {
+
+AskExperimentalVirtualFilesFeatureMessageBox::AskExperimentalVirtualFilesFeatureMessageBox(QWidget *parent)


Don't ask if vfs is non experimental.

src/gui/newwizard/setupwizardcontroller.cpp

ownclouders · 2022-04-12T12:06:25Z

Results for GUI-tests https://drone.owncloud.com/owncloud/client/11400/6/1
💥 The GUI tests failed.
GUI Logs: (https://cache.owncloud.com/public/owncloud/client/11400/guiReportUpload/index.html)
Server Logs: (https://cache.owncloud.com/public/owncloud/client/11400/guiReportUpload/serverlog.log)
Screenshots:

The old confirmation dialog was located within the setup wizard, which did not make a lot of sense. Therefore, the old code was restored, but moved into a new dedicated class, which can be used from other places easily, too.

sonarcloud · 2022-04-12T12:44:10Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
7 Code Smells

0.0% Coverage
0.0% Duplication

fmoc requested a review from a team April 5, 2022 16:24

dragotin reviewed Apr 7, 2022

View reviewed changes

TheOneRing reviewed Apr 8, 2022

View reviewed changes

src/gui/newwizard/pages/accountconfiguredwizardpage.cpp Outdated Show resolved Hide resolved

TheOneRing reviewed Apr 8, 2022

View reviewed changes

fmoc force-pushed the work/wizard-vfs branch from d7897b6 to 4266b1a Compare April 8, 2022 14:25

fmoc requested a review from TheOneRing April 11, 2022 12:55

fmoc force-pushed the work/wizard-vfs branch 3 times, most recently from 8daba56 to d5f8d41 Compare April 11, 2022 19:07

fmoc mentioned this pull request Apr 11, 2022

Fix wizard navigation #9577

Merged

TheOneRing reviewed Apr 12, 2022

View reviewed changes

fmoc force-pushed the work/wizard-vfs branch from 67ecfcd to f2b8100 Compare April 12, 2022 11:38

fmoc requested a review from TheOneRing April 12, 2022 11:50

TheOneRing reviewed Apr 12, 2022

View reviewed changes

src/gui/newwizard/setupwizardcontroller.cpp Outdated Show resolved Hide resolved

fmoc force-pushed the work/wizard-vfs branch from 1ef0a96 to 72f4953 Compare April 12, 2022 12:08

Fabian Müller added 3 commits April 12, 2022 14:15

Restore VFS confirmation dialog in new class

2b76dc2

The old confirmation dialog was located within the setup wizard, which did not make a lot of sense. Therefore, the old code was restored, but moved into a new dedicated class, which can be used from other places easily, too.

Implement advanced sync options in wizard

1958981

Add new PR to existing changelog

9f6a57b

fmoc force-pushed the work/wizard-vfs branch from 72f4953 to 9f6a57b Compare April 12, 2022 12:15

TheOneRing merged commit 6f21ab6 into master Apr 12, 2022

delete-merged-branch bot deleted the work/wizard-vfs branch April 12, 2022 12:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Advanced account configuration for new wizard #9566

Advanced account configuration for new wizard #9566

fmoc commented Apr 5, 2022

TheOneRing commented Apr 7, 2022

fmoc commented Apr 7, 2022

TheOneRing commented Apr 7, 2022

fmoc commented Apr 7, 2022

dragotin left a comment

dragotin Apr 7, 2022

dragotin Apr 7, 2022

dragotin Apr 7, 2022

dragotin Apr 7, 2022

dragotin Apr 7, 2022

TheOneRing commented Apr 11, 2022

fmoc commented Apr 11, 2022 •

edited

Loading

fmoc commented Apr 11, 2022

TheOneRing Apr 12, 2022

TheOneRing Apr 12, 2022

ownclouders commented Apr 12, 2022 •

edited

Loading

sonarcloud bot commented Apr 12, 2022


		namespace OCC {

		AskExperimentalVirtualFilesFeatureMessageBox::AskExperimentalVirtualFilesFeatureMessageBox(QWidget *parent)

Advanced account configuration for new wizard #9566

Advanced account configuration for new wizard #9566

Conversation

fmoc commented Apr 5, 2022

TheOneRing commented Apr 7, 2022

fmoc commented Apr 7, 2022

TheOneRing commented Apr 7, 2022

fmoc commented Apr 7, 2022

dragotin left a comment

Choose a reason for hiding this comment

dragotin Apr 7, 2022

Choose a reason for hiding this comment

dragotin Apr 7, 2022

Choose a reason for hiding this comment

dragotin Apr 7, 2022

Choose a reason for hiding this comment

dragotin Apr 7, 2022

Choose a reason for hiding this comment

dragotin Apr 7, 2022

Choose a reason for hiding this comment

TheOneRing commented Apr 11, 2022

fmoc commented Apr 11, 2022 • edited Loading

fmoc commented Apr 11, 2022

TheOneRing Apr 12, 2022

Choose a reason for hiding this comment

TheOneRing Apr 12, 2022

Choose a reason for hiding this comment

ownclouders commented Apr 12, 2022 • edited Loading

sonarcloud bot commented Apr 12, 2022

fmoc commented Apr 11, 2022 •

edited

Loading

ownclouders commented Apr 12, 2022 •

edited

Loading