Commit adc8c597 authored by Dmitry Kazakov's avatar Dmitry Kazakov

Fix broken shortcuts when user uses Space key as a modifier

The Problem
-----------

Qt cannot use Space (or any non-modifier) key as a modifier. When
we assign "Space+R" shortcut to "Reset rotation" in Krita settings,
Qt actually assigns it not to "Space+R", but to "Space, R". That is,
the user is expected to press the two keys sequentially, not
simultaneously (though the latter option also works).

It means that after the user presses "Space" key, Qt transits into a
special state of an incomplete match and starts waiting for the following
key. While in this state, Qt (without this patch) eats all the shortcut
events.

Solution
--------

There is no proper solution for the problem, because Qt will still wait
for the second key to be pressed. That is, there is no way to instruct
Qt that Space and R keys should be pressed simultaneously (it is not
possible by design of QKeySequence.

This patch adds a workaround for the problem. It makes sure that all the
shortcut events are delivered to Krita, whatever the state Qt has
internally.

Test Plan
---------

Configuration:

1) Assign W as a primary shortcut for Zoom-In in **Canvas Input Settings**
2) Assign Space,R as a primary shortcut for Zoom-Out in **Keyboard Shortcuts**
3) Assign Space,W as a primary shortcut Rotate Canvas Left in **Keyboard Shortcuts**

Testcase 1:

1) Press Space, Release Space
2) Press W, Release W

Expected: canvas is zoomed in and not rotated, space+drag gesture still works fine

Testcase 2:

1) Press Space, Release Space
2) Press W, Release W
2) Press R, Release R

Expected: no zoom-out action happens (because Space modifier has already been consumed by the former action)

Testcase 3:

1) Press Space, Release Space
2) Press R, Release R

Expected: zoom-out action happens, space+drag gesture still works fine


Testcase 4:

Press Space, W and R randomly, it should still behave "in a sane way" :)

