Last Comment Bug 604451 - Provide a way to play the "ding" system sound used to indicate typing too many characters in a text field on Windows
: Provide a way to play the "ding" system sound used to indicate typing too man...
Status: RESOLVED FIXED
: addon-compat, dev-doc-complete
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: mozilla9
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-14 12:27 PDT by :Ehsan Akhgari
Modified: 2011-10-14 13:08 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for new constant in nsISound and Win32 nsSound for max length reached (3.74 KB, patch)
2011-09-05 19:40 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch for new constant in nsISound and Win32 nsSound for max length reached. v2 (2.28 KB, patch)
2011-09-06 06:28 PDT, Brian R. Bondy [:bbondy]
tellrob: review+
roc: superreview+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2010-10-14 12:27:12 PDT
In order to fix bug 603190, we'll need a new value for nsISound::PlayEventSound for the "ding" sound played when someone types more characters than the maximum allowed value inside a text field.
Comment 1 Jan 2010-10-14 15:06:32 PDT
nsISound::beep() seems to play that sound. Tested by setting the specific sound ("Standardton Warnsignal" in my german Windows) to a distinct sound, then evaluating Components.classes["@mozilla.org/sound;1"].createInstance(Components.interfaces.nsISound).beep() in the error console and with a mspaint dialog box that has a length-limited input field. Entering too much into the mspaint dialog created the distinct sound and beep() did too, so beep() should make the correct sound for typing too many characters even if a user has custom sound settings. Additionally, it may already be cross-plattform, so using it may be the best solution.
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-09 17:19:43 PDT
No, we should use nsISound::PlayEventSound. And nsSound for Windows should call beep() if it's correct behavior.

It seems that Windows plays the sound for each discarded character. I mean, when IME commits a couple of characters, Windows plays the sound a couple of times.
Comment 3 Brian R. Bondy [:bbondy] 2011-09-05 19:40:13 PDT
Created attachment 558374 [details] [diff] [review]
Patch for new constant in nsISound and Win32 nsSound for max length reached

The default beep is the one that is used when max length is reached in an edit box in windows.  I verified by changing it to a custom sound and reaching the max length.
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-09-05 20:09:47 PDT
Comment on attachment 558374 [details] [diff] [review]
Patch for new constant in nsISound and Win32 nsSound for max length reached

> +#define NS_SYSSOUND_EDITOR_MAX_LEN  NS_LITERAL_STRING("_moz_editormaxlen")

> +  else if (aSoundAlias.Equals(NS_SYSSOUND_EDITOR_MAX_LEN))
> +    eventId = EVENT_EDITOR_MAX_LEN;

You don't need to support the new event sound by PlaySystemSound(). All callers should call PlayEventSound().
Comment 5 Brian R. Bondy [:bbondy] 2011-09-06 06:28:17 PDT
Created attachment 558458 [details] [diff] [review]
Patch for new constant in nsISound and Win32 nsSound for max length reached. v2

Removed PlaySystemSound support for the new event as per Masayuki Nakano's suggestion.
Comment 6 Brian R. Bondy [:bbondy] 2011-09-07 11:59:12 PDT
Bug 603190 for which this bug was originally created is going to go with a visual cue instead when max length is reached.  I will still push this patch though for other XUL apps such as Thunderbird that may want to support it to get platform default behaviour.
Comment 7 :Ehsan Akhgari 2011-09-07 12:01:48 PDT
(In reply to Brian R. Bondy [:bbondy] from comment #6)
> Bug 603190 for which this bug was originally created is going to go with a
> visual cue instead when max length is reached.  I will still push this patch
> though for other XUL apps such as Thunderbird that may want to support it to
> get platform default behaviour.

They're not going to get that behavior by merely having this patch in, since the editor code won't be using it... :/
Comment 8 Brian R. Bondy [:bbondy] 2011-09-07 12:03:20 PDT
I know they wouldn't without specifically implementing it themselves :)

Would it be better for me to mark this task as RESOLVED | INVALID ?
Comment 9 :Ehsan Akhgari 2011-09-07 12:16:45 PDT
No, I think you should definitely get this landed.  I just wanted to mention that it won't automatically give those applications what they want...
Comment 10 Brian R. Bondy [:bbondy] 2011-09-07 12:52:14 PDT
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=4a73b5bc2681

I selected all platforms instead of only just Windows because there is an interface which gets compiled on all platforms.
Comment 11 Brian R. Bondy [:bbondy] 2011-09-07 12:59:06 PDT
Comment on attachment 558458 [details] [diff] [review]
Patch for new constant in nsISound and Win32 nsSound for max length reached. v2

Super review requested for the minor interface change.
Comment 12 :Ehsan Akhgari 2011-09-07 16:09:13 PDT
Why does this affect add-on compatibility?
Comment 13 Brian R. Bondy [:bbondy] 2011-09-07 16:28:08 PDT
I'm not sure which action items come from marking that keyword, but I was told on IRC to put it and request super review for any and all interface changes.  Pls advise if the keyword is only for modifications and removals.
Comment 14 :Ehsan Akhgari 2011-09-07 16:57:52 PDT
The addon-compat keyword is used for changes which can potentially break existing add-ons.  On a closer look, this patch changes the IID of nsISound, so it's possible for it to break binary add-ons which use that interface.  Therefore, you can ignore my comment, I was wrong!  :-)
Comment 15 Brian R. Bondy [:bbondy] 2011-09-07 17:02:22 PDT
That brings me to another question... I was told to always update the IID of every interface I change. Is that right? If so that implies addon-compat for all interface changes :)
Comment 16 :Ehsan Akhgari 2011-09-07 17:47:03 PDT
(In reply to Brian R. Bondy [:bbondy] from comment #15)
> That brings me to another question... I was told to always update the IID of
> every interface I change. Is that right? If so that implies addon-compat for
> all interface changes :)

Yes.  However, adding a constant doesn't change the vtable layout, so maybe we shouldn't change the IID in this case?  Benjamin would know best.
Comment 17 Brian R. Bondy [:bbondy] 2011-09-08 08:18:57 PDT
Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4a5eb5183ed9
Comment 18 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-09-08 10:22:25 PDT
It is not necessary to rev the IID for just a new constant, but I don't think it would hurt anyone.
Comment 20 Eric Shepherd [:sheppy] 2011-10-14 13:08:51 PDT
This documentation has already been updated.

Note You need to log in before you can comment on or make changes to this bug.