Differential D16434 Diff 44247 src/runtime/plugins/xcb/kglobalaccel_x11.cpp

Changeset View

Standalone View

View Options

src/runtime/plugins/xcb/kglobalaccel_x11.cpp

Show All 27 Lines
28 28​
29#include <QApplication> 29​#include <QApplication>
30#include <QWidget> 30​#include <QWidget>
31#include <QX11Info> 31​#include <QX11Info>
32 32​
33#include <X11/keysym.h> 33​#include <X11/keysym.h>
34 34​
35// xcb 35​// xcb
36​
37​// It uses "excplicit" as a variable name, which is not allowed in C++
38​#define explicit xcb_explicit
36#include <xcb/xcb.h> 39​#include <xcb/xcb.h>
37#include <xcb/xcb_keysyms.h> 40​#include <xcb/xcb_keysyms.h>
38 41​#include <xcb/xkb.h>
42​#undef explicit
39 43​
40// g_keyModMaskXAccel 44​// g_keyModMaskXAccel
41// mask of modifiers which can be used in shortcuts 45​// mask of modifiers which can be used in shortcuts
42// (meta, alt, ctrl, shift) 46​// (meta, alt, ctrl, shift)
43// g_keyModMaskXOnOrOff 47​// g_keyModMaskXOnOrOff
44// mask of modifiers where we don't care whether they are on or off 48​// mask of modifiers where we don't care whether they are on or off
45// (caps lock, num lock, scroll lock) 49​// (caps lock, num lock, scroll lock)
46static uint g_keyModMaskXAccel = 0; 50​static uint g_keyModMaskXAccel = 0;
Show All 11 Lines54{
58 // << "g_keyModMaskXOnOrOff = " << g_keyModMaskXOnOrOff << endl; 62​ // << "g_keyModMaskXOnOrOff = " << g_keyModMaskXOnOrOff << endl;
59} 63​}
60 64​
61//---------------------------------------------------- 65​//----------------------------------------------------
62 66​
63KGlobalAccelImpl::KGlobalAccelImpl(QObject *parent) 67​KGlobalAccelImpl::KGlobalAccelImpl(QObject *parent)
64 : KGlobalAccelInterface(parent) 68​ : KGlobalAccelInterface(parent)
65 , m_keySymbols(nullptr) 69​ , m_keySymbols(nullptr)
70​ , m_xkb_first_event(0)
66{ 71​{
72​ const xcb_query_extension_reply_t *reply = xcb_get_extension_data(QX11Info::connection(), &xcb_xkb_id);
73​ if (reply && reply->present)
74​ m_xkb_first_event = reply->first_event;
75​
romanggUnsubmitted
Not Done

https://techbase.kde.org/Policies/Frameworks_Coding_Style#Braces

romangg: https://techbase.kde.org/Policies/Frameworks_Coding_Style#Braces
67 calculateGrabMasks(); 76​ calculateGrabMasks();
68 if (QX11Info::isPlatformX11()) { ​
69 m_keySymbols = xcb_key_symbols_alloc(QX11Info::connection()); ​
70 } ​
71} 77​}
72 78​
73KGlobalAccelImpl::~KGlobalAccelImpl() 79​KGlobalAccelImpl::~KGlobalAccelImpl()
74{ 80​{
75 if (m_keySymbols) { 81​ if (m_keySymbols) {
76 xcb_key_symbols_free(m_keySymbols); 82​ xcb_key_symbols_free(m_keySymbols);
77 } 83​ }
78} 84​}
79 85​
80bool KGlobalAccelImpl::grabKey( int keyQt, bool grab ) 86​bool KGlobalAccelImpl::grabKey( int keyQt, bool grab )
81{ 87​{
82 //grabKey is called during shutdown 88​ //grabKey is called during shutdown
83 //shutdown might be due to the X server being killed 89​ //shutdown might be due to the X server being killed
84 //if so, fail immediately before trying to make other xcb calls 90​ //if so, fail immediately before trying to make other xcb calls
85 if (!QX11Info::connection() || xcb_connection_has_error(QX11Info::connection())) { 91​ if (!QX11Info::connection() || xcb_connection_has_error(QX11Info::connection())) {
86 return false; 92​ return false;
87 } 93​ }
88 94​
89 if (!m_keySymbols) { 95​ if (!m_keySymbols) {
96​ m_keySymbols = xcb_key_symbols_alloc(QX11Info::connection());
romanggUnsubmitted
Done

Before there was a check to QX11Info::isPlatformX11(). Probably we don't need the check, but did you test on Wayland?

romangg: Before there was a check to `QX11Info::isPlatformX11()`. Probably we don't need the check, but…
fvogtAuthorUnsubmitted
Not Done

The plugin is only loaded if the Qt platform is xcb, so the connection can never be null in that case.
I'll add a Q_ASSERT though.

fvogt: The plugin is only loaded if the Qt platform is xcb, so the connection can never be null in…
97​ if (!m_keySymbols) {
90 return false; 98​ return false;
91 } 99​ }
100​ }
101​
92 if( !keyQt ) { 102​ if( !keyQt ) {
93 qCDebug(KGLOBALACCELD) << "Tried to grab key with null code."; 103​ qCDebug(KGLOBALACCELD) << "Tried to grab key with null code.";
94 return false; 104​ return false;
95 } 105​ }
96 106​
97 uint keyModX; 107​ uint keyModX;
98 xcb_keysym_t keySymX; 108​ xcb_keysym_t keySymX;
99 109​
▲ Show 20 Lines • Show All 80 Lines • ▼ Show 20 Line(s)189 } else {
180 success = true; 190​ success = true;
181 } 191​ }
182 } 192​ }
183 } 193​ }
184 free(keyCodes); 194​ free(keyCodes);
185 return success; 195​ return success;
186} 196​}
187 197​
188bool KGlobalAccelImpl::nativeEventFilter(const QByteArray &eventType, void *message, long *) 198​bool KGlobalAccelImpl::nativeEventFilter(const QByteArray &eventType, void *message, long *)
189{ 199​{
190 if (eventType != "xcb_generic_event_t") { 200​ if (eventType != "xcb_generic_event_t") {
191 return false; 201​ return false;
192 } 202​ }
193 xcb_generic_event_t *event = reinterpret_cast<xcb_generic_event_t*>(message); 203​ xcb_generic_event_t *event = reinterpret_cast<xcb_generic_event_t*>(message);
194 const uint8_t responseType = event->response_type & ~0x80; 204​ const uint8_t responseType = event->response_type & ~0x80;
195 switch (responseType) { 205​ switch (responseType) {
206​
196 case XCB_MAPPING_NOTIFY: 207​ case XCB_MAPPING_NOTIFY:
197 qCDebug(KGLOBALACCELD) << "Got XMappingNotify event"; ​
198 xcb_refresh_keyboard_mapping(m_keySymbols, reinterpret_cast<xcb_mapping_notify_event_t*>(event)); ​
199 x11MappingNotify(); 208​ x11MappingNotify();
anthonyfieroniUnsubmitted
Done

I see what you doing, but don't do it. See below.

anthonyfieroni: I see what you doing, but don't do it. See below.
200 return true; 209​
210​ // Make sure to let Qt handle it as well
211​ return false;
201 212​
202 case XCB_KEY_PRESS: 213​ case XCB_KEY_PRESS:
romanggUnsubmitted
Not Done

https://techbase.kde.org/Policies/Frameworks_Coding_Style#Braces space after if

romangg: https://techbase.kde.org/Policies/Frameworks_Coding_Style#Braces space after `if`
203#ifdef KDEDGLOBALACCEL_TRACE 214​#ifdef KDEDGLOBALACCEL_TRACE
204 qCDebug(KGLOBALACCELD) << "Got XKeyPress event"; 215​ qCDebug(KGLOBALACCELD) << "Got XKeyPress event";
205#endif 216​#endif
206 return x11KeyPress(reinterpret_cast<xcb_key_press_event_t*>(event)); 217​ return x11KeyPress(reinterpret_cast<xcb_key_press_event_t*>(event));
207 218​
208 default: 219​ default:
romanggUnsubmitted
Not Done

https://techbase.kde.org/Policies/Frameworks_Coding_Style#Braces space and on same line as closing brace

romangg: https://techbase.kde.org/Policies/Frameworks_Coding_Style#Braces space and on same line as…
220​ if(m_xkb_first_event && responseType == m_xkb_first_event) {
romanggUnsubmitted
Done
case m_xkb_first_event:
    if (m_xkb_first_event) {
        ...
    }
default:
    return false;

Or directly do the switching on responseType with if-else statements.

romangg: ``` case m_xkb_first_event: if (m_xkb_first_event) { ... } default: return…
fvogtAuthorUnsubmitted
Not Done
case m_xkb_first_event:

Is not valid C++ - case expressions have to be constants.

fvogt: ``` case m_xkb_first_event: ``` Is not valid C++ - case expressions have to be constants.
221​ const uint8_t xkbEvent = event->pad0;
anthonyfieroniUnsubmitted
Done

Cast to xcb_generic_event_t

anthonyfieroni: Cast to xcb_generic_event_t
222​ switch (xkbEvent) {
anthonyfieroniUnsubmitted
Done

Use pad0 (stupid name but you can get it as ref and name as you want)

anthonyfieroni: Use pad0 (stupid name but you can get it as ref and name as you want)
223​ case XCB_XKB_MAP_NOTIFY:
224​ x11MappingNotify();
225​ break;
226​ case XCB_XKB_NEW_KEYBOARD_NOTIFY: {
227​ const xcb_xkb_new_keyboard_notify_event_t *ev =
anthonyfieroniUnsubmitted
Done

Cast event to xcb_xkb_new_keyboard_notify_event_t.

anthonyfieroni: Cast event to xcb_xkb_new_keyboard_notify_event_t.
228​ reinterpret_cast<xcb_xkb_new_keyboard_notify_event_t*>(event);
229​ if (ev->changed & XCB_XKB_NKN_DETAIL_KEYCODES)
230​ x11MappingNotify();
231​ break;
232​ }
233​ default:
234​ break;
235​ }
236​
237​ // Make sure to let Qt handle it as well
238​ return false;
239​ }
240​
209 // We get all XEvents. Just ignore them. 241​ // We get all XEvents. Just ignore them.
210 return false; 242​ return false;
211 } 243​ }
212 244​
213 Q_ASSERT(false); 245​ Q_ASSERT(false);
214 return false; 246​ return false;
215} 247​}
216 248​
217void KGlobalAccelImpl::x11MappingNotify() 249​void KGlobalAccelImpl::x11MappingNotify()
218{ 250​{
251​ qCDebug(KGLOBALACCELD) << "Got XMappingNotify event";
252​
219 // Maybe the X modifier map has been changed. 253​ // Maybe the X modifier map has been changed.
220 // uint oldKeyModMaskXAccel = g_keyModMaskXAccel; 254​ // uint oldKeyModMaskXAccel = g_keyModMaskXAccel;
221 // uint oldKeyModMaskXOnOrOff = g_keyModMaskXOnOrOff; 255​ // uint oldKeyModMaskXOnOrOff = g_keyModMaskXOnOrOff;
222 256​
223 // First ungrab all currently grabbed keys. This is needed because we 257​ // First ungrab all currently grabbed keys. This is needed because we
224 // store the keys as qt keycodes and use KKeyServer to map them to x11 key 258​ // store the keys as qt keycodes and use KKeyServer to map them to x11 key
225 // codes. After calling KKeyServer::initializeMods() they could map to 259​ // codes. After calling KKeyServer::initializeMods() they could map to
226 // different keycodes. 260​ // different keycodes.
227 ungrabKeys(); 261​ ungrabKeys();
228 262​
263​ // Force reloading of the keySym mapping
264​ m_keySymbols = nullptr;
romanggUnsubmitted
Done

Might leak? Use xcb_key_symbols_free.

romangg: Might leak? Use `xcb_key_symbols_free`.
265​
229 KKeyServer::initializeMods(); 266​ KKeyServer::initializeMods();
230 calculateGrabMasks(); 267​ calculateGrabMasks();
231 268​
232 grabKeys(); 269​ grabKeys();
233} 270​}
234 271​
235bool KGlobalAccelImpl::x11KeyPress(xcb_key_press_event_t *pEvent) 272​bool KGlobalAccelImpl::x11KeyPress(xcb_key_press_event_t *pEvent)
236{ 273​{
▲ Show 20 Lines • Show All 44 Lines • Show Last 20 Lines