Patchwork [v2] gtk: Fix accelerator filtering

login
register
mail settings
Submitter Jan Kiszka
Date Feb. 25, 2013, 2:56 p.m.
Message ID <512B7B8B.1000402@siemens.com>
Download mbox | patch
Permalink /patch/222927/
State New
Headers show

Comments

Jan Kiszka - Feb. 25, 2013, 2:56 p.m.
This is in fact very simply: When the input in grabbed, everything
should be exclusively passed to the guest - except it has our magic
CTRL-ALT modifier set. Then let GTK filter out those accels that are in
use. When checking the modifier state, we just need to filter out NUM
and CAPS lock.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Changes in v2 (yeah, almost "very simple"):
 - ignore set NUM or CAPS when testing for hot-key modifiers

 ui/gtk.c |   40 +++++++++++++++++++---------------------
 1 files changed, 19 insertions(+), 21 deletions(-)
Anthony Liguori - Feb. 25, 2013, 3:39 p.m.
Jan Kiszka <jan.kiszka@siemens.com> writes:

> This is in fact very simply: When the input in grabbed, everything
> should be exclusively passed to the guest - except it has our magic
> CTRL-ALT modifier set. Then let GTK filter out those accels that are in
> use. When checking the modifier state, we just need to filter out NUM
> and CAPS lock.

Can you explain what you're fixing?

We shouldn't hard code modifiers like this.  The reason you give
accelerators paths like this is so that they can be overridden by a
user.

That's why I filtered by path.  Once we're running, we shouldn't assume
that accelerators use the modifiers we started with.

Regard,s

Anthony Liguori

