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

[Gregory Casamento]

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?
>>>
>>> _______________________________________________
>>> Gnustep-dev mailing list
>>> address@hidden
>>> https://lists.gnu.org/mailman/listinfo/gnustep-dev
>>>
>>
>>
>>
>> --
>> Gregory Casamento - GNUstep Lead/Principal Consultant, OLC, Inc.
>> yahoo/skype: greg_casamento, aol: gjcasa
>> (240)274-9630 (Cell)
>>
>> _______________________________________________
>> Gnustep-dev mailing list
>> address@hidden
>> https://lists.gnu.org/mailman/listinfo/gnustep-dev
>



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