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

Introduced password input widget #1662

Merged
merged 46 commits into from
Jul 28, 2023

Conversation

jetchirag
Copy link
Contributor

@jetchirag jetchirag commented Mar 18, 2023

Description

Added a new PasswordInput widget for PyQt to improve functionality and reusability.

Not completed and looking for feedback.

Related Issue

I was thinking that we could use a common widget (a self-implemented one) for all password entries in vorta. Can you implement such thing?

Originally posted by @real-yfprojects in #1659 (comment)

Screenshots (if appropriate):

image

visiblity toggle:

image

validation error (min length):

image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have read the CONTRIBUTING guide.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

I provide my contribution under the terms of the license of this repository and I affirm the Developer Certificate of Origin.

@jetchirag jetchirag marked this pull request as ready for review March 19, 2023 09:42
tests/test_repo.py Outdated Show resolved Hide resolved
@jetchirag
Copy link
Contributor Author

jetchirag commented Mar 19, 2023

image

UI is mostly the same.

@real-yfprojects
Copy link
Collaborator

real-yfprojects commented Mar 19, 2023

Nice. Maybe we can reduce duplicated code even more by having a widget that unifies the two password lineEdits and the related labels. This would involve these steps:

  • Rename PasswordInput to something like PassswordLineEdit
  • Make it subclass QLineEdit
  • Add and implement a password input class consisting of two PasswordLineEdits
  • Move all validation logic/rules into the new class
  • Use it in existing ui layouts

Signed-off-by: Chirag Aggarwal <[email protected]>
@jetchirag
Copy link
Contributor Author

jetchirag commented Mar 19, 2023

Add and implement a password input class consisting of two PasswordLineEdits

So a QWidget? What do you think about a link_with parameter in same class to link confirm input with first one? We can still move validation to this class and use features of QLineEdit too.

@real-yfprojects
Copy link
Collaborator

So a QWidget?

For the one containing multiple Widgets yes.

What do you think about a link_with parameter in same class to link confirm input with first one? We can still move validation to this class and use features of QLineEdit too.

Together with the min password length this spreads password validation rules over multiple places and classes.

@jetchirag
Copy link
Contributor Author

Can you please take a look at the use of translation?

Copy link
Collaborator

@real-yfprojects real-yfprojects left a comment

Choose a reason for hiding this comment

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

This is looking like what I had in mind. How Vorta uses the password inputs makes it hard to move it into its own class. Although inheriting QWdiget would be nice, it doesn't make much sense for this use. I think inheriting QObject would be better. This makes PasswordInput a manager of widgets instead of a widget itself.

src/vorta/views/partials/password_input.py Outdated Show resolved Hide resolved
src/vorta/views/partials/password_input.py Outdated Show resolved Hide resolved
src/vorta/views/partials/password_input.py Outdated Show resolved Hide resolved
src/vorta/views/partials/password_input.py Outdated Show resolved Hide resolved
src/vorta/views/partials/password_input.py Outdated Show resolved Hide resolved
src/vorta/views/partials/password_input.py Outdated Show resolved Hide resolved
src/vorta/views/partials/password_input.py Outdated Show resolved Hide resolved
src/vorta/views/partials/password_input.py Outdated Show resolved Hide resolved
src/vorta/views/partials/password_input.py Outdated Show resolved Hide resolved
src/vorta/views/partials/password_input.py Outdated Show resolved Hide resolved
src/vorta/views/partials/password_input.py Outdated Show resolved Hide resolved
src/vorta/views/partials/password_input.py Outdated Show resolved Hide resolved
src/vorta/views/partials/password_input.py Outdated Show resolved Hide resolved
src/vorta/views/repo_add_dialog.py Outdated Show resolved Hide resolved
src/vorta/views/repo_add_dialog.py Outdated Show resolved Hide resolved
src/vorta/views/partials/password_input.py Outdated Show resolved Hide resolved
src/vorta/views/partials/password_input.py Outdated Show resolved Hide resolved
src/vorta/views/repo_add_dialog.py Outdated Show resolved Hide resolved
src/vorta/views/repo_add_dialog.py Outdated Show resolved Hide resolved
src/vorta/views/partials/password_input.py Outdated Show resolved Hide resolved
src/vorta/views/repo_add_dialog.py Outdated Show resolved Hide resolved
Signed-off-by: Chirag Aggarwal <[email protected]>
Signed-off-by: Chirag Aggarwal <[email protected]>
@real-yfprojects real-yfprojects requested a review from m3nu May 19, 2023 08:19
@m3nu
Copy link
Contributor

