Skip to content

Fix standalone password generator so that the esc key quits out of it#1044

Closed
pkill-9 wants to merge 93 commits intokeepassxreboot:developfrom
pkill-9:feature/password-generator-quit
Closed

Fix standalone password generator so that the esc key quits out of it#1044
pkill-9 wants to merge 93 commits intokeepassxreboot:developfrom
pkill-9:feature/password-generator-quit

Conversation

@pkill-9
Copy link
Copy Markdown
Contributor

@pkill-9 pkill-9 commented Oct 7, 2017

Description

In the password generator (standalone, ie not when you're editing an entry) the ESC key is ignored, however in most other dialogs ESC is a shortcut to quit the dialog.

Motivation and context

Reported in issue 953

Types of changes

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

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

Copy link
Copy Markdown

@spaetz spaetz left a comment

Choose a reason for hiding this comment

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

Hi there, I am in no way qualified to comment on the code. Just an observation/clarification question:
Do I see it correctly that you add "ESC" and "Ctrl-Enter" both as closing shortcuts? Why the latter?
And also, isn't it possible to set a keyboard shortcut on the dialog button as other button shortcuts are also being used? Perhaps even with a tooltip, hinting at the user that she can use ESC for closing.

@pkill-9
Copy link
Copy Markdown
Contributor Author

pkill-9 commented Oct 12, 2017

Reading the logic, I don't believe this does make Ctrl-Enter a close shortcut, it just limits when ESC acts as a close shortcut depending on whether modifier keys are also pressed. I added the handling of modifier keys since that's the same behavior for other dialogs (as defined in DialogyWidget.cpp)

What do you mean by adding a keyboard shortcut? (Is that what's done in DialogyWidget?) If that would be a better approach I can definitely explore it further.

(My apologies if my knowledge is limited, this is a first foray into this project so I'm still learning how everything fits together)

Copy link
Copy Markdown
Contributor

@TheZ3ro TheZ3ro left a comment

Choose a reason for hiding this comment

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

Works fine

e->ignore();
}
}
else {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please put this on the same line as the closing brace. Other than that, I think this patch is fine as is.

@phoerious phoerious added this to the v2.3.0 milestone Oct 19, 2017
phoerious and others added 23 commits October 21, 2017 22:23
- Fixed entries with empty URLs being reported to KeePassHTTP clients [keepassxreboot#1031]
- Fixed YubiKey detection and enabled CLI tool for AppImage binary [keepassxreboot#1100]
- Added AppStream description [keepassxreboot#1082]
- Improved TOTP compatibility and added new Base32 implementation [keepassxreboot#1069]
- Fixed error handling when processing invalid cipher stream [keepassxreboot#1099]
- Fixed double warning display when opening a database [keepassxreboot#1037]
- Fixed unlocking databases with --pw-stdin [keepassxreboot#1087]
- Added ability to override QT_PLUGIN_PATH environment variable for AppImages [keepassxreboot#1079]
- Fixed transform seed not being regenerated when saving the database [keepassxreboot#1068]
- Fixed only one YubiKey slot being polled [keepassxreboot#1048]
- Corrected an issue with entry icons while merging [keepassxreboot#1008]
- Corrected desktop and tray icons in Snap package [keepassxreboot#1030]
- Fixed screen lock and Google fallback settings [keepassxreboot#1029]
…ean-up-dockerfile

Reduce external dependencies and install YubiKey deps from EPEL
…ders-in-window-association-title

AutoType: resolve placeholders for title in window associations
@TheZ3ro
Copy link
Copy Markdown
Contributor

TheZ3ro commented Oct 24, 2017

@pkill-9 can you rebase on develop?

@TheZ3ro
Copy link
Copy Markdown
Contributor

TheZ3ro commented Dec 5, 2017

Seems that this PR broke some time ago. Submitting a new PR #1252 with cherry-picked commit (since I can't push to this pr :(

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.