Re: [Qemu-devel] [PATCH v4] Add ability for user to specify mouse ungrab key

["Daniel P. Berrange" <berrange@redhat.com>]

On Fri, Jan 12, 2018 at 10:15:24PM -0500, Programmingkid wrote:
> 
> > On Jan 10, 2018, at 11:14 AM, Daniel P. Berrange <berrange@redhat.com> wrote:
> > 
> > On Tue, Dec 26, 2017 at 08:14:28PM -0500, John Arbuckle wrote:
> >> Currently the ungrab keys for the Cocoa and GTK interface are Control-Alt-g.
> >> This combination may not be very fun for the user to have to enter, so we
> >> now enable the user to specify their own key(s) as the ungrab key(s). The
> >> list of keys that can be used is found in the file qapi/ui.json under QKeyCode.
> >> The max number of keys that can be used is three. The original ungrab keys
> >> still remain usable because there is a real risk of the user forgetting 
> >> the keys he or she specified as the ungrab keys. They remain as a plan B
> >> if plan A is forgotten.
> > 
> > This is a bad idea. A key reason to give a custom ungrab sequence, is because
> > the user wants to be able to use the default ungrab sequence inside the
> > guest. Always having the default ungrab sequence active prevents this.
> 
> Sounds like a great idea.
> 
> > 
> >> Syntax: -ungrab <key-key-key>
> >> 
> >> Example usage:  -ungrab home
> >> 	        -ungrab shift-ctrl
> >> 		-ungrab ctrl-x
> >> 		-ungrab pgup-pgdn
> >> 		-ungrab kp_5-kp_6
> >> 		-ungrab kp_4-kp_5-kp_6
> > 
> > I'm in two minds about using '-' as a separator.  Perhaps '+' is a better
> > choice ?
> 
> I think '-' is better because the user isn't required to push the shift key or order to see the symbol unlike '+'.
> 
> > 
> >> 
> >> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
> >> ---
> >> v4 changes:
> >> - Removed initialization code for key_value_array.
> >> - Added void keyword to console_ungrab_key_sequence(),
> >>  and console_ungrab_key_string() functions.
> >> 
> >> v3 changes:
> >> - Added the ability for any "sendkey supported" key to be used.
> >> - Added ability for one to three key sequences to be used.
> >> 
> >> v2 changes:
> >> - Removed the "int i" code from the for loops. 
> >> 
> >> include/ui/console.h |  6 ++++++
> >> qemu-options.hx      |  2 ++
> >> ui/cocoa.m           | 48 +++++++++++++++++++++++++++++++++++++++--
> >> ui/console.c         | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> vl.c                 |  3 +++
> >> 5 files changed, 117 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/include/ui/console.h b/include/ui/console.h
> >> index 580dfc57ee..37dc150268 100644
> >> --- a/include/ui/console.h
> >> +++ b/include/ui/console.h
> >> @@ -534,4 +534,10 @@ static inline void early_gtk_display_init(int opengl)
> >> /* egl-headless.c */
> >> void egl_headless_init(void);
> >> 
> >> +/* console.c */
> >> +void set_ungrab_seq(const char *new_seq);
> >> +int *console_ungrab_key_sequence(void);
> >> +const char *console_ungrab_key_string(void);
> >> +int console_ungrab_sequence_length(void);
> > 
> > We don't need to have a console_ungrab_sequence_length() method if
> > we make the array returned by console_ungrab_key_sequence() be a
> > zero terminated array.
> 
> I kind of liked having it but calculating it once in the front-end isn't a problem.
> 
> > 
> >> diff --git a/ui/cocoa.m b/ui/cocoa.m
> >> index 330ccebf90..412a5fc02d 100644
> >> --- a/ui/cocoa.m
> >> +++ b/ui/cocoa.m
> >> @@ -303,6 +303,7 @@ - (float) cdx;
> >> - (float) cdy;
> >> - (QEMUScreen) gscreen;
> >> - (void) raiseAllKeys;
> >> +- (bool) user_ungrab_seq;
> >> @end
> >> 
> >> QemuCocoaView *cocoaView;
> >> @@ -674,9 +675,24 @@ - (void) handleEvent:(NSEvent *)event
> >>                 }
> >>             }
> >> 
> >> +            /*
> >> +             * This code has to be here because the user might use a modifier
> >> +             * key like shift as an ungrab key.
> >> +             */
> >> +            if ([self user_ungrab_seq] == true) {
> >> +                [self ungrabMouse];
> >> +                return;
> >> +            }
> >>             break;
> >>         case NSEventTypeKeyDown:
> >>             keycode = cocoa_keycode_to_qemu([event keyCode]);
> >> +            [self toggleModifier: keycode];
> >> +
> >> +            // If the user is issuing the custom ungrab key sequence
> >> +            if ([self user_ungrab_seq] == true) {
> >> +                [self ungrabMouse];
> >> +                return;
> >> +            }
> > 
> > There's a small benefit to only processing the grab sequence in the
> > Up event, rather than Down event if we are clever with tracking
> > the key sequence.
> > 
> > Check if each press as it comes in. If it is part of the ungrab
> > sequence, then record that is is pressed. If is is not part of
> > the ungrab sequence, the clear out all previously pressed keys.
> > Only trigger ungrab upon key release
> > 
> > This means that you can use Ctrl+Alt  as your ungrab sequence
> > and still be able to press Ctrl+Alt+F1 and have it go to the
> > guest without triggering the grab sequence.
> > 
> > ie, in your patch we get
> > 
> > down(ctrl)
> > down(alt)
> > ..ungrab activates..
> > 
> > with my suggest we would get
> > 
> > down(ctrl)
> > down(alt)
> > up(ctrl)
> > up(alt)
> > ..ungrab activates..
> > 
> > down(ctrl)
> > down(alt)
> > down(f1)
> > up(ctrl)
> > up(alt)
> > up(f1)
> > ..no ungrab activates..
> > 
> > to do this I think you need a separate array for tracking the grab
> > instead of reusing the  toggleModifier() tracking.
> 
> This is a challenge. I am currently coming close to being able to do this. 
> 
> snip
> 
> >> 
> >> +/* Sets the mouse ungrab key sequence to what the user wants */
> >> +void set_ungrab_seq(const char *new_seq)
> >> +{
> >> +    char *buffer1 = (char *) malloc(strlen(new_seq) * sizeof(char));
> >> +    char *buffer2 = (char *) malloc(strlen(new_seq) * sizeof(char));
> >> +    int count = 0;
> >> +    int key_value;
> >> +    char *token;
> >> +    const char *separator = "-";  /* What the user places between keys */
> >> +
> >> +    sprintf(buffer1, "%s", new_seq); /* edited by strtok */
> >> +    sprintf(buffer2, "%s", new_seq); /* used for ungrab_key_string */
> >> +    ungrab_key_string = buffer2;
> >> +
> >> +    token = strtok(buffer1, separator);
> >> +    while (token != NULL && count < max_keys) {
> >> +        /* Translate the names into Q_KEY_CODE values */
> >> +        key_value = index_from_key(token, strlen(token));
> >> +        if (key_value == Q_KEY_CODE__MAX) {
> >> +            printf("-ungrab: unknown key: %s\n", token);
> >> +            exit(EXIT_FAILURE);
> >> +        }
> >> +        key_value_array[count] = key_value;
> >> +
> >> +        count++;
> >> +        token = strtok(NULL, separator);
> >> +    }
> > 
> > Rather than this malloc+sprintf+strtok mix, you can just use the
> > glib function  g_strsplit() to break it into tokens.
> 
> I really like these functions because they are so familiar and part of the ANSI standard.
> 
>  CC      qga/qapi-generated/qga-qmp-marshal.o
> rm spapr-rtas.img spapr-rtas.o
>  CC      qemu-img.o
> /var/tmp/patchew-tester-tmp-m3pgevh1/src/ui/console.c:70:12: error: variably modified ‘key_value_array’ at file scope
> static int key_value_array[max_keys + 1];
>            ^
> make: *** [ui/console.o] Error 1
> make: *** Waiting for unfinished jobs....
> === OUTPUT END ===
> 
> Test command exited with code: 2
> 
> Recently the automated patch checking system sent me this error. GCC on my
> system doesn't have a problem with the code. Would you know a way to get
> around this issue?

IIUC, it does not like the fact that you are using a variable 'max_keys' to
declare the array size. I'd suggest using a constant instead, ie turning it
to #define MAX_KEYS 3


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|