179784 - way to select beep in typeaheadfind

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Akkana Peck]

Bug 176602 replaced the typeahead find beep with a sound, enabled/disabled by 
user_pref("accessibility.typeaheadfind.enablesound", true);
I've been thinking about how chatzilla and other apps handle sound: generally
they let you choose a sound, with beep being an option.

We could do the same thing as cz if the pref were changed to something like:
user_pref("accessibility.typeaheadfind.sound", "url");
where url is the url to the sound (defaulting to the existing notfound.wav in
the chrome jar), or "beep" to call the system beep (default on platforms where
sound doesn't work), or "" or unset to disable sound.  Then people on platforms
where sound doesn't work could still have the original behavior, at their option.

Comment 1

[sairuh (rarely reading bugmail)]

just chatted w/akkana about this --she might have a fix for this which might
resolve both bug 179817 and bug 179824.

Comment 2

[Akkana Peck]

Attachment: The general idea

Here's the general idea.  I've only tested this very briefly, and it needs the
default url added (on platforms that can use it) and the string removed from
where it is now, but it does turn on beeping on unix if I set the pref to
"beep".  Can someone try mach-o?

Comment 3

[Akkana Peck]

Taking.

Comment 4

[sairuh (rarely reading bugmail)]

bryner or sfraser, would you be able to test/review this for mach-o, please?

Comment 5

[Aaron Leventhal]

Hmm, okay. I don't plan on exposing this in the prefs -- just a checkmark
whether you want a sound or not there.

How about a value of "default" meaning the sound we've checked in.

Also, the all.js, unix.js etc. files would need to be changed.

Comment 6

[sairuh (rarely reading bugmail)]

and, of course, macprefs.js needs to be updated as well.

Comment 7

[Simon Fraser [no longer active]]

+  char *soundStr = 0;
+  prefs->CopyCharPref("accessibility.typeaheadfind.soundURL", &soundStr);
+  nsSharableCString soundURL;
+  if (soundStr)
+    soundURL.Adopt(soundStr);

Why not use nsXPIDLCString? Looks ok otherwise, though I didn't test it in a build.

Comment 8

[Akkana Peck]

Attachment: More complete pref: do everything but set the mac pref

Aaron: good idea about "default".  I've added that and set the all.js default
to that, and added the unix.js default of "beep".

I'm not sure what to do for macprefs.js, though.  Should I set it to beep to
fix the mach-o builds, thus depriving the carbon builds of the nicer sound?  I
don't see separate macho vs. carbon prefs files.  I don't suppose macho builds
pick up unix.js?  Mac people, tell me what to do here.

I didn't use nsXPIDLCString because all the CharPrefs examples I found used
char*.	I'll do a more extensive search and try to figure out how to use
nsXPIDLCString, if that's the new approved way.  I expect we'll need one more
round here anyway, for adding the right macprefs.js setting.

Comment 9

[Simon Fraser [no longer active]]

macprefs.js affects both cfm and macho builds. unix.js is not used on Mac. I
think we'd be fine if mac used "beep" everywhere; the .wav file sounds crappy
anyway.

Comment 10

[Aaron Leventhal]

The wav file sounds a lot crappier on mac than on win for some reason. It has
extra static. However, when we played the longer version of the wav file it
sounds fine -- so we could still replace it with that.

However, I'm not against using the beep on mac -- just wanted to clarify this.

Comment 11

[Aaron Leventhal]

Simon, how does this sound to you?
http://bugzilla.mozilla.org/attachment.cgi?id=104093&action=view

Comment 12

[Aaron Leventhal]

Comment on attachment 106162 [details] [diff] [review]
More complete pref: do everything but set the mac pref

Looks nice, thanks for doing this.

r=aaronl

Comment 13

[sairuh (rarely reading bugmail)]

my own preference would be that the pref for mac (which would affect both cfm
and mach-o) be the same as for unix. the system beep on mac is easily
configurable (eg, in the Sound system pref panel in OS X), so the user can make
it as pleasant (or unpleasant) a s/he prefers.

Comment 14

[Simon Fraser [no longer active]]

That attachment sounds like a hoarse dog barking.

Comment 15

[Aaron Leventhal]

> That attachment sounds like a hoarse dog barking.

Yeah, but that's better than a horse-dog.

Comment 16

[Akkana Peck]

Attachment: Same as last one, but set the mac default to "beep"

Comment 17

[Simon Fraser [no longer active]]

+  char *soundStr = 0;
+  prefs->CopyCharPref("accessibility.typeaheadfind.soundURL", &soundStr);
+  nsSharableCString soundURL;
+  if (soundStr)
+    soundURL.Adopt(soundStr);
+  typeAheadFind->mNotFoundSoundURL = soundURL;

This would go something like:
  nsXPIDLCString soundStr;
  prefs->CopyCharPref("accessibility.typeaheadfind.soundURL",
getter_Copies(soundStr));
  if (!soundStr.IsEmpty())
    typeAheadFind->mNotFoundSoundURL = (!soundStr;

+    if (mNotFoundSoundURL.Equals("default"))

Why not just put the default sound url in the default prefs, and remove this
special "default" value?

+// Beep instead of playing sound in Linux, at least until nsISound is
+// implemented (bug ) and becomes asynchronous (bug 110385):
+pref("accessibility.typeaheadfind.soundURL", "beep");

Comments in prefs files add to loading time, and provide little benefit.

+// Beep instead of playing sound on Mac, because nsSound doesn't
+// work on Mach-O and sounds crappy on carbon:
+pref("accessibility.typeaheadfind.soundURL", "beep");

It's not that nsSound itself sound crappy on carbon, but that these .wav files
sound crappy. They must have low bitrates or something. Mac's nsSound is capable
of CD-quality sound (it just uses QuickTime, and can play MP3). Nuke the
comment, anway.

(/me, who can't help thinking that it's features like this that bloat mozilla)

Comment 18

[Akkana Peck]

Comment on attachment 106176 [details] [diff] [review]
Same as last one, but set the mac default to "beep"

Sorry for the delay; I finally managed to test the patch on a mach-o build (and
got my system beep working so I could tell when it fired :-), and it does work
for me.

Seeking reviews.

Comment 19

[Aaron Leventhal]

Comment on attachment 106176 [details] [diff] [review]
Same as last one, but set the mac default to "beep"

r=aaronl

Comment 20

[Simon Fraser [no longer active]]

Can you address my points in comment 17?

Comment 21

[Akkana Peck]

Oh, sorry, Simon!  I forgot that I hadn't made a patch yet addressing your
comments yet.  New patch coming (after my build finishes and I test it).

I'm removing the comment for macprefs.js, but leaving the one in unix.js,
because unix people do look at prefs files and comments there are very helpful.
 Besides, the file is already full of comments.  If they're a performance
penalty, should we perhaps strip them when we make the jar files?  Should I file
an RFE for that?

> Why not just put the default sound url in the default prefs, and remove this
> special "default" value?

I did that in my first patch, and Aaron asked me in comment 5 to implement
"default" as I've done it in the later patch.

Comment 22

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

>should we perhaps strip them when we make the jar files?  

all.js and friends are not part of the jar files...

or were you not implying that, and just mentioning that it should happen at the
same time?

Comment 23

[Akkana Peck]

Attachment: Latest patch

Comment 24

[Akkana Peck]

Fixed.  Happy beeping.

Comment 25

[Aaron Leventhal]

Comment on attachment 107504 [details] [diff] [review]
Latest patch

r=aaronl

Comment 26

[sairuh (rarely reading bugmail)]

thanks for fixing this, akkana.

vrfy'd fixed on the following trunk builds:

2002.12.10.07 mach-o mozilla (OS X 10.2.2) - now get the system beep.

2002.12.09.08 comm on linux rh8 - now get the system beep.

2002.12.10.08 comm on win2k - now get the installed soundbyte.