Merge pull request #25126 from cbjeukendrup/macos_shortcuts_fix

Fix custom shortcuts on macOS

src/framework/shortcuts/internal/platform/macos/macosshortcutsinstancemodel.mm

@@ -372,39 +372,88 @@ static QString translateToCurrentKeyboardLayout(const QKeySequence& sequence)
 void MacOSShortcutsInstanceModel::doLoadShortcuts()
 {
     m_shortcuts.clear();
+
+    // RULE: If a sequence is used for several shortcuts but the values for autoRepeat vary depending on
+    // the context, then we should force autoRepeat to false for all shortcuts sharing the sequence in
+    // question. This prevents the creation of ambiguous shortcuts (see QShortcutEvent::isAmbiguous)
+    auto recordAutoRepeat = [this](const QString& sequence, bool autoRepeat) {
+        auto search = m_shortcuts.find(sequence);
+        if (search == m_shortcuts.end()) {
+            // Sequence not found, add it...
+            m_shortcuts.insert(sequence, QVariant(autoRepeat));
+        } else if (search.value().toBool() && !autoRepeat) {
+            // Sequence already exists, but we need to enforce the above rule...
+            search.value() = false;
+        }
+    };
+
     m_macSequenceMap.clear();
 
+    // Suppose the user uses for example a Cyrillic keyboard layout. When they try to press Cmd+A,
+    // Qt will see Cmd+Ф instead, so the QML `Shortcut` object for Cmd+A will not be triggered.
+    // We will a workaround: we treat the shortcuts from the shortcuts file as "a combination of
+    // physical keys" rather than "a character that would be typed"; i.e. `Shift+,` rather than `<`.
+    // We use native macOS APIs to translate from the combination of keys to the character that would
+    // be typed. We create a QML `Shortcut` object for e.g. `Cmd+Ф`; that will be triggered by Qt.
+    // We store a reverse mapping here, so that we can look up the original sequence when the shortcut
+    // is triggered, and then trigger the corresponding action.
+    //
+    // There is one catch; there are two cases where the untranslated sequence is not a combination of
+    // physical keys:
+    // - When the current keyboard layout is different from what the shortcuts file was designed for;
+    //   for example, the AZERTY layout does not have a `.` key, but instead a `.` is typed as `Shift+;`.
+    // - When the sequence was recorded as a character, e.g. `>` rather than `Shift+.`, which is the case
+    //   for all custom shortcuts.
+    // In these cases, the translation will produce unexpected results. Therefore, in our reverse map,
+    // we also store a mapping from the untranslated sequence to the untranslated sequence itself. This
+    // takes precedence over mapping from translated sequences to untranslated sequences.
+    //
+    // The resulting behaviour when a shortcut is pressed by the user is the following:
+    // - If there is an action that is bound to exactly the sequence as Qt sees it, then that action is
+    //   triggered.
+    // - Otherwise, we look up the received sequence in the reverse map, to see by which combination of
+    //   physical keys it would be produced, and trigger the action that is bound to that combination.
+    auto recordMapping = [this](const QString& translatedSequence, const QString& rawSequence, bool overrideExisting) {
+        auto search = m_macSequenceMap.find(translatedSequence);
+        if (search == m_macSequenceMap.end()) {
+            m_macSequenceMap.insert(translatedSequence, rawSequence);
+        } else if (overrideExisting) {
+            search.value() = rawSequence;
+        }
+    };
+
     ShortcutList shortcuts = shortcutsRegister()->shortcuts();
 
     for (const Shortcut& sc : shortcuts) {
         for (const std::string& seq : sc.sequences) {
-            QString sequence = QString::fromStdString(seq);
+            QString untranslatedSequence = QString::fromStdString(seq);
+
+            // Always record the untranslated sequence
+            recordMapping(untranslatedSequence, untranslatedSequence, true);
+            recordAutoRepeat(untranslatedSequence, sc.autoRepeat);
 
-            QString seqStr = translateToCurrentKeyboardLayout(QKeySequence::fromString(sequence, QKeySequence::PortableText));
-            if (seqStr.isEmpty()) {
-                LOGW() << "Failed to translate sequence " << sequence;
+            // Attempt to translate from combination of keys to character, e.g., `Shift+.` becomes `>`, in the case of a QWERTY layout
+            QString translatedSequence
+                = translateToCurrentKeyboardLayout(QKeySequence::fromString(untranslatedSequence, QKeySequence::PortableText));
+            if (translatedSequence.isEmpty()) {
+                LOGW() << "Failed to translate sequence " << untranslatedSequence;
                 continue;
             }
 
-            // RULE: If a sequence is used for several shortcuts but the values for autoRepeat vary depending on
-            // the context, then we should force autoRepeat to false for all shortcuts sharing the sequence in
-            // question. This prevents the creation of ambiguous shortcuts (see QShortcutEvent::isAmbiguous)
-            auto search = m_shortcuts.find(seqStr);
-            if (search == m_shortcuts.end()) {
-                // Sequence not found, add it...
-                m_shortcuts.insert(seqStr, QVariant(sc.autoRepeat));
-                m_macSequenceMap.insert(seqStr, sequence);
-            } else if (search.value().toBool() && !sc.autoRepeat) {
-                // Sequence already exists, but we need to enforce the above rule...
-                search.value() = false;
-            }
+            // If it was successful, record the translated sequence too, and map it to the untranslated sequence
+            recordMapping(translatedSequence, untranslatedSequence, false);
+            recordAutoRepeat(translatedSequence, sc.autoRepeat);
         }
     }
 
+    assert(m_shortcuts.size() == m_macSequenceMap.size());
+
     emit shortcutsChanged();
 }
 
 void MacOSShortcutsInstanceModel::doActivate(const QString& seq)
 {
+    // `seq` will always be in the map, because it comes from one of the QML `Shortcut` objects
+    // and for every such object, we have also created an entry in the map, in `doLoadShortcuts`.
     ShortcutsInstanceModel::doActivate(m_macSequenceMap.value(seq));
 }