>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>
> Changes in v2 (yeah, almost "very simple"):
>  - ignore set NUM or CAPS when testing for hot-key modifiers
>
>  ui/gtk.c |   40 +++++++++++++++++++---------------------
>  1 files changed, 19 insertions(+), 21 deletions(-)
>
> diff --git a/ui/gtk.c b/ui/gtk.c
> index e38ed0d..5682a93 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -74,6 +74,10 @@
>  
>  #define MAX_VCS 10
>  
> +#define HOTKEY_MODIFIERS        (GDK_CONTROL_MASK | GDK_MOD1_MASK)
> +#define IGNORE_MODIFIER_MASK \
> +    (GDK_MODIFIER_MASK & ~(GDK_LOCK_MASK | GDK_MOD2_MASK))
> +
>  typedef struct VirtualConsole
>  {
>      GtkWidget *menu_item;
> @@ -322,22 +326,10 @@ static void gd_mouse_mode_change(Notifier *notify, void *data)
>  static gboolean gd_window_key_event(GtkWidget *widget, GdkEventKey *key, void *opaque)
>  {
>      GtkDisplayState *s = opaque;
> -    GtkAccelGroupEntry *entries;
> -    guint n_entries = 0;
> -    gboolean propagate_accel = TRUE;
>      gboolean handled = FALSE;
>  
> -    entries = gtk_accel_group_query(s->accel_group, key->keyval,
> -                                    key->state, &n_entries);
> -    if (n_entries) {
> -        const char *quark = g_quark_to_string(entries[0].accel_path_quark);
> -
> -        if (gd_is_grab_active(s) && strstart(quark, "<QEMU>/File/", NULL)) {
> -            propagate_accel = FALSE;
> -        }
> -    }
> -
> -    if (!handled && propagate_accel) {
> +    if (!gd_is_grab_active(s) ||
> +        (key->state & IGNORE_MODIFIER_MASK) == HOTKEY_MODIFIERS) {
>          handled = gtk_window_activate_key(GTK_WINDOW(widget), key);
>      }
>  
> @@ -925,7 +917,7 @@ static GSList *gd_vc_init(GtkDisplayState *s, VirtualConsole *vc, int index, GSL
>      vc->menu_item = gtk_radio_menu_item_new_with_mnemonic(group, label);
>      group = gtk_radio_menu_item_get_group(GTK_RADIO_MENU_ITEM(vc->menu_item));
>      gtk_menu_item_set_accel_path(GTK_MENU_ITEM(vc->menu_item), path);
> -    gtk_accel_map_add_entry(path, GDK_KEY_2 + index, GDK_CONTROL_MASK | GDK_MOD1_MASK);
> +    gtk_accel_map_add_entry(path, GDK_KEY_2 + index, HOTKEY_MODIFIERS);
>  
>      vc->terminal = vte_terminal_new();
>  
> @@ -1075,7 +1067,8 @@ static void gd_create_menus(GtkDisplayState *s)
>          gtk_image_menu_item_new_from_stock(GTK_STOCK_FULLSCREEN, NULL);
>      gtk_menu_item_set_accel_path(GTK_MENU_ITEM(s->full_screen_item),
>                                   "<QEMU>/View/Full Screen");
> -    gtk_accel_map_add_entry("<QEMU>/View/Full Screen", GDK_KEY_f, GDK_CONTROL_MASK | GDK_MOD1_MASK);
> +    gtk_accel_map_add_entry("<QEMU>/View/Full Screen", GDK_KEY_f,
> +                            HOTKEY_MODIFIERS);
>      gtk_menu_append(GTK_MENU(s->view_menu), s->full_screen_item);
>  
>      separator = gtk_separator_menu_item_new();
> @@ -1084,19 +1077,22 @@ static void gd_create_menus(GtkDisplayState *s)
>      s->zoom_in_item = gtk_image_menu_item_new_from_stock(GTK_STOCK_ZOOM_IN, NULL);
>      gtk_menu_item_set_accel_path(GTK_MENU_ITEM(s->zoom_in_item),
>                                   "<QEMU>/View/Zoom In");
> -    gtk_accel_map_add_entry("<QEMU>/View/Zoom In", GDK_KEY_plus, GDK_CONTROL_MASK | GDK_MOD1_MASK);
> +    gtk_accel_map_add_entry("<QEMU>/View/Zoom In", GDK_KEY_plus,
> +                            HOTKEY_MODIFIERS);
>      gtk_menu_append(GTK_MENU(s->view_menu), s->zoom_in_item);
>  
>      s->zoom_out_item = gtk_image_menu_item_new_from_stock(GTK_STOCK_ZOOM_OUT, NULL);
>      gtk_menu_item_set_accel_path(GTK_MENU_ITEM(s->zoom_out_item),
>                                   "<QEMU>/View/Zoom Out");
> -    gtk_accel_map_add_entry("<QEMU>/View/Zoom Out", GDK_KEY_minus, GDK_CONTROL_MASK | GDK_MOD1_MASK);
> +    gtk_accel_map_add_entry("<QEMU>/View/Zoom Out", GDK_KEY_minus,
> +                            HOTKEY_MODIFIERS);
>      gtk_menu_append(GTK_MENU(s->view_menu), s->zoom_out_item);
>  
>      s->zoom_fixed_item = gtk_image_menu_item_new_from_stock(GTK_STOCK_ZOOM_100, NULL);
>      gtk_menu_item_set_accel_path(GTK_MENU_ITEM(s->zoom_fixed_item),
>                                   "<QEMU>/View/Zoom Fixed");
> -    gtk_accel_map_add_entry("<QEMU>/View/Zoom Fixed", GDK_KEY_0, GDK_CONTROL_MASK | GDK_MOD1_MASK);
> +    gtk_accel_map_add_entry("<QEMU>/View/Zoom Fixed", GDK_KEY_0,
> +                            HOTKEY_MODIFIERS);
>      gtk_menu_append(GTK_MENU(s->view_menu), s->zoom_fixed_item);
>  
>      s->zoom_fit_item = gtk_check_menu_item_new_with_mnemonic(_("Zoom To _Fit"));
> @@ -1111,7 +1107,8 @@ static void gd_create_menus(GtkDisplayState *s)
>      s->grab_item = gtk_check_menu_item_new_with_mnemonic(_("_Grab Input"));
>      gtk_menu_item_set_accel_path(GTK_MENU_ITEM(s->grab_item),
>                                   "<QEMU>/View/Grab Input");
> -    gtk_accel_map_add_entry("<QEMU>/View/Grab Input", GDK_KEY_g, GDK_CONTROL_MASK | GDK_MOD1_MASK);
> +    gtk_accel_map_add_entry("<QEMU>/View/Grab Input", GDK_KEY_g,
> +                            HOTKEY_MODIFIERS);
>      gtk_menu_append(GTK_MENU(s->view_menu), s->grab_item);
>  
>      separator = gtk_separator_menu_item_new();
> @@ -1121,7 +1118,8 @@ static void gd_create_menus(GtkDisplayState *s)
>      group = gtk_radio_menu_item_get_group(GTK_RADIO_MENU_ITEM(s->vga_item));
>      gtk_menu_item_set_accel_path(GTK_MENU_ITEM(s->vga_item),
>                                   "<QEMU>/View/VGA");
> -    gtk_accel_map_add_entry("<QEMU>/View/VGA", GDK_KEY_1, GDK_CONTROL_MASK | GDK_MOD1_MASK);
> +    gtk_accel_map_add_entry("<QEMU>/View/VGA", GDK_KEY_1,
> +                            HOTKEY_MODIFIERS);
>      gtk_menu_append(GTK_MENU(s->view_menu), s->vga_item);
>  
>      for (i = 0; i < nb_vcs; i++) {
> -- 
> 1.7.3.4
Jan Kiszka - Feb. 25, 2013, 3:44 p.m.
On 2013-02-25 16:39, Anthony Liguori wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> This is in fact very simply: When the input in grabbed, everything
>> should be exclusively passed to the guest - except it has our magic
>> CTRL-ALT modifier set. Then let GTK filter out those accels that are in
>> use. When checking the modifier state, we just need to filter out NUM
>> and CAPS lock.
> 
> Can you explain what you're fixing?

That it's not filtering what it is supposed to.

> 
> We shouldn't hard code modifiers like this.  The reason you give
> accelerators paths like this is so that they can be overridden by a
> user.
> 
> That's why I filtered by path.  Once we're running, we shouldn't assume
> that accelerators use the modifiers we started with.

Your path-based filtering does not work as it uses an unsupported
internal function (see my other mail).

We can make the modifier configurable via QEMU means (command line
parameter, gconfig, whatever). But let's get the basics working first.

Jan
Jan Kiszka - March 24, 2013, 6:06 p.m.
On 2013-02-25 16:44, Jan Kiszka wrote:
> On 2013-02-25 16:39, Anthony Liguori wrote:
>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>
>>> This is in fact very simply: When the input in grabbed, everything
>>> should be exclusively passed to the guest - except it has our magic
>>> CTRL-ALT modifier set. Then let GTK filter out those accels that are in
>>> use. When checking the modifier state, we just need to filter out NUM
>>> and CAPS lock.
>>
>> Can you explain what you're fixing?
> 
> That it's not filtering what it is supposed to.
> 
>>
>> We shouldn't hard code modifiers like this.  The reason you give
>> accelerators paths like this is so that they can be overridden by a
>> user.
>>
>> That's why I filtered by path.  Once we're running, we shouldn't assume
>> that accelerators use the modifiers we started with.
> 
> Your path-based filtering does not work as it uses an unsupported
> internal function (see my other mail).
> 
> We can make the modifier configurable via QEMU means (command line
> parameter, gconfig, whatever). But let's get the basics working first.

The bug still exists, my patch still applies. Unless you have some idea
for a better solution, please apply this for now so that CTRL-q inside a
guest doesn't kill more kittens.

Jan
Anthony Liguori - March 25, 2013, 12:51 p.m.
Jan Kiszka <jan.kiszka@web.de> writes:

> On 2013-02-25 16:44, Jan Kiszka wrote:
>> On 2013-02-25 16:39, Anthony Liguori wrote:
>>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>>
>>>> This is in fact very simply: When the input in grabbed, everything
>>>> should be exclusively passed to the guest - except it has our magic
>>>> CTRL-ALT modifier set. Then let GTK filter out those accels that are in
>>>> use. When checking the modifier state, we just need to filter out NUM
>>>> and CAPS lock.
>>>
>>> Can you explain what you're fixing?
>> 
>> That it's not filtering what it is supposed to.
>> 
>>>
>>> We shouldn't hard code modifiers like this.  The reason you give
>>> accelerators paths like this is so that they can be overridden by a
>>> user.
>>>
>>> That's why I filtered by path.  Once we're running, we shouldn't assume
>>> that accelerators use the modifiers we started with.
>> 
>> Your path-based filtering does not work as it uses an unsupported
>> internal function (see my other mail).
>> 
>> We can make the modifier configurable via QEMU means (command line
>> parameter, gconfig, whatever). But let's get the basics working first.
>
> The bug still exists, my patch still applies. Unless you have some idea
> for a better solution, please apply this for now so that CTRL-q inside a
> guest doesn't kill more kittens.

Hi Jan,

Your patch breaks overriding accelerators which as I said before is a
critical accessibility feature.

The current code works for me but I realize it's using an unsupported
interface.  I'll spend some time today trying to find a work around.
But we definitely cannot assume that the accelerators are using any
specific modifiers.

Regards,

Anthony Liguori

>
> Jan
Jordan Justen - May 7, 2013, 9:03 p.m.
On Sun, Mar 24, 2013 at 11:06 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2013-02-25 16:44, Jan Kiszka wrote:
>> On 2013-02-25 16:39, Anthony Liguori wrote:
>>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>>
>>>> This is in fact very simply: When the input in grabbed, everything
>>>> should be exclusively passed to the guest - except it has our magic
>>>> CTRL-ALT modifier set. Then let GTK filter out those accels that are in
>>>> use. When checking the modifier state, we just need to filter out NUM
>>>> and CAPS lock.
>>>
>>> Can you explain what you're fixing?
>>
>> That it's not filtering what it is supposed to.
>>
>>>
>>> We shouldn't hard code modifiers like this.  The reason you give
>>> accelerators paths like this is so that they can be overridden by a
>>> user.
>>>
>>> That's why I filtered by path.  Once we're running, we shouldn't assume
>>> that accelerators use the modifiers we started with.
>>
>> Your path-based filtering does not work as it uses an unsupported
>> internal function (see my other mail).
>>
>> We can make the modifier configurable via QEMU means (command line
>> parameter, gconfig, whatever). But let's get the basics working first.
>
> The bug still exists, my patch still applies. Unless you have some idea
> for a better solution, please apply this for now so that CTRL-q inside a
> guest doesn't kill more kittens.

I finally built qemu with gtk support, and in general it seems like a
great improvement over SDL.

...except ctrl-q to quit the VM. Why is binding a hotkey to quit a
good idea at all? It seems kind of like attaching your computer's
power to a wall-switch. :)

-Jordan
Jan Kiszka - May 7, 2013, 10:42 p.m.
On 2013-05-07 23:03, Jordan Justen wrote:
> On Sun, Mar 24, 2013 at 11:06 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2013-02-25 16:44, Jan Kiszka wrote:
>>> On 2013-02-25 16:39, Anthony Liguori wrote:
>>>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>>>
>>>>> This is in fact very simply: When the input in grabbed, everything
>>>>> should be exclusively passed to the guest - except it has our magic
>>>>> CTRL-ALT modifier set. Then let GTK filter out those accels that are in
>>>>> use. When checking the modifier state, we just need to filter out NUM
>>>>> and CAPS lock.
>>>>
>>>> Can you explain what you're fixing?
>>>
>>> That it's not filtering what it is supposed to.
>>>
>>>>
>>>> We shouldn't hard code modifiers like this.  The reason you give
>>>> accelerators paths like this is so that they can be overridden by a
>>>> user.
>>>>
>>>> That's why I filtered by path.  Once we're running, we shouldn't assume
>>>> that accelerators use the modifiers we started with.
>>>
>>> Your path-based filtering does not work as it uses an unsupported
>>> internal function (see my other mail).
>>>
>>> We can make the modifier configurable via QEMU means (command line
>>> parameter, gconfig, whatever). But let's get the basics working first.
>>
>> The bug still exists, my patch still applies. Unless you have some idea
>> for a better solution, please apply this for now so that CTRL-q inside a
>> guest doesn't kill more kittens.
> 
> I finally built qemu with gtk support, and in general it seems like a
> great improvement over SDL.
> 
> ...except ctrl-q to quit the VM. Why is binding a hotkey to quit a
> good idea at all? It seems kind of like attaching your computer's
> power to a wall-switch. :)

Yeah, this bug should really be fixed in some way before 1.5 is released
because GTK will be default. Anthony, what is the status of your
experiments with alternative solutions?

Jan
Jan Kiszka - July 17, 2013, 6:54 a.m.
On 2013-05-08 00:42, Jan Kiszka wrote:
> On 2013-05-07 23:03, Jordan Justen wrote:
>> On Sun, Mar 24, 2013 at 11:06 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> On 2013-02-25 16:44, Jan Kiszka wrote:
>>>> On 2013-02-25 16:39, Anthony Liguori wrote:
>>>>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>>>>
>>>>>> This is in fact very simply: When the input in grabbed, everything
>>>>>> should be exclusively passed to the guest - except it has our magic
>>>>>> CTRL-ALT modifier set. Then let GTK filter out those accels that are in
>>>>>> use. When checking the modifier state, we just need to filter out NUM
>>>>>> and CAPS lock.
>>>>>
>>>>> Can you explain what you're fixing?
>>>>
>>>> That it's not filtering what it is supposed to.
>>>>
>>>>>
>>>>> We shouldn't hard code modifiers like this.  The reason you give
>>>>> accelerators paths like this is so that they can be overridden by a
>>>>> user.
>>>>>
>>>>> That's why I filtered by path.  Once we're running, we shouldn't assume
>>>>> that accelerators use the modifiers we started with.
>>>>
>>>> Your path-based filtering does not work as it uses an unsupported
>>>> internal function (see my other mail).
>>>>
>>>> We can make the modifier configurable via QEMU means (command line
>>>> parameter, gconfig, whatever). But let's get the basics working first.
>>>
>>> The bug still exists, my patch still applies. Unless you have some idea
>>> for a better solution, please apply this for now so that CTRL-q inside a
>>> guest doesn't kill more kittens.
>>
>> I finally built qemu with gtk support, and in general it seems like a
>> great improvement over SDL.
>>
>> ...except ctrl-q to quit the VM. Why is binding a hotkey to quit a
>> good idea at all? It seems kind of like attaching your computer's
>> power to a wall-switch. :)
> 
> Yeah, this bug should really be fixed in some way before 1.5 is released
> because GTK will be default. Anthony, what is the status of your
> experiments with alternative solutions?

Re-ping on this, now for 1.6 (with the option to backport the fix).

What defines the modifiers to be used for the accelerators? Is there an
interface to query them (to avoid my original hard-coding)?

Jan
Anthony Liguori - July 17, 2013, 1:02 p.m.
Jan Kiszka <jan.kiszka@web.de> writes:

> On 2013-05-08 00:42, Jan Kiszka wrote:
>> On 2013-05-07 23:03, Jordan Justen wrote:
>> Yeah, this bug should really be fixed in some way before 1.5 is released
>> because GTK will be default. Anthony, what is the status of your
>> experiments with alternative solutions?
>
> Re-ping on this, now for 1.6 (with the option to backport the fix).
>
> What defines the modifiers to be used for the accelerators? Is there an
> interface to query them (to avoid my original hard-coding)?

I hate to let the bug fester for another release although this fix
breaks accelerator remapping.  I guess that's better than nothing so
please rebase.  I'll try to fix it properly on top of your patch.

Regards,

Anthony Liguori

>
> Jan

Patch

diff --git a/ui/gtk.c b/ui/gtk.c
index e38ed0d..5682a93 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -74,6 +74,10 @@ 
 
 #define MAX_VCS 10
 
+#define HOTKEY_MODIFIERS        (GDK_CONTROL_MASK | GDK_MOD1_MASK)
+#define IGNORE_MODIFIER_MASK \
+    (GDK_MODIFIER_MASK & ~(GDK_LOCK_MASK | GDK_MOD2_MASK))
+
 typedef struct VirtualConsole
 {
     GtkWidget *menu_item;
@@ -322,22 +326,10 @@  static void gd_mouse_mode_change(Notifier *notify, void *data)
 static gboolean gd_window_key_event(GtkWidget *widget, GdkEventKey *key, void *opaque)
 {
     GtkDisplayState *s = opaque;
-    GtkAccelGroupEntry *entries;
-    guint n_entries = 0;
-    gboolean propagate_accel = TRUE;
     gboolean handled = FALSE;
 
-    entries = gtk_accel_group_query(s->accel_group, key->keyval,
-                                    key->state, &n_entries);
-    if (n_entries) {
-        const char *quark = g_quark_to_string(entries[0].accel_path_quark);
-
-        if (gd_is_grab_active(s) && strstart(quark, "<QEMU>/File/", NULL)) {
-            propagate_accel = FALSE;
-        }
-    }
-
-    if (!handled && propagate_accel) {
+    if (!gd_is_grab_active(s) ||
+        (key->state & IGNORE_MODIFIER_MASK) == HOTKEY_MODIFIERS) {
         handled = gtk_window_activate_key(GTK_WINDOW(widget), key);
     }
 
@@ -925,7 +917,7 @@  static GSList *gd_vc_init(GtkDisplayState *s, VirtualConsole *vc, int index, GSL
     vc->menu_item = gtk_radio_menu_item_new_with_mnemonic(group, label);
     group = gtk_radio_menu_item_get_group(GTK_RADIO_MENU_ITEM(vc->menu_item));
     gtk_menu_item_set_accel_path(GTK_MENU_ITEM(vc->menu_item), path);
-    gtk_accel_map_add_entry(path, GDK_KEY_2 + index, GDK_CONTROL_MASK | GDK_MOD1_MASK);
+    gtk_accel_map_add_entry(path, GDK_KEY_2 + index, HOTKEY_MODIFIERS);
 
     vc->terminal = vte_terminal_new();
 
@@ -1075,7 +1067,8 @@  static void gd_create_menus(GtkDisplayState *s)
         gtk_image_menu_item_new_from_stock(GTK_STOCK_FULLSCREEN, NULL);
     gtk_menu_item_set_accel_path(GTK_MENU_ITEM(s->full_screen_item),
                                  "<QEMU>/View/Full Screen");
-    gtk_accel_map_add_entry("<QEMU>/View/Full Screen", GDK_KEY_f, GDK_CONTROL_MASK | GDK_MOD1_MASK);
+    gtk_accel_map_add_entry("<QEMU>/View/Full Screen", GDK_KEY_f,
+                            HOTKEY_MODIFIERS);
     gtk_menu_append(GTK_MENU(s->view_menu), s->full_screen_item);
 
     separator = gtk_separator_menu_item_new();
@@ -1084,19 +1077,22 @@  static void gd_create_menus(GtkDisplayState *s)
     s->zoom_in_item = gtk_image_menu_item_new_from_stock(GTK_STOCK_ZOOM_IN, NULL);
     gtk_menu_item_set_accel_path(GTK_MENU_ITEM(s->zoom_in_item),
                                  "<QEMU>/View/Zoom In");
-    gtk_accel_map_add_entry("<QEMU>/View/Zoom In", GDK_KEY_plus, GDK_CONTROL_MASK | GDK_MOD1_MASK);
+    gtk_accel_map_add_entry("<QEMU>/View/Zoom In", GDK_KEY_plus,
+                            HOTKEY_MODIFIERS);
     gtk_menu_append(GTK_MENU(s->view_menu), s->zoom_in_item);
 
     s->zoom_out_item = gtk_image_menu_item_new_from_stock(GTK_STOCK_ZOOM_OUT, NULL);
     gtk_menu_item_set_accel_path(GTK_MENU_ITEM(s->zoom_out_item),
                                  "<QEMU>/View/Zoom Out");
-    gtk_accel_map_add_entry("<QEMU>/View/Zoom Out", GDK_KEY_minus, GDK_CONTROL_MASK | GDK_MOD1_MASK);
+    gtk_accel_map_add_entry("<QEMU>/View/Zoom Out", GDK_KEY_minus,
+                            HOTKEY_MODIFIERS);
     gtk_menu_append(GTK_MENU(s->view_menu), s->zoom_out_item);
 
     s->zoom_fixed_item = gtk_image_menu_item_new_from_stock(GTK_STOCK_ZOOM_100, NULL);
     gtk_menu_item_set_accel_path(GTK_MENU_ITEM(s->zoom_fixed_item),
                                  "<QEMU>/View/Zoom Fixed");
-    gtk_accel_map_add_entry("<QEMU>/View/Zoom Fixed", GDK_KEY_0, GDK_CONTROL_MASK | GDK_MOD1_MASK);
+    gtk_accel_map_add_entry("<QEMU>/View/Zoom Fixed", GDK_KEY_0,
+                            HOTKEY_MODIFIERS);
     gtk_menu_append(GTK_MENU(s->view_menu), s->zoom_fixed_item);
 
     s->zoom_fit_item = gtk_check_menu_item_new_with_mnemonic(_("Zoom To _Fit"));
@@ -1111,7 +1107,8 @@  static void gd_create_menus(GtkDisplayState *s)
     s->grab_item = gtk_check_menu_item_new_with_mnemonic(_("_Grab Input"));
     gtk_menu_item_set_accel_path(GTK_MENU_ITEM(s->grab_item),
                                  "<QEMU>/View/Grab Input");
-    gtk_accel_map_add_entry("<QEMU>/View/Grab Input", GDK_KEY_g, GDK_CONTROL_MASK | GDK_MOD1_MASK);
+    gtk_accel_map_add_entry("<QEMU>/View/Grab Input", GDK_KEY_g,
+                            HOTKEY_MODIFIERS);
     gtk_menu_append(GTK_MENU(s->view_menu), s->grab_item);
 
     separator = gtk_separator_menu_item_new();
@@ -1121,7 +1118,8 @@  static void gd_create_menus(GtkDisplayState *s)
     group = gtk_radio_menu_item_get_group(GTK_RADIO_MENU_ITEM(s->vga_item));
     gtk_menu_item_set_accel_path(GTK_MENU_ITEM(s->vga_item),
                                  "<QEMU>/View/VGA");
-    gtk_accel_map_add_entry("<QEMU>/View/VGA", GDK_KEY_1, GDK_CONTROL_MASK | GDK_MOD1_MASK);
+    gtk_accel_map_add_entry("<QEMU>/View/VGA", GDK_KEY_1,
+                            HOTKEY_MODIFIERS);
     gtk_menu_append(GTK_MENU(s->view_menu), s->vga_item);
 
     for (i = 0; i < nb_vcs; i++) {