1337071 - MailNews will not play any sound if sound_type == 0

Status: VERIFIED FIXED

(Thunderbird :: OS Integration, defect)

Description

[Javi Rueda :javirid]

Very likely a regression from bug 947776. Patch was mine.

Sound will not be played on MacOS if:

a) Play sound is disabled on System Preferences
b) Custom sound file setup in Preferences
c) mail.biff.play_sound.type = 0

Likely cause is in method nsStatusBarBiffManager::PlayBiffSound in  comm-beta/mailnews/base/src/nsStatusBarBiffManager.cpp

Comment 1

[Javi Rueda :javirid]

A fix is under way right now :)

I am building it on my machine.

Sorry for the inconvenience and thank you to Eckard for spotting this bug.

Comment 2

[aleth [:aleth]]

[Tracking Requested - why for this release]:
regression

Comment 3

[Eckard Berberich]

Sorry for my new comment in bug 947776.
Thank you for handling this issue, Javi.

Comment 4

[Javi Rueda :javirid]

Attachment: Patch deprecating preference on macOS

This patch fixes the problem by deprecating finally the preference play_sound.type which was not in use on this platform. Someone from the Suite team should also take a look into this one, also

Comment 5

[Julien Cristau [:jcristau] (back April 22)]

Doesn't seem to affect firefox, resetting tracking flags.

Comment 6

[Richard Marti (:Paenglab)]

[Tracking Requested - why for this release]:
regression

Comment 7

[aleth [:aleth]]

Comment on attachment 8834192 [details] [diff] [review]
Patch deprecating preference on macOS

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

I think this approach is reasonable, though it would work as well to just set sound_type for OS X in the frontend as appropriate. You should choose depending on what you think will happen if/when there is the option of interfacing better with the system prefs (iirc there was a bug for that you were looking at some time ago), so it's future-proof.

But looking at where the pref is used, I discovered that there are actually three pref branches using this code path:
http://searchfox.org/comm-central/source/mailnews/base/src/nsStatusBarBiffManager.cpp#42

So there's also mail.chat.play_sound* and mail.feed.play_sound*. You'll have to do the equivalent of bug 947776 for the Chat pref tab (probably a good idea for consistency, and thankfully the strings are likely the same). I don't know if there's an UI for feed sounds, it's possible there's just the prefs for power users, but that needs checking too ;)

Comment 8

[Javi Rueda :javirid]

Ok. I started on this bug because it looked to me a easy fix. But now that it seems it ios going to need extra work, I will be working again in bug 1229684 which has a higher priority, I think.

I will follow here as soon as possible.

Comment 9

[aleth [:aleth]]

(In reply to Javi Rueda from comment #8)
> Ok. I started on this bug because it looked to me a easy fix. But now that
> it seems it ios going to need extra work, I will be working again in bug
> 1229684 which has a higher priority, I think.

Actually, unless that bug also affects TB 52, which is already in beta and will be the next release, this one is probably more urgent.

Comment 10

[Eckard Berberich]

It does affect TB 52 beta.
See my comment 45 in bug 947776.

Comment 11

[aleth [:aleth]]

Attachment: Keep mail.biff.play_sound.type set properly on OS X

Comment 12

[aleth [:aleth]]

Comment on attachment 8835052 [details] [diff] [review]
Keep mail.biff.play_sound.type set properly on OS X

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

Here's a quick minimal solution that keeps the pref in sync on OS X. This should involve less special casing in the code.

We should still port the new UI to the chat pref pane of course, but in a separate bug.

Comment 13

[aleth [:aleth]]

Attachment: Keep mail.biff.play_sound.type set properly on OS X

Slight simplification that also doesn't change the behaviour of the 'play' button, which is always enabled on OS X if a sound URL is present.

Comment 14

[Richard Marti (:Paenglab)]

Comment on attachment 8835061 [details] [diff] [review]
Keep mail.biff.play_sound.type set properly on OS X

I only tested this on a Daily without patch with checkmark enabled and a sound file selected. mail.biff.play_sound.type was on 0. After this I applied the patch and it was still on 0. Only after I unchecked/checked the play file checkmark mail.biff.play_sound.type changed to 1.

Now, is this normally on TB 45 on 1 and it works after upgrade? Or do we need some migration code?

Comment 15

[Javi Rueda :javirid]

Comment on attachment 8835061 [details] [diff] [review]
Keep mail.biff.play_sound.type set properly on OS X

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

This *patch* does its job avoiding the modification of backend core files. Hiding UI is not a fix, however.

Anyway, your patch works in a simpler way than mine, so r+ if the point in line 165 is added :)

