Allow compiling without QtWebEngine & remove webflow v1 #3353

maxcrees · 2021-05-24T09:50:10Z

Still enabled by default

Fixes: #856 #932
Supersedes: #1808 #2204
Signed-off-by: Max Rees maxcrees@me.com

Based on the feedback from #2204 I reworked things so that Flow2 auth is still usable. Indeed it does not require QtWebEngine at all; only Flow1 does. I think the areas I'm not sure about here are the two places where I fall back on using OAuth - should those be Flow2, OAuth, or Basic?

I wrote this based on the 3.2.1 release then rebased it onto master. I verified that Flow2 auth works from the 3.2.1 version. More testing would be appreciated.

maxcrees · 2021-05-24T11:00:26Z

Also, I'm not sure how to handle WebFlowCredentials::askFromUser. Once that's figured out, the following can be added as well for the sake of hygiene (though not strictly necessary):

diff --git a/src/gui/wizard/owncloudsetuppage.cpp b/src/gui/wizard/owncloudsetuppage.cpp
index 1d20e092d..b6dafbdc2 100644
--- a/src/gui/wizard/owncloudsetuppage.cpp
+++ b/src/gui/wizard/owncloudsetuppage.cpp
@@ -232,8 +232,10 @@ int OwncloudSetupPage::nextId() const
         return WizardCommon::Page_OAuthCreds;
     case DetermineAuthTypeJob::LoginFlowV2:
         return WizardCommon::Page_Flow2AuthCreds;
+#ifdef WITH_WEBENGINE
     case DetermineAuthTypeJob::WebViewFlow:
         return WizardCommon::Page_WebView;
+#endif
     }
     return WizardCommon::Page_HttpCreds;
 }
diff --git a/src/gui/wizard/owncloudwizardcommon.h b/src/gui/wizard/owncloudwizardcommon.h
index 30b4dfb80..a6908898b 100644
--- a/src/gui/wizard/owncloudwizardcommon.h
+++ b/src/gui/wizard/owncloudwizardcommon.h
@@ -46,7 +46,9 @@ namespace WizardCommon {
         Page_HttpCreds,
         Page_OAuthCreds,
         Page_Flow2AuthCreds,
+#ifdef WITH_WEBENGINE
         Page_WebView,
+#endif
         Page_AdvancedSetup,
         Page_Result
     };
diff --git a/src/libsync/networkjobs.h b/src/libsync/networkjobs.h
index 1b0e212d4..f1adf6743 100644
--- a/src/libsync/networkjobs.h
+++ b/src/libsync/networkjobs.h
@@ -446,7 +446,9 @@ public:
         NoAuthType, // used only before we got a chance to probe the server
         Basic, // also the catch-all fallback for backwards compatibility reasons
         OAuth,
+#ifdef WITH_WEBENGINE
         WebViewFlow,
+#endif
         LoginFlowV2
     };
     Q_ENUM(AuthType)

maxcrees · 2021-05-24T11:22:02Z

Likewise with WelcomePage::setupCreateAccountButton, but I think what can be done there is pop open https://nextcloud.com/register using the browser like the webview currently does. That would answer one of my earlier questions then - OwncloudSetupPage::slotGotoProviderList should then be using Flow2 instead of OAuth there if WITH_WEBENGINE is undefined.

Or we could just make WITH_PROVIDERS depend on WITH_WEBENGINE.

CMakeLists.txt

src/gui/CMakeLists.txt

src/gui/creds/webflowcredentials.cpp

src/gui/creds/webflowcredentialsdialog.cpp

src/gui/wizard/owncloudsetuppage.cpp

maxcrees · 2021-05-25T20:06:17Z

V2: incorporate feedback from @mgallien. Additionally, since the preferred_providers app currently only supports webflow v1 auth to my knowledge, make that option for the client (WITH_PROVIDERS) depend on webflow v1 being enabled. This prevents what would be a non-functional "Create account with Provider" button from being displayed.

maxcrees · 2021-05-25T23:16:05Z

V3: add missing includes for config.h.

FlexW · 2021-05-27T22:40:42Z

@tobiasKaminsky @mgallien should we really bother with introducing a compile option for flow1? I think it Flow1 is not used anymore. Should we then not drop it completely? This will also make the code easier to read and maintain.

FlexW · 2021-05-28T08:53:25Z

@sroracle After discussing internally: Could you remove the flow1 code entirely? We then only need the web engine for the providers page. When WITH_WEBENGINE is disabled, the providers page should be displayed in the users web browser instead of QWebEngine. You can use the function Utility::openBrowser() in src/gui/guiutility.cpp

maxcrees · 2021-05-28T12:02:44Z

Thanks for taking the time to discuss it. I will rework it next week.

mgallien

As explained by @FlexW , please modify the PR to remove WebFlow1 and request again review when you are done ?

maxcrees · 2021-06-02T20:19:23Z

V4: incorporate feedback from @FlexW @mgallien.
V5: rebase onto master.

maxcrees · 2021-06-03T01:03:11Z

V6: silence error from clang-tidy by removing no-longer-needed usage of DetermineAuthTypeJob.

The only component that still uses QtWebEngine at this point is the providers functionality (WITH_PROVIDERS). However, you can now also set WITH_WEBENGINE=OFF and the providers functionality (if enabled) will still work - it will just open a link in the external browser instead. Fixes: nextcloud#856 nextcloud#932 Supersedes: nextcloud#1808 nextcloud#2204 Signed-off-by: Max Rees <maxcrees@me.com>

