316076 - complete menu implementation Cocoa widgets

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Josh Aas]

We need to complete the menu implementation in Cocoa widgets in order to switch Firefox and Thunderbird over to using Cocoa widgets. This bug exists for patches to that end, and should be closed when the menu implementation is considered to be basically complete.

Comment 1

[Josh Aas]

Attachment: make menus functional v1.0

This patch makes menu items functional. This does not complete the implementation because we lack a real Application menu, we need to solve the problem that bug 313185 solves (a bug related to how we set up the Application menu), and we have  a few more major bugs (including some enable/disable menu items bugs). Also, there is a class of bugs related to the way the menus interact with windows, but they are not necessarily menu code problems. I will probably be posting at least 2 more patches before this bug can be closed and we can move the normal mode of just fixing bugs one at a time.

The way we deliver command events is to get the Cocoa IBAction notification and send a synthetic Carbon event. Then the event handler we've always had in the MenuBarX code handles the event. This works pretty well and means we don't have to rearchitect these files completely now (that is unnecessary).

Comment 2

[Mark Mentovai]

Comment on attachment 202696 [details] [diff] [review]
make menus functional v1.0

>Index: cocoa/nsMenuX.mm
>===================================================================
>RCS file: /cvsroot/mozilla/widget/src/cocoa/nsMenuX.mm,v
>retrieving revision 1.3
>diff -u -r1.3 nsMenuX.mm
>--- cocoa/nsMenuX.mm	26 Oct 2005 23:25:19 -0000	1.3
>+++ cocoa/nsMenuX.mm	11 Nov 2005 18:21:54 -0000
>-  }
>-  else {
>+  } else {

You changed a bunch of these to "snuggly" elses, but I don't like 'em.  Could we go back to

}
else {

again?

>+      err = ::SendEventToEventTargetWithOptions(newEvent, GetWindowEventTarget((WindowRef)[[NSApp keyWindow] windowRef]), kEventTargetSendToAllHandlers);

We should be able to get away with SendEventToEventTarget, without the WithOptions.  I'd rather not override normal propagation if it's not absolutely necessary, and I don't see why it would be necessary here.

While you're working in nsMenuBarX.mm, you should address this warning:

nsMenuBarX.mm: In member function 'virtual nsresult nsMenuBarX::AddMenu(nsIMenu*)':
nsMenuBarX.mm:501: warning: unused variable 'rv'

The patch is OK and it looks like it should work, but I've got questions about the ownership model.  Would it make more sense for each nsMenuItemX to own its own NSMenuItem?  Off the top of my head, some of the setEnabled: and setState: stuff would be cleaner if that were the case.  Also, I question calling setEnabled: and setState: on all new NSMenuItems instead of relying on the defaults.

I'd still give r+ to the patch without the preceding paragraph addressed to help us get bootstrapped, but it's more cleanup that you should keep in mind (or maybe you've already got it in mind?)

Comment 3

[Mark Mentovai]

> You changed a bunch of these to "snuggly" elses, but I don't like 'em.

Apparently, "} else {" is official Moz style.  Oh well.

Comment 4

[Josh Aas]

As for the "snuggly" "else", that is Mozilla coding style. I don't mind getting rid of it though. As for using "SendEventToEventTarget", sure. Same for fixing the unused variable warning.

I do plan on doing something along the lines of what you outlined with nsMenuItemX. However, for bootstrapping purposes I am waiting to do that later. I definitely have it in mind.

Comment 5

[Mark Mentovai]

Comment on attachment 202696 [details] [diff] [review]
make menus functional v1.0

Cool.

Comment 6

[Josh Aas]

Comment on attachment 202696 [details] [diff] [review]
make menus functional v1.0

requesting sr, will check in with Mark's comments addressed

Comment 7

[Josh Aas]

Attachment: make menus functional v1.1

Same as v1.0, but addresses Mark's comments and swaps out nsSupportsArray (deprecated) for nsCOMArray

Comment 8

[Josh Aas]

When I check in, I'll remove |#include "nsSupportsArray.h"| from nsMenuX.h, forgot to do that in my patch.

Comment 9

[Mark Mentovai]

Comment on attachment 203548 [details] [diff] [review]
make menus functional v1.1

>-    nsSupportsArray             mMenuItemsArray;        // array holds refs
>+    nsCOMArray<nsISupports>     mMenuItemsArray;

I think it's bogus that nsIMenu and nsIMenuItem don't share any common ancestor other than nsISupports, but OK.

> @@ -347,24 +344,23 @@
> {
>   // We're not really appending an nsMenuItem but it needs to be here to make
>   // sure that event dispatching isn't off by one.
>-  mMenuItemsArray.AppendElement(&gDummyMenuItemX); // owning ref
>-  PRUint32  numItems;
>-  mMenuItemsArray.Count(&numItems);
>+  mMenuItemsArray.AppendObject(&gDummyMenuItemX); // owning ref
>   [mMacMenu addItem:[NSMenuItem separatorItem]];
>-  mNumMenuItems++;
>   return NS_OK;
> }

Good catch on the unused variable.

> NS_IMETHODIMP nsMenuX::GetItemAt(const PRUint32 aPos, nsISupports *& aMenuItem)
> {
>-  mMenuItemsArray.GetElementAt(aPos, &aMenuItem);
>+  aMenuItem = mMenuItemsArray.ObjectAt(aPos);
>+  NS_IF_ADDREF(aMenuItem);

Why addref?

> NS_IMETHODIMP nsMenuX::RemoveItem(const PRUint32 aPos)
> {
>-  //XXXJOSH Implement this
>-  NS_WARNING("Not implemented");
>+  if (mMacMenu != NULL) {

I'd use nil on objc objects.

>+    // clear command id
>+    nsCOMPtr<nsIMenuCommandDispatcher> dispatcher(do_QueryInterface(mManager));
>+    if (dispatcher) {
>+      dispatcher->Unregister((PRUint32)[[mMacMenu itemAtIndex:aPos] tag]);

Will we have command-ID-less menu items?  Separators?  The same question applies in nsMenuX::RemoveAll().

> NS_IMETHODIMP nsMenuX::RemoveAll()
> {
>-  //XXXJOSH why don't we set |mNumMenuItems| to 0 after removing all menu items?
>   if (mMacMenu != NULL) {
>-    /*
>     // clear command id's
>     nsCOMPtr<nsIMenuCommandDispatcher> dispatcher(do_QueryInterface(mManager));
>     if (dispatcher) {
>-      for (unsigned int i = 1; i <= mNumMenuItems; ++i) {
>-        PRUint32 commandID = 0L;
>-        OSErr err = ::GetMenuItemCommandID(mMacMenu, i, (unsigned long*)&commandID);
>-        if (!err)
>-          dispatcher->Unregister(commandID);
>-      }
>+      for (int i = 0; i < [mMacMenu numberOfItems]; i++)
>+        dispatcher->Unregister((PRUint32)[[mMacMenu itemAtIndex:i] tag]);
>     }
>-     */
>-    for (int i = [mMacMenu numberOfItems] - 1; i >= 0; i--) {
>+    // get rid of Cocoa menu items
>+    for (int i = [mMacMenu numberOfItems] - 1; i >= 0; i--)
>       [mMacMenu removeItemAtIndex:i];
>-    }

Won't there usually be a dispatcher?  Rather than looping twice, it's probably better (and certainly reads more easily) to loop one time, bottom to top.

The thing that's slightly scary about these removals is that NSMenuItems disappear well before their corresponding nsMenuX/nsMenuItemX where there are submenus, so destruction isn't cleanly nested and I'm paranoid about GC bogeymen.  This won't be an issue when ownership is fixed up.

Comment 10

[Josh Aas]

(In reply to comment #9)

> I think it's bogus that nsIMenu and nsIMenuItem don't share any common ancestor
> other than nsISupports, but OK.

Don't forget nsDummyMenuItemX - its only ancestor is nsISupports.

> >-  mMenuItemsArray.GetElementAt(aPos, &aMenuItem);
> >+  aMenuItem = mMenuItemsArray.ObjectAt(aPos);
> >+  NS_IF_ADDREF(aMenuItem);
> 
> Why addref?

Calling GetElement on an nsSupportsArray addrefs on the way out. That is one of the major problems with nsSupportsArray - you can't even look at an item without adrefing it. So, the equivalent code for nsCOMArray needs to manually addref since nsCOMArray doesn't addref as nsSupportsArray.

> I'd use nil on objc objects.

OK. I'll fix it on checkin.
 
I'll look into the rest of your comments for the next patch if that is OK. Maybe I will change the ownership model in that patch so many of those comments won't be relevant any more.

Comment 11

[Mark Mentovai]

Comment on attachment 203548 [details] [diff] [review]
make menus functional v1.1

Yeah, this is OK since it's in progress, the ownership changes should make a lot of these problems moot.

Comment 12

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 203548 [details] [diff] [review]
make menus functional v1.1

rs=pink

Comment 13

[Josh Aas]

landed "make menus functional v1.1" on trunk.

Comment 14

[Josh Aas]

Attachment: menu item refactoring, v1.0

Putting this here to store it, don't review yet. I might post an update before I ask for reviews.

Comment 15

[Mark Mentovai]

Comment on attachment 204391 [details] [diff] [review]
menu item refactoring, v1.0

Ooh, a brief reprieve!

Comment 16

[Josh Aas]

Attachment: menu item refactoring, v1.1

This is the same as v1.0 except it includes a fix for menu item disabling. Enable/disable should work just fine now.

This patch is getting a bit large and I don't want to do more until this is in. As usual, more to follow this patch. I still need to examine ownership in the menu system more closely, it is on my list.

Comment 17

[Mark Mentovai]

Comment on attachment 204484 [details] [diff] [review]
menu item refactoring, v1.1

> NS_METHOD nsMenuItemX::SetModifiers(PRUint8 aModifiers)
>+{  
>+  mModifiers = aModifiers;
>+
>+  // set up shortcut key modifiers on native menu item
>+  unsigned int macModifiers = 0;
>+  if (knsMenuItemShiftModifier & mModifiers)
>+    macModifiers |= NSShiftKeyMask;
>+  if (knsMenuItemAltModifier & mModifiers)
>+    macModifiers |= NSAlternateKeyMask;
>+  if (knsMenuItemControlModifier & mModifiers)
>+    macModifiers |= NSControlKeyMask;
>+  if (knsMenuItemCommandModifier & mModifiers)
>+    macModifiers |= NSCommandKeyMask;
>+  [mNativeMenuItem setKeyEquivalentModifierMask:macModifiers];
>+  
>+  return NS_OK;
> }

What's with the freaky syntax ripped out of nsMenuX?  Change these to (mModifiers & konstant) and r=mento.

Comment 18

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 204484 [details] [diff] [review]
menu item refactoring, v1.1

rs=pink

Comment 19

[Josh Aas]

"menu item refactoring, v1.1" landed on trunk

Comment 20

[Josh Aas]

Attachment: app menu swapping, nsSupportsArray removal v1.0

This does 2 things. First, it removes nsSupportsArray usage from nsMenuBarX. Second, it cleans up the way we handle the application menu so we definitely don't leak memory, and we correctly share the app menu across windows.

Comment 21

[Josh Aas]

Attachment: work around application menu blessing, v1.0

This works around an Apple bug (a couple, actually) that prevents us from manipulating the application menu programatically. The only way around it is to load a nib, then grab its application menu and use that. This patch requires adding the nib file to the tree and checking in the patch from bug 313185.

Comment 22

[Josh Aas]

"app menu swapping, nsSupportsArray removal v1.0" was landed on the trunk (forgot to note that)

Comment 23

[Josh Aas]

Attachment: nib file for cocoa widgets

This is the nib file for cocoa widgets. It has no l10n requirements as we don't use any text in it. The important thing is that it contains a serialized Application menu, which is something we can't set up programatically.

Comment 24

[Josh Aas]

Attachment: enable popup window creation, v1.0

This enables the creation of an actual window for popup windows in apps using Cocoa widgets *other than Camino*. Camino actually does create |nsCocoaWindow|s for accounting, so that object just needs to bail before creating the native widget.

There was some discussion of how to do this without messing up Camino, and I think this patch reflects the proposed solution. This does allow popup window creation, but the window still doesn't do anything useful like draw its contents :(

Comment 25

[Benjamin Smedberg]

Comment on attachment 206969 [details] [diff] [review]
work around application menu blessing, v1.0

I don't understand why we can't use loadNibFile:externalNameTable:withZone: forming the absolute path from the directory service, which avoids the problem of having to put the nib file in the application bundle (which will seriously screw the xulrunner runtime bundle packaging).

Comment 26

[Josh Aas]

(In reply to comment #25)
> (From update of attachment 206969 [details] [diff] [review] [edit])
> I don't understand why we can't use loadNibFile:externalNameTable:withZone:
> forming the absolute path from the directory service, which avoids the problem
> of having to put the nib file in the application bundle (which will seriously
> screw the xulrunner runtime bundle packaging).

We may be able to, I just don't work on xulrunner and I don't know what you want so I didn't do that. Might I suggest that we take my patch, and then you can put another patch on top that fixes things up for xulrunner in terms of nib loading? So long as the nib loads for Firefox and other non-xulrunner apps, I don't care how you load the nib (*when* does matter, don't change the timing of the load). This won't even be an issue for you until xulrunner uses Cocoa widgets, so you'd have plenty of time to decide how you want to do it and put a patch together.

Comment 27

[Mark Mentovai]

Comment on attachment 206969 [details] [diff] [review]
work around application menu blessing, v1.0

Code's OK, but we need to get some sort of final resolution on 313185.

Comment 28

[Mark Mentovai]

Comment on attachment 206969 [details] [diff] [review]
work around application menu blessing, v1.0

Let's get practical.  bsmedberg, tell us exactly where you envision this nib living in the current-generation trunk Firefox, in XUL.framework, and in XULRunner-bundled apps.

Comment 29

[Benjamin Smedberg]

Attachment: Place the nib in dist/bin/res and load it manually (untested)

This is what I'm envisioning. I haven't gotten my build done yet, and I've never written objective-c before, so the mac APIs might be vastly wrong.

Comment 30

[Mark Mentovai]

Comment on attachment 207350 [details] [diff] [review]
enable popup window creation, v1.0

Good.  Could you just add a comment in nsCocoaWindow::StandardCreate about why we do this for Camino, like the comment in nsComboboxControlFrame::ToolkitHasNativePopup()?

Comment 31

[Benjamin Smedberg]

Attachment: Place the nib in dist/bin/res and load it manually, rev. 1

This at least builds...

Comment 32

[Josh Aas]

Comment on attachment 207542 [details] [diff] [review]
Place the nib in dist/bin/res and load it manually, rev. 1

Your patch only builds if you take the internal files out of the nib file and put them straight into widget/src/cocoa. We should keep them in a directory called "MainMenu.nib" for organizational and clarity purposes.

Also, putting the nib into Contents/MacOS/res in the final app package is an odd thing to do. Traditionally this goes into Content/Resources. It seems like you want it where you put it because it is in the GRE  dir, so I don't know if the traditional placement is going to work for xulrunner. Not such a big deal if this can be fixed or not.

+    withZone: NSDefaultMallocZone()

no space between colon and the argument

Comment 33

[Josh Aas]

Comment on attachment 207542 [details] [diff] [review]
Place the nib in dist/bin/res and load it manually, rev. 1

Requesting a review from Mark even though I already gave it r- because it would be good to have more comments before respinning this patch.

Comment 34

[Simon Fraser [no longer active]]

Comment on attachment 207542 [details] [diff] [review]
Place the nib in dist/bin/res and load it manually, rev. 1

>   // this call initializes NSApplication
>-  [NSApplication sharedApplication];
>+  [NSBundle loadNibFile:
>+    [NSString stringWithUTF8String:(const char*)nibPath.get()]
>+    externalNameTable:
>+      [NSDictionary dictionaryWithObject:
>+        [NSApplication sharedApplication]
>+        forKey:@"NSOwner"
>+      ]
>+    withZone: NSDefaultMallocZone()
>+  ];
> 	return NS_OK;
> }

You could just use loadNibNamed:owner: here. It's equivalent.

Comment 35

[Josh Aas]

> You could just use loadNibNamed:owner: here. It's equivalent.

I think that call only searches in a few places for the nib with the given name. bsmedberg is putting the nib in a non-standard location, so I'm not sure that will work. I haven't tried it though.

Comment 36

[Simon Fraser [no longer active]]

(In reply to comment #35)
> > You could just use loadNibNamed:owner: here. It's equivalent.
> 
> I think that call only searches in a few places for the nib with the given
> name. bsmedberg is putting the nib in a non-standard location, so I'm not sure
> that will work. I haven't tried it though.

Ah, looks like you're right. I don't like the nib being in an non-standard location though.

Comment 37

[Benjamin Smedberg]

The nonstandard location is required so that we don't have to package the nib with each xulrunner app. I don't really care what the location is exactly, as long as it lives somewhere under Contents/MacOS/* in the Firefox application bundle, which is equivalent to Versions/1.8/* in the XULRunner framework.

Comment 38

[Mark Mentovai]

Comment on attachment 207542 [details] [diff] [review]
Place the nib in dist/bin/res and load it manually, rev. 1

For a non-XR application, the nib would belong in *.app/Contents/Resources; for a framework, it belongs in *.framework/Resources (symlink to *.framework/Versions/*/Resources).  I'd prefer to get the nib in the proper location for both apps that don't use/aren't built by the framework and for the framework too.

I also echo Josh's concern that these files belong in a .nib directory in the source tree.  All nibs contain the same three files (confusingly also given the .nib extension), so this arrangement precludes supplying multiple .nibs from a single source directory.  More importantly, it makes the nib difficult to find in the source tree.

Comment 39

[Simon Fraser [no longer active]]

(In reply to comment #38)

> I also echo Josh's concern that these files belong in a .nib directory in the
> source tree.

Agreed. You won't be able to open the nib in Interface Builder if it isn't in a .nib dir. The NIB file is really a package, and should be treated as such.

Comment 40

[Benjamin Smedberg]

> For a non-XR application, the nib would belong in *.app/Contents/Resources; for
> a framework, it belongs in *.framework/Resources (symlink to
> *.framework/Versions/*/Resources).  I'd prefer to get the nib in the proper
> location for both apps that don't use/aren't built by the framework and for the
> framework too.

Does loadNibNamed:owner: load from the XULRunner framework (remembering that you have to load it from "this" particular version of XULRunner, not from the generic directory)? I don't understand the dogmatic fascination with putting this resource file in Contents/Resources in the bundle when we put all of our other resource files (chrome etc) in dist/bin/res or dist/bin/chrome. Unless there is a very good reason to follow the standard mac packaging scheme which I'm missing, we have a good reason and precedent to go our own way.

> I also echo Josh's concern that these files belong in a .nib directory in the
> source tree.  All nibs contain the same three files (confusingly also given 

That's fine, I don't care about that.

Comment 41

[Benjamin Smedberg]

Attachment: Place the nib in dist/bin/res and load it manually, rev. 1.1

Comment 42

[Josh Aas]

Comment on attachment 207999 [details] [diff] [review]
Place the nib in dist/bin/res and load it manually, rev. 1.1

A few more changes to consider:

1) Can we put the nib, MainMenu.nib, inside of a resources folder in cocoa widgets, just in case for some other reason we need to add more resources.
2) The cocoa indentation is odd. Generally you keep arguments aligned, more like this:

+[NSBundle loadNibFile:[NSString stringWithUTF8String:(const char*)nibPath.get()]
+    externalNameTable:[NSDictionary dictionaryWithObject:[NSApplication sharedApplication] forKey:@"NSOwner"]
+             withZone:NSDefaultMallocZone()];

3) Can we package the nib in the standard location (and use the standard call) for non-xulrunner apps? That should be easy to do.

Comment 43

[Mark Mentovai]

Attachment: nsAppShellCocoa with variable nib locations

I don't care so much about Josh's first suggestion, and agree with the second.  As for the third, I don't see what's so difficult about #ifdef MOZ_XULRUNNER.  I'm putting an untested implementation in this attachment.  I haven't done the Makefile changes yet; as described in bug 313185, that will at least need changes to each app to copy a Resources folder to the final app bundle.

Comment 44

[Benjamin Smedberg]

Not only are app-specific (MOZ_XULRUNNER) ifdefs not allowed in gecko (tier 9) code, but I do not want to introduce packaging ifdefs like this. I am opposed to putting anything in the appbundle/Contents/Resources that isn't actually required by the OS to launch the bundle properly.

Comment 45

[Josh Aas]

bsmedberg has landed "Place the nib in dist/bin/res and load it manually, rev. 1.1" on the trunk.

Comment 46

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 207350 [details] [diff] [review]
enable popup window creation, v1.0

sr=pink 

one little nit, as long as you're cleaning up the spaces, remove the space between BaseCreate and the paren.

Comment 47

[Josh Aas]

"enable popup window creation, v1.0" has landed on the trunk. I have filed bug 323062 to address issues this patch may cause with xulrunner in the future. We need to move ahead with the cocoa popup windows so we can turn Cocoa widgets on on the trunk.

Comment 48

[Josh Aas]

Attachment: app menu work in progress, v0.8

This is just for comments and serves as a backup for me. Not asking for reviews.

Comment 49

[Josh Aas]

Attachment: Application menu impl, part 1

I just landed this patch on the trunk. It reorganizes some things to get event handling working and adds support for the Application menu. It does not handle key equivalents, that is up next. Also, we'll need to update all in-tree applications' content with the requisite DOM elements.

This is basically a safe improvement upon the Carbon widget Application menu. It adds the ability to control the text and key equivalents for supported items, and you can choose to exclude items you don't want. I can't believe what we do in Carbon actually worked for so long.

Comment 50

[jhp (no longer active)]

Comment on attachment 209058 [details] [diff] [review]
Application menu impl, part 1

Did you also want to move "Check for Updates..." to the app menu, or will you handle that in a different bug?

Comment 51

[Josh Aas]

Let's make that a different bug.

Comment 52

[Josh Aas]

Attachment: Application menu impl, part 2

This patch adds support for keyboard equivalents in the Application menu, some other cleanup. It has landed on the trunk due to a kind 3 AM sr by sparky the wonder dog.

Comment 53

[Josh Aas]

That's all folks. If there are more Cocoa menus issues, they should be filed as separate bugs.

Note that none of the in-tree apps have a correct Application menu under the new Cocoa menus world order. Will file bugs on each app. Here is a copy of the comment I put in nsMenuBarX.mm:

/*
  We support the following menu items here:

  Menu Item             DOM Node ID             Notes
  
  ==================
  = About This App = <- aboutName
  ==================
  = Preferences... = <- menu_preferences
  ==================
  = Services     > = <- menu_mac_services    <- (do not define key equivalent)
  ==================
  = Hide App       = <- menu_mac_hide_app
  = Hide Others    = <- menu_mac_hide_others
  = Show All       = <- menu_mac_show_all
  ==================
  = Quit           = <- menu_FileQuitItem
  ==================
  
  If any of them are ommitted from the application's DOM, we just don't add
  them. We always add a "Quit" item, but if an app developer does not provide a
  DOM node with the right ID for the Quit item, we add it in English. App
  developers need only add each node with a label and a key equivalent (if they
  want one). Other attributes are optional. Like so:
  
  <menuitem id="menu_preferences"
         label="&preferencesCmdMac.label;"
           key="open_prefs_key"/>
  
  We need to use this system for localization purposes, until we have a better way
  to define the Application menu to be used on Mac OS X.
*/

This can of course change based on comments and experience, but if so it should happen in  another bug.

Comment 54

[Josh Aas]

Attachment: Application menu impl, part 3

Fixed a memory leak and a key modifier issue with the Application menu code. This is checked in.