Closed Bug 16498 Opened 20 years ago Closed 16 years ago

Finished Downloading sounds

Categories

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

x86
Windows 95
enhancement

Tracking

(Not tracked)

RESOLVED FIXED
Future

People

(Reporter: slschroe, Assigned: csthomas)

References

Details

Attachments

(3 files, 5 obsolete files)

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.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → LATER
Summary: Finished Downloading sounds → SilentDownload: Finished Downloading sounds
Good idea.

SilentDL will not be in 1.0 release.  Latering.
Status: RESOLVED → VERIFIED
Component: Silent Download → Installer
Moving from Silent Download (this component being deleted) to Installer.
LATER is deprecated per bug 35839.
Status: VERIFIED → REOPENED
Resolution: LATER → ---
->default
Assignee: dougt → dveditz
Status: REOPENED → NEW
QA Contact: gbush → bugzilla
Summary: SilentDownload: Finished Downloading sounds → Finished Downloading sounds
not installer. Sound like download manager to me
Assignee: dveditz → blaker
Component: Installer → Download Manager
QA Contact: bugzilla → sairuh
Target Milestone: --- → Future
QA Contact: sairuh → petersen
The pop-up blocking sound?
I'm working on this now.
Attached patch Patch v 0.0001a (obsolete) — Splinter Review
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.
Attached patch Patch v 0.0001b (obsolete) — Splinter Review
-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).
Attached patch New patch (obsolete) — Splinter Review
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.
Attachment #133495 - Attachment is obsolete: true
Attachment #133507 - Attachment is obsolete: true
Attachment #133813 - Flags: review?(neil.parkwaycc.co.uk)
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 on attachment 133813 [details] [diff] [review]
New patch

As discussed on IRC.
Attachment #133813 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attached patch Revised patch (obsolete) — Splinter Review
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 (?).
Attachment #133813 - Attachment is obsolete: true
Attachment #134069 - Flags: superreview?(bzbarsky)
Attachment #134069 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #134069 - Attachment is obsolete: true
Attachment #134069 - Flags: superreview?(bzbarsky)
Attachment #134069 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch Re-revised patch (obsolete) — Splinter Review
Fixed indentation, getting of null branch, and used null checks instead of
NS_SUCCEEDED(rv).  Sorry for the bugspam ;).
Attachment #134075 - Flags: superreview?(bzbarsky)
Attachment #134075 - Flags: review?(varga)
Comment on attachment 134075 [details] [diff] [review]
Re-revised patch

looks good
r=varga
Attachment #134075 - Flags: review?(varga) → review+
This needs module owner approval from a UI owner before I'll even look at it...
Attachment #134076 - Flags: superreview?(bzbarsky)
Attachment #134076 - Flags: review?(varga)
Attachment #134076 - Flags: review?(varga) → review+
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 ;-)
Depends on: 132456
Attachment #134076 - Flags: superreview?(bzbarsky) → superreview+
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: ?
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.
Attachment #134075 - Attachment is obsolete: true
Attachment #134186 - Flags: superreview?(bzbarsky)
Attachment #134186 - Flags: review?(varga)
Comment on attachment 134186 [details] [diff] [review]
Ditch support for system sounds.

looks good
Attachment #134186 - Flags: superreview?(bzbarsky) → superreview+
Attachment #134186 - Flags: review?(varga) → review+
Attachment #134075 - Flags: superreview?(bz-vacation)
Attachment #134560 - Flags: superreview?(bz-vacation)
Attachment #134560 - Flags: review?(varga)
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... :( )
Attachment #134560 - Flags: superreview?(bz-vacation) → superreview+
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.
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.
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
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.
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.
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"
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.
Another thing: could it be that the sound doesn't play while there's a SAVE AS
dialog open?
Interesting... I guess the save dialog blocks download manager status changes.
Attachment #134560 - Flags: review?(varga) → review+
Checked in.
Status: NEW → RESOLVED
Closed: 20 years ago16 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
assigning bugs I fixed to myself
Assignee: firefox → cst
Status: REOPENED → NEW
Status: NEW → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.