407883 - Fix Downloads window opening and focusing prefs logic (and names)

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, defect)

Description

[Ben Willmore]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-gb) AppleWebKit/523.10.3 (KHTML, like Gecko) Version/3.0.4 Safari/523.10
Build Identifier: Version 2007112801 (2.0a1pre)

[I apologize if I have filed this before. I certainly can't find it on bugzilla.]

browser.download.progressDnldDialog.bringToFront controls whether the Download window comes to the front when a new download begins. Currently, this works only some of the time:

1. Set browser.download.progressDnldDialog.bringToFront to false
2. Start Camino
3. Start a download. The Download window comes to the front.
4. Bring the browser window to the front and start another download. The Download window remains in the background
5. Bring the Download window forward, and close it.
6. Start another download. The Download window comes to the front.

In other words, the Download window remains in the background unless it has not yet been opened, or the user has explicitly closed it.  IMO, the Download window should never jump forward if the preference is set to false.

I believe the patch below fixes this. I apologize that it is untested; I do not have a Camino checkout at the moment.


--- ProgressDlgController.mm	2007-12-11 10:53:55.000000000 +0000
+++ ProgressDlgController.mm.new	2007-12-11 10:55:34.000000000 +0000
@@ -539,19 +539,20 @@
 
 -(void)didStartDownload:(ProgressViewController*)progressDisplay
 {
-  if (![[self window] isVisible]) {
-    [self showWindow:nil]; // make sure the window is visible
-  }
-  
   // a common cause of user confusion is the window being visible but behind other windows. They
   // have no idea the download was successful, and click the link two or three times before
   // looking around to see what happened. We always want the download window to come to the
   // front on a new download (unless they set a user_pref);
   BOOL gotPref = NO;
   BOOL bringToFront = [[PreferenceManager sharedInstance] getBooleanPref:"browser.download.progressDnldDialog.bringToFront" withSuccess:&gotPref];
-  if (gotPref && bringToFront)
-    [[self window] makeKeyAndOrderFront:self];
+  if (gotPref && bringToFront) {
+    if (![[self window] isVisible]) {
+      [self showWindow:nil]; // make sure the window is visible
+    }
   
+    [[self window] makeKeyAndOrderFront:self];
+  }
+
   [self rebuildViews];
   [self setupDownloadTimer];
   


Reproducible: Always

Comment 1

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

I can't reproduce this on the branch on 10.3.9 or 10.5.1; is this only broken on trunk?

Comment 2

[philippe (part-time)]

On 10.4.11 ppc, all 3 builds (1.5.4, 1.6alpha and trunk) have the same behaviour:
* download window is closed: it opens as the front most window when starting a download.
* download window is open & in the background: it stays in the background.

Comment 3

[Stuart Morgan]

I can't imagine why Smokey can't repro it, since the code obviously has the described behavior. Confirming, since I agree the current behavior doesn't match what someone using the pref would presumably want.

Comment 4

[Stuart Morgan]

Attachment: fix

Cleaned up version of the patch. Since I was touching that code, I also fixed the pref check logic so that if reading the pref fails we do the default, not the opposite.

Comment 5

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

If the window is already open, it doesn't come to the front; if the window is not open, it opens and comes to the front.  That was by design when pink checked it in (unfortunately, without a number again, so I cant find the discussion).

Comment 6

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

I should add: if the window is not open, I want it to open and be visible (in front of windows from other applications that share the same "spot" on my screen); I don't want it to open hidden.  What I do want is for additional downloads not to steal focus from my current page, which is what we currently do.

Comment 7

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

Bug 231580, but I think we must have some of the discussion on irc because it's not all in the bug (or I'm crazy).

Comment 8

[Stuart Morgan]

I'm not clear on why we think someone who has expressed a desire through prefs not to have the window popping up would want it to pop up sometimes. We should talk about it tomorrow.

Comment 9

[Stuart Morgan]

Comment on attachment 292720 [details] [diff] [review]
fix

Not the approach we are taking, per meeting.

Comment 10

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

For those not wanting to brave the log (http://wiki.caminobrowser.org/Status_Meetings:2007-12-12:Log), here's an executive summary:

1) We're going to support two prefs, one for opening the window and one for focusing the window, instead of the single confusingly-named one

2) We're going to adopt the Firefox pref names, which have sane, descriptive, and accurate names for these two prefs

3) Since we're going to need pref migration code anyway, we're going to migrate the other download prefs to their equivalent Firefox/toolkit names as long as those are all sanely named (and at a quick glance, they are)

I'll be back again with a old-name|new-name table; I didn't quite get to that tonight.

