Xen devel - Dec 2010 - [PATCH][qemu-xen]fix ''|'' key display problem in en-us with altgr processing

Changeset 7410 handles altgr-insert problem. Unfortunately, with that patch,
there is a problem in En-us keyboard: ''|'' (bar) cannot be
displayed. After checking keymap files, we found there are two definitions to
"bar" in en-us:
bar 0x56 altgr (in "common")
bar 0x2b shift (in "en-us")
First line is actually invalid in en-us lanuage. The 2nd definition will cover
the 1st one.

Patch in Changeset 7410 didn''t consider multi-definition case. It scans
keymap files, if keysym needs altgr, it will records that, after that, if keysym
is pressed but altgr not pressed, it will add an altgr press opeartion. It is
correct if all keysyms are unique and valid. But in the above multi-definition
case, there is problem: when reading
bar 0x56 altgr (in "common")
it will record altgr needed, but in fact, that definition won''t be
used, it always use the 2nd definition and won''t need altgr. Then if
the keysym is pressed, the code will still add an altgr press operation, that
will cause problem.

So, if we cannot avoid multi-definition in keymap files, the altgr flag (whether
altgr needed or not) should also be refreshed according to the 2nd defintion. In
the above case, when reading the 1st line, it records altgr needed; then reading
2nd line, 2nd definition will cover the 1st, meanwhile the altgr flag should be
reset (the 2nd definition doesn''t need altgr, so altgr flag should be
removed.)

Following patch supplements patch in Changeset 7410, solve the problem. Please
share your comments. Thanks!

Singed-off-by ChunyanLiu 

diff -r 1e5cb7d6a96c keymaps.c
--- a/keymaps.c    Mon Oct 18 17:24:50 2010 +0100
+++ b/keymaps.c    Wed Dec 22 23:56:50 2010 +0800
@@ -54,6 +54,20 @@
     struct key_range *altgr_range;
 } kbd_layout_t;
 
+static void del_key_range(struct key_range **krp, int code) {
+    struct key_range *kr;
+    struct key_range *kr_pr;
+    for (kr = *krp; kr; kr_pr = kr, kr = kr->next) {
+        if (code >= kr->start && code <= kr->end) {
+            if (kr == *krp)
+                *krp = kr->next;
+            else
+                kr_pr->next = kr->next;
+            qemu_free(kr);
+        }
+    }
+}
+
 static void add_to_key_range(struct key_range **krp, int code) {
     struct key_range *kr;
     for (kr = *krp; kr; kr = kr->next) {
@@ -138,6 +152,9 @@
             add_to_key_range(&k->altgr_range, keysym);
             //fprintf(stderr, "altgr keysym %04x keycode %d\n",
keysym, keycode);
             }
+            else {
+            del_key_range(&k->altgr_range, keysym);
+            }
     
             /* if(keycode&0x80)
                keycode=(keycode<<8)^0x80e0; */
