Fix Copy Password button when text is selected (#10853)

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.

Fixes #10734.

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<QPlainTextEdit*>(focusWidget());
-    if (plainTextEdit && plainTextEdit->textCursor().hasSelection()) {
-        clipboard()->setText(plainTextEdit->textCursor().selectedText(), clearClipboard);
-        return;
-    }
-
-    auto label = qobject_cast<QLabel*>(focusWidget());
-    if (label && label->hasSelectedText()) {
-        clipboard()->setText(label->selectedText(), clearClipboard);
-        return;
-    }
-
-    auto textEdit = qobject_cast<QTextEdit*>(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<QPlainTextEdit*>(focusWidget());
+    if (plainTextEdit && plainTextEdit->textCursor().hasSelection()) {
+        clipboard()->setText(plainTextEdit->textCursor().selectedText(), clearClipboard);
+        return true;
+    }
+
+    const auto label = qobject_cast<QLabel*>(focusWidget());
+    if (label && label->hasSelectedText()) {
+        clipboard()->setText(label->selectedText(), clearClipboard);
+        return true;
+    }
+
+    const auto textEdit = qobject_cast<QTextEdit*>(focusWidget());
+    if (textEdit && textEdit->textCursor().hasSelection()) {
+        clipboard()->setText(textEdit->textCursor().selection().toPlainText(), clearClipboard);
+        return true;
+    }
+
+    return false;
+}
+
 void DatabaseWidget::filterByTag()
 {
     QStringList searchTerms;

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();

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<QShortcutEvent*>(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);
 }
 

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:
 
     const QScopedPointer<Ui::MainWindow> m_ui;
     SignalMultiplexer m_actionMultiplexer;
+    ActionEventFilter m_actionEventFilter;
     QPointer<QAction> m_clearHistoryAction;
     QPointer<QAction> m_searchWidgetAction;
     QPointer<QMenu> m_entryContextMenu;
@@ -202,6 +211,7 @@ private:
     QTimer m_trayIconTriggerTimer;
     QSystemTrayIcon::ActivationReason m_trayIconTriggerReason;
 
+    friend class ActionEventFilter;
     friend class MainWindowEventFilter;
 };
 

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<QAction*>("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);