528773 - Closing multiple windows warning should count tabs, too

Status: RESOLVED FIXED

(Camino Graveyard :: Tabbed Browsing, enhancement)

Description

[Chris Lawson (gone)]

See Safari for an example, or here:

http://twitpic.com/pkyql

We currently only count the number of windows, ignoring the number of tabs in each window. It'd be nice if we did what Safari does.

Comment 1

[Chris Lawson (gone)]

Smokey and hendy both concurred with this on IRC; I'll take a look at it soon-ish.

Comment 2

[Chris Lawson (gone)]

Attachment: fix v1.0

hendy, can you take a look at this?

I'm open to suggestions on the strings changes, too, if someone has a better idea. Our current text is:

"You have %u windows open. If you {quit now|close these windows}, you will lose any information that you entered in {those windows|them}."

This patch changes it to read:

"You have %u windows open, containing %u total tabs. If you {quit now|close these windows}, you will lose any information that you entered in {those windows|them}."

cl

Comment 3

[Christopher Henderson]

Comment on attachment 417593 [details] [diff] [review]
fix v1.0

Code looks fine, but I'm wondering if we want to have behaviour even closer to Safari's. With multiple 1-tab windows open, the alert dialog only mentions windows. Eg: "2 windows are open in Safari. Do you want to quit anyway?" Maybe we want to keep the original warning strings around, and use them if numTabs = numWindows.

Also, I don't think we need the "total" in "total tabs". I think the context makes it clear what that tab number refers to.

Comment 4

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

(In reply to comment #3)
> we want to keep the original warning strings around, and use them if numTabs =
> numWindows.

I think we do.

[01:45am] hendy: we could also check for lots of windows with 1 tab, and pop up a dialog saying "Do you know about tabs???"

Comment 5

[Chris Lawson (gone)]

Attachment: fix v1.1

(In reply to comment #4)

> [01:45am] hendy: we could also check for lots of windows with 1 tab, and pop up
> a dialog saying "Do you know about tabs???"

Wanna file that as a separate bug, so we can discuss what the threshold for notification would be and what exactly we want to say? I dunno if hendy was halfway joking or not, but that actually does seem mildly useful.

I've updated the code to preserve the original strings in the windows = tabs case (comment 3/comment 4) and cleaned things up just a tad. This patch also reflects the strings change that Smokey and I discussed on IRC just after I uploaded the original, which IMO addresses the "total tabs" issue better than just dropping the word "total".

We need smorgan/pink eyes on the strings changes in particular to determine the exact text we want to use.

Comment 6

[Christopher Henderson]

Comment on attachment 419799 [details] [diff] [review]
fix v1.1

Looks good.

Comment 7

[Chris Lawson (gone)]

Comment on attachment 419799 [details] [diff] [review]
fix v1.1

Sticking pink on here for additional review to get eyes on these strings.

Comment 8

[Stuart Morgan]

Comment on attachment 419799 [details] [diff] [review]
fix v1.1

>+      if (totalTabs > 1) { // only show the warning if there are multiple tabs

Two spaces between code and //, not just one.

>+"CloseMultipleWindowsAndTabsExpl" = "You have %u windows open, containing a total of %u tabs. If you close these windows, you will lose any information that you entered in them.";

This needs to be:
"You have %1$u windows open, containing a total of %2$u tabs. If you close these windows, you will lose any information that you entered in them."
so that localization works. Similarly with the other new string that has two placeholders.


sr=smorgan with those changes (the language seems fine to me).
However, it would be nice given the duplication of logic for you to add a new method that figures out the tab/window count relationship and returns a value from a new enum (where the values would basically be: multiple tabs and windows, multiple windows no tabs, one window multiple tabs, at most one tab), then have the two blocks just call it and use a switch statement to assign strings. If you do that, go ahead and re-request sr on the new version.

Comment 9

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 419799 [details] [diff] [review]
fix v1.1

r/sr=pink

The code for counting tabs/windows and turning that into a string ident to load looks like it's very similar in two different places. Is there a way to have only 1 instance?

Comment 10

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

It sounds like pink and smorgan both want the same change here (or maybe it's two sets of duplicated code?), so going to hold checkin for a patch with less code duplication.

Comment 11

[Chris Lawson (gone)]

Attachment: fix v1.2

Re-requesting sr as smorgan suggested, since I re-implemented this the other way.

Comment 12

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 421974 [details] [diff] [review]
fix v1.2

sr=pink

+  if (totalWindows == 1)
+    return (totalTabs > 1) ? eMultipleTabsInOneWindow : eOneWindowWithoutTabs;
+  else if (totalWindows > 1)
+    return (totalWindows == totalTabs) ? eMultipleWindowsWithoutTabs : eMultipleTabsInMultipleWindows;
+  else  // totalWindows == 0
+    return eNoWindows;

no need to have else after a return. Just omit the else clause.

Comment 13

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

Landed on cvs trunk without the else.