[Qemu-devel] [PATCH] cocoa: stop using MOUSE_EVENT_*

[Qemu-devel] [PATCH] cocoa: stop using MOUSE_EVENT_*

[Gerd Hoffmann]

No need to go the indirect route with a bitfield and mapping the bits to
INPUT_BUTTON_*.  We can use a bool array and INPUT_BUTTON_* directly
instead.

Untested, not even compiled, due to lack of a osx^Wmacos machine.
Test results are very welcome.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/cocoa.m | 46 +++++++++++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index 26d4a1c..599b899 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -66,7 +66,7 @@ typedef struct {
 
 NSWindow *normalWindow, *about_window;
 static DisplayChangeListener *dcl;
-static int last_buttons;
+static bool last_buttons[INPUT_BUTTON__MAX];
 
 int gArgc;
 char **gArgv;
@@ -511,7 +511,9 @@ QemuCocoaView *cocoaView;
 {
     COCOA_DEBUG("QemuCocoaView: handleEvent\n");
 
-    int buttons = 0;
+    bool buttons[INPUT_BUTTON__MAX] = {
+        [ 0 ... (INPUT_BUTTON__MAX-1) ] = false;
+    };
     int keycode;
     bool mouse_event = false;
     NSPoint p = [event locationInWindow];
@@ -638,34 +640,34 @@ QemuCocoaView *cocoaView;
             break;
         case NSLeftMouseDown:
             if ([event modifierFlags] & NSCommandKeyMask) {
-                buttons |= MOUSE_EVENT_RBUTTON;
+                buttons[INPUT_BUTTON_RIGHT] = true;
             } else {
-                buttons |= MOUSE_EVENT_LBUTTON;
+                buttons[INPUT_BUTTON_LEFT] = true;
             }
             mouse_event = true;
             break;
         case NSRightMouseDown:
-            buttons |= MOUSE_EVENT_RBUTTON;
+            buttons[INPUT_BUTTON_RIGHT] = true;
             mouse_event = true;
             break;
         case NSOtherMouseDown:
-            buttons |= MOUSE_EVENT_MBUTTON;
+            buttons[INPUT_BUTTON_MIDDLE] = true;
             mouse_event = true;
             break;
         case NSLeftMouseDragged:
             if ([event modifierFlags] & NSCommandKeyMask) {
-                buttons |= MOUSE_EVENT_RBUTTON;
+                buttons[INPUT_BUTTON_RIGHT] = true;
             } else {
-                buttons |= MOUSE_EVENT_LBUTTON;
+                buttons[INPUT_BUTTON_LEFT] = true;
             }
             mouse_event = true;
             break;
         case NSRightMouseDragged:
-            buttons |= MOUSE_EVENT_RBUTTON;
+            buttons[INPUT_BUTTON_RIGHT] = true;
             mouse_event = true;
             break;
         case NSOtherMouseDragged:
-            buttons |= MOUSE_EVENT_MBUTTON;
+            buttons[INPUT_BUTTON_MIDDLE] = true;
             mouse_event = true;
             break;
         case NSLeftMouseUp:
@@ -684,8 +686,11 @@ QemuCocoaView *cocoaView;
             break;
         case NSScrollWheel:
             if (isMouseGrabbed) {
-                buttons |= ([event deltaY] < 0) ?
-                    MOUSE_EVENT_WHEELUP : MOUSE_EVENT_WHEELDN;
+                if ([event deltaY] < 0) {
+                    buttons[INPUT_BUTTON_WHEEL_UP] = true;
+                } else {
+                    buttons[INPUT_BUTTON_WHEEL_DOWN] = true;
+                }
             }
             mouse_event = true;
             break;
@@ -703,15 +708,14 @@ QemuCocoaView *cocoaView;
          */
         if ((isMouseGrabbed || [[self window] isKeyWindow]) &&
             (last_buttons != buttons)) {
-            static uint32_t bmap[INPUT_BUTTON__MAX] = {
-                [INPUT_BUTTON_LEFT]       = MOUSE_EVENT_LBUTTON,
-                [INPUT_BUTTON_MIDDLE]     = MOUSE_EVENT_MBUTTON,
-                [INPUT_BUTTON_RIGHT]      = MOUSE_EVENT_RBUTTON,
-                [INPUT_BUTTON_WHEEL_UP]   = MOUSE_EVENT_WHEELUP,
-                [INPUT_BUTTON_WHEEL_DOWN] = MOUSE_EVENT_WHEELDN,
-            };
-            qemu_input_update_buttons(dcl->con, bmap, last_buttons, buttons);
-            last_buttons = buttons;
+            InputButton btn;
+
+            for (btn = 0; btn < INPUT_BUTTON__MAX; btn++) {
+                if (last_button[btn] != button[btn]) {
+                    qemu_input_queue_btn(dcl->con, btn, button[btn]);
+                    last_button[btn] = button[btn];
+                }
+            }
         }
         if (isMouseGrabbed) {
             if (isAbsoluteEnabled) {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] cocoa: stop using MOUSE_EVENT_*

[Peter Maydell]

On 11 January 2017 at 10:58, Gerd Hoffmann <kraxel@redhat.com> wrote:
> No need to go the indirect route with a bitfield and mapping the bits to
> INPUT_BUTTON_*.  We can use a bool array and INPUT_BUTTON_* directly
> instead.
>
> Untested, not even compiled, due to lack of a osx^Wmacos machine.
> Test results are very welcome.

So what does the patch gain us? We'll now loop round
that button array for every event rather than only
the ones where the button state changed, which I
don't suppose is a big deal but does somewhat incline
me towards "just leave the code the way it is"...

thanks
-- PMM

Re: [Qemu-devel] [PATCH] cocoa: stop using MOUSE_EVENT_*

[Gerd Hoffmann]

On Mo, 2017-01-16 at 18:35 +0000, Peter Maydell wrote:
> On 11 January 2017 at 10:58, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > No need to go the indirect route with a bitfield and mapping the bits to
> > INPUT_BUTTON_*.  We can use a bool array and INPUT_BUTTON_* directly
> > instead.
> >
> > Untested, not even compiled, due to lack of a osx^Wmacos machine.
> > Test results are very welcome.
> 
> So what does the patch gain us?

I can drop MOUSE_EVENT_WHEEL* without breaking the osx build.

> We'll now loop round
> that button array for every event rather than only
> the ones where the button state changed,

Well, we loop round anyway, you just don't see it directly b/c it is
hidden in the qemu_input_update_buttons() helper function.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] cocoa: stop using MOUSE_EVENT_*

[Peter Maydell]

On 17 January 2017 at 08:20, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Mo, 2017-01-16 at 18:35 +0000, Peter Maydell wrote:
>> On 11 January 2017 at 10:58, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> > No need to go the indirect route with a bitfield and mapping the bits to
>> > INPUT_BUTTON_*.  We can use a bool array and INPUT_BUTTON_* directly
>> > instead.
>> >
>> > Untested, not even compiled, due to lack of a osx^Wmacos machine.
>> > Test results are very welcome.
>>
>> So what does the patch gain us?
>
> I can drop MOUSE_EVENT_WHEEL* without breaking the osx build.

Why do you want to drop it, though? It's a valid mouse event
type, and you're not going to drop the whole MOUSE_EVENT_* set
of defines, I assume, because they're used in lots of devices.

>> We'll now loop round
>> that button array for every event rather than only
>> the ones where the button state changed,
>
> Well, we loop round anyway, you just don't see it directly b/c it is
> hidden in the qemu_input_update_buttons() helper function.

No, we only call qemu_input_update_buttons() if last_buttons != buttons.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] cocoa: stop using MOUSE_EVENT_*

[Gerd Hoffmann]

On Di, 2017-01-17 at 10:44 +0000, Peter Maydell wrote:
> On 17 January 2017 at 08:20, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > On Mo, 2017-01-16 at 18:35 +0000, Peter Maydell wrote:
> >> On 11 January 2017 at 10:58, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >> > No need to go the indirect route with a bitfield and mapping the bits to
> >> > INPUT_BUTTON_*.  We can use a bool array and INPUT_BUTTON_* directly
> >> > instead.
> >> >
> >> > Untested, not even compiled, due to lack of a osx^Wmacos machine.
> >> > Test results are very welcome.
> >>
> >> So what does the patch gain us?
> >
> > I can drop MOUSE_EVENT_WHEEL* without breaking the osx build.
> 
> Why do you want to drop it, though? It's a valid mouse event
> type, and you're not going to drop the whole MOUSE_EVENT_* set
> of defines, I assume, because they're used in lots of devices.

Dunno where the MOUSE_EVENT_* #defines originally come from, I suspect
from old ps2 mouse protocols, for historical reasons, simliar to the
numeric key codes we have in qemu which are pc scancodes too.

So, yes, I considered dropping them all and use hardware-specific
#defines instead.  But it seems at least for the left/right/middle
buttons most (all?) hardware agrees on which bit is which.  So the
attempt to drop the button defines looks like pointless churn.

For the wheel that isn't the case though, that is either not present at
all (older devices) or implemented in different ways (virtual buttons,
wheel axis, ...).

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] cocoa: stop using MOUSE_EVENT_*

