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.

Also, relatedly:

- Change the global ActionCollection object to be owned by the global
  MainWindow object, instead of being a singleton. This clarifies the
  fact that it represents the set of actions that belong to the main
  window.
- Change the ActionCollection API to support adding an action and
  setting a default keyboard shortcut for it at the same time. In
  MainWindow, take advantage of that to eliminate some repeated mentions
  of each action.
- Other small changes to the ActionCollection API.
- Change ActionCollection to store the default keyboard shortcut for
  each action in a map (QHash) instead of a property on the QAction.
  This seemed to be a bit nicer (and typesafe) since we had to introduce
  this map for another reason (i.e. to store the set of QShortcut
  instances in case the action had a shortcut that clashes with the
  system copy action).
- Minor code cleanups.

src/gui/ActionCollection.cpp

@@ -18,106 +18,144 @@
 #include "ActionCollection.h"
 #include "core/Config.h"
 
-#include <QDebug>
-
-ActionCollection* ActionCollection::instance()
+QList<QAction*> ActionCollection::actions() const
 {
-    static ActionCollection ac;
-    return &ac;
+    return m_actions.keys();
 }
 
-QList<QAction*> ActionCollection::actions() const
+void ActionCollection::addAction(QAction* action)
 {
-    return m_actions;
+    // Register any shortcut that may already be set on the action (e.g. from .ui file)
+    // as the default shortcut for the action.
+    addAction(action, action->shortcuts());
 }
 
-void ActionCollection::addAction(QAction* action)
+void ActionCollection::addAction(QAction* action, const QKeySequence& defaultShortcut)
 {
-    if (!m_actions.contains(action)) {
-        m_actions << action;
-    }
+    addAction(action, QList<QKeySequence>{defaultShortcut});
 }
 
