www.cendio.com
Bug 3074 - Fix XKB/XKEYBOARD support in Xvnc
: Fix XKB/XKEYBOARD support in Xvnc
Status: CLOSED FIXED
: ThinLinc
VNC
: trunk
: PC Unknown
: P2 Normal
: 4.1.0
Assigned To:
:
:
:
: 3523 4262 4417
  Show dependency treegraph
 
Reported: 2009-04-07 09:13 by
Modified: 2013-06-17 11:54 (History)


Attachments


Note

You need to log in before you can comment on or make changes to this bug.


Description From cendio 2009-04-07 09:13:18
Currently, Xvnc from TigerVNC does not correctly work with XKB/XKEYBOARD. Adam
has tried to fix this, but failed. We should perhaps eventually fix this and
start support XKB. See bug 2994.
------- Comment #1 From cendio 2009-04-07 10:30:18 -------
Track to 3.0.1.
------- Comment #3 From cendio 2009-06-03 09:30:54 -------
One advantage with XKB is that applications can distinguish between manual and
automatic key repeats. 
------- Comment #4 From cendio 2011-06-17 17:48:16 -------
Yet another massive breakage caused by people assuming XKB is always present.

One new bright idea the gnome devs had for gnome 3 was to give the key above
Tab special meaning, and XKB is therefore needed as it is the only system that
provides physical layout (although I seriously doubt it is maintained and
accurate).

For bonus points, the code blatantly assumed that Xkb calls always succeed and
crashes metacity on F15 when using our Xvnc (even with XKB turned on as it
probably lacks the layout information). It is fixed in upstream metacity, but
no backport to F15 yet. Upstream bug:

https://bugzilla.redhat.com/show_bug.cgi?id=701978

gnome-settings-daemon is also being very difficult in F15, probably also caused
by the lack of XKB.
------- Comment #5 From cendio 2012-06-07 09:04:15 -------
Tobias Oetiker reports that enabling XKB with +kb works good. Perhaps XKB
support in Xvnc isn't that broken after all.
------- Comment #6 From cendio 2012-11-20 16:18:20 -------
Fedora has been shipping a XKB Xvnc for years, so it isn't horribly broken. We
still need to go through things and make sure everything is okay. Adam probably
stopped once he had something that seemed to work.
------- Comment #7 From cendio 2013-03-20 15:59:33 -------
I've had a look at the code, and it unfortunately uses the compatibility
mappings rather than XKB directly. We already know these are buggy, so this
doesn't seem promising.

And a quick test with Fedora's Xvnc reveals problems; AltGr doesn't work at
all.

More work is definitely needed...
------- Comment #8 From cendio 2013-03-27 16:55:23 -------
As suspected, the approach Fedora has been using isn't viable. It works
somewhat with simple layouts, but anything more complex breaks horribly.

I've rewritten a new algorithm based on the principle that we attempt to toggle
Shift and ISO_Level3_Shift. This should cover most sensible layouts, but is
unfortunately not a generic solution.

There are still a lot of compatibility hacks that need to be ported over, as
well as a lot of testing that needs to be done.
------- Comment #9 From cendio 2013-04-09 18:41:06 -------
All the coding is done and everything is committed upstream as well as dumped
into our tree. Just need to do the final touches on the build system and this
will be part of the nightly builds.
------- Comment #10 From cendio 2013-04-11 13:35:18 -------
Moved these VNC related scripts into the vnc package. Tester should verify that
they still work:

 - tl-clipboard-helper
 - tl-set-title
 - tl-shadow-notify
------- Comment #11 From cendio 2013-04-11 13:36:01 -------
We should probably still accept the KeyboardMap argument, in order to support
old configurations. We can't really respect it though so we should print a
warning.
------- Comment #12 From cendio 2013-04-11 13:55:19 -------
Added a new script, tl-default-keyboard, that handles initial keyboard layout.
------- Comment #13 From cendio 2013-04-11 14:08:53 -------
(In reply to comment #11)
> We should probably still accept the KeyboardMap argument, in order to support
> old configurations. We can't really respect it though so we should print a
> warning.

Fixed in r27046.
------- Comment #14 From cendio 2013-04-15 12:36:44 -------
Everything is now in the nightly builds.

Tester needs to verify that keyboards work fully. Key points to test are:

 - Symbols not present in the remote layout (e.g. ä with a "us" layout)

 - That fake key presses work.

 - That fake key releases work for multiple keys. Turn on caps_lock, press both
shift keys and then 'a'. Remote side should then fake release both shift keys.

 - That alternative keysyms work. E.g. keypad keys can be replaced with their
non-keypad alternative if the proper one isn't in the keymap.

 - "Difficult" applications. E.g. wine, VirtualBox, VMware, rdesktop, freerdp.

 - Lock keys shouldn't make it through.

 - Autorepeat. There are two issues that need to be verified here: 1) server
