Show conflicting shortcut in error message (#5287)

cockatrice/src/settings/shortcuts_settings.cpp

@@ -114,6 +114,17 @@ QString ShortcutsSettings::getShortcutString(const QString &name) const
     return stringifySequence(getShortcut(name));
 }
 
+QString ShortcutsSettings::getShortcutFriendlyName(const QString &shortcutName) const
+{
+    for (auto it = defaultShortCuts.cbegin(); it != defaultShortCuts.cend(); ++it) {
+        if (shortcutName == it.key()) {
+            return it.value().getName();
+        }
+    }
+
+    return {};
+}
+
 QString ShortcutsSettings::stringifySequence(const QList<QKeySequence> &Sequence) const
 {
     QStringList stringSequence;
@@ -150,9 +161,9 @@ void ShortcutsSettings::setShortcuts(const QString &name, const QKeySequence &Se
     setShortcuts(name, QList<QKeySequence>{Sequence});
 }
 
-void ShortcutsSettings::setShortcuts(const QString &name, const QString &Sequences)
+void ShortcutsSettings::setShortcuts(const QString &name, const QString &sequences)
 {
-    setShortcuts(name, parseSequenceString(Sequences));
+    setShortcuts(name, parseSequenceString(sequences));
 }
 
 void ShortcutsSettings::resetAllShortcuts()
@@ -177,33 +188,45 @@ void ShortcutsSettings::clearAllShortcuts()
     emit shortCutChanged();
 }
 
-bool ShortcutsSettings::isKeyAllowed(const QString &name, const QString &Sequences) const
+bool ShortcutsSettings::isKeyAllowed(const QString &name, const QString &sequences) const
 {
     // if the shortcut is not to be used in deck-editor then it doesn't matter
     if (name.startsWith("Player") || name.startsWith("Replays")) {
         return true;
     }
-    QString checkSequence = Sequences.split(sep).last();
+    QString checkSequence = sequences.split(sep).last();
     QStringList forbiddenKeys{"Del",        "Backspace", "Down",  "Up",         "Left",       "Right",
                               "Return",     "Enter",     "Menu",  "Ctrl+Alt+-", "Ctrl+Alt+=", "Ctrl+Alt+[",
                               "Ctrl+Alt+]", "Tab",       "Space", "Shift+S",    "Shift+Left", "Shift+Right"};
     return !forbiddenKeys.contains(checkSequence);
 }
 
