Improve wizard ui #2895

FlexW · 2021-02-01T19:10:52Z

Resolves #2720

Signed-off-by: Felix Weilbach [email protected]

FlexW · 2021-02-01T22:51:19Z

Screencast: https://cloud.nextcloud.com/s/aX7mxczzxxngW8f

Screenshots:
https://cloud.nextcloud.com/s/pN6XtqJsTCQyHKA
https://cloud.nextcloud.com/s/ALWWTLSRgjDtSSn
https://cloud.nextcloud.com/s/ffzr3kW2Lj4Qyrz
https://cloud.nextcloud.com/s/JbsXTr7Wm42XdFo
https://cloud.nextcloud.com/s/yAa33WZLtKjasTF

er-vin

I'm so glad to see the welcome page split out!

One thing for later: this PR would have benefited from being more than one commit, roughly one commit per sub-task in the issue it solves. Oh well...

src/gui/wizard/owncloudadvancedsetuppage.h

src/gui/wizard/owncloudwizard.cpp

src/gui/wizard/owncloudwizardcommon.cpp

src/libsync/theme.cpp

src/gui/wizard/owncloudwizard.cpp

FlexW · 2021-02-02T19:36:09Z

Screenshots:

https://cloud.nextcloud.com/s/PwEPeriyBxGEJYK
https://cloud.nextcloud.com/s/sjQtQPnLgSS8xCo
https://cloud.nextcloud.com/s/XojA5LmK6YKCs3t
https://cloud.nextcloud.com/s/qPnz28CgrYY5Zys
https://cloud.nextcloud.com/s/kDXM3DdpNrjnX8Z

FlexW · 2021-02-03T14:49:54Z

Screenshots:

https://cloud.nextcloud.com/s/QwN5G7d3AnRQRxC
https://cloud.nextcloud.com/s/JsMQWKBaPCZtE5B
https://cloud.nextcloud.com/s/LNFsFMTJrrgRpzY
https://cloud.nextcloud.com/s/RGAKQ9R52K7r7qX
https://cloud.nextcloud.com/s/m8fNfKR542jLKDZ
https://cloud.nextcloud.com/s/e4SY8zoXY2E76wq

jancborchardt · 2021-02-03T18:49:09Z

Wow, really great now @FlexW! :) I’d only say to slightly reduce the size of the two-way arrow, and then it’s good to go!

er-vin · 2021-02-04T07:33:20Z

src/gui/wizard/owncloudadvancedsetuppage.cpp

+void OwncloudAdvancedSetupPage::setupSyncModeLabel()
+{
+    auto sizePolicy = _ui.syncModeLabel->sizePolicy();
+    sizePolicy.setRetainSizeWhenHidden(true);


Just wondering, wouldn't doing this at the page level be enough instead of doing it per widget?

If I do this on the page the layout of the other pages changes too

Well, isn't it what we want? Because we want all the pages to have the same size in the end to avoid resizes on step changes? So wouldn't that lead to not needing the adjustWizardSize in the wizard class?

Sorry I wasn't specific enough. If I set the size policy on the OwncloudAdvancedSetupPage int the constructor then I get a incorrect layout on all the other pages. For example https://cloud.nextcloud.com/s/g5maTToEcwERY3t . I'm unsure what here is happening.

Hm... looks like it's overconstrained somehow... Or that one of the components claims a too small size... would be a good target for investigating with Gammaray.

I admit I'm still dismayed this needs to be done like this for individual widgets, I'd expect there's a better way... as I said I'd have expected it to be enough on individual pages. But if that's really not the case it seems to be on most widgets in each page, so maybe use findChildren<QWidget> and apply it to them all in a loop? Not super in love with that but would at least remove some of the noise related to this...

@FlexW I agree with @er-vin. It would be nice to have it somewhere in a single place.

I agree with you both. I try to fix that.

After some more investigation I'm still unable to solve that. Are you sure that it would be enough to set the size policy on the page level because from my observations in gammaray the size policy got not propagated to the child widgets and I couldn't find any documentation that states something else. However, I simplified the setting of the size policy a little bit so that it makes less noise.

src/gui/wizard/owncloudwizard.cpp

src/libsync/theme.cpp

src/gui/wizard/owncloudadvancedsetuppage.cpp

src/gui/wizard/owncloudsetuppage.cpp

FlexW · 2021-02-08T13:57:51Z

Screenshot:

https://cloud.nextcloud.com/s/G7kLCgwkia7G7Xp

jancborchardt

Looks great design-wise! :) Nice work

