ioemu: fix SDL mouse events processing

PATCH New Samuel Thibault 3 participants 8 messages Mar 5, 2008 – Mar 13, 2008

Share & tools

Samuel Thibault

author

2008-03-05 11:47 UTC #0 lore raw

PATCH 1/1 ioemu: fix SDL mouse events processing

ioemu: fix SDL mouse events processing
- GetRelativeMouseState always returns the last position, so when the polling loop gets several mouse events in one go, we would send useless 'no move' events.
- So as to make sure we don't miss any mouse click / double click, we should not use GetRelativeMouseState() to get the button state, but keep records of the button state ourselves.

Signed-off-by Samuel Thibault <[email protected]>

tools/ioemu/sdl.c +12 ~~-13~~

diff -r 86e64b684fb2 tools/ioemu/sdl.c
@@ -360,6 +360,7 @@
 {
     SDL_Event ev1, *ev = &ev1;
     int mod_state;
+    int state;
                      
     if (last_vm_running != vm_running) {
         last_vm_running = vm_running;

@@ -368,6 +369,7 @@
 
     vga_hw_update();
 
+    state = SDL_GetMouseState(NULL, NULL);
     while (SDL_PollEvent(ev)) {
         switch (ev->type) {
         case SDL_VIDEOEXPOSE:

@@ -475,41 +477,38 @@
         case SDL_MOUSEMOTION:
             if (gui_grab || kbd_mouse_is_absolute() ||
                 absolute_enabled) {
-                int dx, dy, state;
-                state = SDL_GetRelativeMouseState(&dx, &dy);
-                sdl_send_mouse_event(dx, dy, 0, state);
+                int dx, dy;
+                SDL_GetRelativeMouseState(&dx, &dy);
+                if (dx || dy)
+                    sdl_send_mouse_event(dx, dy, 0, state);
             }
             break;
         case SDL_MOUSEBUTTONUP:
             if (gui_grab || kbd_mouse_is_absolute()) {
-                int dx, dy, state;
-                state = SDL_GetRelativeMouseState(&dx, &dy);
-                sdl_send_mouse_event(dx, dy, 0, state);
+                state &= ~SDL_BUTTON(ev->button.button);
+                sdl_send_mouse_event(0, 0, 0, state);
             }
             break;
         case SDL_MOUSEBUTTONDOWN:
             {
                 SDL_MouseButtonEvent *bev = &ev->button;
                 if (!gui_grab && !kbd_mouse_is_absolute()) {
-                    if (ev->type == SDL_MOUSEBUTTONDOWN &&
-                        (bev->state & SDL_BUTTON_LMASK)) {
+                    if (bev->button == SDL_BUTTON_LEFT) {
                         /* start grabbing all events */
                         sdl_grab_start();
                     }
                 } else {
-                    int dx, dy, dz, state;
+                    int dz;
                     dz = 0;
-                    state = SDL_GetRelativeMouseState(&dx, &dy);
+                    state |= SDL_BUTTON(bev->button);
 #ifdef SDL_BUTTON_WHEELUP
                     if (bev->button == SDL_BUTTON_WHEELUP) {
                         dz = -1;
                     } else if (bev->button == SDL_BUTTON_WHEELDOWN) {
                         dz = 1;
-                    } else {
-                        state = bev->button | state;
                     }
 #endif               
-                    sdl_send_mouse_event(dx, dy, dz, state);
+                    sdl_send_mouse_event(0, 0, dz, state);
                 }
             }
             break;

Samuel Thibault

author

2008-03-05 12:18 UTC #1 lore raw

PATCH 1/1 fix SDL mouse events processing

Hello,

The patch below fixes SDL mouse events processing:
- GetRelativeMouseState always returns the last position, so when the polling loop gets several mouse events in one go, we would send useless 'no move' events.
- So as to make sure we don't miss any mouse click / double click, we should not use GetRelativeMouseState() to get the button state, but rather keep records of the button state ourselves (I've requested SDL developers to provide it directly in the event in SDL 1.3).

Index: sdl.c
===================================================================
RCS file: /sources/qemu/qemu/sdl.c,v
retrieving revision 1.45

sdl.c +14 -6

diff -u -p -r1.45 sdl.c
@@ -290,10 +290,9 @@ static void sdl_grab_end(void)
     sdl_update_caption();
 }
 
-static void sdl_send_mouse_event(int dz)
+static void sdl_send_mouse_event(int dx, int dy, int dz, int state)
 {
-    int dx, dy, state, buttons;
-    state = SDL_GetRelativeMouseState(&dx, &dy);
+    int buttons;
     buttons = 0;
     if (state & SDL_BUTTON(SDL_BUTTON_LEFT))
         buttons |= MOUSE_EVENT_LBUTTON;

@@ -347,6 +346,7 @@ static void sdl_refresh(DisplayState *ds
 {
     SDL_Event ev1, *ev = &ev1;
     int mod_state;
+    int state;
 
     if (last_vm_running != vm_running) {
         last_vm_running = vm_running;

@@ -355,6 +355,7 @@ static void sdl_refresh(DisplayState *ds
 
     vga_hw_update();
 
+    state = SDL_GetMouseState(NULL, NULL);
     while (SDL_PollEvent(ev)) {
         switch (ev->type) {
         case SDL_VIDEOEXPOSE:

@@ -474,16 +475,23 @@ static void sdl_refresh(DisplayState *ds
         case SDL_MOUSEMOTION:
             if (gui_grab || kbd_mouse_is_absolute() ||
                 absolute_enabled) {
-                sdl_send_mouse_event(0);
+                int dx, dy;
+                SDL_GetRelativeMouseState(&dx, &dy);
+                if (dx || dy)
+                    sdl_send_mouse_event(dx, dy, 0, state);
             }
             break;
         case SDL_MOUSEBUTTONDOWN:
         case SDL_MOUSEBUTTONUP:
             {
                 SDL_MouseButtonEvent *bev = &ev->button;
+                if (ev->type == SDL_MOUSEBUTTONDOWN)
+                    state |= SDL_BUTTON(bev->button);
+                else
+                    state &= ~SDL_BUTTON(ev->button.button);
                 if (!gui_grab && !kbd_mouse_is_absolute()) {
                     if (ev->type == SDL_MOUSEBUTTONDOWN &&
-                        (bev->state & SDL_BUTTON_LMASK)) {
+                        (bev->button == SDL_BUTTON_LEFT)) {
                         /* start grabbing all events */
                         sdl_grab_start();
                     }

@@ -497,7 +505,7 @@ static void sdl_refresh(DisplayState *ds
                         dz = 1;
                     }
 #endif
-                    sdl_send_mouse_event(dz);
+                    sdl_send_mouse_event(0, 0, dz, state);
                 }
             }
             break;

Johannes Schindelin

2008-03-05 13:09 UTC #2 lore raw

Re: [Qemu-devel] [PATCH] fix SDL mouse events processing

Hi,

Quoted 17 lines · Samuel Thibault

The patch below fixes SDL mouse events processing:
- GetRelativeMouseState always returns the last position, so when the polling loop gets several mouse events in one go, we would send useless 'no move' events.
- So as to make sure we don't miss any mouse click / double click, we should not use GetRelativeMouseState() to get the button state, but rather keep records of the button state ourselves (I've requested SDL developers to provide it directly in the event in SDL 1.3).
Index: sdl.c
===================================================================
RCS file: /sources/qemu/qemu/sdl.c,v
retrieving revision 1.45
diff -u -p -r1.45 sdl.c

sdl.c static void sdl_refresh(DisplayState *ds · line 358

Quoted 4 lines

sdl.c +1 -0

@@ -355,6 +355,7 @@ static void sdl_refresh(DisplayState *ds
 
     vga_hw_update();
 
+    state = SDL_GetMouseState(NULL, NULL);

What is this good for? (I imagine that it would make sense to add a comment to document why this is here, for clueless people like me.)

Maybe it is to initialise the state of the mouse buttons? In that case, it might make sense to rename the variable to something less generic.

sdl.c static void sdl_refresh(DisplayState *ds · line 481

Quoted 9 lines

sdl.c +4 -1

@@ -474,16 +475,23 @@ static void sdl_refresh(DisplayState *ds
         case SDL_MOUSEMOTION:
             if (gui_grab || kbd_mouse_is_absolute() ||
                 absolute_enabled) {
-                sdl_send_mouse_event(0);
+                int dx, dy;
+                SDL_GetRelativeMouseState(&dx, &dy);
+                if (dx || dy)
+                    sdl_send_mouse_event(dx, dy, 0, state);

This means that you avoid sending (0,0) events. Good.

Quoted 4 lines

sdl.c +0 -0

             }
             break;
         case SDL_MOUSEBUTTONDOWN:
         case SDL_MOUSEBUTTONUP:

sdl.c static void sdl_refresh(DisplayState *ds · line 494

Quoted 10 lines

sdl.c +5 -1

             {
                 SDL_MouseButtonEvent *bev = &ev->button;
+                if (ev->type == SDL_MOUSEBUTTONDOWN)
+                    state |= SDL_BUTTON(bev->button);
+                else
+                    state &= ~SDL_BUTTON(ev->button.button);
                 if (!gui_grab && !kbd_mouse_is_absolute()) {
                     if (ev->type == SDL_MOUSEBUTTONDOWN &&
-                        (bev->state & SDL_BUTTON_LMASK)) {
+                        (bev->button == SDL_BUTTON_LEFT)) {

Is this change necessary?

Thanks,
Dscho

Samuel Thibault

author

2008-03-05 13:52 UTC #3 lore raw

Re: [Qemu-devel] [PATCH] fix SDL mouse events processing

Quoted 4 lines · Johannes Schindelin, le Wed 05 Mar 2008 14:09:10 +0100, a écrit :

What is this good for? (I imagine that it would make sense to add a comment to document why this is here, for clueless people like me.)
Maybe it is to initialise the state of the mouse buttons?

That's it.

Quoted 1 line

This means that you avoid sending (0,0) events. Good.

Yes, that's what I said in my comments.

Quoted 11 lines

sdl.c static void sdl_refresh(DisplayState *ds · line 494

Quoted 9 lines

                 SDL_MouseButtonEvent *bev = &ev->button;

sdl.c +5 -1

+                if (ev->type == SDL_MOUSEBUTTONDOWN)
+                    state |= SDL_BUTTON(bev->button);
+                else
+                    state &= ~SDL_BUTTON(ev->button.button);
                 if (!gui_grab && !kbd_mouse_is_absolute()) {
                     if (ev->type == SDL_MOUSEBUTTONDOWN &&
-                        (bev->state & SDL_BUTTON_LMASK)) {
+                        (bev->button == SDL_BUTTON_LEFT)) {

Is this change necessary?

It's actually a bug fix: state doesn't contain the button state but the 0/1 according to the event being a press or release event. Yes, that's duplicate information from SDL.

Samuel

Samuel Thibault

author

2008-03-05 13:55 UTC #4 lore raw

Re: [Qemu-devel] [PATCH] fix SDL mouse events processing

In reply to Johannes Schindelin

Here is a revamped patch:

This fixes SDL mouse events processing:
- GetRelativeMouseState() always returns the last position, so when the polling loop gets several mouse events in one go, we would send useless 'no move' events, let's avoid that.
- So as to make sure we don't miss any mouse click / double click, we should not use GetRelativeMouseState() to get the button state, but rather keep records of the button state ourselves (I've requested SDL developers to provide it directly in the event in SDL 1.3).
- bev->state doesn't contain the button state but whether the event is a press or a release. Use bev->button instead.

Index: sdl.c
===================================================================
RCS file: /sources/qemu/qemu/sdl.c,v
retrieving revision 1.45

sdl.c +14 -6

diff -u -p -r1.45 sdl.c
@@ -290,10 +290,9 @@ static void sdl_grab_end(void)
     sdl_update_caption();
 }
 
-static void sdl_send_mouse_event(int dz)
+static void sdl_send_mouse_event(int dx, int dy, int dz, int state)
 {
-    int dx, dy, state, buttons;
-    state = SDL_GetRelativeMouseState(&dx, &dy);
+    int buttons;
     buttons = 0;
     if (state & SDL_BUTTON(SDL_BUTTON_LEFT))
         buttons |= MOUSE_EVENT_LBUTTON;

@@ -347,6 +346,7 @@ static void sdl_refresh(DisplayState *ds
 {
     SDL_Event ev1, *ev = &ev1;
     int mod_state;
+    int button_state;
 
     if (last_vm_running != vm_running) {
         last_vm_running = vm_running;

@@ -355,6 +355,7 @@ static void sdl_refresh(DisplayState *ds
 
     vga_hw_update();
 
+    button_state = SDL_GetMouseState(NULL, NULL);
     while (SDL_PollEvent(ev)) {
         switch (ev->type) {
         case SDL_VIDEOEXPOSE:

@@ -474,16 +475,23 @@ static void sdl_refresh(DisplayState *ds
         case SDL_MOUSEMOTION:
             if (gui_grab || kbd_mouse_is_absolute() ||
                 absolute_enabled) {
-                sdl_send_mouse_event(0);
+                int dx, dy;
+                SDL_GetRelativeMouseState(&dx, &dy);
+                if (dx || dy)
+                    sdl_send_mouse_event(dx, dy, 0, button_state);
             }
             break;
         case SDL_MOUSEBUTTONDOWN:
         case SDL_MOUSEBUTTONUP:
             {
                 SDL_MouseButtonEvent *bev = &ev->button;
+                if (ev->type == SDL_MOUSEBUTTONDOWN)
+                    button_state |= SDL_BUTTON(bev->button);
+                else
+                    button_state &= ~SDL_BUTTON(ev->button.button);
                 if (!gui_grab && !kbd_mouse_is_absolute()) {
                     if (ev->type == SDL_MOUSEBUTTONDOWN &&
-                        (bev->state & SDL_BUTTON_LMASK)) {
+                        (bev->button == SDL_BUTTON_LEFT)) {
                         /* start grabbing all events */
                         sdl_grab_start();
                     }

@@ -497,7 +505,7 @@ static void sdl_refresh(DisplayState *ds
                         dz = 1;
                     }
 #endif
-                    sdl_send_mouse_event(dz);
+                    sdl_send_mouse_event(0, 0, dz, button_state);
                 }
             }
             break;

Johannes Schindelin

2008-03-05 14:08 UTC #5 lore raw

Re: [Qemu-devel] [PATCH] fix SDL mouse events processing

Hi,

Quoted 1 line · Samuel Thibault

Here is a revamped patch:

Thanks, and thanks for the explanations, too.

Ciao,
Dscho

Aurelien Jarno

2008-03-13 19:50 UTC #6 lore raw

Re: [Qemu-devel] [PATCH] fix SDL mouse events processing

In reply to Samuel Thibault

Quoted 12 lines · Samuel Thibault

Here is a revamped patch:
This fixes SDL mouse events processing:
- GetRelativeMouseState() always returns the last position, so when the polling loop gets several mouse events in one go, we would send useless 'no move' events, let's avoid that.
- So as to make sure we don't miss any mouse click / double click, we should not use GetRelativeMouseState() to get the button state, but rather keep records of the button state ourselves (I've requested SDL developers to provide it directly in the event in SDL 1.3).
- bev->state doesn't contain the button state but whether the event is a press or a release. Use bev->button instead.

This patch does not apply anymore. Could you please to redo it against the current CVS?

Signature · 4 lines

  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   [email protected]         | [email protected]
   `-    people.debian.org/~aurel32 | www.aurel32.net

Samuel Thibault

author

2008-03-13 23:37 UTC #7 lore raw

Re: [Qemu-devel] [PATCH] fix SDL mouse events processing

Quoted 12 lines · Aurelien Jarno, le Thu 13 Mar 2008 20:50:51 +0100, a écrit :

Quoted 10 lines · Samuel Thibault
This fixes SDL mouse events processing:
- GetRelativeMouseState() always returns the last position, so when the polling loop gets several mouse events in one go, we would send useless 'no move' events, let's avoid that.
- So as to make sure we don't miss any mouse click / double click, we should not use GetRelativeMouseState() to get the button state, but rather keep records of the button state ourselves (I've requested SDL developers to provide it directly in the event in SDL 1.3).
- bev->state doesn't contain the button state but whether the event is a press or a release. Use bev->button instead.

Quoted 2 lines

This patch does not apply anymore. Could you please to redo it against the current CVS?

Mmm, well, it actually is conflicting with the "mouse smoothness" patch:
the question is: after the 30ms period, if we got several mouse motion events, should we merge them into a single one for the guest, or should we provide all of them (hence making the cursor looking more smooth, but requiring more treatments from the guest)?

Samuel