Comment 11

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

// Auto download instead of prompt for name/location for each dl
browser.download.autoDownload = browser.download.useDownloadDir

// Open downloaded files when dl complete
browser.download.autoDispatch = [no toolkit equivalent]

// Keep downloads window open when all dls are done
browser.download.progressDnldDialog.keepAlive (true) = \
                                 browser.download.manager.closeWhenDone (false)

// When/how to remove completed downloads from the list
browser.download.downloadRemoveAction = browser.download.manager.retention

// If downloads window is open, bring to front when each dl starts
browser.download.progressDnldDialog.bringToFront = \
                                     browser.download.manager.focusWhenStarting

// If downloads window is not open, open when starting a new dl
[New hidden pref, default to true] = browser.download.manager.showWhenStarting


* Some notes/thoughts from me:
  - our existing "downloadRemoveAction" name seems more clear than the toolkit 
    pref name
  - our existing "autoDownload" name seems marginally better than toolkit pref 
    name, but both leave a bit to be desired
  - "autoDispatch" has no toolkit equivalent, but it should get a more 
    descriptive name (and move to the browser.download.manager. prefix)

Comment 12

[Chris Lawson (gone)]

I'd call the first one browser.download.promptForSaveLocation or something similar to make it more obvious. I'm not a huge fan of either the current pref (see bug 360562) or the toolkit name for it.

cl

Comment 13

[Stuart Morgan]

Where there is a direct toolkit analog, I think we should use it. It makes things less confusing all around, as well as more discoverable for people who either come from Firefox or who find pages like the kb prefs docs.

Comment 14

[Peter da Silva]

I have been bugged about the way the download window behavior was changed to give the focus to the download window when a download starts for some time, and finally decided to file a bug about it. I was surprised to learn that this was not a bug, it was deliberate decision. I will immediately set this option in my prefs file.

On the other hand, I think that if the download window is not open it would be counterintuitive to have it open in the background... is there any other Mac application that does that?

Comment 15

[Peter da Silva]

I think that "autoDispatch" should be removed, and this capability should not be available. It's a security flaw.

Comment 16

[Stuart Morgan]

(In reply to comment #15)
> I think that "autoDispatch" should be removed, and this capability should not
> be available.

That's not at all within the scope of this bug.

Comment 17

[Peter da Silva]

OK, I've tried setting both sets of options in prefs.js and it still pops up when starting a download. Which setting actually works for Camino 1.5?

Comment 18

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

There's not currently a way to suppress the opening of the window altogether.

Comment 19

[Peter da Silva]

I'm not trying to suppress showing the window. I'm trying to suppress it popping up and stealing focus. According to the previous comments in this PR that should be possible, but I can't make it work.

Comment 20

[Stuart Morgan]

This bug is still open because the changes haven't been made yet, so much of the discussion here does not apply to any build of Camino. If you need help with the current options, please use the help pages or the forums, not this bug.

Comment 21

[Peter da Silva]

The changes listed above aren't about the original bug either, which as far as I can tell seems to be a request that Camino break the human interface guidelines and should have been closed "wont fix".

However the documented setting does not seem to work either. That actually seems about as relevant as changing the names of the settings does. Should I open a new bug about the documented setting (in the help pages) not actually working?

Comment 22

[Stuart Morgan]

(In reply to comment #21)
> The changes listed above aren't about the original bug either

The changes listed above are about changing the preferences controlling download focus (and keeping the overall set of preferences consistent in naming while doing so), which is in fact what this bug is about.

> which as far as I can tell seems to be a request that Camino break the
> human interface guidelines

I've read the HIG discussion of windows, and I'm not aware of anything in them preventing respecting user preferences about window ordering. If you provide a link to the relevant section, we'll take that into consideration.

> and should have been closed "wont fix".

Since the people responsible for making WONTFIX decisions agreed on the approach described, no, it should not have been.

> Should I open a new bug about the documented setting (in the help pages)
> not actually working?

Yes, with clear steps to reproduce, expected behavior, and observed behavior.

Comment 23

[Peter da Silva]

OK, you're right, sorry, I misread the code.

Comment 24

[Chris Lawson (gone)]

I'll fix this as soon as bug 384689 lands (since these are both going to touch the Gecko pref constants header file).

Ideally the migration would only happen once, but I think we can expect a fair number of people to do some switching back and forth between 1.6.x and 2.x (as has happened in the past with new versions). How do we want to handle it?

cl

Comment 25

[Chris Lawson (gone)]

Attachment: fix v1.0

There's some gunk in this patch from the other prefs migration bug and from bug 411189, but it should be pretty obvious what it is. I'd like to get this reviewed soon-ish in the hopes that we can do this migration in conjunction with Camino 2.0.

Comment 26

[Christopher Henderson]

Some notes:

Line 49 should read
+// Controls whether the download manager opens when a download is started

The patch won't compile without OrgMozillaChimeraPreferenceDownloads being changed to OrgMozillaCaminoPreferenceDownloads in Download.h too.

Patch gives the following output, but succeeds fine:
Patching file camino/resources/application/all-camino.js.in using Plan A...
Hunk #1 succeeded at 69 (offset -2 lines).

With the focus pref set to false, and the show pref set to true, a download will cause a closed progress window to open and become front and key. Expected behaviour for me would be to display the window, but at back and not key. Perhaps the code could (1) get the key NSWindow; (2) display the progress window; (3) send the progress window back; (4) set the key window to that obtained in (1)?

Comment 27

[Christopher Henderson]

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

Forgot to r- it.

Comment 28

[Chris Lawson (gone)]

I talked with hendy on IRC and the "Chimera" thing was a result of his trying to apply the patch to an old tree ;)

The behaviour is indeed not what I intended, however, so I'll look into that and post a fixed patch as soon as I can.

Comment 29

[Chris Lawson (gone)]

Attachment: fix v1.1

Addresses hendy's issue with v1.0. A couple things to consider...

1) When we migrate prefs, do we care about removing the old ones once they're migrated? The patch as-is doesn't remove them, so we're effectively letting minor cruft build up in the prefs file. I can stick some pref removal code in there if we don't want the cruft.