src/gui/wizard/owncloudwizard.cpp

src/gui/wizard/welcomepage.cpp

src/libsync/theme.cpp

test/themeutils.cpp

test/testtheme.cpp

NEXTCLOUD.cmake

src/gui/wizard/owncloudadvancedsetuppage.cpp

src/libsync/theme.cpp

src/libsync/theme.h

test/themeutils.h

er-vin

I think it's time to try to simplify a bit that history (which is not the same thing as squashing into a single commit). I admit that at that point I'm merely glancing over because I'm overwhelmed. A few logically consistent commits I could cling on to see the steps you're going through would help me.

src/gui/owncloudsetupwizard.cpp

src/gui/wizard/owncloudadvancedsetuppage.cpp

src/gui/wizard/owncloudwizard.cpp

src/gui/wizard/owncloudwizardcommon.cpp

src/gui/wizard/owncloudwizard.cpp

er-vin · 2021-02-22T13:44:04Z

src/gui/wizard/owncloudwizardcommon.cpp

+    void customizeSecondaryButtonStyle(QAbstractButton *button)
+    {
+#ifdef Q_OS_LINUX
+        // Only style push buttons on Linux as on other platforms we are unable to style the background color


What? This sounds surprising to me... Where is it coming from?

Yes, in the past we used style sheets to make the buttons blue. But if we use style sheets, the buttons do not look native anymore as can be seen on the current wizard in OSX and Windows.

See also the warning here: https://doc.qt.io/qt-5/qpalette.html#details
I guess this warning applies to Windows 10 as well but really if you know how to change the background color of push buttons with the palette on Windows and OSX I would love to hear that:) I searched quite a while for a solution...

Yes, in the past we used style sheets to make the buttons blue. But if we use style sheets, the buttons do not look native anymore as can be seen on the current wizard in OSX and Windows.

Yes, that's one of the reasons why I don't like stylesheets.

See also the warning here: https://doc.qt.io/qt-5/qpalette.html#details
I guess this warning applies to Windows 10 as well but really if you know how to change the background color of push buttons with the palette on Windows and OSX I would love to hear that:) I searched quite a while for a solution...

Well let's say that the thing is: you should modify the palette as intended and accept that it might be ignored by some styles (actually even on Linux depending the style the user set it might be ignored). Clearly this doesn't warrant all those ifdefs. And I'd argue it's the same for the spin box by the way.

Just do what's semantically right with the API whatever the platform or the style. Otherwise you're overfitting your code to a particular set of styles and if for some reason that changes things will break in unexpected ways.

And I know that designers might not like that as an answer they don't get 100% the style expected but with widgets based there's a price to the desktop integration it's that you need to abandon some freedom to the platform. This is kind of intended.

And I know that designers might not like that as an answer they don't get 100% the style expected but with widgets based there's a price to the desktop integration it's that you need to abandon some freedom to the platform. This is kind of intended.

I'm with you with that :)

The problem was that when you, for example, set the font to white, you get also a white font on the buttons (If I remember correctly) but the background color of the button does not change to your expected background color (blue for example). Now you can't read the text on the buttons anymore. This is especially an issue for us because we allow our customers to brand their application. Now when they choose a dark background and a light font color they don't see the text anymore on the buttons.

In my opinion, we should stay away from modifying the palette. We would have a lot less trouble, and the application would look native (I would argue that users love that).

EDIT: One thing we can do to remove the ifdefs is doing on Linux the same as on Windows and OSX.

@er-vin I removed the ifdefs as far as possible

src/gui/wizard/owncloudwizardcommon.cpp

src/gui/wizard/welcomepage.h

src/gui/wizard/owncloudwizard.cpp

src/gui/wizard/owncloudadvancedsetuppage.cpp

er-vin · 2021-02-22T13:58:23Z

src/gui/wizard/owncloudadvancedsetuppage.cpp

