On Tue, 2016-05-31 at 01:04 +0200, Fabiano Fidêncio wrote:
> On Mon, May 30, 2016 at 5:08 PM, Pavel Grunt <pgrunt@xxxxxxxxxx> wrote:
> > Related: rhbz#1339572
> > ---
> > src/virt-viewer-app.c | 5 +++++
> > tests/test-hotkeys.c | 4 ++++
> > 2 files changed, 9 insertions(+)
> >
> > diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
> > index d449f72..f8fba4c 100644
> > --- a/src/virt-viewer-app.c
> > +++ b/src/virt-viewer-app.c
> > @@ -2077,6 +2077,11 @@ virt_viewer_app_set_hotkeys(VirtViewerApp *self,
> > const gchar *hotkeys_str)
> > gtk_accelerator_parse(accel, &accel_key, &accel_mods);
> > g_free(accel);
> >
> > + if (strlen(key + 1) != 0 && accel_key == 0 && accel_mods == 0) {
>
> Not your fault, but this key + 1 made me open the code and double
> check what's going on.
> gchar *key = strstr(*hotkey, "=");
> *key = '\0';
>
you want to separate key from value, so you replace '=' by '\0' ("key\0value").
If you print *hotkey, you get only the first part.
> Why? :-)
>
> key++; would be way cleaner as we ened up using it here in your patch
> and somewhere below in the same loop.
ok, I will use key++
>
> Okay, getting back to your patch ... strlen(key + 1) != 0 ... does it
> really happen?
it should always happen.
The condition says - if value_str is not empty and accel_key=0 and accel_key=0
then it means the value could not be parsed as gtk accelerator.
> what's the case?
one can use: virt-viewer --hotkeys="toggle-fullscreen=not_an_accelerator"
which is not consider as a bug in the current code.
> I would like to see it in the commit
> log. IMO, It shouldn't happen at all. We shouldn't receive a string
> that is: "foo=" and if it does and it is provided by oVirt/RHEVM, the
> bug is there not here :-\.
It is not about rhevm, the bug is about informing the user about incorrect usage
of hotkeys - one can have a typo in commandline and it can be hard to figure out
the issue (I know it is a stupid example).
> Even though, I understand we should treat
> this case ... so I'd go for: if (key == NULL || ++key == NULL)
That way you would not set *key = '\0', and if key is not NULL then key + 1 is
also not NULL.
> and let
> it reach the g_warn_reached() part of the code that is just above this
> part you touched.
>
> About accel_key and accel_mode being 0, again, it only will happen if
> the parser fails. So, having the ++key == NULL in the if would save is
> this part of the code.
Ah you mean *(++key) == '\0'
>
> > + g_warning("Invalid value '%s' for key '%s'", key + 1, *hotkey);
> > + continue;
> > + }
> > +
> > if (g_str_equal(*hotkey, "toggle-fullscreen")) {
> > gtk_accel_map_change_entry("<virt-viewer>/view/toggle-
> > fullscreen", accel_key, accel_mods, TRUE);
> > } else if (g_str_equal(*hotkey, "release-cursor")) {
> > diff --git a/tests/test-hotkeys.c b/tests/test-hotkeys.c
> > index d3658f5..4a45216 100644
> > --- a/tests/test-hotkeys.c
> > +++ b/tests/test-hotkeys.c
> > @@ -96,6 +96,10 @@ test_hotkeys_bad(void)
> > "toggle-fullscreen=A,unknown_command=B",
> > G_LOG_LEVEL_WARNING,
> > "Unknown hotkey command unknown_command"
> > + },{
> > + "secure-attention=value",
> > + G_LOG_LEVEL_WARNING,
> > + "Invalid value 'value' for key 'secure-attention'"
> > },
> > };
> >
> > --
> > 2.8.3
> >
> > _______________________________________________
> > virt-tools-list mailing list
> > virt-tools-list@xxxxxxxxxx
> > https://www.redhat.com/mailman/listinfo/virt-tools-list
>
> ACK the code if it follows the suggestion (but feel free to convince
> me or here or on the iRC) :-)
I would rather send v2 :)
Pavel
>
> Best Regards,
> --
> Fabiano Fidêncio
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
