[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Gnustep-cvs] r33234 - in /libs/gui/trunk: ChangeLog Source/GSThemeM
From: |
Eric Wasylishen |
Subject: |
Re: [Gnustep-cvs] r33234 - in /libs/gui/trunk: ChangeLog Source/GSThemeMenu.m |
Date: |
Sat, 4 Jun 2011 14:25:55 -0600 |
Hey,
I'm not too familiar with the code, but I think NSMenu[Item] are model objects
- it seems wrong to be copying them to set up different views (normal openstep,
top of window, top of screen, etc.).
I just wanted to repeat a comment from March from Wolfgang and Fred:
>> So, in the long run I'd prefer NSMenu were changed to handle multiple
>> menu views rather than having this menu duplication cruft. But it is
>> certainly too late for the next release ...
>
> This surely is the way to go. NSMenu should loose its two windows and
> the direct reference to the NSMenuView. All these connections should be
> the other way around. Hopefully we make some progress in that direction
> after the next release.
This is still the goal, right?
Eric
On 2011-06-04, at 4:10 AM, Fred Kiefer 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?
>
>
> _______________________________________________
> Gnustep-dev mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/gnustep-dev
- Re: [Gnustep-cvs] r33234 - in /libs/gui/trunk: ChangeLog Source/GSThemeMenu.m, Fred Kiefer, 2011/06/02
- Re: [Gnustep-cvs] r33234 - in /libs/gui/trunk: ChangeLog Source/GSThemeMenu.m, Gregory Casamento, 2011/06/02
- Re: [Gnustep-cvs] r33234 - in /libs/gui/trunk: ChangeLog Source/GSThemeMenu.m, Fred Kiefer, 2011/06/03
- Re: [Gnustep-cvs] r33234 - in /libs/gui/trunk: ChangeLog Source/GSThemeMenu.m, Gregory Casamento, 2011/06/03
- Re: [Gnustep-cvs] r33234 - in /libs/gui/trunk: ChangeLog Source/GSThemeMenu.m, Fred Kiefer, 2011/06/04
- Re: [Gnustep-cvs] r33234 - in /libs/gui/trunk: ChangeLog Source/GSThemeMenu.m, Gregory Casamento, 2011/06/04
- Re: [Gnustep-cvs] r33234 - in /libs/gui/trunk: ChangeLog Source/GSThemeMenu.m, Gregory Casamento, 2011/06/04
- Re: [Gnustep-cvs] r33234 - in /libs/gui/trunk: ChangeLog Source/GSThemeMenu.m, Gregory Casamento, 2011/06/04
- Re: [Gnustep-cvs] r33234 - in /libs/gui/trunk: ChangeLog Source/GSThemeMenu.m, Fred Kiefer, 2011/06/04
- Re: [Gnustep-cvs] r33234 - in /libs/gui/trunk: ChangeLog Source/GSThemeMenu.m, Gregory Casamento, 2011/06/04
- Re: [Gnustep-cvs] r33234 - in /libs/gui/trunk: ChangeLog Source/GSThemeMenu.m,
Eric Wasylishen <=
- Re: [Gnustep-cvs] r33234 - in /libs/gui/trunk: ChangeLog Source/GSThemeMenu.m, Fred Kiefer, 2011/06/04