16498 - Finished Downloading sounds

Status: RESOLVED FIXED

(SeaMonkey :: Download & File Handling, enhancement, P3)

Description

[slschroe]

In Navigator, I would love to have the computer make some sort of sound when a
file is finished downloading. Often I am doing something else at the time, and
am not at my computer. After a certain period of time, my screen goes blank and
I have no way of knowing it's finished, other than going to my computer and
reactivating the screen. This would be handy for sending e-mail messages, too.
If I have large files to send, it can take a while, and I run into the same
situation.

Comment 1

[Doug Turner (:dougt)]

Good idea.

SilentDL will not be in 1.0 release.  Latering.

Comment 2

[leger]

Moving from Silent Download (this component being deleted) to Installer.

Comment 3

[Christopher Hoess (gone)]

LATER is deprecated per bug 35839.

Comment 4

[Christopher Hoess (gone)]

->default

Comment 5

[Henrik Gemal]

not installer. Sound like download manager to me

Comment 6

[Pham]

The pop-up blocking sound?

Comment 7

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

I'm working on this now.

Comment 8

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

Attachment: Patch v 0.0001a

If you don't have both of the 2 prefs set, you get no sound, and an error in
your JS console.  I suspect I should do a try{}catch() around the pref
accesses, but I don't know for sure.  Maybe someone will enlighten me.

As far as I can tell, this works whether or not the specified
finished_sound_url exists (if it doesn't, you just get a beep).  Also, I don't
know how to detect the difference between a cancelled download and a completed
one, so with this patch, cancelling a download will also result in the sound
playing.  It seems to me that playing sound on cancel could be annoying, since
the user already knows they're cancelling the download.

This might only work for the download manager.	I don't know if separate
download dialogs use the same code or not.

Comment 9

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

Attachment: Patch v 0.0001b

-Doesn't cause any JS errors now.
-Now detects the difference between a cancelled download and a completed
one.
-Definitely only work for the download manager.  Progress dialogs don't use
this code.  I'll take a look at making the change in C++ instead of JS (I'm
told that both download methods should share code there).

Comment 10

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

Attachment: New patch

This patch applies to 2 different files.  It works for progress dialogs and
download manager.  The only real issue I see is the need to maintain the same
code in 2 locations.  I have some somewhat-ugly C++ code that only needs to be
applied to one file which I could clean up if this patch isn't acceptable.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 133813 [details] [diff] [review]
New patch

>+	if (aStatus<0x80000000)	//is there a better way to detect this?

Components.isSuccessCode(aStatus) perhaps.  See 
http://www.mozilla.org/scriptable/components_object.html#_isSuccessCode

>
>+	      var sound = Components.classes["@mozilla.org/sound;1"]
>+				    .createInstance(Components.interfaces.nsISound);

What if this fails?

Comment 12

[[e-mail]]

Comment on attachment 133813 [details] [diff] [review]
New patch

As discussed on IRC.

Comment 13

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

Attachment: Revised patch

Personally, I find these sounds somewhat annoying.  How about a pref for
minimum file size before playing a sound?  For a big download, notification is
good.  When I save a bunch of small pages, notification is annoying.  It looks
like I could just check mMaxBytes and compare that to such a pref.  Maybe that
should be a separate bug (?).

Comment 14

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

Attachment: Re-revised patch

Fixed indentation, getting of null branch, and used null checks instead of
NS_SUCCEEDED(rv).  Sorry for the bugspam ;).

Comment 15

[Jan Varga [:janv]]

Comment on attachment 134075 [details] [diff] [review]
Re-revised patch

looks good
r=varga

Comment 16

[Boris Zbarsky [:bzbarsky]]

This needs module owner approval from a UI owner before I'll even look at it...

Comment 17

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

Attachment: Makefile patch

Comment 18

[[e-mail]]

Marking blocker as ideally we only want finish sounds for true downloads rather
than when opening helper apps. (But feel free to check this in first ;-)

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 134075 [details] [diff] [review]
Re-revised patch

>Index: xpfe/components/download-manager/src/nsDownloadManager.cpp
>+          if (!StringBeginsWith(soundStr, NS_LITERAL_CSTRING("file://")))

What if it's a jar:// url?  Or a chrome:// one?  Or resource:// ?

For that matter, is there a reason this can't work with http:// ?  Or data: ?

Comment 20

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

Attachment: Ditch support for system sounds.

Re comment #19:
Since there doesn't seem to be a reliable way to detect system sounds, this
always uses a URL now.	I tested it with file:// and http:// URLs.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 134186 [details] [diff] [review]
Ditch support for system sounds.

looks good

Comment 22

[[e-mail]]

Attachment: Do we need to patch all.js too?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 134560 [details] [diff] [review]
Do we need to patch all.js too?

I suppose we could do that.. though I'd prefer that we just put the try/catch
around the pref get in the JS (which I forgot when I reviewed... :( )

Comment 24

[Boris Zbarsky [:bzbarsky]]

Er, this is not JS.  What am I smoking?  In any case, we should find a better
way of documenting prefs than bloating the pref hashtables.

Comment 25

[Martin Poirier]

Regarding comment #13, I agree this is a very good Idea, as I would want a sound
for a download that is very quick. However, checking for filesize will to
determine of we play a sound or not will have different effects depending of the
type of connection. As an example, with my cable connection, sometimes I
download files of a few megs in seconds only. I wouldn't like the sound for a
download that only lasts a few seconds. Perhaps the threshold could be set at 5
seconds, or something similar.

Comment 26

[Patrick]

Just a quick question: what's the status of this bug?

I see some conflicting info:
- According to http://www.mozilla.org/status/2003-11-10.html this feature should
work now. 
- But I can't seem to get it to work on my WinXP box and i see no comments here
about "patched checked in". Furthermore, this bug is also still marked as NEW.

Thanks

Comment 27

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

Patrick, the code to support this feature is present, but you'll need to apply
the all.js patch to add the pref "browser.download.finished_sound_url" and give
it a value before a sound will actually be played.  You'd probably want to set
it to something like "file:///c:/Windows/Media/notify.wav".

Someone still needs to review the all.js patch, and then I guess the next step
is making this user-visible through one of the pref dialogs (if the UI people
approve).  I suppose that could be done in a separate bug if we wanted to
resolve this one.

Comment 28

[Patrick]

Chris: thanks for your reply. This was my bad :-\ 
I added the pref to user.js, thinking it would end up in all.js that way...sorry
for the spam.

Comment 29

[Patrick]

FWIW: one other thing, which perhaps is of use to others trying this. Initially
i used the same syntax as for instance is used for the New Mail sound:
user_pref("mail.biff.play_sound.url","c:\\NewMail.wav");

however it seems this pref requires the syntax as given by Chris in comment 27
"file:///c:/FileDownloaded.wav"

Comment 30

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

It's because I gave up on figuring out how to support the other format of
sounds, and since then my free time has been reduced a lot.  I don't think it
would require more than 3 lines of code if someone wanted to try.  Varga still
needs to review+ the all.js patch too.

Comment 31

[Patrick]

Another thing: could it be that the sound doesn't play while there's a SAVE AS
dialog open?

Comment 32

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

Interesting... I guess the save dialog blocks download manager status changes.

Comment 33

[[e-mail]]

Checked in.

Comment 34

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

assigning bugs I fixed to myself