Add confirmation prompt before moving groups to the recycling bin, Refactor MessageBox to allow custom buttons #2376
Conversation
Spawn a yes/no QMessage box when "Delete Group" is selected on a group that is not already in the recycle bin (note: the prompt for deletion from the recycle bin was already implemented). This follows the same pattern and language as entry deletion. Fixes keepassxreboot#2125
|
TBH, I am not a big fan of Yes/No boxes. There is almost never a situation where they are good UX design. I vote for changing the buttons to"Delete" and "Cancel". |
|
@phoerious I could be into that. I made this consistent with the existing dialogs for moving an entry to the recycling bin, so I would want to change those as well. |
|
I do agree with @phoerious on this one. It makes a lot of sense, especially for quick questions, to have the (complete) answer as the choice in the button. Under Material design rules, you would also make the delete button red, but that is overkill. I think most of our dialogs will go this route, perhaps we should extend the MessageBox class to include these additional choices. See: http://doc.qt.io/qt-5/qmessagebox.html#advanced-usage |
|
@phoerious @droidmonkey I went through the Occurrences of
|
|
And to touch on @droidmonkey's last comment: I'm not sure I have the eye to make delete-centric buttons red without making them look like trash 🙂 It seems there are some patterns visible in occurrences listed in my last comment. I agree it would be nice to add some |
|
Which ones do you recommend changing? Most of them seem fine to me (like your comment suggests). The big one for me is on destructive changes there should be explicit actions (Delete/Cancel/Discard/Save) instead of Yes/No question. |
|
I misinterpreted the request to change all yes/no questions to be explicit actions. Here are the questions that I feel make destructive changes, and what I think they should be changed to. Let me know if any of them need adjustment. Proposed modifications
|
|
Great suggestions, I agree with them. Wherever possible, prefer the use of a single verb. Sorry for the confusion in earlier comments. |
|
@kneitinger Do you want to introduce the new buttons in a different PR or add to this one? |
|
Happy to include them on this PR (makes sense to include it where the discussion was had). A few days ago I started working on a different issue when this was sitting for a while, so I'm going to open that one tonight and then get to the button changes. |
…tion_before_group_deletion
Replace yes/no, yes/cancel (and other such buttons on prompts that cause data to be destroyed) use language that indicates the action that it is going to take. This makes destructive/unsafe and/or irreversible operations more clear to the user. Address feedback on PR keepassxreboot#2376
|
@phoerious @droidmonkey I went through and modified all of the prompts Verification
|
|
Apologies, forgot to update the GUI tests after these changes, I'll take care of that. As for the CodeFactor test, it deemed a function whose only edit I made was removing a single trailing space to be "Complex". I wouldn't mind refactoring it, but do you know of a way to approximate CodeFactor's behavior locally so I don't need to push to see if it's sufficient? |
|
Don't worry about codefactor. It looks like you have a crash occurring in the tests. Edit: Looks like you did this outside of the MessageBox class. Unfortunately that will make it impossible to fix the GUI tests. MessageBox class allows tests to "set an answer" that is returned immediately after the message is shown. It would be much cleaner to implement a thin wrapper around what you have done with the QMessageBox button adding. If you need help I can provide an example. |
|
@droidmonkey Thanks, I noticed the same thing. I opted to do it out of MessageBox due to difficulty coming up with nice generic solutions. I didn't see an issue because nextAnswer didn't seem to be used in |
…tion_before_group_deletion
…tion_before_group_deletion
|
I came up with a rough sketch of something that seems to work well: branch: kneitinger/messagebox_revamp The main focus was making the interactions that happen outside of the Breakdown of the changes:
I'd like to hear your thoughts on this. Keep in mind that this is just a rough prototype for illustrating an approach, and a final solution using this approach would be a lot cleaner (possibly using Q_FLAGS on enum, Refactoring for loops out of Thanks in advance! |
|
Nice! That is exactly what I had in mind. For the button map, is that translation ready? You may need to add the "tr()" wrappers in there. For the Qt standard buttons there might be an elegant way to bring in the text, not sure about that. |
|
Awesome! I did get translations working fine, but kept it out to not overcomplicate the proof of concept. Because Thanks for looking at it, I'll move forward on this refactor! Edit: regarding grabbing the standard button text: good idea, I'll look into it. |
Replaces arguments and return values of type QMessageBox::StandardButton(s) with MessageBox::Button(s), which reimplements the entire set of QMessageBox::StandardButton and allows for custom KeePassXC buttons, such as "Skip". Modifies all calls to MessageBox functions to use MessageBox::Button(s). Addresses feedback on keepassxreboot#2376
…tion_before_group_deletion
|
@droidmonkey I worked in the translation on the new buttons as well as using the already translated text from the QT standard buttons. I accrued a CodeFactor issue for the |
Replaced the switch statement mechanism in MessageBox::addButton with a map lookup to address CodeFactor Complex Method issue. This has a side-effect of a small performance/cleanliness increase, as an extra QPushButton is no longer created/destroyed (to obtain it's label text) everytime a MessageBox button based on QMessageBox::StandardButton is created; now the text is obtained once, at application start up.
|
I've moved the I went through and re-ran my manual tests listed in previous comments, and its working the same, however, in reimplementing the buttons, I've overlooked the icons:
If we want them back, It would not be much work to obtain the icons for Edit: not sure what happened with that Windows build renaming classes...any insight into that? |
|
Oh crap, its because Windows.h is included. You have to add a As for the icons, I never noticed them before! They do not appear on Windows.
I think they are hideous and distracting anyway. |
|
😆 Yeah, I thought the lack of icons was a classier experience, but wasn't sure if they were kept for some reason. I haven't changed my QT theme because I though the icons would stay and look out of place in a better looking theme. After seeing your screenshot, tried changing the theme and wow turns out KeePassXC can look so good with a modern style and less stock icons! Implemented the |
kneitinger
changed the title
|
Updated the PR title and added "Update" section to the top of the PR description to provide some context for any new viewers. |
|
Hi all, just a bump that this is ready for further review, or merge if ready 🙂 |
|
Yes sorry, had finals this past week. I will address this asap. |
|
Thanks, congrats on completing your finals! |
|
Thanks! Fantastic work on this PR. |
- New Database Wizard [#1952] - Advanced Search [#1797] - Automatic update checker [#2648] - KeeShare database synchronization [#2109, #1992, #2738, #2742, #2746, #2739] - Improve favicon fetching; transition to Duck-Duck-Go [#2795, #2011, #2439] - Remove KeePassHttp support [#1752] - CLI: output info to stderr for easier scripting [#2558] - CLI: Add --quiet option [#2507] - CLI: Add create command [#2540] - CLI: Add recursive listing of entries [#2345] - CLI: Fix stdin/stdout encoding on Windows [#2425] - SSH Agent: Support OpenSSH for Windows [#1994] - macOS: TouchID Quick Unlock [#1851] - macOS: Multiple improvements; include CLI in DMG [#2165, #2331, #2583] - Linux: Prevent Klipper from storing secrets in clipboard [#1969] - Linux: Use polling based file watching for NFS [#2171] - Linux: Enable use of browser plugin in Snap build [#2802] - TOTP QR Code Generator [#1167] - High-DPI Scaling for 4k screens [#2404] - Make keyboard shortcuts more consistent [#2431] - Warn user if deleting referenced entries [#1744] - Allow toolbar to be hidden and repositioned [#1819, #2357] - Increase max allowed database timeout to 12 hours [#2173] - Password generator uses existing password length by default [#2318] - Improve alert message box button labels [#2376] - Show message when a database merge makes no changes [#2551] - Browser Integration Enhancements [#1497, #2253, #1904, #2232, #1850, #2218, #2391, #2396, #2542, #2622, #2637, #2790] - Overall Code Improvements [#2316, #2284, #2351, #2402, #2410, #2419, #2422, #2443, #2491, #2506, #2610, #2667, #2709, #2731]
UPDATE:
The scope of this PR changed quite a bit since it was open. Originally it was to add a confirmation before moving groups to the recycle bin, which turned into modifying all questions with destructive actions to have buttons that reflect the operation instead of yes/no. To implement that, MessageBox was rewritten to act analogously to how it did when it was
QMessageBox::StandardButton-based, but allows for custom buttons.The manual test cases I used can be seen in this comment
Below is the original PR, other requests can be found in the comments.
Description
Spawn a yes/no QMessage box when "Delete Group" is selected on a group
that is not already in the recycle bin (note: the prompt for deletion
from the recycle bin was already implemented). This follows the same
pattern and language as entry deletion.
Motivation and context
Fixes #2125
How has this been tested?
Do you really want to move the group "TestGroup" to the recycle bin?is shownTypes of changes
Checklist:
-DWITH_ASAN=ON. [REQUIRED]