Differential D16434

Fix keyboard layout change notifications
ClosedPublic
Actions

Authored by fvogt on Oct 26 2018, 8:04 AM.

Details

Reviewers
romangg
Group Reviewers
Frameworks
Plasma
Commits
R268:4d28bd4183d2: Fix keyboard layout change notifications
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.

Diff Detail

Repository
R268 KGlobalAccel
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 4481
Build 4499: arc lint + arc unit
fvogt created this revision.Oct 26 2018, 8:04 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptOct 26 2018, 8:04 AM
fvogt requested review of this revision.Oct 26 2018, 8:04 AM
anthonyfieroni added a subscriber: anthonyfieroni.Oct 26 2018, 11:00 AM
anthonyfieroni added inline comments.
src/runtime/plugins/xcb/kglobalaccel_x11.cpp
200–210

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

223

Cast to xcb_generic_event_t

224

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

229

Cast event to xcb_xkb_new_keyboard_notify_event_t.

fvogt added a comment.Oct 26 2018, 11:11 AM

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.

fvogt updated this revision to Diff 44247.Oct 26 2018, 11:18 AM

Don't use a union. Still works.

fvogt marked 4 inline comments as done.Oct 26 2018, 11:18 AM
romangg requested changes to this revision.Nov 2 2018, 3:17 PM
romangg added a subscriber: romangg.
romangg added inline comments.
src/runtime/plugins/xcb/kglobalaccel_x11.cpp
98–101

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

222
case m_xkb_first_event:
    if (m_xkb_first_event) {
        ...
    }
default:
    return false;

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

266

Might leak? Use xcb_key_symbols_free.

This revision now requires changes to proceed.Nov 2 2018, 3:17 PM
fvogt updated this revision to Diff 44714.Nov 2 2018, 6:52 PM
fvogt marked an inline comment as done.

Add Q_ASSERT and xcb_key_symbols_free.

fvogt updated this revision to Diff 44715.Nov 2 2018, 7:07 PM

Use if-else ladder instead of switch-case and fix a typo.

fvogt marked 2 inline comments as done.Nov 2 2018, 7:08 PM
fvogt added inline comments.
src/runtime/plugins/xcb/kglobalaccel_x11.cpp
98–101

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.

222
case m_xkb_first_event:

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

romangg accepted this revision.Nov 3 2018, 11:36 AM

Pls do the coding style improvements and push.

src/runtime/plugins/xcb/kglobalaccel_x11.cpp
75

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

215

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

221

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

This revision is now accepted and ready to land.Nov 3 2018, 11:36 AM
fvogt updated this revision to Diff 44765.Nov 3 2018, 12:13 PM
fvogt marked 2 inline comments as done.

This file needs to be reformatted anyway.

Closed by commit R268:4d28bd4183d2: Fix keyboard layout change notifications (authored by fvogt). · Explain WhyNov 3 2018, 12:14 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 44714

View Options

CMakeLists.txt

Loading...
View Options

src/runtime/plugins/CMakeLists.txt

Loading...
View Options

src/runtime/plugins/xcb/CMakeLists.txt

Loading...
View Options

src/runtime/plugins/xcb/kglobalaccel_x11.h

Loading...
View Options

src/runtime/plugins/xcb/kglobalaccel_x11.cpp

Loading...