+void OwncloudAdvancedSetupPage::setupSyncModeLabel()
+{
+    auto sizePolicy = _ui.syncModeLabel->sizePolicy();
+    sizePolicy.setRetainSizeWhenHidden(true);


I admit I'm still dismayed this needs to be done like this for individual widgets, I'd expect there's a better way... as I said I'd have expected it to be enough on individual pages. But if that's really not the case it seems to be on most widgets in each page, so maybe use findChildren<QWidget> and apply it to them all in a loop? Not super in love with that but would at least remove some of the noise related to this...

src/gui/wizard/owncloudsetuppage.cpp

src/gui/wizard/owncloudwizard.cpp

src/gui/wizard/owncloudsetuppage.cpp

allexzander · 2021-02-23T12:11:54Z

src/gui/wizard/owncloudsetuppage.cpp

@@ -269,7 +250,7 @@ bool OwncloudSetupPage::validatePage()
        QString u = url();
        QUrl qurl(u);
        if (!qurl.isValid() || qurl.host().isEmpty()) {
-            setErrorString(tr("Invalid URL"), false);
+            setErrorString(tr("Server address does not seem to be valid"), false);


@FlexW Really up to you to decide, but, I think, having it as simple as Server address is invalid would do the trick :)

Was a suggestion by @jancborchardt . I'm fine with both:) Jan what do you think?

src/gui/wizard/owncloudwizard.cpp

src/gui/wizard/welcomepage.cpp

er-vin · 2021-02-25T17:38:04Z

src/gui/wizard/linklabel.h

+
+namespace OCC {
+
+class LinkLabel : public QLabel


I'd argue you don't need that class at all, QLabel supports URLs just fine:
https://doc.qt.io/qt-5/qlabel.html#linkActivated

I created this class because of the hover event. I needed to underline the font on hover. I couldn't find an API in QLabel that allows me to underline the label just on hover. There is QLabel::linkHover() but with that, you get just the hover entry signal not the hover out signal. Please correct me if I'm wrong ;)

Honestly I'd consider like it's overdoing it and I'd be fine to have the text underlined. I'd rather spare the extra class with debatable API just for that. :-)

Now if really... one thing which could be done is to use an event filter to catch the enter/leave events and force the style on the label. We're back at having an extra class though, it's just that it'd have a safer API in my views. Indeed it'd be more focused on "change some property of a widget when it is entered, restore it when it's left" so it's easier to have a focused API.

@er-vin
It was wished by the design team ;)

One problem I see with not making an extra class is how to embed the link into the label? I know I can use rich text for that but when rich text is used the font of the label stays not white anymore it uses a different color. Now I know I can work around this by setting the font in the rich text again but it gets then a lot more complicated than it is now. Another possibility would be to create another event filter or even use the HoverEventFilter and rename it to LinkLabelEventFilter but I'm unsure if this brings any benefit to what we have now. What do you think?

@er-vin
It was wished by the design team ;)

Not argument enough in my opinion if that leads to potentially harmful code piling up. ;-)

(not that this single occurrence is super harmful of course, but my experience is that unchallenged design decisions lead to a path of death by 1000 cuts, hence why the question should be at least asked, every single time)

One problem I see with not making an extra class is how to embed the link into the label? I know I can use rich text for that but when rich text is used the font of the label stays not white anymore it uses a different color. Now I know I can work around this by setting the font in the rich text again but it gets then a lot more complicated than it is now. Another possibility would be to create another event filter or even use the HoverEventFilter and rename it to LinkLabelEventFilter but I'm unsure if this brings any benefit to what we have now. What do you think?

Alright, I prefer the event filter but I see you'll be struggling with that one. Let's scrap that...

Well the main problem really is the QLabel inheritance. I'd be fine even with inheriting from QWidget and have a single QLabel as child (and in a private member). Design wise the problem is that this is still a QLabel with all its API being exposed to the users of LinkLabel. Delegation is way better than inheritance in most cases, here included.

OK... too bad this went in unsolved. Is there any PR I should look forward to regarding this?

@er-vin No not at the moment but I like to do better here. I will create one and let you know then.

src/gui/wizard/owncloudwizard.cpp

er-vin · 2021-02-25T18:02:43Z

src/gui/wizard/owncloudwizardcommon.cpp

+    void customizeSecondaryButtonStyle(QAbstractButton *button)
+    {
+#ifdef Q_OS_LINUX
+        // Only style push buttons on Linux as on other platforms we are unable to style the background color


Yes, in the past we used style sheets to make the buttons blue. But if we use style sheets, the buttons do not look native anymore as can be seen on the current wizard in OSX and Windows.

Yes, that's one of the reasons why I don't like stylesheets.

See also the warning here: https://doc.qt.io/qt-5/qpalette.html#details
I guess this warning applies to Windows 10 as well but really if you know how to change the background color of push buttons with the palette on Windows and OSX I would love to hear that:) I searched quite a while for a solution...

