352674 - Set filter on alarm sound file picker

Status: VERIFIED FIXED

(Calendar :: Alarms, defect)

Description

[Thorsten 'nightrat' Fritz]

The filter for alarm sounds is set to "All Files".

On Linux i can't play mp3 or ogg files as an alarm sound.. So we should set this filter to specific file types depending on the OS.

Comment 1

[Adam Guthrie]

Attachment: Untested patch

Comment 2

[Adam Guthrie]

Make that |#define UNIX_BUT_NOT_MAC|, actually. :)

Comment 3

[Joey Minta]

Comment on attachment 238455 [details] [diff] [review]
Untested patch

I'm a little concerned here about special-casing linux like this.  Are there any docs somewhere that assure us that win/mac can handle all (or nearly all) sounds files?

Comment 4

[Adam Guthrie]

Well, on Linux we /only/ support WAV files explicitly. On Windows and Mac we pass the sound off to system APIs that try and play it.

Not sure how this works on Mac, but on Windows we tell it to just play a default system sound if it can't play the sound we tell it to.

Comment 5

[Stefan Sitter [:ssitter]]

Seems like using *.mp3 for alarm sound doesn't work on Windows 2000 either.

Comment 6

[Matthew (lilmatt) Willis]

(In reply to comment #5)
> Seems like using *.mp3 for alarm sound doesn't work on Windows 2000 either.

FWIW: it does on Mac.

Comment 7

[Adam Guthrie]

Sorry, we tell Windows /not/ to play a default sound if the sound we pass in fails. Also, now that I look at it, Windows can only play WAVs, too. :\

Comment 8

[Adam Guthrie]

Attachment: Filter for only WAVs on all platforms

As discussed with jminta on IRC, just filter for WAVs on all platforms for consistency. If someone wants to use an MP3 or other format, they can change it in about:config.

Also, I would've liked to have used "WAV Files" as a description instead of just "*.wavs", but since we're in a string freeze, this isn't possible. :(

Comment 9

[Stefan Sitter [:ssitter]]

Comment on attachment 238741 [details] [diff] [review]
Filter for only WAVs on all platforms

>-        fp.init(window, title, nsIFilePicker.modeOpen);
>+        fp.appendFilter("*.wav", "*.wav");

I think the init() is still required and appendFilter() should just replace existing appendFilters().

Comment 10

[Matthew (lilmatt) Willis]

Comment on attachment 238741 [details] [diff] [review]
Filter for only WAVs on all platforms

>Index: calendar/base/content/preferences/alarms.js
>===================================================================
>--- calendar/base/content/preferences/alarms.js	19 Jun 2006 18:29:53 -0000	1.1
We're at rev 1.2 after your contribution for bug 344535.
That makes this patch a bit screwy.

>         var title = bundlePreferences.getString("Open");
>-        fp.init(window, title, nsIFilePicker.modeOpen);
>+        fp.appendFilter("*.wav", "*.wav");
> 
>         var ret = fp.show();
As ssitter said, we should leave the fp.init() and simply replace fp.appendFilters() (which is only in rev 1.2)

Please also file a follow-up to fix the string for after the string freeze.

Comment 11

[Adam Guthrie]

Attachment: Filter for only WAVs on all platforms v1.1

Arg, I really didn't mean to do that, I swear. I guess this is what I get for making patches this late at night.

Comment 12

[Matthew (lilmatt) Willis]

Comment on attachment 238849 [details] [diff] [review]
Filter for only WAVs on all platforms v1.1

>Index: calendar/base/content/preferences/alarms.js
>===================================================================
>         var bundlePreferences = document.getElementById("bundlePreferences");
>         var title = bundlePreferences.getString("Open");
>-        fp.appendFilters(nsIFilePicker.filterAll);
>+        fp.appendFilter("*.wav", "*.wav");
>         fp.init(window, title, nsIFilePicker.modeOpen);

Looking at other code in lxr, it looks like the "proper order" is:

         fp.init(window, title, nsIFilePicker.modeOpen);
         fp.appendFilter("*.wav", "*.wav");
         fp.appendFilters(nsIFilePicker.filterAll);


http://lxr.mozilla.org/mozilla/search?string=appendFilter


Here's the rub:
The filepicker's not respecting this filter on Mac, and not just for this dialog.


So... r=lilmatt using the above order.

Comment 13

[Joey Minta]

Comment on attachment 238849 [details] [diff] [review]
Filter for only WAVs on all platforms v1.1

r2=jminta with the ordering tweak mentioned by lilmatt.

Comment 14

[Adam Guthrie]

Attachment: Filter for WAVs and All Files, but default to WAVs

Comment 15

[Adam Guthrie]

I filed bug 353437 for localizing the filter string.

Comment 16

[Matthew (lilmatt) Willis]

Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED

Comment 17

[Thorsten 'nightrat' Fritz]

Verified with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060921 Calendar/0.3a2+

Comment 18

[Damian Szczepanik]

Litmus testcase 2888 created