| Message ID | b0ae6fcc1002111313j56b57aa4tb90a7697d8877283@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Am 11.02.2010 22:13, schrieb Shahar Havivi: > Qemu have a hack for capslock that is not working with Ubuntu. > attached patch that fix it, as describe in this bug: > https://bugs.launchpad.net/qemu/+bug/427612 > > Signed-off-by: Shahar Havivi <shaharh@gmail.com> > > --- > sdl.c | 7 ++++--- > 1 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/sdl.c b/sdl.c > index cf27ad2..b3d5049 100644 > --- a/sdl.c > +++ b/sdl.c > @@ -390,9 +390,10 @@ static void sdl_process_key(SDL_KeyboardEvent *ev) > break; > case 0x45: /* num lock */ > case 0x3a: /* caps lock */ > - /* SDL does not send the key up event, so we generate it */ > - kbd_put_keycode(keycode); > - kbd_put_keycode(keycode | 0x80); > + if (ev->type == SDL_KEYUP) > + kbd_put_keycode(keycode | 0x80); > + else > + kbd_put_keycode(keycode); > return; > } > The previous code explicitly says the SDL doesn't send the key up event. If you think this is wrong generally, this definitely needs an explanation in the commit message, Also it could use an explanation of _why_ it doesn't work with Ubuntu - I assume they use either a newer or a patched SDL version which does generate these events? As you did not provide these explanations, I assume that this just happens to work for your specific Ubuntu version and is wrong for some other systems. What about always generating both keycodes as we currently do, but ignoring the event if ev->type == SDL_KEYUP? This should work with any SDL version. Kevin
It's not true that SDL is not sending up event like the comment say, On Fedora 12 it behave like a toggle button, first press/release will send caps-down event second press/release send caps-up event On Ubuntu 9.10 it work like any other key, i.e. pressing caps will generate two events down and up. Shahar. On Fri, Feb 12, 2010 at 11:54 AM, Kevin Wolf <kwolf@redhat.com> wrote: > Am 11.02.2010 22:13, schrieb Shahar Havivi: >> Qemu have a hack for capslock that is not working with Ubuntu. >> attached patch that fix it, as describe in this bug: >> https://bugs.launchpad.net/qemu/+bug/427612 >> >> Signed-off-by: Shahar Havivi <shaharh@gmail.com> >> >> --- >> sdl.c | 7 ++++--- >> 1 files changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/sdl.c b/sdl.c >> index cf27ad2..b3d5049 100644 >> --- a/sdl.c >> +++ b/sdl.c >> @@ -390,9 +390,10 @@ static void sdl_process_key(SDL_KeyboardEvent *ev) >> break; >> case 0x45: /* num lock */ >> case 0x3a: /* caps lock */ >> - /* SDL does not send the key up event, so we generate it */ >> - kbd_put_keycode(keycode); >> - kbd_put_keycode(keycode | 0x80); >> + if (ev->type == SDL_KEYUP) >> + kbd_put_keycode(keycode | 0x80); >> + else >> + kbd_put_keycode(keycode); >> return; >> } >> > > The previous code explicitly says the SDL doesn't send the key up event. > If you think this is wrong generally, this definitely needs an > explanation in the commit message, Also it could use an explanation of > _why_ it doesn't work with Ubuntu - I assume they use either a newer or > a patched SDL version which does generate these events? As you did not > provide these explanations, I assume that this just happens to work for > your specific Ubuntu version and is wrong for some other systems. > > What about always generating both keycodes as we currently do, but > ignoring the event if ev->type == SDL_KEYUP? This should work with any > SDL version. > > Kevin >
Am 12.02.2010 12:09, schrieb Shahar Havivi: > It's not true that SDL is not sending up event like the comment say, > > On Fedora 12 it behave like a toggle button, first press/release will send > caps-down event second press/release send caps-up event > > On Ubuntu 9.10 it work like any other key, i.e. pressing caps will generate two > events down and up. So I think we have this situation: Without patch: F12 works, Ubuntu 9.10 broken With your patch: F12 broken, Ubuntu 9.10 works I dont' consider fixing one system by breaking another one a solution... Do we know why this difference exists? Is there a way to check at runtime which mode SDL uses? Have you tested if num lock has the same behaviour or are you just assuming? Kevin
On 02/12/2010 12:09 PM, Shahar Havivi wrote: > It's not true that SDL is not sending up event like the comment say, > > On Fedora 12 it behave like a toggle button, first press/release will send > caps-down event second press/release send caps-up event > > On Ubuntu 9.10 it work like any other key, i.e. pressing caps will generate two > events down and up. True. To see why, start at http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=317010 and http://archive.ubuntu.com/ubuntu/pool/main/libs/libsdl1.2/libsdl1.2_1.2.13-4ubuntu4.diff.gz -- it's a nice code reading exercise, so I'll suggest a possible solution before pointing out the reason for this behavior. The solution would be to put hack after hack, i.e. something like this (untested): case 0x3a: /* caps lock */ /* SDL will usually send only 2 events instead of 4, so we generate the missing ones. However, on Debian/Ubuntu systems it may generate 4; in this case we have to discard the extra events. On Debian/Ubuntu ev->key.keysym.mod will always be zero, but for other systems we need the complicated condition below. */ if ((ev->key.keysym.mod & KMOD_CAPS) == (ev->type == SDL_KEYDOWN ? KMOD_CAPS : 0)) { kbd_put_keycode(keycode); kbd_put_keycode(keycode | 0x80); } return; case 0x45: /* num lock */ /* Same as above. */ if ((ev->key.keysym.mod & KMOD_NUM) == (ev->type == SDL_KEYDOWN ? KMOD_NUM : 0)) { kbd_put_keycode(keycode); kbd_put_keycode(keycode | 0x80); } return; Now, the solution of the riddle. The patch was correctly submitted as + SDL_UseLockKeys = getenv ("SDL_DISABLE_LOCK_KEYS") == NULL; ... + use_lock_keys = SDL_UseLockKeys; ... + if (!use_lock_keys) + break; (i.e. by default do not change anything) but the maintainer apparently morphed it into + SDL_UseLockKeys = getenv("SDL_DISABLE_LOCK_KEYS"); ... + use_lock_keys = ( SDL_UseLockKeys && *SDL_UseLockKeys ); ... + if ( ! use_lock_keys ) + break; which changed the default and the meaning of SDL_DISABLE_LOCK_KEYS. I initially thought about removing the caps lock/num lock hack altogether and add the following, however it would need SDL 1.2.14 because SDL_NO_LOCK_KEYS support was added exactly two months after 1.2.13 was released. :-( :-( /* There are two versions around of a Debian patch that changes the way Caps Lock and Num Lock are handled. The first version by default sends only one of the KeyDown/KeyUp events, unless SDL_DISABLE_LOCK_KEYS is present in the environment. The second version instead by default sends both events, unless SDL_DISABLE_LOCK_KEYS is present and not empty. This version is the most commonly found (and a totally braindead idea). Upstream instead supports SDL_NO_LOCK_KEYS which, if set to 1, will generate all four events---which is what we want. Luckily, there is a combination of environment variable that will satisfy all variant. */ putenv ("SDL_DISABLE_LOCK_KEYS", ""); putenv ("SDL_NO_LOCK_KEYS", "1"); Yes, I love Debian. Paolo
On 02/12/2010 06:39 AM, Paolo Bonzini wrote: > /* There are two versions around of a Debian patch that changes the > way Caps Lock and Num Lock are handled. The first version > by default sends only one of the KeyDown/KeyUp events, unless > SDL_DISABLE_LOCK_KEYS is present in the environment. The second > version instead by default sends both events, unless > SDL_DISABLE_LOCK_KEYS is present and not empty. This version > is the most commonly found (and a totally braindead idea). > > Upstream instead supports SDL_NO_LOCK_KEYS which, if set to 1, > will generate all four events---which is what we want. Luckily, > there is a combination of environment variable that will satisfy > all variant. */ > > putenv ("SDL_DISABLE_LOCK_KEYS", ""); > putenv ("SDL_NO_LOCK_KEYS", "1"); > > Yes, I love Debian. So basically, Debian carries a hacked version of SDL that changes the key press behaviour? That's a Debian/Ubuntu bug. Shame on them for changing the behaviour of a library API like that. Regards, Anthony Liguori > Paolo > >
On 02/12/2010 04:15 PM, Anthony Liguori wrote: > > So basically, Debian carries a hacked version of SDL that changes the > key press behaviour? Yes, the patch was submitted to not change the default but the maintainer thought he knew better. Or confused an == with a != more likely. > That's a Debian/Ubuntu bug. Shame on them for changing the behaviour of > a library API like that. Indeed. Maybe the Debian/Ubuntu maintainers for QEMU and KVM will read this thread and make a fuss. The only reason it is interesting, is that QEMU is actually going through efforts to get the same behavior that the Debian/Ubuntu change implements (get a press and a release event whenever you toggle Caps Lock). Paolo
On 02/12/2010 12:17 PM, Paolo Bonzini wrote: > On 02/12/2010 04:15 PM, Anthony Liguori wrote: >> >> So basically, Debian carries a hacked version of SDL that changes the >> key press behaviour? > > Yes, the patch was submitted to not change the default but the > maintainer thought he knew better. Or confused an == with a != more > likely. > >> That's a Debian/Ubuntu bug. Shame on them for changing the behaviour of >> a library API like that. > > Indeed. Maybe the Debian/Ubuntu maintainers for QEMU and KVM will > read this thread and make a fuss. I've already updated the bug report appropriately. Dustin, the ball's in your court :-) > The only reason it is interesting, is that QEMU is actually going > through efforts to get the same behavior that the Debian/Ubuntu change > implements (get a press and a release event whenever you toggle Caps > Lock). Yes, but the problem with > Paolo
On Fri, 2010-02-12 at 12:44 -0600, Anthony Liguori wrote: > On 02/12/2010 12:17 PM, Paolo Bonzini wrote: > > On 02/12/2010 04:15 PM, Anthony Liguori wrote: > >> > >> So basically, Debian carries a hacked version of SDL that changes the > >> key press behaviour? > > > > Yes, the patch was submitted to not change the default but the > > maintainer thought he knew better. Or confused an == with a != more > > likely. > > > >> That's a Debian/Ubuntu bug. Shame on them for changing the behaviour of > >> a library API like that. > > > > Indeed. Maybe the Debian/Ubuntu maintainers for QEMU and KVM will > > read this thread and make a fuss. > > I've already updated the bug report appropriately. Dustin, the ball's > in your court :-) I looked at the original Debian Bug, http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=317010 The libsdl1.2 package in Ubuntu is no longer carrying that patch, debian/patches/005_lock_keys.diff. So I don't think that's quite the cause of this. As for reproducing the bug, eventually, I was able to get my host and guest caps-lock keys out of sync, if I went back and forth, between guest and host, toggling caps-lock. What's the desired behavior here? I don't have much of an opinion (other than that the caps-lock key is a waste of valuable keyboard real estate, that I never actually use on purpose, only ever hitting it accidentally and causing problems :-) :-Dustin
On 02/12/2010 09:49 PM, Dustin Kirkland wrote: > I looked at the original Debian Bug, > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=317010 > > The libsdl1.2 package in Ubuntu is no longer carrying that patch, > debian/patches/005_lock_keys.diff. So I don't think that's quite the > cause of this. The bug was reported in Karmic, which still has the patch with s/005/205/. Paolo
diff --git a/sdl.c b/sdl.c index cf27ad2..b3d5049 100644 --- a/sdl.c +++ b/sdl.c @@ -390,9 +390,10 @@ static void sdl_process_key(SDL_KeyboardEvent *ev) break; case 0x45: /* num lock */ case 0x3a: /* caps lock */ - /* SDL does not send the key up event, so we generate it */ - kbd_put_keycode(keycode); - kbd_put_keycode(keycode | 0x80); + if (ev->type == SDL_KEYUP) + kbd_put_keycode(keycode | 0x80); + else + kbd_put_keycode(keycode); return; }
Qemu have a hack for capslock that is not working with Ubuntu. attached patch that fix it, as describe in this bug: https://bugs.launchpad.net/qemu/+bug/427612 Signed-off-by: Shahar Havivi <shaharh@gmail.com> --- sdl.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)