Well let's say that the thing is: you should modify the palette as intended and accept that it might be ignored by some styles (actually even on Linux depending the style the user set it might be ignored). Clearly this doesn't warrant all those ifdefs. And I'd argue it's the same for the spin box by the way.

Just do what's semantically right with the API whatever the platform or the style. Otherwise you're overfitting your code to a particular set of styles and if for some reason that changes things will break in unexpected ways.

And I know that designers might not like that as an answer they don't get 100% the style expected but with widgets based there's a price to the desktop integration it's that you need to abandon some freedom to the platform. This is kind of intended.

src/gui/wizard/webviewpage.cpp

FlexW · 2021-03-04T11:39:10Z

Screencast (recorded on Linux with i3):
https://cloud.nextcloud.com/s/RamJZNERBXZS2pC

Screencast Windows:
https://cloud.nextcloud.com/s/43cEwZwdjxe8rzZ

jancborchardt · 2021-03-09T10:45:01Z

@FlexW looks very nice with white/default background as well! :) Only 2 details then:

The bidirectional sync arrows in the last step are too present in blue, better would be grey: #969696 (as we use for other action icons)
In the Windows screencast there’s a glitch with the slideshow, where it switches to a previous slide? https://cloud.nextcloud.com/s/43cEwZwdjxe8rzZ

Otherwise all good to go :)

FlexW · 2021-03-09T11:49:13Z

@jancborchardt Okay. I'm unsure which glitch you mean. I think it works as expected. Maybe I was pressing the buttons to fast and therefore it looks like a glitch.

FlexW · 2021-03-09T12:23:07Z

@er-vin Could you please approve this pr or tell me if there is anything that needs to be changed in your opinion? Since we need that pr for the release tomorrow:)

nextcloud-desktop-bot · 2021-03-09T12:27:34Z

AppImage file: Nextcloud-PR-2895-1c511b4b3b9a6940b4068b12b88fd8f7d0c20ab0-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.

er-vin · 2021-03-09T13:07:05Z

@er-vin Could you please approve this pr or tell me if there is anything that needs to be changed in your opinion?

Busy busy, trying to find some free time tonight.

Since we need that pr for the release tomorrow:)

Indeed it's cutting it short now...

er-vin

Still that LinkLabel class which needs a round, other than that... well... this PR is became hard to review a while ago so it might be hiding other things.

er-vin · 2021-03-09T17:31:09Z

src/gui/wizard/linklabel.h

+
+namespace OCC {
+
+class LinkLabel : public QLabel


@er-vin
It was wished by the design team ;)

Not argument enough in my opinion if that leads to potentially harmful code piling up. ;-)

(not that this single occurrence is super harmful of course, but my experience is that unchallenged design decisions lead to a path of death by 1000 cuts, hence why the question should be at least asked, every single time)

One problem I see with not making an extra class is how to embed the link into the label? I know I can use rich text for that but when rich text is used the font of the label stays not white anymore it uses a different color. Now I know I can work around this by setting the font in the rich text again but it gets then a lot more complicated than it is now. Another possibility would be to create another event filter or even use the HoverEventFilter and rename it to LinkLabelEventFilter but I'm unsure if this brings any benefit to what we have now. What do you think?

Alright, I prefer the event filter but I see you'll be struggling with that one. Let's scrap that...

Well the main problem really is the QLabel inheritance. I'd be fine even with inheriting from QWidget and have a single QLabel as child (and in a private member). Design wise the problem is that this is still a QLabel with all its API being exposed to the users of LinkLabel. Delegation is way better than inheritance in most cases, here included.

Signed-off-by: Felix Weilbach <[email protected]>

nextcloud-desktop-bot · 2021-03-10T10:12:14Z

AppImage file: Nextcloud-PR-2895-e0b7ef15b207f59d73884f8e1c12a206803c5c40-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.

Need to merge that for the release

szaimen · 2021-03-10T11:59:49Z

🎉

jancborchardt · 2021-03-10T14:58:44Z

Really great work @FlexW! :) And thanks @er-vin @allexzander for the reviews

camilasan requested review from er-vin, allexzander and camilasan February 1, 2021 19:33

er-vin reviewed Feb 2, 2021

View reviewed changes

src/gui/wizard/owncloudwizard.cpp Outdated Show resolved Hide resolved

er-vin reviewed Feb 4, 2021

View reviewed changes

FlexW force-pushed the wizard_ui_improvements branch from 62d4516 to 070618d Compare February 8, 2021 13:54

