Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display the content of the login dialog correct #2989

Merged
merged 1 commit into from
Mar 16, 2021

Conversation

FlexW
Copy link

@FlexW FlexW commented Mar 11, 2021

This adjustment is necessary because of the changes of the new account
wizard that were introduced with:
e0b7ef1

Signed-off-by: Felix Weilbach felix.weilbach@nextcloud.com

This is how the dialog looks with the fix:
https://cloud.nextcloud.com/s/9WEr7ZcxYrFPWAi

@jancborchardt Your input on this is would be valuable:) Is the dialog ok?

General note:
Maybe we should start to refactor the wizard a bit and remove these void customizeStyle() methods. They are not used at the moment and problems like the one this pr fixes right now would not exist.

@allexzander
Copy link
Contributor

@FlexW I am fine code-wise. Let's see what @jancborchardt says design-wise. Maybe you need to attach screenshots with "before" and "after", so it is easier for Jan to get what's changed.

Regarding this customizeStyle(); I am not sure if we need to remove it. What do you mean by They are not used at the moment? I can see that this method is called in most classes. It just looks weird that we have this, identically named function in every subclass of QWidget. Maybe it would make sense to introduce an intermediate class between QWidget and these subclasses. Then we could turn customizeStyle(); into a virtual customizeStyle(); and then call it in the shared base class's constructor only once.

@FlexW
Copy link
Author

FlexW commented Mar 11, 2021

Regarding this customizeStyle(); I am not sure if we need to remove it. What do you mean by They are not used at the moment? I can see that this method is called in most classes. It just looks weird that we have this, identically named function in every subclass of QWidget. Maybe it would make sense to introduce an intermediate class between QWidget and these subclasses. Then we could turn customizeStyle(); into a virtual customizeStyle(); and then call it in the shared base class's constructor only once.

@allexzander A bit more context from my side would be helpful I guess;) From tinkering around with the wizard I discovered that these methods were originally introduced to have the wizard react to dark/light theme changes. As we don't use such functionality at the moment and I'm unsure if we ever will, it would be better in my opinion to just remove this functionality and make the customizeStyle() methods private and call them only in one place. This would make the code a bit easier to read and less error prone.

@er-vin
Copy link
Member

er-vin commented Mar 12, 2021

I agree with @FlexW it was on my list of things to try to can at some point. I don't think it's the best way to do the light/dark thing. You probably want to set a palette or a style on the application object and let the corresponding events trickle down everywhere. There's a proper Qt mechanism for that.

@@ -156,8 +156,8 @@ void WebFlowCredentials::askFromUser() {
_askDialog->setUrl(url);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor wording fixes :)

  • We don’t need to show "username@cloud.example.com as user username" since that duplicates the username. Just "You have been logged out of cloud.example.com as user username." is enough.
  • Second sentence "Please log in again.", "log in" separate.

@jancborchardt
Copy link
Member

@FlexW I am fine code-wise. Let's see what @jancborchardt says design-wise. Maybe you need to attach screenshots with "before" and "after", so it is easier for Jan to get what's changed.

Yep, I’m not sure what has changed – the text "You have been logged out …" is new to me, and I’m not sure it is even useful to show at all? Cause it’s not really saying why it happened, so I wonder if we can just cut it?

(Regarding dark/light theme support, yeah eventually we should do that too. But not a priority :)

And the grey-ish background is Windows default, right?

@FlexW
Copy link
Author

FlexW commented Mar 16, 2021

/rebase

This adjustment is necessary because of the changes of the new account
wizard that were introduced with:
e0b7ef1

Signed-off-by: Felix Weilbach <felix.weilbach@nextcloud.com>
@github-actions github-actions bot force-pushed the bugfix/fix-appearance-login-dialog branch from 6d27de1 to ff7932b Compare March 16, 2021 13:34
@FlexW
Copy link
Author

FlexW commented Mar 16, 2021

@jancborchardt Yes, the background is the default from Windows. The text was there before. I will merge that pull request and open a follow-up issue to correct that wording or drop it.

@FlexW FlexW mentioned this pull request Mar 16, 2021
@nextcloud-desktop-bot
Copy link

AppImage file: Nextcloud-PR-2989-ff7932bb54cd83d8028325f0f82e19ae346447e4-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 FlexW merged commit 162dff9 into master Mar 16, 2021
@FlexW FlexW deleted the bugfix/fix-appearance-login-dialog branch March 16, 2021 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants