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

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

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.

>
>> +
>> +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.

>> +    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 !

>> 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

>
>>   };
>>
>>   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!

>
>>         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
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|