| Message ID | 864865FE-CA14-4D3A-8B73-84A2E321CA6C@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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.
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
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.
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,
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(-)