[Peter Maydell]

On 17 January 2017 at 15:20, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Di, 2017-01-17 at 10:44 +0000, Peter Maydell wrote:
>> On 17 January 2017 at 08:20, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> > On Mo, 2017-01-16 at 18:35 +0000, Peter Maydell wrote:
>> >> On 11 January 2017 at 10:58, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> >> > No need to go the indirect route with a bitfield and mapping the bits to
>> >> > INPUT_BUTTON_*.  We can use a bool array and INPUT_BUTTON_* directly
>> >> > instead.
>> >> >
>> >> > Untested, not even compiled, due to lack of a osx^Wmacos machine.
>> >> > Test results are very welcome.
>> >>
>> >> So what does the patch gain us?
>> >
>> > I can drop MOUSE_EVENT_WHEEL* without breaking the osx build.
>>
>> Why do you want to drop it, though? It's a valid mouse event
>> type, and you're not going to drop the whole MOUSE_EVENT_* set
>> of defines, I assume, because they're used in lots of devices.
>
> Dunno where the MOUSE_EVENT_* #defines originally come from, I suspect
> from old ps2 mouse protocols, for historical reasons, simliar to the
> numeric key codes we have in qemu which are pc scancodes too.
>
> So, yes, I considered dropping them all and use hardware-specific
> #defines instead.  But it seems at least for the left/right/middle
> buttons most (all?) hardware agrees on which bit is which.  So the
> attempt to drop the button defines looks like pointless churn.

