From 4d4c839afae2beb9da24ee5a5c6a3756a0d6b2c8 Mon Sep 17 00:00:00 2001 From: Kyle Kneitinger Date: Wed, 19 Dec 2018 20:14:11 -0800 Subject: [PATCH] Customize buttons on MessageBox and confirm before recycling (#2376) * Add confirmation prompt before moving groups to the recycling bin 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 #2125 * Make prompts for destructive operations use action words on buttons 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 #2376 * Refactor MessageBox class to allow for custom buttons 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 #2376 * Remove MessageBox::addButton in favor of map lookup 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. --- src/browser/BrowserService.cpp | 85 +++++---- src/core/Bootstrap.cpp | 3 + src/gui/DatabaseTabWidget.cpp | 4 +- src/gui/DatabaseWidget.cpp | 84 +++++---- src/gui/EditWidgetIcons.cpp | 20 ++- src/gui/EditWidgetProperties.cpp | 15 +- src/gui/MessageBox.cpp | 165 +++++++++++++----- src/gui/MessageBox.h | 101 ++++++++--- src/gui/csvImport/CsvImportWidget.cpp | 4 +- .../DatabaseSettingsWidgetBrowser.cpp | 40 ++--- .../DatabaseSettingsWidgetMasterKey.cpp | 14 +- src/gui/entry/EditEntryWidget.cpp | 27 +-- src/gui/entry/EntryAttachmentsWidget.cpp | 32 ++-- tests/TestAutoType.cpp | 6 +- tests/gui/TestGui.cpp | 32 ++-- 15 files changed, 400 insertions(+), 232 deletions(-) diff --git a/src/browser/BrowserService.cpp b/src/browser/BrowserService.cpp index 6b85c7864..bf42eab31 100644 --- a/src/browser/BrowserService.cpp +++ b/src/browser/BrowserService.cpp @@ -34,6 +34,7 @@ #include "core/Metadata.h" #include "core/PasswordGenerator.h" #include "gui/MainWindow.h" +#include "gui/MessageBox.h" const char BrowserService::KEEPASSXCBROWSER_NAME[] = "KeePassXC-Browser Settings"; const char BrowserService::KEEPASSXCBROWSER_OLD_NAME[] = "keepassxc-browser Settings"; @@ -157,7 +158,7 @@ QString BrowserService::storeKey(const QString& key) } bool contains; - QMessageBox::StandardButton dialogResult = QMessageBox::No; + MessageBox::Button dialogResult = MessageBox::Cancel; do { QInputDialog keyDialog; @@ -180,14 +181,15 @@ QString BrowserService::storeKey(const QString& key) contains = db->metadata()->customData()->contains(QLatin1String(ASSOCIATE_KEY_PREFIX) + id); if (contains) { - dialogResult = QMessageBox::warning(nullptr, - tr("KeePassXC: Overwrite existing key?"), - tr("A shared encryption key with the name \"%1\" " - "already exists.\nDo you want to overwrite it?") - .arg(id), - QMessageBox::Yes | QMessageBox::No); + dialogResult = MessageBox::warning(nullptr, + tr("KeePassXC: Overwrite existing key?"), + tr("A shared encryption key with the name \"%1\" " + "already exists.\nDo you want to overwrite it?") + .arg(id), + MessageBox::Overwrite | MessageBox::Cancel, + MessageBox::Cancel); } - } while (contains && dialogResult == QMessageBox::No); + } while (contains && dialogResult == MessageBox::Cancel); db->metadata()->customData()->set(QLatin1String(ASSOCIATE_KEY_PREFIX) + id, key); return id; @@ -367,21 +369,17 @@ void BrowserService::updateEntry(const QString& id, if (username.compare(login, Qt::CaseSensitive) != 0 || entry->password().compare(password, Qt::CaseSensitive) != 0) { - int dialogResult = QMessageBox::No; + MessageBox::Button dialogResult = MessageBox::No; if (!browserSettings()->alwaysAllowUpdate()) { - QMessageBox msgBox; - msgBox.setWindowTitle(tr("KeePassXC: Update Entry")); - msgBox.setText(tr("Do you want to update the information in %1 - %2?").arg(QUrl(url).host(), username)); - msgBox.setStandardButtons(QMessageBox::Yes); - msgBox.addButton(QMessageBox::No); - msgBox.setDefaultButton(QMessageBox::No); - msgBox.setWindowFlags(Qt::WindowStaysOnTopHint); - msgBox.activateWindow(); - msgBox.raise(); - dialogResult = msgBox.exec(); + dialogResult = MessageBox::question(nullptr, + tr("KeePassXC: Update Entry"), + tr("Do you want to update the information in %1 - %2?") + .arg(QUrl(url).host(), username), + MessageBox::Save | MessageBox::Cancel, + MessageBox::Cancel); } - if (browserSettings()->alwaysAllowUpdate() || dialogResult == QMessageBox::Yes) { + if (browserSettings()->alwaysAllowUpdate() || dialogResult == MessageBox::Save) { entry->beginUpdate(); entry->setUsername(login); entry->setPassword(password); @@ -497,24 +495,24 @@ void BrowserService::convertAttributesToCustomData(QSharedPointer curr progress.reset(); if (counter > 0) { - QMessageBox::information(nullptr, - tr("KeePassXC: Converted KeePassHTTP attributes"), - tr("Successfully converted attributes from %1 entry(s).\n" - "Moved %2 keys to custom data.", - "") - .arg(counter) - .arg(keyCounter), - QMessageBox::Ok); + MessageBox::information(nullptr, + tr("KeePassXC: Converted KeePassHTTP attributes"), + tr("Successfully converted attributes from %1 entry(s).\n" + "Moved %2 keys to custom data.", + "") + .arg(counter) + .arg(keyCounter), + MessageBox::Ok); } else if (counter == 0 && keyCounter > 0) { - QMessageBox::information(nullptr, - tr("KeePassXC: Converted KeePassHTTP attributes"), - tr("Successfully moved %n keys to custom data.", "", keyCounter), - QMessageBox::Ok); + MessageBox::information(nullptr, + tr("KeePassXC: Converted KeePassHTTP attributes"), + tr("Successfully moved %n keys to custom data.", "", keyCounter), + MessageBox::Ok); } else { - QMessageBox::information(nullptr, - tr("KeePassXC: No entry with KeePassHTTP attributes found!"), - tr("The active database does not contain an entry with KeePassHTTP attributes."), - QMessageBox::Ok); + MessageBox::information(nullptr, + tr("KeePassXC: No entry with KeePassHTTP attributes found!"), + tr("The active database does not contain an entry with KeePassHTTP attributes."), + MessageBox::Ok); } // Rename password groupName @@ -901,13 +899,14 @@ bool BrowserService::checkLegacySettings() return false; } - auto dialogResult = QMessageBox::warning(nullptr, - tr("KeePassXC: Legacy browser integration settings detected"), - tr("Legacy browser integration settings have been detected.\n" - "Do you want to upgrade the settings to the latest standard?\n" - "This is necessary to maintain compatibility with the browser plugin."), - QMessageBox::Yes | QMessageBox::No); - return dialogResult == QMessageBox::Yes; + auto dialogResult = MessageBox::warning(nullptr, + tr("KeePassXC: Legacy browser integration settings detected"), + tr("Legacy browser integration settings have been detected.\n" + "Do you want to upgrade the settings to the latest standard?\n" + "This is necessary to maintain compatibility with the browser plugin."), + MessageBox::Yes | MessageBox::No); + + return dialogResult == MessageBox::Yes; } void BrowserService::databaseLocked(DatabaseWidget* dbWidget) diff --git a/src/core/Bootstrap.cpp b/src/core/Bootstrap.cpp index a62cc5a9b..2d8213b27 100644 --- a/src/core/Bootstrap.cpp +++ b/src/core/Bootstrap.cpp @@ -18,10 +18,12 @@ #include "Bootstrap.h" #include "core/Config.h" #include "core/Translator.h" +#include "gui/MessageBox.h" #ifdef Q_OS_WIN #include // for createWindowsDACL() #include // for Sleep(), SetDllDirectoryA(), SetSearchPathMode(), ... +#undef MessageBox #endif namespace Bootstrap @@ -56,6 +58,7 @@ namespace Bootstrap setupSearchPaths(); applyEarlyQNetworkAccessManagerWorkaround(); Translator::installTranslators(); + MessageBox::initializeButtonDefs(); #ifdef Q_OS_MACOS // Don't show menu icons on OSX diff --git a/src/gui/DatabaseTabWidget.cpp b/src/gui/DatabaseTabWidget.cpp index b45c5a8a5..21c90cfa0 100644 --- a/src/gui/DatabaseTabWidget.cpp +++ b/src/gui/DatabaseTabWidget.cpp @@ -98,8 +98,8 @@ QSharedPointer DatabaseTabWidget::execNewDatabaseWizard() tr("Database creation error"), tr("The created database has no key or KDF, refusing to save it.\n" "This is definitely a bug, please report it to the developers."), - QMessageBox::Ok, - QMessageBox::Ok); + MessageBox::Ok, + MessageBox::Ok); return {}; } diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp index 234f7e042..fb2d1a9a8 100644 --- a/src/gui/DatabaseWidget.cpp +++ b/src/gui/DatabaseWidget.cpp @@ -445,6 +445,7 @@ void DatabaseWidget::deleteEntries() bool inRecycleBin = recycleBin && recycleBin->findEntryByUuid(selectedEntries.first()->uuid()); if (inRecycleBin || !m_db->metadata()->recycleBinEnabled()) { QString prompt; + refreshSearch(); if (selected.size() == 1) { prompt = tr("Do you really want to delete the entry \"%1\" for good?") .arg(selectedEntries.first()->title().toHtmlEscaped()); @@ -452,12 +453,16 @@ void DatabaseWidget::deleteEntries() prompt = tr("Do you really want to delete %n entry(s) for good?", "", selected.size()); } - QMessageBox::StandardButton result = MessageBox::question( - this, tr("Delete entry(s)?", "", selected.size()), prompt, QMessageBox::Yes | QMessageBox::No); + auto answer = MessageBox::question(this, + tr("Delete entry(s)?", "", selected.size()), + prompt, + MessageBox::Delete | MessageBox::Cancel, + MessageBox::Cancel); - if (result == QMessageBox::Yes) { + if (answer == MessageBox::Delete) { for (Entry* entry : asConst(selectedEntries)) { delete entry; + refreshSearch(); } refreshSearch(); } @@ -470,10 +475,13 @@ void DatabaseWidget::deleteEntries() prompt = tr("Do you really want to move %n entry(s) to the recycle bin?", "", selected.size()); } - QMessageBox::StandardButton result = MessageBox::question( - this, tr("Move entry(s) to recycle bin?", "", selected.size()), prompt, QMessageBox::Yes | QMessageBox::No); + auto answer = MessageBox::question(this, + tr("Move entry(s) to recycle bin?", "", selected.size()), + prompt, + MessageBox::Move | MessageBox::Cancel, + MessageBox::Cancel); - if (result == QMessageBox::No) { + if (answer == MessageBox::Cancel) { return; } @@ -650,16 +658,27 @@ void DatabaseWidget::deleteGroup() bool isRecycleBin = recycleBin && (currentGroup == recycleBin); bool isRecycleBinSubgroup = recycleBin && currentGroup->findGroupByUuid(recycleBin->uuid()); if (inRecycleBin || isRecycleBin || isRecycleBinSubgroup || !m_db->metadata()->recycleBinEnabled()) { - QMessageBox::StandardButton result = MessageBox::question( - this, - tr("Delete group?"), - tr("Do you really want to delete the group \"%1\" for good?").arg(currentGroup->name().toHtmlEscaped()), - QMessageBox::Yes | QMessageBox::No); - if (result == QMessageBox::Yes) { + auto result = MessageBox::question(this, + tr("Delete group"), + tr("Do you really want to delete the group \"%1\" for good?") + .arg(currentGroup->name().toHtmlEscaped()), + MessageBox::Delete | MessageBox::Cancel, + MessageBox::Cancel); + + if (result == MessageBox::Delete) { delete currentGroup; } } else { - m_db->recycleGroup(currentGroup); + auto result = MessageBox::question(this, + tr("Move group to recycle bin?"), + tr("Do you really want to move the group " + "\"%1\" to the recycle bin?") + .arg(currentGroup->name().toHtmlEscaped()), + MessageBox::Move | MessageBox::Cancel, + MessageBox::Cancel); + if (result == MessageBox::Move) { + m_db->recycleGroup(currentGroup); + } } } @@ -1127,8 +1146,8 @@ bool DatabaseWidget::lock() if (currentMode() == DatabaseWidget::Mode::EditMode) { auto result = MessageBox::question(this, tr("Lock Database?"), tr("You are editing an entry. Discard changes and lock anyway?"), - QMessageBox::Discard | QMessageBox::Cancel, QMessageBox::Cancel); - if (result == QMessageBox::Cancel) { + MessageBox::Discard | MessageBox::Cancel, MessageBox::Cancel); + if (result == MessageBox::Cancel) { return false; } } @@ -1146,10 +1165,10 @@ bool DatabaseWidget::lock() msg = tr("Database was modified.\nSave changes?"); } auto result = MessageBox::question(this, tr("Save changes?"), msg, - QMessageBox::Yes | QMessageBox::Discard | QMessageBox::Cancel, QMessageBox::Yes); - if (result == QMessageBox::Yes && !m_db->save(nullptr, false, false)) { + MessageBox::Save | MessageBox::Discard | MessageBox::Cancel, MessageBox::Save); + if (result == MessageBox::Save && !m_db->save(nullptr, false, false)) { return false; - } else if (result == QMessageBox::Cancel) { + } else if (result == MessageBox::Cancel) { return false; } } @@ -1240,9 +1259,9 @@ void DatabaseWidget::reloadDatabaseFile() auto result = MessageBox::question(this, tr("File has changed"), tr("The database file has changed. Do you want to load the changes?"), - QMessageBox::Yes | QMessageBox::No); + MessageBox::Yes | MessageBox::No); - if (result == QMessageBox::No) { + if (result == MessageBox::No) { // Notify everyone the database does not match the file m_db->markAsModified(); // Rewatch the database file @@ -1259,9 +1278,10 @@ void DatabaseWidget::reloadDatabaseFile() auto result = MessageBox::question(this, tr("Merge Request"), tr("The database file has changed and you have unsaved changes.\nDo you want to merge your changes?"), - QMessageBox::Yes | QMessageBox::No); + MessageBox::Merge | MessageBox::Cancel, + MessageBox::Merge); - if (result == QMessageBox::Yes) { + if (result == MessageBox::Merge) { // Merge the old database into the new one Merger merger(m_db.data(), db.data()); merger.merge(); @@ -1447,14 +1467,14 @@ bool DatabaseWidget::save(int attempt) if (attempt > 2 && useAtomicSaves) { // Saving failed 3 times, issue a warning and attempt to resolve - auto choice = MessageBox::question(this, + auto result = MessageBox::question(this, tr("Disable safe saves?"), tr("KeePassXC has failed to save the database multiple times. " "This is likely caused by file sync services holding a lock on " "the save file.\nDisable safe saves and try again?"), - QMessageBox::Yes | QMessageBox::No, - QMessageBox::Yes); - if (choice == QMessageBox::Yes) { + MessageBox::Disable | MessageBox::Cancel, + MessageBox::Disable); + if (result == MessageBox::Disable) { config()->set("UseAtomicSaves", false); return save(attempt + 1); } @@ -1529,13 +1549,13 @@ void DatabaseWidget::emptyRecycleBin() return; } - QMessageBox::StandardButton result = - MessageBox::question(this, - tr("Empty recycle bin?"), - tr("Are you sure you want to permanently delete everything from your recycle bin?"), - QMessageBox::Yes | QMessageBox::No); + auto result = MessageBox::question(this, + tr("Empty recycle bin?"), + tr("Are you sure you want to permanently delete everything from your recycle bin?"), + MessageBox::Empty | MessageBox::Cancel, + MessageBox::Cancel); - if (result == QMessageBox::Yes) { + if (result == MessageBox::Empty) { m_db->emptyRecycleBin(); refreshSearch(); } diff --git a/src/gui/EditWidgetIcons.cpp b/src/gui/EditWidgetIcons.cpp index abb65c00d..6ac298255 100644 --- a/src/gui/EditWidgetIcons.cpp +++ b/src/gui/EditWidgetIcons.cpp @@ -416,16 +416,18 @@ void EditWidgetIcons::removeCustomIcon() int iconUseCount = entriesWithSameIcon.size() + groupsWithSameIcon.size(); if (iconUseCount > 0) { - QMessageBox::StandardButton ans = - MessageBox::question(this, - tr("Confirm Delete"), - tr("This icon is used by %n entry(s), and will be replaced " - "by the default icon. Are you sure you want to delete it?", - "", - iconUseCount), - QMessageBox::Yes | QMessageBox::No); - if (ans == QMessageBox::No) { + + auto result = MessageBox::question(this, + tr("Confirm Delete"), + tr("This icon is used by %n entry(s), and will be replaced " + "by the default icon. Are you sure you want to delete it?", + "", + iconUseCount), + MessageBox::Delete | MessageBox::Cancel, + MessageBox::Cancel); + + if (result == MessageBox::Cancel) { // Early out, nothing is changed return; } else { diff --git a/src/gui/EditWidgetProperties.cpp b/src/gui/EditWidgetProperties.cpp index bc0f6e164..d89bf62b9 100644 --- a/src/gui/EditWidgetProperties.cpp +++ b/src/gui/EditWidgetProperties.cpp @@ -68,13 +68,14 @@ const CustomData* EditWidgetProperties::customData() const void EditWidgetProperties::removeSelectedPluginData() { - if (QMessageBox::Yes - != MessageBox::question(this, - tr("Delete plugin data?"), - tr("Do you really want to delete the selected plugin data?\n" - "This may cause the affected plugins to malfunction."), - QMessageBox::Yes | QMessageBox::Cancel, - QMessageBox::Cancel)) { + auto result = MessageBox::question(this, + tr("Delete plugin data?"), + tr("Do you really want to delete the selected plugin data?\n" + "This may cause the affected plugins to malfunction."), + MessageBox::Delete | MessageBox::Cancel, + MessageBox::Cancel); + + if (result == MessageBox::Cancel) { return; } diff --git a/src/gui/MessageBox.cpp b/src/gui/MessageBox.cpp index 7aba6a2a6..1460a6c78 100644 --- a/src/gui/MessageBox.cpp +++ b/src/gui/MessageBox.cpp @@ -1,5 +1,6 @@ /* * Copyright (C) 2013 Felix Geyer + * Copyright (C) 2018 KeePassXC Team * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -17,69 +18,139 @@ #include "MessageBox.h" -QMessageBox::StandardButton MessageBox::m_nextAnswer(QMessageBox::NoButton); +MessageBox::Button MessageBox::m_nextAnswer(MessageBox::NoButton); -QMessageBox::StandardButton MessageBox::critical(QWidget* parent, - const QString& title, - const QString& text, - QMessageBox::StandardButtons buttons, - QMessageBox::StandardButton defaultButton) +QMap +MessageBox::m_addedButtonLookup = + QMap(); + +QMap> +MessageBox::m_buttonDefs = + QMap>(); + +void MessageBox::initializeButtonDefs() { - if (m_nextAnswer == QMessageBox::NoButton) { - return QMessageBox::critical(parent, title, text, buttons, defaultButton); + m_buttonDefs = + QMap> + { + // Reimplementation of Qt StandardButtons + {Ok, {stdButtonText(QMessageBox::Ok), QMessageBox::ButtonRole::AcceptRole}}, + {Open, {stdButtonText(QMessageBox::Open), QMessageBox::ButtonRole::AcceptRole}}, + {Save, {stdButtonText(QMessageBox::Save), QMessageBox::ButtonRole::AcceptRole}}, + {Cancel, {stdButtonText(QMessageBox::Cancel), QMessageBox::ButtonRole::RejectRole}}, + {Close, {stdButtonText(QMessageBox::Close), QMessageBox::ButtonRole::RejectRole}}, + {Discard, {stdButtonText(QMessageBox::Discard), QMessageBox::ButtonRole::DestructiveRole}}, + {Apply, {stdButtonText(QMessageBox::Apply), QMessageBox::ButtonRole::ApplyRole}}, + {Reset, {stdButtonText(QMessageBox::Reset), QMessageBox::ButtonRole::ResetRole}}, + {RestoreDefaults, {stdButtonText(QMessageBox::RestoreDefaults), QMessageBox::ButtonRole::ResetRole}}, + {Help, {stdButtonText(QMessageBox::Help), QMessageBox::ButtonRole::HelpRole}}, + {SaveAll, {stdButtonText(QMessageBox::SaveAll), QMessageBox::ButtonRole::AcceptRole}}, + {Yes, {stdButtonText(QMessageBox::Yes), QMessageBox::ButtonRole::YesRole}}, + {YesToAll, {stdButtonText(QMessageBox::YesToAll), QMessageBox::ButtonRole::YesRole}}, + {No, {stdButtonText(QMessageBox::No), QMessageBox::ButtonRole::NoRole}}, + {NoToAll, {stdButtonText(QMessageBox::NoToAll), QMessageBox::ButtonRole::NoRole}}, + {Abort, {stdButtonText(QMessageBox::Abort), QMessageBox::ButtonRole::RejectRole}}, + {Retry, {stdButtonText(QMessageBox::Retry), QMessageBox::ButtonRole::AcceptRole}}, + {Ignore, {stdButtonText(QMessageBox::Ignore), QMessageBox::ButtonRole::AcceptRole}}, + + // KeePassXC Buttons + {Overwrite, {QMessageBox::tr("Overwrite"), QMessageBox::ButtonRole::AcceptRole}}, + {Delete, {QMessageBox::tr("Delete"), QMessageBox::ButtonRole::AcceptRole}}, + {Move, {QMessageBox::tr("Move"), QMessageBox::ButtonRole::AcceptRole}}, + {Empty, {QMessageBox::tr("Empty"), QMessageBox::ButtonRole::AcceptRole}}, + {Remove, {QMessageBox::tr("Remove"), QMessageBox::ButtonRole::AcceptRole}}, + {Skip, {QMessageBox::tr("Skip"), QMessageBox::ButtonRole::AcceptRole}}, + {Disable, {QMessageBox::tr("Disable"), QMessageBox::ButtonRole::AcceptRole}}, + {Merge, {QMessageBox::tr("Merge"), QMessageBox::ButtonRole::AcceptRole}}, + }; +} + +QString MessageBox::stdButtonText(QMessageBox::StandardButton button) +{ + QMessageBox buttonHost; + return buttonHost.addButton(button)->text(); +} + +MessageBox::Button MessageBox::messageBox(QWidget* parent, + QMessageBox::Icon icon, + const QString& title, + const QString& text, + MessageBox::Buttons buttons, + MessageBox::Button defaultButton) +{ + if (m_nextAnswer == MessageBox::NoButton) { + QMessageBox msgBox(parent); + msgBox.setIcon(icon); + msgBox.setWindowTitle(title); + msgBox.setText(text); + + for (uint64_t b = First; b <= Last; b <<= 1) { + if (b & buttons) { + QString text = m_buttonDefs[static_cast