Closed
Bug 176602
Opened 22 years ago
Closed 22 years ago
Make typeaheadfind beep less annoying
Categories
(SeaMonkey :: Find In Page, defect, P2)
SeaMonkey
Find In Page
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.3alpha
People
(Reporter: aaronlev, Assigned: aaronlev)
References
Details
(Keywords: polish, Whiteboard: works in windows, needs testing/help in Linux and Mac)
Attachments
(2 files, 14 obsolete files)
(deleted),
audio/wav
|
Details | |
(deleted),
patch
|
aaronlev
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
This is separate from the bug to add a pref for the beep -- we'll do that as well.
What I want to do here is make the beep only occur if at least 1 character has
been successfully found, or if / or ' has been used to start the find. That way
it won't happen when you just accidentally type a stray character in the browser.
I also want to use a nicer sound for the beep. I found some public domain sounds
using google/lycos media search.
Assignee | ||
Comment 1•22 years ago
|
||
Assignee | ||
Comment 2•22 years ago
|
||
Assignee | ||
Comment 3•22 years ago
|
||
Assignee | ||
Comment 4•22 years ago
|
||
Assignee | ||
Comment 5•22 years ago
|
||
Assignee | ||
Comment 6•22 years ago
|
||
Assignee | ||
Comment 7•22 years ago
|
||
Well, that's a start -- any of those okay?
There are more freeware sounds here:
http://www.flashkit.com/soundfx/Interfaces/
Comment 8•22 years ago
|
||
Be aware that mozilla's sound doesn't necessarily work on all platforms, or has
problems which mean that it should not be used by default. For instance, on
unix, due to bug 110385, nsSound blocks until the sound is finished playing,
which can cause very annoying delays (especially if you're trying to type).
Assignee | ||
Comment 9•22 years ago
|
||
Let's choose a short sound then? A little boop?
I think, once a bad key is pressed it's not terrible if there's a pause, because
the rest won't be found anyway. We can make it work.
Assignee | ||
Comment 10•22 years ago
|
||
Attachment #104054 -
Attachment is obsolete: true
Attachment #104055 -
Attachment is obsolete: true
Attachment #104056 -
Attachment is obsolete: true
Attachment #104057 -
Attachment is obsolete: true
Attachment #104058 -
Attachment is obsolete: true
Attachment #104060 -
Attachment is obsolete: true
Comment 11•22 years ago
|
||
I'd prefer something about a quarter of that duration. The pitch is good.
Assignee | ||
Comment 12•22 years ago
|
||
Does anyone have a sound editing tool that can speed it up without changing the
pitch?
Assignee | ||
Comment 13•22 years ago
|
||
I found a pretty good Windows sound editor called Acoustica here:
http://download.com.com/3001-2170-9055050.html
Comment 14•22 years ago
|
||
cool. :)
Comment 15•22 years ago
|
||
Sounds good =)
Assignee | ||
Comment 16•22 years ago
|
||
On Windows, this is only working when I use the debugger and trace into and
step over this code:
BOOL PlaySound(const char *aSoundFile,HMODULE aModule,DWORD aOptions) {
return (mPlay) ? mPlay(aSoundFile, aModule, aOptions) : FALSE;
}
If I just let it run it normally doesn't work.
Would someone be willing to test this on another platform? After applying this
patch, you have to copy the sound file into
mozilla/extensions/typeaheadfind/resources/content/notfound.wav
Attachment #104085 -
Attachment is obsolete: true
Comment 17•22 years ago
|
||
The current patch doesn't work in linux either (haven't tried stepping in the
debugger). I'm happy to test future patches on linux. I like the choice of sound.
If we do this, I want to argue strongly for exposing the pref. Based on my
experience with chatzilla, the problem isn't the length of the sound; the
problem is the time nsSound takes to initiate and finish the sound operation
(moz was locked up for much longer than the length of the sound itself, often
several seconds -- it was very noticable and frustrating). Also, it's very
common for sound not to work -- especially on linux and other Unix machines, but
I know a lot of Windows users who don't have working sound either. Also, what
happens if I do typeahead find while some other app has the sound system locked?
When I first downloaded the sound sample and tried to play it, play hung and I
had to kill it, because realplayer is currently streaming a radio program, and
apparently realplayer doesn't play nice about sharing the sound device. I've
seen bugs that say the flash plugin does the same thing. I'd hate to see my
whole browser hang for that reason. This sound behavior must be optional,
unless we fix nsSound to be asynchronous.
Assignee | ||
Comment 18•22 years ago
|
||
I agree, it should be a pref -- we have a bug for typeahead prefs open somewhere
and I plan to implement it there.
Comment 19•22 years ago
|
||
All sounds should be optional, really. There are some circumstances where
beeping just isn't wanted.
Comment 20•22 years ago
|
||
> it's not terrible if there's a pause
Um... yes it is. The basic issue is that the sound playing is synchronous.
Happening on the UI thread. while that sound is playing there is no painting,
no reflow, nothing. That may not be so bad if the sound actually plays. But
from a brief look at the gtk impl, it just punts the playing over to esd. So
either you have esd running and then all is happy, or you don't have esd running
and we don't start it (then there is just no sound), or we try to start it and
then we get contention for the audio device if something else is using it (read:
same as the "Flash hangs mozilla" bug).
Please figure out which of the two possibilities actually takes place when esd
is not running...
Assignee | ||
Comment 21•22 years ago
|
||
Well it looks like there is no traction on bug 110385. I'm tempted to make the
typeaheadfind bloop sound off by default on Unix/Linux, at least until that's fixed.
Depends on: 110385
Assignee | ||
Comment 22•22 years ago
|
||
I can't get sound to play off a chrome URL, unless I change nsSound to play
sounds from memory synchronously :P
Assignee | ||
Comment 23•22 years ago
|
||
Well someone else try my patch in Windows to see if I'm crazy? It only works if
I change this line in widget/src/windows/nsSound.cpp
- theMM.PlaySound(mLastSound, nsnull, SND_MEMORY | SND_NODEFAULT | SND_SYNC);
+ theMM.PlaySound(mLastSound, nsnull, SND_MEMORY | SND_NODEFAULT | SND_ASYNC);
Is that true for anyone else?
Assignee | ||
Comment 24•22 years ago
|
||
Attachment #104160 -
Attachment is obsolete: true
Assignee | ||
Comment 25•22 years ago
|
||
Attachment #104093 -
Attachment is obsolete: true
Assignee | ||
Comment 26•22 years ago
|
||
Need testing on other platforms. Can someone help me determine what happens on
Linux when esd is not running?
Seeking r=, sr=
Attachment #104312 -
Attachment is obsolete: true
Assignee | ||
Comment 27•22 years ago
|
||
Need testing on other platforms. Can someone help me determine what happens on
Linux when esd is not running?
Seeking r=, sr=
Assignee | ||
Updated•22 years ago
|
Attachment #104333 -
Attachment is obsolete: true
Assignee | ||
Comment 28•22 years ago
|
||
Attachment #104309 -
Attachment is obsolete: true
Comment 29•22 years ago
|
||
Comment on attachment 104334 [details] [diff] [review]
Impl our own sound for typeaheadfind beep, add pref for beep, #ifdef DEBUG some useless code in widget/src/windows/nsSound.cpp, and decrease typeaheadfind timeout a little
>+ // After each keystroke, destrouy sound object to free up memory
^^^^^^^^
You've got a typo there.
Comment 30•22 years ago
|
||
Tested on Redhat 8.0, there is no sound. I traced into the nsSound.cpp under
/widget/src/gtk, and am sure that the following code had been excuted:
nsSound::OnStreamComplete()
{ ...
fd = (*EsdPlayStreamFallback)(mask, rate, NULL, "mozillaSound");
...
write(fd, string, stringLen);
close(fd);
...
}
but still can't hear anything. Is there any other code used nsISound::Play
method? So that I can determine this is a nsSound bug or TypeAheadFind bug.
Assignee | ||
Comment 31•22 years ago
|
||
Mailnews uses nsISound::PlaySystemSound(), but that's the only other thing I can
find that uses nsISound to play a custom wav file when new mail arrives. You can
set that up in mailnews prefs.
The code to play the wav file in mailnews is in
base/prefs/resources/content/pref-notifications.js.
Comment 32•22 years ago
|
||
aaron, out of curiosity, are you able to test this on a mac? mail notification
sounds, iirc, were/are an issue on mac os's as well. or, are you planning for
this to be a win32/unix-only fix?
Comment 33•22 years ago
|
||
yes, there is sound when new mail arrives, but it's from my build-in speaker,
not the sound card. I think it would be from nsISound::Beep().
the pref-notifications.js seems like some pref setting & sound preview stuff.
Assignee | ||
Comment 34•22 years ago
|
||
Kyle, there's a setting in Windows that allows you to specify your own wav sound
file for the new message sound. Do you have that in Linux?
Comment 35•22 years ago
|
||
> Do you have that in Linux?
Yes.
Assignee | ||
Updated•22 years ago
|
Whiteboard: works in windows, needs testing/help in Linux
Comment 36•22 years ago
|
||
Now it works on my RedHat 8.0 after installed a patch (arts-2.2.2-1.i386.rpm),
even if there is no esd.
Assignee | ||
Comment 37•22 years ago
|
||
It actually works? That's great! Maybe someone can test it on a mac now.
Comment 38•22 years ago
|
||
Kyle, please perform the following test:
1) Have no ESD running
2) Have something using /dev/dsp (an mp3 player should do fine)
3) Make sure to use a soundcard without hardware mixing support
4) Try to use nsISound
What happens? Is there just no sound? Or does Mozilla hang?
Updated•22 years ago
|
Attachment #104335 -
Attachment is patch: false
Attachment #104335 -
Attachment mime type: text/plain → audio/wav
Assignee | ||
Comment 39•22 years ago
|
||
I think we should keep sound pref'd off on Unix/Linux until we get bug 110385
(asynchronous sound) fixed for that platform, and until we make sure there are
no problems with system hangs with sound in Linux.
Seeking r=/sr=
Attachment #104334 -
Attachment is obsolete: true
Comment 40•22 years ago
|
||
It doesn't make any sound on Redhat 7.3. nsSound::OnStreamComplete does get
called, but apparently the code after that doesn't work (I haven't had a chance
to chase it any farther than that). Even aside from the asynchrony issue,
nsSound apparently just plain doesn't work on most linux systems, even those
where sound is configured.
Comment 41•22 years ago
|
||
Filed bug 179138 with more details on the problems with nsSound::Play on linux.
I'm fine with the idea of checking this in preffed off for linux until we get
the linux sound problems straightened out, if it works on other platforms.
Updated•22 years ago
|
Whiteboard: works in windows, needs testing/help in Linux → works in windows, needs testing/help in Linux and Mac
Comment 42•22 years ago
|
||
sorry, I was using a gtk2 build. the gtk build still doesn't work (no sound). So
the clue is what's the different between widget\src\gtk\nsSound.cpp and
widget\src\gtk2\nsSound.cpp.
Assignee | ||
Comment 43•22 years ago
|
||
Thanks Kyle. I still think this is worthy of r= now, and we can work out gtk
nsSound problems later (bug 179138).
Seeking r=kyle.
Comment 44•22 years ago
|
||
Comment on attachment 105610 [details] [diff] [review]
Same patch as before, but prefs off sound on Unix
r=kyle with one doubt: does wav file work on all platforms? Seems there is no
other wav files in mozilla except the calendar module.
bz, I'll do your test on Monday.
Attachment #105610 -
Flags: review+
Comment 45•22 years ago
|
||
Comment on attachment 105610 [details] [diff] [review]
Same patch as before, but prefs off sound on Unix
+ mSoundInterface = do_CreateInstance("@mozilla.org/sound;1");
+ mSoundInterface->PlaySystemSound("");
How about a null-check?
+ PRPackedBool mIsSoundInitialized;
What's the point of making this packed?
Attachment #105610 -
Flags: needs-work+
Assignee | ||
Comment 47•22 years ago
|
||
Comment on attachment 105696 [details] [diff] [review]
Same thing, with Boris' tweaks
Carrying forth Kyle's r=
(How does one do that in the new Bugzilla, without making it look like you've
r='d your own bug?)
Attachment #105696 -
Flags: review+
Updated•22 years ago
|
Attachment #105696 -
Flags: superreview+
Assignee | ||
Comment 48•22 years ago
|
||
Fixed -- however the sound is off by default in Linux.
You have to put this in your .js file to get sound in Linux:
user_pref("accessibility.typeaheadfind.enablesound", true);
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 49•22 years ago
|
||
bz, here is my test result on RH8 (I used a gtk2 build, because the gtk build
can't make any sound)
> 1) Have no ESD running
can make sound. if there is no esd process, mozilla will start it.
> 2) Have something using /dev/dsp (an mp3 player should do fine)
no sound if I'm using xmms to play a mp3 file. when I closed xmms, sound is back.
> 3) Make sure to use a soundcard without hardware mixing support
I don't know how to make sure that.
> 4) Try to use nsISound
I applied this patch, turned on the pref in unix.js and typed some wrong char to
trigger nsISound.
In any cases, mozilla didn't hang/crash.
Comment 50•22 years ago
|
||
I forgot to ask (or check): with sound disabled, as in linux, does that mean it
will beep as before, or will it just be silent? Is there a way to select
between those two behaviors?
Assignee | ||
Comment 51•22 years ago
|
||
With sound disabled, it won't beep. There's no way to select the old beep behavior.
Comment 52•22 years ago
|
||
is the sound also disabled by default on Mac? when i used today's
(2002.11.12.07) mach-o build (on OS 10.2.2), no sounds are played when type
ahead find doesn't find a match.
Comment 53•22 years ago
|
||
Filed RFE bug 179784 requesting that the pref point to a sound url (so it could
be changed), or "beep" to use the system beep, or null to disable sound. Sorry,
I should have thought of suggesting that earlier since it would have been a
small change to what Aaron already had here. It's no big deal, though, a little
project for a rainy afternoon when one of us is bored. :-)
Assignee | ||
Comment 54•22 years ago
|
||
No, it means nsSound::Play() isn't working on Mac either.
Comment 55•22 years ago
|
||
nsSound::Play works just fine on Mac. For me, I get beeps the first few times
TAF fails, but it stops beeping aftera a while.
Comment 56•22 years ago
|
||
hrm, how odd --i never got any sound at all with TAF on mach-o. do i need to
blast the volume or something? ;)
Comment 57•22 years ago
|
||
oh, it's a mach-o issue --i've spun off bug 179824 for that.
with a cfm build (today's commercial on 10.2.2), sound does work, albeit with a
rather grundgey sound. ;)
also vrfy'ing as fixed on win2k.
Status: RESOLVED → VERIFIED
Comment 58•22 years ago
|
||
oh, and filed bug 179817 to fix sounds for type ahead find on linux.
Comment 59•22 years ago
|
||
looks like bug 179824 on mach-o is a regression caused by the fix to this bug.
Assignee | ||
Comment 60•22 years ago
|
||
Are people happy with the new typeaheadfind sound? We're trying to minimize the
number of prefs exposed, because there are too many already. I haven't heard any
complaints in a while, so I'm thinking we might be okay with the current behavior.
Comment 61•22 years ago
|
||
>Are people happy with the new typeaheadfind sound?
Well now that you ask it, not really, it just sounds strange. I want the beep back.
Comment 62•22 years ago
|
||
I thought something was broken when this noise first showed up :) I turn it off
completely - with every new build!
Comment 63•22 years ago
|
||
If you look at bug 168574, you will see that gtk-1.x has a bug which means you
can't switch off the standard beep using the xset command on unix (or the gnome
and kde user interfaces for it). This may be an issue...
Comment 64•22 years ago
|
||
Just to drop in my $0.02 -- everything works fine on WinME, and I think the
choice of sound is good. It isn't incredibly loud, and it does the job. Rather
than the windows "ding", or the system beep (worse), the current bloop provides
the needed feedback without too much of a distraction.
Assignee | ||
Comment 65•22 years ago
|
||
*** Bug 168574 has been marked as a duplicate of this bug. ***
Updated•16 years ago
|
Product: Core → SeaMonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•