Closed Bug 531903 Opened 14 years ago Closed 13 years ago

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


(Camino Graveyard :: Toolbars & Menus, defect)

Not set


(Not tracked)



(Reporter: alqahira, Assigned: cpeterson)



(2 files, 2 obsolete files)

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.
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.
Chris, any chance this bug is something you can tackle?
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.
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).
Assignee: nobody →
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?
Attachment #478509 - Flags: review?(alqahira)
Attached file
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).
Attachment #478511 - Attachment description: MainMenu.nib →
Comment on attachment 478509 [details] [diff] [review]

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.
Attachment #478509 - Flags: review?(alqahira) → review?(cl-bugs-new2)
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.
Incorporate Smokey's feedback: fixed 10.4 comments and renamed "setup" to "setUp" (including some other "setup" functions in the same .mm file)
Attachment #478509 - Attachment is obsolete: true
Attachment #478667 - Flags: review?(cl-bugs-new2)
Attachment #478509 - Flags: review?(cl-bugs-new2)
Comment on attachment 478667 [details] [diff] [review]

Kicking r? to ilya 'cause I'm super-busy.
Attachment #478667 - Flags: review?(cl-bugs-new2) → review?(ishermandom+bugs)
Comment on attachment 478667 [details] [diff] [review]

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
Attachment #478667 - Flags: superreview?(mikepinkerton)
Attachment #478667 - Flags: review?(ishermandom+bugs)
Attachment #478667 - Flags: review+
Patch v3 incorporates Ilya's feedback.
Attachment #478667 - Attachment is obsolete: true
Attachment #481156 - Flags: superreview?(mikepinkerton)
Attachment #478667 - Flags: superreview?(mikepinkerton)
Comment on attachment 481156 [details] [diff] [review]

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

why not have this be in an enum too?

Attachment #481156 - Flags: superreview?(mikepinkerton) → superreview+
(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? ;)

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 :(
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.