Closed Bug 83056 Opened 23 years ago Closed 16 years ago

Add support for windows sounds to Menus

Categories

(Core :: XUL, enhancement)

x86
Windows XP
enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: eem12, Assigned: masayuki)

References

(Blocks 2 open bugs)

Details

(Keywords: polish, verified1.9.1)

Attachments

(1 file, 5 obsolete files)

Steps to reproduce:

1. Install a Windows desktop theme that assigns sounds to menu open events.
2. Open Navigator and navigate its menus.
3. Open Mozilla and navigate its menus.

Navigator plays menu sounds, but Mozilla does not.  I understand why, but I
still think this is a bug.
dear god.
Severity: normal → trivial
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Target Milestone: --- → Future
As silly as this is, it's not following platform conventions...  It should be
done someday.  ;)
As much as it pains me to say this, I use a theme on one of my machines that 
has menu sounds (hey, it's a Spiderman theme, gimme a break!).  This should be 
pretty easy to do and, from his comment, I'm sure Mike won't mind me taking 
this from him.
Assignee: pinkerton → dean_tessman
Status: ASSIGNED → NEW
do we need this on the Mac, too?
Preliminary patch coming, so I don't forget where I need to put the sound calls.
 I'd like to move the sound calls to somewhere central, so I don't pollute the
code with #ifdef XP_WIN followed by #ifdef XP_MAC, etc.
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla1.0
Attached patch cvs diff -u (obsolete) — Splinter Review
Thinking about this, that should probably be SND_ASYNC not SND_SYNC, and add
SND_NODEFAULT as well to suppress the default beep.
The patch looks good to me, but I'd rather there be nsILookAndFeel methods for
this.  That way people can easily implement this on OS X or BeOS (or whatever)
without caring about who uses the functionality.
Oh, whoops, I thought I mentioned my idea in my first 08-08 post.  I originally 
thought about nsILookAndFeel, and initially went down that path, but then I 
realized that the functions in there are more for retrieving setting values.  
nsISound already has a playsound() function, and it seems like a good fit to add 
a PlaySystemSound() function which would use platform-independent 
application event constants.
Hyatt mentioned something about sounds on IRC tonight, so I might as well cc: 
him.
I have been meaning for some time to hook up to skins the ability to play 
sounds via event handlers.

So you could do something like this for example:

<handler event="popupshowing" sound="system-sound:popupshowing"/>

we could have some convention of system sound names and also support .wavs etc.

<handler event="popupshowing" 
sound="chrome://global/skin/sounds/mycustomsound.wav"/>

This would all presumably be done using the nsISound interface behind the 
scenes, which IMO should be enhanced to treat certain strings as builtin system 
sounds, e.g., "system-sound:popupshowing" or some such.
Attached patch cvs diff -u mozilla/widget (obsolete) — Splinter Review
This patch adds nsISound::PlaySystemSound() on Windows.  The |strcpy
(systemSound, "\0")| is extraneous, I now realize, but you get the idea.

Hyatt, is this what you were looking for?  If so, I'll go ahead and add blank 
methods on the other platforms' nsSound.cpp.
yes.  go for it.
Since you're changing the IDL for the sound interface you need to change it on
all of the platforms or find a base implementation file so that you don't get
undefined symbols.
That's what I meant when I said I'd add blank methods on the other platforms, or
am I missing something.  I guess the safest thing to do is to add it for all
platforms in the widget directory?
We also need to determine what system sounds PlaySystemSound will recognize.  I
think a good start would be:

menushowing or popupshowing
menucommand or popupcommand
windowmaximize
windowminimize
windowrestoreup
windowrestoredown
windowopen
windowclose
defaultbeep (?)

There are compatible events in both Windows and Mac, and I'm assuming that the
Linux theme managers can handle these basic sounds.  After we get these in we
can start adding sounds for alerts, etc., which are a little different between
Windows and Mac.
Attached patch cvs diff -u mozilla/widget (obsolete) — Splinter Review
This patch adds PlayDefaultSound() on all the platforms that I could find an
nsSound.cpp for.  It also adds support for the system sounds that I mentioned
earlier on Windows.  All other platforms only have support for "defaultbeep",
which just calls nsSound::Beep().
In my last comment I meant PlaySystemSound(), not PlayDefaultSound().
Attachment #45052 - Attachment is obsolete: true
Attachment #48040 - Attachment is obsolete: true
dean, we think alike!  I just landed something very close to this (r=pavlov) 
for #64462.

craft another patch, and since I accidentally obsoleted this patch, I'll gladly 
do the review.

Attachment #48109 - Attachment is obsolete: true
Differences between this and the patch for bug 64462:

1) the 64462 version didn't add SND_NODEFAULT on Windows
2) This version uses an nsAReadableString instead of const char
3) The parameter is named differently in the .idl (aSoundAlias vs. soundAlias)
4) PlaySystemSound() on other platforms doesn't always beep, it only beeps if
aSoundAlias is "defaultbeep"
5) I hard-coded a lot of sound aliases in, instead of defaulting to what Windows
names them, to try to give some of them more xp names.  I only added "_moz_" to
"mailbeep", because I don't think we need it elsewhere.

if these are internal sounds, then prefix them with _moz_*

sfraser asked me to add that for mailbeep, on the off chance someone already had
that system sound.

if they did, then they couldn't select that sound for something else, say on
window open.

