From f1079a8d5aade47825a57faebd5fc80105330f30 Mon Sep 17 00:00:00 2001 From: Carlo Teubner Date: Tue, 11 Jun 2024 21:39:07 +0100 Subject: [PATCH] Fix Copy Password button when text is selected When the user chooses to copy the password for an entry to the clipboard, previously there was logic to check if text was selected, and if so, that text was instead copied to the clipboard. That made sense if (a) the user invoked the Copy Password action via its keyboard shortcut, and (b) that keyboard shortcut was configured (as per default) to be Ctrl-C, i.e. the same as the system action for copy-to-clipboard. However, it made no sense if the user invoked that action in some other way, for example by clicking the corresponding toolbar button. It also made no sense in the case that the Copy Password action had some other keyboard shortcut assigned. Also, if some other action had Ctrl-C assigned, the logic would not kick in then. Fix all of the above by modifying the keyboard shortcut logic to intervene precisely in the case where a shortcut is pressed that matches the system copy-to-clipboard shortcut; only in that case do we now check if text is selected and if so copy that to the clipboard instead of the action we would otherwise take. Add a test case to TestGui.cpp. Fixes #10734. --- src/gui/DatabaseWidget.cpp | 51 +++++++++++++++++++++----------------- src/gui/DatabaseWidget.h | 1 + src/gui/MainWindow.cpp | 30 ++++++++++++++++++++++ src/gui/MainWindow.h | 10 ++++++++ tests/gui/TestGui.cpp | 13 ++++++++++ 5 files changed, 82 insertions(+), 23 deletions(-) diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp index 4087b74e41..fb4297c6d6 100644 --- a/src/gui/DatabaseWidget.cpp +++ b/src/gui/DatabaseWidget.cpp @@ -674,29 +674,6 @@ void DatabaseWidget::copyUsername() void DatabaseWidget::copyPassword() { - // Some platforms do not properly trap Ctrl+C copy shortcut - // if a text edit or label has focus pass the copy operation to it - - bool clearClipboard = config()->get(Config::Security_ClearClipboard).toBool(); - - auto plainTextEdit = qobject_cast(focusWidget()); - if (plainTextEdit && plainTextEdit->textCursor().hasSelection()) { - clipboard()->setText(plainTextEdit->textCursor().selectedText(), clearClipboard); - return; - } - - auto label = qobject_cast(focusWidget()); - if (label && label->hasSelectedText()) { - clipboard()->setText(label->selectedText(), clearClipboard); - return; - } - - auto textEdit = qobject_cast(focusWidget()); - if (textEdit && textEdit->textCursor().hasSelection()) { - clipboard()->setText(textEdit->textCursor().selection().toPlainText(), clearClipboard); - return; - } - auto currentEntry = currentSelectedEntry(); if (currentEntry) { setClipboardTextAndMinimize(currentEntry->resolveMultiplePlaceholders(currentEntry->password())); @@ -737,6 +714,34 @@ void DatabaseWidget::copyAttribute(QAction* action) } } +bool DatabaseWidget::copyFocusedTextSelection() +{ + // If a focused child widget has text selected, copy that text to the clipboard + // and return true. Otherwise, return false. + + const bool clearClipboard = config()->get(Config::Security_ClearClipboard).toBool(); + + const auto plainTextEdit = qobject_cast(focusWidget()); + if (plainTextEdit && plainTextEdit->textCursor().hasSelection()) { + clipboard()->setText(plainTextEdit->textCursor().selectedText(), clearClipboard); + return true; + } + + const auto label = qobject_cast(focusWidget()); + if (label && label->hasSelectedText()) { + clipboard()->setText(label->selectedText(), clearClipboard); + return true; + } + + const auto textEdit = qobject_cast(focusWidget()); + if (textEdit && textEdit->textCursor().hasSelection()) { + clipboard()->setText(textEdit->textCursor().selection().toPlainText(), clearClipboard); + return true; + } + + return false; +} + void DatabaseWidget::filterByTag() { QStringList searchTerms; diff --git a/src/gui/DatabaseWidget.h b/src/gui/DatabaseWidget.h index 8f71a06765..6622394e1a 100644 --- a/src/gui/DatabaseWidget.h +++ b/src/gui/DatabaseWidget.h @@ -189,6 +189,7 @@ public slots: void copyURL(); void copyNotes(); void copyAttribute(QAction* action); + bool copyFocusedTextSelection(); void filterByTag(); void setTag(QAction* action); void showTotp(); diff --git a/src/gui/MainWindow.cpp b/src/gui/MainWindow.cpp index a6a3515b4d..0bd949f10e 100644 --- a/src/gui/MainWindow.cpp +++ b/src/gui/MainWindow.cpp @@ -80,6 +80,28 @@ #include "mainwindowadaptor.h" #endif +// This filter gets installed on all the QAction objects within the MainWindow. +bool ActionEventFilter::eventFilter(QObject* watched, QEvent* event) +{ + auto databaseWidget = getMainWindow()->m_ui->tabWidget->currentDatabaseWidget(); + if (databaseWidget && event->type() == QEvent::Shortcut) { + // We check if we got a Shortcut event that uses the same key sequence as the + // OS default copy-to-clipboard shortcut. + static const auto stdCopyShortcuts = QKeySequence::keyBindings(QKeySequence::Copy); + if (stdCopyShortcuts.contains(static_cast(event)->key())) { + // If so, we ask the database widget to check if any of its sub-widgets has text + // selected, and to copy it to the clipboard if that is the case. We do this + // because that is what the user likely expects to happen, yet Qt does not + // behave like that on all platforms. + if (databaseWidget->copyFocusedTextSelection()) { + // In that case, we return true to stop further processing of this event. + return true; + } + } + } + return QObject::eventFilter(watched, event); +} + const QString MainWindow::BaseWindowTitle = "KeePassXC"; MainWindow* g_MainWindow = nullptr; @@ -2214,6 +2236,14 @@ void MainWindow::initActionCollection() ac->setDefaultShortcut(m_ui->actionEntryRemoveFromAgent, Qt::META + Qt::SHIFT + Qt::Key_H); #endif + // Install an event filter on every action. It improves handling of keyboard + // shortcuts that match the system copy-to-clipboard key sequence; by default + // this applies to actionEntryCopyPassword, but this could differ based on + // shortcuts the user has configured, or may configure later. + for (auto action : ac->actions()) { + action->installEventFilter(&m_actionEventFilter); + } + QTimer::singleShot(1, ac, &ActionCollection::restoreShortcuts); } diff --git a/src/gui/MainWindow.h b/src/gui/MainWindow.h index 7155bd1102..fcc90f4b06 100644 --- a/src/gui/MainWindow.h +++ b/src/gui/MainWindow.h @@ -39,6 +39,14 @@ class InactivityTimer; class SearchWidget; class MainWindowEventFilter; +class ActionEventFilter : public QObject +{ + Q_OBJECT + +protected: + bool eventFilter(QObject* obj, QEvent* event) override; +}; + class MainWindow : public QMainWindow { Q_OBJECT @@ -171,6 +179,7 @@ private slots: const QScopedPointer m_ui; SignalMultiplexer m_actionMultiplexer; + ActionEventFilter m_actionEventFilter; QPointer m_clearHistoryAction; QPointer m_searchWidgetAction; QPointer m_entryContextMenu; @@ -202,6 +211,7 @@ private slots: QTimer m_trayIconTriggerTimer; QSystemTrayIcon::ActivationReason m_trayIconTriggerReason; + friend class ActionEventFilter; friend class MainWindowEventFilter; }; diff --git a/tests/gui/TestGui.cpp b/tests/gui/TestGui.cpp index 03cb5e347d..d520741839 100644 --- a/tests/gui/TestGui.cpp +++ b/tests/gui/TestGui.cpp @@ -1153,6 +1153,19 @@ void TestGui::testSearch() searchTextEdit->selectAll(); QTest::keyClick(searchTextEdit, Qt::Key_C, Qt::ControlModifier); QTRY_COMPARE(clipboard->text(), QString("someTHING")); + // Ensure password copies when clicking on copy password button despite selected text + auto copyPasswordAction = m_mainWindow->findChild("actionEntryCopyPassword"); + QVERIFY(copyPasswordAction); + auto copyPasswordWidget = toolBar->widgetForAction(copyPasswordAction); + QVERIFY(copyPasswordWidget); + QTest::mouseClick(copyPasswordWidget, Qt::LeftButton); + QCOMPARE(clipboard->text(), searchedEntry->password()); + // Deselect text and deselect entry, Ctrl+C should now do nothing + clipboard->clear(); + QTest::mouseClick(searchTextEdit, Qt::LeftButton); + entryView->clearSelection(); + QTest::keyClick(searchTextEdit, Qt::Key_C, Qt::ControlModifier); + QCOMPARE(clipboard->text(), QString()); // Test case sensitive search searchWidget->setCaseSensitive(true);