Hey Cristopher, No problem. :-) I went ahead and committed my fix. Eric On 2011-02-22, at 3:03 PM, Christopher Armstrong wrote:
> 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" > <[email protected]> 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 >>> [email protected] >>> >>> >>> >>> >>> >>> >>> _______________________________________________ >>> Gnustep-dev mailing list >>> [email protected] >>> http://lists.gnu.org/mailman/listinfo/gnustep-dev >> >> >> _______________________________________________ >> Gnustep-dev mailing list >> [email protected] >> http://lists.gnu.org/mailman/listinfo/gnustep-dev >> > -- > Christopher Armstrong > carmstrong ^^AT^ fastmail dOT com /Dot/ au > _______________________________________________ Gnustep-dev mailing list [email protected] http://lists.gnu.org/mailman/listinfo/gnustep-dev
