Closed Bug 340412 Opened 18 years ago Closed 10 years ago

Protect (warn) against (accidentally) opening too many tabs

Categories

(Camino Graveyard :: General, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: hwaara, Assigned: froodian)

References

Details

(Whiteboard: [New strings in comment 29])

Attachments

(1 file, 12 obsolete files)

When Firefox has implemented bug 333734, I think Camino should adher to that pref.

So if a user chooses "Open in tabs" in a menu (with lots and lots of items) and the number of items is more than the pref setting, we'd pop up a dialog asking if this is really the desired action.   It's like the reverse of the warning to accidently close a window with multiple tabs.
Attached patch Patch (obsolete) — Splinter Review
This does as advertised.  The text needs serious polish.  Right now I have:

"OpenManyTabsTitle" = "You are about to open %u tabs";
"OpenManyTabsText" = "You are about to open %u web pages in tabs, which could take a long time to load.  Are you sure you want to open these tabs?";

where %u is the number of items about to be opened (obviously).  

Also, another thing we should think about is if we should be changing the default behavior.  By default the pref is set to true and the number of tabs being opened needs to be 16 to trigger the warning.  They're both changeable through about:config, but does that sound good for our defaults?
Assignee: nobody → stridey
Status: NEW → ASSIGNED
Attachment #229760 - Flags: review?(hwaara)
This will also need a "do not show this warning again" checkbox in the dialog, and a corresponding option in the prefs to enable/disable it.
What Stuart said. And if you're gonna reindent the whole thing, please provide an additional -w diff too. Thanks!
Attached patch Patch (-w, do not checkin) (obsolete) — Splinter Review
This Patch:

- Adds a "Don't show this warning again" checkbox
- Exposes both the warn pref, and the number of tabs before warning (in the same line for minimal bloat)
- Gets rid of kDefaultExpireDays, since it's not used
- Gets rid of an "ensure that the prefs exist" line.  In testing this out, I found that adding that line for the warn-on-open pref also means "don't respect the sheet's checkbox if this prefpane is open", which consequently was the same for the warn-on-close pref.
- Consolidates multiple localized strings which localized to the same thing ("Don't show this warning again")
Attachment #229760 - Attachment is obsolete: true
Attachment #230076 - Flags: review?(stuart.morgan)
Attachment #229760 - Flags: review?(hwaara)
Attached image Screenshot of new Navigation.nib (obsolete) —
Attached file New Navigation.nib (obsolete) —
I also fixed bug 342951 while I was here.
Attachment #230078 - Flags: review?(alqahira)
Attached file New Navigation.nib (obsolete) —
Sigh. Already noticed a problem (vertical placement on "warn me when").  Fixes that.
Attachment #230078 - Attachment is obsolete: true
Attachment #230079 - Flags: review?(alqahira)
Attachment #230078 - Flags: review?(alqahira)
Attached image Screenshot of new Navigation.nib (obsolete) —
Attachment #230077 - Attachment is obsolete: true
Attached patch non -w Patch to match 230076 (obsolete) — Splinter Review
While I'm changing the bug (more)...

NEW STRINGS:

/* open many tabs */
"OpenManyTabsTitle" = "Are you sure you want to open %u tabs?";
"OpenManyTabsText" = "You are about to open %u web pages in tabs, which could take a long time to load.";
"OpenManyTabsButtonText" = "Open Tabs";
"DontShowWarningAgainCheckboxLabel" = "Don’t show this warning again";

and delete

"CloseWindowWithMultipleTabsCheckboxLabel" = "Don’t show this warning again";
"QuitWithMultipleTabsCheckboxLabel" = "Don’t show this warning again";

sorry for the bugspam.
Whiteboard: [New strings in comment 9]
Comment on attachment 230079 [details]
New Navigation.nib

r=me on the nib (I think)

