GNOME.org
 

Re: [PATCH] Implement custom grab key combination definition

On Fri, Jun 04, 2010 at 11:17:31AM +0200, Michal Novotny wrote:
> Hi all,
> I'm working in virtualization and I'm using virt-manager and virt-viewer  
> tools to see the domain's VNC window where the grab key is defined to be  
> "Ctrl + Alt". Since this configuration is not that good in some cases  
> (since "Ctrl+Alt" could be defined for anything else in the window  
> manager) this patch allows you to set the grab key combination in the  
> running GTK-VNC application. I did work on this one in my spare time  
> since I think this is a useful feature to be implemented. At least I've  
> learned how to use GTK in C language if you won't apply the patch.

Thanks for working on this - its been a long standing TODO item

>
> Technically 2 new functions have been added to do the job,  
> vnc_display_set_grab_keys() and vnc_display_get_grab_keys() with the old  
> definition to "Ctrl+Alt" preserved by default.
>
> Please note that the gvncviewer.c and gvncviewer.py codes have been  
> changed to use both of the functions and the definition can be modifiers  
> only, or now even modifiers in combination with some key, i.e. something  
> like: "Ctrl+Alt+[F]" or "Ctrl+Alt+[F|f]". The brackets are important  
> since they specify the character or set of 2 characters to be used with  
> the modifiers combination to trigger the grab and they have to be at the  
> end of the expression. When 2 characters, delimited by logical "or" (|)  
> are there, it means that the expression is being matched by either of  
> them therefore you can specify both uppercase and lowercase letters here  
> to the expression or make possibility of 2 different keys with the  
> expression like "Ctrl+Alt+[a|b]" to match both 'a' and 'b' keys with the  
> Control and Alt modifiers pressed.

Do we really need to have the ability to match multiple different
key sequences ? I don't see why you'd need both Ctrl+Alt+a and 
Ctrl+Alt+b to be release sequences. Likewise I don't see why we'd
need to cope with upper and lowercase matching of letters - why
would the user go to the bother of holding Ctrl+Alt+Shift+f (to get
the uppercase F) when just Ctrl+Alt+f is already a release sequence.

