Let mouse_button monitor command accept coordinates

The mouse_button monitor command currently results in a call like this:

     kbd_mouse_event(0, 0, 0, mouse_button_status);

For a pointer in relative mode, this means a button gets pressed (or
or released) and nothing else. However, if the pointer currently being
controlled is in absolute mode (such as -usbtablet), it means that the
pointer will warp to (0, 0), effectively limiting clicking to the top
left corner of the desktop in the guest.

To work around this, I propose (thanks to Daniel Berrange for the
suggestion) to let the mouse_button monitor command optionally accept
coordinates.

Signed-off-by: Soren Hansen <soren@canonical.com>
---
 monitor.c       |   16 +++++++++++++++-
 qemu-monitor.hx |    6 +++---
 2 files changed, 18 insertions(+), 4 deletions(-)

On Wed, Dec 09, 2009 at 12:07:50PM +0100, Soren Hansen wrote:
> The mouse_button monitor command currently results in a call like this:
> 
>      kbd_mouse_event(0, 0, 0, mouse_button_status);
> 
> For a pointer in relative mode, this means a button gets pressed (or
> or released) and nothing else. However, if the pointer currently being
> controlled is in absolute mode (such as -usbtablet), it means that the
> pointer will warp to (0, 0), effectively limiting clicking to the top
> left corner of the desktop in the guest.
> 
> To work around this, I propose (thanks to Daniel Berrange for the
> suggestion) to let the mouse_button monitor command optionally accept
> coordinates.

The patch basically looks good, but some minor changes are needed. 
Please find my comments below.

> Signed-off-by: Soren Hansen <soren@canonical.com>
> ---
>  monitor.c       |   16 +++++++++++++++-
>  qemu-monitor.hx |    6 +++---
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index e161f7d..8b879cc 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1336,8 +1336,22 @@ static void do_mouse_move(Monitor *mon, const QDict *qdict)
>  static void do_mouse_button(Monitor *mon, const QDict *qdict)
>  {
>      int button_state = qdict_get_int(qdict, "button_state");
> +    int dx, dy, dz;
> +    const char *dx_str = qdict_get_try_str(qdict, "dx_str");
> +    const char *dy_str = qdict_get_try_str(qdict, "dy_str");
> +    const char *dz_str = qdict_get_try_str(qdict, "dz_str");
> +
> +    dx = dy = dz = 0;
> +
> +    if (dx_str && dy_str) {
> +        dx = strtol(dx_str, NULL, 0);
> +        dy = strtol(dy_str, NULL, 0);

I am not sure you should do the conversion here. qdict can directly get
int values.

> +        if (dz_str)
> +            dz = strtol(dz_str, NULL, 0);

This line should be put between curly braces to confirm the CODING
STYLE.

> +    }
> +
>      mouse_button_state = button_state;
> -    kbd_mouse_event(0, 0, 0, mouse_button_state);
> +    kbd_mouse_event(dx, dy, dz, mouse_button_state);
>  }
>  
>  static void do_ioport_read(Monitor *mon, const QDict *qdict)
> diff --git a/qemu-monitor.hx b/qemu-monitor.hx
> index 2b14802..b42b461 100644
> --- a/qemu-monitor.hx
> +++ b/qemu-monitor.hx
> @@ -595,9 +595,9 @@ ETEXI
>  
>      {
>          .name       = "mouse_button",
> -        .args_type  = "button_state:i",
> -        .params     = "state",
> -        .help       = "change mouse button state (1=L, 2=M, 4=R)",
> +        .args_type  = "button_state:i,dx_str:s?,dy_str:s?,dz_str:s?",
> +        .params     = "state [dx dy [dz]]",
> +        .help       = "change mouse button state (1=L, 2=M, 4=R), optionally specifying coordinates as well (useful for pointers in absolute mode)",
>          .mhandler.cmd = do_mouse_button,
>      },
>  
> -- 
> 1.6.5
> 
> -- 
> Soren Hansen                 | 
> Lead virtualisation engineer | Ubuntu Server Team
> Canonical Ltd.               | http://www.ubuntu.com/

Aurelien Jarno <aurelien@aurel32.net> writes:

> On Wed, Dec 09, 2009 at 12:07:50PM +0100, Soren Hansen wrote:
>> The mouse_button monitor command currently results in a call like this:
>> 
>>      kbd_mouse_event(0, 0, 0, mouse_button_status);
>> 
>> For a pointer in relative mode, this means a button gets pressed (or
>> or released) and nothing else. However, if the pointer currently being
>> controlled is in absolute mode (such as -usbtablet), it means that the
>> pointer will warp to (0, 0), effectively limiting clicking to the top
>> left corner of the desktop in the guest.
>> 
>> To work around this, I propose (thanks to Daniel Berrange for the
>> suggestion) to let the mouse_button monitor command optionally accept
>> coordinates.
>
> The patch basically looks good, but some minor changes are needed. 
> Please find my comments below.
>
>> Signed-off-by: Soren Hansen <soren@canonical.com>
>> ---
>>  monitor.c       |   16 +++++++++++++++-
>>  qemu-monitor.hx |    6 +++---
>>  2 files changed, 18 insertions(+), 4 deletions(-)
>> 
>> diff --git a/monitor.c b/monitor.c
>> index e161f7d..8b879cc 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -1336,8 +1336,22 @@ static void do_mouse_move(Monitor *mon, const QDict *qdict)
>>  static void do_mouse_button(Monitor *mon, const QDict *qdict)
>>  {
>>      int button_state = qdict_get_int(qdict, "button_state");
>> +    int dx, dy, dz;
>> +    const char *dx_str = qdict_get_try_str(qdict, "dx_str");
>> +    const char *dy_str = qdict_get_try_str(qdict, "dy_str");
>> +    const char *dz_str = qdict_get_try_str(qdict, "dz_str");
>> +
>> +    dx = dy = dz = 0;
>> +
>> +    if (dx_str && dy_str) {
>> +        dx = strtol(dx_str, NULL, 0);
>> +        dy = strtol(dy_str, NULL, 0);
>
> I am not sure you should do the conversion here. qdict can directly get
> int values.

Yes, this is inappropriate for QMP.  It's copied from do_mouse_move(),
where it's just as inappropriate.  There it should look like

    int dx = (int)qdict_get_int(qdict, "dx_str");
    int dy = (int)qdict_get_int(qdict, "dy_str");
    int dz = (int)qdict_get_try_int(qdict, "dz_str", 0);

>> +        if (dz_str)
>> +            dz = strtol(dz_str, NULL, 0);
[...]