Re: What happened to the code freeze?

[Fred Kiefer]

Am 23.04.2010 19:59, schrieb 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.

If that works for you, all the better. It is always great to remove some
code :-)

Did you also read the rest of my mail? What do you think about the
suggestion to move the last menu theme call out of NSApplication?

With that (and the indentation) sorted out you should be able to push
your change.

Fred