> commit 4f39be84f6adf55896ea3bb5cbdc81dee075bb8e
> Author: Michal Novotny <minovotn redhat com>
> Date:   Fri Jun 4 01:43:50 2010 +0200
> 
>     Implement custom grab key combination definition
>     
>     This is the patch to implement custom grab key combination definition
>     support. The new functions {get|set}_grab_keys has been introduced in
>     this patch. Definition can be done either for modifier keys (e.g.
>     "Ctrl + Alt") or some key could be added to the modifiers, e.g.
>     "Ctrl + Alt + [F]". Please note the brackets in the definition which
>     is necessary to specify a character or an alternative character using
>     "Ctrl + Alt + [F|f]" which allows you to define both uppercase and
>     lowercase letters or two printable letters to grab the VNC window.
>     
>     Please note that only the printable letters can be put into those
>     brackets and no function keys [F1 - F12] or any others yet.
>     
>     Signed-off-by: Michal Novotny <minovotn redhat com>
> 
> diff --git a/examples/gvncviewer.c b/examples/gvncviewer.c
> index c3fbd74..058ea77 100644
> --- a/examples/gvncviewer.c
> +++ b/examples/gvncviewer.c
> @@ -48,12 +48,13 @@ static void set_title(VncDisplay *vncdisplay, GtkWidget *window,
>  {
>  	const char *name;
>  	char title[1024];
> -	const char *subtitle;
> +	char subtitle[512];
>  
>  	if (grabbed)
> -		subtitle = "(Press Ctrl+Alt to release pointer) ";
> +		snprintf(subtitle, 512, "(Press %s to release pointer) ",
> +                     vnc_display_get_grab_keys(VNC_DISPLAY(vncdisplay)));
>  	else
> -		subtitle = "";
> +		memset(subtitle, 0, 512);

Use of snprintf() is generally a bad idea because its fragile with
fixed length buffers. GLib provides a handy alternative that allocs
enough memory:

  g_strdup_printf("(Press %s to release pointer) ",
                   vnc_display_get_grab_keys(VNC_DISPLAY(vncdisplay)));


>  
>  	name = vnc_display_get_name(VNC_DISPLAY(vncdisplay));
>  	snprintf(title, sizeof(title), "%s%s - GVncViewer",
> @@ -467,6 +468,8 @@ int main(int argc, char **argv)
>  	vnc_display_set_keyboard_grab(VNC_DISPLAY(vnc), TRUE);
>  	vnc_display_set_pointer_grab(VNC_DISPLAY(vnc), TRUE);
>  
> +        vnc_display_set_grab_keys(VNC_DISPLAY(vnc), "Ctrl+Alt+[F|f]");
> +

There's a indentation bug there.

>  	if (!gtk_widget_is_composited(window)) {
>  		vnc_display_set_scaling(VNC_DISPLAY(vnc), TRUE);
>  		gtk_check_menu_item_set_active(GTK_CHECK_MENU_ITEM(scaling), TRUE);
> diff --git a/examples/gvncviewer.py b/examples/gvncviewer.py
> old mode 100644
> new mode 100755
> index 9a74268..0572309
> --- a/examples/gvncviewer.py
> +++ b/examples/gvncviewer.py
> @@ -29,7 +29,7 @@ if len(sys.argv) != 2 and len(sys.argv) != 3:
>  def set_title(vnc, window, grabbed):
>      name = vnc.get_name()
>      if grabbed:
> -        subtitle = "(Press Ctrl+Alt to release pointer) "
> +        subtitle = "(Press %s to release pointer) " % vnc.get_grab_keys()
>      else:
>          subtitle = ""
>  
> @@ -173,6 +173,8 @@ layout.add(vnc)
>  vnc.realize()
>  vnc.set_pointer_grab(True)
>  vnc.set_keyboard_grab(True)
> +# Example to define "Ctrl+Alt+F" or "Ctrl+Alt+f" as a grab key combo
> +vnc.set_grab_keys("Ctrl+Alt+[F|f]")
>  #v.set_pointer_local(True)
>  
>  if len(sys.argv) == 3:
> diff --git a/src/libgtk-vnc_sym.version b/src/libgtk-vnc_sym.version
> index b6f5347..41bcf21 100644
> --- a/src/libgtk-vnc_sym.version
> +++ b/src/libgtk-vnc_sym.version
> @@ -68,6 +68,10 @@
>      vnc_image_framebuffer_new;
>      vnc_image_framebuffer_get_image;
>  
> +# grab key settings support
> +    vnc_display_set_grab_keys;
> +    vnc_display_get_grab_keys;
> +
>    local:
>        *;
>  };
> diff --git a/src/vncdisplay.c b/src/vncdisplay.c
> index a031125..960b43c 100644
> --- a/src/vncdisplay.c
> +++ b/src/vncdisplay.c
> @@ -44,6 +44,8 @@
>  #include <winsock2.h>
>  #endif
>  
> +#define VNC_DEFAULT_GRAB_KEYS "Ctrl+Alt"
> +
>  static void winsock_startup (void);
>  static void winsock_cleanup (void);
>  
> @@ -85,6 +87,13 @@ struct _VncDisplayPrivate
>  
>  	GSList *preferable_auths;
>  	const guint8 const *keycode_map;
> +
> +        /* Variables for the grab key */
> +        gchar *grab_key_combo;
> +        guint grab_key;
> +        guint grab_key_alt;
> +        guint grab_key_modifiers;
> +        guint _grab_key_mod;
>  };

Indentation again here

>  
>  G_DEFINE_TYPE(VncDisplay, vnc_display, GTK_TYPE_DRAWING_AREA)
> @@ -95,6 +104,7 @@ enum
>    PROP_0,
>    PROP_POINTER_LOCAL,
>    PROP_POINTER_GRAB,
> +  PROP_GRAB_KEYS,
>    PROP_KEYBOARD_GRAB,
>    PROP_READ_ONLY,
>    PROP_WIDTH,
> @@ -162,6 +172,9 @@ vnc_display_get_property (GObject    *object,
>  
>    switch (prop_id)
>      {
> +      case PROP_GRAB_KEYS:
> +        g_value_set_string(value, vnc->priv->grab_key_combo);
> +        break;
>        case PROP_POINTER_LOCAL:
>          g_value_set_boolean (value, vnc->priv->local_pointer);
>  	break;
> @@ -220,6 +233,9 @@ vnc_display_set_property (GObject      *object,
>        case PROP_POINTER_GRAB:
>          vnc_display_set_pointer_grab (vnc, g_value_get_boolean (value));
>          break;
> +      case PROP_GRAB_KEYS:
> +        vnc_display_set_grab_keys (vnc, g_value_get_string (value));
> +        break;
>        case PROP_KEYBOARD_GRAB:
>          vnc_display_set_keyboard_grab (vnc, g_value_get_boolean (value));
>          break;
> @@ -673,6 +689,93 @@ static gboolean motion_event(GtkWidget *widget, GdkEventMotion *motion)
>  	return TRUE;
>  }
>  
> +#define KEY_MOD_CTRL	0x01
> +#define KEY_MOD_ALT	0x02
> +#define KEY_MOD_SHIFT	0x04
> +#define KEY_MOD_SUPER	0x08
> +#define KEY_MOD_HYPER	0x16
> +
> +static void update_grab_keys(VncDisplay *obj)
> +{
> +        VncDisplayPrivate *priv = obj->priv;
> +        guint mod, len;
> +
> +        if ((len = strlen(priv->grab_key_combo)) < 3)
> +            return;
> +
> +        mod = 0;
> +        if (strstr(priv->grab_key_combo, "Ctrl") != NULL)
> +            mod |= KEY_MOD_CTRL;
> +        if (strstr(priv->grab_key_combo, "Alt") != NULL)
> +            mod |= KEY_MOD_ALT;
> +        if (strstr(priv->grab_key_combo, "Shift") != NULL)
> +            mod |= KEY_MOD_SHIFT;
> +        if (strstr(priv->grab_key_combo, "Super") != NULL)
> +            mod |= KEY_MOD_SUPER;
> +        if (strstr(priv->grab_key_combo, "Hyper") != NULL)
> +            mod |= KEY_MOD_HYPER;
> +
> +        priv->grab_key_modifiers = mod;
> +
> +        if ((priv->grab_key_combo[ len - 3 ] == '[')
> +           && (priv->grab_key_combo[ len - 1 ] == ']'))
> +           priv->grab_key = priv->grab_key_combo[ len - 2 ];
> +
> +        if ((priv->grab_key_combo[ len - 5 ] == '[')
> +           && (priv->grab_key_combo[ len - 3 ] == '|')
> +           && (priv->grab_key_combo[ len - 1 ] == ']')) {
> +           priv->grab_key = priv->grab_key_combo[ len - 4 ];
> +           priv->grab_key_alt = priv->grab_key_combo[ len - 2 ];
> +        }
> +}



> +
> +static gboolean check_for_grab_key(GtkWidget *widget, int type, int keyval)
> +{
> +        VncDisplayPrivate *priv = VNC_DISPLAY(widget)->priv;
> +        int val;
> +
> +        switch (keyval) {
> +            case GDK_Control_L:
> +            case GDK_Control_R:
> +                 val = KEY_MOD_CTRL;
> +                 break;
> +            case GDK_Alt_L:
> +            case GDK_Alt_R:
> +                 val = KEY_MOD_ALT;
> +                 break;
> +            case GDK_Shift_L:
> +            case GDK_Shift_R:
> +                 val = KEY_MOD_SHIFT;
> +                 break;
> +            case GDK_Super_L:
> +            case GDK_Super_R:
> +                 val = KEY_MOD_SUPER;
> +                 break;
> +            case GDK_Hyper_L:
> +            case GDK_Hyper_R:
> +                 val = KEY_MOD_HYPER;
> +                 break;
> +            default:
> +                 val = 0;
> +                 break;
> +        }
> +
> +        if (val) {
> +            if (priv->_grab_key_mod & val)
> +                priv->_grab_key_mod -= val;
> +            else
> +                priv->_grab_key_mod |= val;
> +        }
> +
> +        if ((priv->_grab_key_mod == priv->grab_key_modifiers)
> +           && (((keyval == priv->grab_key) || (keyval == priv->grab_key_alt))
> +               || ((priv->grab_key == 0) && (priv->grab_key_alt == 0)))
> +           && (type == GDK_KEY_PRESS))
> +            return TRUE;
> +
> +        return FALSE;
> +}
> +
>  static gboolean key_event(GtkWidget *widget, GdkEventKey *key)
>  {
>  	VncDisplayPrivate *priv = VNC_DISPLAY(widget)->priv;
> @@ -752,9 +855,7 @@ static gboolean key_event(GtkWidget *widget, GdkEventKey *key)
>  		}
>  	}
>  
> -	if (key->type == GDK_KEY_PRESS &&
> -	    ((keyval == GDK_Control_L && (key->state & GDK_MOD1_MASK)) ||
> -	     (keyval == GDK_Alt_L && (key->state & GDK_CONTROL_MASK)))) {
> +	if (check_for_grab_key(widget, key->type, key->keyval)) {
>  		if (priv->in_pointer_grab)
>  			do_pointer_ungrab(VNC_DISPLAY(widget), FALSE);
>  		else if (!priv->grab_keyboard || !priv->absolute)
> @@ -1817,6 +1918,8 @@ static void vnc_display_init(VncDisplay *display)
>  	priv->shared_flag = FALSE;
>  	priv->force_size = TRUE;
>  
> +	vnc_display_set_grab_keys(display, VNC_DEFAULT_GRAB_KEYS);
> +
>  	/*
>  	 * Both these two provide TLS based auth, and can layer
>  	 * all the other auth types on top. So these two must
> @@ -1898,6 +2001,19 @@ void vnc_display_set_pointer_grab(VncDisplay *obj, gboolean enable)
>  		do_pointer_ungrab(obj, FALSE);
>  }
>  
> +void vnc_display_set_grab_keys(VncDisplay *obj, const gchar *key_combo)
> +{
> +        VncDisplayPrivate *priv = obj->priv;
> +
> +        priv->grab_key_combo = (gchar *)key_combo;
> +        update_grab_keys(obj);
> +}
> +
> +const gchar *vnc_display_get_grab_keys(VncDisplay *obj)
> +{
> +        return obj->priv->grab_key_combo;
> +}
> +
>  void vnc_display_set_keyboard_grab(VncDisplay *obj, gboolean enable)
>  {
>  	VncDisplayPrivate *priv = obj->priv;
> diff --git a/src/vncdisplay.h b/src/vncdisplay.h
> index 1b5b297..1f00f5b 100644
> --- a/src/vncdisplay.h
> +++ b/src/vncdisplay.h
> @@ -94,6 +94,8 @@ void            vnc_display_send_keys_ex(VncDisplay *obj, const guint *keyvals,
>  					 int nkeyvals, VncDisplayKeyEvent kind);
>  
>  void		vnc_display_send_pointer(VncDisplay *obj, gint x, gint y, int button_mask);
> +void		vnc_display_set_grab_keys(VncDisplay *obj, const gchar *key_combo);
> +const gchar*	vnc_display_get_grab_keys(VncDisplay *obj);

I'm not a huge fan of this as an API signature because we're inventing
a compound string format that people now have to parse/build. 

I'm somewhat inclined to think we should be taking a list of modifiers + 
list of keysyms, but that's not all that pleasant either.

eg, to set  Ctrl+Alt+f as the grab sequence, an app could call

  int[] grabmods = { GDK_CONTROL_MASK , GDK_MOD1_MASK, NULL }
  int[] grabkeys = { GDK_f, NULL };
  vnc_display_set_grab_keys(vnc, grabmods, grabkeys);

Or perhaps a simple data type

   typedef struct {
      int modmask;
      int nkeysyms;
      int *keysyms;
   } VncGrabSequence;

   VncGrabSequence seq = {
       .modmask = GDK_CONTROL_MASK | GDK_MOD1_MASK,
       .nkeysyms = 1, .keysyms = { GDK_f }
   };
   vnc_display_set_grab_keys(vnc, seq);

Regards,
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 :|
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]