Re: NSMenuView patch

[Christopher Armstrong]

Hi Eric

Thanks.

I obviously missed this one :-(. I spent more time testing for
intermittent behaviour and the other menu styles to make sure there was
no breakage instead of testing actual menu items.

Its perfectly safe to release it more than once - at least on X11, the
subsequent calls to XReleasePointer should be ignored. On Windows, I
presume SetCapture/ReleaseCapture will just return an error (MSDN
doesn't help here), which is probably also ignored by the backend.

Cheers
Chris

On Tue, 22 Feb 2011 14:24 -0700, "Eric Wasylishen"
<address@hidden> wrote:
> 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
> 
> 
> _______________________________________________
> Gnustep-dev mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/gnustep-dev
> 
-- 
  Christopher Armstrong
  carmstrong ^^AT^ fastmail dOT com /Dot/ au