Re: [Qemu-devel] [Bug 427612] Re: kvm sends caps lock key up event twice

[Stefan Weil <weil@mail.berlios.de>]

Am 24.11.2010 22:44, schrieb Benjamin Drung:
> Attached a new version of my patch. You find two branches linked to this
> bug for maverick and natty. The patch sets SDL_DISABLE_LOCK_KEYS and get
> rid of the complete workaround in qemu-kvm. This requires SDL >= 1.2.14.

For newer versions of SDL (those which use SDL_DISABLE_LOCK_KEYS
to disable special handling of the lock keys), your patch looks ok.
It is a working solution for maintainers of the latest Ubuntu releases.

But what about older versions? For those, your patch will make QEMU's
keyboard emulation unusable. Most older versions don't know
SDL_DISABLE_LOCK_KEYS. As far as I know, some SDL versions (from Debian
and Ubuntu) even used SDL_DISABLE_LOCK_KEYS with an inverted meaning.

I don't think we can simply ignore old or incompatible versions of SDL.
Therefore I suggest adding a runtime version check, so the emulation
can either use the old code (SDL < 1.2.14) or your new code (SDL >= 1.2.14).

The crucial point is whether "old" versions must be supported or not.
This is something which the QEMU community should decide.

> Stefan Weil wrote:
>> The patch might fix part of the problem, but there remain more issues:
>>
>> * SDL also sends an SDL_KEYUP event for caps lock when the
>> environment variable SDL_DISABLE_LOCK_KEYS is set.
>> This mode is very useful but currently unsupported by qemu/kvm.
>
> Addressed by new patch.
>
>> * Num lock and caps lock are handled in a similar way by SDL.
>> The patch only handles caps lock. Maybe this is less important
>> because keyboard layouts which remap num lock are rare
>> (I don't know any).
>
> Addressed by new patch.
>
>> * The keyboard status LEDs and the qemu client's keyboard status
>> can become unsynchronized if the input focus changes from qemu
>> to other applications.
>
> Is this a regression of my patch or is it the case for the unpatched
> qemu too?

That is no regression, but a weakness which existed from the
beginning. Your patch neither makes it better nor makes it worse.

>
> ** Patch added: "caps-lock-key-up-event-v2.patch"
> https://bugs.launchpad.net/ubuntu/+source/qemu-kvm/+bug/427612/+attachment/1743919/+files/caps-lock-key-up-event-v2.patch

Just a small remark:
Inline patches (instead of links) are preferred on qemu-devel
because they make reading easier, and it's also easier for reviewers
to add comments.

Regards,
Stefan