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

VERIFIED FIXED in Camino1.0

Status

--
enhancement
VERIFIED FIXED
16 years ago
13 years ago

People

(Reporter: kerner, Assigned: sfraser_bugs)

Tracking

({fixed1.8})

unspecified
Camino1.0
PowerPC
macOS
fixed1.8
Bug Flags:
camino1.0 +

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

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

16 years ago
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.
Severity: minor → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → Future

Comment 2

16 years ago
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.
(Reporter)

Comment 3

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

Comment 4

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

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

15 years ago
Posted patch proposed patch (obsolete) — Splinter Review
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?
Status: NEW → ASSIGNED
Target Milestone: Future → Camino0.9

Comment 11

14 years ago
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).
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

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

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

Comment 15

14 years ago
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
BTW, confirming a quit is bug 193644, in case this bug ends up fixing that one, too.

Comment 17

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

Updated

14 years ago
Summary: Confirm closing a window with multiple tabs open → Confirm closing a window(s) and window with multiple tabs open
Comment on attachment 169144 [details] [diff] [review]
proposed patch

we should try to get some review love on this patch.
Attachment #169144 - Flags: review?(joshmoz)

Comment 19

14 years ago
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!
Attachment #169144 - Flags: review?(joshmoz) → review-

Comment 20

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

14 years ago
Posted patch Proposed patch 2 (obsolete) — Splinter Review
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.
Attachment #169144 - Attachment is obsolete: true
Attachment #174542 - Flags: review?

Comment 22

14 years ago
Posted file updated Navigation.nib (obsolete) —
updated nib file, had to tar/gzip it to get it to attach
Attachment #174543 - Flags: review?

Comment 23

14 years ago
I manually created a diff (of sorts) of what I added/removed in the
localizable.strings file
Attachment #169146 - Attachment is obsolete: true

Comment 24

14 years ago
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"
(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

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

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

14 years ago
fixed spelling error quiting is now quitting
Attachment #174543 - Attachment is obsolete: true

Comment 29

14 years ago
fixed spelling error quiting is now quitting
Attachment #174542 - Attachment is obsolete: true

Updated

14 years ago
Attachment #174643 - Flags: review?

Comment 30

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

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

14 years ago
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.

Updated

14 years ago
Assignee: pinkerton → joshmoz
Status: ASSIGNED → NEW

Comment 33

14 years ago
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
Attachment #174643 - Flags: review? → review-

Comment 34

14 years ago
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.
Status: NEW → ASSIGNED

Comment 35

14 years ago
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.
Target Milestone: Camino0.9 → Camino1.0
Comment on attachment 174543 [details]
updated Navigation.nib

Clearing the review? flag on obsoleted patches
Attachment #174543 - Flags: review?

Comment 37

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

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

14 years ago
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.
*bump*

Any progress on this one, Josh?

Comment 41

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

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

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

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

14 years ago
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.
Bump.

Any progress on this?

cl
(Assignee)

Comment 47

14 years ago
This is a common request in feedback.
Flags: camino1.0+
(Assignee)

Comment 48

14 years ago
Taking.
Assignee: joshmoz → sfraser_bugs
Status: ASSIGNED → NEW
(Assignee)

Comment 49

14 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
(Assignee)

Comment 50

14 years ago
I combined the two prefs into one ("camino.warn_when_closing") with a single checkbox in the prefs.
Why combine the two? I currently have two separate ones and I've checked one and not the other.
(Assignee)

Comment 52

14 years ago
(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.
(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.
(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.
(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

14 years ago
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.
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.
(Assignee)

Comment 58

14 years ago
Mike? One or two prefs?
one pref please.

Updated

13 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.