[v2] ui/cocoa.m: fix sending mouse event to guest

The mouse down event should not be sent to the guest if the mouse down event
causes QEMU to move the foreground from the background. This patch prevents
these activation clicks from going to the guest.

Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
---
v2 changes:
Fix a bug that caused fullscreen mode not to receive mouse events.

 ui/cocoa.m | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

On 1 January 2017 at 21:31, Programmingkid <programmingkidx@gmail.com> wrote:
> The mouse down event should not be sent to the guest if the mouse down event
> causes QEMU to move the foreground from the background. This patch prevents
> these activation clicks from going to the guest.
>
> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
> ---
> v2 changes:
> Fix a bug that caused fullscreen mode not to receive mouse events.
>
>  ui/cocoa.m | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index 26d4a1c..ae3f5a1 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -695,13 +695,13 @@ QemuCocoaView *cocoaView;
>
>      if (mouse_event) {
>          /* Don't send button events to the guest unless we've got a
> -         * mouse grab or window focus. If we have neither then this event
> -         * is the user clicking on the background window to activate and
> -         * bring us to the front, which will be done by the sendEvent
> -         * call below. We definitely don't want to pass that click through
> -         * to the guest.
> +         * mouse grab, window focus, or in fullscreen mode. If we have neither
> +         * then this event is the user clicking on the background window to
> +         * activate and bring us to the front, which will be done by the
> +         * sendEvent 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] || isFullscreen) &&
>              (last_buttons != buttons)) {
>              static uint32_t bmap[INPUT_BUTTON__MAX] = {
>                  [INPUT_BUTTON_LEFT]       = MOUSE_EVENT_LBUTTON,

Comment doesn't match code. The comment says "mouse grab, full screen
or focus", but the code is doing "mouse grab and (full screen or focus)".

Which is correct?

thanks
-- PMM

On Jan 6, 2017, at 8:48 AM, Peter Maydell wrote:

> On 1 January 2017 at 21:31, Programmingkid <programmingkidx@gmail.com> wrote:
>> The mouse down event should not be sent to the guest if the mouse down event
>> causes QEMU to move the foreground from the background. This patch prevents
>> these activation clicks from going to the guest.
>> 
>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>> ---
>> v2 changes:
>> Fix a bug that caused fullscreen mode not to receive mouse events.
>> 
>> ui/cocoa.m | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>> 
>> diff --git a/ui/cocoa.m b/ui/cocoa.m
>> index 26d4a1c..ae3f5a1 100644
>> --- a/ui/cocoa.m
>> +++ b/ui/cocoa.m
>> @@ -695,13 +695,13 @@ QemuCocoaView *cocoaView;
>> 
>>     if (mouse_event) {
>>         /* Don't send button events to the guest unless we've got a
>> -         * mouse grab or window focus. If we have neither then this event
>> -         * is the user clicking on the background window to activate and
>> -         * bring us to the front, which will be done by the sendEvent
>> -         * call below. We definitely don't want to pass that click through
>> -         * to the guest.
>> +         * mouse grab, window focus, or in fullscreen mode. If we have neither
>> +         * then this event is the user clicking on the background window to
>> +         * activate and bring us to the front, which will be done by the
>> +         * sendEvent 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] || isFullscreen) &&
>>             (last_buttons != buttons)) {
>>             static uint32_t bmap[INPUT_BUTTON__MAX] = {
>>                 [INPUT_BUTTON_LEFT]       = MOUSE_EVENT_LBUTTON,
> 
> Comment doesn't match code. The comment says "mouse grab, full screen
> or focus", but the code is doing "mouse grab and (full screen or focus)".
> 
> Which is correct?


This sounds more accurate:

(mouse grab and main window is the front window) or QEMU is in fullscreen. 

If the comment is the only thing that is the problem, please feel free to
edit it before committing. 

Thank you

On 6 January 2017 at 15:44, Programmingkid <programmingkidx@gmail.com> wrote:
>
> On Jan 6, 2017, at 8:48 AM, Peter Maydell wrote:
>
>> On 1 January 2017 at 21:31, Programmingkid <programmingkidx@gmail.com> wrote:
>>> The mouse down event should not be sent to the guest if the mouse down event
>>> causes QEMU to move the foreground from the background. This patch prevents
>>> these activation clicks from going to the guest.

>>>     if (mouse_event) {
>>>         /* Don't send button events to the guest unless we've got a
>>> -         * mouse grab or window focus. If we have neither then this event
>>> -         * is the user clicking on the background window to activate and
>>> -         * bring us to the front, which will be done by the sendEvent
>>> -         * call below. We definitely don't want to pass that click through
>>> -         * to the guest.
>>> +         * mouse grab, window focus, or in fullscreen mode. If we have neither
>>> +         * then this event is the user clicking on the background window to
>>> +         * activate and bring us to the front, which will be done by the
>>> +         * sendEvent 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] || isFullscreen) &&
>>>             (last_buttons != buttons)) {
>>>             static uint32_t bmap[INPUT_BUTTON__MAX] = {
>>>                 [INPUT_BUTTON_LEFT]       = MOUSE_EVENT_LBUTTON,
>>
>> Comment doesn't match code. The comment says "mouse grab, full screen
>> or focus", but the code is doing "mouse grab and (full screen or focus)".
>>
>> Which is correct?
>
>
> This sounds more accurate:
>
> (mouse grab and main window is the front window) or QEMU is in fullscreen.

That's not what your code does either.

What is the actual situation which we currently get wrong and need
to get right? Your commit message says "if the event causes QEMU
to move to the foreground", but my expectation was that if we
weren't in the foreground then we would not have the mouse grabbed,
and in any case your patch adds a check on isFullscreen but your
commit message doesn't say anything about full-screen or not being
relevant to the problem.

thanks
-- PMM

On Jan 6, 2017, at 11:19 AM, Peter Maydell wrote:

> On 6 January 2017 at 15:44, Programmingkid <programmingkidx@gmail.com> wrote:
>> 
>> On Jan 6, 2017, at 8:48 AM, Peter Maydell wrote:
>> 
>>> On 1 January 2017 at 21:31, Programmingkid <programmingkidx@gmail.com> wrote:
>>>> The mouse down event should not be sent to the guest if the mouse down event
>>>> causes QEMU to move the foreground from the background. This patch prevents
>>>> these activation clicks from going to the guest.
> 
>>>>    if (mouse_event) {
>>>>        /* Don't send button events to the guest unless we've got a
>>>> -         * mouse grab or window focus. If we have neither then this event
>>>> -         * is the user clicking on the background window to activate and
>>>> -         * bring us to the front, which will be done by the sendEvent
>>>> -         * call below. We definitely don't want to pass that click through
>>>> -         * to the guest.
>>>> +         * mouse grab, window focus, or in fullscreen mode. If we have neither
>>>> +         * then this event is the user clicking on the background window to
>>>> +         * activate and bring us to the front, which will be done by the
>>>> +         * sendEvent 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] || isFullscreen) &&
>>>>            (last_buttons != buttons)) {
>>>>            static uint32_t bmap[INPUT_BUTTON__MAX] = {
>>>>                [INPUT_BUTTON_LEFT]       = MOUSE_EVENT_LBUTTON,
>>> 
>>> Comment doesn't match code. The comment says "mouse grab, full screen
>>> or focus", but the code is doing "mouse grab and (full screen or focus)".
>>> 
>>> Which is correct?
>> 
>> 
>> This sounds more accurate:
>> 
>> (mouse grab and main window is the front window) or QEMU is in fullscreen.
> 
> That's not what your code does either.

Yes, you are right. Didn't see the parentheses. This is what it does:
mouse is grabbed and either QEMU window is front window or QEMU is in fullscreen.

The click that causes QEMU to grab the mouse should not be sent to the guest because the
host mouse could be on one part of the screen, and the guest mouse could be in another
part of the screen the user isn't aware of yet.

> What is the actual situation which we currently get wrong and need
> to get right?

When the user clicks on QEMU's window to bring QEMU to the front, that activation
click should not be sent to the guest. I have had a lot of problems without this
patch. When the activation click was wrongly sent to the guest, I'm had stuff 
happen like volumes unmount because the guest mouse cursor was over an eject button.

> Your commit message says "if the event causes QEMU
> to move to the foreground", but my expectation was that if we
> weren't in the foreground then we would not have the mouse grabbed,
> and in any case your patch adds a check on isFullscreen but your
> commit message doesn't say anything about full-screen or not being
> relevant to the problem.

I know the code works correctly, it is just the commit message that needs tweaking.