The issue I have is that the change in number of tabs doesn't auto-apply; you have to switch panes/close the prefs first.  (Actually, I forgot to check whether the pref itself auto-applies; drat!).

Also, why are we removing strings?  Are they dupes, or unused?
Attachment #230079 - Flags: review?(alqahira) → review+
(In reply to comment #10)
> Also, why are we removing strings?

+"DontShowWarningAgainCheckboxLabel" = "Don’t show this warning again";
-"CloseWindowWithMultipleTabsCheckboxLabel" = "Don’t show this warning again";
-"QuitWithMultipleTabsCheckboxLabel" = "Don’t show this warning again";

Because it's silly to have a bunch of different names for the same string used for the same purpose.
In that case, shouldn't we be killing 
"CloseMultipleWindowsCheckboxLabel" = "Don’t show this warning again";
too?
fwiw, see bug #342930 (where we do this for firefox, and where we're about to slightly change the wording.)
(In reply to comment #12)
> In that case, shouldn't we be killing 
> "CloseMultipleWindowsCheckboxLabel" = "Don’t show this warning again";
> too?

Yes.  I just missed that - this patch fixes that.

(In reply to comment #10)
> The issue I have is that the change in number of tabs doesn't auto-apply; you
> have to switch panes/close the prefs first.  (Actually, I forgot to check
> whether the pref itself auto-applies; drat!).

The pref itself does.  The number of tabs problem is a weakness with our "days of history" textfield as well - I stole most of the conceptual code from there.  That doesn't necessarily mean it's ok to do, just that there's precedent. ;)

Since this patch is so similar to the previous, I'm just including the -w patch for now.
Attachment #230076 - Attachment is obsolete: true
Attachment #230172 - Flags: review?(stuart.morgan)
Attachment #230076 - Flags: review?(stuart.morgan)
Summary: Protect against (accidently) opening too many tabs → Protect (warn) against (accidentally) opening too many tabs
Comment on attachment 230172 [details] [diff] [review]
Removes all dupe strings (-w, do not checkin)

I think if you just change our default prefs to a value of 15, you don't have to have the hardcoded value there -- just get the pref value and you'll get the default or user set value.
I was exposing the number in the UI, since it doesn't actually take up any more room.  Is that what you meant?  I'm not sure I understand your comment...
Oh, I see, you mean for kDefaultMaxOpenBeforeWarn.  Yeah, I'll fix that in the next iteration.
Attachment #230172 - Flags: review?(nick.kreeger)
Comment on attachment 230172 [details] [diff] [review]
Removes all dupe strings (-w, do not checkin)

This needs to go in the tab pref pane.  A couple other notes/questions:

>-
>-  [self setPref:"camino.warn_when_closing"  toBoolean:([checkboxWarnWhenClosing state] == NSOnState)];

Why is this being removed (and if there's a good reason, why is the previous one not being removed)?

>+  return (nonDigitsRange.length == 0 && [textFieldMaxOpenBeforeWarn intValue] > 0);

Check for location != NSNotFound, rather than length == 0

>+  BOOL doOpenURLArray       = true;

YES

>+  int maxTabsBeforeWarn     = [[PreferenceManager sharedInstance] getIntPref:"browser.tabs.maxOpenBeforeWarn" withSuccess:NULL];
>   int numItems 		 = (int)[urlArray count];

Fix the tabs in this line as long as you are lining things up.
Attachment #230172 - Flags: review?(stuart.morgan)
Attachment #230172 - Flags: review?(nick.kreeger)
Attachment #230172 - Flags: review-
Attached image Screenshot of the pref in Tabs.nib (obsolete) —
Although I see the logic for having it in the tabs prefpane, having implemented it I think it makes the prefpane look awkward and crowded.  In the general prefpane (even with the new popup menu button) it looks like it belongs (two "warn me when" prefs side-by-side).  I'll be willing to do this in tabs if that's the group decision, but I really think it's nicer in General.

(btw, I didn't hook up the pref in this screenshot - in real life, there'd always be a number in the textfield)
Attached patch Patch in Tabs Prefpane (obsolete) — Splinter Review
Sorry about what ended up being an almost complete rewrite of Tabs.h and Tabs.mm.  I wanted to keep the code readable, and some reordering (to match the new layout) seemed the best way.  Also, there was some |toBoolean:[sender state]| and such that I touched up.  Also allowed mixed state for the "links from other applications" pref to indicate the "reuse" setting.
Attachment #230083 - Attachment is obsolete: true
Attachment #230172 - Attachment is obsolete: true
Attachment #238138 - Flags: review?(stuart.morgan)
Attached file New Tabs.nib (obsolete) —
New Tabs.nib to incorporate pref and proposed changes from bug 352265
Attachment #230079 - Attachment is obsolete: true
Attachment #238140 - Flags: review?(alqahira)
Attached image Screenshot of Tabs.nib (obsolete) —
Attachment #230081 - Attachment is obsolete: true
Attachment #237802 - Attachment is obsolete: true
Blocks: 352265
Comment on attachment 238138 [details] [diff] [review]
Patch in Tabs Prefpane

>Index: PreferencePanes/Tabs/Tabs.h
>===================================================================
>RCS file: /cvsroot/mozilla/camino/PreferencePanes/Tabs/Tabs.h,v
>retrieving revision 1.4
>diff -u -8 -r1.4 Tabs.h
>--- PreferencePanes/Tabs/Tabs.h	20 May 2006 15:46:20 -0000	1.4
>+++ PreferencePanes/Tabs/Tabs.h	13 Sep 2006 03:57:38 -0000
>@@ -16,18 +16,19 @@
>  * The Original Code is the Mozilla browser.
>  *
>  * The Initial Developer of the Original Code is
>  * Netscape Communications Corporation.
>  * Portions created by the Initial Developer are Copyright (C) 2002
>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>- *   william@dell.wisner.name (William Dell Wisner)
>- *   joshmoz@gmail.com (Josh Aas)
>+ *   <william@dell.wisner.name> (William Dell Wisner)
>+ *   <joshmoz@gmail.com> (Josh Aas)
>+ *   <stridey@gmail.com> (Ian 'froodian' Leue)

This looks weird, please make it look like elsewhere.

>@@ -38,18 +39,22 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #import <Cocoa/Cocoa.h>
> #import <PreferencePaneBase.h>
> 
> @interface OrgMozillaChimeraPreferenceTabs : PreferencePaneBase
> {
>-  IBOutlet NSMatrix* radioOpenTabsForCommand;
>-  IBOutlet NSMatrix* radioOpenForAE;
>-  IBOutlet NSButton *checkboxLoadTabsInBackground;
>-  IBOutlet NSButton* mTabBarVisiblity;
>-  IBOutlet NSButton* mSingleWindowMode;
>+  IBOutlet NSButton*    mCheckboxOpenTabsForCommand;
>+  IBOutlet NSButton*    mCheckboxOpenForAE;
>+  IBOutlet NSButton*    mSingleWindowMode;
>+
>+  IBOutlet NSButton*    mCheckboxWarnWhenOpening;
>+  IBOutlet NSTextField* textFieldMaxOpenBeforeWarn;
>+
>+  IBOutlet NSButton*    mCheckboxLoadTabsInBackground;
>+  IBOutlet NSButton*    mTabBarVisiblity;
> }

Now that you have check-in access, you can attach -w patches for review-only, if there's gonna be a lot of reindenting.

>+const int kReuseCurrentWindow = 2;

This looks very lonely. Is it really the only constant in the Tabs-prefpanel code?

>@@ -55,48 +64,91 @@
> }
> 
> - (void)mainViewDidLoad
> {
>   if (!mPrefService)
>     return;
> 
>   BOOL gotPref;
>-  
>-  [radioOpenTabsForCommand selectCellWithTag:[self getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:&gotPref]];
>-  [radioOpenForAE selectCellWithTag:[self getIntPref:"browser.reuse_window" withSuccess:&gotPref]];
>-  [checkboxLoadTabsInBackground setState:[self getBooleanPref:"browser.tabs.loadInBackground" withSuccess:&gotPref]];
>-  [mTabBarVisiblity setState:[self getBooleanPref:"camino.tab_bar_always_visible" withSuccess:&gotPref]];
>+
>+  [mCheckboxOpenTabsForCommand setState:[self getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:&gotPref] ? NSOnState : NSOffState];

What's with the getting of gotPref, if we don't check it? Please either use it for error-checking, or pass in NULL. Eventually, we should make a version of the pref-getter that doesn't include the withSuccess:, since no one uses it anyway. (Of course we should also then investigate if there's any crucial places where this checking is needed.)

Everything else looks good to me.
Maybe I'm missing something, but I can't see any discussion anywhere in this bug about exposing the number of tabs to warn about as UI. In the first patch, this wasn't the case.

I think that's the kind of thing we shouldn't let the user (have to) decide. Let's just set it to some good-enough value, and let the user decide whether to warn or not to warn. If someone for some reason wants to tweak the value, that's what we have about:config for.
In other words, the wording could be something like: "Warn me when opening many tabs at once"
Comment on attachment 238140 [details]
New Tabs.nib

FWIW, I think people have very different ideas of the threshold for "many tabs", and exposing the pref is simple and easy and stops a lot of support queries before they happen.

I still think that change needs to auto-apply, though.  I'm fine with filing a separate bug on that (and history) as long as it gets fixed before 1.1....
Attachment #238140 - Flags: review?(alqahira) → review+
Comment on attachment 238138 [details] [diff] [review]
Patch in Tabs Prefpane

>+  IBOutlet NSButton*    mCheckboxOpenForAE;

As long as you are renaming this, go ahead and get rid of the legibility-reducing abbreviation.

>+  IBOutlet NSButton*    mSingleWindowMode;
>+
>+  IBOutlet NSButton*    mCheckboxWarnWhenOpening;
>+  IBOutlet NSTextField* textFieldMaxOpenBeforeWarn;
>+
>+  IBOutlet NSButton*    mCheckboxLoadTabsInBackground;
>+  IBOutlet NSButton*    mTabBarVisiblity;

Why are you separating out just that one pair of prefs with spaces?

>+    [mCheckboxOpenForAE setState:externalLinksPref]; // 0 == NSOffState, 1 == NSOnState

This is really ugly; the point of enums is that you aren't supposed to care or rely on what their values are.  This also assumes that the reader knows what 0 and 1 mean as external link prefs; those should be declared as constants.  So this should look more like

[mCheckboxOpenForAE setState:((externalLinksPref == kOpenInTab) ? NSOnState : NSOffState)]

>+  [mCheckboxWarnWhenOpening setState:(warnBeforeOpen ? NSOnState : NSOffState)];
...
>+  [mCheckboxLoadTabsInBackground setState:[self getBooleanPref:"browser.tabs.loadInBackground" withSuccess:&gotPref]];
>+  [mTabBarVisiblity setState:[self getBooleanPref:"camino.tab_bar_always_visible" withSuccess:&gotPref]];

I'm not really wild about passing BOOLs directly into setState: (and this code isn't consistent about which it does).

>+  // make sure the maxOpenBeforeWarn value is numbers only (use a formatter for this once we're on Xcode 2.x)

Why is the Xcode version relevant?  That doesn't sound right.

>+    [self setPref:"browser.reuse_window" toInt:[sender state]]; // NSOffState == 0, NSOnState == 1

Again, this is ugly. Use a ternary and constants.

>+  BOOL doOpenURLArray       = YES;

This needs a better name (allowOpen, maybe?)

>+  int maxTabsBeforeWarn     = [[PreferenceManager sharedInstance] getIntPref:"browser.tabs.maxOpenBeforeWarn" withSuccess:NULL];

Don't do this lookup if the warning is disabled.

>+  int numItems              = (int)[urlArray count];

Call this numTabs, rather than leaving it vague (and is there a reason it needs to be signed?)

>+    [NSApp activateIgnoringOtherApps:YES];

Why would this warning need to make Camino come to the front regardless of what the user is doing?

>+    int numItems   = (int)[urlArray count];

No need to get this again
Attachment #238138 - Flags: review?(stuart.morgan) → review-
Per a status meeting, we won't expose any of this pref.  This patch is -w, so you'll have to trust my indentation.

Also, in defense of my code-writing,

int numItems = (int)[urlArray count];

wasn't mine. :)  But fixed in this patch, 'cause there's no need to have it be signed.
Attachment #238138 - Attachment is obsolete: true
Attachment #238140 - Attachment is obsolete: true
Attachment #238141 - Attachment is obsolete: true
Attachment #239077 - Flags: review?(stuart.morgan)
Mostly for my own reference: String changes are

/* open many tabs */
"OpenManyTabsTitle" = "Are you sure you want to open %u tabs?";
"OpenManyTabsText" = "You are about to open %u web pages in tabs, which could
take a long time to load.";
"OpenManyTabsButtonText" = "Open Tabs";

the string consolidation that we lost are covered in bug 343941 now.  Also, bug 352265 will become a real bug again for the prefpane changes.  Also, smorgan's question in comment 18 is now explained fully in bug 353221.
Whiteboard: [New strings in comment 9] → [New strings in comment 29]
No longer blocks: 352265
Comment on attachment 239077 [details] [diff] [review]
Minimal Patch (-w)

>+  BOOL myriadTabWarningPref = [[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.warnOnOpen" withSuccess:NULL];
>+  unsigned int numTabs      = [urlArray count];
>+
>+  if (myriadTabWarningPref && 
>+      (numTabs > [[PreferenceManager sharedInstance] getIntPref:"browser.tabs.maxOpenBeforeWarn" withSuccess:NULL]))

Getting one pref in advance and the other in the if clause reads oddly; more importantly though this seems like a case where it's worth using withSuccess because the 0 value it'll get if there were failure would have undesirable behavior (alternately you could just require that you get a >0 value back to consider it valid, since 0 isn't a realistic value to actually set the pref to). You can still short-circuit the lookup by setting it to something like |myriadTabWarning ? <lookup> : 0|

>+    NS_DURING
>+      allowArrayToOpen = [controller confirmEx:[self window]
>+                                         title:[NSString stringWithFormat:NSLocalizedString(@"OpenManyTabsTitle", nil), numTabs]
>+                                          text:[NSString stringWithFormat:NSLocalizedString(@"OpenManyTabsText", nil), numTabs]
>+                                       button1:NSLocalizedString(@"OpenManyTabsButtonText", @"")
>+                                       button2:NSLocalizedString(@"CancelButtonText", @"")
>+                                       button3:nil];
>+    NS_HANDLER
>+    NS_ENDHANDLER

The app-modal sheets are nasty hacks that we use because there are a bunch of places gecko wants a synchronous answer and we need user input.  This isn't one of those cases, so I don't think there's any reason this couldn't be done the way sheets are supposed to be used (with a callback function).

(There actually was a reason for the int cast as it turns out; tabViewItemAtIndex: takes a signed int, so you either have to put the cast back or change the loop index to unsigned and cast i to an int in the tabViewItemAtIndex: call, or there are warnings.  Sorry about that.)
Attachment #239077 - Flags: review?(stuart.morgan) → review-
This bug has been buried in the graveyard and has not been updated in over 5 years. It is probably safe to assume that it will never be fixed, so resolving as WONTFIX.

[Mass-change filter: graveyard-wontfix-2014-09-24]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: