Re: PATCH: Add configurable key grab sequences (v3)

[Michal Novotny <minovotn redhat com>]

On 06/18/2010 01:53 PM, Daniel P. Berrange wrote:
> On Fri, Jun 18, 2010 at 01:35:46PM +0200, Michal Novotny wrote:
>    
> > > @@ -216,6 +226,145 @@ static void do_scaling(GtkWidget *menu, GtkWidget *vncdisplay)
> > >    		vnc_display_set_scaling(VNC_DISPLAY(vncdisplay), FALSE);
> > >    }
> > >
> > > +static void dialog_update_keysyms(GtkWidget *window, guint *keysyms, guint numsyms)
> > > +{
> > > +	gchar *keys;
> > > +	int i;
> > > +
> > > +	keys = g_strdup("");
> > > +	for (i = 0; i<   numsyms; i++)
> > > +	{
> > > +		keys = g_strdup_printf("%s%s%s", keys,
> > > +			(strlen(keys)>   0) ? "+" : " ", gdk_keyval_name(keysyms[i]));
> > > +	}
> > > +
> > > +	gtk_label_set_text( GTK_LABEL(window), keys);
> > > +}
> > > +
> > > +static gboolean dialog_key_ignore(int keyval)
> > > +{
> > > +	switch (keyval) {
> > > +		case GDK_Return:
> > > +		case GDK_Escape:
> > > +			return TRUE;
> > > +	}
> > > +
> > > +	return FALSE;
> > > +}
> > >
> > >        
> > Maybe we may define some other in the future. However the main reason
> > for this is that usually when starting Fedora (11) that I used for
> > testing I've noticed that when the sequence is being set as Ctrl + Enter
> > it grabs the enter and boots the entry in the Grub bootloader. For Ctrl
> > + Esc it returns to the main Grub bootloader menu when editing the grub
> > boot commands.
> >      
> These examples/gvncviewer.{c,py} files are just examples of usage, so I
> don't think we need to worry too much about getting things perfect in
> their code. They just show people how todo interesting things with GTK-VNC
> code.
>    

You're right about this Daniel. But although examples could be 
implemented well to show the programmers using GTK-VNC how to implement 
it correctly for their purposes. But like you say, not a big deal and 
the application itself have to be programmed the right way, we don't 
need to worry that much about the examples, that's right.

>    
> >      
> > > +
> > > +static gboolean dialog_key_press(GtkWidget *window G_GNUC_UNUSED,
> > > +        GdkEvent *ev, VncGrabDefs *defs)
> > > +{
> > > +	gboolean keySymExists;
> > > +	int i;
> > > +
> > > +	if (dialog_key_ignore(ev->key.keyval))
> > > +		return FALSE;
> > > +
> > > +	if (defs->set&&   defs->curkeys)
> > > +		return FALSE;
> > > +
> > > +	/* Check whether we already have keysym in array - i.e. it was handler by something else */
> > > +	keySymExists = FALSE;
> > > +	for (i = 0; i<   defs->curkeys; i++) {
> > > +		if (defs->keysyms[i] == ev->key.keyval)
> > > +			keySymExists = TRUE;
> > > +	}
> > > +
> > > +	if (!keySymExists) {
> > > +		defs->keysyms = g_renew(guint, defs->keysyms, defs->curkeys + 1);
> > > +		defs->keysyms[defs->curkeys] = ev->key.keyval;
> > > +		defs->curkeys++;
> > > +	}
> > > +
> > > +	dialog_update_keysyms(defs->label, defs->keysyms, defs->curkeys);
> > > +
> > > +	if (!ev->key.is_modifier) {
> > > +		defs->set = TRUE;
> > > +		defs->numkeys = defs->curkeys;
> > > +		defs->curkeys--;
> > > +	}
> > > +
> > > +	return FALSE;
> > > +}
> > > +
> > >
> > >        
> > Just a note about this one since this is my original code: This expects
> > user to press modifiers and end it with some non-modifier key. Like Ctrl
> > + Alt + G for example. It doesn't accept Ctrl + Alt only since both the
> > keys are modifiers. Nevertheless, since we have a good version of the
> > patch now we can play with this when having time for that so we can
> > watch for the curkeys counter count and if the value decremented this
> > means one key has been released and therefore the sequence with this key
> > could be the one to be set, e.g. press and release Ctrl + Alt and this
> > will be registered/triggered:
> >
> > 1. press Ctrl (curkeys = 0 + 1 =>  1)
> > 2. press Alt  (curkeys = 1 + 1 =>  2)
> > 3. release Ctrl (curkeys = 2 -1 =>  1)
> > 4. curkeys number decremented, trigger
> >     4.1. save the remaining key in the sequence (Alt)
> >     4.2. save the key that's been released now (Ctrl)
> > 5. save the sequence to some list and use it for vnc_set_grab_keys()
> >
> > What do you think about this Daniel?
> >      
> It took me a while to realize that it required you to enter at least
> one non-modifier key, but I think this is absolutely fine for the
> example program. Something like Vinagre would want to make it more
> flexible to allow any key sequence.
>
>    