Thinking about this I think the issue here is that it's a bit
confused what the MOUSE_EVENT_* values actually are:
 * actual emulated-hardware bits (in protocol bytes fed to
   the guest) ?
 * bits used in the representation with which we feed
   events from the UI midlayer to the emulated devices ?
 * bits used in the representation where UI frontends
   feed events to the UI midlayer ?

I think cleanup here is worthwhile if we can get consistency
about how we're representing mouse events and what layers
of QEMU these #defines are relevant to.
(For instance hw/input/ps2.c should probably be using its
own private #defines since it wants the exact values that
go into the on-the-wire ps2 protocol bytes.)

I had to apply the patch below to get it to compile, though,
and I've only tested by sticking printfs in for what values
we're passing in, because I don't have any test images to
hand that actually use the mouse.

So if you have a plan for cleaning up all the current
users of MOUSE_EVENT_*, this patch is ok I guess, but
if you're not planning for instance to deal with the
uses in monitor.c and input-legacy.c then I think we
should keep the MOUSE_EVENT_WHEEL* for consistency
with the fact that the INPUT_BUTTON_* consider wheel
buttons to be buttons.


diff --git a/ui/cocoa.m b/ui/cocoa.m
index 599b899..b40ace4 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -511,9 +511,7 @@ QemuCocoaView *cocoaView;
 {
     COCOA_DEBUG("QemuCocoaView: handleEvent\n");

-    bool buttons[INPUT_BUTTON__MAX] = {
-        [ 0 ... (INPUT_BUTTON__MAX-1) ] = false;
-    };
+    bool buttons[INPUT_BUTTON__MAX] = { false };
     int keycode;
     bool mouse_event = false;
     NSPoint p = [event locationInWindow];
@@ -706,14 +704,13 @@ QemuCocoaView *cocoaView;
          * call below. We definitely don't want to pass that click through
          * to the guest.
          */
-        if ((isMouseGrabbed || [[self window] isKeyWindow]) &&
-            (last_buttons != buttons)) {
+        if ((isMouseGrabbed || [[self window] isKeyWindow])) {
             InputButton btn;

             for (btn = 0; btn < INPUT_BUTTON__MAX; btn++) {
-                if (last_button[btn] != button[btn]) {
-                    qemu_input_queue_btn(dcl->con, btn, button[btn]);
-                    last_button[btn] = button[btn];
+                if (last_buttons[btn] != buttons[btn]) {
+                    qemu_input_queue_btn(dcl->con, btn, buttons[btn]);
+                    last_buttons[btn] = buttons[btn];
                 }
             }
         }

thanks
-- PMM