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

[Benjamin Drung]

Am Donnerstag, den 25.11.2010, 21:08 +0100 schrieb Stefan Weil:
> 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).

Yes, that's better than not supporting older SDL versions.

> 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.

I don't know if it can really fixed for all keyboard layout
combinations. Imagine using two keyboard layouts that have the caps lock
key mapped on different keys. How do you want to keep the caps lock
synchronized?

> > ** 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.

You request a new feature for Launchpad, because I used Launchpad to
attach my patch. Look at https://launchpad.net/bugs/427612 . All
comments for bugs in the qemu-kvm Ubuntu package are forwarded to the
mailing list.

-- 
Benjamin Drung
Ubuntu Developer (www.ubuntu.com) | Debian Maintainer (www.debian.org)

signature.asc
Description: This is a digitally signed message part