On Fri, Jan 12, 2018 at 10:15:24PM -0500, Programmingkid wrote: > > > On Jan 10, 2018, at 11:14 AM, Daniel P. Berrange <berra...@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 <programmingk...@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 :|