| Message ID | 20171023210803.20998-5-makovick@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/7] sdl2: Fix broken display updating after the window is hidden | expand |
Why? > --- > ui/sdl2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/ui/sdl2.c b/ui/sdl2.c > index 685e4fabec..fa54353430 100644 > --- a/ui/sdl2.c > +++ b/ui/sdl2.c > @@ -349,7 +349,7 @@ static void handle_keydown(SDL_Event *ev) > } > gui_key_modifier_pressed = mod_state; > > - if (gui_key_modifier_pressed) { > + if (gui_key_modifier_pressed && !ev->key.repeat && > qemu_console_is_graphic(scon->dcl.con)) { > switch (ev->key.keysym.scancode) { > case SDL_SCANCODE_2: > case SDL_SCANCODE_3:
This fixes the following case: 1) Ctrl-Alt-2 for console is pressed 2) console pops up and gets focus 3) console receives the Ctrl-Alt-2 keypress event 4) console closes 5) focus returns to the main window 6) main window gets the keypress event 7) goto 2 until the key is released, with 50% chance that the console window is shown at the end On Wed, Nov 1, 2017 at 11:40 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > Why? > > > --- > > ui/sdl2.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/ui/sdl2.c b/ui/sdl2.c > > index 685e4fabec..fa54353430 100644 > > --- a/ui/sdl2.c > > +++ b/ui/sdl2.c > > @@ -349,7 +349,7 @@ static void handle_keydown(SDL_Event *ev) > > } > > gui_key_modifier_pressed = mod_state; > > > > - if (gui_key_modifier_pressed) { > > + if (gui_key_modifier_pressed && !ev->key.repeat && > > qemu_console_is_graphic(scon->dcl.con)) { > > switch (ev->key.keysym.scancode) { > > case SDL_SCANCODE_2: > > case SDL_SCANCODE_3: >
On Thu, 2017-11-02 at 17:12 +0000, Jindřich Makovička wrote: > This fixes the following case: > > 1) Ctrl-Alt-2 for console is pressed > 2) console pops up and gets focus > 3) console receives the Ctrl-Alt-2 keypress event Hmm, doesn't reproduce here. Also: why should the same event be delivered twice? Ignoring autorepeat events (as the patch does, but which the commit message doesn't mention) makes sense. But I still fail to see why we should limit hotkeys to the main window. cheers, Gerd
On 3 November 2017 at 09:26, Gerd Hoffmann <kraxel@redhat.com> wrote: > Ignoring autorepeat events (as the patch does, but which the commit > message doesn't mention) makes sense. But I still fail to see why we > should limit hotkeys to the main window. Is there documentation about how the input subsystem wants to handle autorepeat? (ie how UI frontends should present repeating keys to it, and how keyboard backends should expect to see repeating keys delivered from it). There's been discussion on this in another thread about xenfb and vnc... Also I think it would be nice to have written up what the "expected" set of hotkeys for UI frontends are, so we can be consistent. I think SDL and gtk are different about what their ungrab keycombo is, for instance. (I've been assuming gtk is the "this is the right behaviour" exemplar to follow.) thanks -- PMM
On Fri, 2017-11-03 at 09:37 +0000, Peter Maydell wrote: > On 3 November 2017 at 09:26, Gerd Hoffmann <kraxel@redhat.com> wrote: > > Ignoring autorepeat events (as the patch does, but which the commit > > message doesn't mention) makes sense. But I still fail to see why > > we > > should limit hotkeys to the main window. > > Is there documentation about how the input subsystem wants to > handle autorepeat? (ie how UI frontends should present repeating > keys to it, and how keyboard backends should expect to see > repeating keys delivered from it). There's been discussion on > this in another thread about xenfb and vnc... Well, that issue didn't got much attention, probably due to the fact that the linux kernel does autorepeat in software anyway so at least for linux guests it doesn't matter at all what qemu is doing here. Currently the input layer doesn't special-case repeat events in any way, so they are just passed through to the device in case the ui submits them. From a design point of view I think it makes sense to have UIs send only keydown and keyup events and not any repeat events. Then leave it to the device emulation (i.e. ps/2 kbs) to actually generate repeat events. That is the only way the repeat rate programmed by the guest will work as expected. > Also I think it would be nice to have written up what the > "expected" set of hotkeys for UI frontends are, so we can > be consistent. I think SDL and gtk are different about what > their ungrab keycombo is, for instance. (I've been assuming > gtk is the "this is the right behaviour" exemplar to follow.) That is another mess I'll plan to look at, because I want rework the display command line switches parsing. So, the current state is: gtk ungrab is ctrl-alt-g. sdl ungrab is ctrl-alt (by default, but see below). cocoa ungrab is ctl-alt too. For reference: virt-viewer (vnc/spice client) likewise uses ctrl-alt. So it seems gtk is the odd one here. Possibly due to how hotkeys are implemented in gtk, could be a GtkAccelGroup simply doesn't support modifier-only hotkeys. Also: We have a -alt-grab switch which makes sdl use the ctrl-alt- shift modifier combo (for both ungrab and the other hotkeys). We have a -ctrl-grab switch which makes sdl use the right ctrl key (again for both ungrab and hotkeys). While being at it: We have a -no-frame switch which makes sdl disable the window frame, but it works on sdl1 only and nobody complained so far ... I'm tempted to just drop support for -alt-grab, -ctrl-grab and -no- frame switches. cheers, Gerd
On 3 November 2017 at 10:35, Gerd Hoffmann <kraxel@redhat.com> wrote: > On Fri, 2017-11-03 at 09:37 +0000, Peter Maydell wrote: >> Also I think it would be nice to have written up what the >> "expected" set of hotkeys for UI frontends are, so we can >> be consistent. I think SDL and gtk are different about what >> their ungrab keycombo is, for instance. (I've been assuming >> gtk is the "this is the right behaviour" exemplar to follow.) > > That is another mess I'll plan to look at, because I want rework the > display command line switches parsing. So, the current state is: > > gtk ungrab is ctrl-alt-g. > sdl ungrab is ctrl-alt (by default, but see below). > cocoa ungrab is ctl-alt too. > For reference: virt-viewer (vnc/spice client) likewise uses ctrl-alt. > > So it seems gtk is the odd one here. Possibly due to how hotkeys are > implemented in gtk, could be a GtkAccelGroup simply doesn't support > modifier-only hotkeys. There's a patch in-flight to switch cocoa to ctrl-alt-g. (If you think that's a bad idea it would be useful to reply to that patch.) Using plain ctrl-alt for ungrab has the disadvantage that you then can't send the guest any ctrl-alt-anything combos at all, I think. thanks -- PMM
On Fri, Nov 03, 2017 at 10:41:35AM +0000, Peter Maydell wrote: > On 3 November 2017 at 10:35, Gerd Hoffmann <kraxel@redhat.com> wrote: > > On Fri, 2017-11-03 at 09:37 +0000, Peter Maydell wrote: > >> Also I think it would be nice to have written up what the > >> "expected" set of hotkeys for UI frontends are, so we can > >> be consistent. I think SDL and gtk are different about what > >> their ungrab keycombo is, for instance. (I've been assuming > >> gtk is the "this is the right behaviour" exemplar to follow.) > > > > That is another mess I'll plan to look at, because I want rework the > > display command line switches parsing. So, the current state is: > > > > gtk ungrab is ctrl-alt-g. > > sdl ungrab is ctrl-alt (by default, but see below). > > cocoa ungrab is ctl-alt too. > > For reference: virt-viewer (vnc/spice client) likewise uses ctrl-alt. > > > > So it seems gtk is the odd one here. Possibly due to how hotkeys are > > implemented in gtk, could be a GtkAccelGroup simply doesn't support > > modifier-only hotkeys. > > There's a patch in-flight to switch cocoa to ctrl-alt-g. > (If you think that's a bad idea it would be useful to reply > to that patch.) > > Using plain ctrl-alt for ungrab has the disadvantage that you > then can't send the guest any ctrl-alt-anything combos at all, > I think. You can deal with that by only processing the ctrl-alt pair on key release, and require that the immediately preceeding key press events were also ctrl-alt. eg require the specific sequence press(ctrl), press(alt), release(alt), release(ctrl) and thus you can still do press(ctrl), press(alt), press(a), release(a) release(alt), release(ctrl) and have it go to the guest as normal Regards, Daniel
Hi, > There's a patch in-flight to switch cocoa to ctrl-alt-g. > (If you think that's a bad idea it would be useful to reply > to that patch.) I personally don't mind much. I rarely use it. With a tablet added to the guest (which I expect most people do these days) mouse grabs don't happen anyway. But when switching cocoa it makes sense to switch sdl too for consistency. > Using plain ctrl-alt for ungrab has the disadvantage that you > then can't send the guest any ctrl-alt-anything combos at all, > I think. I think it works, because SDL does the ungrab on ctrl-alt key *release*. cheers, Gerd
On 3 November 2017 at 11:01, Gerd Hoffmann <kraxel@redhat.com> wrote: > Hi, > >> There's a patch in-flight to switch cocoa to ctrl-alt-g. >> (If you think that's a bad idea it would be useful to reply >> to that patch.) > > I personally don't mind much. I rarely use it. With a tablet added to > the guest (which I expect most people do these days) mouse grabs don't > happen anyway. I don't think we have the same grab semantics exactly on all UI frontends either, so I dunno that that "doesn't grab on tablet" is true either. > But when switching cocoa it makes sense to switch sdl too for > consistency. > >> Using plain ctrl-alt for ungrab has the disadvantage that you >> then can't send the guest any ctrl-alt-anything combos at all, >> I think. > > I think it works, because SDL does the ungrab on ctrl-alt key > *release*. This is exactly the kind of fine detail it would be helpful to have written down so we can be consistent :-) thanks -- PMM
Hi, > I don't think we have the same grab semantics exactly on all > UI frontends either, so I dunno that that "doesn't grab on tablet" > is true either. Well, there are pointer grabs and keyboard grabs. Pointer grabs happen in relative mouse mode only, and I'm pretty sure we are consistent here. Keyboard grabs typically happen if the mouse is within the qemu window, so keys usually captured by the window manager go to the guest instead. Allows to alt-tab cycle through the guest windows instead of the host windows. gtk has a menu option for that to enable/disable, for sdl it is always on. Dunno about cocoa. cheers, Gerd
On 3 November 2017 at 12:07, Gerd Hoffmann <kraxel@redhat.com> wrote: > Hi, > >> I don't think we have the same grab semantics exactly on all >> UI frontends either, so I dunno that that "doesn't grab on tablet" >> is true either. > > Well, there are pointer grabs and keyboard grabs. > > Pointer grabs happen in relative mouse mode only, and I'm pretty sure > we are consistent here. Looking at the cocoa code, we grab the mouse even for absolute. Commit f61c387ea627079b3 says we did that to align with SDL. There's also the question of how grabs interact with fullscreen. thanks -- PMM
On Fri, 2017-11-03 at 12:10 +0000, Peter Maydell wrote: > On 3 November 2017 at 12:07, Gerd Hoffmann <kraxel@redhat.com> wrote: > > Hi, > > > > > I don't think we have the same grab semantics exactly on all > > > UI frontends either, so I dunno that that "doesn't grab on > > > tablet" > > > is true either. > > > > Well, there are pointer grabs and keyboard grabs. > > > > Pointer grabs happen in relative mouse mode only, and I'm pretty > > sure > > we are consistent here. > > Looking at the cocoa code, we grab the mouse even for absolute. > Commit f61c387ea627079b3 says we did that to align with SDL. That seems to be a *keyboard* grab ... cheers, Gerd
On 11/02/2017 01:12 PM, Jindřich Makovička wrote: > This fixes the following case: > > 1) Ctrl-Alt-2 for console is pressed > 2) console pops up and gets focus > 3) console receives the Ctrl-Alt-2 keypress event > 4) console closes > 5) focus returns to the main window > 6) main window gets the keypress event > 7) goto 2 until the key is released, with 50% chance that the console > window is shown at the end Sounds similar to what I reported in my comment here a while back: https://bugzilla.libsdl.org/show_bug.cgi?id=3287 - Cole
On Sun, Nov 05, 2017 at 09:10:45AM -0500, Cole Robinson wrote: > On 11/02/2017 01:12 PM, Jindřich Makovička wrote: > > This fixes the following case: > > > > 1) Ctrl-Alt-2 for console is pressed > > 2) console pops up and gets focus > > 3) console receives the Ctrl-Alt-2 keypress event > > 4) console closes > > 5) focus returns to the main window > > 6) main window gets the keypress event > > 7) goto 2 until the key is released, with 50% chance that the console > > window is shown at the end > > Sounds similar to what I reported in my comment here a while back: > https://bugzilla.libsdl.org/show_bug.cgi?id=3287 Hmm, so it seems to be a SDL bug. I'm running SDL 2.0.3 here, which explains why I can't reproduce it. According to the bug it seem to be *repeat* keydown events, so is simply ignoring repeat events enough to avoid this issue? Also: Jindřich, do you plan to send a new of the patch series with the issues discussed fixed? cheers, Gerd
Hi Gerd, I will try to respin the patch set during the weekend. Regards, Jindrich On Wed, Nov 8, 2017 at 11:10 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > On Sun, Nov 05, 2017 at 09:10:45AM -0500, Cole Robinson wrote: > > On 11/02/2017 01:12 PM, Jindřich Makovička wrote: > > > This fixes the following case: > > > > > > 1) Ctrl-Alt-2 for console is pressed > > > 2) console pops up and gets focus > > > 3) console receives the Ctrl-Alt-2 keypress event > > > 4) console closes > > > 5) focus returns to the main window > > > 6) main window gets the keypress event > > > 7) goto 2 until the key is released, with 50% chance that the console > > > window is shown at the end > > > > Sounds similar to what I reported in my comment here a while back: > > https://bugzilla.libsdl.org/show_bug.cgi?id=3287 > > Hmm, so it seems to be a SDL bug. I'm running SDL 2.0.3 here, which > explains why I can't reproduce it. According to the bug it seem to be > *repeat* keydown events, so is simply ignoring repeat events enough to > avoid this issue? > > Also: Jindřich, do you plan to send a new of the patch series with the > issues discussed fixed? > > cheers, > Gerd > > -- Jindřich Makovička
diff --git a/ui/sdl2.c b/ui/sdl2.c index 685e4fabec..fa54353430 100644 --- a/ui/sdl2.c +++ b/ui/sdl2.c @@ -349,7 +349,7 @@ static void handle_keydown(SDL_Event *ev) } gui_key_modifier_pressed = mod_state; - if (gui_key_modifier_pressed) { + if (gui_key_modifier_pressed && !ev->key.repeat && qemu_console_is_graphic(scon->dcl.con)) { switch (ev->key.keysym.scancode) { case SDL_SCANCODE_2: case SDL_SCANCODE_3: