531903 - Identify the Help menu by a reference in the nib, rather than by index

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Follow-up to bug 413960: 

>+  // Make sure the help menu is identified correctly, since it may be localized.
>+  // TODO: Add a reference to the Help menu once we aren't nib-frozen, rather
>+  // than getting it by position.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #0)
> Follow-up to bug 413960: 

Er, bug 525758.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

We should make sure we don't add more work to bug 569111 when doing this. ;)

Also, I poked Wevah at the last meeting about perhaps fixing this.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Chris, any chance this bug is something you can tackle?

Comment 4

[Chris Peterson [:cpeterson]]

Camino's "Editing Nibs" wiki page says all nib work should be done with Interface Builder 2.5.x on Mac OS X 10.4, but I only have IB 3.2 and Mac OS X 10.6.

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

We can find someone to re-save it before check-in, which is what we've been doing ever since we discovered the perf issue (and actually we've recently used MainMenu saved on 10.5 without the perf hit).

Comment 6

[Chris Peterson [:cpeterson]]

Attachment: set-help-menu-by-nib-tag-v1.patch

Find Help menu by nib tag, not menu index.

I haphazardly chose kHelpMenuItemTag = 7000 because the Help menu is the 7th from the left (assuming the Camino menu is 0). Does Camino have an established numbering convention for nib tag constants I should follow instead?

Comment 7

[Chris Peterson [:cpeterson]]

Attachment: MainMenu.nib.zip

English.lproj/MainMenu.nib with Help menu tag = 7000

I edited this nib with Interface Builder 3.2 on Mac OS X 10.6. Someone else will need to resave this file with IB 2.5 on Mac OS X 10.4 (as per Camino's "Editing Nibs" wiki).

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 478509 [details] [diff] [review]
set-help-menu-by-nib-tag-v1.patch

This needs to be reviewed by someone with some actual Obj-C knowledge and by someone on 10.6 who can give it a cursory test with a fake localization; I don't fit either of those ;) 

(cl, duplicate English.lproj as French.lproj or the like, change French MainMenu's "Help" to "Aide" or something in IB, launch Camino with the pseudo-French l10n active, and check for the Spotlight field.)

If we have a system for tag assignment, it predates the memory of any of us still here ;)  "Don't conflict with anything currently out there" seems like a good rule, and 7000 definitely passes that test.

Two drive-by comments I can make, though:

>+- (void)setupSpotlightForHelp;

"setup" is not a verb, but "set up" is; this should be setUpSpotlightForHelp.  We're inconsistent across the codebase, but we're holding the line at new incorrect usages and gradually cleaning up the old ones ;)

>+  // Mac OS X 10.4 and 10.5 automatically add the "Spotlight for Help" menu item

Spotlight field in Help is 10.5+, so lose the 10.4 there.

Comment 9

[philippe (part-time)]

Testing this on 10.6 with Japanese (help --> ヘルプ) and French: in both cases, after switching languages and logging out and back in to have the whole OS use the language, the help menu was fully functional, including the search box.

Comment 10

[Chris Peterson [:cpeterson]]

Attachment: set-help-menu-by-nib-tag-v2.patch

Incorporate Smokey's feedback: fixed 10.4 comments and renamed "setup" to "setUp" (including some other "setup" functions in the same .mm file)

Comment 11

[Chris Lawson (gone)]

Comment on attachment 478667 [details] [diff] [review]
set-help-menu-by-nib-tag-v2.patch

Kicking r? to ilya 'cause I'm super-busy.

Comment 12

[Ilya Sherman]

Comment on attachment 478667 [details] [diff] [review]
set-help-menu-by-nib-tag-v2.patch

Thanks for looking at this :)

>+- (void)setUpSpotlightForHelp
>+{
>+  // Mac OS X 10.5 automatically adds the "Spotlight for Help" menu item to the
>+  // Help menu, but 10.6 will only add it if the Help menu is actually named
>+  // "Help". This breaks localized Help menus, so we must find our Help menu by
>+  // its nib tag and setHelpMenu to activate the "Spotlight for Help" menu item.
>+

nit: this blank line is unnecessary; please remove it

>+  if ([NSApp respondsToSelector:@selector(setHelpMenu:)]) {
>+    NSMenu* mainMenu = [NSApp mainMenu];
>+    NSMenuItem* helpMenuItem = [mainMenu itemWithTag:kHelpMenuItemTag];
>+    NSMenu* helpMenu = [helpMenuItem submenu];

nit: this would be tidier if the last two lines were combined:
      NSMenu* helpMenu = [[mainMenu itemWithTag:kHelpMenuItemTag] submenu];


r=ilya with those changes

Comment 13

[Chris Peterson [:cpeterson]]

Attachment: set-help-menu-by-nib-tag-v3.patch

Patch v3 incorporates Ilya's feedback.

Comment 14

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 481156 [details] [diff] [review]
set-help-menu-by-nib-tag-v3.patch

+// Help menu
+const int kHelpMenuItemTag = 7000;

why not have this be in an enum too?

sr=pink.

Comment 15

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #14)
> +// Help menu
> +const int kHelpMenuItemTag = 7000;
> 
> why not have this be in an enum too?

Because it's a single-item tag just like all of the other tags in the file, save the odd-one-out pop-up menu that is an enum? ;)

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/c80d145671c1

I meant to mention the related "setup->setUp" cleanup in the comment, too, but forgot by the time I was done twiddling bits on the nib and checking up on cranky tinderboxen :(