[Qemu-devel] [PATCH] monitor: avoid moving cursor during "mouse_button" command

[Qemu-devel] [PATCH] monitor: avoid moving cursor during "mouse_button" command

[Brad Hards]

This addresses https://bugs.launchpad.net/qemu/+bug/752476 which
basically points out that using the mouse_button command causes
the mouse cursor to warp to the origin (when using absolute
pointing device).

I've tested this with a kubuntu 10.10 guest and it works fine
for me with both relative and absolute pointing devices. Note
that testing with realtive pointing device was relatively
light.

Signed-off-by: Brad Hards <bradh@frogmouth.net>
---
 monitor.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/monitor.c b/monitor.c
index f1a08dc..0ce162b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1879,6 +1879,9 @@ static void do_sendkey(Monitor *mon, const QDict *qdict)
                    muldiv64(get_ticks_per_sec(), hold_time, 1000));
 }
 
+static int mouse_x;
+static int mouse_y;
+static int mouse_z;
 static int mouse_button_state;
 
 static void do_mouse_move(Monitor *mon, const QDict *qdict)
@@ -1893,13 +1896,22 @@ static void do_mouse_move(Monitor *mon, const QDict *qdict)
     if (dz_str)
         dz = strtol(dz_str, NULL, 0);
     kbd_mouse_event(dx, dy, dz, mouse_button_state);
+    if (kbd_mouse_is_absolute()) {
+        mouse_x = dx;
+        mouse_y = dy;
+        mouse_z = dz;
+    }
 }
 
 static void do_mouse_button(Monitor *mon, const QDict *qdict)
 {
     int button_state = qdict_get_int(qdict, "button_state");
     mouse_button_state = button_state;
-    kbd_mouse_event(0, 0, 0, mouse_button_state);
+    if (kbd_mouse_is_absolute()) {
+        kbd_mouse_event(mouse_x, mouse_y, mouse_z, mouse_button_state);
+    } else {
+        kbd_mouse_event(0, 0, 0, mouse_button_state);
+    }
 }
 
 static void do_ioport_read(Monitor *mon, const QDict *qdict)
-- 
1.7.1

Re: [Qemu-devel] [PATCH] monitor: avoid moving cursor during "mouse_button" command

[Brad Hards]

On Fri, 8 Apr 2011 05:44:00 pm Brad Hards wrote:
> I've tested this with a kubuntu 10.10 guest and it works fine
> for me with both relative and absolute pointing devices. Note
> that testing with realtive pointing device was relatively
> light.
This fix (in slightly different form) was verified by the original reporter as 
fixing the problem. See https://bugs.launchpad.net/qemu/+bug/752476/comments/3

Brad

Re: [Qemu-devel] [PATCH] monitor: avoid moving cursor during "mouse_button" command

[Markus Armbruster]

Brad Hards <bradh@frogmouth.net> writes:

