Re: [Gnustep-cvs] r31213 - in /libs/gui/trunk: ChangeLog Source/NSMenuView.m Source/NSPopUpButtonCell.m

[Doug Simons]

Hello Fred, Greg, and anyone else who cares to look at this problem with 
Windows events.

Unfortunately, without the former fix in place the pulldown menus are not 
working correctly on Windows. Here's the situation:

Our action method gets called twice on Windows when selecting an item from a 
pulldown menu.
Unfortunately, the mechanism behind this has to do with Windows' tendency to 
call back into
GNUstep whenever the app checks for incoming events. Here's a narrated 
transcript of what's happening:

Breakpoint 3, -[NSMenuView trackWithEvent:] (self=0x1589148, _cmd=0x6dac7728, 
event=0x1474bc0)
    at NSMenuView.m:1444
1444      NSDate *theDistantFuture = [NSDate distantFuture];
(gdb) bt
#0  -[NSMenuView trackWithEvent:] (self=0x1589148, _cmd=0x6dac7728, 
event=0x1474bc0)
    at NSMenuView.m:1444
#1  0x6d9471fb in -[NSMenuView mouseDown:] (self=0x1589148, _cmd=0x6dad7548, 
theEvent=0x1474bc0)
    at NSMenuView.m:1779
#2  0x6d9639e6 in -[NSPopUpButtonCell trackMouse:inRect:ofView:untilMouseUp:] 
(self=0x9aaaef0,
    _cmd=0x6dad5d00, theEvent=0xb886a78, cellFrame=
        {origin = {x = 0, y = 0}, size = {width = 148, height = 26}}, 
controlView=0x9ca5110,
    untilMouseUp=1 '\001') at NSPopUpButtonCell.m:1014
#3  0x6d95f89a in -[NSPopUpButton mouseDown:] (self=0x9ca5110, _cmd=0x6db17d80,
    theEvent=0xb886a78) at NSPopUpButton.m:457
#4  0x6d9ec8f3 in -[NSWindow sendEvent:] (self=0x9ae3f80, _cmd=0x6da77048, 
theEvent=0xb886a78)
    at NSWindow.m:3684
#5  0x6d89dc8e in -[NSApplication run] (self=0x1317938, _cmd=0x6da6cd20) at 
NSApplication.m:1541
#6  0x6d88220a in NSApplicationMain (argc=1, argv=0x10fda18) at Functions.m:89
#7  0x004018bd in main (argc=1, argv=0x10fda18) at main.m:13
(gdb)

This is how we enter the NSMenuView trackWithEvent: method. So far, so good.

The processing then continues normally through this method up to line 1659, at 
which point it
(indirectly) calls our action method and hits another breakpoint:

(gdb) n
1462      original = AUTORELEASE(RETAIN(event));
(gdb)
1464      type = [event type];
(gdb)
[… many steps omitted here …] 
1640              if (!justAttachedNewSubmenu && index != _highlightedItemIndex)
(gdb)
1659          event = [NSApp nextEventMatchingMask: eventMask
(gdb) n

Breakpoint 1, -[VNCConnection(ScriptGeneration) enterTypeTextKeys:] 
(self=0x1420260,
    _cmd=0x1702b78, sender=0x9aaaef0) at VNCConnection+ScriptGeneration.m:1408
1408            NSString *keys = [sender titleOfSelectedItem];
(gdb) bt
#0  -[VNCConnection(ScriptGeneration) enterTypeTextKeys:] (self=0x1420260, 
_cmd=0x1702b78,
    sender=0x9aaaef0) at VNCConnection+ScriptGeneration.m:1408
#1  0x6d897906 in -[NSApplication sendAction:to:from:] (self=0x1317938, 
_cmd=0x6dad7688,
    aSelector=0x1702b78, aTarget=0x1420260, sender=0x9aaaef0) at 
NSApplication.m:2204
#2  0x6d96145a in -[NSPopUpButtonCell(CocoaExtensions) _popUpItemAction:] 
(self=0x9aaaef0,
    _cmd=0x12cf8a8, sender=0x9952c68) at NSPopUpButtonCell.m:1317
