493349 - Opt-click on red globe (close button) should have same behavior as opt-cmd-w (close all windows)

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: General, defect)

Description

[Ilya Sherman]

STR:
 1) Open several windows, some with multiple tabs, others with just one tab
 2) Tick the preference to warn when closing windows
 3a) Close all windows with opt-cmd-w (equivalently, File ~> Close All Windows)
 3b) Close all windows with opt-click on the red globe

When you perform step (3a), a single warning dialog pops up.  When you perform step (3b), any windows that have just one tab open close, and the remaining windows each pop up a warning dialog.  IMHO, a single warning dialog is far preferable.

I'd be happy to take a look at this if somebody could point me in the right direction -- how do we detect an opt-click on the red globe?  Do we need to override |NSWindow|'s |close|, or something along those lines?

Comment 1

[Ilya Sherman]

Attachment: Patch

Comment 2

[Christopher Henderson]

Comment on attachment 378098 [details] [diff] [review]
Patch

Doesn't quite have behaviour parity with Cmd+Opt+W. For example, try both with the Downloads and/or Prefs window(s) open. Keyboard will confirm first; opt-click will close the Downloads and/or Prefs, then confirm. If we want parity—which is what the bug summary requests—we'll have to add the closingAllWindows logic to the Downloads, Seach Engine Editor, and Preferences window controllers. If we aren't bothered with those windows staying open, the following line needs to change:

+    NSEnumerator* windowEnum = [[NSApp orderedWindows] objectEnumerator];
Use MainController's |browserWindows|, since some of our windows don't respond to |setClosingAllWindows:| and |closeAllWindows| ignores non-browser windows.

I think we should at least add the logic to the downloads window, since it is the most likely to be open when the user performs the close-all operation.

Testing also exposed some buggy behaviour in our close-all-windows. If a popup window (or even Downloads) is frontmost, with a single multitab window behind it, opt-clicking close (or Cmd+Opt+W) will close both windows without confirmation. This behaviour should be fixed, by making |closeAllWindows| get the frontmost browser window's controller, rather than the main window's controller.

Comment 3

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

Ilya, any updates here?

Comment 4

[Chris Lawson (gone)]

See also bug 528773.

Comment 5

[Ilya Sherman]

Attachment: Patch v2.0

Sorry about the delay here folks.  This patch addresses hendy's review comments: the opt-click logic is now implemented in a NSWindowController subclass, from which all of our window controllers inherit.

Two things I'm not sure about:

1) "CHWindowController" is probably not the best name for this class.  Thoughts on what it should be called and where it should live?

2) MVPreferencesController previously did not subclass NSWindowController -- is my change kosher?

Comment 6

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

Sorry, I just read hendy's review comment carefully for the very first time, and I'm confused.

The preference is about "closing windows with multiple pages open"; that is, by design, only concerned with browser windows (this was because browser windows closing can be dataloss, which none of the other windows can have). We only care about showing a warning when either we have a single browser window with n > 1 tabs, or multiple browser windows.  We aren't changing that logic by tying in all of the other windows (e.g. Downloads), are we?

(In reply to comment #5)
> 1) "CHWindowController" is probably not the best name for this class.  Thoughts
> on what it should be called and where it should live?

Traditionally CH-prefix files had been restricted to handling Gecko embedding (src/embedding) and other subclasses were prefixless or NSFoo+Utils files in src/extensions, but it looks like CH-files have sprouted up all across the tree (src/extensions, safebrowsing, idl) of late :P

Comment 7

[Ilya Sherman]

(In reply to comment #6)
> We only care about showing a warning when either we have a
> single browser window with n > 1 tabs, or multiple browser windows.
> We aren't changing that logic by tying in all of the other windows
> (e.g. Downloads), are we?

Nope, that logic is remaining unchanged.  If one window with exactly one tab is open, and also the Downloads window is open, then opt-clicking either red globe will close both windows.  But, the behavior with this patch now matches that of pressing shift-cmd-w: If three windows are open, and also the Downloads window is open, then opt-clicking a red globe will *not* close the Downloads window *before* presenting the warning about closing multiple tabs/windows.

That said, I don't have a strong preference for this implementation versus closing all non-browser windows first, then presenting the warning -- might be good to have a quick irc discussion.

> (In reply to comment #5)
> > 1) "CHWindowController" is probably not the best name for this class.  Thoughts
> > on what it should be called and where it should live?
> 
> Traditionally CH-prefix files had been restricted to handling Gecko embedding
> (src/embedding) and other subclasses were prefixless or NSFoo+Utils files in
> src/extensions, but it looks like CH-files have sprouted up all across the tree
> (src/extensions, safebrowsing, idl) of late :P

Yeah, NSFoo+Utils seemed like the other promising choice; but AFAICT, all of these files add convenience methods to the NSFoo classes rather than subclassing them, and I don't see a nice way to avoid subclassing.

Comment 8

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

Comment on attachment 419090 [details] [diff] [review]
Patch v2.0

Passing this review to cl per discussions.

Comment 9

[Mike Pinkerton (not reading bugmail)]

I'd advise against using the CH prefix for stuff like this. I'd maybe call the class BaseWindowController to indicate its intent as a base class.

Comment 10

[Chris Lawson (gone)]

Comment on attachment 419090 [details] [diff] [review]
Patch v2.0

There's no way I'm going to be able to review this anytime soon, unfortunately. At a glance it looks OK, but that's hardly a thorough review :-p