diff -r 1e5cb7d6a96c vnc.c
--- a/vnc.c    Mon Oct 18 17:24:50 2010 +0100
+++ b/vnc.c    Wed Dec 22 23:56:50 2010 +0800
@@ -1279,11 +1279,9 @@
     kbd_put_keycode(0xe0);
     if (down){
         kbd_put_keycode(0xb8 & 0x7f);
-        vs->modifiers_state[0xb8] = 1;
     }
     else {
         kbd_put_keycode(0xb8 | 0x80);
-        vs->modifiers_state[0xb8] = 0;
     }
 }
 
 
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Chun Yan Liu writes ("[Xen-devel][PATCH][qemu-xen]fix ''|''
key display problem in en-us with altgr
processing"):
> Changeset 7410 handles altgr-insert problem.
Do you mean git commit f95d202ed6444dacb15fbea4dee185eb0e048d9a
"ioemu: fix VNC altgr-insert behavior" ?
> Unfortunately, with that patch, there is a problem in En-us keyboard:
''|'' (bar) cannot be displayed. After checking keymap files, we
found there are two definitions to "bar" in en-us:
Thanks for this analysis and the new patch.

I''m inclined to just take this patch unless someone has any
objections.

Note, however, that we intend during the next Xen release cycle (ie,
after 4.1 is released) to switch to a version of qemu based on
upstream qemu.  You should consider whether upstream qemu has the bugs
you are addressing and if it does, engage with the qemu-devel list to
get them fixed.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> Do you mean git commit f95d202ed6444dacb15fbea4dee185eb0e048d9a
> "ioemu: fix VNC altgr-insert behavior" ?
Yes.
>> Unfortunately, with that patch, there is a problem in En-us keyboard:
''|'' (bar) cannot be displayed. After checking keymap
>>files, we found there are two definitions to "bar" in en-us:
>Thanks for this analysis and the new patch.
>I''m inclined to just take this patch unless someone has any
>objections.
>Note, however, that we intend during the next Xen release cycle (ie,
>after 4.1 is released) to switch to a version of qemu based on
>upstream qemu.  You should consider whether upstream qemu has the bugs
>you are addressing and if it does, engage with the qemu-devel list to
>get them fixed.
Yes, I''ll check upstream qemu code.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Wed, 2010-12-22 at 07:51 +0000, Chun Yan Liu wrote:
> 
> Changeset 7410 handles altgr-insert problem. Unfortunately, with that
> patch, there is a problem in En-us keyboard: ''|'' (bar)
cannot be
> displayed. After checking keymap files, we found there are two
> definitions to "bar" in en-us:
> bar 0x56 altgr (in "common")
> bar 0x2b shift (in "en-us")
> First line is actually invalid in en-us lanuage. The 2nd definition
> will cover the 1st one.
> 
> Patch in Changeset 7410 didn''t consider multi-definition case. It
> scans keymap files, if keysym needs altgr, it will records that, after
> that, if keysym is pressed but altgr not pressed, it will add an altgr
> press opeartion. It is correct if all keysyms are unique and valid.
> But in the above multi-definition case, there is problem: when
> reading 
> bar 0x56 altgr (in "common")
> it will record altgr needed, but in fact, that definition won''t be
> used, it always use the 2nd definition and won''t need altgr. Then
if
> the keysym is pressed, the code will still add an altgr press
> operation, that will cause problem.
> 
> So, if we cannot avoid multi-definition in keymap files, the altgr
> flag (whether altgr needed or not) should also be refreshed according
> to the 2nd defintion. In the above case, when reading the 1st line, it
> records altgr needed; then reading 2nd line, 2nd definition will cover
> the 1st, meanwhile the altgr flag should be reset (the 2nd definition
> doesn''t need altgr, so altgr flag should be removed.)
> 
> Following patch supplements patch in Changeset 7410, solve the
> problem. Please share your comments. Thanks!
> 
> Singed-off-by ChunyanLiu <cyliu@novell.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

This fixes an issue I was seeing with the en-gb keymap too.

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Wed, 2011-01-05 at 12:55 +0000, Ian Campbell wrote:
> On Wed, 2010-12-22 at 07:51 +0000, Chun Yan Liu wrote:
> > Singed-off-by ChunyanLiu <cyliu@novell.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
Actually, should make that
Tested-by: Ian Campbell <ian.campbell@citrix.com>
since I''m not really qualified to review/ack the qemu stuff in any
detail.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Chun Yan Liu writes ("[Xen-devel][PATCH][qemu-xen]fix ''|''
key display problem in en-us with altgr
processing"):
> Changeset 7410 handles altgr-insert problem. Unfortunately, with that
patch, there is a problem in En-us keyboard: ''|'' (bar) cannot
be displayed. After checking keymap files, we found there are two definitions to
"bar" in en-us:
> bar 0x56 altgr (in "common")
> bar 0x2b shift (in "en-us")
> First line is actually invalid in en-us lanuage. The 2nd definition will
cover the 1st one.
Thanks for your contributions, I have applied your patch.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel