From 5a2c0b364a3a5830dd6d5069b5f8d3921aeac73a Mon Sep 17 00:00:00 2001 From: Andreas Buhr Date: Wed, 13 Jan 2021 15:29:32 +0100 Subject: Change QQuickShortcut::setSequences to bind to all sequences When binding a Shortcut to a standard key sequence like QKeySequence::FullScreen, it binds only to one key sequence, even though there might be multiple key sequences associated. This patch changes the code to emit a warning in this case and allows to bind to multiple key sequences using 'sequences: [ ]'. Fixes: QTBUG-88682 Change-Id: I88998aa8858d8f2c0c86e46bae94afd7ceb15b66 Reviewed-by: Fabian Kosmale (cherry picked from commit 6511b17038627ac30cb6622b13c7d46d9877bac5) Reviewed-by: Qt Cherry-pick Bot --- src/quick/util/qquickshortcut.cpp | 83 ++++++++++---- .../quick/qquickshortcut/tst_qquickshortcut.cpp | 123 ++++++++++++++++++--- 2 files changed, 166 insertions(+), 40 deletions(-) diff --git a/src/quick/util/qquickshortcut.cpp b/src/quick/util/qquickshortcut.cpp index 2905e36d00..fdd846adb6 100644 --- a/src/quick/util/qquickshortcut.cpp +++ b/src/quick/util/qquickshortcut.cpp @@ -44,6 +44,7 @@ #include #include #include +#include /*! \qmltype Shortcut @@ -124,13 +125,35 @@ Q_QUICK_PRIVATE_EXPORT void qt_quick_set_shortcut_context_matcher(ContextMatcher QT_BEGIN_NAMESPACE -static QKeySequence valueToKeySequence(const QVariant &value) +static QKeySequence valueToKeySequence(const QVariant &value, const QQuickShortcut *const shortcut) { - if (value.userType() == QMetaType::Int) - return QKeySequence(static_cast(value.toInt())); + if (value.userType() == QMetaType::Int) { + const QVector s = + QKeySequence::keyBindings(static_cast(value.toInt())); + if (s.size() > 1) { + const QString templateString = QString::fromUtf16( + u"Shortcut: Only binding to one of multiple key bindings associated with %1. " + u"Use 'sequences: [ ]' to bind to all of them."); + qmlWarning(shortcut) + << templateString.arg(static_cast(value.toInt())); + } + return s.size() > 0 ? s[0] : QKeySequence {}; + } + return QKeySequence::fromString(value.toString()); } +static QList valueToKeySequences(const QVariant &value) +{ + if (value.userType() == QMetaType::Int) { + return QKeySequence::keyBindings(static_cast(value.toInt())); + } else { + QList result; + result.push_back(QKeySequence::fromString(value.toString())); + return result; + } +} + QQuickShortcut::QQuickShortcut(QObject *parent) : QObject(parent), m_enabled(true), m_completed(false), m_autorepeat(true), m_context(Qt::WindowShortcut) { @@ -172,7 +195,7 @@ void QQuickShortcut::setSequence(const QVariant &value) if (value == m_shortcut.userValue) return; - QKeySequence keySequence = valueToKeySequence(value); + QKeySequence keySequence = valueToKeySequence(value, this); ungrabShortcut(m_shortcut); m_shortcut.userValue = value; @@ -207,28 +230,42 @@ QVariantList QQuickShortcut::sequences() const void QQuickShortcut::setSequences(const QVariantList &values) { - QVector remainder = m_shortcuts.mid(values.count()); - m_shortcuts.resize(values.count()); - - bool changed = !remainder.isEmpty(); - for (int i = 0; i < values.count(); ++i) { - const QVariant &value = values.at(i); - Shortcut& shortcut = m_shortcuts[i]; - if (value == shortcut.userValue) - continue; - - QKeySequence keySequence = valueToKeySequence(value); - - ungrabShortcut(shortcut); - shortcut.userValue = value; - shortcut.keySequence = keySequence; - grabShortcut(shortcut, m_context); + // convert QVariantList to QVector + QVector requestedShortcuts; + for (const QVariant &v : values) { + const QVector list = valueToKeySequences(v); + for (const QKeySequence &s : list) { + Shortcut sc; + sc.userValue = v; + sc.keySequence = s; + requestedShortcuts.push_back(sc); + } + } - changed = true; + // if nothing has changed, just return: + if (m_shortcuts.size() == requestedShortcuts.size()) { + bool changed = false; + for (int i = 0; i < requestedShortcuts.count(); ++i) { + const Shortcut &requestedShortcut = requestedShortcuts[i]; + const Shortcut &shortcut = m_shortcuts[i]; + if (!(requestedShortcut.userValue == shortcut.userValue + && requestedShortcut.keySequence == shortcut.keySequence)) { + changed = true; + break; + } + } + if (!changed) { + return; + } } - if (changed) - emit sequencesChanged(); + for (Shortcut &s : m_shortcuts) + ungrabShortcut(s); + m_shortcuts = requestedShortcuts; + for (Shortcut &s : m_shortcuts) + grabShortcut(s, m_context); + + emit sequencesChanged(); } /*! diff --git a/tests/auto/quick/qquickshortcut/tst_qquickshortcut.cpp b/tests/auto/quick/qquickshortcut/tst_qquickshortcut.cpp index 7b7113e2ce..6351b97fb3 100644 --- a/tests/auto/quick/qquickshortcut/tst_qquickshortcut.cpp +++ b/tests/auto/quick/qquickshortcut/tst_qquickshortcut.cpp @@ -42,6 +42,8 @@ class tst_QQuickShortcut : public QQmlDataTest Q_OBJECT private slots: + void standardShortcuts_data(); + void standardShortcuts(); void shortcuts_data(); void shortcuts(); void sequence_data(); @@ -81,6 +83,49 @@ static QVariant shortcutMap(const QVariant &key, bool enabled = true, bool autoR return shortcutMap(key, Qt::WindowShortcut, enabled, autoRepeat); } +void tst_QQuickShortcut::standardShortcuts_data() +{ + QTest::addColumn("standardKey"); + QTest::newRow("Close") << QKeySequence::Close; + QTest::newRow("Cut") << QKeySequence::Cut; + QTest::newRow("NextChild") << QKeySequence::NextChild; + QTest::newRow("PreviousChild") << QKeySequence::PreviousChild; + QTest::newRow("FindNext") << QKeySequence::FindNext; + QTest::newRow("FindPrevious") << QKeySequence::FindPrevious; + QTest::newRow("FullScreen") << QKeySequence::FullScreen; +} + +void tst_QQuickShortcut::standardShortcuts() +{ + QFETCH(QKeySequence::StandardKey, standardKey); + + QQmlApplicationEngine engine; + + engine.load(testFileUrl("multiple.qml")); + QQuickWindow *window = qobject_cast(engine.rootObjects().value(0)); + QVERIFY(window); + window->show(); + QVERIFY(QTest::qWaitForWindowExposed(window)); + window->requestActivate(); + QVERIFY(QTest::qWaitForWindowActive(window)); + + QObject *shortcut = window->property("shortcut").value(); + QVERIFY(shortcut); + + // create list of shortcuts + QVariantList shortcuts; + shortcuts.push_back(standardKey); + shortcut->setProperty("sequences", shortcuts); + + // test all: + QList allsequences = QKeySequence::keyBindings(standardKey); + for (const QKeySequence &s : allsequences) { + window->setProperty("activated", false); + QTest::keySequence(window, s); + QCOMPARE(window->property("activated").toBool(), true); + } +} + void tst_QQuickShortcut::shortcuts_data() { QTest::addColumn("shortcuts"); @@ -188,6 +233,7 @@ void tst_QQuickShortcut::shortcuts() window->setProperty("shortcuts", shortcuts); QTest::keyPress(window, key, modifiers); + QTest::keyRelease(window, key, modifiers); QCOMPARE(window->property("activatedShortcut").toString(), activatedShortcut); QCOMPARE(window->property("ambiguousShortcut").toString(), ambiguousShortcut); } @@ -263,14 +309,22 @@ void tst_QQuickShortcut::sequence() window->setProperty("shortcuts", shortcuts); - if (key1 != 0) + if (key1 != 0) { QTest::keyPress(window, key1, modifiers1); - if (key2 != 0) + QTest::keyRelease(window, key1, modifiers1); + } + if (key2 != 0) { QTest::keyPress(window, key2, modifiers2); - if (key3 != 0) + QTest::keyRelease(window, key2, modifiers2); + } + if (key3 != 0) { QTest::keyPress(window, key3, modifiers3); - if (key4 != 0) + QTest::keyRelease(window, key3, modifiers3); + } + if (key4 != 0) { QTest::keyPress(window, key4, modifiers4); + QTest::keyRelease(window, key4, modifiers4); + } QCOMPARE(window->property("activatedShortcut").toString(), activatedShortcut); QCOMPARE(window->property("ambiguousShortcut").toString(), ambiguousShortcut); @@ -352,6 +406,7 @@ void tst_QQuickShortcut::context() activeWindow->setProperty("shortcuts", activeWindowShortcuts); QTest::keyPress(activeWindow, key); + QTest::keyRelease(activeWindow, key); QCOMPARE(activeWindow->property("activatedShortcut").toString(), activeWindowActivatedShortcut); QCOMPARE(inactiveWindow->property("activatedShortcut").toString(), inactiveWindowActivatedShortcut); @@ -411,6 +466,8 @@ void tst_QQuickShortcut::matcher() QVERIFY(window); window->show(); QVERIFY(QTest::qWaitForWindowExposed(window)); + window->requestActivate(); + QVERIFY(QTest::qWaitForWindowActive(window)); window->setProperty("shortcuts", QVariantList() << shortcut); QTest::keyClick(window, key); @@ -452,6 +509,8 @@ void tst_QQuickShortcut::multiple() QVERIFY(window); window->show(); QVERIFY(QTest::qWaitForWindowExposed(window)); + window->requestActivate(); + QVERIFY(QTest::qWaitForWindowActive(window)); QObject *shortcut = window->property("shortcut").value(); QVERIFY(shortcut); @@ -460,8 +519,33 @@ void tst_QQuickShortcut::multiple() shortcut->setProperty("sequences", sequences); QTest::keyPress(window, key, modifiers); + QTest::keyRelease(window, key, modifiers); QCOMPARE(window->property("activated").toBool(), activated); + + // check it still works after rotating the sequences + QStringList rotatedSequences; + for (int i = 1; i < sequences.size(); ++i) + rotatedSequences.push_back(sequences[i]); + if (sequences.size()) + rotatedSequences.push_back(sequences[0]); + + window->setProperty("activated", false); + shortcut->setProperty("sequences", rotatedSequences); + + QTest::keyPress(window, key, modifiers); + QTest::keyRelease(window, key, modifiers); + QCOMPARE(window->property("activated").toBool(), activated); + + // check setting to no shortcuts + QStringList emptySequence; + + window->setProperty("activated", false); + shortcut->setProperty("sequences", emptySequence); + + QTest::keyPress(window, key, modifiers); + QTest::keyRelease(window, key, modifiers); + QCOMPARE(window->property("activated").toBool(), false); } void tst_QQuickShortcut::contextChange_data() @@ -479,12 +563,12 @@ void tst_QQuickShortcut::contextChange() QQmlApplicationEngine engine; engine.load(testFileUrl("multiple.qml")); - QQuickWindow *inactivewindow = qobject_cast(engine.rootObjects().value(0)); - QVERIFY(inactivewindow); - inactivewindow->show(); - QVERIFY(QTest::qWaitForWindowExposed(inactivewindow)); + QQuickWindow *inactiveWindow = qobject_cast(engine.rootObjects().value(0)); + QVERIFY(inactiveWindow); + inactiveWindow->show(); + QVERIFY(QTest::qWaitForWindowExposed(inactiveWindow)); - QObject *shortcut = inactivewindow->property("shortcut").value(); + QObject *shortcut = inactiveWindow->property("shortcut").value(); QVERIFY(shortcut); shortcut->setProperty("enabled", enabled); @@ -492,18 +576,22 @@ void tst_QQuickShortcut::contextChange() shortcut->setProperty("context", Qt::WindowShortcut); engine.load(testFileUrl("multiple.qml")); - QQuickWindow *activewindow = qobject_cast(engine.rootObjects().value(1)); - QVERIFY(activewindow); - activewindow->show(); - QVERIFY(QTest::qWaitForWindowExposed(activewindow)); + QQuickWindow *activeWindow = qobject_cast(engine.rootObjects().value(1)); + QVERIFY(activeWindow); + activeWindow->show(); + QVERIFY(QTest::qWaitForWindowExposed(activeWindow)); + activeWindow->requestActivate(); + QVERIFY(QTest::qWaitForWindowActive(activeWindow)); - QTest::keyPress(activewindow, key, modifiers); - QCOMPARE(inactivewindow->property("activated").toBool(), false); + QTest::keyPress(activeWindow, key, modifiers); + QTest::keyRelease(activeWindow, key, modifiers); + QCOMPARE(inactiveWindow->property("activated").toBool(), false); shortcut->setProperty("context", Qt::ApplicationShortcut); - QTest::keyPress(activewindow, key, modifiers); - QCOMPARE(inactivewindow->property("activated").toBool(), activated); + QTest::keyPress(activeWindow, key, modifiers); + QTest::keyRelease(activeWindow, key, modifiers); + QCOMPARE(inactiveWindow->property("activated").toBool(), activated); } #ifdef QT_QUICKWIDGETS_LIB @@ -619,6 +707,7 @@ void tst_QQuickShortcut::quickWidgetShortcuts() QQuickItem* item = quickWidget->rootObject(); item->setProperty("shortcuts", shortcuts); QTest::keyPress(quickWidget->quickWindow(), key, modifiers, 1500); + QTest::keyRelease(quickWidget->quickWindow(), key, modifiers, 1600); QCOMPARE(item->property("activatedShortcut").toString(), activatedShortcut); QCOMPARE(item->property("ambiguousShortcut").toString(), ambiguousShortcut); } -- cgit v1.2.3