D16434: Fix keyboard layout change notifications
This revision was automatically updated to reflect the committed changes. Closed by commit R268:4d28bd4183d2: Fix keyboard layout change notifications (authored by fvogt). REPOSITORY R268 KGlobalAccel CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D16434?vs=44765=44766 REVISION DETAIL https://phabricator.kde.org/D16434 AFFECTED FILES CMakeLists.txt src/runtime/plugins/CMakeLists.txt src/runtime/plugins/xcb/CMakeLists.txt src/runtime/plugins/xcb/kglobalaccel_x11.cpp src/runtime/plugins/xcb/kglobalaccel_x11.h To: fvogt, #frameworks, #plasma, romangg Cc: romangg, ngraham, anthonyfieroni, kde-frameworks-devel, michaelh, bruns
D16434: Fix keyboard layout change notifications
fvogt updated this revision to Diff 44765. fvogt marked 2 inline comments as done. fvogt added a comment. This file needs to be reformatted anyway. REPOSITORY R268 KGlobalAccel CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D16434?vs=44715=44765 BRANCH master REVISION DETAIL https://phabricator.kde.org/D16434 AFFECTED FILES CMakeLists.txt src/runtime/plugins/CMakeLists.txt src/runtime/plugins/xcb/CMakeLists.txt src/runtime/plugins/xcb/kglobalaccel_x11.cpp src/runtime/plugins/xcb/kglobalaccel_x11.h To: fvogt, #frameworks, #plasma, romangg Cc: romangg, ngraham, anthonyfieroni, kde-frameworks-devel, michaelh, bruns
D16434: Fix keyboard layout change notifications
romangg accepted this revision.
romangg added a comment.
This revision is now accepted and ready to land.
Pls do the coding style improvements and push.
INLINE COMMENTS
> kglobalaccel_x11.cpp:75
> + const xcb_query_extension_reply_t *reply =
> xcb_get_extension_data(QX11Info::connection(), _xkb_id);
> + if (reply && reply->present)
> + m_xkb_first_event = reply->first_event;
https://techbase.kde.org/Policies/Frameworks_Coding_Style#Braces
> kglobalaccel_x11.cpp:207
> const uint8_t responseType = event->response_type & ~0x80;
> -switch (responseType) {
> -case XCB_MAPPING_NOTIFY:
> -qCDebug(KGLOBALACCELD) << "Got XMappingNotify event";
> -xcb_refresh_keyboard_mapping(m_keySymbols,
> reinterpret_cast(event));
> -x11MappingNotify();
> -return true;
> -
> -case XCB_KEY_PRESS:
> +if(responseType == XCB_MAPPING_NOTIFY) {
> +x11MappingNotify();
https://techbase.kde.org/Policies/Frameworks_Coding_Style#Braces space after
`if`
> kglobalaccel_x11.cpp:213
> +}
> +else if(responseType == XCB_KEY_PRESS) {
> #ifdef KDEDGLOBALACCEL_TRACE
https://techbase.kde.org/Policies/Frameworks_Coding_Style#Braces space and on
same line as closing brace
REPOSITORY
R268 KGlobalAccel
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D16434
To: fvogt, #frameworks, #plasma, romangg
Cc: romangg, ngraham, anthonyfieroni, kde-frameworks-devel, michaelh, bruns
D16434: Fix keyboard layout change notifications
fvogt updated this revision to Diff 44715. fvogt added a comment. Use if-else ladder instead of switch-case and fix a typo. REPOSITORY R268 KGlobalAccel CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D16434?vs=44714=44715 BRANCH master REVISION DETAIL https://phabricator.kde.org/D16434 AFFECTED FILES CMakeLists.txt src/runtime/plugins/CMakeLists.txt src/runtime/plugins/xcb/CMakeLists.txt src/runtime/plugins/xcb/kglobalaccel_x11.cpp src/runtime/plugins/xcb/kglobalaccel_x11.h To: fvogt, #frameworks, #plasma, romangg Cc: romangg, ngraham, anthonyfieroni, kde-frameworks-devel, michaelh, bruns
D16434: Fix keyboard layout change notifications
fvogt marked 2 inline comments as done.
fvogt added inline comments.
INLINE COMMENTS
> romangg wrote in kglobalaccel_x11.cpp:96
> Before there was a check to `QX11Info::isPlatformX11()`. Probably we don't
> need the check, but did you test on Wayland?
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.
> romangg wrote in kglobalaccel_x11.cpp:220
> case m_xkb_first_event:
> if (m_xkb_first_event) {
> ...
> }
> default:
> return false;
>
> Or directly do the switching on `responseType` with if-else statements.
case m_xkb_first_event:
Is not valid C++ - case expressions have to be constants.
REPOSITORY
R268 KGlobalAccel
REVISION DETAIL
https://phabricator.kde.org/D16434
To: fvogt, #frameworks, #plasma, romangg
Cc: romangg, ngraham, anthonyfieroni, kde-frameworks-devel, michaelh, bruns
D16434: Fix keyboard layout change notifications
fvogt updated this revision to Diff 44714. fvogt marked an inline comment as done. fvogt added a comment. Add Q_ASSERT and xcb_key_symbols_free. REPOSITORY R268 KGlobalAccel CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D16434?vs=44247=44714 BRANCH master REVISION DETAIL https://phabricator.kde.org/D16434 AFFECTED FILES CMakeLists.txt src/runtime/plugins/CMakeLists.txt src/runtime/plugins/xcb/CMakeLists.txt src/runtime/plugins/xcb/kglobalaccel_x11.cpp src/runtime/plugins/xcb/kglobalaccel_x11.h To: fvogt, #frameworks, #plasma, romangg Cc: romangg, ngraham, anthonyfieroni, kde-frameworks-devel, michaelh, bruns
D16434: Fix keyboard layout change notifications
romangg requested changes to this revision.
romangg added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> kglobalaccel_x11.cpp:96
> if (!m_keySymbols) {
> -return false;
> +m_keySymbols = xcb_key_symbols_alloc(QX11Info::connection());
> +if (!m_keySymbols) {
Before there was a check to `QX11Info::isPlatformX11()`. Probably we don't need
the check, but did you test on Wayland?
> kglobalaccel_x11.cpp:220
> default:
> +if(m_xkb_first_event && responseType == m_xkb_first_event) {
> +const uint8_t xkbEvent = event->pad0;
case m_xkb_first_event:
if (m_xkb_first_event) {
...
}
default:
return false;
Or directly do the switching on `responseType` with if-else statements.
> kglobalaccel_x11.cpp:264
> + // Force reloading of the keySym mapping
> + m_keySymbols = nullptr;
> +
Might leak? Use `xcb_key_symbols_free`.
REPOSITORY
R268 KGlobalAccel
REVISION DETAIL
https://phabricator.kde.org/D16434
To: fvogt, #frameworks, #plasma, romangg
Cc: romangg, ngraham, anthonyfieroni, kde-frameworks-devel, michaelh, bruns
D16434: Fix keyboard layout change notifications
fvogt updated this revision to Diff 44247. fvogt added a comment. Don't use a union. Still works. REPOSITORY R268 KGlobalAccel CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D16434?vs=44241=44247 BRANCH master REVISION DETAIL https://phabricator.kde.org/D16434 AFFECTED FILES CMakeLists.txt src/runtime/plugins/CMakeLists.txt src/runtime/plugins/xcb/CMakeLists.txt src/runtime/plugins/xcb/kglobalaccel_x11.cpp src/runtime/plugins/xcb/kglobalaccel_x11.h To: fvogt, #frameworks, #plasma Cc: anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns
D16434: Fix keyboard layout change notifications
fvogt marked 4 inline comments as done. REPOSITORY R268 KGlobalAccel REVISION DETAIL https://phabricator.kde.org/D16434 To: fvogt, #frameworks, #plasma Cc: anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns
D16434: Fix keyboard layout change notifications
fvogt added a comment. Note that the way it's done is ~~copied from~~ inspired by QXcbConnection. I don't like it either though, so I'll try getting rid of the union. REPOSITORY R268 KGlobalAccel REVISION DETAIL https://phabricator.kde.org/D16434 To: fvogt, #frameworks, #plasma Cc: anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns
D16434: Fix keyboard layout change notifications
anthonyfieroni added inline comments.
INLINE COMMENTS
> kglobalaccel_x11.cpp:198-208
> +typedef union {
> + /* All XKB events share these fields. */
> + struct {
> + uint8_t response_type;
> + uint8_t xkbType;
> + uint16_t sequence;
> + xcb_timestamp_t time;
I see what you doing, but don't do it. See below.
> kglobalaccel_x11.cpp:233
> +if(m_xkb_first_event && responseType == m_xkb_first_event) {
> +_xkb_event *xkb_event = reinterpret_cast<_xkb_event*>(event);
> +switch (xkb_event->any.xkbType) {
Cast to xcb_generic_event_t
> kglobalaccel_x11.cpp:234
> +_xkb_event *xkb_event = reinterpret_cast<_xkb_event*>(event);
> +switch (xkb_event->any.xkbType) {
> +case XCB_XKB_MAP_NOTIFY:
Use pad0 (stupid name but you can get it as ref and name as you want)
> kglobalaccel_x11.cpp:239
> +case XCB_XKB_NEW_KEYBOARD_NOTIFY: {
> +xcb_xkb_new_keyboard_notify_event_t *ev =
> _event->new_keyboard_notify;
> +if (ev->changed & XCB_XKB_NKN_DETAIL_KEYCODES)
Cast event to xcb_xkb_new_keyboard_notify_event_t.
REPOSITORY
R268 KGlobalAccel
REVISION DETAIL
https://phabricator.kde.org/D16434
To: fvogt, #frameworks, #plasma
Cc: anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns
D16434: Fix keyboard layout change notifications
fvogt created this revision. fvogt added reviewers: Frameworks, Plasma. Herald added a project: Frameworks. fvogt requested review of this revision. REVISION SUMMARY This rework fixes several issues: - Qt wasn't informed about XCB_MAPPING_NOTIFY anymore - With XKB enabled in Qt, X won't send XCB_MAPPING_NOTIFY anymore. So listen for XKB events as well. - Install the event filter before fetching the keysym mapping to close a race window - Use the old mapping for ungrabbing BUG: 350816 BUG: 269403 TEST PLAN Ctrl-Alt-Y global shortcut works even when doing "setxkbmap us; kglobalaccel5 & sleep 1; setxkbmap de;" while it did not before. On some systems, this race happened on every login, now it works reliably. REPOSITORY R268 KGlobalAccel BRANCH master REVISION DETAIL https://phabricator.kde.org/D16434 AFFECTED FILES CMakeLists.txt src/runtime/plugins/CMakeLists.txt src/runtime/plugins/xcb/CMakeLists.txt src/runtime/plugins/xcb/kglobalaccel_x11.cpp src/runtime/plugins/xcb/kglobalaccel_x11.h To: fvogt, #frameworks, #plasma Cc: kde-frameworks-devel, michaelh, ngraham, bruns
