Re: [Gnustep-cvs] r33234 - in /libs/gui/trunk: ChangeLog Source/GSThemeMenu.m

[Gregory Casamento]

Also, it's worth noting this problem only shows up in in-window menu
mode (with or without the GNOME theme)

GC

On Sat, Jun 4, 2011 at 11:55 AM, Gregory Casamento
<address@hidden> wrote:
> The application showing the issue currently is Gorm.   The only other
> application I've tried is Ink and the problem is not showing there.
>
> Here's the backtrace:
>
> #0  0xb761471c in objc_msg_lookup () from /usr/lib/libobjc.so.2
> #1  0xb7b3492d in -[NSApplication targetForAction:to:from:]
> (self=0x81614f0, _cmd=0xb7e1fbe0, theAction=0xb7dd1938,
> theTarget=0x84f37a0, sender=0x886bdb8) at NSApplication.m:2248
> #2  0xb7bfc2be in -[NSMenu update] (self=0x8925d78, _cmd=0xb7e1fb18)
> at NSMenu.m:1157
> #3  0xb7bfc50c in -[NSMenu update] (self=0x8999e18, _cmd=0xb7ea19d0)
> at NSMenu.m:1152
> #4  0xb7d36e19 in -[GSTheme(Menu) setMenu:forWindow:] (self=0x8299d58,
> _cmd=0xb7ea1a28, menu=0x82866b0, window=0x8954ab8) at GSThemeMenu.m:91
> #5  0xb7d371a6 in -[GSTheme(Menu) updateMenu:forWindow:]
> (self=0x8299d58, _cmd=0xb7ea1a50, menu=0x82866b0, window=0x8954ab8) at
> GSThemeMenu.m:149
> #6  0xb7d3729c in -[GSTheme(Menu) updateAllWindowsWithMenu:]
> (self=0x8299d58, _cmd=0xb7e1fc28, menu=0x82866b0) at GSThemeMenu.m:162
> #7  0xb7bfc5bf in -[NSMenu update] (self=0x82866b0, _cmd=0xb7dd16f0)
> at NSMenu.m:1216
> #8  0xb7b34134 in -[NSApplication run] (self=0x81614f0,
> _cmd=0xb7dc7268) at NSApplication.m:1567
> #9  0xb7b1b9ca in NSApplicationMain (argc=1, argv=0xbfffefc4) at 
> Functions.m:89
> #10 0x0804f8cb in main (argc=1, argv=0xbfffefc4) at main.m:30
>
> On Sat, Jun 4, 2011 at 6:10 AM, Fred Kiefer <address@hidden> wrote:
>> I see now that your change is again just a workaround. Would you mind to add
>> a comment, next time you add code that is most likely wrong?
>>
>> It surely would help to track down the real problem if you could share and
>> application showing the issue or at least a back trace of the problem.
>>
>> I had a short look at NSMenuItem and we seem to use the ivar _target for two
>> separate purposes. For the normal target of the menu item and as well for
>> the super menu, when the item holds a submenu. Could you please try to find
>> out, which of these two cases is the problematic one? You could do the
>> retain just in one of the cases (target being an NSMenu or not) and see
>> whether you get the problem. If it isn't the NSMenu target, then I would
>> expect that problem to be in a completely different place in your
>> application. Here a static code analysis or a memory debugger could help.
>>
>> I really would like to see this problem resolved this time. The change you
>> made looks wrong and somebody may remove it again at some point, thereby
>> breaking your application.
>>
>> Fred
>>
>> On 03.06.2011 10:19, Gregory Casamento wrote:
>>>
>>> Fred,
>>>
>>> Yes, at this point I do realize it was wrong.   I'm not entirely
>>> convinced that the fix I did this time around was correct since it
>>> involves retaining the _target ivar of the item.   However, the fix
>>> that is currently in place seems "more correct" than using the
>>> archiver as I was doing before.
>>>
>>> I think there is an extraneous release someplace, but, thus far, I'm
>>> unable to find it.
>>>
>>> Later, GC
>>>
>>> On Fri, Jun 3, 2011 at 3:41 AM, Fred Kiefer<address@hidden>  wrote:
>>>>
>>>> Thank you that you finally took the time to inspect the real issue and
>>>> fix it. In the meantime you surely noticed that the analysis you provided
>>>> below was wrong. The NSMenu did copy the NSMenuItems. The bug was in the
>>>> copy method of the item.
>>>>
>>>> It would have been a lot better if you had looked into the issue when it
>>>> first showed up, but everything should be fine now.
>>>>
>>>> Fred
>>>>
>>>>
>>>> Am 03.06.2011 um 00:24 schrieb Gregory
>>>> Casamento<address@hidden>:
>>>>
>>>>> Fred,
>>>>>
>>>>> It's actually pretty simple to see what's broken in the copyWithZone
>>>>> method in NSMenu.m.   That's what I was trying to explain in the
>>>>> "Revert".   I don't need to write a test program since it's plain to
>>>>> see on inspection that it's incorrect.
>>>>>
>>>>> The method does a recursive copy of the menu and simply inserts the
>>>>> menu items from the menu being copied into the copy.   As I explained
>>>>> in my comment, this causes all of the menus to share all of their
>>>>> NSMenuItems, which is not correct.
>>>>>
>>>>> It wasn't my intention to do a "silent revert" or anything of that
>>>>> nature.  I'm trying right now to fix some issues I'm seeing with the
>>>>> Gnome theme and had forgotten about the debate in March.  I'll try to
>>>>> fix this problem properly sometime this evening.
>>>>>
>>>>> Thanks, GC
>>>>>
>>>>> On Thu, Jun 2, 2011 at 5:47 PM, Fred Kiefer<address@hidden>  wrote:
>>>>>>
>>>>>> On 02.06.2011 22:22, Gregory Casamento wrote:
>>>>>>>
>>>>>>> Author: gcasa
>>>>>>> Date: Thu Jun  2 22:22:39 2011
>>>>>>> New Revision: 33234
>>>>>>>
>>>>>>> URL: http://svn.gna.org/viewcvs/gnustep?rev=33234&view=rev
>>>>>>> Log:
>>>>>>> Use the NSArchiver to copy the menu since the copy method does not do
>>>>>>> a
>>>>>>> deep enough copy.
>>>>>>>
>>>>>>> Modified:
>>>>>>>     libs/gui/trunk/ChangeLog
>>>>>>>     libs/gui/trunk/Source/GSThemeMenu.m
>>>>>>
>>>>>> Haven't we been through that before?
>>>>>>
>>>>>> There was a long mail exchange in March why this code was incorrect and
>>>>>> causing trouble to German. At that point I suggested that you report
>>>>>> back
>>>>>> with a short example program showing what is broken with the [NSMenu
>>>>>> -copyWithZone:] method. Instead you changed the code to use copy and
>>>>>> now you
>>>>>> changed it back and added this comment:
>>>>>>
>>>>>> /*
>>>>>> * NOTE: The reason the copy or copyWithZone method is not used here is
>>>>>> because
>>>>>> * it doesn't make a deep copy of the menu.  A deep copy is needed in
>>>>>> order
>>>>>> to
>>>>>> * allow the individual menu items to be used in multiple menus at one
>>>>>> time
>>>>>> without
>>>>>> * interfering with one another's state.
>>>>>> */
>>>>>>
>>>>>> This doesn't explain anything, nor will it ever help to resolve the
>>>>>> issue,
>>>>>> if there is any. It just breaks German's code again.
>>>>>>
>>>>>> Don't you think that reopening the debate with what ever new evidence
>>>>>> you
>>>>>> came up with would be a lot more productive then doing a silent revert?
>>
>>
>
>
>
> --
> Gregory Casamento - GNUstep Lead/Principal Consultant, OLC, Inc.
> yahoo/skype: greg_casamento, aol: gjcasa
> (240)274-9630 (Cell)
>



-- 
Gregory Casamento - GNUstep Lead/Principal Consultant, OLC, Inc.
yahoo/skype: greg_casamento, aol: gjcasa
(240)274-9630 (Cell)