for review, you'll want pavlov or sfraser.


I don't understand why we need to prefix them.  From hyatt's example of how he
sees this working in skins:

<handler event="popupshowing" sound="system-sound:popupshowing"/>

So these would be prefixed with "system-sound:".  If in code you want to
hard-code a sound, you're going to call PlaySound() if you want to play a sound
file, and PlaySystemSound() if you want to translate the sound event to a system
sound.
Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)
Target Milestone: mozilla1.0 → mozilla1.0.1
I've done all I can on this.  --> hyatt
Assignee: dean_tessman → hyatt
Status: ASSIGNED → NEW
*** Bug 160078 has been marked as a duplicate of this bug. ***
Blocks: 162749
would this cover alerts and such as well (bug 162749)? (mac: bug 74628)
Tuukka: yes, my approach in attachment 53481 [details] [diff] [review] lays the framework for playing of
standard Windows sounds.  The theme support, as described in comment 12, would
also need to be added.  Of course, I'm sure that patch is incredibly out of date
now.
*** Bug 162749 has been marked as a duplicate of this bug. ***
*** Bug 183155 has been marked as a duplicate of this bug. ***
retargeting
Target Milestone: mozilla1.0.1 → Future
Severity: trivial → enhancement
Keywords: helpwanted
Summary: Menus don't play Windows sounds. → Add support for windows sounds to Menus
Target Milestone: Future → ---
*** Bug 240861 has been marked as a duplicate of this bug. ***
Keywords: polish
*** Bug 286725 has been marked as a duplicate of this bug. ***
What is going on with this bug?  Anything?  Or is it just sitting?  The last
useful status I see is in 2002...
(In reply to comment #38)
> What is going on with this bug?  Anything?  Or is it just sitting?  The last
> useful status I see is in 2002...

If you don't see any comments, then nothing is happening.  When bugs get started
/usually/ someone will comment on the bug to that effect.  If you personally
want something to happen, either start something yourself or hire someone to do
it, because nothing's happening now.
*** Bug 289176 has been marked as a duplicate of this bug. ***
It is not just menus that do not cause Windows sound events.  A window restore up also fails to trigger the appropriate event.

This is an accesibility issue - people may depend on sound events for feedback.
While you're at it, it'd be nice to put in the start navigation sound as described in bug #98674.
Assignee: hyatt → nobody
No longer blocks: 162749
OS: Windows NT → Windows XP
QA Contact: jrgmorrison → xptoolkit.menus
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.menus → xptoolkit.widgets
Blocks: 265037
Attached patch Patch v1.0Splinter Review
simple fix on win32.
Assignee: nobody → masayuki
Attachment #53481 - Attachment is obsolete: true
Attachment #53501 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #351791 - Flags: superreview?(roc)
Attachment #351791 - Flags: review?(neil)
Attachment #351791 - Flags: ui-review?(jboriss)
Comment on attachment 351791 [details] [diff] [review]
Patch v1.0

This patch added to support menu popup sound and menu execute sound. It's using system setting sounds. Of course, if users don't set sound to them in Control Panel, this patch doesn't play sounds.
Attachment #351791 - Flags: review?(neil) → review?(enndeakin)
Attachment #351791 - Flags: superreview?(roc) → superreview+
Attachment #351791 - Flags: ui-review?(jboriss) → ui-review+
Comment on attachment 351791 [details] [diff] [review]
Patch v1.0

may god have mercy on our souls. ui+
Comment on attachment 351791 [details] [diff] [review]
Patch v1.0

Looks OK. It would probably be better to play the popup sound when the popup actually appears rather than when the request to have it appear is made, assuming that triggering sounds during a layout is safe. But I'd go with this for now.
Attachment #351791 - Flags: review?(enndeakin) → review+
Attachment #351791 - Flags: approval1.9.1?
Comment on attachment 351791 [details] [diff] [review]
Patch v1.0

Thank you for the reviewers!

The risk is low. But the sounds when menu popup and menu execute may help some users who need the sounds for accessibility.
http://hg.mozilla.org/mozilla-central/rev/8602d55fe61f

pushed to trunk.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [c-n: baking for 1.9.1]
Target Milestone: --- → mozilla1.9.2a1
Comment on attachment 351791 [details] [diff] [review]
Patch v1.0

a191=beltzner
Attachment #351791 - Flags: approval1.9.1? → approval1.9.1+
Keywords: fixed1.9.1
Whiteboard: [c-n: baking for 1.9.1]
i object to the statement that the risk is low. you're introducing code in a critical layout path. code which will call out to system libraries and may lock for some period of time.
we call PlaySoundW with the ASYNC flag for a reason.
I'm not sure which case is locked by PlaySound. Should I add SND_NOWAIT flag??
I'm not saying the code is definitely wrong. I'm just saying that such a thing isn't truly low risk, especially given the state of sound systems on Linux. And if the implementor isn't certain about such characteristics then it's not something to say "risk is low". This is the kind of thing that I believe should be baked a bit longer (and preferably with some advertising to ensure it's tested).
I understood that timeless knows such cases. We are calling PlaySoundW with ASYNC as Stuart said. In such case, PlaySound API doesn't wait until to success to play sounds, I think (especially, at system sounds). Therefore, I said "risk is low". But I can understand timeless's objection too. But I still think the risk is low.
Verified.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090127 Shiretoko/3.1b3pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.