<div dir="ltr">Hi,<div class="gmail_extra"><br><div class="gmail_quote">On 25 November 2014 at 02:38, Keith Packard <span dir="ltr"><<a href="mailto:keithp@keithp.com" target="_blank">keithp@keithp.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">Daniel Stone <<a href="mailto:daniel@fooishbar.org">daniel@fooishbar.org</a>> writes:<br>
> Because that's still a bunch of work,<br>
<br>
</span>I think it's just:<br>
<br>-    wl_array_for_each(k, &xwl_seat->keys)<br>
<span class="">-        QueueKeyboardEvents(xwl_seat->keyboard, KeyPress, *k + 8, &mask);<br>
</span>+    wl_array_for_each(k, &xwl_seat->keys) {<br>
+        if (xwl_seat->keyboard->key->xkbInfo->desc->map->modmap[*k+8] != 0)<br>
<span class="">+            QueueKeyboardEvents(xwl_seat->keyboard, KeyPress, *k + 8, &mask);<br>
+    }<br>
 }<br></span></blockquote><div><br></div><div>Is it? You have to make sure to the focus is NULL at this point, otherwise you send surprise events to windows that don't want it. And if the focus is NULL, it has pretty much the same effect as my patch; it's just that it's stripped of context when you're trying to debug things at lower levels.</div><div><br></div><div>Think of the case where you enter with Alt down. If the user presses F, you want the file menu to trigger, but when the user releases Alt without pressing anything, you don't want to activate the menubar.</div><div><br></div><div>I can also see the case for passing all keys along on enter: after all, that's exactly what KeymapNotify does. If you think this is fine, would you also accept a patch to do this on every focus change inside normal X11? And if not, why not ... ?</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> This patch isn't the end of the road; in the end, it should be<br>
> entirely possible to achieve perfect nesting wrt KeymapNotify events<br>
> with XWayland, Xephyr, XQuartz, and any combination thereof. This is a<br>
> relatively start in that direction.<br>
<br>
</span>Yeah, I'm fine with figuring out how to make it perfect; if the above<br>
patch will suffice to get modifiers handled correctly for now, I'd like<br>
to go with that for this release and pend the perfect solution until<br>
after 1.17; just too many changes outside of hw/xwayland to make me<br>
happy at this point in the release cycle.<br></blockquote><div><br></div><div>I think they're all pretty harmless as they're confined to new codepaths which were not previously triggerable, and are still not triggerable outside of Xwayland.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
In particular, I can imagine passing a new boolean in, or setting some<br>
device state, that muted event generation and avoided having to check<br>
for ET_KeyFocusIn in a bunch of places now and in the future. That would<br>
also encourage us to actually deal with this case in Xephyr and Xnest.<br></blockquote><div><br></div><div>How is that better? You've shifted some local state (what's the event type?) into global state (is this thing set?). The penalties for missing a check are exactly the same for both, except the intention is much more obvious with an explicit event. The event type also has the benefit of being able to be posted through the queue, rather than OsBlockSignals -> ProcessInputEvents -> mangle magic global flag -> QueueKeyboardEvents -> ProcessKeyboardEvents -> OsUnblockSignals. I'd much rather take the hit of tiny extra complexity in the core (really, it's not much at all) rather than having all the users get it wrong, or simply not bother because it's too hard. Much like they do today.</div><div><br></div><div>If you're implacably opposed, then I'll ack the minimal change (once we can be certain that the focus is indeed NULL whilst posting those events) for 1.17, but I'll also request a revert immediately after master reopens, and resubmit this patchset. Your patch does just happen to work, but it's an extra whack of magic bonghits right where we least need them.</div><div><br></div><div>Cheers,</div><div>Daniel </div></div></div></div>