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

[Peter Maydell]

On 17 January 2017 at 15:20, Gerd Hoffmann <address@hidden> wrote:
> On Di, 2017-01-17 at 10:44 +0000, Peter Maydell wrote:
>> On 17 January 2017 at 08:20, Gerd Hoffmann <address@hidden> wrote:
>> > On Mo, 2017-01-16 at 18:35 +0000, Peter Maydell wrote:
>> >> On 11 January 2017 at 10:58, Gerd Hoffmann <address@hidden> 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