That's right. The thing I wanted to was to demonstrate how it could be 
done the widely usable way, i.e. including the modifier-only sequence. 
It was Ctrl + Alt before that so both Vinagre and Virt-manager should be 
having the option to define such a sequence and it should be predefined 
as Ctrl + Alt to preserve the old behaviour which is being done in your 
version of this patch as I can see:

+	priv->vncgrabseq = vnc_grab_sequence_new_from_string("Control_L+Alt_L");


but wouldn't it be better to define the sequence using the VncGrabSeq 
instead of string? Maybe it would be faster a bit but just a minor thing 
since this is only on GTK-VNC initialization and it's not being called 
often. But the point is that it's good to have it predefined to Ctrl + 
Alt but C example (there is no implementation of key recording in python 
example AFAIK) not allowing such a sequence seems to be rather confusing.

But it's just an example, we don't have to worry about and the 
programmers will find this out when exploring GTK-VNC functionality :)

> > > +    keystr = None
> > > +    for k in keys:
> > > +        if keystr is None:
> > > +            keystr = gtk.gdk.keyval_name(k)
> > > +        else:
> > > +            keystr = keystr + "+" + gtk.gdk.keyval_name(k)
> > >        if grabbed:
> > > -        subtitle = "(Press Ctrl+Alt to release pointer) "
> > > +        subtitle = "(Press %s to release pointer) " % keystr
> > >        else:
> > >            subtitle = ""
> > >
> > > @@ -173,6 +180,11 @@ layout.add(vnc)
> > >    vnc.realize()
> > >    vnc.set_pointer_grab(True)
> > >    vnc.set_keyboard_grab(True)
> > > +
> > > +# Example to change grab key combination to Ctrl+Alt+g
> > > +grab_keys = [ gtk.keysyms.Control_L, gtk.keysyms.Alt_L, gtk.keysyms.g ]
> > >
> > >        
> > Oh. Thanks. This is what I wrote above that I've been looking for. It's
> > not being defined as "gtk.gdk.Alt_L" but like "gtk.keysyms.Alt_L"
> > instead. Thanks for this code. It may be useful to know that some time.
> >      
> It took me alot of googling to fine out that they were  under
> gtk.keysyms instead of gtk.gdk !
>    

Yeah, in fact I was thinking about this originally as well but finally I 
decided to use string list definition. But it's OK to have it this way 
and I think this is better after all.

>    
> > > diff --git a/src/vncdisplay.c b/src/vncdisplay.c
> > > index 71e158b..8eeb243 100644
> > > --- a/src/vncdisplay.c
> > > +++ b/src/vncdisplay.c
> > > @@ -85,6 +85,9 @@ struct _VncDisplayPrivate
> > >
> > >    	GSList *preferable_auths;
> > >    	const guint8 const *keycode_map;
> > > +
> > > +	VncGrabSequence *vncgrabseq; /* the configured key sequence */
> > > +	gboolean *vncactiveseq; /* the currently pressed keys */
> > >
> > >        
> > Ok, you did rename it to vncactiveseq. That's good. But what is this
> > gboolean? Shouldn't this be the currently active sequence defined by
> > keys pressed and numkeys/curkeys to identify number of keys being
> > currently pressed?
> >      
> I realized that with the two VncGrabSequence objects, we need to
> iterate over both arrays, to detect a key grab sequence, since
> keys can be pressed in any order. By changing to a boolean we
> can just track up/down state directly and not worry about ordering
>
>    

