Closed Bug 947776 Opened 10 years ago Closed 8 years ago

Notifications in Thunderbird plays two sounds

Categories

(Thunderbird :: OS Integration, defect)

All
macOS
defect
Not set
normal

Tracking

(thunderbird52? affected)

RESOLVED FIXED
Thunderbird 52.0
Tracking Status
thunderbird52 ? affected

People

(Reporter: soeren.hentzschel, Assigned: javirid)

References

(Depends on 2 open bugs)

Details

Attachments

(3 files, 4 obsolete files)

After the landing of bug 852648 there are two sounds at once when I receive a new email.

OS X version: 10.9
Blocks: 852648
See bug 981769.

If Notification Center integration is enabled on ALL OS X supported vdersions, then we could remove those settings from Preferences dialog in TB and then this bug would be needed to be moved to Preferences module.

In other words:

When was Notification Center added to Mac OS X? I think it was on 10.8, but I am unsure as I have been a Mac user since just a year now.

Is that the oldest version Thunderbird is supposed to run? From what I see on TBPL, 10.7 is also supported by us.

Confirming because this bug is about double playing a sound.
Status: UNCONFIRMED → NEW
Ever confirmed: true
I'm on Thunderbird 31.3.0 on a MacBookPro running OSX 10.9.5. I set a custom sound for Thunderbird, and when mail came in I got both it and the default sound. So I went to System Preferences -> Notifications -> Thunderbird and deselected "Play sound for notifications". That stopped the default sound and let my selected sound play for a while, but then after a while I got no sounds at all. (I guess at some point Thunderbird checked the Notification Center setting?)

See https://support.mozilla.org/en-US/questions/1038142 for details.

Note that Notification Center doesn't allow you to customize the sound.
Hello. Just wanted to confirm this issue. I have same 31.3.0 Thunderbird, Mac OS X 10.9.5 and the same problem. Also this problem started about 2 months ago, i.e. in previous version(s).

For me there is no sound at all if I turn off notification sounds from OS X settings for Thunderbird. But if I leave it - 2 sounds are played.

Please don't remove custom sound possibility since It's very-very useful to have custom sound for mails. OS X notification settings do not allow to customize it, it'e either of or off there.

Thank you very much for helping solving this issue. Thunderbird is the best!
When a new mail arrives, as soon as bug 1106815 fix lands, we will always show an alert box (a banner one, more precisely speaking). That will register Thunderbird (and SeaMonkey) on the Notification Center settings on OS X.

When we show an alert, OS X will play the sound we ask to be played along the notification.

Currently, we are using toolkit to deal with notifications. That implies that we can only play "default sound" defined by the system. Firefox doesn't allow for customize sound for notifications, so they are happy the way it is working right now.

As toolkit does not allow to play a custom sound, /mailnews plays its own, by doing it by itself.

When user sets preference to not play any sound, that is what happens.

So, I see two possible fixes.

A. Ask toolkit developers to add a new method which accepts a sound file, passed as an URL. Then we could simplify our UI by just letting user to select which sound to be played along the notification. Again, we *must* bypass sound settings and *always* play a sound. OS X will play it only if that is the setting on Notification Center settings.

B. The "patch but not fix" way. Add a new label to the UI -the one which would be added on A solution- directing user to Notification Center settings. Remove the "Play a sound" checkbox, and bypass always that preference. Focus code changes in 

nsresult nsStatusBarBiffManager::PlayBiffSound(const char *aPrefBranch)

method.

Fix B is much less desirable, as it goes exactly in the oposite direction we are trying to go: let user setup everything on the Notification Center settings.

All of this would be easier if we removed the custom sound feature but, although it is not used by myself on my computer, it is one of those features that our users are appreciating, as seen in this bug report.
This screenshot shows how the General pane would look with the patch I am testing.

It removes the "Play default sound" because now we are playing it *always*, as bug 1106815 has landed in comm-central.

User has now only two options: let OS X play default sound or disable it on Notification Center and set a custom sound to be played.

As before, user could also disable or enable sound, but only if he wanted to play a custom one.

Maybe you could think that these changes are not needed, as we could do whatever we wanted with those settings. However, I think that leaving the radio-buttons may confuse user, because he could be setting it to play default sound in TB but disabling it on Notification Center, or viceversa.

With this UI change I would like to clarify to the user that this is not the place to set these preferences.

My previous comment said something about adding a new label which directed user to Notification Center for disabling sound. I have changed a little bit my mind and now I think that that label is not needed. Mmmm... Or maybe replacing current text "Alerts can be disabled (...)" with "Default sound and alerts can be disabled (...)".

Thoughts, Blake? Thank you.
Attachment #8789393 - Flags: ui-review?(bwinton)
Comment on attachment 8789393 [details]
New UI for alerts sound options on OS X

Redirecting to Paenglab, who's way more involved in the project than I am these days…  :)
Attachment #8789393 - Flags: ui-review?(bwinton) → ui-review?(richard.marti)
Does setting "Play the following..." automatically disable the system sound, or has the user to disable it?