> This addresses https://bugs.launchpad.net/qemu/+bug/752476 which
> basically points out that using the mouse_button command causes
> the mouse cursor to warp to the origin (when using absolute
> pointing device).
>
> I've tested this with a kubuntu 10.10 guest and it works fine
> for me with both relative and absolute pointing devices. Note
> that testing with realtive pointing device was relatively
> light.
>
> Signed-off-by: Brad Hards <bradh@frogmouth.net>
> ---
>  monitor.c |   14 +++++++++++++-
>  1 files changed, 13 insertions(+), 1 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index f1a08dc..0ce162b 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1879,6 +1879,9 @@ static void do_sendkey(Monitor *mon, const QDict *qdict)
>                     muldiv64(get_ticks_per_sec(), hold_time, 1000));
>  }
>  
> +static int mouse_x;
> +static int mouse_y;
> +static int mouse_z;
>  static int mouse_button_state;
>  
>  static void do_mouse_move(Monitor *mon, const QDict *qdict)
> @@ -1893,13 +1896,22 @@ static void do_mouse_move(Monitor *mon, const QDict *qdict)
>      if (dz_str)
>          dz = strtol(dz_str, NULL, 0);
>      kbd_mouse_event(dx, dy, dz, mouse_button_state);
> +    if (kbd_mouse_is_absolute()) {
> +        mouse_x = dx;
> +        mouse_y = dy;
> +        mouse_z = dz;
> +    }
>  }
>  
>  static void do_mouse_button(Monitor *mon, const QDict *qdict)
>  {
>      int button_state = qdict_get_int(qdict, "button_state");
>      mouse_button_state = button_state;
> -    kbd_mouse_event(0, 0, 0, mouse_button_state);
> +    if (kbd_mouse_is_absolute()) {
> +        kbd_mouse_event(mouse_x, mouse_y, mouse_z, mouse_button_state);
> +    } else {
> +        kbd_mouse_event(0, 0, 0, mouse_button_state);
> +    }
>  }
>  
>  static void do_ioport_read(Monitor *mon, const QDict *qdict)

There's one instance of state: position (if absolute) + buttons for any
number of mice.  Funny things can happen when you have more than one
mouse and switch between them.

Even if there's just one mouse: the state is updated only for monitor
mouse action.  Funny things can happen when something other than monitor
commands uses the mouse.

Shouldn't the state be kept per-mouse?  Monitor could ask for current
coordinates + button state then.

Note buttons are already funny.  The patch just extends the funniness to
position.  Could be a valid excuse for committing it as is.

Re: [Qemu-devel] [PATCH] monitor: avoid moving cursor during "mouse_button" command

[Luiz Capitulino]

On Fri, 08 Apr 2011 16:34:21 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Brad Hards <bradh@frogmouth.net> writes:
> 
> > This addresses https://bugs.launchpad.net/qemu/+bug/752476 which
> > basically points out that using the mouse_button command causes
> > the mouse cursor to warp to the origin (when using absolute
> > pointing device).
> >
> > I've tested this with a kubuntu 10.10 guest and it works fine
> > for me with both relative and absolute pointing devices. Note
> > that testing with realtive pointing device was relatively
> > light.
> >
> > Signed-off-by: Brad Hards <bradh@frogmouth.net>
> > ---
> >  monitor.c |   14 +++++++++++++-
> >  1 files changed, 13 insertions(+), 1 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index f1a08dc..0ce162b 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -1879,6 +1879,9 @@ static void do_sendkey(Monitor *mon, const QDict *qdict)
> >                     muldiv64(get_ticks_per_sec(), hold_time, 1000));
> >  }
> >  
> > +static int mouse_x;
> > +static int mouse_y;
> > +static int mouse_z;
> >  static int mouse_button_state;
> >  
> >  static void do_mouse_move(Monitor *mon, const QDict *qdict)
> > @@ -1893,13 +1896,22 @@ static void do_mouse_move(Monitor *mon, const QDict *qdict)
> >      if (dz_str)
> >          dz = strtol(dz_str, NULL, 0);
> >      kbd_mouse_event(dx, dy, dz, mouse_button_state);
> > +    if (kbd_mouse_is_absolute()) {
> > +        mouse_x = dx;
> > +        mouse_y = dy;
> > +        mouse_z = dz;
> > +    }
> >  }
> >  
> >  static void do_mouse_button(Monitor *mon, const QDict *qdict)
> >  {
> >      int button_state = qdict_get_int(qdict, "button_state");
> >      mouse_button_state = button_state;
> > -    kbd_mouse_event(0, 0, 0, mouse_button_state);
> > +    if (kbd_mouse_is_absolute()) {
> > +        kbd_mouse_event(mouse_x, mouse_y, mouse_z, mouse_button_state);
> > +    } else {
> > +        kbd_mouse_event(0, 0, 0, mouse_button_state);
> > +    }
> >  }
> >  
> >  static void do_ioport_read(Monitor *mon, const QDict *qdict)
> 
> There's one instance of state: position (if absolute) + buttons for any
> number of mice.  Funny things can happen when you have more than one
> mouse and switch between them.
> 
> Even if there's just one mouse: the state is updated only for monitor
> mouse action.  Funny things can happen when something other than monitor
> commands uses the mouse.
> 
> Shouldn't the state be kept per-mouse?  Monitor could ask for current
> coordinates + button state then.
> 
> Note buttons are already funny.  The patch just extends the funniness to
> position.  Could be a valid excuse for committing it as is.

