260818 - Pop up an alert when a download finishes

Status: VERIFIED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Mailnews shows a visual alert when new mail arrives.  Presently, seamonkey can
only play a sound (see bug 16498) or do nothing.  We should show an alert
similar to the mailnews alert when a download finishes.

Comment 1

[Colin S]

Firefox currently does this. Are you suggesting a backport of this functionality
or were you not aware it existed?

Comment 2

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch (backend)

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

+        //we know prefs is not null and we got the pref branch
+        // alertPref (initialized to 0, if it changed, prefs are working)

please be consistent with spaces after the // - file style is a space.

+        if (NS_SUCCEEDED(rv))
+          {

style of this file is:
    if (NS_SUCCEEDED(rv)) {

+            NS_ENSURE_SUCCESS(rv, rv);

no, you cannot do that. see the comment below:
       // can't do an early return; have to break reference cycle below

+            rv =
alertsService->ShowAlertNotification("http://ctho.ath.cx/moztree/mozilla/themes/modern/messenger/icons/new-mail-alert.png",
finishedTitle, finishedText, PR_TRUE, 

you aren't suggesting checkin this in as-is here, do you? :)

Comment 4

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch

Do we want to play a sound too with the alert?	Mailnews does.	It would be
fairly easy to add.

(In reply to comment #3)
> no, you cannot do that. see the comment below:
>	 // can't do an early return; have to break reference cycle below
Fixed.

>
alertsService->ShowAlertNotification("http://ctho.ath.cx/moztree/mozilla/themes/modern/messenger/icons/new-mail-alert.png",

> finishedTitle, finishedText, PR_TRUE, 
> 
> you aren't suggesting checkin this in as-is here, do you? :)

This way I can do a "download finished icon of the day" thing by rotating the
image on my server ;).

The frontend changes in this patch are not done yet (I have to clean it up,
move the JS to a separate file, etc.)

Comment 5

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: pref-download.js

New JS file

Comment 6

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch to existing files

Comment 7

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

     content/communicator/pref/pref-keynav.xul                  
(prefwindow/resources/content/pref-keynav.xul)
+    content/communicator/pref/pref-download.js                
(prefwindow/resources/content/pref-download.js)

I fixed the indentation there on my local copy.

Comment 8

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: slightly revised patch

Patch browser-prefs.js, fix the whitespace.

Comment 9

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Should I track the alerts in mDownloadManager so it only has one up at a time? 
I considered doing it in nsDownload, but that wouldn't have actually worked.  I
just tested, and it's quite slow when you have a lot of alerts sliding up and
down simultaneously.

Comment 10

[jag (Peter Annema)]

Comment on attachment 159908 [details] [diff] [review]
slightly revised patch

I like this patch in general, but nitpicky as I am:

I think I'd prefer it if you just moved the GetCharPref for the sound_url
inside |if (NS_SUCCEEDED(rv) && playSound)| on the GetBoolPref for the
download_sound, and left the |if (!soundStr.IsEmpty())| as it is, instead of
having this rather strange dependency between prefBranch not being null and
playSound being true. Yes, I understand the code, and yes, the comment helps,
but really, there's no need for the comment if you rearrange the code a little
as suggested. B.t.w., use PR_FALSE, not 0, to set the showAlert bool to false.

I understand the need for the do { } while (0); there, it's clever, but boy is
that ugly. I wish I could think of a more elegant way to do that. I guess you
could put the stuff inside the |if (NS_SUCCEEDED(rv))| in a helper function
with two PRUnichar** params... Or just go ahead and nest.

+	   const PRUnichar *strings[] ={ mDisplayName.get() };

Add a space between = and {

+  <stringbundle id="bundle_prefutilities"
+		
src="chrome://communicator/locale/pref/prefutilities.properties"/>  

Why are you adding that? Oh, I see, choosesoundurl, which you seem to have
replaced with an entity in pref-download.dtd. So you can undo your change to
prefutilities.properties.

+<!ENTITY doNothing2.accesskey "N">

Hmm? Unused?

+<!ENTITY playSound.accesskey "S">
+<!ENTITY showAlert.accesskey "A">

Typically those would be interleaved with the associated labels, see the rest
of that file.

Comment 11

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: re-revised patch

(In reply to comment #10)
> could put the stuff inside the |if (NS_SUCCEEDED(rv))| in a helper function
> with two PRUnichar** params... Or just go ahead and nest.
I didn't need to pass any parameters.

Comment 12

[jag (Peter Annema)]

Ah, yeah, of course. Nice, clean, I like it. Waiting for r=, then I'll sr=.

Comment 13

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Timeless suggested I ask for a cvs repository copy of
http://lxr.mozilla.org/seamonkey/source/mailnews/base/prefs/resources/content/pref-notifications.js#5
to pref-download.js, and then patch against that.

Comment 14

[timeless]

Comment on attachment 159814 [details]
pref-download.js

perhaps this should be a cvs repository copy + diff for download changes.

>  // if we don't have the alert service, hide the pref UI for using alerts to notify on new mail
>  // see bug #158711

comment should be fixed to say download completion instead of new mail

>  var fp = Components.classes["@mozilla.org/filepicker;1"]
>                       .createInstance(nsIFilePicker);

it'd be nice if the .'s were in the same column ...

>    var ioService = Components.classes["@mozilla.org/network/io-service;1"]
>                     .getService(Components.interfaces.nsIIOService);

and here

Comment 15

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: diff between pref-download.js and the mailnews original

<timeless> chris: could you post a (cvs)diff between mail and your version?

Comment 16

[jag (Peter Annema)]

Comment on attachment 160277 [details] [diff] [review]
re-revised patch

+      nsCOMPtr<nsIPrefBranch> prefBranch;
       if (prefs) {
-	 nsCOMPtr<nsIPrefBranch> prefBranch;

that prefBranch is only needed inside |if (prefs)|, I think it's a left-over
from a previous iteration.

PRBool playSound is only needed inside |if (prefBranch)|. Try to restrict
declarations to the smallest scope possible, it allows humans reading the code
to keep track of fewer variables (no pun intended).

Fix those two nits, and sr=jag

Comment 17

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: to check in

Comment 18

[Dave Miller [:justdave]]

I went to do this move, and notice there's an .xul file by the same name right
next to it, and based on the other contents of both the source and destination
directories, they appear to come in pairs.  Do both files need to be moved?

Comment 19

[[e-mail]]

(In reply to comment #18)
>Do both files need to be moved?
I don't think you want too do that, Dave ;-)
pref-downloads.xul already exists, so we only need to create pref-downloads.js

Comment 20

[Dave Miller [:justdave]]

OK, for the record, what's the source and destination directories for the cvs copy?

I see the source in comment 13, but I don't see the destination here.  It was
told to me on IRC a few days ago, but I can't find the transcript.  It should be
on the bug anyway, just for posterity.

Comment 21

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #20)
> OK, for the record, what's the source and destination directories for the cvs
copy?
> 
> I see the source in comment 13, but I don't see the destination here.  It was
> told to me on IRC a few days ago, but I can't find the transcript.  It should be
> on the bug anyway, just for posterity.

Source:
mailnews/base/prefs/resources/content/pref-notifications.js
Dest:
xpfe/components/prefwindow/resources/content/pref-download.js

Comment 22

[Dave Miller [:justdave]]

OK, file copy/tag removal completed.

Comment 23

[Stephen Donner [:stephend] Not actively reading bugmail]

I've verified that this works on Windows XP using build 2004-10-19-04 with the
Seamonkey trunk.

Saved links from:
 * Save target as
 * Files that were prompted from a page
 * Regular clicking

Tested sounds as well.

Windows 9x architectures may be different -- if they work here, great, if not,
file a separate spin-off bug.