Drop repeat key events #98
Conversation
| * is used by window managers during task switching (either classic task | ||
| * switcher, or KDE "present windows" feature). | ||
| */ | ||
| if (ev->mode != NotifyNormal && ev->mode != NotifyWhileGrabbed) |
There was a problem hiding this comment.
Turns out this is the reason for extraneous keystrokes when cycling windows.
The window manager use NotifyGrab to signal temponany focus out.
There was a problem hiding this comment.
I kept having problems with this when writing the Wayland compositor. I had no idea what the cause was, though. I wound up having to try different approaches until I found one that worked. Hopefully this PR will fix the actual problem.
There was a problem hiding this comment.
I still don't know how to send correct focus events to windows. I tried:
XSendEventXGrabKeyboard
I don't know about Wayland though, I only tested with X server in VM.
There was a problem hiding this comment.
I don't know about Wayland though, I only tested with X server in VM.
Don’t worry about that. Something that works properly under X is hopefully going to be easier to handle with Wayland as well.
|
Actually, there is no telling that this "keypress" would come from a keyboard or not. |
| XIDeviceEvent * xi_device = (XIDeviceEvent *)xi_event; | ||
| XILeaveEvent * xi_leave = (XILeaveEvent *)xi_event; | ||
| switch (xi_event->evtype) { | ||
| // ideally raw input events are better, but I'm relying on X server's built-in event filtering and routing feature here |
There was a problem hiding this comment.
Is this to avoid information leaks if the GUI daemon thinks it has focus when it does not?
There was a problem hiding this comment.
Raw input events are sent to the root window, not individual windows. If I want to use raw events, I need to track window focus myself. Also, raw key events don't have modifiers key state (Ctrl, Alt), so I'll have to track that too.
| * is used by window managers during task switching (either classic task | ||
| * switcher, or KDE "present windows" feature). | ||
| */ | ||
| if (ev->mode != NotifyNormal && ev->mode != NotifyWhileGrabbed) |
There was a problem hiding this comment.
I kept having problems with this when writing the Wayland compositor. I had no idea what the cause was, though. I wound up having to try different approaches until I found one that worked. Hopefully this PR will fix the actual problem.
|
Code-signing check is failing because GitHub’s key isn’t considered verified. |
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.1&build=2022060604-4.1&flavor=pull-requests New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.1&build=2022031706-4.1&flavor=update
Failed tests9 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/36922#dependencies 5 fixed
Unstable tests
|
|
Also, don’t forget to add “FIxes: QubesOS/qubes-issues#7396” to whichever commit fixed that bug. |
|
I've used it for a month, and it's pretty stable. Sometimes, shortcuts like Super+S to open windows in dom0 will get S key stuck (likely focus issue). |
|
It doesn't build, see CI logs: |
|
Please also squash git history into functional commits (no WIP commits). |
|
Is it possible to use libxcb instead of libXi? |
Yes, but need to change source code a lot. Also I'm not sure what will happen if xcb is used together with Xlib. |
How do I do that? I feel like the commit history is descriptive enough. |
Using libxcb and libX11-like together, both for handling events is a bad idea. You will run into events reordering (and probably a lot of other issues). Not worth it in this PR. |
I don't like introducing not working commit that is known to be not working at merge time already (that is later fixed or even reverted in the same PR). For example 60ee007 is later basically reverted. You included also some non-trivial changes into a merge commit 6bddd0e... |
|
I'm not good at git to know how to rebase something like (A -> B -> revert A). I also think that git commits should reflect how software is actually developed, not how it should be (source). The diff of 6bddd0e is simple if you use a language-aware diff tool like https://github.com/Wilfred/difftastic. Is it possible to squash the whole thing into a single commit? |
Take a look here for example: https://gitbetter.substack.com/p/how-to-squash-git-commits Anyway, I tried to use it, and the key repeat in VM doesn't work. It isn't surprising, because it's disabled. After enabling it, it works. Just enabling it is not a problem, but I'm worried about extra parameters (delay, rate). Those user normally set in dom0 (or wherever GUI runs) and should apply to all the VMs. Synchronizing them may be messy but probably doable... Anyway, handling it in some way is necessary for merging this change. |
This failure actually may be related to this PR. The test is supposed to type in |
|
I have no idea how to untangle this git history. I cleaned up the history up to the last merge commit. Git is broken. I tried to do |
gui-daemon/xside.c
Outdated
| XISetMask(xi_mask.mask, XI_KeyRelease); | ||
| XISetMask(xi_mask.mask, XI_FocusIn); | ||
| XISetMask(xi_mask.mask, XI_FocusOut); | ||
| XISelectEvents(g->display, child_win, &xi_mask, 1); |
There was a problem hiding this comment.
If this fails (returns anything but Success) the code should exit.
gui-daemon/xside.c
Outdated
| { | ||
| struct msg_hdr hdr; | ||
| char keys[32]; | ||
| XQueryKeymap(g->display, keys); |
There was a problem hiding this comment.
If this does not succeed, just exit.
|
|
||
| // select xinput events | ||
| XIEventMask xi_mask; | ||
| xi_mask.deviceid = XIAllMasterDevices; // https://stackoverflow.com/questions/44095001/getting-double-rawkeypress-events-using-xinput2 |
There was a problem hiding this comment.
| xi_mask.deviceid = XIAllMasterDevices; // https://stackoverflow.com/questions/44095001/getting-double-rawkeypress-events-using-xinput2 | |
| // https://stackoverflow.com/questions/44095001/getting-double-rawkeypress-events-using-xinput2 | |
| xi_mask.deviceid = XIAllMasterDevices; |
What do you normally use? One option is to just create a patch series and use |
|
How is this normally handled by X applications? That is, what event sequence do X applications normally get if a key is pressed while they have keyboard focus, and is released after they have lost focus? Qubes should send the same event sequence in the same order. This should fix other bugs as well. |
We discussed about this earlier. The problem is
I suspect it's sometimes
I never rebase graph. I usually rebase linear history. |
|
Still one more bug that I can't fix: When 1 VM switch from vm0:window0 to vm0:window1, I can't regain focus by sending focus events. This bug didn't exist before this PR. Sending focus on mouse down is not working. |
How does the dom0 window manager give focus to a window? It should be possible for the GUI agent to do the same with its windows.
Those events should be able to be sent via the X11 driver, which is part of the GUI agent. |
|
"Focus window when clicked" was originally handled by
Some special focus events are traditionally sent by like XFCE4, and Xlib API cannot send that. |
Presumably there is some API that XFCE4 is using; Qubes can use the same. |
|
I think XFCE4 use xf86 (library for window manager/ X driver). This should be solved in vmside.c. See QubesOS/qubes-gui-agent-linux#154 (comment) |
|
@locriacyber Can you drop the focus-related changes from this PR? I based #104 on them. |
|
I wrote them at the same time, and it would be difficult to separate in git history. Do you want just the focus part as separate branch? Also, just having |
|
The focus code has a bug: when clicking on IBus's tray icon, I get three simultaneous popups. I don't know if IBus think it's been clicked thrice, or it's something else. |
|
Is it possible to dynamically negotiate protocol version? Currently in vmside.c: it's hardcoded. |
|
@locriacyber Not yet. It will be starting with protocol 1.4 (#105). |
|
@locriacyber Protocol 1.4 has been merged, so dynamic negotiation is now possible. |
|
To whom may be reading this |
This is actually usable now.
Close QubesOS/qubes-issues#7231