-void ActionCollection::addActions(const QList<QAction*>& actions)
+void ActionCollection::addAction(QAction* action,
+                                 QKeySequence::StandardKey defaultShortcut,
+                                 const QKeySequence& fallback)
 {
-    for (auto a : actions) {
-        addAction(a);
+    if (!QKeySequence::keyBindings(defaultShortcut).isEmpty()) {
+        addAction(action, QKeySequence::keyBindings(defaultShortcut));
+    } else if (!fallback.isEmpty()) {
+        addAction(action, QKeySequence(fallback));
+    } else {
+        addAction(action);
     }
 }
 
-QKeySequence ActionCollection::defaultShortcut(const QAction* action) const
+void ActionCollection::addAction(QAction* action, const QList<QKeySequence>& defaultShortcuts)
 {
-    auto shortcuts = defaultShortcuts(action);
-    return shortcuts.isEmpty() ? QKeySequence() : shortcuts.first();
+    m_actions[action].defaultShortcuts = defaultShortcuts;
+    setShortcuts(action, defaultShortcuts);
 }
 
-QList<QKeySequence> ActionCollection::defaultShortcuts(const QAction* action) const
+QKeySequence ActionCollection::defaultShortcut(QAction* action) const
 {
-    return action->property("defaultShortcuts").value<QList<QKeySequence>>();
+    const auto shortcuts = m_actions.value(action).defaultShortcuts;
+    return shortcuts.isEmpty() ? QKeySequence() : shortcuts.first();
 }
 
-void ActionCollection::setDefaultShortcut(QAction* action, const QKeySequence& shortcut)
+const QKeySequence ActionCollection::shortcut(QAction* a) const
 {
-    setDefaultShortcuts(action, {shortcut});
+    const ActionInfo& actionInfo = m_actions.value(a);
+    if (!actionInfo.interceptedShortcuts.isEmpty()) {
+        return actionInfo.interceptedShortcuts.front()->key();
+    }
+    return a->shortcut();
 }
 
-void ActionCollection::setDefaultShortcut(QAction* action,
-                                          QKeySequence::StandardKey standard,
-                                          const QKeySequence& fallback)
+void ActionCollection::setShortcuts(QAction* action, const QList<QKeySequence>& shortcuts)
 {
-    if (!QKeySequence::keyBindings(standard).isEmpty()) {
-        setDefaultShortcuts(action, QKeySequence::keyBindings(standard));
-    } else if (fallback != 0) {
-        setDefaultShortcut(action, QKeySequence(fallback));
+    // For any provided shortcuts that match the system shortcut for copy-to-clipboard,
+    // instead of registering them directly with the action, give the
+    // m_copyShortcutActionCallback a chance to intercept the event.
+
+    ActionInfo& actionInfo = m_actions[action];
+    for (const auto& qshortcut : actionInfo.interceptedShortcuts) {
+        delete qshortcut.data();
+    }
+    actionInfo.interceptedShortcuts.clear();
+
+    static const auto StandardCopyShortcuts = QKeySequence::keyBindings(QKeySequence::Copy);
+
+    QList<QKeySequence> otherShortcuts;
+    for (const QKeySequence& shortcut : shortcuts) {
+        if (StandardCopyShortcuts.contains(shortcut)) {
+            const auto interceptedShortcut = new QShortcut(action->parentWidget());
+            interceptedShortcut->setKey(shortcut);
+            connect(interceptedShortcut, &QShortcut::activated, this, [this, action]() {
+                if (m_copyShortcutActionCallback && m_copyShortcutActionCallback()) {
+                    return;
+                }
+                action->activate(QAction::Trigger);
+            });
+            actionInfo.interceptedShortcuts.append(interceptedShortcut);
+        } else {
+            otherShortcuts.append(shortcut);
+        }
     }
+
+    action->setShortcuts(otherShortcuts);
 }
 
-void ActionCollection::setDefaultShortcuts(QAction* action, const QList<QKeySequence>& shortcuts)
+void ActionCollection::restoreShortcutsFromDefaults()
 {
-    action->setShortcuts(shortcuts);
-    action->setProperty("defaultShortcuts", QVariant::fromValue(shortcuts));
+    for (auto it = m_actions.constBegin(), end = m_actions.constEnd(); it != end; ++it) {
+        setShortcuts(it.key(), it.value().defaultShortcuts);
+    }
 }
 
-void ActionCollection::restoreShortcuts()
+void ActionCollection::restoreShortcutsFromConfig()
 {
-    const auto shortcuts = Config::instance()->getShortcuts();
+    const auto configShortcuts = Config::instance()->getShortcuts();
     QHash<QString, QAction*> actionsByName;
-    for (auto action : m_actions) {
-        actionsByName.insert(action->objectName(), action);
+    for (auto it = m_actions.keyBegin(), end = m_actions.keyEnd(); it != end; ++it) {
+        actionsByName.insert((*it)->objectName(), *it);
     }
-    for (const auto& shortcut : shortcuts) {
-        if (actionsByName.contains(shortcut.name)) {
-            const auto key = QKeySequence::fromString(shortcut.shortcut);
-            actionsByName.value(shortcut.name)->setShortcut(key);
+    for (const auto& configShortcut : configShortcuts) {
+        if (actionsByName.contains(configShortcut.name)) {
+            const auto shortcut = QKeySequence::fromString(configShortcut.shortcut);
+            setShortcuts(actionsByName.value(configShortcut.name), {shortcut});
         }
     }
 }
 
-void ActionCollection::saveShortcuts()
+void ActionCollection::saveShortcutsToConfig()
 {
-    QList<Config::ShortcutEntry> shortcuts;
-    shortcuts.reserve(m_actions.size());
-    for (auto a : m_actions) {
+    QList<Config::ShortcutEntry> configShortcuts;
+    configShortcuts.reserve(m_actions.size());
+    for (auto it = m_actions.keyBegin(), end = m_actions.keyEnd(); it != end; ++it) {
         // Only store non-default shortcut assignments
-        if (a->shortcut() != defaultShortcut(a)) {
-            shortcuts << Config::ShortcutEntry{a->objectName(), a->shortcut().toString()};
+        const auto s = shortcut(*it);
+        if (s != defaultShortcut(*it)) {
+            configShortcuts << Config::ShortcutEntry{(*it)->objectName(), s.toString()};
         }
     }
-    Config::instance()->setShortcuts(shortcuts);
+    Config::instance()->setShortcuts(configShortcuts);
 }
 
-QAction* ActionCollection::isConflictingShortcut(const QAction* action, const QKeySequence& seq) const
+QAction* ActionCollection::getConflictingShortcut(const QAction* action, const QKeySequence& seq) const
 {
     // Empty sequences don't conflict with anything
     if (seq.isEmpty()) {
         return nullptr;
     }
 
-    for (auto a : m_actions) {
-        if (a != action && a->shortcut() == seq) {
-            return a;
+    for (auto it = m_actions.keyBegin(), end = m_actions.keyEnd(); it != end; ++it) {
+        if (*it != action && shortcut(*it) == seq) {
+            return *it;
         }
     }
 
     return nullptr;
 }
+
+void ActionCollection::setCopyShortcutActionCallback(const std::function<bool()>& callback)
+{
+    m_copyShortcutActionCallback = callback;
+}

src/gui/ActionCollection.h

@@ -19,8 +19,13 @@
 #define KEEPASSXC_ACTION_COLLECTION_H
 
 #include <QAction>
+#include <QHash>
 #include <QKeySequence>
 #include <QObject>
+#include <QPointer>
+#include <QShortcut>
+
+#include <functional>
 
 /**
  * This class manages all actions that are shortcut configurable.
@@ -30,32 +35,48 @@
 class ActionCollection : public QObject
 {
     Q_OBJECT
-    ActionCollection() = default;
 
 public:
-    static ActionCollection* instance();
+    ActionCollection() = default;
 
     QList<QAction*> actions() const;
+    void addAction(QAction* a);
+    void addAction(QAction* a, const QKeySequence& defaultShortcut);
+    void addAction(QAction* a, QKeySequence::StandardKey defaultShortcut, const QKeySequence& fallback);
+    void addAction(QAction* a, const QList<QKeySequence>& defaultShortcuts);
 
-    void addAction(QAction* action);
-    void addActions(const QList<QAction*>& actions);
-
-    QKeySequence defaultShortcut(const QAction* a) const;
-    QList<QKeySequence> defaultShortcuts(const QAction* a) const;
+    QKeySequence defaultShortcut(QAction* a) const;
+    const QKeySequence shortcut(QAction* a) const;
+    void setShortcuts(QAction* a, const QList<QKeySequence>& keys);
 
-    void setDefaultShortcut(QAction* a, const QKeySequence& shortcut);
-    void setDefaultShortcut(QAction* a, QKeySequence::StandardKey standard, const QKeySequence& fallback);
-    void setDefaultShortcuts(QAction* a, const QList<QKeySequence>& shortcut);
+    // Check if any action conflicts with @p seq and return the conflicting action or nullptr
+    QAction* getConflictingShortcut(const QAction* action, const QKeySequence& seq) const;
 
-    // Check if any action conflicts with @p seq and return the conflicting action
-    QAction* isConflictingShortcut(const QAction* action, const QKeySequence& seq) const;
+    // Register a callback function that gets invoked when a shortcut gets triggered
+    // that has been registered with an action, but that is also a system shortcut
+    // for the copy-to-clipboard action. If the callback function return true, the
+    // action is not triggered.
+    void setCopyShortcutActionCallback(const std::function<bool()>& callback);
 
-public slots:
-    void restoreShortcuts();
-    void saveShortcuts();
+    void restoreShortcutsFromDefaults();
+    void restoreShortcutsFromConfig();
+    void saveShortcutsToConfig();
 
 private:
-    QList<QAction*> m_actions;
+    struct ActionInfo
+    {
+        // Default shortcut for action
+        QList<QKeySequence> defaultShortcuts;
+
+        // Shortcuts for action that conflict with system-wide shortcuts;
+        // these get special handling. (Normally, shortcuts are set directly on the action.)
+        QList<QPointer<QShortcut>> interceptedShortcuts;
+    };
+
+    QHash<QAction*, ActionInfo> m_actions;
+    std::function<bool()> m_copyShortcutActionCallback;
+
+    Q_DISABLE_COPY(ActionCollection)
 };
 
 #endif

src/gui/DatabaseWidget.cpp

@@ -658,29 +658,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()));
@@ -762,6 +739,34 @@ void DatabaseWidget::setClipboardTextAndMinimize(const QString& text)
     }
 }
 
+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;
+}
+
 #ifdef WITH_XC_SSHAGENT
 void DatabaseWidget::addToAgent()
 {

src/gui/DatabaseWidget.h

@@ -183,6 +183,7 @@ public slots:
     void copyTotp();
     void copyPasswordTotp();
     void setupTotp();
+    bool copyFocusedTextSelection();
 #ifdef WITH_XC_SSHAGENT
     void addToAgent();
     void removeFromAgent();