Re: What happened to the code freeze?

[Fred Kiefer]

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.

>> I like all the simplifications you did for the menu update for the in
>> window menu. Why not hide all the details in the
>> setMenuChangedMessagesEnabled: method? That is have the change passed up
>> to the super menu and when we are the app menu, do what is needed there?
>> You will then miss out on some more menu changes, for example when the
>> main menu doesn't have autoenabled items. Looks like I need to think
>> about this some more.
> 
> Good point. I've moved the call to menuChanged into all of the individual 
> methods that note the changes.

Great.

>> The changes to NSApplication I don't like. Why should the application be
>> concerned about the display of the main menu at all? At least one of
>> these could be moved into NSMenu -setMain: and perhaps that change could
>> take care of the second call as well?
> 
> Moving the one call into [NSMenu setMain:] is reasonable. Unfortunately, the 
> call to updateAllWindowsWithMenu: in _didFinishLaunching is necessary, now 
> that 
> it isn't being called all the time from [NSMenu update]. Without this call 
> the 
> menus never get built. I guess either setMain: isn't being called at launch, 
> or 
> it is called too early to be effective.

This is rather sad. Anybody out there with an idea how to resolve this?
We could of course have the main menu listen for the
NSApplicationDidFinishLaunchingNotification notification, but this looks
like overkill for this small issue. But then we already do! We call
_showTornOffMenuIfAny: inthat case. Why not add your code there?
Perhaps we should rename this method then, but it looks like the right
place to me.

>> Your patch also seems to have an indentation problem are you using tabs?
> 
> Sorry, it looks consistent to me. I'm using Notepad++ on Windows. I guess 
> I'll 
> have to fiddle with the settings and see if I can get it to conform properly. 
> Does anyone know the right settings to use?

Us two spaces in stead of tabs or set your tab depth to two, then I wont
notice the difference :-)