#3  0x6d897906 in -[NSApplication sendAction:to:from:] (self=0x1317938, 
_cmd=0x6290ea58,
    aSelector=0x12cf8a8, aTarget=0x9aaaef0, sender=0x9952c68) at 
NSApplication.m:2204
#4  0x62905430 in -[WinUXTheme(NSMenu) processCommand:] (self=0x137d118, 
_cmd=0x700e9c30,
    context=0x126) at WinNSMenu.m:385
#5  0x700d3fb6 in -[WIN32Server windowEventProc::::] (self=0x136a640, 
_cmd=0x700e7258,
    hwnd=0x4c0134, uMsg=273, wParam=294, lParam=0) at WIN32Server.m:692
#6  0x700d0101 in MainWndProc (hwnd=0x4c0134, uMsg=273, wParam=294, lParam=0)
    at WIN32Server.m:2141
#7  0x77d48709 in USER32!GetDC () from C:\WINDOWS\system32\user32.dll
#8  0x004c0134 in -[VNCConnection(ScriptGeneration) 
toolbar:itemForItemIdentifier:willBeInsertedInto
Toolbar:] (self=0x700d00a4, _cmd=0x4c0134, toolbar=0x111, 
itemIdentifier=0x700d00a4,
    flag=0 '\000') at VNCConnection+ScriptGeneration.m:2059
#9  0x77d487eb in USER32!GetDC () from C:\WINDOWS\system32\user32.dll
#10 0x700d00a4 in -[WIN32Server(WindowOps) windowbacking::] () at 
WIN32Server.m:964
#11 0x77d489a5 in USER32!GetWindowLongW () from C:\WINDOWS\system32\user32.dll
#12 0x00000000 in ?? ()
(gdb)

Here, you can see that obtaining the next event from NSApp involves calls to 
Windows that give
the Windows event mechanism an opportunity to call back into the MainWndProc() 
function (I believe
the methods shown at frames 7 and above in the backtrace here are bogus -- gdb 
gets confused and
clearly can't identify the fact that this is all occurring within the earlier 
call stack into
the NSMenuView trackWithEvent: method).

Frame 4 is the processCommand: method in WinUXTheme. This is the only place I'm 
aware of where the
theme is called during this process. It doesn't look to me like a useful place 
to intervene.

So, at this point our action method has been called indirectly as a result of 
the call to
[NSApp nextEventMatchingMask: ... ] at line 1659 of [NSMenuView 
trackWithEvent:]. I've cleverly
entered a breakpoint at the next executable line of that method, which is hit 
when we continue
from our action method:

(gdb) c
Continuing.
warning: HEAP[Eggplant.exe]:
warning: Heap block at 016EC540 modified at 016EC549 past requested size of 1


Program received signal SIGTRAP, Trace/breakpoint trap.
warning: HEAP[Eggplant.exe]:
warning: Invalid Address specified to RtlFreeHeap( 00AB0000, 016EC548 )


Program received signal SIGTRAP, Trace/breakpoint trap.

Breakpoint 4, -[NSMenuView trackWithEvent:] (self=0x1589148, _cmd=0x6dac7728, 
event=0x9bc3670)
    at NSMenuView.m:1663
1663          type = [event type];
(gdb)

[note: the warnings and SIGTRAPs above seem to be normal fare when debugging in 
Windows, so I don't think
they're significant, but I left them in for completeness, in case anyone thinks 
they may be relevant]

Now that we've arrived at this point, the trackWithEvent: method proceeds 
normally until line 1734,
where it calls our action method a second time (although getting this process 
to work in gdb is tricky,
as it relies on the mouse location in the window, so you have to put the mouse 
in the right place in the
application window while working in the terminal window, and hope that the 
planets are properly aligned):

