196359 - Confirm closing a window(s) and window with multiple tabs open

Status: VERIFIED FIXED

(Camino Graveyard :: Tabbed Browsing, enhancement)

Description

[Etan Kerner]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.0.1) Gecko/20030306 Camino/0.7
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.0.1) Gecko/20030306 Camino/0.7

I think it would be very useful to have a sheet slide out when a user attempts
to close a window with multiple tabs open, warning them that all open tabs will
be closed. There could be a preference in this window to not be notified again.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Greg K.]

I don't see a Camino dup for this, so confirming as an enhancement request to
get it off UNCO. Camino's developers should decide whether they want to do it or
not.

See also Mozilla equivalent Bug 108973.

Comment 2

[Sam Johnston]

It seems this functionality has been added in 1.5a, as after browsing with a few
(3?) tabs, and closing the tabs followed by the window itself, I was warned that
multiple tabs were open. There was, however, only one.

Comment 3

[Etan Kerner]

Can we add this to Camino, now that it is in Mozilla?

Comment 4

[Jasper]

It seem sto me this would be an exelent featrure. I cougt myself cloing windows
full of tabs not realising I would loose all my tabs. I know everybody has this
now and then. I think it would be great to add this functionality.

Perhaps we could even add an option where ou could command click on the close
window widget and get camino to override the sheet.

Comment 5

[Steve Jacobs (irc: encro)]

We could perhaps include a "Do not ask me this question again" Checkbox to avoid
the repetition once the user has decided if their setting should be either ask
to close or close the window immediately.

Comment 6

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

BTW, the way Firefox implements this, it also asks when one tries to quit the
entire app, which is very annoying (an extra click when trying to quit). 
When/if Camino implements the warning, I'd prefer it only do so for closing
windows full of tabs...it's much easier to accidentally click the little red
globe than invoke quit... :)

Comment 7

[Colleen Fischer]

True, additional clicks can be annoying, but I think it's just as easy to
accidentally invoke the quit command. Especially when Camino had no close button
on the tabs, I often would quit accidentally when trying to press command-W --
which is right next to command-Q. Possibly this could be worked around by
assigning the close tab command to command-shift-W and close window to command-W
(right now, if you have no tabs open, command-W is the close window shortcut
anyway).

Comment 8

[Nathaniel Madura]

Attachment: proposed patch

Comment 9

[Nathaniel Madura]

Attachment: updated Localizable.strings file

Comment 10

[Mike Pinkerton (not reading bugmail)]

bumping up so the patch gets some love. 

do we want to warn if the user quits with > 1 window open? does ff do that as
well? could that be really annoying, or just an intelligent extension of this
behavior? thoughts?

Comment 11

[Martin Girschick]

Firefox only seems to warn for multiple tabs and not for multiple windows.
Sounds like a good idea to add that as well (again with a "don't bother me"
checkbox). Just make sure that the dialog is not shown if the second window is
the download manager.

Might be an idea to add "command-option-q" for a quit without asking. I think
this is standard behaviour (using the option key for an alternative which
doesn't ask the user).

Comment 12

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

Firefox does not warn on cmd-q if you have multiple windows open--unless any of
those windows have multiple tabs in them.  In that case, you get one warning
dialogue per window of tabs when trying to quit.

I'd prefer a different setting for cmd-q with multiple windows vs. cmd-w for a
window full of tabs, if that's not too arcane for Camino.  Martin's suggestion
of using opt for a one-time toggle/override sounds good, too.

Comment 13

[Jasper]

I think it is a good idea to allow showing a warning for:
1) Closing a window containing multiple tabs. (same as quiting with one window
with multiple tabs)
2) Quiting Camino with multiple windows open.
3) Quiting Camino with multipe windows containing multiple tabs.

We do however have to make sure it is not anoying, so adding an option on all
warnings to "Not show this message again". Do we need to put all these option in
the tab preference pane? I for one would defenitly want these options easily
accesible.

1) I think this option is most valueble, and most users will really apreciate
this. Make it a sheet.
2) This should be a single alert window.
3) This should also be a single alert window.

Comment 14

[Nathaniel Madura]

All of those options have already been implemented in the patch I proposed.

Comment 15

[Nathaniel Madura]

Sorry for the additional post, I was just looking through the thread, and
realized that I hadn't added a summary to the thread when I created the
patch.... so here it is...
When a window is closed with multiple tabs, it opens a sheet and say something
to the affect of this.. ``You are about to close m tabs, are you sure?''