::: mail/components/preferences/general.js
@@ +161,5 @@
>  
>    updatePlaySound: function()
>    {
> +    // update the sound type radio buttons based on the state of the
> +    // play sound checkbox

Add a point at the end of the sentence, like you did in line 171.

Comment 16

[aleth [:aleth]]

(In reply to Javi Rueda from comment #15)
> This *patch* does its job avoiding the modification of backend core files.
> Hiding UI is not a fix, however.

Yes, it's not ideal, but we can safely uplift it to beta without having to worry about potential side-effects due to backend changes.

> Anyway, your patch works in a simpler way than mine, so r+ if the point in
> line 165 is added :)

Ah, thanks :-)

Comment 17

[Javi Rueda :javirid]

(In reply to Richard Marti (:Paenglab) from comment #14)

> I only tested this on a Daily without patch with checkmark enabled and a
> sound file selected. mail.biff.play_sound.type was on 0. After this I
> applied the patch and it was still on 0. Only after I unchecked/checked the
> play file checkmark mail.biff.play_sound.type changed to 1.
> 

Oh! Yes. That was my first approach in a previous bug. If this could be done without messing the core files, it is better, but... it didn't worked. Also, backend code should do its job in a more abstract way: let it decide which sound -if any- should be played.

So, I changed my mind about my (first for Mozilla) review.

Comment 18

[Javi Rueda :javirid]

Comment on attachment 8835061 [details] [diff] [review]
Keep mail.biff.play_sound.type set properly on OS X

I have just said that I am going to cancel my grant. But, at the end, I found it unfair, as it works excepting the fact that: an action from the user (checking or unchecking the "Play sound" checkbox) is needed and I still don't found this bug an urgent one. Maybe a "Known issues".

Keeping my r+ :)

Comment 19

[aleth [:aleth]]

