Re: What happened to the code freeze?

[Doug Simons]

On Apr 23, 2010, at 2:21 AM, Fred Kiefer wrote:

Am 23.04.2010 07:42, schrieb Doug Simons:

On Apr 22, 2010, at 1:41 AM, Fred Kiefer wrote:

You change to set the super menu on a decoded submenu in NSMenu is most

likely wrong. This value already gets set in NSMenuItem +setSubmenu: and

before doing so the code checks that the old super menu is nil. I don't

see how your code could have ever worked.

If there was an issue with the super menu not being set correctly we

need to investigate it. I don't think this is the right fix for it.

Okay, on looking at this more closely now, I think you are right about the call

to setSupermenu: while decoding. I'm not sure of exactly what was happening (and

don't have time to debug it at the moment), but the supermenu was nil in all of

the submenus of our main menu (which is loaded from a nib file). The new call to

setSupermenu: in insertItem:atIndex: seems to be enough to fix the problem for

me. My guess now is that the root problem is probably in the nib loading code

for the menu. But the fix as it stands now shouldn't be a problem I think.

Your new change to this code surely wont break things as the old one
did. Still I am not convinced that this is needed. And looking at the
code surely proves this. I believe you that it makes a difference for
you and we need to find out where this difference comes from. If we
don't we not only clutter GNUstep with unneeded code, we will also
surely break things the next time this code gets touched. As a
maintainer I am not only thinking about current code correctness, but
try to assure we can guarantee this over a longer time. Sorry, for
causing you extra work here. On the other hand you would expect me to do
the same for other peoples changes, or for that for my own ones.

Your change in NSMenu looks like this (last line added):

 // Set this after the insert notification has been sent.
 [newItem setMenu: self];
 [[newItem submenu] setSupermenu:self];

And in NSMenuItem we have:

- (void) setMenu: (NSMenu*)menu
{
 /* The menu is retaining us.  Do not retain it.  */
 _menu = menu;
 if (_submenu != nil)
   {
     [_submenu setSupermenu: menu];
     [self setTarget: _menu];
   }
}

In which cases could your code make any difference?
- newItem could not be of class NSMenuItem.

- submenu returned from the method could be different from the slot.
This is not the case for NSMenuItem, so it falls back to the first case.

- setTarget: on the menu item does something strange. This is also not
the case for NSMenuItem.

- There is a redefinition of setMenu: somewhere.

The most likely reason seems to be that we are not dealing with an
NSMenuItem here. Could you please check that?

What is strange when comparing your new patch with your old is that when
NIB loading the new patch will now set the menu twice and the NIB
loading code will set it to nil again and then get it set again via
setSubmenu:forItem:. This all looks so completely wrong. I really would
prefer to understand this before we plaster it over.

Okay, it looks like I was mistaken about some things. The hazards of coding after my usual bedtime, I suppose. ;-)

It turns out that the call to setSupermenu: that I added (in the insert method) does absolutely nothing. It was the change I made to setSupermenu:self instead of setSupermenu:nil during decoding that fixed the problem for me. But I didn't look closely enough at the time to fully understand why that fixed it. As you've astutely observed, that looks like it ought to cause problems. But what's actually happening is that everything is already set at that point. I should have paid more attention to the FIXME comment at that point in the code:

      // FIXME: We propably don't need this, as all the fields are

      // already set up for the submenu item.

      if (sub != nil)

{

 [sub setSupermenu: nil];

 [self setSubmenu: sub  forItem: item];

}

By changing it to call [sub setSupermenu:self] what I achieved was a no-op, since the supermenu was already set by the earlier call to [self addItem:item]. The next line was also a no-op, since item's submenu was also set already at this point. The root of the problem was the call to [sub setSupermenu: nil] which wiped out the supermenu that was already set correctly!

So the correct fix is simply to delete that block of code (and the declaration and assignment of 'sub' a few lines earlier). I've done that, and also delete the other call to setSupermenu: that I had added.

 

Doug

NSMenu.m
Description: Binary data