Re: NSMenuView patch

[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
> 
> --------
> Christopher Armstrong
> address@hidden
> 
> 
> 
> 
> 
> 
> _______________________________________________
> Gnustep-dev mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/gnustep-dev