(In reply to Javi Rueda from comment #5)
> Created attachment 8789393 [details]
> New UI for alerts sound options on OS X
> 
> My previous comment said something about adding a new label which directed
> user to Notification Center for disabling sound. I have changed a little bit
> my mind and now I think that that label is not needed. Mmmm... Or maybe
> replacing current text "Alerts can be disabled (...)" with "Default sound
> and alerts can be disabled (...)".

Yes, such would be needed. maybe a "Alerts can be configured (...)" when the answer to my question above is yes.
(In reply to Richard Marti (:Paenglab) from comment #7)
> Does setting "Play the following..." automatically disable the system sound,
> or has the user to disable it?
> 

No. System sound cannot be disabled by any app on OS X. User has to do it by going to Notification panel in System Settings. This is the reason why we are fixing this.

> (In reply to Javi Rueda from comment #5)
> > Created attachment 8789393 [details]
> > New UI for alerts sound options on OS X
> > 
> > My previous comment said something about adding a new label which directed
> > user to Notification Center for disabling sound. I have changed a little bit
> > my mind and now I think that that label is not needed. Mmmm... Or maybe
> > replacing current text "Alerts can be disabled (...)" with "Default sound
> > and alerts can be disabled (...)".
> 
> Yes, such would be needed. maybe a "Alerts can be configured (...)" when the
> answer to my question above is yes.

Answer was "No". However, if you think that change is needed, I will post a new screenshot with the patch (I was waiting until there was a ui-r+ before attaching it). Which phrase do you prefer, my suggestion or yours?
Oh, as I didn't attach the patch you are not able to see how it would fix the problem. The fix should just stop playing any sound if not a custom one was selected, as OS X WILL play default one if user enabled it on Notification panel of System Settings.

If user likes to play a custom one instead, he should disable the setting on Notification panel of System Settings. In that case, Thunderbird WILL try to play the custom one.
Comment on attachment 8789393 [details]
New UI for alerts sound options on OS X

(In reply to Javi Rueda from comment #8)
> (In reply to Richard Marti (:Paenglab) from comment #7)
> > Does setting "Play the following..." automatically disable the system sound,
> > or has the user to disable it?
> > 
> 
> No. System sound cannot be disabled by any app on OS X. User has to do it by
> going to Notification panel in System Settings. This is the reason why we
> are fixing this.
> 
> > (In reply to Javi Rueda from comment #5)
> > > my mind and now I think that that label is not needed. Mmmm... Or maybe
> > > replacing current text "Alerts can be disabled (...)" with "Default sound
> > > and alerts can be disabled (...)".
> > 
> > Yes, such would be needed. maybe a "Alerts can be configured (...)" when the
> > answer to my question above is yes.
> 
> Answer was "No". However, if you think that change is needed, I will post a
> new screenshot with the patch (I was waiting until there was a ui-r+ before
> attaching it). Which phrase do you prefer, my suggestion or yours?

Then the "Default sound and alerts can be disabled (...)" would be better.
Attachment #8789393 - Flags: ui-review?(richard.marti) → ui-review+
Attached patch doble.patch (obsolete) — Splinter Review
:aleth, could you please take a look at the general.js/general.xul changes on this patch? Thank you.

Magnus, could you please take a look on nsStatusBarBiff.cpp changes? Thank you.

On OS X, system will play a default sound when an alert is shown. There is no way to change this setting from any app, user must do it. So we could just let Alert service show the alert but, on OS X, don't play any sound. That is the fix to the "notification sound played twice".

This patch also includes the suggested label from ui-r+ by :Paenglab
Attachment #8790106 - Flags: review?(mkmelin+mozilla)
Attachment #8790106 - Flags: review?(aleth)
Comment on attachment 8790106 [details] [diff] [review]
doble.patch

Review of attachment 8790106 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/preferences/general.js
@@ +111,5 @@
>      var soundLocation;
>      soundLocation = document.getElementById('soundType').value == 1 ?
>                      document.getElementById('soundUrlLocation').value : "";
>  
> +    if (!soundLocation.includes("file://"))

Please add a comment here explaining when each branch of the if clause applies (eg. "The user set a custom sound...")

@@ +159,5 @@
>      // update the sound type radio buttons based on the state of the play sound checkbox
>      var soundsDisabled = !document.getElementById('newMailNotification').checked;
> +    var soundTypeEl;
> +
> +    if (!isOSX) {

This also needs a comment explaining the reasoning, for future reference.

@@ +169,5 @@
> +
> +    document.getElementById('browseForSound').disabled =
> +      soundsDisabled || (!isOSX && soundTypeEl.value != 1);
> +    document.getElementById('playSound').disabled =
> +      soundsDisabled || (!soundUrlLocation && ((!isOSX && soundTypeEl.value != 0) || (isOSX && !soundsDisabled)));

It would be more readable to include the !OSX version of these lines inside the if (!isOSX) to avoid the nested booleans.
Attachment #8790106 - Flags: review?(aleth) → feedback+
(In reply to Javi Rueda from comment #8)
> (In reply to Richard Marti (:Paenglab) from comment #7)
> > Does setting "Play the following..." automatically disable the system sound,
> > or has the user to disable it?
> > 
> 
> No. System sound cannot be disabled by any app on OS X. User has to do it by
> going to Notification panel in System Settings. This is the reason why we
> are fixing this.

That is incorrect. You can tell OS X not to play a sound by setting the `soundName` property to nil.

https://developer.apple.com/reference/foundation/nsusernotification/1412612-soundname?language=objc

(In reply to Javi Rueda from comment #11)
> There is no way to change this setting from any app, user must do it.

This is also incorrect, see above.
(In reply to Matthew N. [:MattN] from comment #13)
> (In reply to Javi Rueda from comment #11)
> > There is no way to change this setting from any app, user must do it.
> 
> This is also incorrect, see above.

More details at https://stackoverflow.com/a/17493822
(In reply to Matthew N. [:MattN] from comment #13)
> (In reply to Javi Rueda from comment #8)
> > (In reply to Richard Marti (:Paenglab) from comment #7)
> > > Does setting "Play the following..." automatically disable the system sound,
> > > or has the user to disable it?
> > > 
> > 
> > No. System sound cannot be disabled by any app on OS X. User has to do it by
> > going to Notification panel in System Settings. This is the reason why we
> > are fixing this.
> 
> That is incorrect. You can tell OS X not to play a sound by setting the
> `soundName` property to nil.
> 
> https://developer.apple.com/reference/foundation/nsusernotification/1412612-
> soundname?language=objc
> 
> (In reply to Javi Rueda from comment #11)
> > There is no way to change this setting from any app, user must do it.
> 
> This is also incorrect, see above.

Thank you for this info, Matthew. Then we can leave the actual UI as it is.
Thank you very much for your information, :MattN. As I saw that in order to change that, code had to be modified on the backend, as TB is not using its own Alerts service (Toolkit is providing one for us), I tried to report it and saw this bug 1105226.

That was, from the begining the root of the problem. My patch provides a fix while they do something about that.

I believe that Thunderbird, as any other application, should integrate as much as possible with the overlaying operating system. In our case, OS X provides a way to allow user to set up if a sound should be played. If we are providing also to the user the option to set it up in our own UI, then we are duplicating somdething that it is provided by the operating system by free.

Previously we changed the way we are showing alerts when a new mail arrives. Following Best Practices, we are now showing them always and let user set it up on the system-provided interface. A problem less for us! With this fix I would like to do the same for the sound component. However, as users would like to still be able to customize which sound should be played, it is not so easy.

UI currently shown can be confusing for users, as they could also know that setting from Notification pane.

With the UI part of this patch, the problem of the duplicated interface is fixed. And when bug 1105226 lands, if it will any day, no changes would be needed from the front-end side.
I think you actually want bug 1105225 since OS X notifications aren't XUL on 10.9+.

It's up to the TB team if you want a short-term fix like this instead of fixing bug 1105225 (to at least support no sound), I was just pointing out inaccuracies in your statements so an informed decision could be made.
Depends on: 1105225
It is fine. Sorry about my confusion when finding the root problem.

Now, there it is my patch. That's it! It "patches" the problem, doesn't fix the root problem. The root bug, now that :MattN pointed in the right direction, was reported in 2014 and there has not have any activity.

When that bug become fixed, I don't know, maybe is an easy one and we all are volunteers here, there is still the need to avoid the duplication of the UI (ours and system provided one).

I am not asking to hurry up and just patch it. I am providing a fix which, once bug 1105225 lands, let developer improve code without having to annoy user with another change, which, again, will be needed if we wanted to integrate in a better way with the operating system.
(In reply to Javi Rueda from comment #18)
> I am not asking to hurry up and just patch it. I am providing a fix which,
> once bug 1105225 lands, let developer improve code without having to annoy
> user with another change, which, again, will be needed if we wanted to
> integrate in a better way with the operating system.

Javi, have you looked at how hard bug 1105225 would be to fix?
Comment on attachment 8790106 [details] [diff] [review]
doble.patch

Review of attachment 8790106 [details] [diff] [review]:
-----------------------------------------------------------------

Could you fix the comments from aleth, and then ask him to review it :‑P
Attachment #8790106 - Flags: review?(mkmelin+mozilla)
(In reply to aleth [:aleth] from comment #19)
> (In reply to Javi Rueda from comment #18)
> > I am not asking to hurry up and just patch it. I am providing a fix which,
> > once bug 1105225 lands, let developer improve code without having to annoy
> > user with another change, which, again, will be needed if we wanted to
> > integrate in a better way with the operating system.
> 
> Javi, have you looked at how hard bug 1105225 would be to fix?

No, I haven't.
Attached patch doble.patch (obsolete) — Splinter Review
This patch implements the UI which was ui-r+ previously and also addresses comments by :aleth.

Not asking a review as a decission has not been taken on which way should we fix the double sound problem: asking Mozilla to provide us (and everyone) a way to customize the sound which is played along a notification or what this patch is trying to do, "patch it" as they (Mozilla) fixes it.
Attachment #8790106 - Attachment is obsolete: true
(In reply to Javi Rueda from comment #22)
> Not asking a review as a decission has not been taken on which way should we
> fix the double sound problem: asking Mozilla to provide us (and everyone) a
> way to customize the sound which is played along a notification or what this
> patch is trying to do, "patch it" as they (Mozilla) fixes it.

There's probably no good way to move it up the priority list, apart from fixing it yourself ;)

As the linked bugs refer to DOM Notifications and the TB code currently does not use those (see [1]), it looks like you could in principle get away without the DOM part, doing the following: 
* add a soundfile argument to nsIAlertsService::showAlertNotification in [2]
* implement it for OSX using comment 13 and 14 here [3]
* decide what to do with the argument on other OS (implement or leave as a todo)
This should be done in a separate bugzilla bug of course. You could ask MattN if such an approach would be acceptable.

You can still request review on this patch as a temporary workaround if you like.

[1] https://dxr.mozilla.org/comm-central/source/mailnews/base/src/nsMessengerOSXIntegration.mm#392
[2] http://searchfox.org/mozilla-central/source/toolkit/components/alerts/nsIAlertsService.idl#185
[3] http://searchfox.org/mozilla-central/source/widget/cocoa/OSXNotificationCenter.mm#235
(In reply to aleth [:aleth] from comment #23)
> As the linked bugs refer to DOM Notifications and the TB code currently does
> not use those (see [1]), it looks like you could in principle get away
> without the DOM part, doing the following: 
> * add a soundfile argument to nsIAlertsService::showAlertNotification in [2]
> * implement it for OSX using comment 13 and 14 here [3]
> * decide what to do with the argument on other OS (implement or leave as a
> todo)
> This should be done in a separate bugzilla bug of course. You could ask
> MattN if such an approach would be acceptable.

Actually, it looks like there's already an old WIP that overlaps with this in bug 1105222. Take a look!
Assignee: nobody → leofigueres
What's the status of this bug? Javi, are you investigating comment 23/24 and/or should we review and land the workaround in this patch for the next ESR?
Flags: needinfo?(leofigueres)
I am not actively investigating it right now, although I have in my "to do list" a task to try to fix the root problem. It could be before the next merge in November, so maybe it is not needed to land current patch yet.

I think there has not been many people asking this to be fixed and previous bugs added a description directing people to System PReferences which could make others to disable sound there.

If there is going to be another merge before November, maybe landing attachment 8791948 [details] [diff] [review] patch could improve user experience, as the UI would be changed anyway when fixing the root problem.
Flags: needinfo?(leofigueres)
Comment on attachment 8791948 [details] [diff] [review]
doble.patch

Don't know exactly when next merge is going to be done, so I am asking :aleth if he could review this patch in case merge is going to occur sooner than I expected.

As I said in my previous comment, I am not currently investigating the way to fix the root problem, although I have it in my schedule. If module owner thinks landing this patch would help improving the quality of Thunderbird -which I really think, but I am not the owner- then maybe landing this workaround would be fine for the moment.

Could you take a look into this patch, :aleth? Thank you.
Attachment #8791948 - Flags: review?(aleth)
Comment on attachment 8791948 [details] [diff] [review]
doble.patch

Review of attachment 8791948 [details] [diff] [review]:
-----------------------------------------------------------------

This workaround looks OK. Requesting additional review from Paenglab to check the UI and string changes.

::: mail/components/preferences/general.js
@@ +117,1 @@
>          sound.playEventSound(Components.interfaces.nsISound.EVENT_NEW_MAIL_RECEIVED);

Nit: Now there are two lines in the if clause, you should use brackets {...}, even if one of the lines is a comment.

@@ +120,1 @@
>        sound.play(Services.io.newURI(soundLocation, null, null));

Same here

@@ +161,5 @@
>      var soundUrlLocation = document.getElementById('soundUrlLocation').value;
> +
> +    // On Mac OS X we only allow the user to play a custom sound or let the
> +    // system play default one. As "soundTypeEl" is non-existant on OS X,
> +    // we are disabling parts of the UI only if we are not runing on OS X.

How about
// The UI is different on OS X as the user can only choose between letting the system play a default sound or setting a custom one. Therefore, "soundTypeEl" does not exist on OS X.

::: mail/locales/en-US/chrome/messenger/preferences/general.dtd
@@ +17,5 @@
>  <!ENTITY newMessagesArrive.label          "When new messages arrive:">
>  <!ENTITY playSound.label                  "Play a sound">
> +<!ENTITY playSoundMac.label               "Play the following sound file:">
> +<!ENTITY playSound1.accesskey             "d">
> +<!ENTITY playSoundMac.accesskey           "y">

Why the different key for mac?

@@ +22,3 @@
>  <!ENTITY showAnimatedAlert.label          "Show an alert">
>  <!ENTITY showAnimatedAlert.accesskey      "S">
> +<!ENTITY notificationAlertSettings2.label "Default sound and alerts can be disabled on the Notification pane of System Preferences.">

This is confusing because you can read it as 'default (sound and alerts) can...'

How about simply
"Alerts and sounds can be disabled..."
Attachment #8791948 - Flags: review?(richard.marti)
I notice when testing this patch that in my preference pane, "Play the following sound file" is checked by default, but the file name is empty by default. Wouldn't it make sense to have it not checked when the default sound is going to be played?
Attachment #8791948 - Flags: review?(aleth) → review-
(In reply to aleth [:aleth] from comment #28)
> > +<!ENTITY notificationAlertSettings2.label "Default sound and alerts can be disabled on the Notification pane of System Preferences.">
> 
> How about simply
> "Alerts and sounds can be disabled..."

Hmm, probably "Alerts and the default sound can be disabled..." is even clearer.
Comment on attachment 8791948 [details] [diff] [review]
doble.patch

I second aleth's comments.

(In reply to aleth [:aleth] from comment #30)
> (In reply to aleth [:aleth] from comment #28)
> > > +<!ENTITY notificationAlertSettings2.label "Default sound and alerts can be disabled on the Notification pane of System Preferences.">
> > 
> > How about simply
> > "Alerts and sounds can be disabled..."
> 
> Hmm, probably "Alerts and the default sound can be disabled..." is even
> clearer.

This sounds good.
Attachment #8791948 - Flags: review?(richard.marti) → review-
(In reply to aleth [:aleth] from comment #29)
> I notice when testing this patch that in my preference pane, "Play the
> following sound file" is checked by default, but the file name is empty by
> default. Wouldn't it make sense to have it not checked when the default
> sound is going to be played?

Ooooops! I didn't see that when testing it. Sorry. In order to fix it, I have set now the preference to false by default.

Also, "Browse..." button should probably be enabled always. What do you think, :Paenglab? On the following patch I check for the file URL preference and, if it returns false, then I uncheck the "Play following sound" check box and disable it.
Flags: needinfo?(richard.marti)
This patch corrects previous nits by reviewers. Also tries to fix an issue previously undetected of consistency between the file URL being empty and "Play following sound" being checked.
Attachment #8791948 - Attachment is obsolete: true
Flags: needinfo?(richard.marti)
Attachment #8799820 - Flags: ui-review?(richard.marti)
Attachment #8799820 - Flags: review?(mkmelin+mozilla)
Attachment #8799820 - Flags: review?(aleth)
(In reply to Javi Rueda from comment #32)
> (In reply to aleth [:aleth] from comment #29)
> > I notice when testing this patch that in my preference pane, "Play the
> > following sound file" is checked by default, but the file name is empty by
> > default. Wouldn't it make sense to have it not checked when the default
> > sound is going to be played?
> 
> Ooooops! I didn't see that when testing it. Sorry. In order to fix it, I
> have set now the preference to false by default.
> 
> Also, "Browse..." button should probably be enabled always. What do you
> think, :Paenglab? On the following patch I check for the file URL preference
> and, if it returns false, then I uncheck the "Play following sound" check
> box and disable it.

The "Browse..." button is okay how it works now with your patch.
Comment on attachment 8799820 [details] [diff] [review]
Ammended patch with nits from reviewers

Review of attachment 8799820 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/mailnews.js
@@ +664,5 @@
>  pref("mail.biff.alert.preview_length", 40);
>  
> +#ifdef XP_MACOSX
> +pref("mail.biff.play_sound", false);
> +#elif

This has to be #else. Now the build fails at least on Windows.
Attachment #8799820 - Flags: ui-review?(richard.marti) → ui-review+
Attached patch Ammended fix 2 (obsolete) — Splinter Review
Fixes #elif problem for the preprocessor on mailnews.js preferences file.
Attachment #8799820 - Attachment is obsolete: true
Attachment #8799820 - Flags: review?(mkmelin+mozilla)
Attachment #8799820 - Flags: review?(aleth)
Attachment #8799962 - Flags: review?(mkmelin+mozilla)
Attachment #8799962 - Flags: review?(aleth)
Comment on attachment 8799962 [details] [diff] [review]
Ammended fix 2

Review of attachment 8799962 [details] [diff] [review]:
-----------------------------------------------------------------

Is it really the case that in 2016, it's not possible to use mp3 sound files as well here? Please check.

When I select a wav file and click play, I get an error and no sound:
  TypeError: document.getElementById(...) is null 1 general.js:112:21
	gGeneralPane.previewSound chrome://messenger/content/preferences/general.js:112:21
	oncommand about:preferences:1:1

::: mail/components/preferences/general.js
@@ +117,1 @@
>          sound.playEventSound(Components.interfaces.nsISound.EVENT_NEW_MAIL_RECEIVED);

The indentation is wrong here.

@@ +120,1 @@
>        sound.play(Services.io.newURI(soundLocation, null, null));

and here

::: mail/locales/en-US/chrome/messenger/preferences/general.dtd
@@ +22,3 @@
>  <!ENTITY showAnimatedAlert.label          "Show an alert">
>  <!ENTITY showAnimatedAlert.accesskey      "S">
> +<!ENTITY notificationAlertSettings2.label "Alerts and default sound can be disabled on the Notification pane of System Preferences.">

"...the default sound..." makes the meaning clearer in English.
Attachment #8799962 - Flags: review?(mkmelin+mozilla)
Attachment #8799962 - Flags: review?(aleth)
Attachment #8799962 - Flags: review-
I don't know about 2016, but a few years back in my Tonequilla extension, playing mp3 sound files was handled by sending them to the operating system like this, with a launch:

    switch (mimeType)
    {
      case "video/ogg":
      case "audio/ogg":
        that._audioElement = new that.window.Audio(playSpec);
        that._audioElement.autoplay = true;
        that._audioElement.load();
        break;
      case "audio/wav":
      case "audio/x-wav":
        let url = that._nsIIOService
                      .newURI(playSpec, null, null)
                      .QueryInterface(Ci.nsIURL);
        that._nsISound.play(url);
        break;
      default:
        // We're going to blindly let the OS handle this
        nsIFileURL.file
                  .QueryInterface(Ci.nsILocalFile)
                  .launch();
    }
(In reply to aleth [:aleth] from comment #37)
> Comment on attachment 8799962 [details] [diff] [review]
> Ammended fix 2
> 
> Review of attachment 8799962 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is it really the case that in 2016, it's not possible to use mp3 sound files
> as well here? Please check.

That would be another bug report.

> 
> When I select a wav file and click play, I get an error and no sound:
>   TypeError: document.getElementById(...) is null 1 general.js:112:21
> 	gGeneralPane.previewSound
> chrome://messenger/content/preferences/general.js:112:21
> 	oncommand about:preferences:1:1
> 

Yep. That is a regression from my patch. I am going to fix it by checking also for OS X platform when getting the sound file value.

> ::: mail/components/preferences/general.js
> @@ +117,1 @@
> >          sound.playEventSound(Components.interfaces.nsISound.EVENT_NEW_MAIL_RECEIVED);
> 
> The indentation is wrong here.
> 
> @@ +120,1 @@
> >        sound.play(Services.io.newURI(soundLocation, null, null));
> 
> and here
> 
> ::: mail/locales/en-US/chrome/messenger/preferences/general.dtd
> @@ +22,3 @@
> >  <!ENTITY showAnimatedAlert.label          "Show an alert">
> >  <!ENTITY showAnimatedAlert.accesskey      "S">
> > +<!ENTITY notificationAlertSettings2.label "Alerts and default sound can be disabled on the Notification pane of System Preferences.">
> 
> "...the default sound..." makes the meaning clearer in English.

All of these are my fault.
(heartbeat)

I am compiling the patch with some fixes to try to ammend latest comments from reviewers. I will try it when finished.
Attached patch Ammended fix 3Splinter Review
This patch fixes the regression with the play button in macOS. Also provides a corrected description string, the one suggested before posting previous patch but I didn't write correctly.
Attachment #8799962 - Attachment is obsolete: true
Attachment #8804474 - Flags: review?(aleth)
Comment on attachment 8804474 [details] [diff] [review]
Ammended fix 3

Review of attachment 8804474 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks! I'd like Paenglab to give this another spin, too.

>> Is it really the case that in 2016, it's not possible to use mp3 sound files
>> as well here? Please check.
>
>That would be another bug report.

Please file a bug for this. I just added *.mp3 to the appendFilter call in general.js and selecting a mp3 file seemed to work fine, so it looks like there are more extensions that should be included there.

::: mail/components/preferences/general.js
@@ +108,5 @@
>    {
>      let sound = Components.classes["@mozilla.org/sound;1"].createInstance(Components.interfaces.nsISound);
>  
>      var soundLocation;
> +    // soundType radio-group isn't used for macOS so it not in the XUL file for the platform.

Nit: I know this file is really bad at sticking to the 80 character line length limit, but let's not make it worse ;)
Attachment #8804474 - Flags: review?(richard.marti)
Attachment #8804474 - Flags: review?(aleth)
Attachment #8804474 - Flags: review+
Comment on attachment 8804474 [details] [diff] [review]
Ammended fix 3

Looks good. Thank you for the patience.
Attachment #8804474 - Flags: review?(richard.marti) → review+
https://hg.mozilla.org/comm-central/rev/6579cdf860afdd4ce231b78472afdf01a54647cf
Bug 947776 - Notifications in TB on OS X shouldn't play two sounds by default. ui-r=paenglab r=aleth
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 52.0
Depends on: 1313322
This patch seems to have landed in Thunderbird 52.0b1.
I just upgraded from TB 51.0b2 to TB 52.0b1 and immediately noticed that when new messages arrive I neither got my usual new message alert sound nor any other sound. 
In Preferences > General > When new messages arrive > "Play a sound" in previous TB versions I had always activated the radio button "System Alert Sound" which is set to "Glass" in my System Preferences > Sounds.
And since I had unchecked "Play sound when receiving notifications" in System Preferences > Notifications > Thunderbird, I did not encounter the "Thunderbird plays two sounds" issue.

Now I realized that in Preferences > General > "When new messages arrive" both radio buttons have disappeared and been replaced with "Play the following sound file", as shown in attachment 8789393 [details].
So I browsed to choose the "Glass.aiff" sound file in my system library, then set a check-mark at "Play the following sound file". When I hit the "Play" button, I would hear the "glass" sound but still no alert sound when new messages arrived.
Finally I had to open the config editor and change the value for mail.biff.play_sound.type from "0" to "1" to get my usual alert sound work again when new messages arrive.

I am afraid that a lot of Mac users will run into the same issue when they upgrade to the upcoming TB 52 release version, unless they actually already have activated the radio button "Use the following sound file" or will do so before upgrading (which will set mail.biff.play_sound.type to "1"). 
But new Thunderbird (Mac) users won’t be able to customize the alert sound since the hidden preference mail.biff.play_sound.type defaults to "0"!
(In reply to Eckard Berberich from comment #45)
> This patch seems to have landed in Thunderbird 52.0b1.
> In previous TB versions I had always activated the radio button "System Alert
> Sound" which is set to "Glass" in my System Preferences > Sounds.

> And since I had unchecked "Play sound when receiving notifications" in
> System Preferences > Notifications > Thunderbird, I did not encounter the
> "Thunderbird plays two sounds" issue.

This is why people might run into trouble after an update. I'm not sure how many OS X users would have this configuration.
Paenglab, what do you think? Is there a good way to mitigate this during an update?

> Now I realized that in Preferences > General > "When new messages arrive"
> both radio buttons have disappeared and been replaced with "Play the
> following sound file", as shown in attachment 8789393 [details].
> So I browsed to choose the "Glass.aiff" sound file in my system library,
> then set a check-mark at "Play the following sound file". When I hit the
> "Play" button, I would hear the "glass" sound but still no alert sound when
> new messages arrived.

This sounds like a bug :-( mail.biff.play_sound.type should be set to "1" if you set a custom sound file.

> Finally I had to open the config editor and change the value for
> mail.biff.play_sound.type from "0" to "1" to get my usual alert sound work
> again when new messages arrive.

Just to check, if you enabled "Play sound when receiving notifications" in OS X System Preferences > Notifications > Thunderbird instead of what you did, does that also work for you?
Flags: needinfo?(richard.marti)
Flags: needinfo?(leofigueres)
(In reply to aleth [:aleth] from comment #46)
> (In reply to Eckard Berberich from comment #45)
> > This patch seems to have landed in Thunderbird 52.0b1.
> > In previous TB versions I had always activated the radio button "System Alert
> > Sound" which is set to "Glass" in my System Preferences > Sounds.
> 
> > And since I had unchecked "Play sound when receiving notifications" in
> > System Preferences > Notifications > Thunderbird, I did not encounter the
> > "Thunderbird plays two sounds" issue.
> 
> This is why people might run into trouble after an update. I'm not sure how
> many OS X users would have this configuration.
> Paenglab, what do you think? Is there a good way to mitigate this during an
> update?
> 
> > Now I realized that in Preferences > General > "When new messages arrive"
> > both radio buttons have disappeared and been replaced with "Play the
> > following sound file", as shown in attachment 8789393 [details].
> > So I browsed to choose the "Glass.aiff" sound file in my system library,
> > then set a check-mark at "Play the following sound file". When I hit the
> > "Play" button, I would hear the "glass" sound but still no alert sound when
> > new messages arrived.
> 
> This sounds like a bug :-( mail.biff.play_sound.type should be set to "1" if
> you set a custom sound file.
> 


Here we are fighting against the fact that Mozilla doesn't allow us to customize the notification sound been played by the Notification Center. If mail.biff.play_sound.type is set to 1, this means that user wants Thunderbird to play the custom sound. Here, it is expected that the user disables the preference on System Settings in order to avoid what THIS BUG is about: playing two sounds.

On the UI side, user is allowed to set whatever sound he/she would like to be played. And then command Thunderbird to play it, instead of remaining silent (because user has enabled that where it MUST be set up: in System Settings).

It is a very edge case that the user wanted Thunderbird to play the sound which woud normally be played by the operating system.
> > Finally I had to open the config editor and change the value for
> > mail.biff.play_sound.type from "0" to "1" to get my usual alert sound work
> > again when new messages arrive.
> 

That would cause Thunderbird to play the sound which should have been passed to the Notification Center to be played along the alert, instead of having to do it by ourselves and telling user to disable that setting because, if not, then two sounds will be played, which, again that was what this bug is about and which was fixed.

I think that Eckard should file a new bug and flag it as a regression from this one.

And excuse me for my rudeness.
Flags: needinfo?(leofigueres)
(In reply to Javi Rueda from comment #47)
> > This sounds like a bug :-( mail.biff.play_sound.type should be set to "1" if
> > you set a custom sound file.
> > 
> Here we are fighting against the fact that Mozilla doesn't allow us to
> customize the notification sound been played by the Notification Center. If
> mail.biff.play_sound.type is set to 1, this means that user wants
> Thunderbird to play the custom sound. Here, it is expected that the user
> disables the preference on System Settings in order to avoid what THIS BUG
> is about: playing two sounds.
> 
> On the UI side, user is allowed to set whatever sound he/she would like to
> be played. And then command Thunderbird to play it, instead of remaining
> silent (because user has enabled that where it MUST be set up: in System
> Settings).

If I understand correctly, Eckard set up the following config:
- Sounds for TB disabled in System Preferences
- Custom sound file "Glass.aiff" set in TB Preferences
- "Play the following sound file" set in TB Preferences
This seems a valid setup to me. It should work. But Eckard reports it does not work until he
set mail.biff.play_sound.type=1 by hand.
(In reply to aleth [:aleth] from comment #46)
> (In reply to Eckard Berberich from comment #45)
> > This patch seems to have landed in Thunderbird 52.0b1.
> > In previous TB versions I had always activated the radio button "System Alert
> > Sound" which is set to "Glass" in my System Preferences > Sounds.
> 
> > And since I had unchecked "Play sound when receiving notifications" in
> > System Preferences > Notifications > Thunderbird, I did not encounter the
> > "Thunderbird plays two sounds" issue.
> 
> This is why people might run into trouble after an update. I'm not sure how
> many OS X users would have this configuration.
> Paenglab, what do you think? Is there a good way to mitigate this during an
> update?

I'm not deep enough in this code what setting is needed and what needs a change depending a previous setting. But yes, this could be a case for mailMigrator.js.
Flags: needinfo?(richard.marti)
(In reply to aleth [:aleth] from comment #48)
> But Eckard reports it does not work until he
> set mail.biff.play_sound.type=1 by hand.

That pref is set by a radiogroup which is ifdef'd out on OS X after this patch. I don't see any other places in the code that set it. It is not set after the STR of comment 48, but it should be so that the custom sound actually gets played. Am I missing something?
Ok. I just have filed bug 1337071. This could likely have been my fault. Sorry about that. I CC'd Eckard.
(In reply to aleth [:aleth] from comment #48)

> If I understand correctly, Eckard set up the following config:
> - Sounds for TB disabled in System Preferences
> - Custom sound file "Glass.aiff" set in TB Preferences
> - "Play the following sound file" set in TB Preferences
> This seems a valid setup to me. It should work. But Eckard reports it does
> not work until he
> set mail.biff.play_sound.type=1 by hand.

My TB-Preferences > General > "When new messages arrive" setup in the previous version (TB 51.0b2) was:
"√Play a sound" activated
Radio-button "System Alert Sound" activated
As system alert sound I'd chosen "Glass" in System Preferences > Sounds.
When new messages arrived I heard the "Glass" sound.
After the update to TB 52.0b1 there was no sound at all.

I just did a new test.
Installed Earlybird 53.0a2, created a new profile, set up a IMAP account, opened TB > Preferences > General, chose the "Glass" file via "Browse..." and activated "√Play the following sound file".
Sent messages from another TB instance to the IMAP address. When these messages arrived in the Earlybird 53.0a2 profile, no sound was played. When I activated "Play sound when receiving notifications" in System Preferences > Notifications > Earlybird, I heard the standard macOS notification sound which I don't want.
Opened the config editor and saw that mail.biff.play_sound.type had the value "0" (see attached screen shot). Changed its value to "1", then new messages played both my custom sound and the standard macOS notification sound.
Disabled the standard macOS notification sound again in System Preferences > Notifications and now I'm hearing only my custom sound as desired. 

Maybe the value of mail.biff.play_sound.type should default to "1" (instead of "0") in the future TB 52 release?
Please, Eckard, comment in bug 1337071. This bug is closed now.
Depends on: 1337071
(In reply to Richard Marti (:Paenglab) from comment #49)
> > Paenglab, what do you think? Is there a good way to mitigate this during an
> > update?
> 
> I'm not deep enough in this code what setting is needed and what needs a
> change depending a previous setting. But yes, this could be a case for
> mailMigrator.js.

I don't think there's a way to automate this as you can't easily read out the OS preferences the user has set. However, we should add a helpful release note after the regression fixes land.
Depends on: 1338122
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: