Re: NSMenuView patch

[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