side autorepeat should be ignored, 2) applications that call
XkbSetDetectableAutoRepeat() should only see KeyPress on repeat, no
KeyRelease:s. Do a modified build of xev for that test.

Also browse through bug 3523 for inspiration on older problems that we don't
want to resurface.
------- Comment #15 From cendio 2013-04-15 16:49:57 -------
Bah Windows! Something is off with their "emulate AltGr using Ctrl+Alt" stuff.
I'm getting the symbols expected for a Shift modifier instead of AltGr. E.g.
"=" instead of "}".
------- Comment #16 From cendio 2013-04-15 17:23:04 -------
(In reply to comment #15)
> Bah Windows! Something is off with their "emulate AltGr using Ctrl+Alt" stuff.
> I'm getting the symbols expected for a Shift modifier instead of AltGr. E.g.
> "=" instead of "}".

Brown paper bag. Copy/paste error that resulted in shift being pressed even
when we wanted AltGr. Fixed in r27082.
------- Comment #17 From cendio 2013-04-16 09:55:41 -------
The Shift+Tab mess is rearing its ugly head again. Need to have another look at
it.
------- Comment #18 From cendio 2013-04-16 11:05:56 -------
(In reply to comment #17)
> The Shift+Tab mess is rearing its ugly head again. Need to have another look at
> it.

Fixed in r27086.
------- Comment #19 From cendio 2013-05-20 14:51:12 -------
(In reply to comment #10)
> Moved these VNC related scripts into the vnc package. Tester should verify that
> they still work:
> 
>  - tl-clipboard-helper
>  - tl-set-title
>  - tl-shadow-notify

The first two works, but tl-shadow-notify is broken:

$ tl-shadow-notify 
$ execvp: No such file or directory
------- Comment #20 From cendio 2013-05-21 09:57:09 -------
(In reply to comment #19)
> 
> The first two works, but tl-shadow-notify is broken:
> 
> $ tl-shadow-notify 
> $ execvp: No such file or directory

Fixed in r27418.
------- Comment #21 From cendio 2013-05-21 12:43:18 -------
I patched "numlockx" to be able to set CapsLock instead of NumLock. This causes
the Xserver to crash. The binary is here:

/home/astrand/tmp/numlockx-1.2-capslock/numlockx

I executed it as "numlockx on". In xinit.log, I got:

The XKEYBOARD keymap compiler (xkbcomp) reports:
> Warning:          Compat map for group 2 redefined
>                   Using new definition
> Warning:          Compat map for group 3 redefined
>                   Using new definition
> Warning:          Compat map for group 4 redefined
>                   Using new definition
Errors from xkbcomp are not fatal to the X server
(EE) 
(EE) Backtrace:
(EE) 0: /opt/thinlinc/libexec/Xvnc (xorg_backtrace+0x36) [0x5c7716]
(EE) 1: /opt/thinlinc/libexec/Xvnc (0x400000+0x1cb609) [0x5cb609]
(EE) 2: /lib64/libpthread.so.0 (0x3c23e00000+0xf500) [0x3c23e0f500]
(EE) 3: /opt/thinlinc/libexec/Xvnc (XkbConvertCase+0x2) [0x523f62]
(EE) 4: /opt/thinlinc/libexec/Xvnc
(_ZN11InputDevice15keysymToKeycodeEjjPj+0x1ec) [0x5e8efc]
(EE) 5: /opt/thinlinc/libexec/Xvnc (_ZN11InputDevice8keyEventEjb+0x141)
[0x5e7fb1]
(EE) 6: /opt/thinlinc/libexec/Xvnc
(_ZN3rfb16VNCSConnectionST8keyEventEjb+0x124) [0x60dd64]
(EE) 7: /opt/thinlinc/libexec/Xvnc
(_ZN3rfb16VNCSConnectionST15processMessagesEv+0x87) [0x60cc87]
(EE) 8: /opt/thinlinc/libexec/Xvnc
(_ZN14XserverDesktop13wakeupHandlerEP6fd_seti+0xe8) [0x5e48d8]
(EE) 9: /opt/thinlinc/libexec/Xvnc (0x400000+0x1dab64) [0x5dab64]
(EE) 10: /opt/thinlinc/libexec/Xvnc (WakeupHandler+0x5b) [0x5682cb]
(EE) 11: /opt/thinlinc/libexec/Xvnc (WaitForSomething+0x4b6) [0x5c4496]
(EE) 12: /opt/thinlinc/libexec/Xvnc (Dispatch+0xb2) [0x563762]
(EE) 13: /opt/thinlinc/libexec/Xvnc (main+0x3da) [0x55112a]
(EE) 14: /lib64/libc.so.6 (__libc_start_main+0xfd) [0x3c2361ecdd]
(EE) 15: /opt/thinlinc/libexec/Xvnc (0x400000+0x58ee9) [0x458ee9]
(EE) 
(EE) Segmentation fault at address 0x0

Strangely enough, Xvnc did not crash right away, but after a few seconds.
------- Comment #22 From cendio 2013-05-22 16:40:06 -------
(In reply to comment #21)
> I patched "numlockx" to be able to set CapsLock instead of NumLock. This causes
> the Xserver to crash. The binary is here:
...
> 
> Strangely enough, Xvnc did not crash right away, but after a few seconds.

It happens on the next keypress actually and is because I screwed up the code
that handles lookups with CapsLock active.

It did expose two other bugs though:

 1. With caps lock active it fails to look up a candidate keycode properly

 2. We cannot use the ONE_LEVEL keytype for new keysyms if those keysyms are
affected by CapsLock.
------- Comment #23 From cendio 2013-05-23 13:56:22 -------
(In reply to comment #22)
> (In reply to comment #21)
> > I patched "numlockx" to be able to set CapsLock instead of NumLock. This causes
> > the Xserver to crash. The binary is here:
> ...
> > 
> > Strangely enough, Xvnc did not crash right away, but after a few seconds.
> 
> It happens on the next keypress actually and is because I screwed up the code
> that handles lookups with CapsLock active.
> 
> It did expose two other bugs though:
> 
>  1. With caps lock active it fails to look up a candidate keycode properly
> 
>  2. We cannot use the ONE_LEVEL keytype for new keysyms if those keysyms are
> affected by CapsLock.

All three things fixed in r27439.
------- Comment #24 From cendio 2013-05-27 11:08:49 -------
Now tested:

> - tl-clipboard-helper
> - tl-set-title
> - tl-shadow-notify

Works. 


>We should probably still accept the KeyboardMap argument, in order to support
>old configurations. We can't really respect it though so we should print a
>warning.

Works. 


>Added a new script, tl-default-keyboard, that handles initial keyboard layout.

Works. Note that this must be tested with ie xterm, because GNOME sets the
layout as well. 

More results will follow.
------- Comment #25 From cendio 2013-05-27 11:41:26 -------
> - Symbols not present in the remote layout (e.g. ä with a "us" layout)

Works. 

> - That fake key presses work.

Works. Tested with EuroSign sent without modifiers, caused ISO_Level3_Shift to
be faked. Tested with "percent", caused shift to be faked. 

> - That fake key releases work for multiple keys. Turn on caps_lock, press both
>shift keys and then 'a'. Remote side should then fake release both shift keys.

Works. 

> - That alternative keysyms work. E.g. keypad keys can be replaced with their
>non-keypad alternative if the proper one isn't in the keymap.

Works. Removed KP_Down from keymap with xmodmap, sent KP_Down, got Down. 

> - "Difficult" applications. E.g. wine, VirtualBox, VMware, rdesktop, freerdp.

Wine: Tested in sndrec32 file dialog. Everything works except numpad keys with
NumLock. Seems to be this bug: http://bugs.winehq.org/show_bug.cgi?id=33414 .
Probably caused by the fact that we are generating KP_4 etc by fake shifting,
instead of actually toggling numlock.
------- Comment #26 From cendio 2013-05-27 13:29:02 -------
Tested VirtualBox, using gparted live CD. Surprised to see that VirtualBox
works inside VMware. Even more surprised to see that the keyboard worked
perfectly fine. This was with Swedish layout on the client side, Swedish in the
TL session (set by GNOME), Swedish selected in the VM.
------- Comment #27 From cendio 2013-05-27 13:49:31 -------
Tested rdesktop. Mostly works, except: You cannot mark text with Shift+arrows
on the numeric keyboard. This is a regression: Earlier we were not including
KP_Down etc, to avoid this problem. This should also avoid the Wine problem
reported earlier. Reopening for discussion.
------- Comment #28 From cendio 2013-05-27 14:06:02 -------
> - Lock keys shouldn't make it through.

Scroll_Lock makes it, but I think that's OK. 


> - Autorepeat. There are two issues that need to be verified here: 1) server
>side autorepeat should be ignored, 2) applications that call
>XkbSetDetectableAutoRepeat() should only see KeyPress on repeat, no
>KeyRelease:s. Do a modified build of xev for that test.

Works great. Was using this patch:

--- a/xev.c
+++ b/xev.c
@@ -1091,6 +1091,15 @@ main (int argc, char **argv)
                 ProgramName, XDisplayName (displayname));
        exit (1);
     }
+    
+#include <X11/XKBlib.h>
+    if (1) {
+       Bool ret, supported;
+       ret = XkbSetDetectableAutoRepeat(dpy, True, &supported);
+       if (!ret) {
+           fprintf(stderr, "XkbSetDetectableAutoRepeat failed\n");
+       }
+    }


I've not tested VMware nor FreeRDP, but I'd say that the new code works very
well. Need to consider the KP arrow keys, but I think that's it.
------- Comment #29 From cendio 2013-05-30 11:32:30 -------
(In reply to comment #25)
> 
> Wine: Tested in sndrec32 file dialog. Everything works except numpad keys with
> NumLock. Seems to be this bug: http://bugs.winehq.org/show_bug.cgi?id=33414 .
> Probably caused by the fact that we are generating KP_4 etc by fake shifting,
> instead of actually toggling numlock.

We've discussed this and the proper fix would have to be syncing num lock (bug
400). Unfortunately it is not something we can fix now. We also cannot fully
ignore applications that mishandle the fake shifts, so we need some temporary
hack.
------- Comment #30 From cendio 2013-05-30 16:58:06 -------
(In reply to comment #29)
> 
> We've discussed this and the proper fix would have to be syncing num lock (bug
> 400). Unfortunately it is not something we can fix now. We also cannot fully
> ignore applications that mishandle the fake shifts, so we need some temporary
> hack.

Fixed in r27470.

Tester needs to verify correct behaviour both with NumLock on and off in the
server.

Alternative keysyms also needs to be retested as it was changed as part of
this.
------- Comment #31 From cendio 2013-06-03 14:57:38 -------
Tested a lot of applications, including:

* gedit

* rdesktop

* wine. Note that you'll need a Swedish layout on the server side for correct
behaviour; wine tries to detect the layout this way. 

I did find bug 4677, but this is a client side problem. 

Besides this, everything works beautifully!
------- Comment #32 From cendio 2013-06-12 14:09:10 -------
Unfortunately the moving of tl-clipboard-helper to the vnc-server package from
tlmisc in r27041 breaks upgrades, at least on .deb systems. From
thinlinc-install.log:

2013-06-12 04:24:09,187: Warning during installation of
'/home/aaron/tl-4.1.0-server/serverkit-linux/thinlinc-vnc-server_4.1.0-3974_i386.deb':
" trying to overwrite '/opt/thinlinc/libexec/tl-clipboard-helper', which is
also in package thinlinc-tlmisc 4.0.0-3717"

This results in a strange package state after upgrade, where
thinlinc-vnc-server is still at v4.0.0, but other packages have been upgraded.
------- Comment #33 From cendio 2013-06-13 10:52:19 -------
(In reply to comment #32)
> Unfortunately the moving of tl-clipboard-helper to the vnc-server package from
> tlmisc in r27041 breaks upgrades, at least on .deb systems. From
> thinlinc-install.log:
> 
> 2013-06-12 04:24:09,187: Warning during installation of
> '/home/aaron/tl-4.1.0-server/serverkit-linux/thinlinc-vnc-server_4.1.0-3974_i386.deb':
> " trying to overwrite '/opt/thinlinc/libexec/tl-clipboard-helper', which is
> also in package thinlinc-tlmisc 4.0.0-3717"
> 
> This results in a strange package state after upgrade, where
> thinlinc-vnc-server is still at v4.0.0, but other packages have been upgraded.

Bah. dpkg is apparently not very bright. It doesn't consider the entire
transaction it is currently performing and doesn't realise that there will be
no conflict once everything is upgraded. There are some details here:

http://raphaelhertzog.com/2011/08/01/understanding-dpkgs-file-overwrite-error/

The "correct" solution according to that page is to add a "Replaces" whenever
you move a file. This however is very sub-optimal as we do not want the full
behaviour of Replaces. E.g. we do not want tlmisc to be removed just because
someone installed a hotfix of vnc-server.

So we're going with the alternative approach, to tell dpkg to ignore these
conflicts. There is a slight risk we'll overwrite something we shouldn't, but
that's unfortunately something we'll have to live with.
------- Comment #34 From cendio 2013-06-13 11:01:02 -------
Fixed in r27516.
------- Comment #35 From cendio 2013-06-17 11:54:02 -------
(In reply to comment #34)
> Fixed in r27516.

Testing will be done on the other "test install/upgrade" test bugs. Testing of
the XKB stuff has already been done.