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

VERIFIED FIXED in Camino1.5

Status

Camino Graveyard
Toolbars & Menus
--
enhancement
VERIFIED FIXED
15 years ago
11 years ago

People

(Reporter: Stephane Moureau, Assigned: froodian (Ian Leue))

Tracking

({fixed1.8.1})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1
Dependency tree / graph

Details

(Whiteboard: [New strings in comment 32])

Attachments

(3 attachments, 8 obsolete attachments)

24.13 KB, application/zip
Bruce Davidson
: review+
Details
24.94 KB, application/zip
Details
12.15 KB, patch
Håkan Waara
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
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

15 years ago
Stephane, please be more specific about what OS X applications behave in this way.
(Reporter)

Comment 2

15 years ago
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

15 years ago
Sounds reasonable. Stephane, would you also file one for Mozilla if there isn't
one already? Thanks.
Assignee: saari → brade
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Component: General → Toolbars & Menus
Ever confirmed: true
QA Contact: winnie → sairuh
Summary: Option-Close/Minimize/Zoom/Save/Print All → Change appropriate menu items to "All" when Option key held

Updated

15 years ago
Depends on: 182936

Comment 4

15 years ago
Cocoa doesn't have support for dynamically-updatable menu items at present.
Assignee: brade → sfraser

Comment 5

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

Comment 6

12 years ago
This is only really doable under OS X 10.3, and we still need to support 10.2.
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.
Target Milestone: --- → Future

Comment 8

12 years ago
Alt + Zoom button in Safari gives fullscreen, should Camino mimick that behavior?
Blocks: 311840
Blocks: 315986
Blocks: 328173

Comment 9

11 years ago
Anyone who would like to do this now can look at how to use alternate menuitems in the first patch of bug 317214.
Target Milestone: Future → Camino1.1
(Assignee)

Updated

11 years ago
Blocks: 312904
(Assignee)

Comment 10

11 years ago
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).
Assignee: sfraser_bugs → stridey
QA Contact: bugzilla → toolbars
(Assignee)

Comment 11

11 years ago
Created attachment 226192 [details] [diff] [review]
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.
Attachment #226192 - Flags: review?(hwaara)
(Assignee)

Comment 12

11 years ago
Created attachment 226194 [details]
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
Attachment #226194 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #226194 - Flags: review? → review?(bugzilla)
(Assignee)

Comment 13

11 years ago
Created attachment 226195 [details]
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)
Attachment #226195 - Flags: review?(bugzilla)

Comment 14

11 years ago
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?
(Assignee)

Comment 15

11 years ago
Created attachment 226214 [details]
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.
Attachment #226194 - Attachment is obsolete: true
Attachment #226214 - Flags: review?(alqahira)
Attachment #226194 - Flags: review?(bugzilla)
(Assignee)

Updated

11 years ago
Attachment #226195 - Flags: review?(bugzilla) → review?(alqahira)

Comment 16

11 years ago
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?
(Assignee)

Comment 17

11 years ago
(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

11 years ago
Comment on attachment 226192 [details] [diff] [review]
Patch

Nice work!
Attachment #226192 - Flags: review?(hwaara) → review+
(Assignee)

Updated

11 years ago
Attachment #226192 - Flags: review?(bugzilla)

Comment 19

11 years ago
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.
Attachment #226192 - Flags: review?(bugzilla) → review+
(Assignee)

Updated

11 years ago
Attachment #226192 - Flags: superreview?(mikepinkerton)
this removes cmd-shift-r as force-reload of a page. Why?
(Assignee)

Comment 21

11 years ago
(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).
force reload is a menu item now? really? is that something we want?
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
Attachment #226192 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Updated

11 years ago
Attachment #226195 - Flags: review?(alqahira) → review?(bugzilla)
(Assignee)

Updated

11 years ago
Attachment #226214 - Flags: review?(alqahira) → review?(bugzilla)
(Assignee)

Updated

11 years ago
Attachment #226195 - Flags: review?(bugzilla) → review?(mozilla)
(Assignee)

Updated

11 years ago
Attachment #226214 - Flags: review?(bugzilla) → review?(mozilla)
(Assignee)

Comment 24

11 years ago
Created attachment 226957 [details] [diff] [review]
Patch

Now with shiny new unbitrottedness!
Attachment #226192 - Attachment is obsolete: true
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?
(Assignee)

Comment 26

11 years ago
> - 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.
Status: NEW → ASSIGNED
(Assignee)

Comment 27

11 years ago
Created attachment 226983 [details] [diff] [review]
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
Attachment #226957 - Attachment is obsolete: true
Attachment #226983 - Flags: review?(bugzilla)
(Assignee)

Comment 28

11 years ago
Created attachment 226984 [details]
New MainMenu.nib
Attachment #226214 - Attachment is obsolete: true
Attachment #226214 - Flags: review?(mozilla)
(Assignee)

Comment 29

11 years ago
Created attachment 226985 [details]
New BrowserWindow.nib
Attachment #226195 - Attachment is obsolete: true
Attachment #226985 - Flags: review?(mozilla)
Attachment #226195 - Flags: review?(mozilla)
(Assignee)

Updated

11 years ago
Attachment #226984 - Flags: review?(mozilla)

Updated

11 years ago
Attachment #226984 - Flags: review?(mozilla) → review+

Comment 30

11 years ago
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.
Attachment #226985 - Flags: review?(mozilla) → review+
(Assignee)

Comment 31

11 years ago
Created attachment 227023 [details] [diff] [review]
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.
Attachment #226983 - Attachment is obsolete: true
Attachment #227023 - Flags: review?(stuart.morgan)
Attachment #226983 - Flags: review?(bugzilla)
(Assignee)

Comment 32

11 years ago
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";
(Assignee)

Updated

11 years ago
Blocks: 342780
Whiteboard: [New strings in comment 32]
(Assignee)

Updated

11 years ago
Attachment #227023 - Flags: review?(stuart.morgan) → review?(hwaara)
(Assignee)

Comment 33

11 years ago
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

11 years ago
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.
Attachment #227023 - Flags: review?(hwaara) → review-

Comment 35

11 years ago
(Ouch, bugzilla destroyed the wrapping in my comment.)
(Assignee)

Comment 36

11 years ago
Created attachment 228053 [details]
Ubitrots BrowserWindow.nib

This is the same as the r+ed nib, just with pink's spell-checking changes.
Attachment #226985 - Attachment is obsolete: true
(Assignee)

Comment 37

11 years ago
Created attachment 228056 [details] [diff] [review]
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.
Attachment #227023 - Attachment is obsolete: true
Attachment #228056 - Flags: review?(hwaara)

Comment 38

11 years ago
Comment on attachment 228056 [details] [diff] [review]
Patch

Looks good to me.
Attachment #228056 - Flags: review?(hwaara) → review+
(Assignee)

Updated

11 years ago
Attachment #228056 - Flags: superreview?(mikepinkerton)
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?
Attachment #228056 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Updated

11 years ago
Whiteboard: [New strings in comment 32] → [New strings in comment 32] [needs checkin]
(Assignee)

Comment 40

11 years ago
When we check this in, we should grab bug 342829 too (just string changes), and the go menu string change it references (bug 335983).
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!
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [New strings in comment 32] [needs checkin] → [New strings in comment 32]

Comment 42

11 years ago
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 
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 ;)
(Assignee)

Updated

11 years ago
Depends on: 343762

Comment 44

11 years ago
Verified with trunk nightly from 06 July.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.