Re: [gtk-vnc-devel] gtk-vnc and AltGr

["Daniel P. Berrange" <berrange redhat com>]

On Tue, Nov 11, 2008 at 09:58:14PM +0000, Daniel P. Berrange wrote:
> On Tue, Nov 11, 2008 at 05:02:56PM -0300, Jonh Wendell wrote:
> > Em Sáb, 2008-05-10 às 09:53 -0300, Jonh Wendell escreveu:
> > > Em Sáb, 2008-04-19 às 22:33 +0200, Alexander Bürger escreveu:
> > > 
> > > People are telling that patch works for them:
> > > http://bugzilla.gnome.org/show_bug.cgi?id=499804
> > > https://bugs.launchpad.net/ubuntu/+source/vinagre/+bug/212013
> > > 
> > > So, can we just commit it?
> > > 
> > > Cheers,
> > 
> > Ping on this; people are still complaining about this issue.
> > 
> > I am one of the affected by this bug. Without this patch, I'm unable to
> > send keys with AltGR modifier. With it applied, everything worked for
> > me. I've made some tests and it worked fine. No regressions detected.
> 
> The patch from that bug is basically reverting to what we used to
> do along while back. The problem was that you can end up with stuck
> keys in the server depending on the order in which you released
> the keys. eg, when pressing Alt_l, we'd send 'Press Alt_L' and later
> when it was released, we'd send 'Release ISO_Prev_Group'. The result
> was a 'stuck' Alt key in the server. 
> 
> I'll have to do some more investigation to see if there's any way
> we can track these inconsistencies better, and guarentee to send
> the correct matching 'Release' event in all cases.

Since I first made this change, we've added code which tracks all key
press events, and their keycode & keyval. I can make use of this to
ensure the correct matching release events are always set. This I think
we can kill the entire gdk_keymap_translate_keyboard_state() call and
just use what's provided in the key event. This patch is a little large
but I think it'll be more reliable than either the current code or the
proposed patch from launchpad. Can you give this a try with any wierd
keyboards you have - particularly check that 'normal' things happen
when using modifiers keys - eg things like Ctrl-W, Alt-Tab, Shift-Alt-Tab,
etc.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

