Re: NSMenuView patch

[Fred Kiefer]

I just ran into a very annoying problem caused by our mouse capture in
NSMenuView. When debugging an action method I put a breakpoint into that
method and ended up with an unusable X windows desktop. The debugger
would display its command prompt, but the mouse was still captured by
the GNUstep menu window and I was unable to do anything.
Looks like a duplicate call to capture actually has an effect only when
out one is released the mouse will be usable again.
This should also manifest itself in other cases, for example a modal
window that gets started from a nested menu item. But I am unable to
reproduce the issue there. I am not really sure what goes on here.

The annoying bit is that I have to restart X each time I try this :-(

Fred

Am 22.02.2011 23:32, schrieb Fred Kiefer:
> On Windows a duplicate call to release the mouse wont do any harm as we
> ignore the results from both calls. On X11 it looks like we once stored
> the grab window but luckily this isn't used any more. We may though get
> a message from NSDebugLLog on duplicated calls to capturemouse:. Perhaps
> we shouldn't be calling trackWithEvent: on the other menu view, but
> rather use the new method?
> 
> 
> Am 22.02.2011 22:24, schrieb Eric Wasylishen:
>> Hey Fred & Cristopher,
>>
>> The only problem with this patch is the mouse isn't uncaptured before
>> executing the menu item's action, so when you open a file dialog the
>> mouse stops working now (at least, on X11).
>>
>> I think NSMenuView.m needs the following change:
>>
>> Index: Source/NSMenuView.m 
>> =================================================================== 
>> --- Source/NSMenuView.m      (revision 32303) +++ Source/NSMenuView.m
>> (working copy) @@ -1773,6 +1773,9 @@ return YES; }
>>
>> +  // Before executing the action, uncapture the mouse +  [_window
>> _releaseMouse: self]; + if([self _executeItemAtIndex:
>> indexOfActionToExecute removeSubmenu: subMenusNeedRemoving] == NO) {
>>
>> Does that make sense? Is there any harm in uncapturing the mouse
>> twice? (once in the line I added, and once in -trackWithEvent:)
>>
>> Cheers, Eric
>>
>> On 2011-02-20, at 5:10 AM, Christopher Armstrong wrote:
>>
>>> Hi Wolfgang and Fred
>>>
>>> I have had another attempt at this patch, trying to address some of
>>> your concerns.
>>>
>>> On 20/02/2011, at 19:51 PM, Wolfgang Lux wrote:
>>>
>>>>> Christopher also took create care to release the captured mouse
>>>>> on all possible paths. This is very important, as otherwise the
>>>>> application could stop responding. But now consider that
>>>>> somebody else works on this method in the future. The person
>>>>> might easily not know about that necessity and accidentally
>>>>> break stuff. Even in this patch the mouse sometimes gets
>>>>> release after a tracking in a submenu. I don't have a clue how
>>>>> this is supposed to work. We always have to release the mouse 
>>>>> before handling over control to a different window. This of
>>>>> course is easy to fix for now. But who will remember this for
>>>>> future changes?
>>>
>>> Actually, IMHO *shouldn't* release the mouse before handing control
>>> over to another window while we are still tracking the mouse, as it
>>> introduces a possible (but tiny) race condition where another app
>>> could capture the mouse. When we capture the mouse in the first
>>> window, we get events for all the windows sent to GNUstep, and
>>> GNUstep just passes them all along. All the internal calls for
>>> other windows to capture/release mouse are just ignored by X11 (it
>>> just returns a error value to the caller). The last (outermost)
>>> call should release the mouse.
>>>
>>>>> As much as I like the idea of this patch, I think it is too
>>>>> fragile code to be added. We should think hard on how to
>>>>> restructure the code here to make it less fragile and then we
>>>>> might be able to safely add a mouse capture to it.
>>>>
>>>> The safe solution is of course to move the body of this method
>>>> without the code for capturing the mouse into a new private
>>>> method -_trackWithEvent: and to define trackWithEvent: as a thin
>>>> wrapper around the new method, which performs mouse capturing.
>>>> E.g., something like
>>>
>>>
>>> I've created a new patch incorporating Wolfgang's suggestion, which
>>> seems like an elegant way to solve the mouse capture problem
>>> without interspersing the method with capture/release calls. It can
>>> be found here:
>>>
>>> https://savannah.gnu.org/patch/index.php?7470
>>>
>>> Cheers Chris