Bug 16145

Created attachment 16798 [details]
CapsLockToggle.c

Compile it with 'gcc -Wall -o CapsToggle CapsToggle.c -l Xtst'.

On Thu, May 29, 2008 at 12:37:12AM -0700, bugzilla-daemon@freedesktop.org wrote:
> A simulated Caps Lock key press makes the logical and the physical states of
> the Caps Lock out of sync.  This causes a problem as one of OpenOffice's
> functionality - correct accidental use of cAPS LOCK key - relies on this
> feature of X.

confirmed with git master.

Now, with 1.4 and master having similar subsystems I think I found what the
issue is.
1.3 and before had one physical device as the "core keyboard". 1.4 and beyond
however feature a "virtual core keyboard" (VCK), all physical input devices
are extension keyboards.

If you hit a key on the keyboard, the core event is generated by the VCK, and
an XI event by the actual keyboard. The LED change then happens when the XI
event is passed through xkb (the LEDS on the virtual keyboard change too,
but, well...).

If you use xtest for core events, the server picks the VCK and processes it
accordingly - but, because it is the VCK you don't see the LED change.
To fix this, you essentially have to pick which devices need the LED state
updated and run it past them as well. Which gets a bit iffy, if you have
multiple keyboards, some of which have CL on and some of which have it off
(which AFAIK should work now). Which ones do you pick? Do you switch CL on? or
off?

Comments?

As you can see in the test program

a) I didn't pick any special keyboard at all
b) it's just a toggle (no explicit turn on/off)

I attach a more complex test program for turning numlock on/off or toggle it,
which still works with xserver 1.4.0.90. Not sure how to adjust it to get also Capslock working. I'm afraid numlock and Capslock needs to be handled slightly different.

Created attachment 16806 [details]
numlockx-1.1.tar.gz

Upstream hasn't maintained numlockx code in ages. Already, Debian/Ubuntu had to autoreconf to accommodate the modular X.org structure. If anybody feels like taking over upstream maintenance and issuing a patched source with proper #ifdef to compile this on recent X. please do so and inform me of the new upstream tarball location, so that I update the debian package. Thanks!

On Thu, May 29, 2008 at 01:28:54AM -0700, bugzilla-daemon@freedesktop.org wrote:
> If you hit a key on the keyboard, the core event is generated by the VCK, and
> an XI event by the actual keyboard. The LED change then happens when the XI
> event is passed through xkb (the LEDS on the virtual keyboard change too,
> but, well...).
> 
> If you use xtest for core events, the server picks the VCK and processes it
> accordingly - but, because it is the VCK you don't see the LED change.
> To fix this, you essentially have to pick which devices need the LED state
> updated and run it past them as well. Which gets a bit iffy, if you have
> multiple keyboards, some of which have CL on and some of which have it off
> (which AFAIK should work now). Which ones do you pick? Do you switch CL on? or
> off?
> 
> Comments?

I think the only thing that makes sense is for XTest to use GKE to
generate events for the keyboard that last sent core events.  Principle
of least surprise and all -- why mangle other keyboards?

