| Message ID | 1302248640-24913-1-git-send-email-bradh@frogmouth.net |
|---|---|
| State | New |
| Headers | show |
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
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.
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.
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
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
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
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
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)
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(-)