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 password generator close button for good #9743

Merged
merged 1 commit into from
Nov 23, 2023

Conversation

droidmonkey
Copy link
Member

@droidmonkey droidmonkey commented Aug 15, 2023

  • Avoids using QDialog which breaks the standalone password generator

Revert "Fix password dialog close button"

This reverts commit 5b47190.

Fixes #7952.

Testing strategy

Tested on Windows with latest browser extension

Type of change

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

@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (49f2924) 64.86% compared to head (35e38fb) 64.87%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #9743      +/-   ##
===========================================
+ Coverage    64.86%   64.87%   +0.01%     
===========================================
  Files          335      335              
  Lines        41066    41064       -2     
===========================================
+ Hits         26636    26638       +2     
+ Misses       14430    14426       -4     
Files Coverage Δ
src/browser/BrowserService.h 100.00% <ø> (ø)
src/gui/PasswordGeneratorWidget.cpp 68.50% <0.00%> (-0.07%) ⬇️
src/browser/BrowserAction.cpp 10.21% <0.00%> (-0.05%) ⬇️
src/browser/BrowserService.cpp 25.17% <0.00%> (+0.08%) ⬆️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@varjolintu varjolintu left a comment

Choose a reason for hiding this comment

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

This works ok, but I need to fix the messaging problems to the extension side.

@droidmonkey
Copy link
Member Author

@varjolintu Which PR over on the browser side will fix the message issues?

@varjolintu
Copy link
Member

varjolintu commented Aug 20, 2023

@droidmonkey This one: keepassxreboot/keepassxc-browser#1961. I'd suggest you test it with multiple browsers.

I had to add a new boolean to keepass.js to prevent multiple simultaneous generate-password calls. This is not an issue in Chromium based browsers, but in Firefox native messaging does something differently and some messages are clearly left in the queue, which also causes BrowserHost's socket->readAll() call to return multiple messages, causing "garbage end of buffer" errors etc. And when a new generate-password is requested when the generator is already open, all those messages in the Firefox's internal buffer gets to KeePassXC.

This might cause Firefox to drop the accepted password if user manually requested the generator again. But in Chromium based browsers the password is filled normally even after multiple requests.

* Avoids using QDialog which breaks the standalone password generator

Revert "Fix password dialog close button"

This reverts commit 5b47190.
@droidmonkey
Copy link
Member Author

@varjolintu I made substantial edits here, I think this makes the experience better and solves the issue using a QWidget. Major change includes raising an existing password generator dialog if the user didn't close it before.

@droidmonkey droidmonkey merged commit 013db19 into develop Nov 23, 2023
10 of 11 checks passed
@droidmonkey droidmonkey deleted the fix/password-gen-close branch November 23, 2023 03:49
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.

UI frozen after password generator is used via browser plugin
2 participants