diff --git a/src/vncdisplay.c b/src/vncdisplay.c
--- a/src/vncdisplay.c
+++ b/src/vncdisplay.c
@@ -622,9 +622,7 @@ static gboolean key_event(GtkWidget *wid
 static gboolean key_event(GtkWidget *widget, GdkEventKey *key)
 {
 	VncDisplayPrivate *priv = VNC_DISPLAY(widget)->priv;
-	guint keyval;
-	gint group, level;
-	GdkModifierType consumed;
+	int i;
 
 	if (priv->gvnc == NULL || !gvnc_is_initialized(priv->gvnc))
 		return FALSE;
@@ -632,35 +630,12 @@ static gboolean key_event(GtkWidget *wid
 	if (priv->read_only)
 		return FALSE;
 
-	/*
-	 * Key handling in VNC is screwy. The event.keyval from GTK is
-	 * interpreted relative to modifier state. This really messes
-	 * up with VNC which has no concept of modifiers - it just sees
-	 * key up & down events - the remote end interprets modifiers
-	 * itself. So if we interpret at the client end you can end up
-	 * with 'Alt' key press generating Alt_L, and key release generating
-	 * ISO_Prev_Group. This really really confuses the VNC server
-	 * with 'Alt' getting stuck on.
-	 *
-	 * So we have to redo GTK's  keycode -> keyval translation
-	 * using only the SHIFT modifier which the RFB explicitly
-	 * requires to be interpreted at client end.
-	 *
-	 * Arggggh.
-	 */
-	gdk_keymap_translate_keyboard_state(gdk_keymap_get_default(),
-					    key->hardware_keycode,
-					    key->state & (GDK_SHIFT_MASK | GDK_LOCK_MASK),
-					    key->group,
-					    &keyval,
-					    &group,
-					    &level,
-					    &consumed);
-
-	keyval = x_keymap_get_keyval_from_keycode(key->hardware_keycode, keyval);
+	fprintf(stderr, "\n\n%s keycode: %d  state: %d  group %d, keyval: %d\n",
+		key->type == GDK_KEY_PRESS ? "press" : "release",
+		key->hardware_keycode, key->state, key->group, key->keyval);
 
 	/*
-	 * More VNC suckiness with key state & modifiers in particular
+	 * Some VNC suckiness with key state & modifiers in particular
 	 *
 	 * Because VNC has no concept of modifiers, we have to track what keys are
 	 * pressed and when the widget looses focus send fake key up events for all
@@ -676,48 +651,49 @@ static gboolean key_event(GtkWidget *wid
 	 *
 	 * Arggggh.
 	 */
+
+	/*
+	 * First the key release handling. This is *always* run, even for Key press
+	 * events, because GTK will often merge sequential press+release pairs of
+	 * the same key into a sequence of press+press+press+press+release. VNC
+	 * servers don't like this, so we have to see if we're already pressed
+	 * send release events. So, we run the release handling code all the time.
+	 */
+	for (i = 0 ; i < (int)(sizeof(priv->down_keyval)/sizeof(priv->down_keyval[0])) ; i++) {
+		/* We were pressed, and now we're released, so... */
+		if (priv->down_scancode[i] == key->hardware_keycode) {
+			/*
+			 * ..send the key release event we're dealing with
+			 *
+			 * NB, we use priv->down_keyval[i], and not our
+			 * current 'keyval', because we need to make sure
+			 * that the release keyval is identical to the
+			 * press keyval. In some layouts, this isn't always
+			 * true, with "Tab" generating Tab on press, and
+			 * ISO_Prev_Group on release.
+			 */
+			gvnc_key_event(priv->gvnc, 0, priv->down_keyval[i], key->hardware_keycode);
+			priv->down_keyval[i] = 0;
+			priv->down_scancode[i] = 0;
+			break;
+		}
+	}
+
 	if (key->type == GDK_KEY_PRESS) {
-		int i;
 		for (i = 0 ; i < (int)(sizeof(priv->down_keyval)/sizeof(priv->down_keyval[0])) ; i++) {
 			if (priv->down_scancode[i] == 0) {
-				priv->down_keyval[i] = keyval;
+				priv->down_keyval[i] = key->keyval;
 				priv->down_scancode[i] = key->hardware_keycode;
 				/* Send the actual key event we're dealing with */
-				gvnc_key_event(priv->gvnc, 1, keyval, key->hardware_keycode);
-				break;
-			} else if (priv->down_scancode[i] == key->hardware_keycode) {
-				/* Got an press when we're already pressed ! Why ... ?
-				 *
-				 * Well, GTK merges sequential press+release pairs of the same
-				 * key so instead of press+release,press+release,press+release
-				 * we only get press+press+press+press+press+release. This
-				 * really annoys some VNC servers, so we have to un-merge
-				 * them into a sensible stream of press+release pairs
-				 */
-				/* Fake an up event for the previous down event */
-				gvnc_key_event(priv->gvnc, 0, keyval, key->hardware_keycode);
-				/* Now send our actual ldown event */
-				gvnc_key_event(priv->gvnc, 1, keyval, key->hardware_keycode);
-				break;
-			}
-		}
-	} else {
-		int i;
-		for (i = 0 ; i < (int)(sizeof(priv->down_keyval)/sizeof(priv->down_keyval[0])) ; i++) {
-			/* We were pressed, and now we're released, so... */
-			if (priv->down_scancode[i] == key->hardware_keycode) {
-				priv->down_keyval[i] = 0;
-				priv->down_scancode[i] = 0;
-				/* ..send the key release event we're dealing with */
-				gvnc_key_event(priv->gvnc, 0, keyval, key->hardware_keycode);
+				gvnc_key_event(priv->gvnc, 1, key->keyval, key->hardware_keycode);
 				break;
 			}
 		}
 	}
 
 	if (key->type == GDK_KEY_PRESS &&
-	    ((keyval == GDK_Control_L && (key->state & GDK_MOD1_MASK)) ||
-	     (keyval == GDK_Alt_L && (key->state & GDK_CONTROL_MASK)))) {
+	    ((key->keyval == GDK_Control_L && (key->state & GDK_MOD1_MASK)) ||
+	     (key->keyval == GDK_Alt_L && (key->state & GDK_CONTROL_MASK)))) {
 		if (priv->in_pointer_grab)
 			do_pointer_ungrab(VNC_DISPLAY(widget), FALSE);
 		else if (!priv->grab_keyboard || !priv->absolute)