When the browser is quit with multiple tabs or multiple windows it issuse an
Alert window that says one of three things
1. ``You have n windows open, are you sure you want to quit?''
2. ``You have m tabs open, are you sure you want to quit?''
3. ``You have m tabs open in n windows, are you sure you want to quit?''

In writing the patch I found that windows that had been closed were not being
released (should I create a new bug in bugzilla for this?), this was throwing
off some of the counting for the quit warning messages (additionally, this
probably accounts for one of the memory leaks I have seen in Camino). So if I
found a window that ``appeared'' to have been closed then I released it so that
the accounting for the dialog was correct. I use the term appeared because I
wasn't sure how to accurately detect if a window was closed (with out adding
code). So I first check to see if the window is visible, and then check to see
if the window is miniturized.... so it isn't fool proof... the one case that I
came across that might cause a problem is the following...
1. A user has multiple windows open in Camino.
2. the user hides camino (cmd-h)
3. the user command tabs over to camino and issues a quit (pressing q) while
camino is still hidden.

Regardless, this part of the code should probably be removed at some point
anyway. As I said this check is only there because something else isn't
happening that should.

Last but not least, I didn't implement a preference control for this...
primarily because I am not familiar with the way that preferences are being
handled in Mozilla, and it looked like (at least some of) the preferences are
being handled outside of Camino (by mozilla?)

Hope this helps (I for one am definitely interested in this patch landing cause
I accidently close windows all the time), let me know if there is anything more
I can do to help
-Nathaniel

Comment 16

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

BTW, confirming a quit is bug 193644, in case this bug ends up fixing that one, too.

Comment 17

[Jasper]

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

Comment 18

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 169144 [details] [diff] [review]
proposed patch

we should try to get some review love on this patch.

Comment 19

[Josh Aas]

Comment on attachment 169144 [details] [diff] [review]
proposed patch

> Last but not least, I didn't implement a preference control for this...