1659          event = [NSApp nextEventMatchingMask: eventMask
(gdb) n

Breakpoint 4, -[NSMenuView trackWithEvent:] (self=0x9c1a130, _cmd=0x6dac7728, 
event=0x9b263a0)
    at NSMenuView.m:1663
1663          type = [event type];
(gdb)
1665      while (type != end || shouldFinish == NO);
(gdb)
[… more steps omitted here …] 
(gdb)
1728      if([self _executeItemAtIndex: indexOfActionToExecute
(gdb)
1734      [_attachedMenu performActionForItemAtIndex: indexOfActionToExecute];
(gdb)

Breakpoint 1, -[VNCConnection(ScriptGeneration) enterTypeTextKeys:] 
(self=0x999d708,
    _cmd=0x14f5540, sender=0x9a9a3e8) at VNCConnection+ScriptGeneration.m:1408
1408            NSString *keys = [sender titleOfSelectedItem];
(gdb) bt
#0  -[VNCConnection(ScriptGeneration) enterTypeTextKeys:] (self=0x999d708, 
_cmd=0x14f5540,
    sender=0x9a9a3e8) at VNCConnection+ScriptGeneration.m:1408
#1  0x6d897906 in -[NSApplication sendAction:to:from:] (self=0x1317938, 
_cmd=0x6dad7688,
    aSelector=0x14f5540, aTarget=0x999d708, sender=0x9a9a3e8) at 
NSApplication.m:2204
#2  0x6d96145a in -[NSPopUpButtonCell(CocoaExtensions) _popUpItemAction:] 
(self=0x9a9a3e8,
    _cmd=0x12cf8a8, sender=0x14dd458) at NSPopUpButtonCell.m:1317
#3  0x6d897906 in -[NSApplication sendAction:to:from:] (self=0x1317938, 
_cmd=0x6dac4748,
    aSelector=0x12cf8a8, aTarget=0x9a9a3e8, sender=0x14dd458) at 
NSApplication.m:2204
#4  0x6d93ede7 in -[NSMenu performActionForItemAtIndex:] (self=0x9c6a968, 
_cmd=0x6dac76a8,
    index=10) at NSMenu.m:1300
#5  0x6d948388 in -[NSMenuView trackWithEvent:] (self=0x9c1a130, 
_cmd=0x6dac7728,
    event=<value optimized out>) at NSMenuView.m:1734
#6  0x6d9471fb in -[NSMenuView mouseDown:] (self=0x9c1a130, _cmd=0x6dad7548, 
theEvent=0x9da16b8)
    at NSMenuView.m:1779
#7  0x6d9639e6 in -[NSPopUpButtonCell trackMouse:inRect:ofView:untilMouseUp:] 
(self=0x9a9a3e8,
    _cmd=0x6dad5d00, theEvent=0x9e07d48, cellFrame=
        {origin = {x = 0, y = 0}, size = {width = 148, height = 26}}, 
controlView=0x9a9a230,
    untilMouseUp=1 '\001') at NSPopUpButtonCell.m:1014
#8  0x6d95f89a in -[NSPopUpButton mouseDown:] (self=0x9a9a230, _cmd=0x6db17d80,
    theEvent=0x9e07d48) at NSPopUpButton.m:457
#9  0x6d9ec8f3 in -[NSWindow sendEvent:] (self=0x9aafa40, _cmd=0x6da77048, 
theEvent=0x9e07d48)
    at NSWindow.m:3684
#10 0x6d89dc8e in -[NSApplication run] (self=0x1317938, _cmd=0x6da6cd20) at 
NSApplication.m:1541
#11 0x6d88220a in NSApplicationMain (argc=1, argv=0x10fda18) at Functions.m:89
#12 0x004018bd in main (argc=1, argv=0x10fda18) at main.m:13
(gdb)

So here our action has been called a second time from the same event.

Possible solutions:

1. The one I checked in earlier, which uses the hack of removing our action 
during the call to nextEventMatchingMask:
in order to prevent the action from being called twice. Not very elegant I 
admit, but it does the job.

2. Figure out a more general solution for blocking callbacks from Windows while 
getting events. This would be ideal,
because these callbacks cause lots of problems that I've worked around in 
various ways, such as a recent change I
made to to NSPasteboard to change an NSLock to an NSRecursiveLock, which was 
probably only needed on Windows.

One thought to do this would be to set a flag in NSApplication while fetching 
events, and queue any events that come in to handle them later… I'm not sure 
exactly how this would work, and it might not solve the current problem anyway, 
if the queued events were only delayed somewhat but were still delivered.