Oh, right. When I was reading the code I saw what you mean and it makes 
the perfect sense.

> >      
> > >    };
> > >
> > >    G_DEFINE_TYPE(VncDisplay, vnc_display, GTK_TYPE_DRAWING_AREA)
> > > @@ -104,7 +107,8 @@ enum
> > >      PROP_SCALING,
> > >      PROP_SHARED_FLAG,
> > >      PROP_FORCE_SIZE,
> > > -  PROP_DEPTH
> > > +  PROP_DEPTH,
> > > +  PROP_GRAB_KEYS,
> > >    };
> > >
> > >    /* Signals */
> > > @@ -198,6 +202,9 @@ vnc_display_get_property (GObject    *object,
> > >          case PROP_DEPTH:
> > >            g_value_set_enum (value, vnc->priv->depth);
> > >    	break;
> > > +      case PROP_GRAB_KEYS:
> > > +	g_value_set_boxed(value, vnc->priv->vncgrabseq);
> > > +	break;
> > >
> > >        
> > Honestly I don't know what does this one do. Could you please explain?
> >
> >      
> > >          default:
> > >    	G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
> > >    	break;
> > > @@ -241,6 +248,9 @@ vnc_display_set_property (GObject      *object,
> > >          case PROP_DEPTH:
> > >            vnc_display_set_depth (vnc, g_value_get_enum (value));
> > >            break;
> > > +      case PROP_GRAB_KEYS:
> > > +	vnc_display_set_grab_keys(vnc, g_value_get_boxed(value));
> > > +	break;
> > >
> > >        
> > Same as above.
> >      
> Properties are a feature that is nice for non-C languages using
> glib. It lets people write bindings to enable code like
>
>       vnc.grab_keys = [ gtk.keysyms.Control_L ]
>
> instead of having to invoke
>
>       vnc.set_grab_keys([ gtk.keysyms.Control_L ])
>
> functionally both end up doing the same thing though!
>
>    

Ok, good. I didn't know that. Good to know and it will be useful IMHO. 
Thanks for explanation and implementation!

> >      
> > >          default:
> > >            G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
> > >            break;
> > > @@ -673,6 +683,37 @@ static gboolean motion_event(GtkWidget *widget, GdkEventMotion *motion)
> > >    	return TRUE;
> > >    }
> > >
> > > +
> > > +static gboolean check_for_grab_key(GtkWidget *widget, int type, int keyval)
> > > +{
> > > +	VncDisplayPrivate *priv = VNC_DISPLAY(widget)->priv;
> > > +	int i;
> > > +
> > > +	if (!priv->vncgrabseq->nkeysyms)
> > > +		return FALSE;
> > > +
> > > +	if (type == GDK_KEY_RELEASE) {
> > > +		/* Any key release resets the whole grab sequence */
> > > +		memset(priv->vncactiveseq, 0,
> > > +		       sizeof(gboolean)*priv->vncgrabseq->nkeysyms);
> > >
> > >        
> > Resets the whole grab sequence? Shouldn't it remove only the keysym
> > that's currently being pressed? But this is OK. This should work as well.
> >      
> The problem I saw was that if you did a sequence of
>
>    Press(ctrl)
>    Press(f)
>    Release(f)
>    Press(alt)
>
> then it would trigger the Ctrl-Alt grab sequence. IMHO it should
> only trigger the grab sequence if you immediately press the
> required sequence with no other key press/release in between
> eg only allow
>
>    Press(ctrl)
>    Press(alt)
>
> or
>
>    Press(alt)
>    Press(ctrl)
>
>
> Thanks for your additional review !
>
> Daniel
>    

Ok, good. Maybe there were some issues with that and this implementation 
is good.

Did you do the testing for various key sequences using the patch 
applied? Like "Ctrl + Alt", "Ctrl + Tab", "Alt + Tab" or with 
non-modifier key defined like "Ctrl + Alt + g" or "Ctrl + Alt + G" ? How 
is this solved with the uppercase and lowercase letters now? When you 
have Caps Lock off and you have to press Ctrl + Alt + G you would press 
Ctrl + Alt + Shift + g which would result into 4 keycodes instead of 3, 
right? So the Caps Lock have to be in the right state for that, right?

Michal

--
Michal Novotny<minovotn redhat com>, RHCE
Virtualization Team (xen userspace), Red Hat