Fixed key sequence eating behavior for QShortcut and QAction.

A disabled QShortcut should eat its key sequence even for complex
sequences like "Ctrl-E 1". For example, a line edit with such a
shortcut should not display 1 after typing "Ctrl-E" and then "1".
The patch achieves this essentially by moving more of the
decision making (of whether to eat the key secuence) from
shortcutmap.cpp to qhortcut.cpp.

Moreover, an invisible QAction should not eat any of its key sequences
(primary nor alternates). In the example above, the line edit
would display 1 when typing this sequence for an invisible action.
The patch achieves this by temporarily unregistering all of the
action's shortcuts while the action is invisible.

Reviewed-by: mariusSO
Task-number: 251246

6 files changed, 86 insertions, 31 deletions

diff --git a/src/gui/kernel/qaction.cpp b/src/gui/kernel/qaction.cpp
index abb17d7..4edc1ca 100644
--- a/src/gui/kernel/qaction.cpp
+++ b/src/gui/kernel/qaction.cpp
@@ -136,25 +136,27 @@ void QActionPrivate::redoGrab(QShortcutMap &map)
 void QActionPrivate::redoGrabAlternate(QShortcutMap &map)
 {
     Q_Q(QAction);
-    foreach (int id, alternateShortcutIds)
-        if (id)
+    for (int i = 0; i < alternateShortcutIds.size(); ++i)
+        if (int id = alternateShortcutIds.at(i))
             map.removeShortcut(id, q);
     alternateShortcutIds.clear();
     if (alternateShortcuts.isEmpty())
         return;
-    foreach (const QKeySequence& alternate, alternateShortcuts) {
+    for (int i = 0; i < alternateShortcuts.size(); ++i) {
+        const QKeySequence &alternate = alternateShortcuts.at(i);
         if (!alternate.isEmpty())
             alternateShortcutIds.append(map.addShortcut(q, alternate, shortcutContext));
         else
             alternateShortcutIds.append(0);
     }
+
     if (!enabled) {
-        foreach (int id, alternateShortcutIds)
-            map.setShortcutEnabled(false, id, q);
+        for (int i = 0; i < alternateShortcutIds.size(); ++i)
+            map.setShortcutEnabled(false, alternateShortcutIds.at(i), q);
     }
     if (!autorepeat) {
-        foreach (int id, alternateShortcutIds)
-            map.setShortcutAutoRepeat(false, id, q);
+        for (int i = 0; i < alternateShortcutIds.size(); ++i)
+            map.setShortcutAutoRepeat(false, alternateShortcutIds.at(i), q);
     }
 }
 
@@ -163,10 +165,26 @@ void QActionPrivate::setShortcutEnabled(bool enable, QShortcutMap &map)
     Q_Q(QAction);
     if (shortcutId)
         map.setShortcutEnabled(enable, shortcutId, q);
-    foreach (int id, alternateShortcutIds)
-        if (id)
+    for (int i = 0; i < alternateShortcutIds.size(); ++i)
+        if (int id = alternateShortcutIds.at(i))
             map.setShortcutEnabled(enable, id, q);
 }
+
+void QActionPrivate::removeAll(QShortcutMap &map)
+{
+    Q_Q(QAction);
+    if (shortcutId) {
+        map.removeShortcut(shortcutId, q);
+        shortcutId = 0;
+    }
+
+    for (int i = 0; i < alternateShortcutIds.size(); ++i)
+        if (int id = alternateShortcutIds.at(i))
+            map.removeShortcut(id, q);
+
+    alternateShortcutIds.clear();
+}
+
 #endif // QT_NO_SHORTCUT
 
 
@@ -615,8 +633,8 @@ QAction::~QAction()
 #ifndef QT_NO_SHORTCUT
     if (d->shortcutId && qApp) {
         qApp->d_func()->shortcutMap.removeShortcut(d->shortcutId, this);
-        foreach (int id, d->alternateShortcutIds)
-            qApp->d_func()->shortcutMap.removeShortcut(id, this);
+        for (int i = 0; i < d->alternateShortcutIds.size(); ++i)
+            qApp->d_func()->shortcutMap.removeShortcut(d->alternateShortcutIds.at(i), this);
     }
 #endif
 }