2a. Assuming the extra call could be eliminated, for the situation with 
pulldown menus, it would also be necessary to modify the NSMenuView 
trackWithEvent: method to properly record the selected item. In my previous fix 
I modified the [NSPopUpButtonCell _popUpItemAction:] method to set the popup's 
selected item. As the code stands now, it's necessary to call that method for 
the menu to work properly, and that code is called as part of the chain 
initiated by the callback from Windows. So if the callback were eliminated the 
trackWithEvent: method would need to set the selected item, which probably 
makes better sense anyway.

3. Any other ideas? I would really like to see a good general solution to the 
callback problem, but it may not be trivial to do. 

Fred, you suggested that it might be possible to do something in the 
processCommand: method in the theme, but I'm not sure quite what you had in 
mind. Based on what I've described above, do you still think a solution might 
be possible there? I'm not sure how that method would decide whether to send 
the action callback or not.

Comments and ideas are very welcome.

Doug


On Oct 19, 2010, at 2:31 PM, Fred Kiefer wrote:

> Hi Doug,
> 
> sorry the revert is already committed, but if there is a way to help you
> I am willing to jump in.
> In my first mail I suggested to add the popup test into the
> processCommand: method of the tehme. I still think this should work, but
> it may not be the best way to resolve the issue. Greg, who is more
> familiar with that theme should know.
> 
> Fred
> 
> Am 19.10.2010 17:42, schrieb Doug Simons:
>> Hi Fred,
>> 
>> I apologize for letting this slide and not responding before. The
>> changes I made definitely resolved some specific problems that we
>> were seeing, but perhaps not solved in the best possible way. I
>> certainly understand your point about addressing any Windows-specific
>> issues in the backend or in the theme if possible. I just haven't had
>> time to revisit the issue to see if there is a way to do that. If you
>> have reverted my original fix then I will no doubt have to look into
>> it again soon! ;-)
>> 
>> I'll see if I can come up with a better solution this time. Working
>> with the Windows integration is a challenge, particularly the way
>> Windows has a habit of generating callbacks into our code while we're
>> in the middle of doing something else, which I believe was one of the
>> problems in this case.
>> 
>> Doug
>> 
>> On Oct 17, 2010, at 6:04 AM, Fred Kiefer wrote:
>> 
>>> I never got a reply on this mail. I will now undo this dubious
>>> change. If it was really required for the WinUX theme I hope that
>>> somebody will add a corresponding change into that theme. I really
>>> would have preferred to have some discussion on this subject.
>>> 
>>> Fred
>>> 
>>> Am 12.09.2010 20:49, schrieb Fred Kiefer:
>>>> Am 31.08.2010 01:02, schrieb Doug Simons:
>>>>> Author: dpsimons Date: Tue Aug 31 01:02:21 2010 New Revision:
>>>>> 31213
>>>>> 
>>>>> URL: http://svn.gna.org/viewcvs/gnustep?rev=31213&view=rev 
>>>>> Log: fix problem of pulldown action not being called for
>>>>> correct cell, and being called twice on Windows
>>>>> 
>>>>> Modified: libs/gui/trunk/ChangeLog 
>>>>> libs/gui/trunk/Source/NSMenuView.m 
>>>>> libs/gui/trunk/Source/NSPopUpButtonCell.m
>>>> 
>>>> Hi Doug,
>>>> 
>>>> could you please explain the first part of this change? The code
>>>> itself looks to me horribly wrong but I am sure you had good
>>>> reasons for it. The change note says it was needed for Windows,
>>>> but I cannot find any special handling for this case in our
>>>> Windows backend. This leads me to the assumption that you needed
>>>> this change to get the WinUX theme working. If this is correct,
>>>> wouldn't it be better to fix the theme instead? Currently we have
>>>> the basic idea that themes don't change any behaviour they only
>>>> result in a different appearance. If this isn't true for the
>>>> WinUX theme, and there may be good reasons for that, eg for a 
>>>> better Windows integration, it is the obligation of the theme to
>>>> keep the results at least consistent. Most likely the
>>>> processCommand: method of that theme will need some tweaking to
>>>> work correctly in your case. Could you please look into this and
>>>> undo the change on gui?
>>>> 
>>>> Fred
> 
> 
> _______________________________________________
> Gnustep-dev mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/gnustep-dev
>