[virt-tools-list] [PATCH] Virt-manager: Add configurable grab keys

[Cole Robinson]

On 08/10/2010 08:15 AM, Michal Novotny wrote:
> Hi,
> this is the patch to add configurable grab keys to the virt-manager I 
> did in my spare time for my own purposes originally (and also it's 
> partially based on a request from a collegue in our office). It requires 
> at least Gtk-VNC 0.4.0 since git commit 378721ec1 of Gtk-VNC introduced 
> this feature. It's been tested and this patch is for the latest 
> mercurial codebase of VMM and a bug 616355 ( [RFE] Add configurable grab 
> key sequences for VMM) has been filed by myself some time ago for this 
> request.
> 
> This is the second version of the patch that's including the exception 
> handling for case the user is using some older version of Gtk-VNC 
> (pre-0.4.0) that doesn't support the configurable grab keys. For the VMM 
> interface itself, a new tab in preferences dialog, called "Keys", has 
> been added showing the current grab keys combination and new GConf entry 
> is being written when edited. Also, when you press the "Define" button 
> on this tab a new dialog window is being opened and you have to press 
> all the keys you want to use as grab keys and when you have all the keys 
> you want to use in your combination pressed you have to click OK button 
> to allow VMM to remember it.
> 
> Also, one slight issue is when you opened the console window already 
> since the grab key combination is being read only on init() apparently 
> so when changing the grab keys combination the restart of virt-manager 
> is recommended.
> 
> Please write me your feedback.
> 
> Thanks,
> Michal
> 

Sorry for the late reply, and thanks for the patch.

I'd prefer if the preferences UI was just merged into the Prefs->VM
Details tab. Under the console options, I'd just have a line:

VNC grab sequence: Control_L+Alt_L  [Change...]

(that last piece is a button).

Some more comments inline.

> diff -r 17c8e3d3ee10 src/virtManager/config.py
> --- a/src/virtManager/config.py	Sat Aug 07 15:40:49 2010 +0000
> +++ b/src/virtManager/config.py	Tue Aug 10 13:54:05 2010 +0200
> @@ -289,6 +289,33 @@
>                               cb)
>  
>  
> +    # Keys preferences
> +    def get_keys_combination(self, syms = False):
> +        val = self.conf.get_string(self.conf_dir + "/keys/grab-keys")
> +        if syms == True:
> +            return val
> +        # We don't allow return of None, we use default of Ctrl + Alt instead
> +        if val is None:
> +            return "Control_L+Alt_L"
> +

Actually I'd prefer just returning None if nothing is explicitly set,
and update the get_keys_combination() callers to deal with None. That
way we can differentiate between a manually set key combo, or the
default, which means we can only call vncViewer.set_grab_keys when needed.

> +        # We convert keysyms to names
> +        keystr = None
> +        for k in val.split(','):
> +            if keystr is None:
> +                keystr = gtk.gdk.keyval_name(int(k))
> +            else:
> +                keystr = keystr + "+" + gtk.gdk.keyval_name(int(k))

Probably want to wrap this small chunk in a try: except:, someone could
manually edit gconf and enter bogus values that cant be int()'d

> +        # Disallow None
> +        if keystr is None:
> +            keystr = ""
> +
> +        return keystr
> +
> +    def set_keys_combination(self, val):
> +        # Val have to be a list of integers
> +        val = ','.join(map(str, val))
> +        self.conf.set_string(self.conf_dir + "/keys/grab-keys", val)
> +
>      # Confirmation preferences
>      def get_confirm_forcepoweroff(self):
>          return self.conf.get_bool(self.conf_dir + "/confirm/forcepoweroff")
> diff -r 17c8e3d3ee10 src/virtManager/console.py
> --- a/src/virtManager/console.py	Sat Aug 07 15:40:49 2010 +0000
> +++ b/src/virtManager/console.py	Tue Aug 10 13:54:05 2010 +0200
> @@ -84,6 +84,18 @@
>          self.vncViewer = gtkvnc.Display()
>          self.window.get_widget("console-vnc-viewport").add(self.vncViewer)
>  
> +        # Set default grab key combination if found
> +        # We encapsulate it into the try/except block for case the
> +        # Gtk-VNC is older and it's not supporting it
> +        try:
> +            grab_keys = self.config.get_keys_combination(True)
> +            if grab_keys is not None:
> +                keys = map(int, grab_keys.split(','))
> +                self.vncViewer.set_grab_keys( keys )
> +        except Exception, e:
> +            logging.debug("Error when getting the grab keys combination: %s" % str(e))
> +            pass
> +
>          self.init_vnc()
>  

make check-pylint throws a warning here. please make sure to run that
command before re-submitting.

In this case you should just be able to do

if hasattr(self.vncViewer, "set_grab_keys"):

rather than wrap it all in a try: block.

>          finish_img = gtk.image_new_from_stock(gtk.STOCK_YES,
> @@ -165,7 +177,18 @@
>          self._enable_modifiers()
>  
>      def notify_grabbed(self, src):
> -        self.topwin.set_title(_("Press Ctrl+Alt to release pointer.") +
> +        try:
> +            keys = src.get_grab_keys()
> +            keystr = None
> +            for k in keys:
> +                if keystr is None:
> +                    keystr = gtk.gdk.keyval_name(k)
> +                else:
> +                    keystr = keystr + "+" + gtk.gdk.keyval_name(k)
> +        except:
> +            keystr = "Control_L+Alt_L"
> +
> +        self.topwin.set_title(_("Press %s to release pointer.") % keystr +
>                                " " + self.title)
>  
>          if (not self.config.show_console_grab_notify() or
> @@ -182,7 +205,7 @@
>                                                          0,
>                                                          '',
>                                                          _("Pointer grabbed"),
> -                                                        _("The mouse pointer has been restricted to the virtual console window. To release the pointer, press the key pair: Ctrl+Alt"),
> +                                                        _("The mouse pointer has been restricted to the virtual console window. To release the pointer, press the key pair") + " " + keystr,
>                                                          ["dismiss", _("Do not show this notification in the future.")],
>                                                          {"desktop-entry": "virt-manager",
>                                                          "x": x+200, "y": y},
> diff -r 17c8e3d3ee10 src/virtManager/preferences.py
> --- a/src/virtManager/preferences.py	Sat Aug 07 15:40:49 2010 +0000
> +++ b/src/virtManager/preferences.py	Tue Aug 10 13:54:05 2010 +0200
> @@ -65,6 +65,7 @@
>          self.refresh_sound_remote()
>          self.refresh_disk_poll()
>          self.refresh_net_poll()
> +        self.refresh_grabkeys_combination()
>          self.refresh_confirm_forcepoweroff()
>          self.refresh_confirm_poweroff()
>          self.refresh_confirm_pause()
> @@ -90,6 +91,7 @@
>              "on_prefs_confirm_pause_toggled": self.change_confirm_pause,
>              "on_prefs_confirm_removedev_toggled": self.change_confirm_removedev,
>              "on_prefs_confirm_interface_toggled": self.change_confirm_interface,
> +            "on_prefs_btn_keys_define_clicked": self.change_grab_keys,
>              })
>          util.bind_escape_key_close(self)
>  

One general thing here, we shouldn't allow the user to change the
keybinding if gtk-vnc doesn't support it. So you can do a basic:

def grab_keys_supported():
  try:
    import gtkvnc
    return hasattr(gtkvnc.Display, "set_grab_keys")
  except:
    return False

If not supported, we'll want to disable the 'Change' button I
recommended above, and add a tooltip 'GTKVNC version does not support
changing grab sequence'

I think the rest is fine.

Thanks,
Cole