gnustep-dev
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Gnustep-cvs] r33234 - in /libs/gui/trunk: ChangeLog Source/GSThemeM


From: Fred Kiefer
Subject: Re: [Gnustep-cvs] r33234 - in /libs/gui/trunk: ChangeLog Source/GSThemeMenu.m
Date: Sat, 04 Jun 2011 22:39:22 +0200
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; de; rv:1.9.2.17) Gecko/20110414 SUSE/3.1.10 Thunderbird/3.1.10

That is still the goal, but for now we should try to get the code working and the hack out again. And perhaps somebody will start working on that bigger goal.

On 04.06.2011 22:25, Eric Wasylishen wrote:
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?




reply via email to

[Prev in Thread] Current Thread [Next in Thread]