181978 - Change appropriate menu items to "All" when Option key held

Status: VERIFIED FIXED

(Camino Graveyard :: Toolbars & Menus, enhancement)

Description

[Stephane Moureau]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.1) Gecko/20021125 Chimera/0.6+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.1) Gecko/20021125 Chimera/0.6+

In several OS X programs, holding down the Option key change the following items:
Close, Minimize, Zoom, Print, Save (As...)
to
Close All, Minimize All, Print All, Save All.

Close,Minimize and Zoom are from the menus or from the red/orange/green icons of
a window.

Chimera seems to react like that for some of the commands (Not Save nor Print)
but the menu items are not modified to reflect the "All".

Reproducible: Always

Steps to Reproduce:
1. Open 2 windows
2. Hold down Option key
3. Try the listed possibilities


Expected Results:  
Update the menus items to reflect the "All".

Comment 1

[Greg K.]

Stephane, please be more specific about what OS X applications behave in this way.

Comment 2

[Stephane Moureau]

http://developer.apple.com/techpubs/macosx/Essentials/AquaHIGuidelines/AHIGMenus/The_File_Menu.html
and similar pages for the "ALL" items.
BBEdit, Finder (partly - Window menu), etc.

Comment 3

[Greg K.]

Sounds reasonable. Stephane, would you also file one for Mozilla if there isn't
one already? Thanks.

Comment 4

[Simon Fraser [no longer active]]

Cocoa doesn't have support for dynamically-updatable menu items at present.

Comment 5

[Simon Fraser [no longer active]]

*** Bug 182936 has been marked as a duplicate of this bug. ***

Comment 6

[Wevah (Nate Weaver)]

This is only really doable under OS X 10.3, and we still need to support 10.2.

Comment 7

[Samuel Sidler (old account; do not CC)]

Since this is a valid request, though it isn't possible with 10.2, I'm targeting
for Future, when we won't be supporting 10.2.

Comment 8

[David Munch]

Alt + Zoom button in Safari gives fullscreen, should Camino mimick that behavior?

Comment 9

[Håkan Waara]

Anyone who would like to do this now can look at how to use alternate menuitems in the first patch of bug 317214.

Comment 10

[froodian (Ian Leue)]

Most of these we get for free.

We should discuss whether we want Print all and Save all, since it's something we'll actually have to implement (and not something that really seems that useful to me).

Comment 11

[froodian (Ian Leue)]

Attachment: Patch

Now that I think about it, this patch is entirely for bug 312904, but I made all the nib changes together, and I'd rather keep all attachments confined to one bug.

The removed code in BrowserWindow.mm is because we're providing menu items now, so we can (have to, actually, for force reload all tabs to work) remove the hardwired shortcuts.

Comment 12

[froodian (Ian Leue)]

Attachment: New MainMenu.nib

-Changes "Close Window" to "Close All Windows" when option is held
-Changes "Reload Page" to "Force Reload Page" when shift is held
-Adds "Reload All Tabs" (with a shortcut of Cmd-option-R), and changes it to "Force Reload All Tabs" when shift is held
-Changes "Minimize" to "Minimize All" when option is held
-Changes "Zoom" to "Zoom All" when option is held

Comment 13

[froodian (Ian Leue)]

Attachment: New BrowserWindow.nib

Removes the "Reload All Tabs" option from the context menu, and re-implements it as an alternate menu item (when option is held)

Comment 14

[Håkan Waara]

Wasn't the concensus on bug 312904 to add "Reload All Tabs" as an alternate menuitem when option was held down?  Looks like the nib adds it permanently as soon as there are > 1 tab, unless I'm missing something?

Comment 15

[froodian (Ian Leue)]

Attachment: New MainMenu.nib

