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

[Programmingkid]

> On Jan 10, 2018, at 11:14 AM, Daniel P. Berrange <address@hidden> 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 <address@hidden>
>> ---
>> 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?