Re: NSMenuView patch

[Eric Wasylishen]

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"
> <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
>