<table><tr><td style="">dfaure added inline comments.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D6233" rel="noreferrer">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D6233#inline-25781" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">graesslin</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kkeyserver.cpp:976-980</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">This is a special handling from KGlobalAccel. Are you sure it belongs into the generic implementation?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I would say yes, because e.g. Ctrl+& (on qwerty) is going to include Shift no matter which application is calling this method, and it should be removed (just like Qt does when recording "Ctrl+&" rather than "Ctrl+Shift+&" or "Ctrl+Shift+7".</p>

<p style="padding: 0; margin: 8px;">But anyway in practice the only other caller of this method is kwin, and it only cares for [Ctrl|Alt|None]+[Left|Right|Up|Down|Space|Return|Escape], if I read the code correctly.</p>

<p style="padding: 0; margin: 8px;">See <a href="https://lxr.kde.org/ident?_i=xcbKeyPressEventToQt&_remember=1" class="remarkup-link" target="_blank" rel="noreferrer">https://lxr.kde.org/ident?_i=xcbKeyPressEventToQt&_remember=1</a><br />
and AbstractClient::keyPressEvent in kwin.</p>

<p style="padding: 0; margin: 8px;">So, while technically this code could stay in kglobalaccel, it still seems more correct to me to have it here in the generic "xcb key event to keyQt conversion" code (and this avoids calling xcb_key_symbols_alloc again in kglobalaccel).</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D6233#inline-25782" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">graesslin</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kkeyserver_x11.h:161</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">xkb has the modifier mask defined as uint32_t:</p>

<p style="padding: 0; margin: 8px;">typedef uint32_t xkb_mod_mask_t</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">strange, I see this:</p>

<p style="padding: 0; margin: 8px;">typedef struct xcb_key_press_event_t {<br />
[...]</p>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">uint16_t        state; /**<  */</pre></div>

<p style="padding: 0; margin: 8px;">[...]</p>

<p style="padding: 0; margin: 8px;">and <tt style="background: #ebebeb; font-size: 13px;">grep modifiers /usr/include/xcb/xproto.h| grep uint</tt> shows consistent use of uint16_t.</p>

<p style="padding: 0; margin: 8px;">xkb_mod_mask_t seems to come from xcbcommon which is unrelated to the APIs used here, right?</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R278 KWindowSystem</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D6233" rel="noreferrer">https://phabricator.kde.org/D6233</a></div></div><br /><div><strong>To: </strong>dfaure, graesslin<br /><strong>Cc: </strong>graesslin, Frameworks<br /></div>