I need Gerd's input here, or anyone who has a better idea of the trade offs
involved and how this code should evolve.

Re: [Qemu-devel] [PATCH] monitor: avoid moving cursor during "mouse_button" command

[Gerd Hoffmann]

On 04/08/11 18:37, Luiz Capitulino wrote:
> On Fri, 08 Apr 2011 16:34:21 +0200
> Markus Armbruster<armbru@redhat.com>  wrote:
>
>> Brad Hards<bradh@frogmouth.net>  writes:
>>
>>> This addresses https://bugs.launchpad.net/qemu/+bug/752476 which
>>> basically points out that using the mouse_button command causes
>>> the mouse cursor to warp to the origin (when using absolute
>>> pointing device).
>>>
>>> I've tested this with a kubuntu 10.10 guest and it works fine
>>> for me with both relative and absolute pointing devices. Note
>>> that testing with realtive pointing device was relatively
>>> light.
>>>
>>> Signed-off-by: Brad Hards<bradh@frogmouth.net>
>>> ---
>>>   monitor.c |   14 +++++++++++++-
>>>   1 files changed, 13 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/monitor.c b/monitor.c
>>> index f1a08dc..0ce162b 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -1879,6 +1879,9 @@ static void do_sendkey(Monitor *mon, const QDict *qdict)
>>>                      muldiv64(get_ticks_per_sec(), hold_time, 1000));
>>>   }
>>>
>>> +static int mouse_x;
>>> +static int mouse_y;
>>> +static int mouse_z;
>>>   static int mouse_button_state;
>>>
>>>   static void do_mouse_move(Monitor *mon, const QDict *qdict)
>>> @@ -1893,13 +1896,22 @@ static void do_mouse_move(Monitor *mon, const QDict *qdict)
>>>       if (dz_str)
>>>           dz = strtol(dz_str, NULL, 0);
>>>       kbd_mouse_event(dx, dy, dz, mouse_button_state);
>>> +    if (kbd_mouse_is_absolute()) {
>>> +        mouse_x = dx;
>>> +        mouse_y = dy;
>>> +        mouse_z = dz;
>>> +    }
>>>   }
>>>
>>>   static void do_mouse_button(Monitor *mon, const QDict *qdict)
>>>   {
>>>       int button_state = qdict_get_int(qdict, "button_state");
>>>       mouse_button_state = button_state;
>>> -    kbd_mouse_event(0, 0, 0, mouse_button_state);
>>> +    if (kbd_mouse_is_absolute()) {
>>> +        kbd_mouse_event(mouse_x, mouse_y, mouse_z, mouse_button_state);
>>> +    } else {
>>> +        kbd_mouse_event(0, 0, 0, mouse_button_state);
>>> +    }
>>>   }
>>>
>>>   static void do_ioport_read(Monitor *mon, const QDict *qdict)
>>
>> There's one instance of state: position (if absolute) + buttons for any
>> number of mice.  Funny things can happen when you have more than one
>> mouse and switch between them.
>>
>> Even if there's just one mouse: the state is updated only for monitor
>> mouse action.  Funny things can happen when something other than monitor
>> commands uses the mouse.
>>
>> Shouldn't the state be kept per-mouse?  Monitor could ask for current
>> coordinates + button state then.
>>
>> Note buttons are already funny.  The patch just extends the funniness to
>> position.  Could be a valid excuse for committing it as is.
>
> I need Gerd's input here, or anyone who has a better idea of the trade offs
> involved and how this code should evolve.

