328483 - Lightning: Play sound on alarm does not work

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect)

Description

[Stefan Sitter [:ssitter]]

Lightning: Play sound on alarm does not work.

1) Enable Alarm -> Play a sound in Lightning option dialog. Restart Thunderbird.

2) Create an event with alarm and wait until alarm dialog appears on screen

--> No sound is played. Console displays:

unable to play sound...
[Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getCharPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: chrome://calendar/content/calendar-alarm-dialog.js :: addAlarm :: line 58"  data: no]

3) Press 'Dismiss' button in alarm window:

--> Error in JavaScript Console:

  Warning: assignment to undeclared variable item
  Source: chrome://calendar/content/calendar-alarm-dialog.js Line: 79

  Warning: reference to undefined property kid.item
  Source: chrome://calendar/content/calendar-alarm-dialog.js Line: 79

  Error: kid.item has no properties
  Source: chrome://calendar/content/calendar-alarm-dialog.js Line: 79

Note:
This might be a problem with the existing alarm preferences in Lightning (all):
  calendar.alarms.defaultsnoozelength=5
  calendar.alarms.playfile=
  calendar.alarms.playsound=true

In Sunbird I have the following preferences (only showing the important ones):
  calendar.alarms.soundURL=chrome://calendar/content/sound.wav
  calendar.alarms.playsound=true

Tested with Tb 1.5 + Lightning (win32-2006-02-24-mozilla1.8).

Comment 1

[Stefan Sitter [:ssitter]]

After setting the preference calendar.alarms.soundURL manually and adding sound.wav to calendar.jar file the alarm sound worked.

So we just have to made a patch doing the same.

The issue mentioned in 3) seems not related to this problem but is a separate one.

Comment 2

[Stefan Sitter [:ssitter]]

Attachment: make alarm sound work

This patch corrects the preferences for lightning and adds the existing alarm sound file to xpi.

Comment 3

[Joey Minta]

Comment on attachment 213133 [details] [diff] [review]
make alarm sound work

I think I'd rather see us use beep() http://www.xulplanet.com/references/xpcomref/ifaces/nsISound.html#method_beep since that avoids the download size and will match better with the user's OS.  (My system 'beep' is very different than sound.wav)  If we're going to do sound.wav, then I'd really like to see
-<!--                <filewidget flex="1" preference="calendar.alarms.playfile" preference-editable="true"/> -->
+<!--                <filewidget flex="1" preference="calendar.alarms.soundURL" preference-editable="true"/> -->
fixed, so that users can get away from that sound when they want.

Comment 4

[Stefan Sitter [:ssitter]]

Attachment: fix lightning preferences

Minimal patch fixing Lightning preferences to use the right one. Patch does not add UI to select the sound file to play. Users can do this manually using 'Config Editor...' button in Thunderbird preferences dialog (aka about:config).

Comment 5

[Stefan Sitter [:ssitter]]

Attachment: optional: use beep() fallback

Optional patch: 
If an alarm sound should be played but no sound url is specified we use beep() as fallback solution.

Note on the patch: makeURL() throws an exception when called with an empty string. So it is now called after the result of getCharPref is checked. Sound.init() is called in the if and else branch. I did that on purpose so we call it only if we are sure that nothing else can fail before.

Comment 6

[Joey Minta]

Comment on attachment 213169 [details] [diff] [review]
fix lightning preferences

I'm actually pushing for an RC in the next 24-48 hours, so I'm not going to take anything non-critical at this stage.  Is it possible to get a patch with the filepicker part fixed?  If not, I'll review this and keep the bug open, but having a filepicker would be the ideal fix.

Comment 7

[Joey Minta]

Comment on attachment 213170 [details] [diff] [review]
optional: use beep() fallback

+       if (soundURL & soundURL.length & soundURL.length > 0) {
+         soundURL = makeURL(soundURL);
+         sound.init();
+         sound.play(soundURL);
+       } else {
+         sound.init();
+         sound.beep();
+       }
If sound.init() is in both blocks and doesn't need to happen in that particular order, you can probably pull it out in front. r=jminta with that, but this still won't go for 0.1.

Comment 8

[Stefan Sitter [:ssitter]]

(In reply to comment #6)
I thought fixing the prefs is sufficient and low risk for Lightning 0.1. See my comment #4. I already filed Bug 328560 to add the UI yesterday.

(In reply to comment #7)
I did that on purpose so we call sound.init() only if we are sure that nothing else failed before. But I can subit a new iteration of that patch.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Joey: I think taking this before 0.1 would be reasonable.  It seems reasonably low-risk, and some people might miss meetings since they'd reasonably expect this might work.

Comment 10

[Joey Minta]

Comment on attachment 213169 [details] [diff] [review]
fix lightning preferences

Ok, I'll bite. r=jminta if you file a bug about the filepicker.

Comment 11

[Joey Minta]

Patches checked in.  Leaving bug open for follow-up filings.

Comment 12

[Stefan Sitter [:ssitter]]

(In reply to comment #11)
> Patches checked in.  Leaving bug open for follow-up filings.

Bug 328560 is already filed. Is there anything else that has to be filed?

Comment 13

[Joey Minta]

(In reply to comment #12)
> (In reply to comment #11)
> > Patches checked in.  Leaving bug open for follow-up filings.
> 
> Bug 328560 is already filed. Is there anything else that has to be filed?
> 
Ahh... that's what I get for not reading carefully!  Nope, nothing else.  We're done here.