(In reply to comment #14)
> Wasn't the concensus on bug 312904 to add "Reload All Tabs" as an alternate
> menuitem when option was held down?  Looks like the nib adds it permanently as
> soon as there are > 1 tab, unless I'm missing something?

No, you're not missing anything, I was.  This does just that.

Comment 16

[Håkan Waara]

Comment on attachment 226192 [details] [diff] [review]
Patch

> 
>   // only activate if we've got multiple tabs open.
>-  if ((action == @selector(nextTab:) ||
>-       action == @selector(previousTab:)))
>+  if (action == @selector(nextTab:) ||
>+      action == @selector(previousTab:) ||
>+      action == @selector(doReloadAllTabs:))
>   {
>     return (browserController && [[browserController getTabBrowser] numberOfTabViewItems] > 1);
>   }
>   

Can't really see the context of this, but is it still needed after your last nib change? Also, are all new connections hooked up to the actions?

Comment 17

[froodian (Ian Leue)]

(In reply to comment #16)
> (From update of attachment 226192 [details] [diff] [review] [edit])
> > 
> >   // only activate if we've got multiple tabs open.
> >-  if ((action == @selector(nextTab:) ||
> >-       action == @selector(previousTab:)))
> >+  if (action == @selector(nextTab:) ||
> >+      action == @selector(previousTab:) ||
> >+      action == @selector(doReloadAllTabs:))
> >   {
> >     return (browserController && [[browserController getTabBrowser] numberOfTabViewItems] > 1);
> >   }
> >   
> 
> Can't really see the context of this, but is it still needed after your last
> nib change? Also, are all new connections hooked up to the actions?

It's in validateMenuItem.  It's still needed, because even though they're (exposed as) a single menu item now, we still want the "all tabs" cases to be greyed out when there's only one tab.

Yeah, I tested out all the connections and they all work as advertised.  Someone else probably ought to too, since I'm only human, but I'm pretty sure everything is hooked up and working.

Comment 18

[Håkan Waara]

Comment on attachment 226192 [details] [diff] [review]
Patch

Nice work!

Comment 19

[Chris Lawson (gone)]

Comment on attachment 226192 [details] [diff] [review]
Patch

>Index: application/MainController.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/application/MainController.mm,v
>retrieving revision 1.185
>diff -u -8 -p -r1.185 MainController.mm
>--- application/MainController.mm	18 Jun 2006 05:01:14 -0000	1.185
>+++ application/MainController.mm	19 Jun 2006 19:28:28 -0000
>@@ -834,16 +834,24 @@ Otherwise, we return the URL we original
> -(IBAction) doReload:(id)aSender
> {
>   BrowserWindowController* browserController = [self getMainWindowBrowserController];
>   if (browserController)
>     [browserController reload: aSender]; 
> }
> 
> // XXX move to BWC
>+-(IBAction) doReloadAllTabs:(id)aSender
>+{
>+  BrowserWindowController* browserController = [self getMainWindowBrowserController];
>+  if (browserController)
>+    [browserController reloadAllTabs: aSender];

No space after the colon.

Otherwise, r=me. The checker-inner can take care of that if he wants.

Comment 20

[Mike Pinkerton (not reading bugmail)]

this removes cmd-shift-r as force-reload of a page. Why?

Comment 21

[froodian (Ian Leue)]

(In reply to comment #20)
> this removes cmd-shift-r as force-reload of a page. Why?

Because it's a menu item now, so we don't need to put it in the code (and in fact, it breaks Cmd-option-shift-R, force reload all if we leave it in).

Comment 22

[Mike Pinkerton (not reading bugmail)]

force reload is a menu item now? really? is that something we want?

Comment 23

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 226192 [details] [diff] [review]
Patch

sorry, i thought force-reload was a separate item. it has been explained to me that this isn't the case.

sr=pink

Comment 24

[froodian (Ian Leue)]

Attachment: Patch

Now with shiny new unbitrottedness!

Comment 25

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

Several problems with these nibs (or with the patch+nibs combo), plus a couple questions:

-Zoom All only zooms the front/active window *unless* opt is held down *before* opening the menu

- Minimize All, when using the menu item, only closes the frontmost/active window *unless* opt is held down *before* opening the menu

- Close All Windows, when using the menu item, only closes the frontmost/active window *unless* opt is held down *before* opening the menu

(The various Reload commands, or at least the easily-verifiable "Reload All Tabs" version, *does* work if you press opt after opening the menus, which rules out bug 333765, I think.)

- Shouldn't the page and tab bar context menu items "Reload" and "Reload Tab" switch to "Force Reload Tab" when shift is held down (and the latter to "Force Reload All Tabs" when both shift and opt are held down)?

- Do any of these need Localizable.strings changes, or are all of the alternate command names present in the nibs (and if the latter, will l10n find them or will we need to notify them)?

- Zoom All/Close All Windows both only work on the subset of all windows that are not minimized; is that expected?

Comment 26

[froodian (Ian Leue)]

> - Shouldn't the page and tab bar context menu items "Reload" and "Reload Tab"
> switch to "Force Reload Tab" when shift is held down (and the latter to "Force
> Reload All Tabs" when both shift and opt are held down)?

We can't do this (have context menu items recognize the shift modifier) unless we're willing to give them key equivalents (which will show up like regular menu keyboard shortcuts).  The HIG says "If a command has a keyboard shortcut, don't display the shortcut in the contextual menu (you should display the shortcut in the menu bar menu, as described in “The Menu Bar and Its Menus”)."  Plus, if we give the reload item key equivalents, it will look weird if we don't give *all* the context menu items key equivalents.  So personally, I'm against it.

Comment 27

[froodian (Ian Leue)]

Attachment: Patch

(In reply to comment #25)
> -Zoom All only zooms the front/active window *unless* opt is held down       > *before* opening the menu
Fixed
> - Minimize All, when using the menu item, only closes the frontmost/active
> window *unless* opt is held down *before* opening the menu
Fixed
> - Close All Windows, when using the menu item, only closes the               > frontmost/active window *unless* opt is held down *before* opening the menu
Fixed

- All new strings appear as individual menu items in the nib (ie l10n will find them easily)

> - Zoom All/Close All Windows both only work on the subset of all windows that
> are not minimized; is that expected?

I think the first is expected and the second is not.  In my experience, minimized windows are impervious to all action except unminimizing them or closing them.  In this patch, Close All Windows affect minimized windows as well.

-I didn't enable force reloading in context menus re: my previous comment.

-Anybody looking at this patch, note that I moved around a couple items in MainController.h to keep them under the right menu

Comment 28

[froodian (Ian Leue)]

Attachment: New MainMenu.nib

Comment 29

[froodian (Ian Leue)]

Attachment: New BrowserWindow.nib

Comment 30

[Bruce Davidson]

Comment on attachment 226985 [details]
New BrowserWindow.nib

The NIBs are now good to go. The faults that Smokey found have been fixed.

One quibble on the bug is that "Close All Windows" appears to bypass the warning about having multiple tabs in a window open. That seems unsafe.

Comment 31

[froodian (Ian Leue)]

Attachment: Patch

> One quibble on the bug is that "Close All Windows" appears to bypass the
> warning about having multiple tabs in a window open. That seems unsafe.

Fixed.

Comment 32

[froodian (Ian Leue)]

Forgot to mention when I uploaded the patch...

NEW STRINGS:

"CloseMultipleWindowsMsg" = "Are you sure you want to close all windows?";
"CloseMultipleWindowsExpl" = "You have %u windows open.  If you close these windows, you will lose any information that you entered in them.";
"CloseMultipleWindowsCheckboxLabel" = "Don’t show this warning again";

Comment 33

[froodian (Ian Leue)]

Specifically, in the 6/25 patches I've:

-Explicitly hooked minimize all up to NSApplication's -miniaturizeAll
-Made a method -zoomAll which does just that (only works on unminimized windows by design)
-Made a method -closeAllWindows which makes the "do you really want to close multiple tabs/windows" warning if necessary, then closes all windows (including minimized windows).
-Shuffled around a couple method declarations in MainController.h to actually be under the right comment heading.

The reason I can use basically the same code for zoomAll and closeAllWindows (the enumerator) and have it have different behavior is that a zoom message sent to a minimized window doesn't do anything.

Comment 34

[Håkan Waara]

Comment on attachment 227023 [details] [diff] [review]
Patch

>+-(IBAction)closeAllWindows:(id)aSender
>+{
>+  BOOL doCloseWindows = YES;
>+  PreferenceManager* prefManager = [PreferenceManager sharedInstanceDontCreate];

Normally, obj-c singletons are lazily created in an class-method |sharedInstance|. That is,
if the object doesn't exist (already), create it, if it does, just return the same instance
to everyone.

+ (id)sharedInstance {
  static MyClass *mySingleton = nil;
  if (!mySingleton) {
    // will be created only if it hasn't ever been before.
    mySingleton = [MyClass new];
  }

  // otherwise, just return a ref to the existing instance.
  return mySingleton;
}

That's why the |sharedInstanceDontCreate| method we apparently had is useless... (and potentially dangerous)
If we don't have too many callers, please rip it out.

The second problem is that you could (theoretically, though not realistically) get a null
pointer from that method, so in your case, since you want the prefManager instance no matter
what, you should use the "safe" version. 

>+
>+  if ([prefManager getBooleanPref:"camino.warn_when_closing" withSuccess:NULL]) {
>+    NSString* closeAlertMsg = nil;
>+    NSString* closeAlertExpl = nil;
>+
>+    NSArray* openBrowserWins = [self browserWindows];
>+    if ([openBrowserWins count] == 1)
>+    {

Please use only one bracing style ;)

Personally, I'd prefer the code adher to moz code guidelines. That'd be if (foo) {

>+      BrowserWindowController* bwc = [[openBrowserWins firstObject] windowController];

Since you're the MainController, I think you already have an easier way to get the BWC.
I think there's a (hopelessly-named) |getMainWindowBrowserController| method?

>+      unsigned int numTabs = [[bwc getTabBrowser] numberOfTabViewItems];
>+      if (numTabs > 1)
>+      {
>+        closeAlertMsg = NSLocalizedString(@"CloseWindowWithMultipleTabsMsg", @"");
>+        closeAlertExpl = [NSString stringWithFormat:NSLocalizedString(@"CloseWindowWithMultipleTabsExplFormat", @""), numTabs];
>+      }

Obj-c lines tend to become very long. :)  Please try wrap it at least a bit, for example:

closeAlertExpl = 
   [NSString stringWithFormat:NSLocalizedString(@"CloseWindowWithMultipleTabsExplFormat", @""), numTabs];

(There are also more "aggressive" wrapping behaviors, if one is in that mood.  This is an art in itself.)

>+    }
>+    else if ([openBrowserWins count] > 1)
>+    {

Check bracing here too. Probably "} else if (foo) {"

>+      closeAlertMsg = NSLocalizedString(@"CloseMultipleWindowsMsg", @"");
>+      closeAlertExpl = [NSString stringWithFormat:NSLocalizedString(@"CloseMultipleWindowsExpl", @""), [openBrowserWins count]];
>+    }
>+
>+    if (closeAlertMsg)
>+    {
>+      [NSApp activateIgnoringOtherApps:YES];
>+      nsAlertController* controller = CHBrowserService::GetAlertController();
>+      BOOL dontShowAgain = NO;
>+      BOOL confirmed = NO;
>+
>+      NS_DURING
>+        confirmed = [controller confirmCheckEx:nil
>+                                         title:closeAlertMsg
>+                                          text:closeAlertExpl
>+                                       button1:NSLocalizedString(@"OKButtonText", @"")
>+                                       button2:NSLocalizedString(@"CancelButtonText", @"")
>+                                       button3:nil
>+                                      checkMsg:NSLocalizedString(@"CloseMultipleWindowsCheckboxLabel", @"")
>+                                    checkValue:&dontShowAgain];
>+      NS_HANDLER
>+      NS_ENDHANDLER
>+
>+      if (dontShowAgain)
>+        [prefManager setPref:"camino.warn_when_closing" toBoolean:NO];
>+
>+      doCloseWindows = (confirmed) ? YES : NO;

No need for the parens, and you could as well skip the confirmed BOOL and just assign to the doCloseWindows
boolean directly, I suppose?

> // XXX move to BWC
>+-(IBAction) doReloadAllTabs:(id)aSender
>+{
>+  BrowserWindowController* browserController = [self getMainWindowBrowserController];
>+  if (browserController)
>+    [browserController reloadAllTabs: aSender];
>+}

Please remove the space before the arg.

What's with the doFoo naming? I realize you're just doing the same as doReload:, but isn't that named like that
because of some name-conflict? (Perhaps there's already a reload:) 

A final request, please sprinkle some comments in the new code you're writing, since the closeAllWindows: method
is quite long.

Everything else looks good.

Comment 35

[Håkan Waara]

(Ouch, bugzilla destroyed the wrapping in my comment.)

Comment 36

[froodian (Ian Leue)]

Attachment: Ubitrots BrowserWindow.nib

This is the same as the r+ed nib, just with pink's spell-checking changes.

Comment 37

[froodian (Ian Leue)]

Attachment: Patch

This patch:
- Shuffles some methods in MainController.h to be under the right menu comment
- Implements closeAllWindows, which respects the "warn before closing multiple tabs" pref
- Implements doReloadAllTabs, which just calls reloadsAllTabs in bwc
- Implements zoomAll, which zooms all visible windows (since a zoom: call to a non-visible window does nothing)
- Gets rid of the hard-wiring of Cmd-Shift-R, since it's a (alternate) menu item now

Things this patch does that the last one didn't:
- Addresses hwaara's stylistic comments
- Gets rid of all the // XXX move to bwc comments.  The reason those were in there in the first place (we presume) was because they're methods that are basically just calls to the bwc implementation.  Per IRC, we should probably leave these methods in Maincontroller, since it's the app's delegate, and all menu items should appear there.  It's fine to just do a check, then pass to bwc if appropriate.
- Doesn't use sharedInstanceDontCreate, but leaves it elsewhere, since in all other places it's used to see if there is a preference manager.

This leaves the doFoo naming, since it might do weird things to the "action-automatically-finding-its-target-thingy" to change it.

Comment 38

[Håkan Waara]

Comment on attachment 228056 [details] [diff] [review]
Patch

Looks good to me.

Comment 39

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 228056 [details] [diff] [review]
Patch

sr=pink

+-(IBAction) zoomAll:(id)aSender

should this zoom *all* windows, or just browser windows? do we want to zoom the download manager?

Comment 40

[froodian (Ian Leue)]

When we check this in, we should grab bug 342829 too (just string changes), and the go menu string change it references (bug 335983).

Comment 41

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

hwaara landed this on trunk and branch (the latter with some bumps because branch nibs had gotten out-of-sync for some evil yet unknown reason); thanks!

Comment 42

[Håkan Waara]

Smokey saw a menu crash, and BWC gives some serious compiler warnings that this patch could've caused.

See for example the warnings (barring the error) at http://tinderbox.mozilla.org/showlog.cgi?log=Camino/1152182520.18157.gz 

Comment 43

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

As I somehow managed not to fall asleep before my trunk rebuild completed, I can now say that both the menu crash and the unblock crash are gone; there must have been something stale in that Camino's Gecko causing a bad interaction.  

So froodian and hwaara can now rest easy knowing there are no more problems associated with today's two landings ;)

Comment 44

[Chris Lawson (gone)]

Verified with trunk nightly from 06 July.