[Qemu-devel] [PATCH] ui/cocoa.m: fix sending mouse event to guest

[Qemu-devel] [PATCH] ui/cocoa.m: fix sending mouse event to guest

[Programmingkid]

The mouse down event should not be sent to the guest if the mouse down event
causes an activation of QEMU. This patch prevents activation clicks from going
to the guest.

Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
---
 ui/cocoa.m | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index 60a7c07..07d9c86 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -698,7 +698,7 @@ QemuCocoaView *cocoaView;
          * call below. We definitely don't want to pass that click through
          * to the guest.
          */
-        if ((isMouseGrabbed || [[self window] isKeyWindow]) &&
+        if ((isMouseGrabbed && [[self window] isKeyWindow]) &&
             (last_buttons != buttons)) {
             static uint32_t bmap[INPUT_BUTTON__MAX] = {
                 [INPUT_BUTTON_LEFT]       = MOUSE_EVENT_LBUTTON,
-- 
2.7.2

Re: [Qemu-devel] [PATCH] ui/cocoa.m: fix sending mouse event to guest

[Peter Maydell]

On 2 April 2016 at 17:56, Programmingkid <programmingkidx@gmail.com> wrote:
> The mouse down event should not be sent to the guest if the mouse down event
> causes an activation of QEMU. This patch prevents activation clicks from going
> to the guest.
>
> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
> ---
>  ui/cocoa.m | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index 60a7c07..07d9c86 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -698,7 +698,7 @@ QemuCocoaView *cocoaView;
>           * call below. We definitely don't want to pass that click through
>           * to the guest.
>           */
> -        if ((isMouseGrabbed || [[self window] isKeyWindow]) &&
> +        if ((isMouseGrabbed && [[self window] isKeyWindow]) &&
>              (last_buttons != buttons)) {
>              static uint32_t bmap[INPUT_BUTTON__MAX] = {
>                  [INPUT_BUTTON_LEFT]       = MOUSE_EVENT_LBUTTON,
> --
> 2.7.2

I'm afraid I don't really understand why you think this
should change. On the face of it the current code looks right:
we pass through the mouse button if:
   (1) we've got the mouse grab
or (2) our window has the focus, even if it's not grabbed

I would expect the "activation click" to be "we don't have
the mouse grab, and we don't have focus either (some other
app is foreground)".

thanks
-- PMM

Re: [Qemu-devel] [PATCH] ui/cocoa.m: fix sending mouse event to guest

[Programmingkid]

On Apr 2, 2016, at 1:07 PM, Peter Maydell wrote:

> On 2 April 2016 at 17:56, Programmingkid <programmingkidx@gmail.com> wrote:
>> The mouse down event should not be sent to the guest if the mouse down event
>> causes an activation of QEMU. This patch prevents activation clicks from going
>> to the guest.
>> 
>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>> ---
>> ui/cocoa.m | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/ui/cocoa.m b/ui/cocoa.m
>> index 60a7c07..07d9c86 100644
>> --- a/ui/cocoa.m
>> +++ b/ui/cocoa.m
>> @@ -698,7 +698,7 @@ QemuCocoaView *cocoaView;
>>          * call below. We definitely don't want to pass that click through
>>          * to the guest.
>>          */
>> -        if ((isMouseGrabbed || [[self window] isKeyWindow]) &&
>> +        if ((isMouseGrabbed && [[self window] isKeyWindow]) &&
>>             (last_buttons != buttons)) {
>>             static uint32_t bmap[INPUT_BUTTON__MAX] = {
>>                 [INPUT_BUTTON_LEFT]       = MOUSE_EVENT_LBUTTON,
>> --
>> 2.7.2
> 
> I'm afraid I don't really understand why you think this
> should change. On the face of it the current code looks right:
> we pass through the mouse button if:
>   (1) we've got the mouse grab
> or (2) our window has the focus, even if it's not grabbed
> 
> I would expect the "activation click" to be "we don't have
> the mouse grab, and we don't have focus either (some other
> app is foreground)".

When QEMU's main window is in the background and the user clicks on it, the NSLeftMouseUp case in the handleEvent: method will set the isMouseGrabbed variable to true. This means the "if ((isMouseGrabbed || [[self window] isKeyWindow]) && (last_buttons != buttons))" code will always be true for a left mouse button click. The mouse event will be sent to the guest when it shouldn't be. 

Re: [Qemu-devel] [PATCH] ui/cocoa.m: fix sending mouse event to guest

[Peter Maydell]

On 2 April 2016 at 18:25, Programmingkid <programmingkidx@gmail.com> wrote:
>
> On Apr 2, 2016, at 1:07 PM, Peter Maydell wrote:
>
>> On 2 April 2016 at 17:56, Programmingkid <programmingkidx@gmail.com> wrote:
>>> The mouse down event should not be sent to the guest if the mouse down event
>>> causes an activation of QEMU. This patch prevents activation clicks from going
>>> to the guest.
>>>
>>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>>> ---
>>> ui/cocoa.m | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/ui/cocoa.m b/ui/cocoa.m
>>> index 60a7c07..07d9c86 100644
>>> --- a/ui/cocoa.m
>>> +++ b/ui/cocoa.m
>>> @@ -698,7 +698,7 @@ QemuCocoaView *cocoaView;
>>>          * call below. We definitely don't want to pass that click through
>>>          * to the guest.
>>>          */
>>> -        if ((isMouseGrabbed || [[self window] isKeyWindow]) &&
>>> +        if ((isMouseGrabbed && [[self window] isKeyWindow]) &&
>>>             (last_buttons != buttons)) {
>>>             static uint32_t bmap[INPUT_BUTTON__MAX] = {
>>>                 [INPUT_BUTTON_LEFT]       = MOUSE_EVENT_LBUTTON,
>>> --
>>> 2.7.2
>>
>> I'm afraid I don't really understand why you think this
>> should change. On the face of it the current code looks right:
>> we pass through the mouse button if:
>>   (1) we've got the mouse grab
>> or (2) our window has the focus, even if it's not grabbed
>>
>> I would expect the "activation click" to be "we don't have
>> the mouse grab, and we don't have focus either (some other
>> app is foreground)".
>
> When QEMU's main window is in the background and the user clicks on it,
> the NSLeftMouseUp case in the handleEvent: method will set the
> isMouseGrabbed variable to true. This means the
> "if ((isMouseGrabbed || [[self window] isKeyWindow]) &&
> (last_buttons != buttons))" code will always be true for a left
> mouse button click. The mouse event will be sent to the guest
> when it shouldn't be.

OK, that sounds like a bug, but this doesn't look like the
right way to fix it, because with your change we won't
pass through the click if this is a click on the window
when it's not in the background.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] ui/cocoa.m: fix sending mouse event to guest

[Programmingkid]

On Apr 2, 2016, at 1:35 PM, Peter Maydell wrote:

> On 2 April 2016 at 18:25, Programmingkid <programmingkidx@gmail.com> wrote:
>> 
>> On Apr 2, 2016, at 1:07 PM, Peter Maydell wrote:
>> 
>>> On 2 April 2016 at 17:56, Programmingkid <programmingkidx@gmail.com> wrote:
>>>> The mouse down event should not be sent to the guest if the mouse down event
>>>> causes an activation of QEMU. This patch prevents activation clicks from going
>>>> to the guest.
>>>> 
>>>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>>>> ---
>>>> ui/cocoa.m | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>> 
>>>> diff --git a/ui/cocoa.m b/ui/cocoa.m
>>>> index 60a7c07..07d9c86 100644
>>>> --- a/ui/cocoa.m
>>>> +++ b/ui/cocoa.m
>>>> @@ -698,7 +698,7 @@ QemuCocoaView *cocoaView;
>>>>         * call below. We definitely don't want to pass that click through
>>>>         * to the guest.
>>>>         */
>>>> -        if ((isMouseGrabbed || [[self window] isKeyWindow]) &&
>>>> +        if ((isMouseGrabbed && [[self window] isKeyWindow]) &&
>>>>            (last_buttons != buttons)) {
>>>>            static uint32_t bmap[INPUT_BUTTON__MAX] = {
>>>>                [INPUT_BUTTON_LEFT]       = MOUSE_EVENT_LBUTTON,
>>>> --
>>>> 2.7.2
>>> 
>>> I'm afraid I don't really understand why you think this
>>> should change. On the face of it the current code looks right:
>>> we pass through the mouse button if:
>>>  (1) we've got the mouse grab
>>> or (2) our window has the focus, even if it's not grabbed
>>> 
>>> I would expect the "activation click" to be "we don't have
>>> the mouse grab, and we don't have focus either (some other
>>> app is foreground)".
>> 
>> When QEMU's main window is in the background and the user clicks on it,
>> the NSLeftMouseUp case in the handleEvent: method will set the
>> isMouseGrabbed variable to true. This means the
>> "if ((isMouseGrabbed || [[self window] isKeyWindow]) &&
>> (last_buttons != buttons))" code will always be true for a left
>> mouse button click. The mouse event will be sent to the guest
>> when it shouldn't be.
> 
> OK, that sounds like a bug, but this doesn't look like the
> right way to fix it, because with your change we won't
> pass through the click if this is a click on the window
> when it's not in the background.

I think logically this is the best solution. If the window is in the foreground and doesn't have the mouse grabbed, the user can't move the mouse pointer in the guest. This could cause stuff to be clicked that might cause undesirable effects.

Re: [Qemu-devel] [PATCH] ui/cocoa.m: fix sending mouse event to guest

[Peter Maydell]

On 2 April 2016 at 18:53, Programmingkid <programmingkidx@gmail.com> wrote:
>
> On Apr 2, 2016, at 1:35 PM, Peter Maydell wrote:
>
>> On 2 April 2016 at 18:25, Programmingkid <programmingkidx@gmail.com> wrote:
>>>
>>> On Apr 2, 2016, at 1:07 PM, Peter Maydell wrote:
>>>
>>>> On 2 April 2016 at 17:56, Programmingkid <programmingkidx@gmail.com> wrote:
>>>>> The mouse down event should not be sent to the guest if the mouse down event
>>>>> causes an activation of QEMU. This patch prevents activation clicks from going
>>>>> to the guest.
>>>>>
>>>>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>>>>> ---
>>>>> ui/cocoa.m | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/ui/cocoa.m b/ui/cocoa.m
>>>>> index 60a7c07..07d9c86 100644
>>>>> --- a/ui/cocoa.m
>>>>> +++ b/ui/cocoa.m
>>>>> @@ -698,7 +698,7 @@ QemuCocoaView *cocoaView;
>>>>>         * call below. We definitely don't want to pass that click through
>>>>>         * to the guest.
>>>>>         */
>>>>> -        if ((isMouseGrabbed || [[self window] isKeyWindow]) &&
>>>>> +        if ((isMouseGrabbed && [[self window] isKeyWindow]) &&
>>>>>            (last_buttons != buttons)) {
>>>>>            static uint32_t bmap[INPUT_BUTTON__MAX] = {
>>>>>                [INPUT_BUTTON_LEFT]       = MOUSE_EVENT_LBUTTON,
>>>>> --
>>>>> 2.7.2
>>>>
>>>> I'm afraid I don't really understand why you think this
>>>> should change. On the face of it the current code looks right:
>>>> we pass through the mouse button if:
>>>>  (1) we've got the mouse grab
>>>> or (2) our window has the focus, even if it's not grabbed
>>>>
>>>> I would expect the "activation click" to be "we don't have
>>>> the mouse grab, and we don't have focus either (some other
>>>> app is foreground)".
>>>
>>> When QEMU's main window is in the background and the user clicks on it,
>>> the NSLeftMouseUp case in the handleEvent: method will set the
>>> isMouseGrabbed variable to true. This means the
>>> "if ((isMouseGrabbed || [[self window] isKeyWindow]) &&
>>> (last_buttons != buttons))" code will always be true for a left
>>> mouse button click. The mouse event will be sent to the guest
>>> when it shouldn't be.
>>
>> OK, that sounds like a bug, but this doesn't look like the
>> right way to fix it, because with your change we won't
>> pass through the click if this is a click on the window
>> when it's not in the background.
>
> I think logically this is the best solution. If the window is in
> the foreground and doesn't have the mouse grabbed, the user can't
> move the mouse pointer in the guest. This could cause stuff to be
> clicked that might cause undesirable effects.

So why do we now have a different condition for "don't pass
through buttons" and "don't pass through mouse events" ?

The process for making changes should be something like:
 * identify the model we are trying to use to decide whether
   to pass events through to the guest or not
 * if this model is wrong (ie there is a design flaw),
   identify what the changes to the design need to be
   (possibly looking at other OSX VM UIs and how we handle
   similar cases in our other UI frontends, to identify what
   might be reasonable behaviour), so that we have a coherent
   design which we can use now and in the future to fix the code
 * work out where the code is diverging from the design
   (either because the code is buggy or because we made a
   change to the design in the previous step)
 * change the code
 * make sure that comments match the changed code/design

I don't have strong feelings about what the right design for
handling mouse events should be, but I do think we should
have one, and this patch seems to be making a local change
rather than considering the larger picture.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] ui/cocoa.m: fix sending mouse event to guest

[Programmingkid]

On Apr 3, 2016, at 8:21 AM, Peter Maydell wrote:

> On 2 April 2016 at 18:53, Programmingkid <programmingkidx@gmail.com> wrote:
>> 
>> On Apr 2, 2016, at 1:35 PM, Peter Maydell wrote:
>> 
>>> On 2 April 2016 at 18:25, Programmingkid <programmingkidx@gmail.com> wrote:
>>>> 
>>>> On Apr 2, 2016, at 1:07 PM, Peter Maydell wrote:
>>>> 
>>>>> On 2 April 2016 at 17:56, Programmingkid <programmingkidx@gmail.com> wrote:
>>>>>> The mouse down event should not be sent to the guest if the mouse down event
>>>>>> causes an activation of QEMU. This patch prevents activation clicks from going
>>>>>> to the guest.
>>>>>> 
>>>>>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>>>>>> ---
>>>>>> ui/cocoa.m | 2 +-
>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>> 
>>>>>> diff --git a/ui/cocoa.m b/ui/cocoa.m
>>>>>> index 60a7c07..07d9c86 100644
>>>>>> --- a/ui/cocoa.m
>>>>>> +++ b/ui/cocoa.m
>>>>>> @@ -698,7 +698,7 @@ QemuCocoaView *cocoaView;
>>>>>>        * call below. We definitely don't want to pass that click through
>>>>>>        * to the guest.
>>>>>>        */
>>>>>> -        if ((isMouseGrabbed || [[self window] isKeyWindow]) &&
>>>>>> +        if ((isMouseGrabbed && [[self window] isKeyWindow]) &&
>>>>>>           (last_buttons != buttons)) {
>>>>>>           static uint32_t bmap[INPUT_BUTTON__MAX] = {
>>>>>>               [INPUT_BUTTON_LEFT]       = MOUSE_EVENT_LBUTTON,
>>>>>> --
>>>>>> 2.7.2
>>>>> 
>>>>> I'm afraid I don't really understand why you think this
>>>>> should change. On the face of it the current code looks right:
>>>>> we pass through the mouse button if:
>>>>> (1) we've got the mouse grab
>>>>> or (2) our window has the focus, even if it's not grabbed
>>>>> 
>>>>> I would expect the "activation click" to be "we don't have
>>>>> the mouse grab, and we don't have focus either (some other
>>>>> app is foreground)".
>>>> 
>>>> When QEMU's main window is in the background and the user clicks on it,
>>>> the NSLeftMouseUp case in the handleEvent: method will set the
>>>> isMouseGrabbed variable to true. This means the
>>>> "if ((isMouseGrabbed || [[self window] isKeyWindow]) &&
>>>> (last_buttons != buttons))" code will always be true for a left
>>>> mouse button click. The mouse event will be sent to the guest
>>>> when it shouldn't be.
>>> 
>>> OK, that sounds like a bug, but this doesn't look like the
>>> right way to fix it, because with your change we won't
>>> pass through the click if this is a click on the window
>>> when it's not in the background.
>> 
>> I think logically this is the best solution. If the window is in
>> the foreground and doesn't have the mouse grabbed, the user can't
>> move the mouse pointer in the guest. This could cause stuff to be
>> clicked that might cause undesirable effects.
> 
> So why do we now have a different condition for "don't pass
> through buttons" and "don't pass through mouse events" ?
> 
> The process for making changes should be something like:
> * identify the model we are trying to use to decide whether
>   to pass events through to the guest or not
> * if this model is wrong (ie there is a design flaw),
>   identify what the changes to the design need to be
>   (possibly looking at other OSX VM UIs and how we handle
>   similar cases in our other UI frontends, to identify what
>   might be reasonable behaviour), so that we have a coherent
>   design which we can use now and in the future to fix the code
> * work out where the code is diverging from the design
>   (either because the code is buggy or because we made a
>   change to the design in the previous step)
> * change the code
> * make sure that comments match the changed code/design
> 
> I don't have strong feelings about what the right design for
> handling mouse events should be, but I do think we should
> have one, and this patch seems to be making a local change
> rather than considering the larger picture.


Maybe the mouse cursor in the guest should be able to move even when the mouse isn't grabbed. So if the mouse is grabbed or not in QEMU, the mouse pointer in the guest should always move when the host mouse pointer is in the QEMU window. If this is true, then sending mouse button events to the guest would be ok whether grabbed or not because the user can just move the mouse out of the way of danger at any time. You may have to read this several times before it makes sense.