2) Is "open the manager immediately behind the key window" a good experience for multi-window users? I only ever really have maybe two Camino windows open maximum, so I might not be the best person to be making this call. I'm just sort of guessing at what hopefully doesn't suck for these sorts of users. (Of course, how many of our users routinely use multiple windows *and* would have this -- hidden -- prefs combination set? Two? Five? :-p)

Comment 30

[Ilya Sherman]

(In reply to comment #29)
> 1) When we migrate prefs, do we care about removing the old ones once they're
> migrated? The patch as-is doesn't remove them, so we're effectively letting
> minor cruft build up in the prefs file. I can stick some pref removal code in
> there if we don't want the cruft.

What's the harm in leaving the cruft?  It might be good to leave, so that switching back to an older version of Camino minimally breaks things.
 
> 2) Is "open the manager immediately behind the key window" a good experience
> for multi-window users? I only ever really have maybe two Camino windows open
> maximum, so I might not be the best person to be making this call. I'm just
> sort of guessing at what hopefully doesn't suck for these sorts of users. (Of
> course, how many of our users routinely use multiple windows *and* would have
> this -- hidden -- prefs combination set? Two? Five? :-p)

I'm also the wrong person to ask, but I think having it be the last window in the ordering might be better.

Comment 31

[Chris Lawson (gone)]

Attachment: fix v1.2

This patch:

* also migrates browser.download.autoDispatch, per comment 11, to a new pref called browser.download.manager.openDownloadedFiles
* removes the old download prefs, since Camino 2 is going to break a lot of 1.x profile data anyway
* orders the downloads window behind any browser windows, not just the key window

I feel sort of skanky about making ProgressDlgController know about BrowserWindowController (or BrowserWindow, which is the other obvious choice), but I can't think of any other way to do it. |orderBack| puts the window at the bottom of its layer, but that's system-wide, not app-specific, so you can get the download manager appearing behind background apps' windows. That just seems wrong. I also tried using |orderWindow:relativeTo:| with [[[NSApp orderedWindows] lastObject] windowNumber], but that had the same effect as |orderBack|. (There might be an Apple bug at work in the latter case, since that definitely seems wrong to me.)

Comment 32

[Christopher Henderson]

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

Preferences migrate OK, and the download window behaviour works as it should.

This patch needs to be un-bitrotted due to the landing of Bug 407883.

There are some long method invocations that could benefit from some : alignment.

+  NSWindow* manager = [self window];
I think it would be clearer if it were named something like downloadsWindow.

+    NSMutableArray* windowArray = [NSMutableArray array];
Instead of an array, just keep a reference to the last appropriate NSWindow found and order the download's window relative to its window number.

Good stuff otherwise. I'm fine with the way of ordering the window back, as it's clear what we're doing.

Comment 33

[Chris Lawson (gone)]

Attachment: fix v1.21

Addresses review comments.

Comment 34

[Stuart Morgan]

Comment on attachment 353357 [details] [diff] [review]
fix v1.21

Looks great except for one tweak:

