Merge lp:~gerboland/unity-2d/keymonitor-shutdown-fixes into lp:unity-2d

Proposed by Gerry Boland on 2011-10-28
Status: Merged
Approved by: Olivier Tilloy on 2011-11-24
Approved revision: 770
Merged at revision: 792
Proposed branch: lp:~gerboland/unity-2d/keymonitor-shutdown-fixes
Merge into: lp:unity-2d
Diff against target: 108 lines (+22/-11)
2 files modified
libunity-2d-private/src/keymonitor.cpp (+17/-7)
libunity-2d-private/src/keymonitor.h (+5/-4)
To merge this branch: bzr merge lp:~gerboland/unity-2d/keymonitor-shutdown-fixes
Reviewer Review Type Date Requested Status
Olivier Tilloy 2011-11-24 Approve on 2011-11-24
Michał Sawicz 2011-10-28 Pending
Review via email: mp+80722@code.launchpad.net

Description of the change

Rewrite KeyMonitor XEvent handler to use QSocketNotifier instead of separate thread. This singleton will now be deconstructed properly on application quit. Also free X11 objects when they're unnecessary.

To post a comment you must log in.
Alberto Mardegan (mardy) wrote :

I tested this, but I don't see an improvement on session logout time.

Gerry Boland (gerboland) wrote :

No unfortunately it doesn't help with logout-time, I suspect the communication between gnome-session and Launcher at logout is incomplete.

But without this patch QCoreApplication::quit() does not quit the Launcher as the QtConcurrent thread does not halt. So this is a step in right direction, as otherwise the process needs to be killed.

This also keeps code consistency with GEIS gesture handler.

Olivier Tilloy (osomon) wrote :

The code looks good and clean, and it fixes the unit test that was hanging.

I have a tiny remark style-wise, which you may safely ignore if you feel like it: the x11FileDescriptor variable is referred to only once, so you can remove it and invoke directly ConnectionNumber(m_display) when instantiating the QSocketNotifier. Other than that, it all looks perfect to me.

As to bug #859596 I can’t really tell whether that fixes it, but it should considering comment #8.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'libunity-2d-private/src/keymonitor.cpp'
2--- libunity-2d-private/src/keymonitor.cpp 2011-09-23 12:45:33 +0000
3+++ libunity-2d-private/src/keymonitor.cpp 2011-10-28 22:27:24 +0000
4@@ -21,7 +21,7 @@
5 #include "keymonitor.h"
6
7 // Qt
8-#include <QtConcurrentRun>
9+#include <QSocketNotifier>
10 #include <QDebug>
11
12 // X11
13@@ -38,19 +38,17 @@
14
15
16 KeyMonitor::KeyMonitor(QObject* parent)
17-: QObject(parent), m_stop(false)
18+: QObject(parent)
19 {
20 if (this->registerEvents()) {
21 getModifiers();
22- m_future = QtConcurrent::run(this, &KeyMonitor::run);
23 }
24 }
25
26 KeyMonitor::~KeyMonitor()
27 {
28- /* let the running thread know that it should stop */
29- m_stop = true;
30 m_eventList.clear();
31+ XCloseDisplay(m_display);
32 }
33
34 KeyMonitor* KeyMonitor::instance()
35@@ -74,6 +72,8 @@
36 m_modList.append(xmodmap->modifiermap[i]);
37 }
38 }
39+
40+ XFreeModifiermap(xmodmap);
41 }
42
43 bool KeyMonitor::registerEvents()
44@@ -82,6 +82,7 @@
45 Window window;
46
47 XDeviceInfo *devices;
48+ int x11FileDescriptor;
49 int num_devices;
50 int i, j;
51
52@@ -116,8 +117,11 @@
53 }
54 }
55 }
56+ XCloseDevice(m_display, device);
57 }
58
59+ XFreeDeviceList(devices);
60+
61 if (m_eventList.size() == 0) {
62 UQ_WARNING << "No input devices found.";
63 return false;
64@@ -128,15 +132,21 @@
65 return false;
66 }
67
68+ /* Dispatch XEvents when there is activity on the X11 file descriptor */
69+ x11FileDescriptor = ConnectionNumber(m_display);
70+ QSocketNotifier* socketNotifier = new QSocketNotifier(x11FileDescriptor, QSocketNotifier::Read, this);
71+ connect(socketNotifier, SIGNAL(activated(int)), this, SLOT(x11EventDispatch()));
72+
73 return true;
74 }
75
76
77-void KeyMonitor::run()
78+void KeyMonitor::x11EventDispatch()
79 {
80 XEvent event;
81
82- while(!m_stop && !XNextEvent(m_display, &event)) {
83+ while (XPending(m_display) > 0) {
84+ XNextEvent(m_display, &event);
85 if (event.type == key_press_type) {
86 XDeviceKeyEvent *keyEvent = (XDeviceKeyEvent *) &event;
87 if (!m_modList.contains((KeyCode) keyEvent->keycode)) {
88
89=== modified file 'libunity-2d-private/src/keymonitor.h'
90--- libunity-2d-private/src/keymonitor.h 2011-09-19 19:06:54 +0000
91+++ libunity-2d-private/src/keymonitor.h 2011-10-28 22:27:24 +0000
92@@ -51,11 +51,12 @@
93
94 void getModifiers();
95 bool registerEvents();
96- void run();
97-
98+
99+private Q_SLOTS:
100+ void x11EventDispatch();
101+
102+private:
103 Display *m_display;
104- QFuture<void> m_future;
105- bool m_stop;
106 QVector<XEventClass> m_eventList;
107 QVector<KeyCode> m_modList;
108 };

Subscribers

People subscribed via source and target branches