allexzander reviewed Feb 8, 2021

View reviewed changes

jancborchardt approved these changes Feb 8, 2021

View reviewed changes

allexzander reviewed Feb 8, 2021

View reviewed changes

er-vin previously requested changes Feb 10, 2021

View reviewed changes

jancborchardt mentioned this pull request Feb 11, 2021

Login Textfield Design nextcloud/android#7971

Merged

1 task

er-vin reviewed Feb 13, 2021

View reviewed changes

FlexW force-pushed the wizard_ui_improvements branch 3 times, most recently from b52c98d to b98e706 Compare February 19, 2021 08:55

er-vin reviewed Feb 22, 2021

View reviewed changes

FlexW force-pushed the wizard_ui_improvements branch 3 times, most recently from d6a0efc to 7482ddc Compare February 23, 2021 08:33

allexzander reviewed Feb 23, 2021

View reviewed changes

src/gui/wizard/owncloudsetuppage.cpp Outdated Show resolved Hide resolved

allexzander reviewed Feb 23, 2021

View reviewed changes

src/gui/wizard/owncloudwizard.cpp Outdated Show resolved Hide resolved

allexzander reviewed Feb 23, 2021

View reviewed changes

src/gui/wizard/owncloudwizard.cpp Outdated Show resolved Hide resolved

allexzander reviewed Feb 23, 2021

View reviewed changes

src/gui/wizard/owncloudwizard.cpp Outdated Show resolved Hide resolved

allexzander reviewed Feb 24, 2021

View reviewed changes

src/gui/wizard/welcomepage.cpp Show resolved Hide resolved

FlexW force-pushed the wizard_ui_improvements branch 2 times, most recently from 70a2e4b to 89d0397 Compare February 25, 2021 14:49

er-vin reviewed Feb 25, 2021

View reviewed changes

FlexW force-pushed the wizard_ui_improvements branch 3 times, most recently from 9b9aede to 5a39db6 Compare March 4, 2021 11:37

FlexW force-pushed the wizard_ui_improvements branch from 5a39db6 to 91a3c3d Compare March 4, 2021 11:40

allexzander approved these changes Mar 4, 2021

View reviewed changes

elsiehupp mentioned this pull request Mar 5, 2021

It isn't possible to quit the first-launch wizard (on macOS) #2969

Closed

FlexW force-pushed the wizard_ui_improvements branch from 91a3c3d to 1c511b4 Compare March 9, 2021 12:08

er-vin reviewed Mar 9, 2021

View reviewed changes

Felix Weilbach added 4 commits March 10, 2021 09:55

Add new wizard icons and remove obsolete ones

05d2a87

Signed-off-by: Felix Weilbach <[email protected]>

Split out wizard welcome page

9185956

Signed-off-by: Felix Weilbach <[email protected]>

Restyle wizard

f55e5b1

Signed-off-by: Felix Weilbach <[email protected]>

Restyle wizard pages

e0b7ef1

Signed-off-by: Felix Weilbach <[email protected]>

FlexW force-pushed the wizard_ui_improvements branch from 1c511b4 to e0b7ef1 Compare March 10, 2021 08:55

FlexW merged commit b5cf820 into master Mar 10, 2021

FlexW deleted the wizard_ui_improvements branch March 10, 2021 11:54

FlexW mentioned this pull request Apr 29, 2021

Connect to Nextcloud Wizard: Go back at login screen #1161

Closed

Improve wizard ui #2895

Improve wizard ui #2895

Conversation

FlexW commented Feb 1, 2021

FlexW commented Feb 1, 2021

er-vin left a comment

Choose a reason for hiding this comment

FlexW commented Feb 2, 2021

FlexW commented Feb 3, 2021

jancborchardt commented Feb 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FlexW commented Feb 8, 2021

jancborchardt left a comment

Choose a reason for hiding this comment

er-vin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FlexW Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FlexW commented Mar 4, 2021 • edited Loading

jancborchardt commented Mar 9, 2021

FlexW commented Mar 9, 2021

FlexW commented Mar 9, 2021

nextcloud-desktop-bot commented Mar 9, 2021

er-vin commented Mar 9, 2021

er-vin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nextcloud-desktop-bot commented Mar 10, 2021

szaimen commented Mar 10, 2021

jancborchardt commented Mar 10, 2021

FlexW Feb 25, 2021 •

edited

Loading

FlexW commented Mar 4, 2021 •

edited

Loading