>+    // Send it to the back of all browser windows for now, since we'll re-focus it 
>+    // later if we need to. This is sort of arbitrary, but it beats |orderBack|
>+    // (which includes visible windows from other apps and can thus look weird).
>+    // NB: We include popups and view-source windows in this definition of
>+    // "browser window", though we generally don't elsewhere.
>+    NSEnumerator* windowEnum = [[NSApp orderedWindows] objectEnumerator];
>+    NSWindow* rearmostBrowserWindow = nil;
>+    
>+    NSWindow* curWindow;
>+    while ((curWindow = [windowEnum nextObject])) {
>+      if ([[curWindow windowController] isMemberOfClass:[BrowserWindowController class]])
>+        rearmostBrowserWindow = curWindow;
>+    }
>+    
>+    [downloadManagerWindow orderWindow:NSWindowBelow relativeTo:[rearmostBrowserWindow windowNumber]];
>+    // Restore the stored key/main window.
>+    [key makeKeyAndOrderFront:self];

99.9% of users will not have set a hidden pref to change the download window focus behavior, so we shouldn't do this dance for them all the time. Conditionalize this whole block on the hidden pref for suppressing focus.

Comment 35

[Chris Lawson (gone)]

Attachment: fix v1.22

No more dancing windows for pink's girlfriend or my mom.

Comment 36

[Stuart Morgan]

Comment on attachment 357293 [details] [diff] [review]
fix v1.22