(In reply to Richard Marti (:Paenglab) from comment #14)
> I only tested this on a Daily without patch with checkmark enabled and a
> sound file selected. mail.biff.play_sound.type was on 0. After this I
> applied the patch and it was still on 0. Only after I unchecked/checked the
> play file checkmark mail.biff.play_sound.type changed to 1.
> 
> Now, is this normally on TB 45 on 1 and it works after upgrade? Or do we
> need some migration code?

The default value of the pref is 0 and it will only be 1 for users of 45 who selected the "custom sound file" option. After updating, people with a custom sound file will experience no change. 

People with "don't play any sound" will hear whatever they had set in OS preferences (i.e. no change for them)

People with "play system sound" (pref value 0) and disabled OS notification sounds will end up in Eckard's situation and experience a change in behaviour as they will hear nothing. They'll then either have to set a custom sound or enable the sound in OS preferences. 

It's the third group for which we should add a release note. I wonder what would be a good way to word this? There is no migration code I can think of that will help them as we can't turn on the OS setting automatically for them (yet?).

Comment 20

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #19)
> People with "play system sound" (pref value 0) and disabled OS notification
> sounds will end up in Eckard's situation and experience a change in
> behaviour as they will hear nothing. They'll then either have to set a
> custom sound or enable the sound in OS preferences. 

Hmm, reading comment 18, I see that what it confusing here is that they end up in a state with checked "Play sound" box but no sound playing. So if we wanted we could migrate this case as follows

.play_sound == true && .play_sound.type == 0  -> .play_sound=false

ie. uncheck the box.

Comment 21

[aleth [:aleth]]

Attachment: Keep mail.biff.play_sound.type set properly on OS X

Migration added as described in comment 20. Please check my reasoning...

Comment 22

[Javi Rueda :javirid]

I didn't notice that :aleth took the bug.

I believed that that behavior was only to be seen from long time Mozilla'employees. Now I see that it is not the case anymore.

I could agree (in fact, I don't agree and I will not) from the urgency this bug might have. I don't have the full scope vision, so it might be.

However, taking someone else's bug without telling about it is not very respectful. In other moments, the person just took it and told the original person the reason of it.

I am not a good coder. In fact, from time to time there are regressions from my patches. I will always say that it is my fault, when it is the case, of course. But this is an open-source project and that is the way it works, thankfully.

Comment 23

[aleth [:aleth]]

(In reply to Javi Rueda from comment #22)
> I didn't notice that :aleth took the bug.
> 
> I believed that that behavior was only to be seen from long time
> Mozilla'employees. Now I see that it is not the case anymore.

I am sorry this happened. Actually, it was not intentional at all, but the result of the hg bzexport extension which unfortunately defaults to taking a bug when you upload a patch, unless you remember to add the --no-take-bug option. There's an open bug about this behaviour...

Comment 24

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #23)
> There's an open bug about this behaviour...

Bug 1239457 fwiw...

Comment 25

[Javi Rueda :javirid]

(In reply to aleth [:aleth] from comment #23)
> (In reply to Javi Rueda from comment #22)
> > I didn't notice that :aleth took the bug.
> > 
> > I believed that that behavior was only to be seen from long time
> > Mozilla'employees. Now I see that it is not the case anymore.
> 
> I am sorry this happened. Actually, it was not intentional at all, but the
> result of the hg bzexport extension which unfortunately defaults to taking a
> bug when you upload a patch, unless you remember to add the --no-take-bug
> option. There's an open bug about this behaviour...

Well. I don't mind if you fix the bug, as it seems it will be. I have said it before, that your fix looks better to me. As it seems simpler.

What annoyed me was the way that this bug was taken. But now I have understood the reason. Sorry about some of my previous comments if they disturbed you, :aleth.

Comment 26

[Richard Marti (:Paenglab)]

Comment on attachment 8835158 [details] [diff] [review]
Keep mail.biff.play_sound.type set properly on OS X

Yeah, I think this is the way we need to do.

Comment 27

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/c585bb3bb888eb6bc051b244f05766e7384831a1
Bug 1337071 - Keep mail.biff.play_sound.type set properly on OS X. r=paenglab,jrueda

Comment 28

[aleth [:aleth]]

Comment on attachment 8835158 [details] [diff] [review]
Keep mail.biff.play_sound.type set properly on OS X

[Approval Request Comment]
Regression caused by (bug #): 947776
User impact if declined: Based on beta tester feedback, a confusing update experience and UI/prefs not in sync.
Testing completed (on c-c, etc.): Give it a day or two on c-c before uplifting
Risk to taking this patch (and alternatives if risky): low

Comment 29

[Jorg K (CEST = GMT+2)]

Aurora (TB 53):
https://hg.mozilla.org/releases/comm-aurora/rev/2111adc15881107f57243e7d2ab1b23cdc1f2aa8

Comment 30

[Jorg K (CEST = GMT+2)]

I have a problem uplifting this to TB 52 beta since the UI version in beta is at 13:
https://dxr.mozilla.org/comm-beta/rev/7ee416445f296f50e82c220ba667574fcb421ebd/mail/base/modules/mailMigrator.js#105

So the patch won't apply. The deeper issue is of course that another migration is missing in TB 52 beta, that is the one from bug 1330640 which pushed from 13 to 14 which can be seen in Aurora and Trunk.

I cannot go straight from 13 to 15 since then the migration from bug 1330640 will never be done and that one is important.

To there are four options here:
1) Back out bug 947776 from beta.
2) Not uplift this bug to beta.
3) Uplift bug 1330640 and bug 1329494 to beta and then uplift this one.
4) Uplift this bug but lose the migration so the UI version stays at 13.
   But your comment says: Upgrade those with versions < 52.

So what do I do?

Comment 31

[Javi Rueda :javirid]

(In reply to Jorg K (GMT+1) from comment #30)
> I have a problem uplifting this to TB 52 beta since the UI version in beta
> is at 13:
> https://dxr.mozilla.org/comm-beta/rev/
> 7ee416445f296f50e82c220ba667574fcb421ebd/mail/base/modules/mailMigrator.
> js#105
> 
> So the patch won't apply. The deeper issue is of course that another
> migration is missing in TB 52 beta, that is the one from bug 1330640 which
> pushed from 13 to 14 which can be seen in Aurora and Trunk.
> 
> I cannot go straight from 13 to 15 since then the migration from bug 1330640
> will never be done and that one is important.
> 
> To there are four options here:
> 1) Back out bug 947776 from beta.

+1

> 2) Not uplift this bug to beta.

Bug 947776 is still not in release and bug 1337021 -this one- fixes a problem introduced in 947776. I think that, if we back-out 947776, as it has still not been released to the public in Release, would be fine if both of them are not uplifted and goes together in Aurora.

> 3) Uplift bug 1330640 and bug 1329494 to beta and then uplift this one.

-1

Those are unrelated to these ones, so I think they should follow their own way into release.