m3nu commented Jun 9, 2023

Hey, thanks for removing some duplicate code here. Just tried it locally and noticed this:

  • The first time someone reveals the password, the icon doesn't match the state. (Icon and tooltip says "Show password", but password is already shown)
  • For the case of Existing Repo, the spacing at the bottom could be more consistent with the New Repo case.

Screenshot 2023-06-09 at 12 28 53

@real-yfprojects real-yfprojects self-assigned this Jun 24, 2023
jetchirag added 2 commits July 4, 2023 18:30
Signed-off-by: Chirag Aggarwal <[email protected]>
@jetchirag
Copy link
Contributor Author

Fixed in new commits. Also resolved merge conflicts.

@m3nu
Copy link
Contributor

m3nu commented Jul 5, 2023

The show/hide icon state works as expected now.

Just for the "Add Existing Repo" dialog, there may be a fixed height set, which squeezes the lines a bit. I know we wanted to reduce the extra space a bit, but this needs to be done without settings a fixed height on the dialog window, but by editing the ?vertical space element within it. (I know Qt can be fickle).

Screenshot 2023-07-05 at 11 21 30

@jetchirag
Copy link
Contributor Author

Ah, it doesn't happen on OS and UI I'm testing on. I'll fix that.

@jetchirag
Copy link
Contributor Author

@m3nu Without changing fixed height, do you want to expand the inner widget?

image

I enabled verstretch for this.

@m3nu
Copy link
Contributor

m3nu commented Jul 6, 2023

The lines are fine now, but the "Add existing" dialog is too high. Seems to have the same height, but one fewer row of inputs.

Would be good to make make the inner widget flexible height too? Currently the "Add Existing" dialog looks somewhat unfinished with this extra space.

Screenshot 2023-07-06 at 11 57 56
Screenshot 2023-07-06 at 11 58 04

@jetchirag
Copy link
Contributor Author

Sorry I'm still unsure what you are proposing. I don't think inner widget has a fixed height but because main layout has fixed height, it's expanding. The fixed height seems to be based on Add new repo so can we use a lower fixed height for this dialog?

92608f9#diff-804dae53d409307e28853c1c80251c625c13f04bd7b0037ef30dc99fe347a77dR10

@real-yfprojects
Copy link
Collaborator

If possible we shouldn't use fixed heights when creating a GUI.

@jetchirag
Copy link
Contributor Author

Okay so I've removed the fixed size property and added some margin. I've also set a min width otherwise existing repo dialog would be quite smaller than the new repo dialog. Let me know if I understood it correctly.

@real-yfprojects
Copy link
Collaborator

Sounds good, I'll test it when I find the time.

@m3nu
Copy link
Contributor

m3nu commented Jul 17, 2023

Looks good on macOS 👍

Screenshot 2023-07-17 at 13 11 56
Screenshot 2023-07-17 at 13 12 01

Copy link
Collaborator

@real-yfprojects real-yfprojects left a comment

Choose a reason for hiding this comment

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

Very nice! One slight problem though:
Screenshot_20230721_165455
This only happens when the border colour is altered.

@jetchirag
Copy link
Contributor Author

Ah, fixing it.

@jetchirag
Copy link
Contributor Author

Fixed the red border. Is that white text an issue with this widget or the theme?

Signed-off-by: Chirag Aggarwal <[email protected]>
@real-yfprojects
Copy link
Collaborator

Fixed the red border. Is that white text an issue with this widget or the theme?

Both is fixed now. On my desktop the text is white and the background black.

@real-yfprojects real-yfprojects merged commit 25b4cc0 into borgbase:master Jul 28, 2023
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.

4 participants