>+  if (gotPref && shouldOpenManager && ![downloadManagerWindow isVisible]) {

This isn't quite right; the default is to open it, so if somehow the pref system has failed then we should open the window. It should be:

    if ((shouldOpenManager || !gotPref) && ![downloadManagerWindow isVisible]) {

And two other things I missed:

>+      NSEnumerator* windowEnum = [[NSApp orderedWindows] objectEnumerator];
>+      NSWindow* rearmostBrowserWindow = nil;
>+
>+      NSWindow* curWindow;
>+      while ((curWindow = [windowEnum nextObject])) {
>+        if ([[curWindow windowController] isMemberOfClass:[BrowserWindowController class]])
>+          rearmostBrowserWindow = curWindow;
>+      }

It's much faster to find the last window from the back than the front:

        NSEnumerator* windowEnum = [[NSApp orderedWindows] reverseObjectEnumerator];
...
        while ([...]) {
          if ([[curWindow windowController] isMemberOfClass:[BrowserWindowController class]]) {
            rearmostBrowserWindow = curWindow;
            break;
        }


>+      [downloadManagerWindow orderWindow:NSWindowBelow relativeTo:[rearmostBrowserWindow windowNumber]];
>+      // Restore the stored key/main window.
>+      [key makeKeyAndOrderFront:self];

If there are no browser windows, then nothing should happen, so the three lines above should be conditionalized on |if (rearmostBrowserWindow)|

sr=smorgan with those changes.

Comment 37

[Chris Lawson (gone)]

Attachment: fix v1.23

Addresses sr comments and un-rotted, ready for checkin.

Comment 38

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

Comment on attachment 359697 [details] [diff] [review]
fix v1.23

So, 

1) the UI for "Manual" and "Upon Completion" are reversed with this patch

2) focusWhenStarting doesn't appear to work; the only time it ever focuses is when it also opens the window, regardless of whether the pref is set to true or false.

I think those are the only two problems, but I'm not positive I tested everything.

Comment 39

[Chris Lawson (gone)]

Attachment: fix v1.3

This addresses Smokey's r- comments; the former was caused by my missing one spot in the code where translation between old menu layout and new prefs had to take place, and the latter was a logic error.

Comment 40

[Ilya Sherman]

Comment on attachment 366041 [details] [diff] [review]
fix v1.3

A few minor issues:

>+    BOOL bringToFront = [[PreferenceManager sharedInstance] getBooleanPref:kGeckoPrefFocusDownloadManagerOnDownload
>+                                                               withSuccess:NULL];
>+    if (![downloadManagerWindow isVisible] || bringToFront)
>+      [self showWindow:nil];
>+
>+    if (!bringToFront) {

Both of these if statements should use gotPref since our default is YES.

>+        if ([[curWindow windowController] isMemberOfClass:[BrowserWindowController class]])
>+          rearmostBrowserWindow = curWindow;
>+          break;

Missing curly braces here


And a bunch of nits; feel free to ignore ones that are too nitpicky :)

>+  // Translate the old-prefs menu layout to the new-prefs numbers.
>+  int prefTagValue = (2 - [self getIntPref:kGeckoPrefDownloadCleanupPolicy withSuccess:NULL]);
>+  [mDownloadRemovalPolicy selectItem:[[mDownloadRemovalPolicy menu] itemWithTag:prefTagValue]];

The comment here is a little confusing -- the layout is the same, but the numbers are different.

>+  // The old pref is the opposite of the new one, so old 0 = new 2, and old 2 = new 0.
>+  // To avoid creating a new nib, just subtract the tag value from 2.

Again, it's not that we don't want to muck with creating a new nib -- it's that our menu layout doesn't match the toolkit ordering, and we like our ordering better but want to be consistent with toolkit.

>+    NSWindow* key = [NSApp keyWindow];

I would call this keyWindow

>     BOOL gotPref;
>+    BOOL shouldCloseWindow = [[PreferenceManager sharedInstance] getBooleanPref:kGeckoPrefCloseDownloadManagerWhenDone
>+                                                                     withSuccess:&gotPref];
>+    if (gotPref && shouldCloseWindow)

No longer need gotPref here since default is now NO

> CHBrowserService::Show(nsIHelperAppLauncher* inLauncher, nsISupports* inContext, PRUint32 aReason)
> {
>   PRBool autoDownload = PR_FALSE;
>   
>   // See if pref enabled to allow automatic download
>   nsCOMPtr<nsIPref> prefService (do_GetService(NS_PREF_CONTRACTID));
>   if (prefService)
>-    prefService->GetBoolPref("browser.download.autoDownload", &autoDownload);
>+    prefService->GetBoolPref(kGeckoPrefDownloadToDefaultLocation, &autoDownload);
>   
>   nsCOMPtr<nsIFile> downloadFile;
>   if (autoDownload)
>   {

Do we want to also update the variable name and comments to be clearer than "auto download"?


r+ with the above addressed

Comment 41

[Chris Lawson (gone)]

Attachment: fix v1.31

Addresses Ilya's review comments.

Comment 42

[Chris Lawson (gone)]

--> trunk, since we don't have any plans to fix this for 1.6.x.

Comment 43

[Stuart Morgan]

Comment on attachment 366450 [details] [diff] [review]
fix v1.31

>+  // To avoid rearranging the menu, whose layout we're happy with,
>+  // just subtract the tag value from 2.

Tags have nothing to do with layout (tag != index), so this comment doesn't make sense.

sr=smorgan with that comment changed, but why are we doing the math rather than changing the nib, given that we aren't loc-frozen?

Comment 44

[Chris Lawson (gone)]

Attachment: fix 1.32

This is an updated version of 1.31 for current trunk and without the math, since I updated the nib.

Comment 45

[Chris Lawson (gone)]

Attachment: New nib, needs to be resaved with old toolchain

I don't have my 10.4 box on the road with me, so I couldn't save this with the older IB.

Comment 46

[Stuart Morgan]

I don't think it's in the startup path (it's not unless Gecko is causing it to be created on init), in which case it doesn't matter.

Comment 47

[Chris Lawson (gone)]

Attachment: fix v1.33, now with less bitrot

The recent tree re-opening and pile of landings rotted this, so here's a fresh one.

Comment 48

[Stuart Morgan]

I missed this during earlier review, but the code that moves the download window backwards needs to run iff we actually opened the window. Starting a new download with the don't-focus pref set but the window already open shouldn't move the window backward; it should leave it alone.

Comment 49

[Chris Lawson (gone)]

Attachment: fix v1.4

Tweaked logic to account for Stuart's last comment. Basically:

If we need to open the manager
  If we need to bring it to the front --> do it.
  Otherwise, if it isn't already open
    Open it and send it to the back

If we don't need to open the manager, or if we do, and it's already open somewhere, we do nothing.

That seems right to me.

Comment 50

[Ilya Sherman]

Comment on attachment 380796 [details] [diff] [review]
fix v1.4

Looks good :)

r=ilya

Comment 51

[Stuart Morgan]

Comment on attachment 380796 [details] [diff] [review]
fix v1.4

>+    // Store the current key window before we (maybe) stomp it in case we need it later.
>+    NSWindow* storedKeyWindow = [NSApp keyWindow];

Move this to the beginning of the |else if (![downloadManagerWindow isVisible])| block, and remove the "maybe" and "in case we need it later".

sr=smorgan with that change.

Comment 52

[Chris Lawson (gone)]

Attachment: fix v1.41

Last sr comment addressed, ready for checkin. Good catch, Stuart ;)

Comment 53

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

Landed, with one file change to rename the variable and remove the ! in -(void)maybeCloseWindow

Thanks to everyone who worked through the pain here to bring this nightmare to completion.