Program lock out if a download completes while "are you sure you want to quit" sheet is up, and pref set to close download window

RESOLVED FIXED in Camino1.0

Status

--
critical
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: ed.gough, Assigned: nick.kreeger)

Tracking

({fixed1.8, hang})

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

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

14 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051107 Camino/1.0b1
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051107 Camino/1.0b1

Program will lock out and not quit if a download completes while I am quitting.

I.E. The cancel quit/end download window appears, download completes, it disappears and Camino is locked up.

Reproducible: Always

Steps to Reproduce:
1.Download a file
2.Whilst downloading attempt to quit
3.Don't touch any requests to cancel, end, quit etc.
4.Wait for download to complete
Actual Results:  
Camino locks up
Hmm, WFM 2005110708 (v1.0b1).  When the download completes, I'm able to move windows around, etc., and then click the "Cancel" button and clear the sheet.
(Reporter)

Comment 2

14 years ago
(In reply to comment #1)
> Hmm, WFM 2005110708 (v1.0b1).  When the download completes, I'm able to move
> windows around, etc., and then click the "Cancel" button and clear the sheet.
> 

I have "When Downloads finish", "Close Download Window" Activated in my preferences.
Indeed, that's the key.  Then the DL Manager disappears with the sheet still focused, and nothing else works.  Another locked state bug, fun!
Blocks: 294129
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: hang
Target Milestone: --- → Camino1.0
(Assignee)

Comment 4

14 years ago
Confirming, what happens when open file is selected as a pref as well?
(Reporter)

Comment 5

14 years ago
(In reply to comment #4)
> Confirming, what happens when open file is selected as a pref as well?
> 

It hangs still.
(Assignee)

Comment 6

14 years ago
Thanks, i'll take
Assignee: mikepinkerton → nick.kreeger
(Assignee)

Comment 7

14 years ago
Created attachment 202522 [details] [diff] [review]
Proposed patch

This fixes the crash, I just added a boolean flag to help break the conditional if in |didEndDownload:|. The flag turns itself on when the confirm quit modal runs, and sets itself off when the dialog finishes.

To the Ed: Nice Catch on this bug!
(Assignee)

Comment 8

14 years ago
Created attachment 202541 [details] [diff] [review]
Fixed comments
(Assignee)

Updated

14 years ago
Attachment #202522 - Attachment is obsolete: true

Comment 9

14 years ago
Comment on attachment 202541 [details] [diff] [review]
Fixed comments

If the user opts not to quit, shouldn't the download window close after the modal dialog is dismissed?
(Assignee)

Comment 10

14 years ago
Created attachment 202735 [details] [diff] [review]
Updated to mento's comments

Updated so that if a download completes and the user clicks continue and their pref enables the download window to close, the download window will close after they clicked cancel if a download ended in the time period that the confirm terminate modul was up.
Attachment #202541 - Attachment is obsolete: true
Flags: camino1.0+

Comment 11

14 years ago
Comment on attachment 202735 [details] [diff] [review]
Updated to mento's comments

>+  BOOL                  mAwaitingToCloseWindow;     // true when a download completes when termination log is up

Termination what?

Call it mWaitingToCloseWindow.  Better yet, mShouldCloseWindow.

>+-(void)closeWindowByPref;

I'm glad you moved this into its own method, but I don't really like the name.  Maybe closeWindowAccordingToPref?

> -(void)didEndDownload:(id <CHDownloadProgressDisplay>)progressDisplay withSuccess:(BOOL)completedOK statusCode:(nsresult)status
>-{
>+{  

Nit, nuke extra whitespace after your brace, there shouldn't be a diff on this line.

>+  if (completedOK && !mAwaitingTermination)
>   {
>+    [self closeWindowByPref];
>   }
>+  else
>+  {
>+    mAwaitingToCloseWindow = YES;
>+  }

That's faulty logic, you'll set mAwaitingToCloseWindow if !completedOK.  You want:

if (completedOK) {
  if (!mAwaitingTermination)
    [self closeWindowByPref];
  else
    mAwaitingToCloseWindow = YES;
}
else if (NS_FAILED(status) && status != NS_BINDING_ABORTED) ...

>+-(void)closeWindowByPref
>+{
>+  BOOL gotPref;
>+  BOOL keepDownloadsOpen = [[PreferenceManager sharedInstance] getBooleanPref:"browser.download.progressDnldDialog.keepAlive" withSuccess:&gotPref];

We're not using gotPref, but we probably should be.  Not your fault, it's in the existing code, but let's take this opportunity to fix it.
Attachment #202735 - Flags: review-
(Assignee)

Comment 12

14 years ago
Created attachment 202833 [details] [diff] [review]
More mento updates

Updated via mento's comments
Attachment #202735 - Attachment is obsolete: true

Updated

14 years ago
Summary: Program lock out if a download completes while quitting → Program lock out if a download completes while "are you sure you want to quit" sheet is up, and pref set to close download window
(Assignee)

Comment 13

14 years ago
Created attachment 203027 [details] [diff] [review]
Updated to match the changes in bug 307753

Updated to include the changes made from bug 307753's checkin. I also fixed a comment in |initWithDictionary:| in ProgressViewController.h.
Attachment #202833 - Attachment is obsolete: true

Comment 14

14 years ago
Comment on attachment 203027 [details] [diff] [review]
Updated to match the changes in bug 307753

>+  BOOL                  mShouldCloseWindow;

Put the comment back in.

>+    // these 2 bool's helps us from locking when the termination modal is running
>+    // and a download completes

Get rid of the apostrophe, and add a dose of subject-verb agreement.

>+      // if a request to close was set when the modal was up, 
>+      // checck if the users pref enables the download window to close

Edit this comment, too.

>-    // since we are creating a progressview from a dict, the file
>-    // will not be restarted, so setup file system notifications here

Huh?

Code looks OK, I haven't tested.
(Assignee)

Comment 15

14 years ago
Created attachment 203031 [details] [diff] [review]
Update

Fixed comments, if you read my comments from the last patch posted, the comments in ProgressViewController.h are no longer valid, they weren't taken out when smfr landed the patch for bug 307753.
Attachment #203027 - Attachment is obsolete: true
(Assignee)

Comment 16

14 years ago
Created attachment 203034 [details] [diff] [review]
Comment fixes
Attachment #203031 - Attachment is obsolete: true
(Assignee)

Comment 17

13 years ago
FYI. Just tested this patch with the most current source and it patches and works perfect.

Comment 18

13 years ago
Waiting for review?
(Assignee)

Comment 19

13 years ago
Yes can someone please take a look at this patch?

Updated

13 years ago
Attachment #203034 - Flags: review?

Comment 20

13 years ago
Comment on attachment 203034 [details] [diff] [review]
Comment fixes

>Index: ProgressDlgController.mm
>===================================================================

>+-(void)closeWindowAccordingToPref
>+{
>+  BOOL gotPref;
>+  BOOL keepDownloadsOpen = [[PreferenceManager sharedInstance] getBooleanPref:"browser.download.progressDnldDialog.keepAlive" withSuccess:&gotPref];
>+  if (gotPref && !keepDownloadsOpen && ([self numDownloadsInProgress] == 0))
>+  {
>+    // don't call -performClose: on the window, because we don't want Cocoa to look
>+    // for the option key and try to close all windows
>+    [self close];
>+  }
>+}
>+

I don't like the name of this method; it's really "maybeCloseWindowIfAllDownloadsAreDone"
which is unweildy. -maybeCloseWindow is almost better.

Also, it would be cheaper to test for [self numDownloadsInProgress] == 0 first, and
only then read the pref.

r=me with those changes.
Attachment #203034 - Flags: review? → review+
(Assignee)

Comment 21

13 years ago
Created attachment 204644 [details] [diff] [review]
Fixed smfr's nits

Update to meet the last few comments
Attachment #203034 - Attachment is obsolete: true
(Assignee)

Comment 22

13 years ago
Fixed by checkin from mento, landed on branch and trunk.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Keywords: fixed1.8
You need to log in before you can comment on or make changes to this bug.