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

Fix wrong horizontal QR code resizing #9027

Merged
merged 1 commit into from
Feb 2, 2023

Conversation

dmaslenko
Copy link
Contributor

Fixes #8944 issue when the QR code was wrongly resized horizontally

Screenshots (have dummy QR codes based on AAAA key).

Before

stratched-qr-code

After

fixed-qr-code-centered

Testing strategy

  • Have an entry with defined TOTP.
  • Open the QR code window and try to stretch it.

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)

@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2023

Codecov Report

Base: 64.57% // Head: 64.83% // Increases project coverage by +0.26% 🎉

Coverage data is based on head (1ea4e2e) compared to base (3243243).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #9027      +/-   ##
===========================================
+ Coverage    64.57%   64.83%   +0.26%     
===========================================
  Files          342      342              
  Lines        44372    44392      +20     
===========================================
+ Hits         28650    28779     +129     
+ Misses       15722    15613     -109     
Impacted Files Coverage Δ
src/gui/SquareSvgWidget.h 100.00% <ø> (+100.00%) ⬆️
src/gui/SquareSvgWidget.cpp 100.00% <100.00%> (+100.00%) ⬆️
src/gui/TotpExportSettingsDialog.cpp 71.01% <100.00%> (+71.01%) ⬆️
src/core/Entry.cpp 82.52% <0.00%> (-0.20%) ⬇️
src/gui/DatabaseWidget.cpp 61.77% <0.00%> (+0.39%) ⬆️
src/core/FileWatcher.cpp 86.75% <0.00%> (+1.20%) ⬆️
src/qrcode/QrCode.cpp 79.69% <0.00%> (+79.69%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dmaslenko dmaslenko marked this pull request as draft January 26, 2023 06:31
@phoerious phoerious modified the milestone: v2.8.0 Jan 26, 2023
@phoerious phoerious marked this pull request as ready for review January 26, 2023 10:18
Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me.

@droidmonkey
Copy link
Member

This doesn't solve the problem because the QR Code remains the same size. The point of resizing the window is to get a bigger qr code.

@phoerious
Copy link
Member

Do we need that? IMHO we could even make the window a fixed size.

@droidmonkey
Copy link
Member

I don't disagree. Perhaps the real problem is initial sizing of the qrcode is not dpi dependent, although it works great on my 4k screen so not sure what's the matter.

@phoerious
Copy link
Member

Proportional resizing would be fine as well, I don't really care.

@dmaslenko
Copy link
Contributor Author

dmaslenko commented Jan 27, 2023

I am still working to find a solution to center it or stop me here :)
scalable-qr-code-not-centered

@dmaslenko
Copy link
Contributor Author

The next commit allows to scale and center the QR code. It looks like:
Screencast from 01-28-2023 03 13 26 PM

Let me check if UI test is possible here...

@droidmonkey
Copy link
Member

You could test by setting the dialog size and checking that the size of the qr svg is bigger and still square.

@droidmonkey droidmonkey modified the milestones: v2.8.0, v2.7.5 Jan 29, 2023
src/gui/SquareSvgWidget.h Outdated Show resolved Hide resolved
src/gui/SquareSvgWidget.cpp Outdated Show resolved Hide resolved
src/gui/SquareSvgWidget.cpp Outdated Show resolved Hide resolved
src/gui/SquareSvgWidget.h Outdated Show resolved Hide resolved
src/gui/TotpExportSettingsDialog.cpp Show resolved Hide resolved
void SquareSvgWidget::resizeEvent(QResizeEvent*)
{
Q_ASSERT(parentWidget());
auto containerRect = parentWidget()->contentsRect();
Copy link
Member

Choose a reason for hiding this comment

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

Do a null check here, there is no guarantee the widget will have a parent and you will instacrash here if it doesn't. Assert only gets hit in debug build

Copy link
Contributor Author

@dmaslenko dmaslenko Jan 30, 2023

Choose a reason for hiding this comment

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

Sure, added the checking for null.

@dmaslenko
Copy link
Contributor Author

dmaslenko commented Jan 30, 2023

Hm, seems I rebased my branch incorrectlly.

@dmaslenko
Copy link
Contributor Author

Added a test for QR code: open -> check the initial shape for square -> resize -> check the shape again.
In general the TOTP tests look like this (with temporary small delays between steps, not committed):
test-totp

* Also add GUI test for QR code resizing
@droidmonkey droidmonkey merged commit f703736 into keepassxreboot:develop Feb 2, 2023
@dmaslenko dmaslenko deleted the fix/qr_code_resizing branch February 2, 2023 05:29
Perlover added a commit to Perlover/keepassxc that referenced this pull request May 18, 2023
Release 2.7.5

- Add menu option to allow screenshots [keepassxreboot#8841]
- Add support for Botan 3 [keepassxreboot#9388]
- Increase max TOTP step to 24 hours [keepassxreboot#9149]
- Improve HTML export layout [keepassxreboot#8987]
- Turn search reset off by default [keepassxreboot#9153]
- Use QClipboard::clear() instead of setting blank text [keepassxreboot#9148]
- Hide group column header choice when not in search [keepassxreboot#9171]
- Improve look of KeePassXC logo and icons [keepassxreboot#9355]
- Add keyboard shortcuts for app and database settings [keepassxreboot#9007]
- Hide rename button from attachments preview panel [keepassxreboot#8842]
- Linux: Set SingleMainWindow in .desktop file [keepassxreboot#7430]

- Fix crash when search clears while creating new entry [keepassxreboot#9230]
- Fix crash when using Windows Hello in a Remote Desktop session [keepassxreboot#9006]
- Fix crash in Group Edit after enabling Browser Integration [keepassxreboot#8778]
- Fix canceling quick unlock when it is unavailable [keepassxreboot#9034]
- Set password input field font correctly [keepassxreboot#8732]
- Greatly improve performance when rendering entry view [keepassxreboot#9398]
- Fix various accessibility issues [keepassxreboot#9138]
- Fix arrows size when expand/collapse a group [keepassxreboot#9096]
- Select the clone instead of the original after cloning an entry [keepassxreboot#9070]
- Fix bugs with preview widget [keepassxreboot#9170]
- Fix status bar update when switching to other DB [keepassxreboot#9073]
- Fix database settings spin box bug [keepassxreboot#9101]
- Fix Ctrl+Tab shortcut to cycle databases in unlock dialog [keepassxreboot#8839]
- Fix TOTP QR code maintaining square ratio [keepassxreboot#9027]
- Fix Auto-Type configuration page on custom sequence selection [keepassxreboot#8752]
- Fix unexpected behavior of `--lock` when KeePassXC is not running [keepassxreboot#8889]
- Make open folder icon exempt from "Apply group icon to entry" [keepassxreboot#9205]
- Allow setting default file open directory with env var [keepassxreboot#9192]
- SSH Agent: Fix support for AES-256/GCM openssh keys [keepassxreboot#8968]
- Browser: Fix Native Messaging script path with BSD OS's [keepassxreboot#8835]
- MacOS: Fix text selection for Auto-Type clear field [keepassxreboot#9066]
- MacOS: Don't rely on AppleInterfaceStyle for theme switching [keepassxreboot#8615]
- Windows: Remove registry detection of desktop shortcut [keepassxreboot#9380]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QR code is stretched horizontally within its window
4 participants