nextcloud-desktop-bot · 2021-06-03T01:09:41Z

AppImage file: Nextcloud-PR-3353-68939a9f9b23bb5df3097a11e803c4e92bff4adf-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

FlexW · 2021-06-09T13:53:07Z

src/gui/creds/webflowcredentials.cpp

-        _askDialog->setInfo(msg);
+    QString msg = tr("You have been logged out of %1 as user %2. Please login again.")
+                      .arg(_account->displayName(), _user);
+    _askDialog->setInfo(msg);


Unrelated changes

FlexW · 2021-06-09T13:53:15Z

src/gui/creds/webflowcredentials.cpp


-        _askDialog->show();
+    _askDialog->show();


Unrelated changes

FlexW · 2021-06-09T14:02:46Z

src/gui/creds/webflowcredentials.cpp

-            QString path = url.path() + "/index.php/login/flow";
-            url.setPath(path);
-            _askDialog->setUrl(url);
-        }


Why did you remove the auth job? I think you shouldn't remove it.

As far as I can tell it was dead code since it's only needed to distinguish whether flowv1 is needed over v2. Do you want an error message or something in that case instead?

For me, it looks like it also checked if basic authentication is needed.

FlexW · 2021-06-09T14:08:00Z

I will have some more comments. I just couldn't finish it today.

FlexW · 2021-06-22T14:58:18Z

@sroracle I've realized that it might be not feasible at the moment to remove the flow1 entirely. There were some problems that I wasn't aware of. Sorry for this. To move this forward a little bit quicker I created another pull request that allows compiling without QWebEngine. We need that pull request to make it possible to compile native clients for Apple M1. I would like to close this pull request in favour of the other. I'm sorry that we then will not be able to merge your pull request, but your pull request helped me to get the other pull request implemented quicker. Please continue to contribute to the desktop client. If you need some inspiration for other issues you could solve, just tell me, I will then give you some ;)

New pull request: #3471

maxcrees requested review from er-vin and FlexW May 24, 2021 09:50

maxcrees linked an issue May 24, 2021 that may be closed by this pull request

Build without qt-webengine #932

Closed

mgallien requested changes May 25, 2021

View reviewed changes

mgallien added the 1. to develop label May 25, 2021

maxcrees force-pushed the no-webengine branch from 6f62acd to de9f403 Compare May 25, 2021 20:04

maxcrees force-pushed the no-webengine branch from de9f403 to 4adcedf Compare May 25, 2021 23:15

maxcrees requested a review from mgallien May 25, 2021 23:24

mgallien requested changes May 28, 2021

View reviewed changes

maxcrees force-pushed the no-webengine branch 2 times, most recently from 6430805 to 48ef852 Compare June 2, 2021 20:18

maxcrees requested a review from mgallien June 2, 2021 20:19

maxcrees changed the title ~~Allow compiling without QtWebEngine/webflow v1~~ Allow compiling without QtWebEngine & remove webflow v1 Jun 2, 2021

maxcrees force-pushed the no-webengine branch from 48ef852 to 68939a9 Compare June 3, 2021 01:02

FlexW requested changes Jun 9, 2021

View reviewed changes

FlexW closed this Jun 22, 2021

bill-auger mentioned this pull request Feb 9, 2023

makes Qt WebEngine optional only on macOS #4875

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow compiling without QtWebEngine & remove webflow v1 #3353

Allow compiling without QtWebEngine & remove webflow v1 #3353

maxcrees commented May 24, 2021

maxcrees commented May 24, 2021

maxcrees commented May 24, 2021

maxcrees commented May 25, 2021

maxcrees commented May 25, 2021

FlexW commented May 27, 2021

FlexW commented May 28, 2021

maxcrees commented May 28, 2021

mgallien left a comment

maxcrees commented Jun 2, 2021

maxcrees commented Jun 3, 2021

nextcloud-desktop-bot commented Jun 3, 2021

FlexW Jun 9, 2021

FlexW Jun 9, 2021

FlexW Jun 9, 2021

maxcrees Jun 14, 2021

FlexW Jun 15, 2021

FlexW commented Jun 9, 2021

FlexW commented Jun 22, 2021

Allow compiling without QtWebEngine & remove webflow v1 #3353

Allow compiling without QtWebEngine & remove webflow v1 #3353

Conversation

maxcrees commented May 24, 2021

maxcrees commented May 24, 2021

maxcrees commented May 24, 2021

maxcrees commented May 25, 2021

maxcrees commented May 25, 2021

FlexW commented May 27, 2021

FlexW commented May 28, 2021

maxcrees commented May 28, 2021

mgallien left a comment

Choose a reason for hiding this comment

maxcrees commented Jun 2, 2021

maxcrees commented Jun 3, 2021

nextcloud-desktop-bot commented Jun 3, 2021

FlexW Jun 9, 2021

Choose a reason for hiding this comment

FlexW Jun 9, 2021

Choose a reason for hiding this comment

FlexW Jun 9, 2021

Choose a reason for hiding this comment

maxcrees Jun 14, 2021

Choose a reason for hiding this comment

FlexW Jun 15, 2021

Choose a reason for hiding this comment

FlexW commented Jun 9, 2021

FlexW commented Jun 22, 2021