(In reply to comment #5)
> Upstream hasn't maintained numlockx code in ages. Already, Debian/Ubuntu had
> to autoreconf to accommodate the modular X.org structure. If anybody feels
> like taking over upstream maintenance and issuing a patched source with
> proper #ifdef to compile this on recent X. please do so and inform me of the
> new upstream tarball location, so that I update the debian package. Thanks!

This is completely off-topic. Please discuss this issue with Lubos Lunak via private mail, IRC or whatever. numlockx is *not* the problem here at all!

> --- Comment #6 from Daniel Stone <daniel@fooishbar.org>  2008-05-29 03:20:01 PST ---
> I think the only thing that makes sense is for XTest to use GKE to
> generate events for the keyboard that last sent core events.  Principle
> of least surprise and all -- why mangle other keyboards?

that would actually be easy now. MDs keep dev->u.lastSlave around, so we could
just route through that. For 1.4, this would be slightly more complicated.
First thought is to just add two fields to inputInfo.

(In reply to comment #8)
> > --- Comment #6 from Daniel Stone <daniel@fooishbar.org>  2008-05-29 03:20:01 PST ---
> > I think the only thing that makes sense is for XTest to use GKE to
> > generate events for the keyboard that last sent core events.  Principle
> > of least surprise and all -- why mangle other keyboards?
> 
> that would actually be easy now. MDs keep dev->u.lastSlave around, so we could
> just route through that. For 1.4, this would be slightly more complicated.
> First thought is to just add two fields to inputInfo.

possibly not so complicated -- what about the following patch:
diff --git a/dix/getevents.c b/dix/getevents.c
index 2fd4e54..79366f8 100644
--- a/dix/getevents.c
+++ b/dix/getevents.c
@@ -414,6 +414,10 @@ GetKeyboardValuatorEvents(xEvent *events, DeviceIntPtr pDev, int type,
     CARD32 ms = 0;
     deviceKeyButtonPointer *kbp = NULL;
 
+    if (pDev == inputInfo.keyboard)
+        pDev = dixLookupPrivate(&inputInfo.keyboard->devPrivates,
+                                CoreDevicePrivateKey);
+
     if (!events)
         return 0;
 

(assuming all callers of GK{,V}E are happy with this.)

Daniel, is the patch for 1.4 or git master? I would be happy to try it with 1.4.0.90.

On Thu, May 29, 2008 at 06:50:35AM -0700, bugzilla-daemon@freedesktop.org wrote:
> Daniel, is the patch for 1.4 or git master? I would be happy to try it with
> 1.4.0.90.

Should work with both, modulo merge detritus (although MPX may get rid
of the private and replace it with ->lastSlave).  The point is that, if
GetKeyboardEvents is called with inputInfo.keyboard as the device, to
change the device to the last device which used it to send events.  It's
definitely not suitable for applying or distributions though, as I
haven't checked all callers of GK(V)E; it may have bad side effects.

On Thu, May 29, 2008 at 07:08:33 -0700, bugzilla-daemon@freedesktop.org wrote:

> --- Comment #11 from Daniel Stone <daniel@fooishbar.org>  2008-05-29 07:08:33 PST ---
> On Thu, May 29, 2008 at 06:50:35AM -0700, bugzilla-daemon@freedesktop.org
> wrote:
> > Daniel, is the patch for 1.4 or git master? I would be happy to try it with
> > 1.4.0.90.
> 
> Should work with both, modulo merge detritus

Modulo devPrivates rework, too, as 1.4 doesn't have dixLookupPrivate().
You'll need pDev->devPrivates[CoreDevicePrivatesIndex].ptr I think.

Cheers,
Julien

> possibly not so complicated -- what about the following patch:
> diff --git a/dix/getevents.c b/dix/getevents.c
> index 2fd4e54..79366f8 100644
> --- a/dix/getevents.c
> +++ b/dix/getevents.c
> @@ -414,6 +414,10 @@ GetKeyboardValuatorEvents(xEvent *events, DeviceIntPtr
> pDev, int type,
>      CARD32 ms = 0;
>      deviceKeyButtonPointer *kbp = NULL;
> 
> +    if (pDev == inputInfo.keyboard)
> +        pDev = dixLookupPrivate(&inputInfo.keyboard->devPrivates,
> +                                CoreDevicePrivateKey);
> +
>      if (!events)
>          return 0;
> 
> 
> (assuming all callers of GK{,V}E are happy with this.)

won't work, ProcXTestFakeInput doesn't use GPE/GKVE right now. I have a patch
for master in the pipe here, but it still needs some special handling for
xinerama. once it is in, it can probably be backported to 1.4.

I just pushed a fix to master [1] that fixes this issue. Unfortunately it turned
out rather invasive, so it needs active backporting to 1.4.

The main part is to switch ProcXTestFakeInput to use GPE/GKVE, and flip the
device to the last-used slave device.

[1] 105d28652d1fb80dd8ce8511e2605dccc8812e99

Is all this related to bug#15359 and bug#9964 ? I am having a hard time trying to sort out all our LED-related bugs.

> --- Comment #15 from Brice Goglin <brice.goglin@ens-lyon.org>  2008-06-21 11:16:22 PST ---
> Is all this related to bug#15359 and bug#9964 ? I am having a hard time trying
> to sort out all our LED-related bugs.

I don't think so. This bug is a special case as it modifies the state of the
VCP without going through the physical device (using XTest).
#15359 and #9964 are caused by wrong processing of the device.

CLosing as fixed.

Hi *,

It seems it's broken again on current 7.5 version. If you take and launch the sample program (CapsLockToggle.c), it won't light the LED as it should.

Can someone please re-open the bug ?

Thanks,

I can confirm Michel Loiseleur's comment, the original issue is present again with 7.5. I'm reopening the bug.

Thanks!

I can confirm that the behaviour is broken again (LED doesn't lighten up/down) with xorg-server 1.9.0 when running the CapsLockToggle sample.

numlockx works as expected (including LED status) on xorg-server 1.9.0.

right, so this situation is actually quite complicated.

First - any XTEST event will be executed on the virtual XTEST device, which will have the LED on. Being virtual, that's of limited use. Since the same event travels through the matching master device (virtual core keyboard (VCK), in most cases), this one will also have the LED on. Of limited, use, it's virtual too.

Using XTEST to switch a physical keyboard's LEDs on or off becomes complicated because each keyboard may have different indicators (and modifiers, etc.) so it's not clear-cut which indicators to switch on or off and under which conditions to do so. Furthermore, because we handle keyboards separately (and the event handling is quite frankly a disaster) there is a disconnect between the actual keyboard devices and the merged master device, the VCK. Hitting CapsLock on a physical keyboard will set the lock on this keyboard and thus on the master device. Sending a CapsLock through XTEST will set CapsLock on the XTEST device but unlock it on the merged device. So now you have two keyboard devices with CapsLock on, but the master device has it off. For indicators, the same is true of course.

I have yet to find a correct solution for the server to handle this case, especially when there are multiple keyboard layouts present. However, there's a simple solution for clients to deal with this: send XTEST events through the matching physical device that forced the CapsLock on. This will unset CapsLock on the physical keyboard and thus also extinguish the LED.

(In reply to comment #22)
> Using XTEST to switch a physical keyboard's LEDs on or off becomes
> complicated because each keyboard may have different indicators (and
> modifiers, etc.) so it's not clear-cut which indicators to switch on or off
> and under which conditions to do so. Furthermore, because we handle
> keyboards separately (and the event handling is quite frankly a disaster)
> there is a disconnect between the actual keyboard devices and the merged
> master device, the VCK. Hitting CapsLock on a physical keyboard will set the
> lock on this keyboard and thus on the master device. Sending a CapsLock
> through XTEST will set CapsLock on the XTEST device but unlock it on the
> merged device. So now you have two keyboard devices with CapsLock on, but
> the master device has it off. For indicators, the same is true of course.

You have my sincere thanks. Now that I finally know what causes the LED to go out of sync, I can trigger it intentionally by running this on login:

numlockx off; xdotool key Num_Lock

I still don't know how to break the modifier state and the LED apart so I can flash the LED to indicate something like unread mail (see Bug 49276), but at least it can warn me if I accidentally disable NumLock (which I only do intentionally in certain games), rather than requiring me to put a piece of tape over it.

(My keyboard is a Rosewill RK-9000I. The LED is one of those ultra-bright, water-clear lens'd blue ones meant more for illumination than use as indicators and the surrounding plastic is white as snow. It'd be bothersome to find pure white electrical tape, ugly to apply enough layers to block the LED, and I don't want void the warranty by desoldering it.)

From what I can tell this got fixed some time ago:

https://cgit.freedesktop.org/xorg/xserver/commit/?id=45fb3a934dc0db51584aba37c2f9d73deff9191d

That patch seems to solve the use cases I'm seeing with XTest at least.

indeed, that one should've fixed this issue. Thanks for the follow-up.