Fix for qLineEdits everywhere

Authored by WhaleKit on Mar 31 2017, 9:06 PM.



QLineEdit gives away to Qt's shortcut system some key events (basically, everything with modiiers) even if it could handle them and use for text editing, when there is registered shortcut, matching this key input.
Example: pressing "ctrl+backspace" (used on my machine to delete word left from cursor) in layer name editing field will cause filing canvas with background color, instead of deleting word left from cursor.
This patch fixes that without replacing QLineEdits all over the application.

Also, now pressing tab and backTab when editing layer's name will switch you to editing next/previous layer's name, even if there is shorcut assigned to tab and backtab.

Diff Detail

R37 Krita
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
WhaleKit created this revision.Mar 31 2017, 9:06 PM
WhaleKit updated this revision to Diff 13030.Mar 31 2017, 9:12 PM

almost forgot to put name

According to this page: this behavior of QLineEdit

is by design, and not a bug.

"KisQtWidgetsTweaker"'s class name is up to discussion.

dkazakov requested changes to this revision.Apr 4 2017, 10:55 AM

Hi, @WhaleKit!

Your patch seems to fix the issue. In general I have only two minor issues blocking it from being pushed into the repository. Please fix them, I'll push the patch. After that we can continue with fixes I noted below:

I think we should extend the patch and make it more universal, like accept the events for all the possible widget types we use. We could have a map to store key sequences and check them iteratively:

QHash<QString, QSet<QKeySequence>> typeToSeq;

Q_FOREACH (const QString &type, typeToSeq.keys()) {
    if (widget->inherits(type)) {
        Q_FOREACH (const QKeySequence &seq; typeToSeq[type]) {
            if (seq->matches(key) {

Here is rough list of widgets we use in our interface:

  • QAbstractButton
  • QAbstractSpinBox
  • KisAbstractSliderSpinBox
  • QAbstractSlider
  • QComboBox

Accepting events for these widgets would fix a veeeeery old bug, when people could not use Tab button for tab-focus switch....


Please use full type name in this case. We use auto only for iterators and lambdas. For the rest, we prefer to use clearly readable types.

204 ↗(On Diff #13030)

Please move the installation of this filter into, where we install other "fix-qt" filters, like KisXi2EventFilter. Then you will not need to check for isInstalled() state.

This revision now requires changes to proceed.Apr 4 2017, 10:55 AM
WhaleKit updated this revision to Diff 13165.Apr 6 2017, 7:21 PM
WhaleKit edited edge metadata.

Now KisQtWidgetsTweaker affects QComboBox, Both QSpinBoxes
KisQtWidgetsTweaker now installed on app in
QAbstratcSliderSpinBox now accept QShortcutOverride event by itself (no need to do that using KisQtWidgetsTweaker).

WhaleKit updated this revision to Diff 13210.EditedApr 7 2017, 10:32 PM

Now, tab key presses interpreted as shortcut only when canvas in focus - otherwise it will be handled by widgets to change focus.
Since i coulndn't find documentation about order, in which widgets' event handlers are invoked nor guarantee that qt won't start handling next event before all handlers for previous is called (though it makes sence), i can't really claim it will work perfectly in further versions of qt.

Hi, @WhaleKit!

I have tested your patch and it works almost perfectly. There is still one problem (Shift+Tab) doesn't work in dockers, but it also doesn't work without your patch, so it shouldn't block the patch. So now we should just clean the coding style an duplications and it will be ready for landing! :)

We will obviously have to extend the class later to handle more widget and event types, so we need to create some kind of framework for it. Here is an example patch how that could be handled. Using a general function for this check will not only remove the duplications, but will also self-document the code a bit.

1diff --git a/libs/ui/input/KisQtWidgetsTweaker.cpp b/libs/ui/input/KisQtWidgetsTweaker.cpp
2index a21b3a3..8c25cca 100644
3--- a/libs/ui/input/KisQtWidgetsTweaker.cpp
4+++ b/libs/ui/input/KisQtWidgetsTweaker.cpp
5@@ -56,6 +56,48 @@ KisQtWidgetsTweaker::~KisQtWidgetsTweaker()
7 }
9+QKeySequence toKeySequence(const QKeyEvent& event)
11+ return QKeySequence(event.key() | event.modifiers());
14+struct StandardSequence
16+ QVector<QLatin1String> widgetTypes;
17+ QVector<QKeySequence::StandardKey> sequences;
19+ bool matches(QKeyEvent *event, QKeySequence::StandardKey key) {
20+ return event->matches(key);
21+ }
24+struct CustomSequence
26+ QVector<QLatin1String> widgetTypes;
27+ QVector<QKeySequence> sequences;
29+ bool matches(QKeyEvent *event, QKeySequence key) {
30+ return key.matches(toKeySequence(*event)) == QKeySequence::ExactMatch;
31+ }
34+template <class SequenceMap>
35+bool tryAcceptSequence(QObject *receiver, QEvent *event, const SequenceMap &sequenceMap)
37+ Q_FOREACH (const QLatin1String &type, sequenceMap.widgetTypes) {
38+ if (receiver->inherits( {
39+ QKeyEvent* key = static_cast<QKeyEvent*>(event);
40+ Q_FOREACH (QKeySequence seq, sequenceMap.sequences) {
41+ if (sequenceMap.matches(key, seq)) {
42+ event->accept();
43+ return true;
44+ }
45+ }
46+ }
47+ }
48+ return false;
51 bool KisQtWidgetsTweaker::eventFilter(QObject *receiver, QEvent *event)
52 {

And there is a linking problem on Linux, please add this macro to make this class exportable on Linux :)

1diff --git a/libs/ui/input/KisQtWidgetsTweaker.h b/libs/ui/input/KisQtWidgetsTweaker.h
2index cd97e3f..d0d5ea6 100644
3--- a/libs/ui/input/KisQtWidgetsTweaker.h
4+++ b/libs/ui/input/KisQtWidgetsTweaker.h
5@@ -18,6 +18,7 @@
6 */
8 #include <QObject>
9+#include "kritaui_export.h"
11 class QEvent;
13@@ -27,7 +28,7 @@ class QEvent;
14 * by filtering events adressed to them
15 * It expected to be installed on the application
16 */
17-class KisQtWidgetsTweaker : public QObject
18+class KRITAUI_EXPORT KisQtWidgetsTweaker : public QObject
19 {
21 public:


Coding style. In Krita we would format the structures in the following way:

if ((key->modifiers() == Qt::NoModifier &&
     (key->key() == Qt::Key_Tab ||
      key->key() == Qt::Key_Backtab)) ||
    (key->modifiers() == Qt::ShiftModifier &&
     key->key() == Qt::Key_Backtab)) {

  1. if's braces are always split from if and { with a single space.
  2. No additional spaces in the middle of the lines, if possible
  3. Operators are split from the operands with a single space as well
  4. (different from Qt's rule) Put the operators of composite logical expressions before the new line and align the next line by the corresponding brace if possible.

Basically our coding style is based on these documents (with small exceptions, like in point 4):


In d-pointer pattern, the d-pointer should be named either d or m_d. And the pointer from the inside of a d-pointer to the parent class should be called q. That conforms with what the Qt's d-pointer framework defines.

dkazakov requested changes to this revision.Jul 6 2017, 11:39 AM

The patch needs changes, so I'll mark it as such

This revision now requires changes to proceed.Jul 6 2017, 11:39 AM
WhaleKit updated this revision to Diff 16729.Jul 15 2017, 7:02 AM
WhaleKit edited edge metadata.

more organized. i hope

This revision was automatically updated to reflect the committed changes.