-bool ShortcutsSettings::isValid(const QString &name, const QString &Sequences) const
+/**
+ *  Checks that the shortcut doesn't overlap with an existing shortcut
+ *
+ * @param name The name of the shortcut
+ * @param sequences The shortcut key sequence
+ * @return Whether the shortcut is valid.
+ */
+bool ShortcutsSettings::isValid(const QString &name, const QString &sequences) const
 {
-    QString checkSequence = Sequences.split(sep).last();
+    return findOverlaps(name, sequences).isEmpty();
+}
+
+QStringList ShortcutsSettings::findOverlaps(const QString &name, const QString &sequences) const
+{
+    QString checkSequence = sequences.split(sep).last();
     QString checkKey = name.left(name.indexOf("/"));
 
-    QList<QString> allKeys = shortCuts.keys();
-    for (const auto &key : allKeys) {
+    QStringList overlaps;
+    for (const auto &key : shortCuts.keys()) {
         if (key.startsWith(checkKey) || key.startsWith("MainWindow") || checkKey.startsWith("MainWindow")) {
             QString storedSequence = stringifySequence(shortCuts.value(key));
-            QStringList stringSequences = storedSequence.split(sep);
-            if (stringSequences.contains(checkSequence)) {
-                return false;
+            if (storedSequence.split(sep).contains(checkSequence)) {
+                overlaps.append(getShortcutFriendlyName(key));
             }
         }
     }
-    return true;
-}
+
+    return overlaps;
+}

cockatrice/src/settings/shortcuts_settings.h

@@ -2,9 +2,7 @@
 #define SHORTCUTSSETTINGS_H
 
 #include <QApplication>
-#include <QHash>
 #include <QKeySequence>
-#include <QObject>
 #include <QSettings>
 
 class ShortcutGroup
@@ -80,18 +78,18 @@ public:
 class ShortcutKey : public QList<QKeySequence>
 {
 public:
-    ShortcutKey(const QString &_name = QString(),
-                QList<QKeySequence> _sequence = QList<QKeySequence>(),
-                ShortcutGroup::Groups _group = ShortcutGroup::Main_Window);
-    void setSequence(QList<QKeySequence> _sequence)
+    explicit ShortcutKey(const QString &_name = QString(),
+                         QList _sequence = QList(),
+                         ShortcutGroup::Groups _group = ShortcutGroup::Main_Window);
+    void setSequence(const QList &_sequence)
     {
-        QList<QKeySequence>::operator=(_sequence);
+        QList::operator=(_sequence);
     };
-    const QString getName() const
+    QString getName() const
     {
         return QApplication::translate("shortcutsTab", name.toUtf8().data());
     };
-    const QString getGroupName() const
+    QString getGroupName() const
     {
         return ShortcutGroup::getGroupName(group);
     };
@@ -105,13 +103,14 @@ class ShortcutsSettings : public QObject
 {
     Q_OBJECT
 public:
-    ShortcutsSettings(const QString &settingsFilePath, QObject *parent = nullptr);
+    explicit ShortcutsSettings(const QString &settingsFilePath, QObject *parent = nullptr);
 
     ShortcutKey getDefaultShortcut(const QString &name) const;
     ShortcutKey getShortcut(const QString &name) const;
     QKeySequence getSingleShortcut(const QString &name) const;
     QString getDefaultShortcutString(const QString &name) const;
     QString getShortcutString(const QString &name) const;
+    QString getShortcutFriendlyName(const QString &shortcutName) const;
     QList<QString> getAllShortcutKeys() const
     {
         return shortCuts.keys();
@@ -119,10 +118,11 @@ public:
 
     void setShortcuts(const QString &name, const QList<QKeySequence> &Sequence);
     void setShortcuts(const QString &name, const QKeySequence &Sequence);
-    void setShortcuts(const QString &name, const QString &Sequences);
+    void setShortcuts(const QString &name, const QString &sequences);
 
-    bool isKeyAllowed(const QString &name, const QString &Sequences) const;
-    bool isValid(const QString &name, const QString &Sequences) const;
+    bool isKeyAllowed(const QString &name, const QString &sequences) const;
+    bool isValid(const QString &name, const QString &sequences) const;
+    QStringList findOverlaps(const QString &name, const QString &sequences) const;
 
     void resetAllShortcuts();
     void clearAllShortcuts();

cockatrice/src/utility/sequence_edit.cpp

@@ -154,26 +154,49 @@ int SequenceEdit::translateModifiers(Qt::KeyboardModifiers state, const QString
     return result;
 }
 
+/**
+ *Validates that shortcut is valid (is a valid shortcut key sequence and doesn't conflict with any other shortcuts).
+ *Displays warning messages if it's not valid.
+ *
+ * @param sequence The shortcut key sequence
+ * @return True if the sequence isn't already self-contained
+ */
+bool SequenceEdit::validateShortcut(const QKeySequence &sequence)
+{
+    if (sequence.isEmpty() || !valid) {
+        return true;
+    }
+
+    const auto &shortcutsSettings = SettingsCache::instance().shortcuts();
+    const QString sequenceString = sequence.toString();
+
+    if (!shortcutsSettings.isKeyAllowed(shortcutName, sequenceString)) {
+        QToolTip::showText(lineEdit->mapToGlobal(QPoint()), tr("Invalid key"));
+        return true;
+    }
+
+    if (!shortcutsSettings.isValid(shortcutName, sequenceString)) {
+        auto overlaps = shortcutsSettings.findOverlaps(shortcutName, sequenceString);
+        QToolTip::showText(lineEdit->mapToGlobal(QPoint()),
+                           tr("Shortcut already in use by:") + " " + overlaps.join(", "));
+        return true;
+    }
+
+    if (!lineEdit->text().isEmpty()) {
+        if (lineEdit->text().contains(sequenceString)) {
+            return false;
+        }
+        lineEdit->setText(lineEdit->text() + ";");
+    }
+    lineEdit->setText(lineEdit->text() + sequenceString);
+
+    return true;
+}
+
 void SequenceEdit::finishShortcut()
 {
-    QKeySequence sequence(keys);
-    if (!sequence.isEmpty() && valid) {
-        QString sequenceString = sequence.toString();
-        if (SettingsCache::instance().shortcuts().isKeyAllowed(shortcutName, sequenceString)) {
-            if (SettingsCache::instance().shortcuts().isValid(shortcutName, sequenceString)) {
-                if (!lineEdit->text().isEmpty()) {
-                    if (lineEdit->text().contains(sequenceString)) {
-                        return;
-                    }
-                    lineEdit->setText(lineEdit->text() + ";");
-                }
-                lineEdit->setText(lineEdit->text() + sequenceString);
-            } else {
-                QToolTip::showText(lineEdit->mapToGlobal(QPoint()), tr("Shortcut already in use"));
-            }
-        } else {
-            QToolTip::showText(lineEdit->mapToGlobal(QPoint()), tr("Invalid key"));
-        }
+    if (!validateShortcut(QKeySequence(keys))) {
+        return;
     }
 
     currentKey = 0;

cockatrice/src/utility/sequence_edit.h

@@ -36,6 +36,7 @@ private:
 
     void processKey(QKeyEvent *e);
     int translateModifiers(Qt::KeyboardModifiers state, const QString &text);
+    bool validateShortcut(const QKeySequence &sequence);
     void finishShortcut();
     void updateSettings();
 };