I think this is a necessary part of fixing this problem. It is important enough
that I don't think we should land a patch that doesen't support
enabling/disabling this.
------------------------
Please remove binary files and nib modifications from patches.
------------------------
If there really is a problem with windows not being release after they are
closed, I don't think your code should be reacting to it. That problem should
be fixed first, and then this code should operate the correct way.
------------------------
Remove lines like "// NSLog(@"Minimized Window");" in the final patch please.
------------------------
+    unsigned i;
...
+    for(i = 0; i < [openBrowserWins count]; i++) {

Should be:

+    for (unsigned int i = 0; i < [openBrowserWins count]; i++) {

Put the i in the more local scope, and there should be a space between the end
of the keyword "for" and the beginning of the parentheses.
---------------------------
Shouldn't "#if DEBUG" be "#ifdef DEBUG"?
---------------------------
+    // not all browser windows are created equal. We only consider those with
+    // an empty chrome mask, or ones with a toolbar, status bar, and resize
control
+    // to be real top-level browser windows for purposes of saving size and 
+    // loading urls in. Others are popups and are transient.
+    if ([[thisWindow windowController]
isMemberOfClass:[BrowserWindowController class]]) {
+      unsigned int chromeMask = [[thisWindow windowController] chromeMask];
+      if (chromeMask == 0 || 
+	   (chromeMask & nsIWebBrowserChrome::CHROME_TOOLBAR &&

I think this and some more code after it comes from somewhere else in Camino.
Perhaps it would be a good idea to create a method for doing what this code
does instead of duplicating it in the codebase?
--------------------------
Same thing about spaces between keywords and parentheses: else if(
--------------------------

I'll do a more thorough review after these comments get addressed. Thanks for
the patch!

Comment 20

[Nathaniel Madura]

Sorry, this was the first time I submitted a patch, I wasn't sure how much I
could change, I will update and resubmit the patch soon.

Comment 21

[Nathaniel Madura]

Attachment: Proposed patch 2

An updated diff patch, I tried to address all the comments, and also created a
preference lookup. I debated which file to add the preferences to, and in the
end I thought it was best to add the preferences to ``Navigation'' (general).
An updated NIB will follow.

Comment 22

[Nathaniel Madura]

Attachment: updated Navigation.nib

updated nib file, had to tar/gzip it to get it to attach

Comment 23

[Nathaniel Madura]

Attachment: changes in the localizable.strings file

I manually created a diff (of sorts) of what I added/removed in the
localizable.strings file

Comment 24

[Nathaniel Madura]

I forgot to mention that I didn't know what to call the preferences, and in
looking through some of the other preferences I didn't see a clear naming
scheme, so I just called them "camino.warn_when_closing" and
"camino.warn_when_quiting"

Comment 25

[sairuh (rarely reading bugmail)]

(In reply to comment #24)
> so I just called them "camino.warn_when_closing" and
> "camino.warn_when_quiting"

should the latter one be named "camino.warn_when_quitting"? (to avoid another
misspelling like refering. :)

Comment 26

[Jasper]

Prefpane looks good, I however do wonder if these prefs belong in the tabs pane.
If the option to not show the alert again exist I think it's best to move the
pref to the tabs pane, as only those users using tabs often would want to change
them. Normal users shouldn't be bothered with "expert" prefs like this in the
general pane.

On the other hand the prefs don't only handle tabs but also windows in which
case it's good the way it's now.

Any idea's?

Comment 27

[Nathaniel Madura]

My first inclination was to put them into the tabs prefpane, but I ultimately
decided that that wasn't the best place for it. I chose the general prefpane
because you will also get a message if you have multiple windows open (1 tab in
each window) and you quit Camino. For the non-power user you want them to be
able to easily disable the feature, and the logical place (in my mind) is to put
the warn when quitting (notice I am learning from my spelling mistakes ;)) check
box is in the general tab. 
After deciding that the warn when quitting checkbox belonged in the general
prefpane I thought that the two messages should not be separated. So I put the
warn when closing message there too.

Another argument is that these messages come up when either a window is closed,
or the application is quit, and those are application wide events, so the
general tab might be best for them.

I am by no means saying I am right, but it is something I thought about and even
got a second opinion on.

Comment 28

[Nathaniel Madura]

Attachment: Updated Navigation.nib 2

fixed spelling error quiting is now quitting

Comment 29

[Nathaniel Madura]

Attachment: Proposed patch 2b

fixed spelling error quiting is now quitting

Comment 30

[Jasper]

I think it's that the prefs are in general, seems the most sensible solution.
Last question I have, are there "Don't show this message again" checkboxez in
the alerts to lets people easily turn them off for the future?

Comment 31

[Nathaniel Madura]

No, at this point there are not checkboxes directly in the alerts. Currently the
patch uses Cocoa built-ins to show the alerts, and I was not immediately aware
of a way to add a checkbox to those alerts (I didn't look either). If there is a
supported method of adding GUI elements to the Alerts it should be simple to do
that. However, it will require a lot more effort to do.

I might flip the question around though. Is it really necessary to do? It seems
to me that a person either likes the functionality or they don't. For a person
that likes the functionality, they are going to see a way to disable the
functionality everytime the close/quit! This seems unnecessary to me. For people
that don't like the functionality, the first logical step to disable it is in
the preferences. The checkboxes are currently in the first prefernce that shows
up when people load the preference window.

Just my two cents... 

Comment 32

[derek fong]

I agree with Nathaniel about leaving this in the preferences.  If Camino already
used that "Don't show this message again" checkbox in other dialogs, I'd be
inclined to agree that this would be "essential" to have for this patch.  But
since this is not used elsewhere in Camino (AFAIK), then putting this checkbox
in the warning dialog would be inconsistent.

Comment 33

[Josh Aas]

Comment on attachment 174643 [details] [diff] [review]
Proposed patch 2b

1. doesn't apply any more
2. pref names are to general

I'll probably finish this patch myself per discussions I had with Mike and
Geoff since we're having a difficult time deciding what we actually want

Comment 34

[Josh Aas]

For now, I don't think this should block 0.9. If it gets done by then, great,
but we shouldn't wait for it.

Comment 35

[Josh Aas]

For now, I don't think this should block 0.9. If it gets done by then, great,
but we shouldn't wait for it.

Comment 36

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

Comment on attachment 174543 [details]
updated Navigation.nib

Clearing the review? flag on obsoleted patches

Comment 37

[Jasper]

Nathan it would be great if you could finish this before we do the 0.9 release.
The prefs part is prrety important.

Comment 38

[Nathaniel Madura]

I thought josh didn't want me to work on this bug anymore, if you read one of
his comments he says he/mike and someone else don't know what they want, and
that josh would probably do the patch himself, so I figured my time had been wasted.

Comment 39

[Jasper]

Well as it is I don't see josh doing this anytime soon. So for the sake of not
wasting time and effort I think it would be great if we could continue work on
this. Mike , Josh care to share the discussion you had about this a while ago so
Nathan can go and finish this. Users, including myself and my girlfriend, would
love to see this happen sonner then later.

Comment 40

[Chris Lawson (gone)]

*bump*

Any progress on this one, Josh?

Comment 41

[Josh Aas]

I implemented most of this a while ago, the problem is that there are a lot of
tricky behavioral choices to make. Its not too hard to code once we can decide
on behaviors. We can probably get around to this after 0.9.

Comment 42

[Jasper]

Coudln't we just have a good conversation this week or something. I really think
that this feature should be added especially since two persons claim to have
patches, only what's need is some discussion. So let's discuss and not push it a
head of us.

Comment 43

[Charles Cooper]

Wouldn't it be easier for the user if closing windows could just be "undone"
(command z) eliminating those thousands of times the confirmation dialog would
pop up unneeded? Accessing history almost works but I think multiple tabbed
windows show up there as multiple single tabbed windows. That is the only fix
that's needed.

Comment 44

[Jasper]

Nathan, as we are already working on the 10.0 release which we hope wil come
soon I think it's a good idea if you start working on this patch again. This is
defenitly something we want.

Comment 45

[Martin Girschick]

I want to suggest to show this dialog (then with a different warning text) when
a "tab group bookmark" is openend. This effectivly kills all existing tabs also
with no way back, therefore similar to closing a window with multiple tabs open.

Comment 46

[Chris Lawson (gone)]

Bump.

Any progress on this?

cl

Comment 47

[Simon Fraser [no longer active]]

This is a common request in feedback.

Comment 48

[Simon Fraser [no longer active]]

Taking.

Comment 49

[Simon Fraser [no longer active]]

I checked a modified version of this patch into the trunk and branch, with "Do
not show again" checkboxes, but without any kind of option-key shortcut (can't
use the option key, since that means "apply to all windows").

Please file new bugs on any tweaking you want to see.

Comment 50

[Simon Fraser [no longer active]]

I combined the two prefs into one ("camino.warn_when_closing") with a single checkbox in the prefs.

Comment 51

[Paul Borokhov (lensovet)]

Why combine the two? I currently have two separate ones and I've checked one and not the other.

Comment 52

[Simon Fraser [no longer active]]

(In reply to comment #51)
> Why combine the two? I currently have two separate ones and I've checked one
> and not the other.

Because we want to make the prefs as simple as possible.

Comment 53

[Paul Borokhov (lensovet)]

(In reply to comment #52)
> (In reply to comment #51)
> > Why combine the two? I currently have two separate ones and I've checked one
> > and not the other.
> 
> Because we want to make the prefs as simple as possible.
> 

but seriously that's lame. the reason i'd want a prompt specifically on QUIT is that i might not realize that there are additional windows behind the one i currently have open. i'd never "not" realize that i have multiple tabs open because i can see all of them anyway. there's also no danger in "accidentally" closing a window with multiple tabs, because the keyboard command is apple-SHIFT-w, and you don't accidentally ever press the shift key. however, in trying to close a tab, you MAY accidentally press Q instead of W and quit the browser.
if nothing else, then at least turn off the prompt when closing using apple-shift-w. that can't be accidental.

Comment 54

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

(In reply to comment #53)
> there's also no danger in "accidentally"
> closing a window with multiple tabs, because the keyboard command is
> apple-SHIFT-w, and you don't accidentally ever press the shift key. however, in
> trying to close a tab, you MAY accidentally press Q instead of W and quit the
> browser.
> if nothing else, then at least turn off the prompt when closing using
> apple-shift-w. that can't be accidental.
> 

That's not true. You could accidentally click the red circle instead of the yellow one (or something similar). This change makes sense to me.

Comment 55

[Paul Borokhov (lensovet)]

(In reply to comment #54)
> (In reply to comment #53)
> > ...
> > if nothing else, then at least turn off the prompt when closing using
> > apple-shift-w. that can't be accidental.
> > 
> 
> That's not true. You could accidentally click the red circle instead of the
> yellow one (or something similar). This change makes sense to me.
> 

look at the text i just quoted. i can see why you'd accidentally click the red gumdrop, but i don't see how you'd accidentally hit the shift key. can we at least turn off this confirmation box for when the keyboard is used instead of the mouse?

Comment 56

[Noemi]

I strongly agree that separate preferences would be preferable.  Everyone uses this stuff differently.  I often will intentionally close a window with multiple tabs using the red button, and having to agree to an extra doalogue woul dbe a real pain, but I also often clumsily hit cmd-Q instead of cmd-W when trying to close a tab, and then lose all of my research.

FWIW.

Comment 57

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

FWIW, I agree with comment 56.  We have two separate prefs for whether to load the home page depending on whether we open a new tab or a new window, so I don't see how this is so different from that.

Comment 58

[Simon Fraser [no longer active]]

Mike? One or two prefs?

Comment 59

[Mike Pinkerton (not reading bugmail)]

one pref please.