Details

Reviewers

Commits

R37:657156ed256b: Fix for qLineEdits everywhere

Summary

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

Repository

R37 Krita

Lint

Lint Skipped

Unit

Unit Tests Skipped

WhaleKit created this revision.Mar 31 2017, 9:06 PM

almost forgot to put name

According to this page: https://bugreports.qt.io/browse/QTBUG-3385 this behavior of QLineEdit

is by design, and not a bug.

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

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....

libs/ui/KisNodeDelegate.cpp
808	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.
libs/ui/KisPart.cpp
204 ↗	(On Diff #13030)	Please move the installation of this filter into main.cc, 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

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

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.

P74 Masterwork From Distant Lands

1	diff --git a/libs/ui/input/KisQtWidgetsTweaker.cpp b/libs/ui/input/KisQtWidgetsTweaker.cpp
2	index a21b3a3..8c25cca 100644
3	--- a/libs/ui/input/KisQtWidgetsTweaker.cpp
4	+++ b/libs/ui/input/KisQtWidgetsTweaker.cpp
5	@@ -56,6 +56,48 @@ KisQtWidgetsTweaker::~KisQtWidgetsTweaker()
6
7	}
8
9	+QKeySequence toKeySequence(const QKeyEvent& event)
10	+{
11	+ return QKeySequence(event.key() \| event.modifiers());
12	+}
13	+
14	+struct StandardSequence
15	+{
16	+ QVector<QLatin1String> widgetTypes;
17	+ QVector<QKeySequence::StandardKey> sequences;
18	+
19	+ bool matches(QKeyEvent *event, QKeySequence::StandardKey key) {
20	+ return event->matches(key);
21	+ }
22	+};
23	+
24	+struct CustomSequence
25	+{
26	+ QVector<QLatin1String> widgetTypes;
27	+ QVector<QKeySequence> sequences;
28	+
29	+ bool matches(QKeyEvent *event, QKeySequence key) {
30	+ return key.matches(toKeySequence(*event)) == QKeySequence::ExactMatch;
31	+ }
32	+};
33	+
34	+template <class SequenceMap>
35	+bool tryAcceptSequence(QObject receiver, QEvent event, const SequenceMap &sequenceMap)
36	+{
37	+ Q_FOREACH (const QLatin1String &type, sequenceMap.widgetTypes) {
38	+ if (receiver->inherits(type.data())) {
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;
49	+}
50	+
51	bool KisQtWidgetsTweaker::eventFilter(QObject receiver, QEvent event)
52	{
53

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

P75 Masterwork From Distant Lands

1	diff --git a/libs/ui/input/KisQtWidgetsTweaker.h b/libs/ui/input/KisQtWidgetsTweaker.h
2	index cd97e3f..d0d5ea6 100644
3	--- a/libs/ui/input/KisQtWidgetsTweaker.h
4	+++ b/libs/ui/input/KisQtWidgetsTweaker.h
5	@@ -18,6 +18,7 @@
6	*/
7
8	#include <QObject>
9	+#include "kritaui_export.h"
10
11	class QEvent;
12
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	{
20	Q_OBJECT
21	public:

libs/ui/input/KisQtWidgetsTweaker.cpp
151	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)) { ... } `if`'s braces are always split from `if` and `{` with a single space. No additional spaces in the middle of the lines, if possible Operators are split from the operands with a single space as well (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): http://wiki.qt.io/Qt_Coding_Style https://techbase.kde.org/Policies/Frameworks_Coding_Style
libs/ui/input/KisQtWidgetsTweaker.h
41	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.

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

This revision now requires changes to proceed.Jul 6 2017, 11:39 AM

more organized. i hope

Closed by commit R37:657156ed256b: Fix for qLineEdits everywhere (authored by WhaleKit, committed by dkazakov). · Explain WhyJul 18 2017, 10:26 AM

This revision was automatically updated to reflect the committed changes.

		Path
M		krita/main.cc (3 lines)
M		libs/ui/CMakeLists.txt (1 line)
M		libs/ui/KisNodeDelegate.cpp (19 lines)
A	M	libs/ui/input/KisQtWidgetsTweaker.h (42 lines)
A	M	libs/ui/input/KisQtWidgetsTweaker.cpp (190 lines)
M		libs/ui/widgets/kis_slider_spin_box.h (1 line)
M		libs/ui/widgets/kis_slider_spin_box.cpp (19 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	13028			Mar 31 2017, 8:38 PM	★	★
Diff 2	13030			Mar 31 2017, 9:11 PM	★	★
Diff 3	13165			Apr 6 2017, 6:58 PM	★	★
Diff 4	13210			Apr 7 2017, 10:05 PM	★	★
Diff 5	16729			Jul 15 2017, 7:01 AM	★	★
Diff 6	16862	1af925b	R37:657156ed256bfd65d56e2c1c5d1beb0a572785ed	Jul 18 2017, 10:26 AM	★	★

Fix for qLineEdits everywhere
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 13210

krita/main.cc

libs/ui/CMakeLists.txt

libs/ui/KisNodeDelegate.cpp

libs/ui/input/KisQtWidgetsTweaker.h

libs/ui/input/KisQtWidgetsTweaker.cpp

libs/ui/widgets/kis_slider_spin_box.h

libs/ui/widgets/kis_slider_spin_box.cpp

Fix for qLineEdits everywhereClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 13210

krita/main.cc

libs/ui/CMakeLists.txt

libs/ui/KisNodeDelegate.cpp

libs/ui/input/KisQtWidgetsTweaker.h

libs/ui/input/KisQtWidgetsTweaker.cpp

libs/ui/widgets/kis_slider_spin_box.h

libs/ui/widgets/kis_slider_spin_box.cpp

Fix for qLineEdits everywhere
ClosedPublic
Actions

Revision Contents
Changeset List