@@ -1049,7 +1067,12 @@ void QAction::setVisible(bool b)
     d->visible = b;
     d->enabled = b && !d->forceDisabled && (!d->group || d->group->isEnabled()) ;
 #ifndef QT_NO_SHORTCUT
-    d->setShortcutEnabled(d->enabled, qApp->d_func()->shortcutMap);
+    if (b) {
+        d->redoGrab(qApp->d_func()->shortcutMap);
+        d->redoGrabAlternate(qApp->d_func()->shortcutMap);
+    } else {
+        d->removeAll(qApp->d_func()->shortcutMap);
+    }
 #endif
     d->sendDataChanged();
 }
diff --git a/src/gui/kernel/qaction_p.h b/src/gui/kernel/qaction_p.h
index 0617ef5..a5b3731 100644
--- a/src/gui/kernel/qaction_p.h
+++ b/src/gui/kernel/qaction_p.h
@@ -111,6 +111,7 @@ public:
     void redoGrab(QShortcutMap &map);
     void redoGrabAlternate(QShortcutMap &map);
     void setShortcutEnabled(bool enable, QShortcutMap &map);
+    void removeAll(QShortcutMap &map);
 
     static QShortcutMap *globalMap;
 #endif // QT_NO_SHORTCUT
diff --git a/src/gui/kernel/qshortcut.cpp b/src/gui/kernel/qshortcut.cpp
index 50b6e59..f3c93c6 100644
--- a/src/gui/kernel/qshortcut.cpp
+++ b/src/gui/kernel/qshortcut.cpp
@@ -385,19 +385,21 @@ bool QShortcut::event(QEvent *e)
 {
     Q_D(QShortcut);
     bool handled = false;
-    if (d->sc_enabled && e->type() == QEvent::Shortcut) {
+    if (e->type() == QEvent::Shortcut) {
         QShortcutEvent *se = static_cast<QShortcutEvent *>(e);
         if (se->shortcutId() == d->sc_id && se->key() == d->sc_sequence){
+            if (d->sc_enabled) {
 #ifndef QT_NO_WHATSTHIS
-            if (QWhatsThis::inWhatsThisMode()) {
-                QWhatsThis::showText(QCursor::pos(), d->sc_whatsthis);
-                handled = true;
-            } else
+                if (QWhatsThis::inWhatsThisMode()) {
+                    QWhatsThis::showText(QCursor::pos(), d->sc_whatsthis);
+                    handled = true;
+                } else
 #endif
-            if (se->isAmbiguous())
-                emit activatedAmbiguously();
-            else
-                emit activated();
+                if (se->isAmbiguous())
+                    emit activatedAmbiguously();
+                else
+                    emit activated();
+            }
             handled = true;
         }
     }
diff --git a/src/gui/kernel/qshortcutmap.cpp b/src/gui/kernel/qshortcutmap.cpp
index ed9654b..9766a69 100644
--- a/src/gui/kernel/qshortcutmap.cpp
+++ b/src/gui/kernel/qshortcutmap.cpp
@@ -370,9 +370,8 @@ bool QShortcutMap::tryShortcutEvent(QObject *o, QKeyEvent *e)
     default:
 	break;
     }
-    // If nextState is QKeySequence::ExactMatch && identicals.count == 0
-    // we've only found disabled shortcuts
-    return identicalMatches > 0 || result == QKeySequence::PartialMatch;
+
+    return true;
 }
 
 /*! \internal
@@ -494,9 +493,7 @@ QKeySequence::SequenceMatch QShortcutMap::find(QKeyEvent *e)
                     // We don't need partials, if we have identicals
                     if (d->identicals.size())
                         break;
-                    // We only care about enabled partials, so we don't consume
-                    // key events when all partials are disabled!
-                    partialFound |= (*it).enabled;
+                    partialFound = true;
                 }
             }
             ++it;
diff --git a/tests/auto/qaction/tst_qaction.cpp b/tests/auto/qaction/tst_qaction.cpp
index 34f2dfd..8e4ae86 100644
--- a/tests/auto/qaction/tst_qaction.cpp
+++ b/tests/auto/qaction/tst_qaction.cpp
@@ -46,6 +46,7 @@
 #include <qevent.h>
 #include <qaction.h>
 #include <qmenu.h>
+#include <qlineedit.h>
 
 //TESTED_CLASS=
 //TESTED_FILES=
@@ -74,6 +75,7 @@ private slots:
     void setStandardKeys();
     void alternateShortcuts();
     void enabledVisibleInteraction();
+    void invisibleActionWithComplexShortcut();
     void task200823_tooltip();
 
 private:
@@ -322,5 +324,36 @@ void tst_QAction::task200823_tooltip()
     QCOMPARE(action->toolTip(), ref);
 }
 
+void tst_QAction::invisibleActionWithComplexShortcut()
+{
+    QAction action(0);
+    action.setShortcut(QKeySequence(Qt::CTRL + Qt::Key_E, Qt::Key_1));
+
+    QLineEdit edit;
+    edit.addAction(&action);
+    edit.show();
+    QTest::qWait(100);
+
+    QSignalSpy spy(&action, SIGNAL(triggered()));
+
+    action.setVisible(true);
+    QTest::keyPress(&edit, Qt::Key_E, Qt::ControlModifier);
+    QTest::keyRelease(&edit, Qt::Key_E, Qt::ControlModifier);
+    QTest::keyPress(&edit, Qt::Key_1, Qt::NoModifier);
+    QTest::keyRelease(&edit, Qt::Key_1, Qt::NoModifier);
+    QCOMPARE(spy.count(), 1);
+    QCOMPARE(edit.text(), QLatin1String(""));
+
+    edit.clear();
+    spy.clear();
+    action.setVisible(false);
+    QTest::keyPress(&edit, Qt::Key_E, Qt::ControlModifier);
+    QTest::keyRelease(&edit, Qt::Key_E, Qt::ControlModifier);
+    QTest::keyPress(&edit, Qt::Key_1, Qt::NoModifier);
+    QTest::keyRelease(&edit, Qt::Key_1, Qt::NoModifier);
+    QCOMPARE(spy.count(), 0);
+    QCOMPARE(edit.text(), QLatin1String("1"));
+}
+
 QTEST_MAIN(tst_QAction)
 #include "tst_qaction.moc"
diff --git a/tests/auto/qshortcut/tst_qshortcut.cpp b/tests/auto/qshortcut/tst_qshortcut.cpp
index 69ebf74..cd80204 100644
--- a/tests/auto/qshortcut/tst_qshortcut.cpp
+++ b/tests/auto/qshortcut/tst_qshortcut.cpp
@@ -987,17 +987,16 @@ void tst_QShortcut::keypressConsumption()
     cut1->setEnabled(false);
     cut2->setEnabled(false);
 
-    // Make sure keypresses is passed on, since all multiple keysequences
-    // with Ctrl+I are disabled
+    edit->clear();
     sendKeyEvents(edit, Qt::CTRL + Qt::Key_I, 0);   // Send key to edit
     QCOMPARE( currentResult, NoResult );
     QCOMPARE( ambigResult, NoResult );
-    QVERIFY(edit->toPlainText().endsWith("<Ctrl+I>"));
+    QVERIFY(edit->toPlainText().isEmpty());
 
     sendKeyEvents(edit, Qt::Key_A, 'a');         // Send key to edit
     QCOMPARE( currentResult, NoResult );
     QCOMPARE( ambigResult, NoResult );
-    QVERIFY(edit->toPlainText().endsWith("<Ctrl+I>a"));
+    QVERIFY(edit->toPlainText().isEmpty());
 
     clearAllShortcuts();
 }