BUG:409613
parent 64eb2488
From c0e035236696cd274f03ff15a7ca09d358aac9f1 Mon Sep 17 00:00:00 2001
From: Dmitry Kazakov <dimula73@gmail.com>
Date: Mon, 29 Jun 2020 23:41:13 +0300
Subject: [PATCH] Don't eat ShortcutOverride events when there is a
partial-match
Some applications (e.g. Krita) may have its own shortcuts system. That
is, it should always get ShortcutOverride events, even when they are
matched to something inside Qt.
If the application (Krita) accepts the event, then all the partially
matched shortcuts should reset.
See this bug for more details:
https://bugs.kde.org/show_bug.cgi?id=409613
---
src/gui/kernel/qshortcutmap_p.h | 4 +++-
src/gui/kernel/qwindowsysteminterface.cpp | 10 ++++++++--
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/gui/kernel/qshortcutmap_p.h b/src/gui/kernel/qshortcutmap_p.h
index 8fc68229..91191288 100644
--- a/src/gui/kernel/qshortcutmap_p.h
+++ b/src/gui/kernel/qshortcutmap_p.h
@@ -91,8 +91,10 @@ public:
void dumpMap() const;
#endif
-private:
void resetState();
+
+private:
+
QKeySequence::SequenceMatch nextState(QKeyEvent *e);
void dispatchEvent(QKeyEvent *e);
diff --git a/src/gui/kernel/qwindowsysteminterface.cpp b/src/gui/kernel/qwindowsysteminterface.cpp
index b3b6167c..9bb43f2a 100644
--- a/src/gui/kernel/qwindowsysteminterface.cpp
+++ b/src/gui/kernel/qwindowsysteminterface.cpp
@@ -441,7 +441,8 @@ bool QWindowSystemInterface::handleShortcutEvent(QWindow *window, ulong timestam
window = QGuiApplication::focusWindow();
QShortcutMap &shortcutMap = QGuiApplicationPrivate::instance()->shortcutMap;
- if (shortcutMap.state() == QKeySequence::NoMatch) {
+ if (shortcutMap.state() != QKeySequence::ExactMatch) {
+
// Check if the shortcut is overridden by some object in the event delivery path (typically the focus object).
// If so, we should not look up the shortcut in the shortcut map, but instead deliver the event as a regular
// key event, so that the target that accepted the shortcut override event can handle it. Note that we only
@@ -451,8 +452,13 @@ bool QWindowSystemInterface::handleShortcutEvent(QWindow *window, ulong timestam
QEvent::ShortcutOverride, keyCode, modifiers, nativeScanCode, nativeVirtualKey, nativeModifiers, text, autorepeat, count);
{
- if (QWindowSystemInterfacePrivate::handleWindowSystemEvent<SynchronousDelivery>(shortcutOverrideEvent))
+ if (QWindowSystemInterfacePrivate::handleWindowSystemEvent<SynchronousDelivery>(shortcutOverrideEvent)) {
+ if (shortcutMap.state() != QKeySequence::NoMatch) {
+ shortcutMap.resetState();
+ }
+
return false;
+ }
}
}
--
2.20.1.windows.1
......@@ -77,6 +77,7 @@ if (WIN32)
COMMAND ${PATCH_COMMAND} -p1 -d qtbase -i ${CMAKE_CURRENT_SOURCE_DIR}/0081-Fix-no-warning-for-overwriting-files-in-non-native-d.patch
COMMAND ${PATCH_COMMAND} -p1 -d qtbase -i ${CMAKE_CURRENT_SOURCE_DIR}/0082-Make-jp-e-g-default-extensions-context-aware.patch
COMMAND ${PATCH_COMMAND} -p1 -d qtbase -i ${CMAKE_CURRENT_SOURCE_DIR}/0100-Fix-artifacts-when-rendering-multisubpath-dashed-QPa.patch
COMMAND ${PATCH_COMMAND} -p1 -d qtbase -i ${CMAKE_CURRENT_SOURCE_DIR}/0101-Don-t-eat-ShortcutOverride-events-when-there-is-a-pa.patch
# COMMAND ${PATCH_COMMAND} -p1 -d qtbase -i ${CMAKE_CURRENT_SOURCE_DIR}/0031-Compute-logical-DPI-on-a-per-screen-basis.patch
# COMMAND ${PATCH_COMMAND} -p1 -d qtbase -i ${CMAKE_CURRENT_SOURCE_DIR}/0032-Update-Dpi-and-scale-factor-computation.patch
# COMMAND ${PATCH_COMMAND} -p1 -d qtbase -i ${CMAKE_CURRENT_SOURCE_DIR}/0033-Move-QT_FONT_DPI-to-cross-platform-code.patch
......@@ -136,6 +137,7 @@ elseif (NOT APPLE)
COMMAND ${PATCH_COMMAND} -p1 -d qtbase -i ${CMAKE_CURRENT_SOURCE_DIR}/0081-Fix-no-warning-for-overwriting-files-in-non-native-d.patch
COMMAND ${PATCH_COMMAND} -p1 -d qtbase -i ${CMAKE_CURRENT_SOURCE_DIR}/0082-Make-jp-e-g-default-extensions-context-aware.patch
COMMAND ${PATCH_COMMAND} -p1 -d qtbase -i ${CMAKE_CURRENT_SOURCE_DIR}/0100-Fix-artifacts-when-rendering-multisubpath-dashed-QPa.patch
COMMAND ${PATCH_COMMAND} -p1 -d qtbase -i ${CMAKE_CURRENT_SOURCE_DIR}/0101-Don-t-eat-ShortcutOverride-events-when-there-is-a-pa.patch
CMAKE_ARGS -DOPENSSL_LIBS='-L${EXTPREFIX_qt}/lib -lssl -lcrypto'
......@@ -244,6 +246,7 @@ else( APPLE )
COMMAND ${PATCH_COMMAND} -p1 -d qtbase -i ${CMAKE_CURRENT_SOURCE_DIR}/0081-Fix-no-warning-for-overwriting-files-in-non-native-d.patch
COMMAND ${PATCH_COMMAND} -p1 -d qtbase -i ${CMAKE_CURRENT_SOURCE_DIR}/0082-Make-jp-e-g-default-extensions-context-aware.patch
COMMAND ${PATCH_COMMAND} -p1 -d qtbase -i ${CMAKE_CURRENT_SOURCE_DIR}/0100-Fix-artifacts-when-rendering-multisubpath-dashed-QPa.patch
COMMAND ${PATCH_COMMAND} -p1 -d qtbase -i ${CMAKE_CURRENT_SOURCE_DIR}/0101-Don-t-eat-ShortcutOverride-events-when-there-is-a-pa.patch
#COMMAND ${PATCH_COMMAND} -p1 -b -d <SOURCE_DIR>/qtbase/mkspecs/features/mac -i ${CMAKE_CURRENT_SOURCE_DIR}/mac-default.patch
)
......@@ -256,6 +259,7 @@ else( APPLE )
COMMAND ${PATCH_COMMAND} -p1 -d qtbase -i ${CMAKE_CURRENT_SOURCE_DIR}/0081-Fix-no-warning-for-overwriting-files-in-non-native-d.patch
COMMAND ${PATCH_COMMAND} -p1 -d qtbase -i ${CMAKE_CURRENT_SOURCE_DIR}/0082-Make-jp-e-g-default-extensions-context-aware.patch
COMMAND ${PATCH_COMMAND} -p1 -d qtbase -i ${CMAKE_CURRENT_SOURCE_DIR}/0100-Fix-artifacts-when-rendering-multisubpath-dashed-QPa.patch
COMMAND ${PATCH_COMMAND} -p1 -d qtbase -i ${CMAKE_CURRENT_SOURCE_DIR}/0101-Don-t-eat-ShortcutOverride-events-when-there-is-a-pa.patch
)
endif()
......
......@@ -367,14 +367,12 @@ bool KisInputManager::eventFilterImpl(QEvent * event)
retval = d->matcher.autoRepeatedKeyPressed(key);
}
/**
* Workaround for temporary switching of tools by
* KoCanvasControllerWidget. We don't need this switch because
* we handle it ourselves.
*/
retval |= !d->forwardAllEventsToTool &&
(keyEvent->key() == Qt::Key_Space ||
keyEvent->key() == Qt::Key_Escape);
// In case we matched ashortcut we should accept the event to
// notify Qt that it shouldn't try to trigger its partially matched
// shortcuts.
if (retval) {
keyEvent->setAccepted(true);
}
break;
}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment