From 96ccb25f97ab2fd2ec08d11dfe5d281764a732f2 Mon Sep 17 00:00:00 2001 From: Emmanuele Bassi Date: Thu, 26 Mar 2020 17:11:25 +0000 Subject: [PATCH 1/5] Return a full reference when parsing triggers We're not returning a full reference for GtkNeverTrigger, but we are returning full references for mnemonic and keyval triggers; this means we're either going to leak mnemonic and keyval triggers if we consider this function a "transfer none" one, or we are going to trigger an assertion failure when finalizing a never trigger, if we consider this function a "transfer full" one. Let's be consistent, and always return a full reference to the caller. --- gtk/gtkshortcuttrigger.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/gtk/gtkshortcuttrigger.c b/gtk/gtkshortcuttrigger.c index 50f6958b18..fc36e809e5 100644 --- a/gtk/gtkshortcuttrigger.c +++ b/gtk/gtkshortcuttrigger.c @@ -121,7 +121,8 @@ gtk_shortcut_trigger_trigger (GtkShortcutTrigger *self, * - a string parsed by gtk_accelerator_parse(), for a #GtkKeyvalTrigger * - underscore, followed by a single character, for #GtkMnemonicTrigger * - * Returns: (nullable): a new #GtkShortcutTrigger or %NULL on error + * Returns: (nullable) (transfer full): a new #GtkShortcutTrigger + * or %NULL on error */ GtkShortcutTrigger * gtk_shortcut_trigger_parse_string (const char *string) @@ -132,7 +133,7 @@ gtk_shortcut_trigger_parse_string (const char *string) g_return_val_if_fail (string != NULL, NULL); if (g_str_equal (string, "never")) - return gtk_never_trigger_get (); + return g_object_ref (gtk_never_trigger_get ()); if (string[0] == '_') { From 170e8bd605bf983a9826ca255829846f40a2c59c Mon Sep 17 00:00:00 2001 From: Emmanuele Bassi Date: Thu, 26 Mar 2020 16:47:15 +0000 Subject: [PATCH 2/5] Parse keyval name directly for mnemonic triggers We don't need to parse the full accelerator format for mnemonic triggers. --- gtk/gtkshortcuttrigger.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/gtk/gtkshortcuttrigger.c b/gtk/gtkshortcuttrigger.c index fc36e809e5..6840ef10d2 100644 --- a/gtk/gtkshortcuttrigger.c +++ b/gtk/gtkshortcuttrigger.c @@ -137,8 +137,9 @@ gtk_shortcut_trigger_parse_string (const char *string) if (string[0] == '_') { - if (gtk_accelerator_parse (string + 1, &keyval, &modifiers)) - return gtk_mnemonic_trigger_new (keyval); + keyval = gdk_keyval_from_name (string + 1); + if (keyval != GDK_KEY_VoidSymbol) + return gtk_mnemonic_trigger_new (gdk_keyval_to_lower (keyval)); } if (gtk_accelerator_parse (string, &keyval, &modifiers)) From c75fdda8ddb0c0593f2f7d5cb7aa43f215462808 Mon Sep 17 00:00:00 2001 From: Emmanuele Bassi Date: Thu, 26 Mar 2020 16:48:03 +0000 Subject: [PATCH 3/5] tests: Add more cases for the trigger parser --- testsuite/gtk/shortcuts.c | 59 ++++++++++++++++++++++++++++++++------- 1 file changed, 49 insertions(+), 10 deletions(-) diff --git a/testsuite/gtk/shortcuts.c b/testsuite/gtk/shortcuts.c index 27df0c839e..1236baeff0 100644 --- a/testsuite/gtk/shortcuts.c +++ b/testsuite/gtk/shortcuts.c @@ -153,17 +153,31 @@ test_trigger_equal (void) static void test_trigger_parse (void) { + enum + { + TRIGGER_KEYVAL, + TRIGGER_MNEMONIC, + TRIGGER_NEVER, + TRIGGER_INVALID + }; + struct { const char *str; GdkModifierType modifiers; guint keyval; + int trigger_type; } tests[] = { - { "z", GDK_CONTROL_MASK|GDK_MOD1_MASK, 'z' }, - { "U", GDK_CONTROL_MASK, 'u' }, - { "x", GDK_HYPER_MASK, 'x' }, - { "y", GDK_META_MASK, 'y' }, - { "KP_7", 0, GDK_KEY_KP_7 }, - { "exclam", GDK_SHIFT_MASK, '!' }, + { "z", GDK_CONTROL_MASK | GDK_MOD1_MASK, 'z', TRIGGER_KEYVAL }, + { "U", GDK_CONTROL_MASK, 'u', TRIGGER_KEYVAL }, + { "x", GDK_HYPER_MASK, 'x', TRIGGER_KEYVAL }, + { "y", GDK_META_MASK, 'y', TRIGGER_KEYVAL }, + { "KP_7", 0, GDK_KEY_KP_7, TRIGGER_KEYVAL }, + { "exclam", GDK_SHIFT_MASK, '!', TRIGGER_KEYVAL }, + { "never", 0, 0, TRIGGER_NEVER }, + { "_A", 0, GDK_KEY_a, TRIGGER_MNEMONIC }, + { "_s", 0, GDK_KEY_s, TRIGGER_MNEMONIC }, + { "foo", 0, 0, TRIGGER_INVALID }, + { "B", 0, 0, TRIGGER_INVALID }, }; GtkShortcutTrigger *trigger; int i; @@ -172,11 +186,36 @@ test_trigger_parse (void) { trigger = gtk_shortcut_trigger_parse_string (tests[i].str); - g_assert_true (GTK_IS_KEYVAL_TRIGGER (trigger)); - g_assert_cmpint (gtk_keyval_trigger_get_modifiers (GTK_KEYVAL_TRIGGER (trigger)), ==, tests[i].modifiers); - g_assert_cmpuint (gtk_keyval_trigger_get_keyval (GTK_KEYVAL_TRIGGER (trigger)), ==, tests[i].keyval); + switch (tests[i].trigger_type) + { + case TRIGGER_INVALID: + g_assert_null (trigger); + break; + case TRIGGER_KEYVAL: + g_assert_true (GTK_IS_KEYVAL_TRIGGER (trigger)); + g_assert_cmpint (gtk_keyval_trigger_get_modifiers (GTK_KEYVAL_TRIGGER (trigger)), + ==, + tests[i].modifiers); + g_assert_cmpuint (gtk_keyval_trigger_get_keyval (GTK_KEYVAL_TRIGGER (trigger)), + ==, + tests[i].keyval); + break; + case TRIGGER_NEVER: + g_assert_true (GTK_IS_NEVER_TRIGGER (trigger)); + break; + case TRIGGER_MNEMONIC: + g_assert_true (GTK_IS_MNEMONIC_TRIGGER (trigger)); + g_assert_cmpuint (gtk_mnemonic_trigger_get_keyval (GTK_MNEMONIC_TRIGGER (trigger)), + ==, + tests[i].keyval); + break; + default: + g_assert_not_reached (); + break; + } - g_object_unref (trigger); + if (tests[i].trigger_type != TRIGGER_INVALID) + g_object_unref (trigger); } } From 6719d3044dc1677c264086345f2ef6084a63e590 Mon Sep 17 00:00:00 2001 From: Emmanuele Bassi Date: Thu, 26 Mar 2020 16:58:55 +0000 Subject: [PATCH 4/5] Add parsing for GtkAlternativeTrigger Alternative triggers are separate by a pipe character. --- gtk/gtkshortcuttrigger.c | 41 +++++++++++++++++++++++++++++++++++++-- testsuite/gtk/shortcuts.c | 5 +++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/gtk/gtkshortcuttrigger.c b/gtk/gtkshortcuttrigger.c index 6840ef10d2..0fdc5edb17 100644 --- a/gtk/gtkshortcuttrigger.c +++ b/gtk/gtkshortcuttrigger.c @@ -120,6 +120,8 @@ gtk_shortcut_trigger_trigger (GtkShortcutTrigger *self, * - `never`, for #GtkNeverTrigger * - a string parsed by gtk_accelerator_parse(), for a #GtkKeyvalTrigger * - underscore, followed by a single character, for #GtkMnemonicTrigger + * - two valid trigger strings, separated by a `|` character, for a + * #GtkAlternativeTrigger * * Returns: (nullable) (transfer full): a new #GtkShortcutTrigger * or %NULL on error @@ -129,9 +131,44 @@ gtk_shortcut_trigger_parse_string (const char *string) { GdkModifierType modifiers; guint keyval; + const char *sep; g_return_val_if_fail (string != NULL, NULL); + if ((sep = strchr (string, '|')) != NULL) + { + char *frag_a = g_strndup (string, sep - string); + const char *frag_b = sep + 1; + GtkShortcutTrigger *t1, *t2; + + /* empty first slot */ + if (*frag_a == '\0') + return NULL; + + /* empty second slot */ + if (*frag_b == '\0') + return NULL; + + t1 = gtk_shortcut_trigger_parse_string (frag_a); + if (t1 == NULL) + { + g_free (frag_a); + return NULL; + } + + t2 = gtk_shortcut_trigger_parse_string (frag_b); + if (t2 == NULL) + { + g_object_unref (t1); + g_free (frag_a); + return NULL; + } + + g_free (frag_a); + + return gtk_alternative_trigger_new (t1, t2); + } + if (g_str_equal (string, "never")) return g_object_ref (gtk_never_trigger_get ()); @@ -398,7 +435,7 @@ static void gtk_never_trigger_print (GtkShortcutTrigger *trigger, GString *string) { - g_string_append (string, ""); + g_string_append (string, "never"); } static gboolean @@ -1091,7 +1128,7 @@ gtk_alternative_trigger_print (GtkShortcutTrigger *trigger, GtkAlternativeTrigger *self = GTK_ALTERNATIVE_TRIGGER (trigger); gtk_shortcut_trigger_print (self->first, string); - g_string_append (string, ", "); + g_string_append (string, "|"); gtk_shortcut_trigger_print (self->second, string); } diff --git a/testsuite/gtk/shortcuts.c b/testsuite/gtk/shortcuts.c index 1236baeff0..fca321a49f 100644 --- a/testsuite/gtk/shortcuts.c +++ b/testsuite/gtk/shortcuts.c @@ -157,6 +157,7 @@ test_trigger_parse (void) { TRIGGER_KEYVAL, TRIGGER_MNEMONIC, + TRIGGER_ALT, TRIGGER_NEVER, TRIGGER_INVALID }; @@ -178,6 +179,7 @@ test_trigger_parse (void) { "_s", 0, GDK_KEY_s, TRIGGER_MNEMONIC }, { "foo", 0, 0, TRIGGER_INVALID }, { "B", 0, 0, TRIGGER_INVALID }, + { "U|U", GDK_CONTROL_MASK, 'u', TRIGGER_ALT } }; GtkShortcutTrigger *trigger; int i; @@ -200,6 +202,9 @@ test_trigger_parse (void) ==, tests[i].keyval); break; + case TRIGGER_ALT: + g_assert_true (GTK_IS_ALTERNATIVE_TRIGGER (trigger)); + break; case TRIGGER_NEVER: g_assert_true (GTK_IS_NEVER_TRIGGER (trigger)); break; From b1327167e2117211581a02711e6ee4abbb8387ca Mon Sep 17 00:00:00 2001 From: Emmanuele Bassi Date: Fri, 27 Mar 2020 14:35:09 +0000 Subject: [PATCH 5/5] Improve GtkShortcutTrigger parsing tests Split off all possible cases into sub-tests, and add more thorough checks on the invalid strings unit. --- testsuite/gtk/shortcuts.c | 208 +++++++++++++++++++++++++++++--------- 1 file changed, 158 insertions(+), 50 deletions(-) diff --git a/testsuite/gtk/shortcuts.c b/testsuite/gtk/shortcuts.c index fca321a49f..1f235b71ff 100644 --- a/testsuite/gtk/shortcuts.c +++ b/testsuite/gtk/shortcuts.c @@ -151,76 +151,180 @@ test_trigger_equal (void) } static void -test_trigger_parse (void) +test_trigger_parse_never (void) { - enum - { - TRIGGER_KEYVAL, - TRIGGER_MNEMONIC, - TRIGGER_ALT, - TRIGGER_NEVER, - TRIGGER_INVALID - }; + GtkShortcutTrigger *trigger; - struct { + trigger = gtk_shortcut_trigger_parse_string ("never"); + g_assert_true (GTK_IS_NEVER_TRIGGER (trigger)); + + g_object_unref (trigger); +} + +static void +test_trigger_parse_keyval (void) +{ + const struct + { const char *str; GdkModifierType modifiers; guint keyval; int trigger_type; } tests[] = { - { "z", GDK_CONTROL_MASK | GDK_MOD1_MASK, 'z', TRIGGER_KEYVAL }, - { "U", GDK_CONTROL_MASK, 'u', TRIGGER_KEYVAL }, - { "x", GDK_HYPER_MASK, 'x', TRIGGER_KEYVAL }, - { "y", GDK_META_MASK, 'y', TRIGGER_KEYVAL }, - { "KP_7", 0, GDK_KEY_KP_7, TRIGGER_KEYVAL }, - { "exclam", GDK_SHIFT_MASK, '!', TRIGGER_KEYVAL }, - { "never", 0, 0, TRIGGER_NEVER }, - { "_A", 0, GDK_KEY_a, TRIGGER_MNEMONIC }, - { "_s", 0, GDK_KEY_s, TRIGGER_MNEMONIC }, - { "foo", 0, 0, TRIGGER_INVALID }, - { "B", 0, 0, TRIGGER_INVALID }, - { "U|U", GDK_CONTROL_MASK, 'u', TRIGGER_ALT } + { "z", GDK_CONTROL_MASK | GDK_MOD1_MASK, 'z' }, + { "U", GDK_CONTROL_MASK, 'u' }, + { "x", GDK_HYPER_MASK, 'x' }, + { "y", GDK_META_MASK, 'y' }, + { "KP_7", 0, GDK_KEY_KP_7 }, + { "exclam", GDK_SHIFT_MASK, '!' }, }; - GtkShortcutTrigger *trigger; - int i; - for (i = 0; i < G_N_ELEMENTS (tests); i++) + for (int i = 0; i < G_N_ELEMENTS (tests); i++) { - trigger = gtk_shortcut_trigger_parse_string (tests[i].str); + g_test_message ("Checking: '%s'", tests[i].str); - switch (tests[i].trigger_type) + GtkShortcutTrigger *trigger = gtk_shortcut_trigger_parse_string (tests[i].str); + + g_assert_true (GTK_IS_KEYVAL_TRIGGER (trigger)); + g_assert_cmpint (gtk_keyval_trigger_get_modifiers (GTK_KEYVAL_TRIGGER (trigger)), + ==, + tests[i].modifiers); + g_assert_cmpuint (gtk_keyval_trigger_get_keyval (GTK_KEYVAL_TRIGGER (trigger)), + ==, + tests[i].keyval); + g_object_unref (trigger); + } +} + +static void +test_trigger_parse_mnemonic (void) +{ + struct + { + const char *str; + guint keyval; + } tests[] = { + { "_A", GDK_KEY_a }, + { "_s", GDK_KEY_s }, + }; + + for (int i = 0; i < G_N_ELEMENTS (tests); i++) + { + g_test_message ("Checking: '%s'", tests[i].str); + + GtkShortcutTrigger *trigger = gtk_shortcut_trigger_parse_string (tests[i].str); + + g_assert_true (GTK_IS_MNEMONIC_TRIGGER (trigger)); + g_assert_cmpuint (gtk_mnemonic_trigger_get_keyval (GTK_MNEMONIC_TRIGGER (trigger)), + ==, + tests[i].keyval); + g_object_unref (trigger); + } +} + +static void +test_trigger_parse_alternative (void) +{ + enum + { + TRIGGER_NEVER, + TRIGGER_KEYVAL, + TRIGGER_MNEMONIC, + TRIGGER_ALTERNATIVE + }; + + const struct + { + const char *str; + int first; + int second; + } tests[] = { + { "U|U", TRIGGER_KEYVAL, TRIGGER_KEYVAL }, + { "_U|u", TRIGGER_MNEMONIC, TRIGGER_KEYVAL }, + { "x|_x|x", TRIGGER_KEYVAL, TRIGGER_ALTERNATIVE }, + }; + + for (int i = 0; i < G_N_ELEMENTS (tests); i++) + { + g_test_message ("Checking: '%s'", tests[i].str); + + GtkShortcutTrigger *trigger = gtk_shortcut_trigger_parse_string (tests[i].str); + + g_assert_true (GTK_IS_ALTERNATIVE_TRIGGER (trigger)); + + GtkShortcutTrigger *t1 = gtk_alternative_trigger_get_first (GTK_ALTERNATIVE_TRIGGER (trigger)); + + switch (tests[i].first) { - case TRIGGER_INVALID: - g_assert_null (trigger); - break; - case TRIGGER_KEYVAL: - g_assert_true (GTK_IS_KEYVAL_TRIGGER (trigger)); - g_assert_cmpint (gtk_keyval_trigger_get_modifiers (GTK_KEYVAL_TRIGGER (trigger)), - ==, - tests[i].modifiers); - g_assert_cmpuint (gtk_keyval_trigger_get_keyval (GTK_KEYVAL_TRIGGER (trigger)), - ==, - tests[i].keyval); - break; - case TRIGGER_ALT: - g_assert_true (GTK_IS_ALTERNATIVE_TRIGGER (trigger)); - break; case TRIGGER_NEVER: - g_assert_true (GTK_IS_NEVER_TRIGGER (trigger)); + g_assert_true (GTK_IS_NEVER_TRIGGER (t1)); break; + + case TRIGGER_KEYVAL: + g_assert_true (GTK_IS_KEYVAL_TRIGGER (t1)); + break; + case TRIGGER_MNEMONIC: - g_assert_true (GTK_IS_MNEMONIC_TRIGGER (trigger)); - g_assert_cmpuint (gtk_mnemonic_trigger_get_keyval (GTK_MNEMONIC_TRIGGER (trigger)), - ==, - tests[i].keyval); + g_assert_true (GTK_IS_MNEMONIC_TRIGGER (t1)); break; + + case TRIGGER_ALTERNATIVE: + g_assert_true (GTK_IS_ALTERNATIVE_TRIGGER (t1)); + break; + default: g_assert_not_reached (); break; } - if (tests[i].trigger_type != TRIGGER_INVALID) - g_object_unref (trigger); + GtkShortcutTrigger *t2 = gtk_alternative_trigger_get_second (GTK_ALTERNATIVE_TRIGGER (trigger)); + + switch (tests[i].second) + { + case TRIGGER_NEVER: + g_assert_true (GTK_IS_NEVER_TRIGGER (t2)); + break; + + case TRIGGER_KEYVAL: + g_assert_true (GTK_IS_KEYVAL_TRIGGER (t2)); + break; + + case TRIGGER_MNEMONIC: + g_assert_true (GTK_IS_MNEMONIC_TRIGGER (t2)); + break; + + case TRIGGER_ALTERNATIVE: + g_assert_true (GTK_IS_ALTERNATIVE_TRIGGER (t2)); + break; + + default: + g_assert_not_reached (); + break; + } + + g_object_unref (trigger); + } +} + +static void +test_trigger_parse_invalid (void) +{ + const char *tests[] = { + "", + "Never", + "Foo", + "Nyaa", + "never|", + "|never", + }; + + for (int i = 0; i < G_N_ELEMENTS (tests); i++) + { + g_test_message ("Checking: '%s'", tests[i]); + + GtkShortcutTrigger *trigger = gtk_shortcut_trigger_parse_string (tests[i]); + + g_assert_null (trigger); } } @@ -344,7 +448,11 @@ main (int argc, char *argv[]) g_test_add_func ("/shortcuts/trigger/basic", test_trigger_basic); g_test_add_func ("/shortcuts/trigger/equal", test_trigger_equal); - g_test_add_func ("/shortcuts/trigger/parse", test_trigger_parse); + g_test_add_func ("/shortcuts/trigger/parse/never", test_trigger_parse_never); + g_test_add_func ("/shortcuts/trigger/parse/keyval", test_trigger_parse_keyval); + g_test_add_func ("/shortcuts/trigger/parse/mnemonic", test_trigger_parse_mnemonic); + g_test_add_func ("/shortcuts/trigger/parse/alternative", test_trigger_parse_alternative); + g_test_add_func ("/shortcuts/trigger/parse/invalid", test_trigger_parse_invalid); g_test_add_func ("/shortcuts/trigger/trigger", test_trigger_trigger); g_test_add_func ("/shortcuts/action/basic", test_action_basic); g_test_add_func ("/shortcuts/action/activate", test_action_activate);