I think it would be much better to keep track of the mouse position (and 
button state while being at it) in input.c instead of monitor.c.

Once this is in place it should be easy to add kbd_mouse_* functions 
which update position or buttons only, which the monitor code can use 
then to avoid the unwanted pointer warp.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH] monitor: avoid moving cursor during "mouse_button" command

[Brad Hards]

On Thursday 28 April 2011 20:46:25 Gerd Hoffmann wrote:
> I think it would be much better to keep track of the mouse position (and
> button state while being at it) in input.c instead of monitor.c.
> 
> Once this is in place it should be easy to add kbd_mouse_* functions
> which update position or buttons only, which the monitor code can use
> then to avoid the unwanted pointer warp.
This turns out to be a bit more difficult than we discussed.

The new functions work well for the monitor code side (not unexpected, since 
its essentially the same as the original code I proposed for monitor-only 
changes).

The problem is that almost all input code (in absolute mode) keeps track of 
the position itself - monitor was the exception.

So a sequence like the following:
1. Move cursor in SDL
2. Use mouse_move in monitor
3. Use mouse_button 2 in monitor
4. Click mouse in SDL
works ok up to step 3, but step 4 causes the pointer to warp back to where it 
was at the end of step 1.

So it looks like we'd have to modify all callers of kbd_mouse_event(), and the 
code paths are already a bit convoluted. As discussed on IRC, I'm a bit 
concerned about testing cocoa and spice.

Thoughts?

Brad

Re: [Qemu-devel] [PATCH] monitor: avoid moving cursor during "mouse_button" command

[Gerd Hoffmann]

   Hi,

> The problem is that almost all input code (in absolute mode) keeps track of
> the position itself - monitor was the exception.
>
> So a sequence like the following:
> 1. Move cursor in SDL
> 2. Use mouse_move in monitor
> 3. Use mouse_button 2 in monitor
> 4. Click mouse in SDL
> works ok up to step 3, but step 4 causes the pointer to warp back to where it
> was at the end of step 1.

There is DisplayState->mouse_set() which can be used to ask the UI to 
warp the pointer to some place.  When using that one you should see the 
mouse move according to the monitor command on the SDL display.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH] monitor: avoid moving cursor during "mouse_button" command

[Brad Hards]

On Sat, 9 Apr 2011 12:34:21 am Markus Armbruster wrote:
> There's one instance of state: position (if absolute) + buttons for any
> number of mice.  Funny things can happen when you have more than one
> mouse and switch between them.
For the common case (in most OS), each of the mice are mixed together. 
Switching (with the guest powered up) is pretty rare.
 
> Even if there's just one mouse: the state is updated only for monitor
> mouse action.  Funny things can happen when something other than monitor
> commands uses the mouse.
That already happens. If SDL and monitor mouse_move are both used, then "last 
wins".
 
> Shouldn't the state be kept per-mouse?  Monitor could ask for current
> coordinates + button state then.
I thought about keeping the state in input.c code, but that adds more 
complexity and probably won't work properly either (as Anthony pointed out on 
IRC), because the inputs that you've provided to the guest get modified by 
guest code (like mouse acceleration).

> Note buttons are already funny.  The patch just extends the funniness to
> position.  Could be a valid excuse for committing it as is.
Note that the diff doesn't change the behaviour of mouse_move (i.e. position). 
It just "breaks less" for the mouse_button command for the following specific 
situation:
1. You've previously used mouse_move to select the point you want
and
2. You're using an absolute pointing device.

Going back to the original bug report, with current trunk (and the common case 
of an absolute pointing device), mouse_button warps the mouse to the origin if 
you press a button. It seems less surprising to use the last position.

Brad