> 4) Uplift this bug but lose the migration so the UI version stays at 13.
>    But your comment says: Upgrade those with versions < 52.

-1

Comment 32

[Jorg K (CEST = GMT+2)]

(In reply to Javi Rueda from comment #31)
> > 1) Back out bug 947776 from beta.
> +1

> > 3) Uplift bug 1330640 and bug 1329494 to beta and then uplift this one.
> -1

3) was my favourite option. These are only minor bugs and the sooner we get the conversion done, the better.

Backing out bug 947776 from beta would unfix "Notifications in Thunderbird play two sounds". How bad is that?

Comment 33

[Javi Rueda :javirid]

Bug 947776 tried to fix a bad behavior from Thunderbird on macOS. User could improve the experience by disabling playing a sound on the Notification Center settings in System Settings. The fix for that bug tried to do things the way it should be: Thunderbird should not play any sound and let operating system play it instead.

If fix for bug 947776 doesn't land in Beta, then user had to disable sound on the System Settings to avoid two sounds being played. So it is not a problem to fix that from Aurora instead of from Beta.

About uplifting those unrelated bugs, I don't know the rules about what can be uplifted and what not. Also, I don't know what is changing those patches, but I think that any patch should be tested in their channels the most time it is needed.

We tried to uplift this bug as it is fixing a regression introduced in 947776, so uplift could be ok. However, now we have that problem about the UI version. As 947776+1337071 are not in a hurry and 1330640+1329494 is neither, then I think that backing out 947776 and landing it -along this one- in aurora instead is a better solution.

Comment 34

[Jorg K (CEST = GMT+2)]

Thanks for your reply. I will let Aleth make the decision.

Bug 947776 is in TB 52 (Beta) and TB 53 (Aurora). Bug 1337071 (this bug) is also in Aurora (comment #29). If I back out the former from TB 52, both bugs we hit the user in TB 53. Only users which have been using TB 52 beta might get confused.

Comment 35

[Jorg K (CEST = GMT+2)]

Comment on attachment 8835158 [details] [diff] [review]
Keep mail.biff.play_sound.type set properly on OS X

OK, as discussed with Javi: No uplift to TB 52 beta. Instead backout of bug 947776.

Comment 36

[Jorg K (CEST = GMT+2)]

Comment on attachment 8835158 [details] [diff] [review]
Keep mail.biff.play_sound.type set properly on OS X

Bug 947776 can't be backed out since it has strings, and plenty of them. So there is not way backwards, it can only go forward.

Comment 37

[Jorg K (CEST = GMT+2)]

Beta (TB 52):
https://hg.mozilla.org/releases/comm-beta/rev/4508615d66d8ebb573e3121f0b1d31fb315a91b3

Comment 38

[Wayne Mery (:wsmwk)]

Is comment 0 the full steps, and is there more extensive testing protocol we should ask of testers?

Comment 39

[Javi Rueda :javirid]

STR (for QA testing)
--------------------
0. With blank profile (default preferences settings) in a macOS system with Sounds disabled on the Notification Center settings.
1. Open Preferences
2. On the bottom of the pane, click on Browse and choose an audio file
3. Check "Play the following sound file:"

EXpected
--------
When a new mail arrives, selected file should be played

Results
-------
No sound played

Setting mail.biff.play_sound.type to 1 in about:config makes the sound file to be played as expected.

Comment 40

[aleth [:aleth]]

(In reply to Javi Rueda from comment #39)
> STR (for QA testing)
> --------------------
> 0. With blank profile (default preferences settings) in a macOS system with
> Sounds disabled on the Notification Center settings.
> 1. Open Preferences
> 2. On the bottom of the pane, click on Browse and choose an audio file
> 3. Check "Play the following sound file:"
> 
> EXpected
> --------
> When a new mail arrives, selected file should be played
> 
> Results
> -------
> No sound played
> 
> Setting mail.biff.play_sound.type to 1 in about:config makes the sound file
> to be played as expected.

Since both patches were uplifted (comment 37) and I added some pref migration, hopefully things should 'just work' for testers (i.e. no about:config tricks necessary). They *may* find no notification sounds being played after updating, but a quick glance at the preferences should provide the correct information (i.e. either go to the OS notification center to enable them, or set a custom sound).

Comment 41

[Wayne Mery (:wsmwk)]

FWIW we did have a couple beta testers report good results
https://public.etherpad-mozilla.org/p/thunderbird-release-52.0b3

Comment 42

[aleth [:aleth]]

Excellent!